linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Felipe Balbi <balbi@kernel.org>
To: "Linyu Yuan (QUIC)" <quic_linyyuan@quicinc.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
Subject: Re: [PATCH] usb: dwc3: gadget: clear gadget pointer after exit
Date: Thu, 09 Sep 2021 12:44:59 +0300	[thread overview]
Message-ID: <877dfqaxm6.fsf@kernel.org> (raw)
In-Reply-To: <DM8PR02MB8198BC47C3C0598542B51A8BE3D59@DM8PR02MB8198.namprd02.prod.outlook.com>


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?

-- 
balbi

      reply	other threads:[~2021-09-09  9:48 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-09  5:45 [PATCH] usb: dwc3: gadget: clear gadget pointer after exit Linyu Yuan
2021-09-09  6:10 ` Greg Kroah-Hartman
2021-09-09  7:17   ` Linyu Yuan (QUIC)
2021-09-09  7:37     ` Greg Kroah-Hartman
2021-09-09  8:02       ` Linyu Yuan (QUIC)
2021-09-09  8:23         ` Greg Kroah-Hartman
2021-09-09  8:45         ` Felipe Balbi
2021-09-09  9:37           ` Linyu Yuan (QUIC)
2021-09-09  9:44             ` Felipe Balbi [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877dfqaxm6.fsf@kernel.org \
    --to=balbi@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=quic_linyyuan@quicinc.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).