| Message ID | 20201221164819.792019-10-ribalda@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hi Ricardo, Thank you for the patch. On Mon, Dec 21, 2020 at 05:48:16PM +0100, Ricardo Ribalda wrote: > Some devices, can only read the privacy_pin if the device is s/devices,/devices/ > streaming. > > This patch implement a quirk for such devices, in order to avoid invalid > reads and/or spurious events. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_driver.c | 57 ++++++++++++++++++++++++++++-- > drivers/media/usb/uvc/uvc_queue.c | 3 ++ > drivers/media/usb/uvc/uvcvideo.h | 4 +++ > 3 files changed, 61 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 72516101fdd0..7af37d4bd60a 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -7,6 +7,7 @@ > */ > > #include <linux/atomic.h> > +#include <linux/dmi.h> > #include <linux/gpio/consumer.h> > #include <linux/kernel.h> > #include <linux/list.h> > @@ -1472,6 +1473,17 @@ static int uvc_parse_control(struct uvc_device *dev) > /* ----------------------------------------------------------------------------- > * Privacy GPIO > */ There should be a blank line here. > +static bool uvc_gpio_is_streaming(struct uvc_device *dev) > +{ > + struct uvc_streaming *streaming; > + > + list_for_each_entry(streaming, &dev->streams, list) { > + if (uvc_queue_streaming(&streaming->queue)) > + return true; > + } > + > + return false; > +} > > But not too blank lines here. > static u8 uvc_gpio_update_value(struct uvc_device *dev, > @@ -1499,7 +1511,12 @@ static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity, > if (cs != UVC_CT_PRIVACY_CONTROL || size < 1) > return -EINVAL; > > + if ((dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) && > + !uvc_gpio_is_streaming(dev)) > + return -EBUSY; > + > *(uint8_t *)data = uvc_gpio_update_value(dev, entity); > + > return 0; > } > > @@ -1528,19 +1545,50 @@ static struct uvc_entity *uvc_gpio_find_entity(struct uvc_device *dev) > return NULL; > } > > -static irqreturn_t uvc_gpio_irq(int irq, void *data) > +void uvc_privacy_gpio_event(struct uvc_device *dev) > { > - struct uvc_device *dev = data; > struct uvc_entity *unit; > > + > unit = uvc_gpio_find_entity(dev); > if (!unit) > - return IRQ_HANDLED; > + return; > > uvc_gpio_update_value(dev, unit); > +} > + > +static irqreturn_t uvc_gpio_irq(int irq, void *data) > +{ > + struct uvc_device *dev = data; > + > + /* Ignore privacy events during streamoff */ > + if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) > + if (!uvc_gpio_is_streaming(dev)) > + return IRQ_HANDLED; I'm still a bit concerned of race conditions. When stopping the stream, vb2_queue.streaming is set to 0 after calling the driver's .stop_stream() handler. This means that the device will cut power before uvc_gpio_is_streaming() can detect that streaming has stopped, and the GPIO could thus trigger an IRQ. You mentioned that devices have a pull-up or pull-down on the GPIO line. As there are only two devices affected, do you know if it's a pull-up or pull-down ? Would it be worse to expose that state to userspace than to return -EBUSY when reading the control ? > + > + uvc_privacy_gpio_event(dev); > + > return IRQ_HANDLED; > } > > +static const struct dmi_system_id privacy_valid_during_streamon[] = { > + { > + .ident = "HP Elite c1030 Chromebook", > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > + DMI_MATCH(DMI_PRODUCT_NAME, "Jinlon"), > + }, > + }, > + { > + .ident = "HP Pro c640 Chromebook", > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > + DMI_MATCH(DMI_PRODUCT_NAME, "Dratini"), > + }, > + }, > + { } /* terminate list */ > +}; > + > static int uvc_gpio_parse(struct uvc_device *dev) > { > struct uvc_entity *unit; > @@ -1577,6 +1625,9 @@ static int uvc_gpio_parse(struct uvc_device *dev) > > list_add_tail(&unit->list, &dev->entities); > > + if (dmi_check_system(privacy_valid_during_streamon)) > + dev->quirks |= UVC_QUIRK_PRIVACY_DURING_STREAM; This will also match any external UVC camera plugged to one of the affected systems, right ? It shouldn't matter in practice as those devices won't have a GPIO entity. I suppose we can't match on VID:PID instead because the same VID:PID is used in both devices affected by this issue, and devices immune to it ? > + > return 0; > } > > diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c > index cd60c6c1749e..e800d491303f 100644 > --- a/drivers/media/usb/uvc/uvc_queue.c > +++ b/drivers/media/usb/uvc/uvc_queue.c > @@ -337,9 +337,12 @@ int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf, > int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type) > { > int ret; > + struct uvc_streaming *stream = uvc_queue_to_stream(queue); Please swap the two lines. > > mutex_lock(&queue->mutex); > ret = vb2_streamon(&queue->queue, type); > + if (stream->dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) > + uvc_privacy_gpio_event(stream->dev); Even when vb2_streamon() failed ? > mutex_unlock(&queue->mutex); > > return ret; > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index 079a407ebba5..32c1ba246d97 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -209,6 +209,7 @@ > #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400 > #define UVC_QUIRK_FORCE_Y8 0x00000800 > #define UVC_QUIRK_FORCE_BPP 0x00001000 > +#define UVC_QUIRK_PRIVACY_DURING_STREAM 0x00002000 > > /* Format flags */ > #define UVC_FMT_FLAG_COMPRESSED 0x00000001 > @@ -826,6 +827,9 @@ extern const struct v4l2_file_operations uvc_fops; > int uvc_mc_register_entities(struct uvc_video_chain *chain); > void uvc_mc_cleanup_entity(struct uvc_entity *entity); > > +/* Privacy gpio */ > +void uvc_privacy_gpio_event(struct uvc_device *dev); > + > /* Video */ > int uvc_video_init(struct uvc_streaming *stream); > int uvc_video_suspend(struct uvc_streaming *stream); -- Regards, Laurent Pinchart
HI Laurent Thanks for your review! On Tue, Dec 22, 2020 at 11:30 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Mon, Dec 21, 2020 at 05:48:16PM +0100, Ricardo Ribalda wrote: > > Some devices, can only read the privacy_pin if the device is > > s/devices,/devices/ > > > streaming. > > > > This patch implement a quirk for such devices, in order to avoid invalid > > reads and/or spurious events. > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_driver.c | 57 ++++++++++++++++++++++++++++-- > > drivers/media/usb/uvc/uvc_queue.c | 3 ++ > > drivers/media/usb/uvc/uvcvideo.h | 4 +++ > > 3 files changed, 61 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > > index 72516101fdd0..7af37d4bd60a 100644 > > --- a/drivers/media/usb/uvc/uvc_driver.c > > +++ b/drivers/media/usb/uvc/uvc_driver.c > > @@ -7,6 +7,7 @@ > > */ > > > > #include <linux/atomic.h> > > +#include <linux/dmi.h> > > #include <linux/gpio/consumer.h> > > #include <linux/kernel.h> > > #include <linux/list.h> > > @@ -1472,6 +1473,17 @@ static int uvc_parse_control(struct uvc_device *dev) > > /* ----------------------------------------------------------------------------- > > * Privacy GPIO > > */ > > There should be a blank line here. > > > +static bool uvc_gpio_is_streaming(struct uvc_device *dev) > > +{ > > + struct uvc_streaming *streaming; > > + > > + list_for_each_entry(streaming, &dev->streams, list) { > > + if (uvc_queue_streaming(&streaming->queue)) > > + return true; > > + } > > + > > + return false; > > +} > > > > > > But not too blank lines here. > > > static u8 uvc_gpio_update_value(struct uvc_device *dev, > > @@ -1499,7 +1511,12 @@ static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity, > > if (cs != UVC_CT_PRIVACY_CONTROL || size < 1) > > return -EINVAL; > > > > + if ((dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) && > > + !uvc_gpio_is_streaming(dev)) > > + return -EBUSY; > > + > > *(uint8_t *)data = uvc_gpio_update_value(dev, entity); > > + > > return 0; > > } > > > > @@ -1528,19 +1545,50 @@ static struct uvc_entity *uvc_gpio_find_entity(struct uvc_device *dev) > > return NULL; > > } > > > > -static irqreturn_t uvc_gpio_irq(int irq, void *data) > > +void uvc_privacy_gpio_event(struct uvc_device *dev) > > { > > - struct uvc_device *dev = data; > > struct uvc_entity *unit; > > > > + > > unit = uvc_gpio_find_entity(dev); > > if (!unit) > > - return IRQ_HANDLED; > > + return; > > > > uvc_gpio_update_value(dev, unit); > > +} > > + > > +static irqreturn_t uvc_gpio_irq(int irq, void *data) > > +{ > > + struct uvc_device *dev = data; > > + > > + /* Ignore privacy events during streamoff */ > > + if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) > > + if (!uvc_gpio_is_streaming(dev)) > > + return IRQ_HANDLED; > > I'm still a bit concerned of race conditions. When stopping the stream, > vb2_queue.streaming is set to 0 after calling the driver's .stop_stream() > handler. This means that the device will cut power before > uvc_gpio_is_streaming() can detect that streaming has stopped, and the > GPIO could thus trigger an IRQ. On the affected devices I have not seen this. I guess it takes some time to discharge. Anyway I am implementing a workaround. Tell me if it is too ugly. > > You mentioned that devices have a pull-up or pull-down on the GPIO line. > As there are only two devices affected, do you know if it's a pull-up or > pull-down ? Would it be worse to expose that state to userspace than to > return -EBUSY when reading the control ? The module has a 100K pull up. This is, it will return "Privacy = 0". We cannot return the default value, as it would make the user believe that the privacy is in a different state that currently is. In other words, userspace needs to know at all times if the privacy is in : unknow_state, on, off. > > > + > > + uvc_privacy_gpio_event(dev); > > + > > return IRQ_HANDLED; > > } > > > > +static const struct dmi_system_id privacy_valid_during_streamon[] = { > > + { > > + .ident = "HP Elite c1030 Chromebook", > > + .matches = { > > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > > + DMI_MATCH(DMI_PRODUCT_NAME, "Jinlon"), > > + }, > > + }, > > + { > > + .ident = "HP Pro c640 Chromebook", > > + .matches = { > > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > > + DMI_MATCH(DMI_PRODUCT_NAME, "Dratini"), > > + }, > > + }, > > + { } /* terminate list */ > > +}; > > + > > static int uvc_gpio_parse(struct uvc_device *dev) > > { > > struct uvc_entity *unit; > > @@ -1577,6 +1625,9 @@ static int uvc_gpio_parse(struct uvc_device *dev) > > > > list_add_tail(&unit->list, &dev->entities); > > > > + if (dmi_check_system(privacy_valid_during_streamon)) > > + dev->quirks |= UVC_QUIRK_PRIVACY_DURING_STREAM; > > This will also match any external UVC camera plugged to one of the > affected systems, right ? It shouldn't matter in practice as those > devices won't have a GPIO entity. I did think about that but did not make it explicit in the code. Adding a comment. > > I suppose we can't match on VID:PID instead because the same VID:PID is > used in both devices affected by this issue, and devices immune to it ? The problem with VID:PID, is that the manufacturer can decide to change the camera module and then the quirk will not work. We cannot rely ONLY in VID:PID as these modules are also used in other models not affected by the quirk. I believe that it is also correct to rely on the dmi, as the quirk is caused for the way the camera module is wired, which is on the motherboard. > > > + > > return 0; > > } > > > > diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c > > index cd60c6c1749e..e800d491303f 100644 > > --- a/drivers/media/usb/uvc/uvc_queue.c > > +++ b/drivers/media/usb/uvc/uvc_queue.c > > @@ -337,9 +337,12 @@ int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf, > > int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type) > > { > > int ret; > > + struct uvc_streaming *stream = uvc_queue_to_stream(queue); > > Please swap the two lines. > > > > > mutex_lock(&queue->mutex); > > ret = vb2_streamon(&queue->queue, type); > > + if (stream->dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) > > + uvc_privacy_gpio_event(stream->dev); > > Even when vb2_streamon() failed ? > > > mutex_unlock(&queue->mutex); > > > > return ret; > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > index 079a407ebba5..32c1ba246d97 100644 > > --- a/drivers/media/usb/uvc/uvcvideo.h > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > @@ -209,6 +209,7 @@ > > #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400 > > #define UVC_QUIRK_FORCE_Y8 0x00000800 > > #define UVC_QUIRK_FORCE_BPP 0x00001000 > > +#define UVC_QUIRK_PRIVACY_DURING_STREAM 0x00002000 > > > > /* Format flags */ > > #define UVC_FMT_FLAG_COMPRESSED 0x00000001 > > @@ -826,6 +827,9 @@ extern const struct v4l2_file_operations uvc_fops; > > int uvc_mc_register_entities(struct uvc_video_chain *chain); > > void uvc_mc_cleanup_entity(struct uvc_entity *entity); > > > > +/* Privacy gpio */ > > +void uvc_privacy_gpio_event(struct uvc_device *dev); > > + > > /* Video */ > > int uvc_video_init(struct uvc_streaming *stream); > > int uvc_video_suspend(struct uvc_streaming *stream); > > -- > Regards, > > Laurent Pinchart -- Ricardo Ribalda
Hi Ricardo, On Tue, Dec 22, 2020 at 09:04:19PM +0100, Ricardo Ribalda wrote: > On Tue, Dec 22, 2020 at 11:30 AM Laurent Pinchart wrote: > > On Mon, Dec 21, 2020 at 05:48:16PM +0100, Ricardo Ribalda wrote: > > > Some devices, can only read the privacy_pin if the device is > > > > s/devices,/devices/ > > > > > streaming. > > > > > > This patch implement a quirk for such devices, in order to avoid invalid > > > reads and/or spurious events. > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > drivers/media/usb/uvc/uvc_driver.c | 57 ++++++++++++++++++++++++++++-- > > > drivers/media/usb/uvc/uvc_queue.c | 3 ++ > > > drivers/media/usb/uvc/uvcvideo.h | 4 +++ > > > 3 files changed, 61 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > > > index 72516101fdd0..7af37d4bd60a 100644 > > > --- a/drivers/media/usb/uvc/uvc_driver.c > > > +++ b/drivers/media/usb/uvc/uvc_driver.c > > > @@ -7,6 +7,7 @@ > > > */ > > > > > > #include <linux/atomic.h> > > > +#include <linux/dmi.h> > > > #include <linux/gpio/consumer.h> > > > #include <linux/kernel.h> > > > #include <linux/list.h> > > > @@ -1472,6 +1473,17 @@ static int uvc_parse_control(struct uvc_device *dev) > > > /* ----------------------------------------------------------------------------- > > > * Privacy GPIO > > > */ > > > > There should be a blank line here. > > > > > +static bool uvc_gpio_is_streaming(struct uvc_device *dev) > > > +{ > > > + struct uvc_streaming *streaming; > > > + > > > + list_for_each_entry(streaming, &dev->streams, list) { > > > + if (uvc_queue_streaming(&streaming->queue)) > > > + return true; > > > + } > > > + > > > + return false; > > > +} > > > > > > > > > > But not too blank lines here. > > > > > static u8 uvc_gpio_update_value(struct uvc_device *dev, > > > @@ -1499,7 +1511,12 @@ static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity, > > > if (cs != UVC_CT_PRIVACY_CONTROL || size < 1) > > > return -EINVAL; > > > > > > + if ((dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) && > > > + !uvc_gpio_is_streaming(dev)) > > > + return -EBUSY; > > > + > > > *(uint8_t *)data = uvc_gpio_update_value(dev, entity); > > > + > > > return 0; > > > } > > > > > > @@ -1528,19 +1545,50 @@ static struct uvc_entity *uvc_gpio_find_entity(struct uvc_device *dev) > > > return NULL; > > > } > > > > > > -static irqreturn_t uvc_gpio_irq(int irq, void *data) > > > +void uvc_privacy_gpio_event(struct uvc_device *dev) > > > { > > > - struct uvc_device *dev = data; > > > struct uvc_entity *unit; > > > > > > + > > > unit = uvc_gpio_find_entity(dev); > > > if (!unit) > > > - return IRQ_HANDLED; > > > + return; > > > > > > uvc_gpio_update_value(dev, unit); > > > +} > > > + > > > +static irqreturn_t uvc_gpio_irq(int irq, void *data) > > > +{ > > > + struct uvc_device *dev = data; > > > + > > > + /* Ignore privacy events during streamoff */ > > > + if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) > > > + if (!uvc_gpio_is_streaming(dev)) > > > + return IRQ_HANDLED; > > > > I'm still a bit concerned of race conditions. When stopping the stream, > > vb2_queue.streaming is set to 0 after calling the driver's .stop_stream() > > handler. This means that the device will cut power before > > uvc_gpio_is_streaming() can detect that streaming has stopped, and the > > GPIO could thus trigger an IRQ. > > On the affected devices I have not seen this. I guess it takes some > time to discharge. Anyway I am implementing a workaround. Tell me if > it is too ugly. > > > You mentioned that devices have a pull-up or pull-down on the GPIO line. > > As there are only two devices affected, do you know if it's a pull-up or > > pull-down ? Would it be worse to expose that state to userspace than to > > return -EBUSY when reading the control ? > > The module has a 100K pull up. This is, it will return "Privacy = 0". > > We cannot return the default value, as it would make the user believe > that the privacy is in a different state that currently is. > In other words, userspace needs to know at all times if the privacy is > in : unknow_state, on, off. This seems to be the core of the issue: we're trying to shove 3 states into a boolean. Would this call for turning the V4L2_CID_PRIVACY control into a menu ? Or maybe setting V4L2_CTRL_FLAG_INACTIVE ? Returning -EBUSY when the control is read while not streaming, and not generating an event that tells the control value becomes unknown, seems like a hack designed to work with a single userspace implementation. As the rest of the series is getting ready, I'd propose merging it without this patch until we figure out what should be done to support these lovely devices. Would that work for you ? > > > + > > > + uvc_privacy_gpio_event(dev); > > > + > > > return IRQ_HANDLED; > > > } > > > > > > +static const struct dmi_system_id privacy_valid_during_streamon[] = { > > > + { > > > + .ident = "HP Elite c1030 Chromebook", > > > + .matches = { > > > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > > > + DMI_MATCH(DMI_PRODUCT_NAME, "Jinlon"), > > > + }, > > > + }, > > > + { > > > + .ident = "HP Pro c640 Chromebook", > > > + .matches = { > > > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > > > + DMI_MATCH(DMI_PRODUCT_NAME, "Dratini"), > > > + }, > > > + }, > > > + { } /* terminate list */ > > > +}; > > > + > > > static int uvc_gpio_parse(struct uvc_device *dev) > > > { > > > struct uvc_entity *unit; > > > @@ -1577,6 +1625,9 @@ static int uvc_gpio_parse(struct uvc_device *dev) > > > > > > list_add_tail(&unit->list, &dev->entities); > > > > > > + if (dmi_check_system(privacy_valid_during_streamon)) > > > + dev->quirks |= UVC_QUIRK_PRIVACY_DURING_STREAM; > > > > This will also match any external UVC camera plugged to one of the > > affected systems, right ? It shouldn't matter in practice as those > > devices won't have a GPIO entity. > > I did think about that but did not make it explicit in the code. > Adding a comment. > > > I suppose we can't match on VID:PID instead because the same VID:PID is > > used in both devices affected by this issue, and devices immune to it ? > > The problem with VID:PID, is that the manufacturer can decide to > change the camera module and then the quirk will not work. > > We cannot rely ONLY in VID:PID as these modules are also used in other > models not affected by the quirk. > > I believe that it is also correct to rely on the dmi, as the quirk is > caused for the way the camera module is wired, which is on the > motherboard. I can't comment on the hardware side as I lack details. Using the VID:PID only seems a problem indeed. We could combine DMI and VID:PID, but that wouldn't make a difference in practice, so I suppose this is good enough. > > > + > > > return 0; > > > } > > > > > > diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c > > > index cd60c6c1749e..e800d491303f 100644 > > > --- a/drivers/media/usb/uvc/uvc_queue.c > > > +++ b/drivers/media/usb/uvc/uvc_queue.c > > > @@ -337,9 +337,12 @@ int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf, > > > int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type) > > > { > > > int ret; > > > + struct uvc_streaming *stream = uvc_queue_to_stream(queue); > > > > Please swap the two lines. > > > > > > > > mutex_lock(&queue->mutex); > > > ret = vb2_streamon(&queue->queue, type); > > > + if (stream->dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) > > > + uvc_privacy_gpio_event(stream->dev); > > > > Even when vb2_streamon() failed ? > > > > > mutex_unlock(&queue->mutex); > > > > > > return ret; > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > > index 079a407ebba5..32c1ba246d97 100644 > > > --- a/drivers/media/usb/uvc/uvcvideo.h > > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > > @@ -209,6 +209,7 @@ > > > #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400 > > > #define UVC_QUIRK_FORCE_Y8 0x00000800 > > > #define UVC_QUIRK_FORCE_BPP 0x00001000 > > > +#define UVC_QUIRK_PRIVACY_DURING_STREAM 0x00002000 > > > > > > /* Format flags */ > > > #define UVC_FMT_FLAG_COMPRESSED 0x00000001 > > > @@ -826,6 +827,9 @@ extern const struct v4l2_file_operations uvc_fops; > > > int uvc_mc_register_entities(struct uvc_video_chain *chain); > > > void uvc_mc_cleanup_entity(struct uvc_entity *entity); > > > > > > +/* Privacy gpio */ > > > +void uvc_privacy_gpio_event(struct uvc_device *dev); > > > + > > > /* Video */ > > > int uvc_video_init(struct uvc_streaming *stream); > > > int uvc_video_suspend(struct uvc_streaming *stream); -- Regards, Laurent Pinchart
Hi Laurent On Wed, Dec 23, 2020 at 9:05 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > On Tue, Dec 22, 2020 at 09:04:19PM +0100, Ricardo Ribalda wrote: > > On Tue, Dec 22, 2020 at 11:30 AM Laurent Pinchart wrote: > > > On Mon, Dec 21, 2020 at 05:48:16PM +0100, Ricardo Ribalda wrote: > > > > Some devices, can only read the privacy_pin if the device is > > > > > > s/devices,/devices/ > > > > > > > streaming. > > > > > > > > This patch implement a quirk for such devices, in order to avoid invalid > > > > reads and/or spurious events. > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > --- > > > > drivers/media/usb/uvc/uvc_driver.c | 57 ++++++++++++++++++++++++++++-- > > > > drivers/media/usb/uvc/uvc_queue.c | 3 ++ > > > > drivers/media/usb/uvc/uvcvideo.h | 4 +++ > > > > 3 files changed, 61 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > > > > index 72516101fdd0..7af37d4bd60a 100644 > > > > --- a/drivers/media/usb/uvc/uvc_driver.c > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c > > > > @@ -7,6 +7,7 @@ > > > > */ > > > > > > > > #include <linux/atomic.h> > > > > +#include <linux/dmi.h> > > > > #include <linux/gpio/consumer.h> > > > > #include <linux/kernel.h> > > > > #include <linux/list.h> > > > > @@ -1472,6 +1473,17 @@ static int uvc_parse_control(struct uvc_device *dev) > > > > /* ----------------------------------------------------------------------------- > > > > * Privacy GPIO > > > > */ > > > > > > There should be a blank line here. > > > > > > > +static bool uvc_gpio_is_streaming(struct uvc_device *dev) > > > > +{ > > > > + struct uvc_streaming *streaming; > > > > + > > > > + list_for_each_entry(streaming, &dev->streams, list) { > > > > + if (uvc_queue_streaming(&streaming->queue)) > > > > + return true; > > > > + } > > > > + > > > > + return false; > > > > +} > > > > > > > > > > > > > > But not too blank lines here. > > > > > > > static u8 uvc_gpio_update_value(struct uvc_device *dev, > > > > @@ -1499,7 +1511,12 @@ static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity, > > > > if (cs != UVC_CT_PRIVACY_CONTROL || size < 1) > > > > return -EINVAL; > > > > > > > > + if ((dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) && > > > > + !uvc_gpio_is_streaming(dev)) > > > > + return -EBUSY; > > > > + > > > > *(uint8_t *)data = uvc_gpio_update_value(dev, entity); > > > > + > > > > return 0; > > > > } > > > > > > > > @@ -1528,19 +1545,50 @@ static struct uvc_entity *uvc_gpio_find_entity(struct uvc_device *dev) > > > > return NULL; > > > > } > > > > > > > > -static irqreturn_t uvc_gpio_irq(int irq, void *data) > > > > +void uvc_privacy_gpio_event(struct uvc_device *dev) > > > > { > > > > - struct uvc_device *dev = data; > > > > struct uvc_entity *unit; > > > > > > > > + > > > > unit = uvc_gpio_find_entity(dev); > > > > if (!unit) > > > > - return IRQ_HANDLED; > > > > + return; > > > > > > > > uvc_gpio_update_value(dev, unit); > > > > +} > > > > + > > > > +static irqreturn_t uvc_gpio_irq(int irq, void *data) > > > > +{ > > > > + struct uvc_device *dev = data; > > > > + > > > > + /* Ignore privacy events during streamoff */ > > > > + if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) > > > > + if (!uvc_gpio_is_streaming(dev)) > > > > + return IRQ_HANDLED; > > > > > > I'm still a bit concerned of race conditions. When stopping the stream, > > > vb2_queue.streaming is set to 0 after calling the driver's .stop_stream() > > > handler. This means that the device will cut power before > > > uvc_gpio_is_streaming() can detect that streaming has stopped, and the > > > GPIO could thus trigger an IRQ. > > > > On the affected devices I have not seen this. I guess it takes some > > time to discharge. Anyway I am implementing a workaround. Tell me if > > it is too ugly. > > > > > You mentioned that devices have a pull-up or pull-down on the GPIO line. > > > As there are only two devices affected, do you know if it's a pull-up or > > > pull-down ? Would it be worse to expose that state to userspace than to > > > return -EBUSY when reading the control ? > > > > The module has a 100K pull up. This is, it will return "Privacy = 0". > > > > We cannot return the default value, as it would make the user believe > > that the privacy is in a different state that currently is. > > In other words, userspace needs to know at all times if the privacy is > > in : unknow_state, on, off. > > This seems to be the core of the issue: we're trying to shove 3 states > into a boolean. Would this call for turning the V4L2_CID_PRIVACY control > into a menu ? Or maybe setting V4L2_CTRL_FLAG_INACTIVE ? Returning > -EBUSY when the control is read while not streaming, and not generating > an event that tells the control value becomes unknown, seems like a hack > designed to work with a single userspace implementation. > I think that the V4L2_CTRL_FLAG_INACTIVE seems more correct. I will see how can I wire that up. > As the rest of the series is getting ready, I'd propose merging it > without this patch until we figure out what should be done to support > these lovely devices. Would that work for you ? Yes that sounds good to me. Thanks! > > > > > + > > > > + uvc_privacy_gpio_event(dev); > > > > + > > > > return IRQ_HANDLED; > > > > } > > > > > > > > +static const struct dmi_system_id privacy_valid_during_streamon[] = { > > > > + { > > > > + .ident = "HP Elite c1030 Chromebook", > > > > + .matches = { > > > > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > > > > + DMI_MATCH(DMI_PRODUCT_NAME, "Jinlon"), > > > > + }, > > > > + }, > > > > + { > > > > + .ident = "HP Pro c640 Chromebook", > > > > + .matches = { > > > > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > > > > + DMI_MATCH(DMI_PRODUCT_NAME, "Dratini"), > > > > + }, > > > > + }, > > > > + { } /* terminate list */ > > > > +}; > > > > + > > > > static int uvc_gpio_parse(struct uvc_device *dev) > > > > { > > > > struct uvc_entity *unit; > > > > @@ -1577,6 +1625,9 @@ static int uvc_gpio_parse(struct uvc_device *dev) > > > > > > > > list_add_tail(&unit->list, &dev->entities); > > > > > > > > + if (dmi_check_system(privacy_valid_during_streamon)) > > > > + dev->quirks |= UVC_QUIRK_PRIVACY_DURING_STREAM; > > > > > > This will also match any external UVC camera plugged to one of the > > > affected systems, right ? It shouldn't matter in practice as those > > > devices won't have a GPIO entity. > > > > I did think about that but did not make it explicit in the code. > > Adding a comment. > > > > > I suppose we can't match on VID:PID instead because the same VID:PID is > > > used in both devices affected by this issue, and devices immune to it ? > > > > The problem with VID:PID, is that the manufacturer can decide to > > change the camera module and then the quirk will not work. > > > > We cannot rely ONLY in VID:PID as these modules are also used in other > > models not affected by the quirk. > > > > I believe that it is also correct to rely on the dmi, as the quirk is > > caused for the way the camera module is wired, which is on the > > motherboard. > > I can't comment on the hardware side as I lack details. Using the > VID:PID only seems a problem indeed. We could combine DMI and VID:PID, > but that wouldn't make a difference in practice, so I suppose this is > good enough. > > > > > + > > > > return 0; > > > > } > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c > > > > index cd60c6c1749e..e800d491303f 100644 > > > > --- a/drivers/media/usb/uvc/uvc_queue.c > > > > +++ b/drivers/media/usb/uvc/uvc_queue.c > > > > @@ -337,9 +337,12 @@ int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf, > > > > int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type) > > > > { > > > > int ret; > > > > + struct uvc_streaming *stream = uvc_queue_to_stream(queue); > > > > > > Please swap the two lines. > > > > > > > > > > > mutex_lock(&queue->mutex); > > > > ret = vb2_streamon(&queue->queue, type); > > > > + if (stream->dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) > > > > + uvc_privacy_gpio_event(stream->dev); > > > > > > Even when vb2_streamon() failed ? > > > > > > > mutex_unlock(&queue->mutex); > > > > > > > > return ret; > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > > > index 079a407ebba5..32c1ba246d97 100644 > > > > --- a/drivers/media/usb/uvc/uvcvideo.h > > > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > > > @@ -209,6 +209,7 @@ > > > > #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400 > > > > #define UVC_QUIRK_FORCE_Y8 0x00000800 > > > > #define UVC_QUIRK_FORCE_BPP 0x00001000 > > > > +#define UVC_QUIRK_PRIVACY_DURING_STREAM 0x00002000 > > > > > > > > /* Format flags */ > > > > #define UVC_FMT_FLAG_COMPRESSED 0x00000001 > > > > @@ -826,6 +827,9 @@ extern const struct v4l2_file_operations uvc_fops; > > > > int uvc_mc_register_entities(struct uvc_video_chain *chain); > > > > void uvc_mc_cleanup_entity(struct uvc_entity *entity); > > > > > > > > +/* Privacy gpio */ > > > > +void uvc_privacy_gpio_event(struct uvc_device *dev); > > > > + > > > > /* Video */ > > > > int uvc_video_init(struct uvc_streaming *stream); > > > > int uvc_video_suspend(struct uvc_streaming *stream); > > -- > Regards, > > Laurent Pinchart -- Ricardo Ribalda
Hi again On Wed, Dec 23, 2020 at 9:31 AM Ricardo Ribalda <ribalda@chromium.org> wrote: > > Hi Laurent > > On Wed, Dec 23, 2020 at 9:05 AM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Ricardo, > > > > On Tue, Dec 22, 2020 at 09:04:19PM +0100, Ricardo Ribalda wrote: > > > On Tue, Dec 22, 2020 at 11:30 AM Laurent Pinchart wrote: > > > > On Mon, Dec 21, 2020 at 05:48:16PM +0100, Ricardo Ribalda wrote: > > > > > Some devices, can only read the privacy_pin if the device is > > > > > > > > s/devices,/devices/ > > > > > > > > > streaming. > > > > > > > > > > This patch implement a quirk for such devices, in order to avoid invalid > > > > > reads and/or spurious events. > > > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > --- > > > > > drivers/media/usb/uvc/uvc_driver.c | 57 ++++++++++++++++++++++++++++-- > > > > > drivers/media/usb/uvc/uvc_queue.c | 3 ++ > > > > > drivers/media/usb/uvc/uvcvideo.h | 4 +++ > > > > > 3 files changed, 61 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > > > > > index 72516101fdd0..7af37d4bd60a 100644 > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c > > > > > @@ -7,6 +7,7 @@ > > > > > */ > > > > > > > > > > #include <linux/atomic.h> > > > > > +#include <linux/dmi.h> > > > > > #include <linux/gpio/consumer.h> > > > > > #include <linux/kernel.h> > > > > > #include <linux/list.h> > > > > > @@ -1472,6 +1473,17 @@ static int uvc_parse_control(struct uvc_device *dev) > > > > > /* ----------------------------------------------------------------------------- > > > > > * Privacy GPIO > > > > > */ > > > > > > > > There should be a blank line here. > > > > > > > > > +static bool uvc_gpio_is_streaming(struct uvc_device *dev) > > > > > +{ > > > > > + struct uvc_streaming *streaming; > > > > > + > > > > > + list_for_each_entry(streaming, &dev->streams, list) { > > > > > + if (uvc_queue_streaming(&streaming->queue)) > > > > > + return true; > > > > > + } > > > > > + > > > > > + return false; > > > > > +} > > > > > > > > > > > > > > > > > > But not too blank lines here. > > > > > > > > > static u8 uvc_gpio_update_value(struct uvc_device *dev, > > > > > @@ -1499,7 +1511,12 @@ static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity, > > > > > if (cs != UVC_CT_PRIVACY_CONTROL || size < 1) > > > > > return -EINVAL; > > > > > > > > > > + if ((dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) && > > > > > + !uvc_gpio_is_streaming(dev)) > > > > > + return -EBUSY; > > > > > + > > > > > *(uint8_t *)data = uvc_gpio_update_value(dev, entity); > > > > > + > > > > > return 0; > > > > > } > > > > > > > > > > @@ -1528,19 +1545,50 @@ static struct uvc_entity *uvc_gpio_find_entity(struct uvc_device *dev) > > > > > return NULL; > > > > > } > > > > > > > > > > -static irqreturn_t uvc_gpio_irq(int irq, void *data) > > > > > +void uvc_privacy_gpio_event(struct uvc_device *dev) > > > > > { > > > > > - struct uvc_device *dev = data; > > > > > struct uvc_entity *unit; > > > > > > > > > > + > > > > > unit = uvc_gpio_find_entity(dev); > > > > > if (!unit) > > > > > - return IRQ_HANDLED; > > > > > + return; > > > > > > > > > > uvc_gpio_update_value(dev, unit); > > > > > +} > > > > > + > > > > > +static irqreturn_t uvc_gpio_irq(int irq, void *data) > > > > > +{ > > > > > + struct uvc_device *dev = data; > > > > > + > > > > > + /* Ignore privacy events during streamoff */ > > > > > + if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) > > > > > + if (!uvc_gpio_is_streaming(dev)) > > > > > + return IRQ_HANDLED; > > > > > > > > I'm still a bit concerned of race conditions. When stopping the stream, > > > > vb2_queue.streaming is set to 0 after calling the driver's .stop_stream() > > > > handler. This means that the device will cut power before > > > > uvc_gpio_is_streaming() can detect that streaming has stopped, and the > > > > GPIO could thus trigger an IRQ. > > > > > > On the affected devices I have not seen this. I guess it takes some > > > time to discharge. Anyway I am implementing a workaround. Tell me if > > > it is too ugly. > > > > > > > You mentioned that devices have a pull-up or pull-down on the GPIO line. > > > > As there are only two devices affected, do you know if it's a pull-up or > > > > pull-down ? Would it be worse to expose that state to userspace than to > > > > return -EBUSY when reading the control ? > > > > > > The module has a 100K pull up. This is, it will return "Privacy = 0". > > > > > > We cannot return the default value, as it would make the user believe > > > that the privacy is in a different state that currently is. > > > In other words, userspace needs to know at all times if the privacy is > > > in : unknow_state, on, off. > > > > This seems to be the core of the issue: we're trying to shove 3 states > > into a boolean. Would this call for turning the V4L2_CID_PRIVACY control > > into a menu ? Or maybe setting V4L2_CTRL_FLAG_INACTIVE ? Returning > > -EBUSY when the control is read while not streaming, and not generating > > an event that tells the control value becomes unknown, seems like a hack > > designed to work with a single userspace implementation. > > > > I think that the V4L2_CTRL_FLAG_INACTIVE seems more correct. I will > see how can I wire that up. Let me correct myself here. FLAG_INACTIVE is also not a good idea. you need two ioctls to read the value: -queryctrl() -g_ctrl() and you cannot send both at the same time. I guess we need to keep the -EBUSY or move to a menu. Since we will probably delay this patch for a while. I will resend based on -EBUSY at the end of my patchset, so it can be easily ignored if we find a better solution. Thanks! > > > > As the rest of the series is getting ready, I'd propose merging it > > without this patch until we figure out what should be done to support > > these lovely devices. Would that work for you ? > > Yes that sounds good to me. Thanks! > > > > > > > > > + > > > > > + uvc_privacy_gpio_event(dev); > > > > > + > > > > > return IRQ_HANDLED; > > > > > } > > > > > > > > > > +static const struct dmi_system_id privacy_valid_during_streamon[] = { > > > > > + { > > > > > + .ident = "HP Elite c1030 Chromebook", > > > > > + .matches = { > > > > > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > > > > > + DMI_MATCH(DMI_PRODUCT_NAME, "Jinlon"), > > > > > + }, > > > > > + }, > > > > > + { > > > > > + .ident = "HP Pro c640 Chromebook", > > > > > + .matches = { > > > > > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > > > > > + DMI_MATCH(DMI_PRODUCT_NAME, "Dratini"), > > > > > + }, > > > > > + }, > > > > > + { } /* terminate list */ > > > > > +}; > > > > > + > > > > > static int uvc_gpio_parse(struct uvc_device *dev) > > > > > { > > > > > struct uvc_entity *unit; > > > > > @@ -1577,6 +1625,9 @@ static int uvc_gpio_parse(struct uvc_device *dev) > > > > > > > > > > list_add_tail(&unit->list, &dev->entities); > > > > > > > > > > + if (dmi_check_system(privacy_valid_during_streamon)) > > > > > + dev->quirks |= UVC_QUIRK_PRIVACY_DURING_STREAM; > > > > > > > > This will also match any external UVC camera plugged to one of the > > > > affected systems, right ? It shouldn't matter in practice as those > > > > devices won't have a GPIO entity. > > > > > > I did think about that but did not make it explicit in the code. > > > Adding a comment. > > > > > > > I suppose we can't match on VID:PID instead because the same VID:PID is > > > > used in both devices affected by this issue, and devices immune to it ? > > > > > > The problem with VID:PID, is that the manufacturer can decide to > > > change the camera module and then the quirk will not work. > > > > > > We cannot rely ONLY in VID:PID as these modules are also used in other > > > models not affected by the quirk. > > > > > > I believe that it is also correct to rely on the dmi, as the quirk is > > > caused for the way the camera module is wired, which is on the > > > motherboard. > > > > I can't comment on the hardware side as I lack details. Using the > > VID:PID only seems a problem indeed. We could combine DMI and VID:PID, > > but that wouldn't make a difference in practice, so I suppose this is > > good enough. > > > > > > > + > > > > > return 0; > > > > > } > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c > > > > > index cd60c6c1749e..e800d491303f 100644 > > > > > --- a/drivers/media/usb/uvc/uvc_queue.c > > > > > +++ b/drivers/media/usb/uvc/uvc_queue.c > > > > > @@ -337,9 +337,12 @@ int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf, > > > > > int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type) > > > > > { > > > > > int ret; > > > > > + struct uvc_streaming *stream = uvc_queue_to_stream(queue); > > > > > > > > Please swap the two lines. > > > > > > > > > > > > > > mutex_lock(&queue->mutex); > > > > > ret = vb2_streamon(&queue->queue, type); > > > > > + if (stream->dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) > > > > > + uvc_privacy_gpio_event(stream->dev); > > > > > > > > Even when vb2_streamon() failed ? > > > > > > > > > mutex_unlock(&queue->mutex); > > > > > > > > > > return ret; > > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > > > > index 079a407ebba5..32c1ba246d97 100644 > > > > > --- a/drivers/media/usb/uvc/uvcvideo.h > > > > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > > > > @@ -209,6 +209,7 @@ > > > > > #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400 > > > > > #define UVC_QUIRK_FORCE_Y8 0x00000800 > > > > > #define UVC_QUIRK_FORCE_BPP 0x00001000 > > > > > +#define UVC_QUIRK_PRIVACY_DURING_STREAM 0x00002000 > > > > > > > > > > /* Format flags */ > > > > > #define UVC_FMT_FLAG_COMPRESSED 0x00000001 > > > > > @@ -826,6 +827,9 @@ extern const struct v4l2_file_operations uvc_fops; > > > > > int uvc_mc_register_entities(struct uvc_video_chain *chain); > > > > > void uvc_mc_cleanup_entity(struct uvc_entity *entity); > > > > > > > > > > +/* Privacy gpio */ > > > > > +void uvc_privacy_gpio_event(struct uvc_device *dev); > > > > > + > > > > > /* Video */ > > > > > int uvc_video_init(struct uvc_streaming *stream); > > > > > int uvc_video_suspend(struct uvc_streaming *stream); > > > > -- > > Regards, > > > > Laurent Pinchart > > > > -- > Ricardo Ribalda -- Ricardo Ribalda
On Wed, Dec 23, 2020 at 9:56 PM Ricardo Ribalda <ribalda@chromium.org> wrote: > > Hi again > > On Wed, Dec 23, 2020 at 9:31 AM Ricardo Ribalda <ribalda@chromium.org> wrote: > > > > Hi Laurent > > > > On Wed, Dec 23, 2020 at 9:05 AM Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > Hi Ricardo, > > > > > > On Tue, Dec 22, 2020 at 09:04:19PM +0100, Ricardo Ribalda wrote: > > > > On Tue, Dec 22, 2020 at 11:30 AM Laurent Pinchart wrote: > > > > > On Mon, Dec 21, 2020 at 05:48:16PM +0100, Ricardo Ribalda wrote: > > > > > > Some devices, can only read the privacy_pin if the device is > > > > > > > > > > s/devices,/devices/ > > > > > > > > > > > streaming. > > > > > > > > > > > > This patch implement a quirk for such devices, in order to avoid invalid > > > > > > reads and/or spurious events. > > > > > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > > --- > > > > > > drivers/media/usb/uvc/uvc_driver.c | 57 ++++++++++++++++++++++++++++-- > > > > > > drivers/media/usb/uvc/uvc_queue.c | 3 ++ > > > > > > drivers/media/usb/uvc/uvcvideo.h | 4 +++ > > > > > > 3 files changed, 61 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > > > > > > index 72516101fdd0..7af37d4bd60a 100644 > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c > > > > > > @@ -7,6 +7,7 @@ > > > > > > */ > > > > > > > > > > > > #include <linux/atomic.h> > > > > > > +#include <linux/dmi.h> > > > > > > #include <linux/gpio/consumer.h> > > > > > > #include <linux/kernel.h> > > > > > > #include <linux/list.h> > > > > > > @@ -1472,6 +1473,17 @@ static int uvc_parse_control(struct uvc_device *dev) > > > > > > /* ----------------------------------------------------------------------------- > > > > > > * Privacy GPIO > > > > > > */ > > > > > > > > > > There should be a blank line here. > > > > > > > > > > > +static bool uvc_gpio_is_streaming(struct uvc_device *dev) > > > > > > +{ > > > > > > + struct uvc_streaming *streaming; > > > > > > + > > > > > > + list_for_each_entry(streaming, &dev->streams, list) { > > > > > > + if (uvc_queue_streaming(&streaming->queue)) > > > > > > + return true; > > > > > > + } > > > > > > + > > > > > > + return false; > > > > > > +} > > > > > > > > > > > > > > > > > > > > > > But not too blank lines here. > > > > > > > > > > > static u8 uvc_gpio_update_value(struct uvc_device *dev, > > > > > > @@ -1499,7 +1511,12 @@ static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity, > > > > > > if (cs != UVC_CT_PRIVACY_CONTROL || size < 1) > > > > > > return -EINVAL; > > > > > > > > > > > > + if ((dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) && > > > > > > + !uvc_gpio_is_streaming(dev)) > > > > > > + return -EBUSY; > > > > > > + > > > > > > *(uint8_t *)data = uvc_gpio_update_value(dev, entity); > > > > > > + > > > > > > return 0; > > > > > > } > > > > > > > > > > > > @@ -1528,19 +1545,50 @@ static struct uvc_entity *uvc_gpio_find_entity(struct uvc_device *dev) > > > > > > return NULL; > > > > > > } > > > > > > > > > > > > -static irqreturn_t uvc_gpio_irq(int irq, void *data) > > > > > > +void uvc_privacy_gpio_event(struct uvc_device *dev) > > > > > > { > > > > > > - struct uvc_device *dev = data; > > > > > > struct uvc_entity *unit; > > > > > > > > > > > > + > > > > > > unit = uvc_gpio_find_entity(dev); > > > > > > if (!unit) > > > > > > - return IRQ_HANDLED; > > > > > > + return; > > > > > > > > > > > > uvc_gpio_update_value(dev, unit); > > > > > > +} > > > > > > + > > > > > > +static irqreturn_t uvc_gpio_irq(int irq, void *data) > > > > > > +{ > > > > > > + struct uvc_device *dev = data; > > > > > > + > > > > > > + /* Ignore privacy events during streamoff */ > > > > > > + if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) > > > > > > + if (!uvc_gpio_is_streaming(dev)) > > > > > > + return IRQ_HANDLED; > > > > > > > > > > I'm still a bit concerned of race conditions. When stopping the stream, > > > > > vb2_queue.streaming is set to 0 after calling the driver's .stop_stream() > > > > > handler. This means that the device will cut power before > > > > > uvc_gpio_is_streaming() can detect that streaming has stopped, and the > > > > > GPIO could thus trigger an IRQ. > > > > > > > > On the affected devices I have not seen this. I guess it takes some > > > > time to discharge. Anyway I am implementing a workaround. Tell me if > > > > it is too ugly. > > > > > > > > > You mentioned that devices have a pull-up or pull-down on the GPIO line. > > > > > As there are only two devices affected, do you know if it's a pull-up or > > > > > pull-down ? Would it be worse to expose that state to userspace than to > > > > > return -EBUSY when reading the control ? > > > > > > > > The module has a 100K pull up. This is, it will return "Privacy = 0". > > > > > > > > We cannot return the default value, as it would make the user believe > > > > that the privacy is in a different state that currently is. > > > > In other words, userspace needs to know at all times if the privacy is > > > > in : unknow_state, on, off. > > > > > > This seems to be the core of the issue: we're trying to shove 3 states > > > into a boolean. Would this call for turning the V4L2_CID_PRIVACY control > > > into a menu ? Or maybe setting V4L2_CTRL_FLAG_INACTIVE ? Returning > > > -EBUSY when the control is read while not streaming, and not generating > > > an event that tells the control value becomes unknown, seems like a hack > > > designed to work with a single userspace implementation. > > > > > > > I think that the V4L2_CTRL_FLAG_INACTIVE seems more correct. I will > > see how can I wire that up. > > Let me correct myself here. FLAG_INACTIVE is also not a good idea. > > you need two ioctls to read the value: > -queryctrl() > -g_ctrl() > and you cannot send both at the same time. > > I guess we need to keep the -EBUSY or move to a menu. Since we will > probably delay this patch for a while. I will resend based on -EBUSY > at the end of my patchset, so it can be easily ignored if we find a > better solution. Hi Laurent, Kieran, Would it be okay to keep the behavior as suggested by Ricardo? Best regards, Tomasz > > Thanks! > > > > > > > > As the rest of the series is getting ready, I'd propose merging it > > > without this patch until we figure out what should be done to support > > > these lovely devices. Would that work for you ? > > > > Yes that sounds good to me. Thanks! > > > > > > > > > > > > > + > > > > > > + uvc_privacy_gpio_event(dev); > > > > > > + > > > > > > return IRQ_HANDLED; > > > > > > } > > > > > > > > > > > > +static const struct dmi_system_id privacy_valid_during_streamon[] = { > > > > > > + { > > > > > > + .ident = "HP Elite c1030 Chromebook", > > > > > > + .matches = { > > > > > > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > > > > > > + DMI_MATCH(DMI_PRODUCT_NAME, "Jinlon"), > > > > > > + }, > > > > > > + }, > > > > > > + { > > > > > > + .ident = "HP Pro c640 Chromebook", > > > > > > + .matches = { > > > > > > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > > > > > > + DMI_MATCH(DMI_PRODUCT_NAME, "Dratini"), > > > > > > + }, > > > > > > + }, > > > > > > + { } /* terminate list */ > > > > > > +}; > > > > > > + > > > > > > static int uvc_gpio_parse(struct uvc_device *dev) > > > > > > { > > > > > > struct uvc_entity *unit; > > > > > > @@ -1577,6 +1625,9 @@ static int uvc_gpio_parse(struct uvc_device *dev) > > > > > > > > > > > > list_add_tail(&unit->list, &dev->entities); > > > > > > > > > > > > + if (dmi_check_system(privacy_valid_during_streamon)) > > > > > > + dev->quirks |= UVC_QUIRK_PRIVACY_DURING_STREAM; > > > > > > > > > > This will also match any external UVC camera plugged to one of the > > > > > affected systems, right ? It shouldn't matter in practice as those > > > > > devices won't have a GPIO entity. > > > > > > > > I did think about that but did not make it explicit in the code. > > > > Adding a comment. > > > > > > > > > I suppose we can't match on VID:PID instead because the same VID:PID is > > > > > used in both devices affected by this issue, and devices immune to it ? > > > > > > > > The problem with VID:PID, is that the manufacturer can decide to > > > > change the camera module and then the quirk will not work. > > > > > > > > We cannot rely ONLY in VID:PID as these modules are also used in other > > > > models not affected by the quirk. > > > > > > > > I believe that it is also correct to rely on the dmi, as the quirk is > > > > caused for the way the camera module is wired, which is on the > > > > motherboard. > > > > > > I can't comment on the hardware side as I lack details. Using the > > > VID:PID only seems a problem indeed. We could combine DMI and VID:PID, > > > but that wouldn't make a difference in practice, so I suppose this is > > > good enough. > > > > > > > > > + > > > > > > return 0; > > > > > > } > > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c > > > > > > index cd60c6c1749e..e800d491303f 100644 > > > > > > --- a/drivers/media/usb/uvc/uvc_queue.c > > > > > > +++ b/drivers/media/usb/uvc/uvc_queue.c > > > > > > @@ -337,9 +337,12 @@ int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf, > > > > > > int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type) > > > > > > { > > > > > > int ret; > > > > > > + struct uvc_streaming *stream = uvc_queue_to_stream(queue); > > > > > > > > > > Please swap the two lines. > > > > > > > > > > > > > > > > > mutex_lock(&queue->mutex); > > > > > > ret = vb2_streamon(&queue->queue, type); > > > > > > + if (stream->dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) > > > > > > + uvc_privacy_gpio_event(stream->dev); > > > > > > > > > > Even when vb2_streamon() failed ? > > > > > > > > > > > mutex_unlock(&queue->mutex); > > > > > > > > > > > > return ret; > > > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > > > > > index 079a407ebba5..32c1ba246d97 100644 > > > > > > --- a/drivers/media/usb/uvc/uvcvideo.h > > > > > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > > > > > @@ -209,6 +209,7 @@ > > > > > > #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400 > > > > > > #define UVC_QUIRK_FORCE_Y8 0x00000800 > > > > > > #define UVC_QUIRK_FORCE_BPP 0x00001000 > > > > > > +#define UVC_QUIRK_PRIVACY_DURING_STREAM 0x00002000 > > > > > > > > > > > > /* Format flags */ > > > > > > #define UVC_FMT_FLAG_COMPRESSED 0x00000001 > > > > > > @@ -826,6 +827,9 @@ extern const struct v4l2_file_operations uvc_fops; > > > > > > int uvc_mc_register_entities(struct uvc_video_chain *chain); > > > > > > void uvc_mc_cleanup_entity(struct uvc_entity *entity); > > > > > > > > > > > > +/* Privacy gpio */ > > > > > > +void uvc_privacy_gpio_event(struct uvc_device *dev); > > > > > > + > > > > > > /* Video */ > > > > > > int uvc_video_init(struct uvc_streaming *stream); > > > > > > int uvc_video_suspend(struct uvc_streaming *stream); > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart > > > > > > > > -- > > Ricardo Ribalda > > > > -- > Ricardo Ribalda
On Tue, Apr 20, 2021 at 1:54 PM Tomasz Figa <tfiga@chromium.org> wrote: > > On Wed, Dec 23, 2020 at 9:56 PM Ricardo Ribalda <ribalda@chromium.org> wrote: > > > > Hi again > > > > On Wed, Dec 23, 2020 at 9:31 AM Ricardo Ribalda <ribalda@chromium.org> wrote: > > > > > > Hi Laurent > > > > > > On Wed, Dec 23, 2020 at 9:05 AM Laurent Pinchart > > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > > > Hi Ricardo, > > > > > > > > On Tue, Dec 22, 2020 at 09:04:19PM +0100, Ricardo Ribalda wrote: > > > > > On Tue, Dec 22, 2020 at 11:30 AM Laurent Pinchart wrote: > > > > > > On Mon, Dec 21, 2020 at 05:48:16PM +0100, Ricardo Ribalda wrote: > > > > > > > Some devices, can only read the privacy_pin if the device is > > > > > > > > > > > > s/devices,/devices/ > > > > > > > > > > > > > streaming. > > > > > > > > > > > > > > This patch implement a quirk for such devices, in order to avoid invalid > > > > > > > reads and/or spurious events. > > > > > > > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > > > --- > > > > > > > drivers/media/usb/uvc/uvc_driver.c | 57 ++++++++++++++++++++++++++++-- > > > > > > > drivers/media/usb/uvc/uvc_queue.c | 3 ++ > > > > > > > drivers/media/usb/uvc/uvcvideo.h | 4 +++ > > > > > > > 3 files changed, 61 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > > > > > > > index 72516101fdd0..7af37d4bd60a 100644 > > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c > > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c > > > > > > > @@ -7,6 +7,7 @@ > > > > > > > */ > > > > > > > > > > > > > > #include <linux/atomic.h> > > > > > > > +#include <linux/dmi.h> > > > > > > > #include <linux/gpio/consumer.h> > > > > > > > #include <linux/kernel.h> > > > > > > > #include <linux/list.h> > > > > > > > @@ -1472,6 +1473,17 @@ static int uvc_parse_control(struct uvc_device *dev) > > > > > > > /* ----------------------------------------------------------------------------- > > > > > > > * Privacy GPIO > > > > > > > */ > > > > > > > > > > > > There should be a blank line here. > > > > > > > > > > > > > +static bool uvc_gpio_is_streaming(struct uvc_device *dev) > > > > > > > +{ > > > > > > > + struct uvc_streaming *streaming; > > > > > > > + > > > > > > > + list_for_each_entry(streaming, &dev->streams, list) { > > > > > > > + if (uvc_queue_streaming(&streaming->queue)) > > > > > > > + return true; > > > > > > > + } > > > > > > > + > > > > > > > + return false; > > > > > > > +} > > > > > > > > > > > > > > > > > > > > > > > > > > But not too blank lines here. > > > > > > > > > > > > > static u8 uvc_gpio_update_value(struct uvc_device *dev, > > > > > > > @@ -1499,7 +1511,12 @@ static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity, > > > > > > > if (cs != UVC_CT_PRIVACY_CONTROL || size < 1) > > > > > > > return -EINVAL; > > > > > > > > > > > > > > + if ((dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) && > > > > > > > + !uvc_gpio_is_streaming(dev)) > > > > > > > + return -EBUSY; > > > > > > > + > > > > > > > *(uint8_t *)data = uvc_gpio_update_value(dev, entity); > > > > > > > + > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > @@ -1528,19 +1545,50 @@ static struct uvc_entity *uvc_gpio_find_entity(struct uvc_device *dev) > > > > > > > return NULL; > > > > > > > } > > > > > > > > > > > > > > -static irqreturn_t uvc_gpio_irq(int irq, void *data) > > > > > > > +void uvc_privacy_gpio_event(struct uvc_device *dev) > > > > > > > { > > > > > > > - struct uvc_device *dev = data; > > > > > > > struct uvc_entity *unit; > > > > > > > > > > > > > > + > > > > > > > unit = uvc_gpio_find_entity(dev); > > > > > > > if (!unit) > > > > > > > - return IRQ_HANDLED; > > > > > > > + return; > > > > > > > > > > > > > > uvc_gpio_update_value(dev, unit); > > > > > > > +} > > > > > > > + > > > > > > > +static irqreturn_t uvc_gpio_irq(int irq, void *data) > > > > > > > +{ > > > > > > > + struct uvc_device *dev = data; > > > > > > > + > > > > > > > + /* Ignore privacy events during streamoff */ > > > > > > > + if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) > > > > > > > + if (!uvc_gpio_is_streaming(dev)) > > > > > > > + return IRQ_HANDLED; > > > > > > > > > > > > I'm still a bit concerned of race conditions. When stopping the stream, > > > > > > vb2_queue.streaming is set to 0 after calling the driver's .stop_stream() > > > > > > handler. This means that the device will cut power before > > > > > > uvc_gpio_is_streaming() can detect that streaming has stopped, and the > > > > > > GPIO could thus trigger an IRQ. > > > > > > > > > > On the affected devices I have not seen this. I guess it takes some > > > > > time to discharge. Anyway I am implementing a workaround. Tell me if > > > > > it is too ugly. > > > > > > > > > > > You mentioned that devices have a pull-up or pull-down on the GPIO line. > > > > > > As there are only two devices affected, do you know if it's a pull-up or > > > > > > pull-down ? Would it be worse to expose that state to userspace than to > > > > > > return -EBUSY when reading the control ? > > > > > > > > > > The module has a 100K pull up. This is, it will return "Privacy = 0". > > > > > > > > > > We cannot return the default value, as it would make the user believe > > > > > that the privacy is in a different state that currently is. > > > > > In other words, userspace needs to know at all times if the privacy is > > > > > in : unknow_state, on, off. > > > > > > > > This seems to be the core of the issue: we're trying to shove 3 states > > > > into a boolean. Would this call for turning the V4L2_CID_PRIVACY control > > > > into a menu ? Or maybe setting V4L2_CTRL_FLAG_INACTIVE ? Returning > > > > -EBUSY when the control is read while not streaming, and not generating > > > > an event that tells the control value becomes unknown, seems like a hack > > > > designed to work with a single userspace implementation. > > > > > > > > > > I think that the V4L2_CTRL_FLAG_INACTIVE seems more correct. I will > > > see how can I wire that up. > > > > Let me correct myself here. FLAG_INACTIVE is also not a good idea. > > > > you need two ioctls to read the value: > > -queryctrl() > > -g_ctrl() > > and you cannot send both at the same time. > > > > I guess we need to keep the -EBUSY or move to a menu. Since we will > > probably delay this patch for a while. I will resend based on -EBUSY > > at the end of my patchset, so it can be easily ignored if we find a > > better solution. > > Hi Laurent, Kieran, > > Would it be okay to keep the behavior as suggested by Ricardo? Gentle ping. > > Best regards, > Tomasz > > > > > Thanks! > > > > > > > > > > > > As the rest of the series is getting ready, I'd propose merging it > > > > without this patch until we figure out what should be done to support > > > > these lovely devices. Would that work for you ? > > > > > > Yes that sounds good to me. Thanks! > > > > > > > > > > > > > > > > > + > > > > > > > + uvc_privacy_gpio_event(dev); > > > > > > > + > > > > > > > return IRQ_HANDLED; > > > > > > > } > > > > > > > > > > > > > > +static const struct dmi_system_id privacy_valid_during_streamon[] = { > > > > > > > + { > > > > > > > + .ident = "HP Elite c1030 Chromebook", > > > > > > > + .matches = { > > > > > > > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > > > > > > > + DMI_MATCH(DMI_PRODUCT_NAME, "Jinlon"), > > > > > > > + }, > > > > > > > + }, > > > > > > > + { > > > > > > > + .ident = "HP Pro c640 Chromebook", > > > > > > > + .matches = { > > > > > > > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > > > > > > > + DMI_MATCH(DMI_PRODUCT_NAME, "Dratini"), > > > > > > > + }, > > > > > > > + }, > > > > > > > + { } /* terminate list */ > > > > > > > +}; > > > > > > > + > > > > > > > static int uvc_gpio_parse(struct uvc_device *dev) > > > > > > > { > > > > > > > struct uvc_entity *unit; > > > > > > > @@ -1577,6 +1625,9 @@ static int uvc_gpio_parse(struct uvc_device *dev) > > > > > > > > > > > > > > list_add_tail(&unit->list, &dev->entities); > > > > > > > > > > > > > > + if (dmi_check_system(privacy_valid_during_streamon)) > > > > > > > + dev->quirks |= UVC_QUIRK_PRIVACY_DURING_STREAM; > > > > > > > > > > > > This will also match any external UVC camera plugged to one of the > > > > > > affected systems, right ? It shouldn't matter in practice as those > > > > > > devices won't have a GPIO entity. > > > > > > > > > > I did think about that but did not make it explicit in the code. > > > > > Adding a comment. > > > > > > > > > > > I suppose we can't match on VID:PID instead because the same VID:PID is > > > > > > used in both devices affected by this issue, and devices immune to it ? > > > > > > > > > > The problem with VID:PID, is that the manufacturer can decide to > > > > > change the camera module and then the quirk will not work. > > > > > > > > > > We cannot rely ONLY in VID:PID as these modules are also used in other > > > > > models not affected by the quirk. > > > > > > > > > > I believe that it is also correct to rely on the dmi, as the quirk is > > > > > caused for the way the camera module is wired, which is on the > > > > > motherboard. > > > > > > > > I can't comment on the hardware side as I lack details. Using the > > > > VID:PID only seems a problem indeed. We could combine DMI and VID:PID, > > > > but that wouldn't make a difference in practice, so I suppose this is > > > > good enough. > > > > > > > > > > > + > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c > > > > > > > index cd60c6c1749e..e800d491303f 100644 > > > > > > > --- a/drivers/media/usb/uvc/uvc_queue.c > > > > > > > +++ b/drivers/media/usb/uvc/uvc_queue.c > > > > > > > @@ -337,9 +337,12 @@ int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf, > > > > > > > int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type) > > > > > > > { > > > > > > > int ret; > > > > > > > + struct uvc_streaming *stream = uvc_queue_to_stream(queue); > > > > > > > > > > > > Please swap the two lines. > > > > > > > > > > > > > > > > > > > > mutex_lock(&queue->mutex); > > > > > > > ret = vb2_streamon(&queue->queue, type); > > > > > > > + if (stream->dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) > > > > > > > + uvc_privacy_gpio_event(stream->dev); > > > > > > > > > > > > Even when vb2_streamon() failed ? > > > > > > > > > > > > > mutex_unlock(&queue->mutex); > > > > > > > > > > > > > > return ret; > > > > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > > > > > > index 079a407ebba5..32c1ba246d97 100644 > > > > > > > --- a/drivers/media/usb/uvc/uvcvideo.h > > > > > > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > > > > > > @@ -209,6 +209,7 @@ > > > > > > > #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400 > > > > > > > #define UVC_QUIRK_FORCE_Y8 0x00000800 > > > > > > > #define UVC_QUIRK_FORCE_BPP 0x00001000 > > > > > > > +#define UVC_QUIRK_PRIVACY_DURING_STREAM 0x00002000 > > > > > > > > > > > > > > /* Format flags */ > > > > > > > #define UVC_FMT_FLAG_COMPRESSED 0x00000001 > > > > > > > @@ -826,6 +827,9 @@ extern const struct v4l2_file_operations uvc_fops; > > > > > > > int uvc_mc_register_entities(struct uvc_video_chain *chain); > > > > > > > void uvc_mc_cleanup_entity(struct uvc_entity *entity); > > > > > > > > > > > > > > +/* Privacy gpio */ > > > > > > > +void uvc_privacy_gpio_event(struct uvc_device *dev); > > > > > > > + > > > > > > > /* Video */ > > > > > > > int uvc_video_init(struct uvc_streaming *stream); > > > > > > > int uvc_video_suspend(struct uvc_streaming *stream); > > > > > > > > -- > > > > Regards, > > > > > > > > Laurent Pinchart > > > > > > > > > > > > -- > > > Ricardo Ribalda > > > > > > > > -- > > Ricardo Ribalda
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 72516101fdd0..7af37d4bd60a 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -7,6 +7,7 @@ */ #include <linux/atomic.h> +#include <linux/dmi.h> #include <linux/gpio/consumer.h> #include <linux/kernel.h> #include <linux/list.h> @@ -1472,6 +1473,17 @@ static int uvc_parse_control(struct uvc_device *dev) /* ----------------------------------------------------------------------------- * Privacy GPIO */ +static bool uvc_gpio_is_streaming(struct uvc_device *dev) +{ + struct uvc_streaming *streaming; + + list_for_each_entry(streaming, &dev->streams, list) { + if (uvc_queue_streaming(&streaming->queue)) + return true; + } + + return false; +} static u8 uvc_gpio_update_value(struct uvc_device *dev, @@ -1499,7 +1511,12 @@ static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity, if (cs != UVC_CT_PRIVACY_CONTROL || size < 1) return -EINVAL; + if ((dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) && + !uvc_gpio_is_streaming(dev)) + return -EBUSY; + *(uint8_t *)data = uvc_gpio_update_value(dev, entity); + return 0; } @@ -1528,19 +1545,50 @@ static struct uvc_entity *uvc_gpio_find_entity(struct uvc_device *dev) return NULL; } -static irqreturn_t uvc_gpio_irq(int irq, void *data) +void uvc_privacy_gpio_event(struct uvc_device *dev) { - struct uvc_device *dev = data; struct uvc_entity *unit; + unit = uvc_gpio_find_entity(dev); if (!unit) - return IRQ_HANDLED; + return; uvc_gpio_update_value(dev, unit); +} + +static irqreturn_t uvc_gpio_irq(int irq, void *data) +{ + struct uvc_device *dev = data; + + /* Ignore privacy events during streamoff */ + if (dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) + if (!uvc_gpio_is_streaming(dev)) + return IRQ_HANDLED; + + uvc_privacy_gpio_event(dev); + return IRQ_HANDLED; } +static const struct dmi_system_id privacy_valid_during_streamon[] = { + { + .ident = "HP Elite c1030 Chromebook", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "HP"), + DMI_MATCH(DMI_PRODUCT_NAME, "Jinlon"), + }, + }, + { + .ident = "HP Pro c640 Chromebook", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "HP"), + DMI_MATCH(DMI_PRODUCT_NAME, "Dratini"), + }, + }, + { } /* terminate list */ +}; + static int uvc_gpio_parse(struct uvc_device *dev) { struct uvc_entity *unit; @@ -1577,6 +1625,9 @@ static int uvc_gpio_parse(struct uvc_device *dev) list_add_tail(&unit->list, &dev->entities); + if (dmi_check_system(privacy_valid_during_streamon)) + dev->quirks |= UVC_QUIRK_PRIVACY_DURING_STREAM; + return 0; } diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c index cd60c6c1749e..e800d491303f 100644 --- a/drivers/media/usb/uvc/uvc_queue.c +++ b/drivers/media/usb/uvc/uvc_queue.c @@ -337,9 +337,12 @@ int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf, int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type) { int ret; + struct uvc_streaming *stream = uvc_queue_to_stream(queue); mutex_lock(&queue->mutex); ret = vb2_streamon(&queue->queue, type); + if (stream->dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM) + uvc_privacy_gpio_event(stream->dev); mutex_unlock(&queue->mutex); return ret; diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 079a407ebba5..32c1ba246d97 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -209,6 +209,7 @@ #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400 #define UVC_QUIRK_FORCE_Y8 0x00000800 #define UVC_QUIRK_FORCE_BPP 0x00001000 +#define UVC_QUIRK_PRIVACY_DURING_STREAM 0x00002000 /* Format flags */ #define UVC_FMT_FLAG_COMPRESSED 0x00000001 @@ -826,6 +827,9 @@ extern const struct v4l2_file_operations uvc_fops; int uvc_mc_register_entities(struct uvc_video_chain *chain); void uvc_mc_cleanup_entity(struct uvc_entity *entity); +/* Privacy gpio */ +void uvc_privacy_gpio_event(struct uvc_device *dev); + /* Video */ int uvc_video_init(struct uvc_streaming *stream); int uvc_video_suspend(struct uvc_streaming *stream);
Some devices, can only read the privacy_pin if the device is streaming. This patch implement a quirk for such devices, in order to avoid invalid reads and/or spurious events. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_driver.c | 57 ++++++++++++++++++++++++++++-- drivers/media/usb/uvc/uvc_queue.c | 3 ++ drivers/media/usb/uvc/uvcvideo.h | 4 +++ 3 files changed, 61 insertions(+), 3 deletions(-)