| Message ID | 1631166347-25021-1-git-send-email-quic_linyyuan@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: gadget: clear gadget pointer after exit | expand |
On Thu, Sep 09, 2021 at 01:45:47PM +0800, Linyu Yuan wrote: > change device release function to clear gadget pointer. That does not properly describe what and why this change is needed. > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > --- > drivers/usb/dwc3/gadget.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 804b505..e2ab5f6 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -4188,9 +4188,10 @@ static int dwc3_gadget_get_irq(struct dwc3 *dwc) > > static void dwc_gadget_release(struct device *dev) > { > - struct usb_gadget *gadget = container_of(dev, struct usb_gadget, dev); > + struct dwc3 *dwc = dev_get_platdata(dev); Are you sure this is the same? > > - kfree(gadget); > + kfree(dwc->gadget); > + dwc->gadget = NULL; Why set this to NULL? Who cares about this now? What changed to make it required? thanks, greg k-h
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Sent: Thursday, September 9, 2021 2:11 PM > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > Subject: Re: [PATCH] usb: dwc3: gadget: clear gadget pointer after exit > > On Thu, Sep 09, 2021 at 01:45:47PM +0800, Linyu Yuan wrote: > > change device release function to clear gadget pointer. > > That does not properly describe what and why this change is needed. > > > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > > --- > > drivers/usb/dwc3/gadget.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > index 804b505..e2ab5f6 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -4188,9 +4188,10 @@ static int dwc3_gadget_get_irq(struct dwc3 > *dwc) > > > > static void dwc_gadget_release(struct device *dev) > > { > > - struct usb_gadget *gadget = container_of(dev, struct usb_gadget, > dev); > > + struct dwc3 *dwc = dev_get_platdata(dev); > > Are you sure this is the same? Yes, in dwc3_gadget_init() usb_initialize_gadget(dwc->dev, dwc->gadget, dwc_gadget_release); dev = &dwc->gadget->dev; dev->platform_data = dwc; here original code use follow container_of, it use same dev, container_of(dev, struct usb_gadget, dev); > > > > > - kfree(gadget); > > + kfree(dwc->gadget); > > + dwc->gadget = NULL; > > Why set this to NULL? Who cares about this now? What changed to make > it required? It better to set to NULL for better understanding. > > thanks, > > greg k-h
On Thu, Sep 09, 2021 at 07:17:57AM +0000, Linyu Yuan (QUIC) wrote: > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Sent: Thursday, September 9, 2021 2:11 PM > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > > Subject: Re: [PATCH] usb: dwc3: gadget: clear gadget pointer after exit > > > > On Thu, Sep 09, 2021 at 01:45:47PM +0800, Linyu Yuan wrote: > > > change device release function to clear gadget pointer. > > > > That does not properly describe what and why this change is needed. > > > > > > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > > > --- > > > drivers/usb/dwc3/gadget.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > > index 804b505..e2ab5f6 100644 > > > --- a/drivers/usb/dwc3/gadget.c > > > +++ b/drivers/usb/dwc3/gadget.c > > > @@ -4188,9 +4188,10 @@ static int dwc3_gadget_get_irq(struct dwc3 > > *dwc) > > > > > > static void dwc_gadget_release(struct device *dev) > > > { > > > - struct usb_gadget *gadget = container_of(dev, struct usb_gadget, > > dev); > > > + struct dwc3 *dwc = dev_get_platdata(dev); > > > > Are you sure this is the same? > Yes, in dwc3_gadget_init() > usb_initialize_gadget(dwc->dev, dwc->gadget, dwc_gadget_release); > dev = &dwc->gadget->dev; > dev->platform_data = dwc; > > here original code use follow container_of, it use same dev, > container_of(dev, struct usb_gadget, dev); > > > > > > > > - kfree(gadget); > > > + kfree(dwc->gadget); > > > + dwc->gadget = NULL; > > > > Why set this to NULL? Who cares about this now? What changed to make > > it required? > It better to set to NULL for better understanding. Understanding of what? What issue does this fix? You need a reason to make this, or any, kernel change. thanks, greg k-h
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Sent: Thursday, September 9, 2021 3:38 PM > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > Subject: Re: [PATCH] usb: dwc3: gadget: clear gadget pointer after exit > > On Thu, Sep 09, 2021 at 07:17:57AM +0000, Linyu Yuan (QUIC) wrote: > > > > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Sent: Thursday, September 9, 2021 2:11 PM > > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > > > Subject: Re: [PATCH] usb: dwc3: gadget: clear gadget pointer after exit > > > > > > On Thu, Sep 09, 2021 at 01:45:47PM +0800, Linyu Yuan wrote: > > > > change device release function to clear gadget pointer. > > > > > > That does not properly describe what and why this change is needed. > > > > > > > > > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > > > > --- > > > > drivers/usb/dwc3/gadget.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > > > index 804b505..e2ab5f6 100644 > > > > --- a/drivers/usb/dwc3/gadget.c > > > > +++ b/drivers/usb/dwc3/gadget.c > > > > @@ -4188,9 +4188,10 @@ static int dwc3_gadget_get_irq(struct dwc3 > > > *dwc) > > > > > > > > static void dwc_gadget_release(struct device *dev) > > > > { > > > > - struct usb_gadget *gadget = container_of(dev, struct usb_gadget, > > > dev); > > > > + struct dwc3 *dwc = dev_get_platdata(dev); > > > > > > Are you sure this is the same? > > Yes, in dwc3_gadget_init() > > usb_initialize_gadget(dwc->dev, dwc->gadget, dwc_gadget_release); > > dev = &dwc->gadget->dev; > > dev->platform_data = dwc; > > > > here original code use follow container_of, it use same dev, > > container_of(dev, struct usb_gadget, dev); > > > > > > > > > > > - kfree(gadget); > > > > + kfree(dwc->gadget); > > > > + dwc->gadget = NULL; > > > > > > Why set this to NULL? Who cares about this now? What changed to > make > > > it required? > > It better to set to NULL for better understanding. > > Understanding of what? What issue does this fix? You need a reason to > make this, or any, kernel change. Sorry, let explain, for example, when do role switch, we can check it value to make sure it switch complete, If we do not set to NULL, it will be invalid. > > thanks, > > greg k-h
On Thu, Sep 09, 2021 at 08:02:19AM +0000, Linyu Yuan (QUIC) wrote: > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Sent: Thursday, September 9, 2021 3:38 PM > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > > Subject: Re: [PATCH] usb: dwc3: gadget: clear gadget pointer after exit > > > > On Thu, Sep 09, 2021 at 07:17:57AM +0000, Linyu Yuan (QUIC) wrote: > > > > > > > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > Sent: Thursday, September 9, 2021 2:11 PM > > > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > > > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > > > > Subject: Re: [PATCH] usb: dwc3: gadget: clear gadget pointer after exit > > > > > > > > On Thu, Sep 09, 2021 at 01:45:47PM +0800, Linyu Yuan wrote: > > > > > change device release function to clear gadget pointer. > > > > > > > > That does not properly describe what and why this change is needed. > > > > > > > > > > > > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > > > > > --- > > > > > drivers/usb/dwc3/gadget.c | 5 +++-- > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > > > > index 804b505..e2ab5f6 100644 > > > > > --- a/drivers/usb/dwc3/gadget.c > > > > > +++ b/drivers/usb/dwc3/gadget.c > > > > > @@ -4188,9 +4188,10 @@ static int dwc3_gadget_get_irq(struct dwc3 > > > > *dwc) > > > > > > > > > > static void dwc_gadget_release(struct device *dev) > > > > > { > > > > > - struct usb_gadget *gadget = container_of(dev, struct usb_gadget, > > > > dev); > > > > > + struct dwc3 *dwc = dev_get_platdata(dev); > > > > > > > > Are you sure this is the same? > > > Yes, in dwc3_gadget_init() > > > usb_initialize_gadget(dwc->dev, dwc->gadget, dwc_gadget_release); > > > dev = &dwc->gadget->dev; > > > dev->platform_data = dwc; > > > > > > here original code use follow container_of, it use same dev, > > > container_of(dev, struct usb_gadget, dev); > > > > > > > > > > > > > > - kfree(gadget); > > > > > + kfree(dwc->gadget); > > > > > + dwc->gadget = NULL; > > > > > > > > Why set this to NULL? Who cares about this now? What changed to > > make > > > > it required? > > > It better to set to NULL for better understanding. > > > > Understanding of what? What issue does this fix? You need a reason to > > make this, or any, kernel change. > Sorry, let explain, for example, when do role switch, we can check it value to make sure it switch complete, > If we do not set to NULL, it will be invalid. Are you checking that already? If not, there is no need for this. Please see the kernel documentation for how to write a changelog text, otherwise we have no idea why you are wanting to make a change. Please fix this all up when resubmitting the patch. thanks, greg k-h
"Linyu Yuan (QUIC)" <quic_linyyuan@quicinc.com> writes: >> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Sent: Thursday, September 9, 2021 3:38 PM >> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> >> Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org >> Subject: Re: [PATCH] usb: dwc3: gadget: clear gadget pointer after exit >> >> On Thu, Sep 09, 2021 at 07:17:57AM +0000, Linyu Yuan (QUIC) wrote: >> > >> > >> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> > > Sent: Thursday, September 9, 2021 2:11 PM >> > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> >> > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org >> > > Subject: Re: [PATCH] usb: dwc3: gadget: clear gadget pointer after exit >> > > >> > > On Thu, Sep 09, 2021 at 01:45:47PM +0800, Linyu Yuan wrote: >> > > > change device release function to clear gadget pointer. >> > > >> > > That does not properly describe what and why this change is needed. >> > > >> > > > >> > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> >> > > > --- >> > > > drivers/usb/dwc3/gadget.c | 5 +++-- >> > > > 1 file changed, 3 insertions(+), 2 deletions(-) >> > > > >> > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> > > > index 804b505..e2ab5f6 100644 >> > > > --- a/drivers/usb/dwc3/gadget.c >> > > > +++ b/drivers/usb/dwc3/gadget.c >> > > > @@ -4188,9 +4188,10 @@ static int dwc3_gadget_get_irq(struct dwc3 >> > > *dwc) >> > > > >> > > > static void dwc_gadget_release(struct device *dev) >> > > > { >> > > > - struct usb_gadget *gadget = container_of(dev, struct usb_gadget, >> > > dev); >> > > > + struct dwc3 *dwc = dev_get_platdata(dev); >> > > >> > > Are you sure this is the same? >> > Yes, in dwc3_gadget_init() >> > usb_initialize_gadget(dwc->dev, dwc->gadget, dwc_gadget_release); >> > dev = &dwc->gadget->dev; >> > dev->platform_data = dwc; >> > >> > here original code use follow container_of, it use same dev, >> > container_of(dev, struct usb_gadget, dev); >> > > >> > > > >> > > > - kfree(gadget); >> > > > + kfree(dwc->gadget); >> > > > + dwc->gadget = NULL; >> > > >> > > Why set this to NULL? Who cares about this now? What changed to >> make >> > > it required? >> > It better to set to NULL for better understanding. >> >> Understanding of what? What issue does this fix? You need a reason to >> make this, or any, kernel change. > > Sorry, let explain, for example, when do role switch, we can check it > value to make sure it switch complete, > > If we do not set to NULL, it will be invalid. Using this pointer as a role switch flag seems fragile, though.
> From: Felipe Balbi <balbi@kernel.org> > Sent: Thursday, September 9, 2021 4:45 PM > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux- > usb@vger.kernel.org > Subject: Re: [PATCH] usb: dwc3: gadget: clear gadget pointer after exit > > > "Linyu Yuan (QUIC)" <quic_linyyuan@quicinc.com> writes: > > >> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >> Sent: Thursday, September 9, 2021 3:38 PM > >> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > >> Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > >> Subject: Re: [PATCH] usb: dwc3: gadget: clear gadget pointer after exit > >> > >> On Thu, Sep 09, 2021 at 07:17:57AM +0000, Linyu Yuan (QUIC) wrote: > >> > > >> > > >> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >> > > Sent: Thursday, September 9, 2021 2:11 PM > >> > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > >> > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > >> > > Subject: Re: [PATCH] usb: dwc3: gadget: clear gadget pointer after exit > >> > > > >> > > On Thu, Sep 09, 2021 at 01:45:47PM +0800, Linyu Yuan wrote: > >> > > > change device release function to clear gadget pointer. > >> > > > >> > > That does not properly describe what and why this change is needed. > >> > > > >> > > > > >> > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > >> > > > --- > >> > > > drivers/usb/dwc3/gadget.c | 5 +++-- > >> > > > 1 file changed, 3 insertions(+), 2 deletions(-) > >> > > > > >> > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > >> > > > index 804b505..e2ab5f6 100644 > >> > > > --- a/drivers/usb/dwc3/gadget.c > >> > > > +++ b/drivers/usb/dwc3/gadget.c > >> > > > @@ -4188,9 +4188,10 @@ static int dwc3_gadget_get_irq(struct > dwc3 > >> > > *dwc) > >> > > > > >> > > > static void dwc_gadget_release(struct device *dev) > >> > > > { > >> > > > - struct usb_gadget *gadget = container_of(dev, struct > usb_gadget, > >> > > dev); > >> > > > + struct dwc3 *dwc = dev_get_platdata(dev); > >> > > > >> > > Are you sure this is the same? > >> > Yes, in dwc3_gadget_init() > >> > usb_initialize_gadget(dwc->dev, dwc->gadget, dwc_gadget_release); > >> > dev = &dwc->gadget->dev; > >> > dev->platform_data = dwc; > >> > > >> > here original code use follow container_of, it use same dev, > >> > container_of(dev, struct usb_gadget, dev); > >> > > > >> > > > > >> > > > - kfree(gadget); > >> > > > + kfree(dwc->gadget); > >> > > > + dwc->gadget = NULL; > >> > > > >> > > Why set this to NULL? Who cares about this now? What changed to > >> make > >> > > it required? > >> > It better to set to NULL for better understanding. > >> > >> Understanding of what? What issue does this fix? You need a reason to > >> make this, or any, kernel change. > > > > Sorry, let explain, for example, when do role switch, we can check it > > value to make sure it switch complete, > > > > If we do not set to NULL, it will be invalid. > > Using this pointer as a role switch flag seems fragile, though. Hi Felipe, I update a v2 patch, no, I didn't use this pointer for role switch, just used to check role switch status, make sure it complete. when do role switch from device to host mode, first disable device mode, currently there is no better way to confirm when gadget was complete removed. change device release function to clear gadget pointer, this will confirm device stop complete when it is NULL. > > -- > balbi
Hi, "Linyu Yuan (QUIC)" <quic_linyyuan@quicinc.com> writes: >> >> > > Why set this to NULL? Who cares about this now? What changed to >> >> make >> >> > > it required? >> >> > It better to set to NULL for better understanding. >> >> >> >> Understanding of what? What issue does this fix? You need a reason to >> >> make this, or any, kernel change. >> > >> > Sorry, let explain, for example, when do role switch, we can check it >> > value to make sure it switch complete, >> > >> > If we do not set to NULL, it will be invalid. >> >> Using this pointer as a role switch flag seems fragile, though. > Hi Felipe, > > I update a v2 patch, no, I didn't use this pointer for role switch, > just used to check role switch status, make sure it complete. exactly, you're using it as a flag for role switch. But gadget pointer being invalid is not very indication of that :-) It could be that right now these two things correlate, but there's no guarantee. You end up writing code that has possibility of failing in the future ;-) > when do role switch from device to host mode, > first disable device mode, currently there is no better way to > confirm when gadget was complete removed. if ->udc_stop() finished running, you know for a fact that gadget was completely removed ;-) If you want to be doubly sure, you can somehow notify the UDC from the gadget_release() function, but that's a little more invasive change > change device release function to clear gadget pointer, > this will confirm device stop complete when it is NULL. And instead of checking for the pointer (which can be racy in some occasions), perhaps tell the UDC about it with a callback or something like that?
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 804b505..e2ab5f6 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -4188,9 +4188,10 @@ static int dwc3_gadget_get_irq(struct dwc3 *dwc) static void dwc_gadget_release(struct device *dev) { - struct usb_gadget *gadget = container_of(dev, struct usb_gadget, dev); + struct dwc3 *dwc = dev_get_platdata(dev); - kfree(gadget); + kfree(dwc->gadget); + dwc->gadget = NULL; } /**
change device release function to clear gadget pointer. Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- drivers/usb/dwc3/gadget.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)