Re: [PATCH] usb: dwc3: gadget: clear gadget pointer after exit

[Felipe Balbi <balbi@kernel.org>]

"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.

-- 
balbi