| Message ID | 20230131095738.429197-1-dan.scally@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: uvc: Make bmControls attr read/write | expand |
On Tue, Jan 31, 2023 at 09:57:38AM +0000, Daniel Scally wrote: > For the Processing Unit and Camera Terminal descriptors defined in > the UVC Gadget we currently hard-code values into their bmControls > fields, which enumerates the controls the gadget is able to > support. This isn't appropriate since only the userspace companion > program to the kernel driver will know which controls are supported. > Make the configfs attributes that point to those fields read/write > so userspace can set them to appropriate values. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > > This patch depends on "usb: gadget: uvc: Generalise helper functions for reuse" > from my recent series: > > https://lore.kernel.org/linux-usb/20230130093443.25644-3-dan.scally@ideasonboard.com/T/#u Odd, I think I have that in my tree, yet this fails to build with: CALL scripts/checksyscalls.sh CC [M] drivers/usb/gadget/function/uvc_configfs.o drivers/usb/gadget/function/uvc_configfs.c: In function ‘uvcg_default_processing_bm_controls_store’: drivers/usb/gadget/function/uvc_configfs.c:295:15: error: implicit declaration of function ‘__uvcg_iter_item_entries’ [-Werror=implicit-function-declaration] 295 | ret = __uvcg_iter_item_entries(page, len, __uvcg_count_item_entries, &n, | ^~~~~~~~~~~~~~~~~~~~~~~~ drivers/usb/gadget/function/uvc_configfs.c:295:51: error: ‘__uvcg_count_item_entries’ undeclared (first use in this function) 295 | ret = __uvcg_iter_item_entries(page, len, __uvcg_count_item_entries, &n, | ^~~~~~~~~~~~~~~~~~~~~~~~~ drivers/usb/gadget/function/uvc_configfs.c:295:51: note: each undeclared identifier is reported only once for each function it appears in drivers/usb/gadget/function/uvc_configfs.c:311:51: error: ‘__uvcg_fill_item_entries’ undeclared (first use in this function) 311 | ret = __uvcg_iter_item_entries(page, len, __uvcg_fill_item_entries, &tmp, | ^~~~~~~~~~~~~~~~~~~~~~~~ drivers/usb/gadget/function/uvc_configfs.c: In function ‘uvcg_default_camera_bm_controls_store’: drivers/usb/gadget/function/uvc_configfs.c:465:51: error: ‘__uvcg_count_item_entries’ undeclared (first use in this function) 465 | ret = __uvcg_iter_item_entries(page, len, __uvcg_count_item_entries, &n, | ^~~~~~~~~~~~~~~~~~~~~~~~~ drivers/usb/gadget/function/uvc_configfs.c:481:51: error: ‘__uvcg_fill_item_entries’ undeclared (first use in this function) 481 | ret = __uvcg_iter_item_entries(page, len, __uvcg_fill_item_entries, &tmp, | ^~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors What did I do wrong? thanks, greg k-h
Hi Greg On 02/02/2023 10:18, Greg KH wrote: > On Tue, Jan 31, 2023 at 09:57:38AM +0000, Daniel Scally wrote: >> For the Processing Unit and Camera Terminal descriptors defined in >> the UVC Gadget we currently hard-code values into their bmControls >> fields, which enumerates the controls the gadget is able to >> support. This isn't appropriate since only the userspace companion >> program to the kernel driver will know which controls are supported. >> Make the configfs attributes that point to those fields read/write >> so userspace can set them to appropriate values. >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> >> This patch depends on "usb: gadget: uvc: Generalise helper functions for reuse" >> from my recent series: >> >> https://lore.kernel.org/linux-usb/20230130093443.25644-3-dan.scally@ideasonboard.com/T/#u > Odd, I think I have that in my tree, yet this fails to build with: > > CALL scripts/checksyscalls.sh > CC [M] drivers/usb/gadget/function/uvc_configfs.o > drivers/usb/gadget/function/uvc_configfs.c: In function ‘uvcg_default_processing_bm_controls_store’: > drivers/usb/gadget/function/uvc_configfs.c:295:15: error: implicit declaration of function ‘__uvcg_iter_item_entries’ [-Werror=implicit-function-declaration] > 295 | ret = __uvcg_iter_item_entries(page, len, __uvcg_count_item_entries, &n, > | ^~~~~~~~~~~~~~~~~~~~~~~~ > drivers/usb/gadget/function/uvc_configfs.c:295:51: error: ‘__uvcg_count_item_entries’ undeclared (first use in this function) > 295 | ret = __uvcg_iter_item_entries(page, len, __uvcg_count_item_entries, &n, > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/usb/gadget/function/uvc_configfs.c:295:51: note: each undeclared identifier is reported only once for each function it appears in > drivers/usb/gadget/function/uvc_configfs.c:311:51: error: ‘__uvcg_fill_item_entries’ undeclared (first use in this function) > 311 | ret = __uvcg_iter_item_entries(page, len, __uvcg_fill_item_entries, &tmp, > | ^~~~~~~~~~~~~~~~~~~~~~~~ > drivers/usb/gadget/function/uvc_configfs.c: In function ‘uvcg_default_camera_bm_controls_store’: > drivers/usb/gadget/function/uvc_configfs.c:465:51: error: ‘__uvcg_count_item_entries’ undeclared (first use in this function) > 465 | ret = __uvcg_iter_item_entries(page, len, __uvcg_count_item_entries, &n, > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/usb/gadget/function/uvc_configfs.c:481:51: error: ‘__uvcg_fill_item_entries’ undeclared (first use in this function) > 481 | ret = __uvcg_iter_item_entries(page, len, __uvcg_fill_item_entries, &tmp, > | ^~~~~~~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > > What did I do wrong? That's the error set I would expect if you didn't have that series earlier in the tree as __uvcg_iter_item_entries() is declared in one of those patches...if you have a link to the tree I can look more closely for you? > > thanks, > > greg k-h
On Thu, Feb 02, 2023 at 10:25:20AM +0000, Dan Scally wrote: > Hi Greg > > On 02/02/2023 10:18, Greg KH wrote: > > On Tue, Jan 31, 2023 at 09:57:38AM +0000, Daniel Scally wrote: > > > For the Processing Unit and Camera Terminal descriptors defined in > > > the UVC Gadget we currently hard-code values into their bmControls > > > fields, which enumerates the controls the gadget is able to > > > support. This isn't appropriate since only the userspace companion > > > program to the kernel driver will know which controls are supported. > > > Make the configfs attributes that point to those fields read/write > > > so userspace can set them to appropriate values. > > > > > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > > > --- > > > > > > This patch depends on "usb: gadget: uvc: Generalise helper functions for reuse" > > > from my recent series: > > > > > > https://lore.kernel.org/linux-usb/20230130093443.25644-3-dan.scally@ideasonboard.com/T/#u > > Odd, I think I have that in my tree, yet this fails to build with: > > > > CALL scripts/checksyscalls.sh > > CC [M] drivers/usb/gadget/function/uvc_configfs.o > > drivers/usb/gadget/function/uvc_configfs.c: In function ‘uvcg_default_processing_bm_controls_store’: > > drivers/usb/gadget/function/uvc_configfs.c:295:15: error: implicit declaration of function ‘__uvcg_iter_item_entries’ [-Werror=implicit-function-declaration] > > 295 | ret = __uvcg_iter_item_entries(page, len, __uvcg_count_item_entries, &n, > > | ^~~~~~~~~~~~~~~~~~~~~~~~ > > drivers/usb/gadget/function/uvc_configfs.c:295:51: error: ‘__uvcg_count_item_entries’ undeclared (first use in this function) > > 295 | ret = __uvcg_iter_item_entries(page, len, __uvcg_count_item_entries, &n, > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > drivers/usb/gadget/function/uvc_configfs.c:295:51: note: each undeclared identifier is reported only once for each function it appears in > > drivers/usb/gadget/function/uvc_configfs.c:311:51: error: ‘__uvcg_fill_item_entries’ undeclared (first use in this function) > > 311 | ret = __uvcg_iter_item_entries(page, len, __uvcg_fill_item_entries, &tmp, > > | ^~~~~~~~~~~~~~~~~~~~~~~~ > > drivers/usb/gadget/function/uvc_configfs.c: In function ‘uvcg_default_camera_bm_controls_store’: > > drivers/usb/gadget/function/uvc_configfs.c:465:51: error: ‘__uvcg_count_item_entries’ undeclared (first use in this function) > > 465 | ret = __uvcg_iter_item_entries(page, len, __uvcg_count_item_entries, &n, > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > drivers/usb/gadget/function/uvc_configfs.c:481:51: error: ‘__uvcg_fill_item_entries’ undeclared (first use in this function) > > 481 | ret = __uvcg_iter_item_entries(page, len, __uvcg_fill_item_entries, &tmp, > > | ^~~~~~~~~~~~~~~~~~~~~~~~ > > cc1: all warnings being treated as errors > > > > What did I do wrong? > > > That's the error set I would expect if you didn't have that series earlier > in the tree as __uvcg_iter_item_entries() is declared in one of those > patches...if you have a link to the tree I can look more closely for you? It's my usb-testing branch right now, rebase on that and see what happens. thanks, greg k-h
Hi Greg On 02/02/2023 10:46, Greg KH wrote: > On Thu, Feb 02, 2023 at 10:25:20AM +0000, Dan Scally wrote: >> Hi Greg >> >> On 02/02/2023 10:18, Greg KH wrote: >>> On Tue, Jan 31, 2023 at 09:57:38AM +0000, Daniel Scally wrote: >>>> For the Processing Unit and Camera Terminal descriptors defined in >>>> the UVC Gadget we currently hard-code values into their bmControls >>>> fields, which enumerates the controls the gadget is able to >>>> support. This isn't appropriate since only the userspace companion >>>> program to the kernel driver will know which controls are supported. >>>> Make the configfs attributes that point to those fields read/write >>>> so userspace can set them to appropriate values. >>>> >>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>>> --- >>>> >>>> This patch depends on "usb: gadget: uvc: Generalise helper functions for reuse" >>>> from my recent series: >>>> >>>> https://lore.kernel.org/linux-usb/20230130093443.25644-3-dan.scally@ideasonboard.com/T/#u >>> Odd, I think I have that in my tree, yet this fails to build with: >>> >>> CALL scripts/checksyscalls.sh >>> CC [M] drivers/usb/gadget/function/uvc_configfs.o >>> drivers/usb/gadget/function/uvc_configfs.c: In function ‘uvcg_default_processing_bm_controls_store’: >>> drivers/usb/gadget/function/uvc_configfs.c:295:15: error: implicit declaration of function ‘__uvcg_iter_item_entries’ [-Werror=implicit-function-declaration] >>> 295 | ret = __uvcg_iter_item_entries(page, len, __uvcg_count_item_entries, &n, >>> | ^~~~~~~~~~~~~~~~~~~~~~~~ >>> drivers/usb/gadget/function/uvc_configfs.c:295:51: error: ‘__uvcg_count_item_entries’ undeclared (first use in this function) >>> 295 | ret = __uvcg_iter_item_entries(page, len, __uvcg_count_item_entries, &n, >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~ >>> drivers/usb/gadget/function/uvc_configfs.c:295:51: note: each undeclared identifier is reported only once for each function it appears in >>> drivers/usb/gadget/function/uvc_configfs.c:311:51: error: ‘__uvcg_fill_item_entries’ undeclared (first use in this function) >>> 311 | ret = __uvcg_iter_item_entries(page, len, __uvcg_fill_item_entries, &tmp, >>> | ^~~~~~~~~~~~~~~~~~~~~~~~ >>> drivers/usb/gadget/function/uvc_configfs.c: In function ‘uvcg_default_camera_bm_controls_store’: >>> drivers/usb/gadget/function/uvc_configfs.c:465:51: error: ‘__uvcg_count_item_entries’ undeclared (first use in this function) >>> 465 | ret = __uvcg_iter_item_entries(page, len, __uvcg_count_item_entries, &n, >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~ >>> drivers/usb/gadget/function/uvc_configfs.c:481:51: error: ‘__uvcg_fill_item_entries’ undeclared (first use in this function) >>> 481 | ret = __uvcg_iter_item_entries(page, len, __uvcg_fill_item_entries, &tmp, >>> | ^~~~~~~~~~~~~~~~~~~~~~~~ >>> cc1: all warnings being treated as errors >>> >>> What did I do wrong? >> >> That's the error set I would expect if you didn't have that series earlier >> in the tree as __uvcg_iter_item_entries() is declared in one of those >> patches...if you have a link to the tree I can look more closely for you? > It's my usb-testing branch right now, rebase on that and see what > happens. I get the same build errors, but I also don't see the earlier patch in that tree, or indeed any of that series. I probably need to rebase that series to usb-testing anyway as it was also built on usb-linus. I'll do that series now and resend this patch immediately after too. > > thanks, > > greg k-h
diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc index 31636695f2d0..ddd532f30323 100644 --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc @@ -74,7 +74,7 @@ Date: Dec 2014 KernelVersion: 4.0 Description: Default camera terminal descriptors - All attributes read only: + All attributes read only except bmControls: ======================== ==================================== bmControls bitmap specifying which controls are @@ -99,7 +99,7 @@ Date: Dec 2014 KernelVersion: 4.0 Description: Default processing unit descriptors - All attributes read only: + All attributes read only except bmControls: =============== ======================================== iProcessing index of string descriptor diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c index ffa338643cc8..5e6848420728 100644 --- a/drivers/usb/gadget/function/uvc_configfs.c +++ b/drivers/usb/gadget/function/uvc_configfs.c @@ -333,6 +333,64 @@ UVCG_DEFAULT_PROCESSING_ATTR(i_processing, iProcessing, 8); #undef UVCG_DEFAULT_PROCESSING_ATTR +static ssize_t uvcg_default_processing_bm_controls_store( + struct config_item *item, const char *page, size_t len) +{ + struct config_group *group = to_config_group(item); + struct mutex *su_mutex = &group->cg_subsys->su_mutex; + struct uvc_processing_unit_descriptor *pd; + struct config_item *opts_item; + struct f_uvc_opts *opts; + u8 *bm_controls, *tmp; + unsigned int i; + int ret, n = 0; + + mutex_lock(su_mutex); + + opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; + opts = to_f_uvc_opts(opts_item); + pd = &opts->uvc_processing; + + mutex_lock(&opts->lock); + if (opts->refcnt) { + ret = -EBUSY; + goto unlock; + } + + ret = __uvcg_iter_item_entries(page, len, __uvcg_count_item_entries, &n, + sizeof(u8)); + if (ret) + goto unlock; + + if (n > pd->bControlSize) { + ret = -EINVAL; + goto unlock; + } + + tmp = bm_controls = kcalloc(n, sizeof(u8), GFP_KERNEL); + if (!bm_controls) { + ret = -ENOMEM; + goto unlock; + } + + ret = __uvcg_iter_item_entries(page, len, __uvcg_fill_item_entries, &tmp, + sizeof(u8)); + if (ret) + goto free_mem; + + for (i = 0; i < n; i++) + pd->bmControls[i] = bm_controls[i]; + + ret = len; + +free_mem: + kfree(bm_controls); +unlock: + mutex_unlock(&opts->lock); + mutex_unlock(su_mutex); + return ret; +} + static ssize_t uvcg_default_processing_bm_controls_show( struct config_item *item, char *page) { @@ -362,7 +420,7 @@ static ssize_t uvcg_default_processing_bm_controls_show( return result; } -UVC_ATTR_RO(uvcg_default_processing_, bm_controls, bmControls); +UVC_ATTR(uvcg_default_processing_, bm_controls, bmControls); static struct configfs_attribute *uvcg_default_processing_attrs[] = { &uvcg_default_processing_attr_b_unit_id, @@ -444,6 +502,65 @@ UVCG_DEFAULT_CAMERA_ATTR(w_ocular_focal_length, wOcularFocalLength, #undef UVCG_DEFAULT_CAMERA_ATTR +static ssize_t uvcg_default_camera_bm_controls_store( + struct config_item *item, const char *page, size_t len) +{ + struct config_group *group = to_config_group(item); + struct mutex *su_mutex = &group->cg_subsys->su_mutex; + struct uvc_camera_terminal_descriptor *cd; + struct config_item *opts_item; + struct f_uvc_opts *opts; + u8 *bm_controls, *tmp; + unsigned int i; + int ret, n = 0; + + mutex_lock(su_mutex); + + opts_item = group->cg_item.ci_parent->ci_parent->ci_parent-> + ci_parent; + opts = to_f_uvc_opts(opts_item); + cd = &opts->uvc_camera_terminal; + + mutex_lock(&opts->lock); + if (opts->refcnt) { + ret = -EBUSY; + goto unlock; + } + + ret = __uvcg_iter_item_entries(page, len, __uvcg_count_item_entries, &n, + sizeof(u8)); + if (ret) + goto unlock; + + if (n > cd->bControlSize) { + ret = -EINVAL; + goto unlock; + } + + tmp = bm_controls = kcalloc(n, sizeof(u8), GFP_KERNEL); + if (!bm_controls) { + ret = -ENOMEM; + goto unlock; + } + + ret = __uvcg_iter_item_entries(page, len, __uvcg_fill_item_entries, &tmp, + sizeof(u8)); + if (ret) + goto free_mem; + + for (i = 0; i < n; i++) + cd->bmControls[i] = bm_controls[i]; + + ret = len; + +free_mem: + kfree(bm_controls); +unlock: + mutex_unlock(&opts->lock); + mutex_unlock(su_mutex); + return ret; +} + static ssize_t uvcg_default_camera_bm_controls_show( struct config_item *item, char *page) { @@ -473,7 +590,7 @@ static ssize_t uvcg_default_camera_bm_controls_show( return result; } -UVC_ATTR_RO(uvcg_default_camera_, bm_controls, bmControls); +UVC_ATTR(uvcg_default_camera_, bm_controls, bmControls); static struct configfs_attribute *uvcg_default_camera_attrs[] = { &uvcg_default_camera_attr_b_terminal_id,
For the Processing Unit and Camera Terminal descriptors defined in the UVC Gadget we currently hard-code values into their bmControls fields, which enumerates the controls the gadget is able to support. This isn't appropriate since only the userspace companion program to the kernel driver will know which controls are supported. Make the configfs attributes that point to those fields read/write so userspace can set them to appropriate values. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- This patch depends on "usb: gadget: uvc: Generalise helper functions for reuse" from my recent series: https://lore.kernel.org/linux-usb/20230130093443.25644-3-dan.scally@ideasonboard.com/T/#u .../ABI/testing/configfs-usb-gadget-uvc | 4 +- drivers/usb/gadget/function/uvc_configfs.c | 121 +++++++++++++++++- 2 files changed, 121 insertions(+), 4 deletions(-)