"Linyu Yuan (QUIC)" <quic_linyyuan@xxxxxxxxxxx> writes:
>> From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> Sent: Thursday, September 9, 2021 3:38 PM
>> To: Linyu Yuan (QUIC) <quic_linyyuan@xxxxxxxxxxx>
>> Cc: Felipe Balbi <balbi@xxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxxxxxx>
>> > > Sent: Thursday, September 9, 2021 2:11 PM
>> > > To: Linyu Yuan (QUIC) <quic_linyyuan@xxxxxxxxxxx>
>> > > Cc: Felipe Balbi <balbi@xxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx
>> > > 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@xxxxxxxxxxx>
>> > > > ---
>> > > > 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
