Hi Michael,
Thank you for the patch.
On Thu, Sep 30, 2021 at 12:27:17PM +0200, Michael Grzeschik wrote:
> It is possible that the configfs gadget layer will be calling the unbind
> functions of all gadget functions on gadget_dev_desc_UDC_store and
> cleaned up the cdev structures pointer to the gadget. This will not
> prevent the usage of the usb_function_de/activate functions.
>
> f_obex and f_uvc are the candidates to still call the functions with no
> valid gadget set. This patch prevents the usage of the gadget if it was
> already unset.
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
> ---
> drivers/usb/gadget/composite.c | 4 ++--
> drivers/usb/gadget/udc/core.c | 3 +++
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 504c1cbc255d1..1b4cd01e2ae13 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -393,7 +393,7 @@ int usb_function_deactivate(struct usb_function *function)
>
> spin_lock_irqsave(&cdev->lock, flags);
>
> - if (cdev->deactivations == 0) {
> + if (cdev->deactivations == 0 && cdev->gadget) {
> spin_unlock_irqrestore(&cdev->lock, flags);
> status = usb_gadget_deactivate(cdev->gadget);
> spin_lock_irqsave(&cdev->lock, flags);
> @@ -428,7 +428,7 @@ int usb_function_activate(struct usb_function *function)
> status = -EINVAL;
> else {
> cdev->deactivations--;
> - if (cdev->deactivations == 0) {
> + if (cdev->deactivations == 0 && cdev->gadget) {
> spin_unlock_irqrestore(&cdev->lock, flags);
> status = usb_gadget_activate(cdev->gadget);
> spin_lock_irqsave(&cdev->lock, flags);
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 14fdf918ecfeb..52964d0e26fa6 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -756,6 +756,9 @@ int usb_gadget_deactivate(struct usb_gadget *gadget)
> {
> int ret = 0;
>
> + if (!gadget)
> + return ret;
> +
As far as I can tell, the only caller of usb_gadget_deactivate() is
usb_function_deactivate(). With the NULL check in
usb_function_deactivate(), do we need one here ? If so, I would expect a
similar NULL check in usb_gadget_activate().
Apart from this the patch looks ok to me, but I'm not sure if it's
really fixing the problem globally or only in specific places. I'll let
Felipe comment on this.
> if (gadget->deactivated)
> goto out;
>
--
Regards,
Laurent Pinchart
