Re: [PATCH] usb: xhci: dbc: get rid of global pointer
On 19.6.2019 9.33, Felipe Balbi wrote: @Mathias, can you drop the previous fix? I'll try to come up with a better version of this. Dropped -Mathias
Re: [PATCH] usb: core: devio: add ioctls for suspend and resume
Am Dienstag, den 18.06.2019, 11:50 -0400 schrieb Alan Stern: > On Tue, 18 Jun 2019, Mayuresh Kulkarni wrote: > > > > You're right that the program needs to know when the device is about > > > to > > > be suspended. But waiting for an ioctl to return isn't a good way > > > to do it; this needs to be a callback of some sort. That is, the > > > kernel also needs to know when the program is ready for the suspend. > > > > > > I don't know what is the best approach. > > > > This is becoming tricky now. > > Yes. There probably are mechanisms already in use in other parts of > the kernel that would be suitable here, but I don't know what they are. > We could try asking some other people for advice. Waiting for an ioctl() is horrible. If you really want to do this poll() would be the obvious API. It is made for waiting for changes of states. [..] > The suspend callback is _not_ responsible for actually suspending the > device; that is handled by the USB subsystem core. > > These ideas are indeed applicable to programs using usbfs. The kernel Not fully. Usbfs has the same issue as FUSE here. Drivers are per interface but power management is per device. Hence every driver is in the block IO path for these issues. You cannot do block IO in user space. The best you can do is notify of state changes, but you cannot wait for them. > needs to have a way to inform the program that the device is about > enter (or has just left) a low-power state, so that the program can > stop (or start) trying to communicate with the device. And the kernel > needs to know when the program is ready for the state change. That has difficulties based in fundamental issues. We can let user space block power transitions. We can notify it. But we cannot block on it. It would be easiest to export the usb_autopm_* API to user space. Regards Oliver
Re: [PATCH] USB: serial: option: add support for GosunCn ME3630 RNDIS mode
On Wed, Jun 19, 2019 at 12:30:19AM +0200, Jörgen Storvist wrote: > Added USB IDs for GosunCn ME3630 cellular module in RNDIS mode. > > T: Bus=03 Lev=01 Prnt=01 Port=01 Cnt=03 Dev#= 18 Spd=480 MxCh= 0 > D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1 > P: Vendor=19d2 ProdID=0601 Rev=03.18 > S: Manufacturer=Android > S: Product=Android > S: SerialNumber=b950269c > C: #Ifs= 5 Cfg#= 1 Atr=a0 MxPwr=500mA > I: If#=0x0 Alt= 0 #EPs= 1 Cls=e0(wlcon) Sub=01 Prot=03 Driver=rndis_host > I: If#=0x1 Alt= 0 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=rndis_host > I: If#=0x2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option > I: If#=0x3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option > I: If#=0x4 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option > > Signed-off-by: Jörgen Storvist Applied, thanks. Johan
Re: [PATCH] usb: xhci: dbc: get rid of global pointer
On Wed, Jun 19, 2019 at 09:33:40AM +0300, Felipe Balbi wrote: > Johan Hovold writes: > > Are you sure you actually did register two xhci debug ttys? > > hmm, let me check: ... > Hmm, so it only really registers after writing to sysfs file. Man, this > is an odd driver :-) I hear you. :) > @Mathias, can you drop the previous fix? I'll try to come up with a > better version of this. > > @Johan, thanks for the review. You're welcome. Johan signature.asc Description: PGP signature
Re: [PATCH] usb: core: devio: add ioctls for suspend and resume
On Wed, 19 Jun 2019, Oliver Neukum wrote: > Am Dienstag, den 18.06.2019, 11:50 -0400 schrieb Alan Stern: > > On Tue, 18 Jun 2019, Mayuresh Kulkarni wrote: > > > > > > You're right that the program needs to know when the device is about > > > > to > > > > be suspended. But waiting for an ioctl to return isn't a good way > > > > to do it; this needs to be a callback of some sort. That is, the > > > > kernel also needs to know when the program is ready for the suspend. > > > > > > > > I don't know what is the best approach. > > > > > > This is becoming tricky now. > > > > Yes. There probably are mechanisms already in use in other parts of > > the kernel that would be suitable here, but I don't know what they are. > > We could try asking some other people for advice. > > Waiting for an ioctl() is horrible. If you really want to do this > poll() would be the obvious API. It is made for waiting for changes > of states. > > [..] > > The suspend callback is _not_ responsible for actually suspending the > > device; that is handled by the USB subsystem core. > > > > These ideas are indeed applicable to programs using usbfs. The kernel > > Not fully. Usbfs has the same issue as FUSE here. Drivers are per > interface but power management is per device. Hence every driver > is in the block IO path for these issues. You cannot do block IO > in user space. The best you can do is notify of state changes, > but you cannot wait for them. usbfs access is per-device, not per-interface, but your point remains valid. > > needs to have a way to inform the program that the device is about > > enter (or has just left) a low-power state, so that the program can > > stop (or start) trying to communicate with the device. And the kernel > > needs to know when the program is ready for the state change. > > That has difficulties based in fundamental issues. We can let user > space block power transitions. We can notify it. But we cannot > block on it. > > It would be easiest to export the usb_autopm_* API to user space. But ->suspend and ->resume callbacks are part of that API, and as you say, it is not feasible to have these callbacks run in userspace. The only solution I can think of is for the userspace program to first set the device's autosuspend delay to 0. Then whenever the WAIT_FOR_RESUME ioctl returns -- even if it returns immediately -- the program should assume the suspend is over or is not going to happen. Then the program can run normally for a short while (10 seconds, perhaps) before making another attempt to suspend. The only change I would make to the patch posted earlier is to have proc_wait_for_resume() call usb_autoresume_device() and set ps->suspend_requested = false before returning. Mayuresh, how does that sound? Alan Stern
Re: [PATCH] USB/Gadget: Fix race between gether_disconnect and rx_submit
On Wed, Jun 19, 2019 at 06:41:10AM +, kvaradarajan wrote:
> On spin lock release in rx_submit, gether_disconnect get
> a chance to run, it makes port_usb NULL, rx_submit access
> NULL port USB, hence null pointer crash.
>
> Fixed by releasing the lock in rx_submit after port_usb
> is used.
Meta-comments about the patch information...
Why is this indented? Please keep comments all the way to the left and
wrap the columns at 72.
> Signed-off-by: KVaradarajan
I need a "legal name" here, I don't think you sign documents that way.
It also needs to match the From: line of your email.
> ---
> drivers/usb/gadget/function/u_ether.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/u_ether.c
> b/drivers/usb/gadget/function/u_ether.c
> index 737bd77..76cf1e4 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -186,11 +186,11 @@ static void defer_kevent(struct eth_dev *dev, int flag)
> out = dev->port_usb->out_ep;
> else
> out = NULL;
> - spin_unlock_irqrestore(&dev->lock, flags);
>
> - if (!out)
> + if (!out) {
> + spin_unlock_irqrestore(&dev->lock, flags);
> return -ENOTCONN;
> -
> + }
>
> /* Padding up to RX_EXTRA handles minor disagreements with host.
>* Normally we use the USB "terminate on short read" convention;
> @@ -215,6 +215,7 @@ static void defer_kevent(struct eth_dev *dev, int flag)
> if (dev->port_usb->is_fixed)
> size = max_t(size_t, size, dev->port_usb->fixed_out_len);
>
> + spin_unlock_irqrestore(&dev->lock, flags);
Patch looks sane to me. I'll let Felipe do the real review after you
resend based on the information above.
thanks,
greg k-h
Re: [PATCH] usb: core: devio: add ioctls for suspend and resume
Am Mittwoch, den 19.06.2019, 10:40 -0400 schrieb Alan Stern: > On Wed, 19 Jun 2019, Oliver Neukum wrote: > > It would be easiest to export the usb_autopm_* API to user space. > > But ->suspend and ->resume callbacks are part of that API, and as you > say, it is not feasible to have these callbacks run in userspace. We can notify user space about resume(). There will be a delay, but there will always be a delay, even in kernel space. > The only solution I can think of is for the userspace program to first > set the device's autosuspend delay to 0. Then whenever the > WAIT_FOR_RESUME ioctl returns -- even if it returns immediately -- the > program should assume the suspend is over or is not going to happen. > Then the program can run normally for a short while (10 seconds, > perhaps) before making another attempt to suspend. That is ugly. Quite ugly actually. Why actually do we care about suspend()? Would the user space task do anything else but cease IO? We can do that in usbfs, assuming we do not care about the order of killing. User space will learn from an appropriate error code the device went to power save. And if it does not do IO, why care? I have never seen a driver that actually saves device state in suspend(). Regards Oliver
Re: [PATCH] usb: core: devio: add ioctls for suspend and resume
On Wed, 19 Jun 2019, Oliver Neukum wrote: > Am Mittwoch, den 19.06.2019, 10:40 -0400 schrieb Alan Stern: > > On Wed, 19 Jun 2019, Oliver Neukum wrote: > > > > It would be easiest to export the usb_autopm_* API to user space. > > > > But ->suspend and ->resume callbacks are part of that API, and as you > > say, it is not feasible to have these callbacks run in userspace. > > We can notify user space about resume(). There will be a delay, but > there will always be a delay, even in kernel space. > > > The only solution I can think of is for the userspace program to first > > set the device's autosuspend delay to 0. Then whenever the > > WAIT_FOR_RESUME ioctl returns -- even if it returns immediately -- the > > program should assume the suspend is over or is not going to happen. > > Then the program can run normally for a short while (10 seconds, > > perhaps) before making another attempt to suspend. > > That is ugly. Quite ugly actually. > > Why actually do we care about suspend()? Would the user space > task do anything else but cease IO? We can do that in usbfs, > assuming we do not care about the order of killing. > User space will learn from an appropriate error code the device > went to power save. And if it does not do IO, why care? > I have never seen a driver that actually saves device state in > suspend(). Learning from error codes is problematic. According to the current design, any usbfs interaction (except for the specific suspend/resume ioctls) will cause usbfs to do a usb_autoresume_device(), thus preventing a suspend from occurring. Evidently we need to rethink this. In fact, the userspace program needs to be able to interact with the device right up until the point where it actually gets suspended. And of course, this implies that the program needs to be able to reap URBs even after the device is suspended. That sort of approach ought to be workable. Once the program sees that its URBs are failing with the appropriate error codes, it can call the WAIT_FOR_RESUME ioctl. Alan Stern
Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in adutux driver
Greg KH writes:
> On Tue, Jun 18, 2019 at 10:22:52AM -0700, dmg wrote:
>>
>> Greg KH writes:
>>
>> >> diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
>> >> index 9465fb95d70a..4a9fa3152f2a 100644
>> >> --- a/drivers/usb/misc/adutux.c
>> >> +++ b/drivers/usb/misc/adutux.c
>> >> @@ -379,7 +379,7 @@ static ssize_t adu_read(struct file *file, __user
>> >> char *buffer, size_t count,
>> >>
>> >> if (data_in_secondary) {
>> >> /* drain secondary buffer */
>> >> - int amount = bytes_to_read < data_in_secondary ?
>> >> bytes_to_read : data_in_secondary;
>> >> + int amount = min_t(size_t, bytes_to_read,
>> >> data_in_secondary);
>> >
>> > Shouldn't amount and data_in_secondary be of size_t type? Then you can
>> > just use min() here, right?
>>
>>
>> I looked at the code.
>>
>> there is an implicit assertion that
>>
>>dev->secondary_tail >= dev->secondary_head
>>
>>
>> (which are pointers to the secondary buffer)
>
> No, those are integers for the buffer, secondary_tail seems just to be
> the "length" of the buffer, and secondary_head is the current "start" of
> the buffer that has not been sent yet.
>
> So these can not ever "wrap", thank goodness.
I looked at the code and yes, the cannot wrap.
>
> But really, ick ick ick, this code is odd. Seems like they are trying
> to go with a "flip buffer" scheme when there are many simpler ways to do
> all of this...
>
> Oh well, we deal with what we have...
>
>> while (bytes_to_read) {
>> int data_in_secondary = dev->secondary_tail -
>> dev->secondary_head;
>> dev_dbg(&dev->udev->dev,
>> "%s : while, data_in_secondary=%d, status=%d\n",
>> __func__, data_in_secondary,
>> dev->interrupt_in_urb->status);
>>
>> if (data_in_secondary) {
>> /* drain secondary buffer */
>> int amount = bytes_to_read < data_in_secondary ?
>> bytes_to_read : data_in_secondary;
>> i = copy_to_user(buffer,
>> dev->read_buffer_secondary+dev->secondary_head, amount);
>> if (i) {
>> retval = -EFAULT;
>> goto exit;
>> }
>> dev->secondary_head += (amount - i);
>> bytes_read += (amount - i);
>> bytes_to_read -= (amount - i);
>> } else {
>> /* we check the primary buffer */
>>
>> As computed, data_in_secondary is the number of bytes left in the secondary
>> buffer and hence, it should always be larger or equal 0.
>
> Yes.
>
>> The if statement seems to imply this fact. There is no handling of the case
>> where data_in_secondary is negative--if that was the case, copy_to_user will
>> be
>> called with a negative number, which I assume will return an error.
>
> Looking at the code, it never can be.
>
> And no, copy_to_user() with a negative number is just a really BIG
> number, and bad things happen, we never want to do that as it's usually
> a security issue then.
>
>> This is interesting. It means that if the pointers are incorrect (head points
>> after tail), the current code will probably be able to recover from the bug
>> with
>> an error (i assume copy_to_user will return != 0 if called with a negative
>> number).
>>
>> If we change data_in_secondary to size_t, and the head points after the tail,
>> the data_in_secondary will be invalid (but positive) and copy_to_user will
>> try
>> to read those number of bytes. I don't know what would happen in that case.
>
> Looking at the code, I do not see how this can happen, do you?
I agree. No, it cannot.
>
>> Amount is number of bytes to read from this buffer, so it does not make
>> sense for it to be
>> negative. So it being size_t sounds ok.
>>
>> Barring that potential bug in the values of the pointers of the head and the
>> tail, it appears it is safe to change the type of both data_in_secondary and
>> amount to size_t, and use min instead. It will also require to change the
>> printf-style
>> modifier (%d => %lu)
>>
>> Also, note the use of "amount -i" the end of the if statement. At this
>> point i
>> is equal to zero. the "- i" can be dropped from all these computations.
>
> That is someone who did not know what copy_to_user() returned and
> assumed it was the number of bytes copied :(
>
>> please let me know if this is something that sounds ok, and I'll prepare it
>> and
>> submit a new patch.
>
> It sounds ok. And if you want to fix anything else up here in this
> mess, it's always appreciated. Odds are no one uses this driver
> anymore, but that's no reason to keep it being such a mess :)
I have created a new patch. But I am not sure what is the best way to 'connect'
my new patch to this one. I am currently using git-send-mail to avoid
interference from my mail client. Unless you
RE: [RFC] Sorting out dwc3 ISOC endpoints once and for all
> -Original Message- > From: linux-usb-ow...@vger.kernel.org On > Behalf Of Felipe Balbi > Sent: Tuesday, June 18, 2019 11:29 PM > To: Thinh Nguyen ; Thinh Nguyen > ; John Youn > Cc: linux-usb@vger.kernel.org > Subject: Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all > > > Hi, > > Thinh Nguyen writes: > > Would there be any obvious draw-back to going down this route? The > thing > > is that, as it is, it seems like we will *always* have some corner case > > where we can't guarantee that we can even start a transfer since there's > > no upper-bound between XferNotReady and gadget driver finally > queueing a > > request. Also, I can't simply read DSTS for the frame number because of > > top-most 2 bits. > > > For non-affected version of the IP, the xfernotready -> starttransfer > time will have to be off by more than a couple seconds for the driver > to produce an incorrect 16-bit frame number. If you're seeing errors > here, maybe we just need to code review the relevant sections to make > sure the 14/16-bit and rollover conditions are all handled correctly. > >>> I think what Felipe may see is some delay in the system that causes the > >>> SW to not handle XferNotReady event in time. We already have the "retry" > >>> method handle that to a certain extend. > >>> > But I can't think of any obvious drawbacks of the quirk, other than > doing some unnecessary work, which shouldn't produce any bad > side-effects. But we haven't really tested that. > > >>> The workaround for the isoc_quirk requires 2 tries sending > >>> START_TRANSFER command. This means that you have to account the > delay of > >>> that command completion plus potentially 1 more END_TRANSFER > completion. > >>> That's why the quirk gives a buffer of at least 4 uframes of the > >>> scheduled isoc frame. So, it cannot schedule immediately on the next > >>> uframe, that's one of the drawbacks. > >>> > >>> > >>> Hi Felipe, > >>> > >>> Since you're asking this, it means you're still seeing issue with your > >>> setup despite retrying to send START_TRANSFER command 5 times. What's > >>> the worse delay responding to XferNotReady you're seeing in your setup? > >> There's no upper-bound on how long the gadget will take to enqueue a > >> request. We see problems with UVC gadget all the time. It can take a lot > >> of time to decide to enqueue data. > > > > That's why there's a mechanism in the controller to return bus-expiry > > status to let the SW know if it had scheduled isoc too late. SW can do 2 > > things: 1) re-schedule at a later timer or 2) send END_TRANSFER command > > to wait for the next XferNotReady to try again. > > All of this is still rather flakey. Can I send consecutive END_TRANSFER > commands and get new XferNotReady at any moment? Consider this > situation: > > . transfer started > . transfer completes with status Missed ISOC > . driver issues END_TRANSFER (as required by docs) > . XferNotReady fires > . driver updates current frame number > . several mS of nothing > . finally gadget enqueues a transfer > . Start Transfer command > . completes with Bus Expiry > > Can I issue *another* END_TRANSFER at this point? I don't even have a > valid transfer resource since transfer wasn't started. > > The best "workaround" I can think of would be to delay END_TRASFER until > ep_queue time. > > >> Usually I hear this from folks using UVC gadget with a real sensor on > >> the background. > >> > >> I've seen gadget enqueueing as far as 20 intervals in the future. But > >> remember, there's no upper-bound. And that's the problem. If we could > >> just read the frame number from DSTS and use that, we wouldn't have any > >> issues. But since DSTS only contains 14 our of the 16 bits the > >> controller needs, then we can't really use that. > > > > You can create another quirk for devices that have this behavior to use > > frame number in DSTS instead. As John had pointed out and mentioned, > > this will only work if the service interval and the delay in the > > scheduling of isoc is within 2 seconds. > > well, that's better than nothing, but there's no upper-bound for the > gadget driver, really. > This will take care of the scenario you described above. Using xfernotready+DSTS to calculate the start transfer frame number should probably just be the default behavior. For the case the gadget driver takes > 2 seconds to queue, you can go through the quirk. It's probably best to do this pre-emptively rather than rely on bus expiry. Because bus expiry only happens when your start frame is off by > 2 seconds. So you may get the top-2 bits wrong and start transfer will succeed, but you will have introduced a delay in the stream. > They are *not* internal if SW needs to know that to start a transfer > properly it needs these extra two bits :-) What I meant to say was that > we should never have a 16-bit frame number. Only 14 bits. But in any
Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in adutux driver
On Wed, Jun 19, 2019 at 09:39:55AM -0700, dmg wrote:
>
> Greg KH writes:
>
> > On Tue, Jun 18, 2019 at 10:22:52AM -0700, dmg wrote:
> >>
> >> Greg KH writes:
> >>
> >> >> diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
> >> >> index 9465fb95d70a..4a9fa3152f2a 100644
> >> >> --- a/drivers/usb/misc/adutux.c
> >> >> +++ b/drivers/usb/misc/adutux.c
> >> >> @@ -379,7 +379,7 @@ static ssize_t adu_read(struct file *file, __user
> >> >> char *buffer, size_t count,
> >> >>
> >> >> if (data_in_secondary) {
> >> >> /* drain secondary buffer */
> >> >> - int amount = bytes_to_read < data_in_secondary
> >> >> ? bytes_to_read : data_in_secondary;
> >> >> + int amount = min_t(size_t, bytes_to_read,
> >> >> data_in_secondary);
> >> >
> >> > Shouldn't amount and data_in_secondary be of size_t type? Then you can
> >> > just use min() here, right?
> >>
> >>
> >> I looked at the code.
> >>
> >> there is an implicit assertion that
> >>
> >>dev->secondary_tail >= dev->secondary_head
> >>
> >>
> >> (which are pointers to the secondary buffer)
> >
> > No, those are integers for the buffer, secondary_tail seems just to be
> > the "length" of the buffer, and secondary_head is the current "start" of
> > the buffer that has not been sent yet.
> >
> > So these can not ever "wrap", thank goodness.
>
> I looked at the code and yes, the cannot wrap.
>
> >
> > But really, ick ick ick, this code is odd. Seems like they are trying
> > to go with a "flip buffer" scheme when there are many simpler ways to do
> > all of this...
> >
> > Oh well, we deal with what we have...
> >
> >>while (bytes_to_read) {
> >>int data_in_secondary = dev->secondary_tail -
> >> dev->secondary_head;
> >>dev_dbg(&dev->udev->dev,
> >>"%s : while, data_in_secondary=%d, status=%d\n",
> >>__func__, data_in_secondary,
> >>dev->interrupt_in_urb->status);
> >>
> >>if (data_in_secondary) {
> >>/* drain secondary buffer */
> >>int amount = bytes_to_read < data_in_secondary ?
> >> bytes_to_read : data_in_secondary;
> >>i = copy_to_user(buffer,
> >> dev->read_buffer_secondary+dev->secondary_head, amount);
> >>if (i) {
> >>retval = -EFAULT;
> >>goto exit;
> >>}
> >>dev->secondary_head += (amount - i);
> >>bytes_read += (amount - i);
> >>bytes_to_read -= (amount - i);
> >>} else {
> >>/* we check the primary buffer */
> >>
> >> As computed, data_in_secondary is the number of bytes left in the secondary
> >> buffer and hence, it should always be larger or equal 0.
> >
> > Yes.
> >
> >> The if statement seems to imply this fact. There is no handling of the case
> >> where data_in_secondary is negative--if that was the case, copy_to_user
> >> will be
> >> called with a negative number, which I assume will return an error.
> >
> > Looking at the code, it never can be.
> >
> > And no, copy_to_user() with a negative number is just a really BIG
> > number, and bad things happen, we never want to do that as it's usually
> > a security issue then.
> >
> >> This is interesting. It means that if the pointers are incorrect (head
> >> points
> >> after tail), the current code will probably be able to recover from the
> >> bug with
> >> an error (i assume copy_to_user will return != 0 if called with a negative
> >> number).
> >>
> >> If we change data_in_secondary to size_t, and the head points after the
> >> tail,
> >> the data_in_secondary will be invalid (but positive) and copy_to_user will
> >> try
> >> to read those number of bytes. I don't know what would happen in that case.
> >
> > Looking at the code, I do not see how this can happen, do you?
>
> I agree. No, it cannot.
>
> >
> >> Amount is number of bytes to read from this buffer, so it does not make
> >> sense for it to be
> >> negative. So it being size_t sounds ok.
> >>
> >> Barring that potential bug in the values of the pointers of the head and
> >> the
> >> tail, it appears it is safe to change the type of both data_in_secondary
> >> and
> >> amount to size_t, and use min instead. It will also require to change the
> >> printf-style
> >> modifier (%d => %lu)
> >>
> >> Also, note the use of "amount -i" the end of the if statement. At this
> >> point i
> >> is equal to zero. the "- i" can be dropped from all these computations.
> >
> > That is someone who did not know what copy_to_user() returned and
> > assumed it was the number of bytes copied :(
> >
> >> please let me know if this is something that sounds ok, and I'll prepare
> >> it and
> >> submit a new patch.
> >
> > It sounds ok. And if you want to fix anything else up here in this
> > mess, i
Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all
Felipe Balbi wrote: > Hi, > > Thinh Nguyen writes: >> Would there be any obvious draw-back to going down this route? The thing >> is that, as it is, it seems like we will *always* have some corner case >> where we can't guarantee that we can even start a transfer since there's >> no upper-bound between XferNotReady and gadget driver finally queueing a >> request. Also, I can't simply read DSTS for the frame number because of >> top-most 2 bits. >> > For non-affected version of the IP, the xfernotready -> starttransfer > time will have to be off by more than a couple seconds for the driver > to produce an incorrect 16-bit frame number. If you're seeing errors > here, maybe we just need to code review the relevant sections to make > sure the 14/16-bit and rollover conditions are all handled correctly. I think what Felipe may see is some delay in the system that causes the SW to not handle XferNotReady event in time. We already have the "retry" method handle that to a certain extend. > But I can't think of any obvious drawbacks of the quirk, other than > doing some unnecessary work, which shouldn't produce any bad > side-effects. But we haven't really tested that. > The workaround for the isoc_quirk requires 2 tries sending START_TRANSFER command. This means that you have to account the delay of that command completion plus potentially 1 more END_TRANSFER completion. That's why the quirk gives a buffer of at least 4 uframes of the scheduled isoc frame. So, it cannot schedule immediately on the next uframe, that's one of the drawbacks. Hi Felipe, Since you're asking this, it means you're still seeing issue with your setup despite retrying to send START_TRANSFER command 5 times. What's the worse delay responding to XferNotReady you're seeing in your setup? >>> There's no upper-bound on how long the gadget will take to enqueue a >>> request. We see problems with UVC gadget all the time. It can take a lot >>> of time to decide to enqueue data. >> That's why there's a mechanism in the controller to return bus-expiry >> status to let the SW know if it had scheduled isoc too late. SW can do 2 >> things: 1) re-schedule at a later timer or 2) send END_TRANSFER command >> to wait for the next XferNotReady to try again. > All of this is still rather flakey. Can I send consecutive END_TRANSFER > commands and get new XferNotReady at any moment? Consider this > situation: > > . transfer started > . transfer completes with status Missed ISOC > . driver issues END_TRANSFER (as required by docs) > . XferNotReady fires > . driver updates current frame number > . several mS of nothing > . finally gadget enqueues a transfer > . Start Transfer command > . completes with Bus Expiry > > Can I issue *another* END_TRANSFER at this point? I don't even have a > valid transfer resource since transfer wasn't started. Yes you can. If the START_TRANSFER command completes, you will get the transfer resource index to use for END_TRANSFER command (regardless of bus-expiry status). However, there's a chance if the SW somehow keeps handling the XferNotReady event late over and over and the isoc transfer never get started. That's where you can create a quirk and use frame number from DSTS instead. > The best "workaround" I can think of would be to delay END_TRASFER until > ep_queue time. > >>> Usually I hear this from folks using UVC gadget with a real sensor on >>> the background. >>> >>> I've seen gadget enqueueing as far as 20 intervals in the future. But >>> remember, there's no upper-bound. And that's the problem. If we could >>> just read the frame number from DSTS and use that, we wouldn't have any >>> issues. But since DSTS only contains 14 our of the 16 bits the >>> controller needs, then we can't really use that. >> You can create another quirk for devices that have this behavior to use >> frame number in DSTS instead. As John had pointed out and mentioned, >> this will only work if the service interval and the delay in the >> scheduling of isoc is within 2 seconds. > well, that's better than nothing, but there's no upper-bound for the > gadget driver, really. > >> You will need to calculate this value along with BIT(15) and BIT(14) of >> XferNotReady for rollovers. >> >>> To me, it seems like this part of the controller wasn't well >>> thought-out. These extra two bits, perhaps, should be internal to the >>> controller and SW should have no knowledge that they exist. >> These values are internal. SW should not have knowledge of it. This >> implementation will not follow the programming guide and should be used >> as a quirk for devices that are too slow to handle the XferNotReady >> event but want to schedule isoc immediately after handling the event. > They are *not* internal if SW needs to know that to start a transfer > properly it needs these extra two bits :-) What I meant to s
xHCI Driver Compliance Test Support
Hi Mathias, I am working on running our custom USB dual-role product through some compliance testing. It seems that the SoC and host controller are not responding to the LFPS signaling and timeout that is supposed to automatically begin the compliance test sequence. I'm currently running a 4.9.115 kernel, and I'm afraid I might be missing some critical patches for compliance test support. I noticed these two patches came up in a google search: https://patchwork.kernel.org/patch/10415345/ https://www.spinics.net/lists/linux-usb/msg160002.html Besides these patches, is there anything else that comes to mind that I might need to do to start compliance testing? I'm about to build a more recent kernel to see if that improves my situation as well. Just for reference, our product uses an intel atom z8550 SoC that uses an xHCI host controller and a dwc3 device controller. Our platform also uses a USB 3.0 redriver. The datasheet for this redriver (tusb542) indicates that it's internal LFPS controller supports full USB 3.0 compliance requirements. Thanks in advance for your guidance! Cheers, Rob Weber
[PATCH] usb: clean up some of the computations in adu_read
From: Daniel M German
Replace ?: with min to calculate the number of bytes in the secondary buffer,
including changing the data type of data_in_secondary to size_t to be
type-consistent. data_in_secondary can never be negative.
Remove some spurious calculations (copy_to_user returns zero on success),
making one variable redundant (i)
This change does not alter the functionality of the code.
Signed-off-by: Daniel M German
---
drivers/usb/misc/adutux.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index 9465fb95d70a..cbc0e54508bf 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -343,7 +343,6 @@ static ssize_t adu_read(struct file *file, __user char
*buffer, size_t count,
struct adu_device *dev;
size_t bytes_read = 0;
size_t bytes_to_read = count;
- int i;
int retval = 0;
int timeout = 0;
int should_submit = 0;
@@ -371,23 +370,22 @@ static ssize_t adu_read(struct file *file, __user char
*buffer, size_t count,
timeout = COMMAND_TIMEOUT;
dev_dbg(&dev->udev->dev, "%s : about to start looping\n", __func__);
while (bytes_to_read) {
- int data_in_secondary = dev->secondary_tail -
dev->secondary_head;
+ size_t data_in_secondary = dev->secondary_tail -
dev->secondary_head;
dev_dbg(&dev->udev->dev,
- "%s : while, data_in_secondary=%d, status=%d\n",
+ "%s : while, data_in_secondary=%lu, status=%d\n",
__func__, data_in_secondary,
dev->interrupt_in_urb->status);
if (data_in_secondary) {
/* drain secondary buffer */
- int amount = bytes_to_read < data_in_secondary ?
bytes_to_read : data_in_secondary;
- i = copy_to_user(buffer,
dev->read_buffer_secondary+dev->secondary_head, amount);
- if (i) {
+ size_t amount = min(bytes_to_read, data_in_secondary);
+ if (copy_to_user(buffer,
dev->read_buffer_secondary+dev->secondary_head, amount)) {
retval = -EFAULT;
goto exit;
}
- dev->secondary_head += (amount - i);
- bytes_read += (amount - i);
- bytes_to_read -= (amount - i);
+ dev->secondary_head += amount;
+ bytes_read += amount;
+ bytes_to_read -= amount;
} else {
/* we check the primary buffer */
spin_lock_irqsave (&dev->buflock, flags);
--
2.20.1
RE: Adding NovAtel USB vendor & device ID to Kernel
Hi Johan,
I apologize for the extreme delay in replying to your latest message. I was
considering approaching this an entirely different way and have since returned
to submitting the request directly to you. As such, I will now address each of
your questions.
-- Q1. Do your products support other serial interfaces as well (e.g. UART or
i2c)? --
Our USB interface offers 3 virtual UARTs that can be used a serial ports. All
of our products also can be interfaced to using RS232 and many offer Ethernet,
Wifi and CAN support. But all that I think is relevant for adding support to
the kernel is the USB virtual serial ports.
-- Q2. As Oliver mentioned it would be useful to know what USB-serial converter
chip you use in the device, if any, or of this is a custom implementation. --
As far as I am aware, this is our own implementation. Our Windows driver's have
a mention in their configuration about a 16550 compatible UART, however I'm not
certain if that is relevant. For your purposes.
-- Q3. Since 4.19 we actually have GNSS subsystem in the kernel, which if we
implement a driver for your devices would show up as /dev/gnssN with an
associated attribute describing the GNSS type (reflecting the supported
protocol, e.g. NMEA, UBX or SiRF). What protocols do your devices use (besides
NMEA)? Do use any common GNSS receivers chips, or do you have your own? --
That's very interesting!
The GNSS chips used are our own.
Out of the box our receivers communicate using our own protocol. Users can send
proprietary binary or ASCII messages, which are our own standard format that
has been used for decades. This "NOVATEL" protocol is well-documented online.
Our serial communication ports can be reconfigured to comply with various other
formats, the list is quite long. You can see the full set of supported
protocols at the following page under the title "Serial Port Interface Modes":
https://docs.novatel.com/OEM7/Content/Commands/INTERFACEMODE.htm?
-- Q4. What are the three ports used for, or is that configurable? --
Our ports are highly configurable by the user. Some users will only ever use
one port. While others will configure some ports to specific dedicated uses
such as for configuration or logging different formats of GNSS, Timing, and INS
type data records.
-- Q5. Depending a bit on your answers to the above questions, we could also
add your VID/PID to a USB-serial driver, which would give you ttyUSBN devices
without any need to mess with modprobe. --
This is all I was hoping we could get done here. So, "yes please!".
Here's a tiny bit more context for you - When our Linux users ask us how to
configure their Linux machines to recognize our receivers over USB, we
typically ask them to the following:
1. Create the file /etc/udev/rules.d/z90_novatel.rules
2. Paste in these lines:
SUBSYSTEM=="usb", SYSFS{idProduct}=="0100", SYSFS{idVendor}=="09d7",
PROGRAM="/sbin/modprobe usbserial vendor=0x09d7 product=0x0100"
BUS=="usb", SYSFS{idProduct}=="0100", SYSFS{idVendor}=="09d7",
SYSFS{product}=="NovAtel GPS Receiver", SYSFS{manufacturer}=="NovAtel Inc.",
SYMLINK+="gps%n"
This doesn't work for every distro, but most. Another approach that can work is
to simply enter this in to the shell:
echo '09d7 0100' > /sys/bus/usb-serial/drivers/generic/new_id
Does this satisfy your questions?
Thank you very much. Best regards,
James Snell
-Original Message-
From: Johan Hovold On Behalf Of Johan Hovold
Sent: Monday, November 12, 2018 4:00 AM
To: SNELL James
Cc: linux-usb@vger.kernel.org
Subject: Re: Adding NovAtel USB vendor & device ID to Kernel
On Thu, Nov 08, 2018 at 01:07:47AM +, SNELL James wrote:
> Hello,
> We produce extremely high-end GNSS (GPS, etc) receivers that are often
> used for a very wide range of applications. Our receivers can be
> connected to via USB, which will provide 3 USB-to-serial ports that
> can be used to issue commands and get receiver data.
Do your products support other serial interfaces as well (e.g. UART or i2c)?
> We typically get Linux users to create a udev file so their systems
> attach the USB serial ports to /dev.
>
> I just noticed that when my receiver enumerates, dmesg outputs:
> [ 414.374523] usb 1-1.1.3: new full-speed USB device number 8 using
> dwc_otg [ 414.508473] usb 1-1.1.3: New USB device found,
> idVendor=09d7, idProduct=0100 [ 414.508488] usb 1-1.1.3: New USB
> device strings: Mfr=1, Product=2, SerialNumber=3 [ 414.508497] usb
> 1-1.1.3: Product: NovAtel GPS Receiver [ 414.508505] usb 1-1.1.3:
> Manufacturer: NovAtel Inc.
> [ 414.508514] usb 1-1.1.3: SerialNumber: DMGW18050122R [ 414.511608]
> usbserial_generic 1-1.1.3:1.0: The "generic" usb-serial driver is only for
> testing and one-off prototypes.
> [ 414.511624] usbserial_generic 1-1.1.3:1.0: Tell
> mailto:linux-usb@vger.kernel.org to add your device to a proper driver.
> [ 414.511636] usbserial_generic 1-1.1.3:1.0: generic converter
> detected [
Re: [PATCH] usb: gadget: avoid using gadget after freed
On Tue, Jun 18, 2019 at 11:21 PM Felipe Balbi wrote: > > > Hi, > > Lianwei Wang writes: > >> Lianwei Wang writes: > >> > On Mon, Jun 17, 2019 at 5:40 AM Felipe Balbi wrote: > >> >> > >> >> Lianwei Wang writes: > >> >> > >> >> > The udc and gadget device will be deleted when udc device is > >> >> > disconnected and the related function will be unbind with it. > >> >> > > >> >> > But if the configfs is not deleted, then the function object > >> >> > will be kept and the bound status is kept true. > >> >> > > >> >> > Then after udc device is connected again and a new udc and > >> >> > gadget objects will be created and passed to bind interface. > >> >> > But because the bound is still true, the new gadget is not > >> >> > updated to netdev and a previous freed gadget will be used > >> >> > in netdev after bind. > >> >> > > >> >> > To fix this using after freed issue, always set the gadget > >> >> > object to netdev in bind interface. > >> >> > > >> >> > Signed-off-by: Lianwei Wang > >> >> > >> >> I can't actually understand what's the problem here. The gadget is not > >> >> deleted when we disconnect the cable. > >> >> > >> >> -- > >> >> balbi > >> > > >> > The issue was observed with a dual-role capable USB controller (e.g. > >> > Intel > >> > XHCI controller), which has the ability to switch role between host and > >> > device > >> > mode. The gadget is deleted when we switch role to device mode from host > >> > mode. See below log: > >> > # echo p > > >> > /sys/devices/pci:00/:00:15.1/intel-cht-otg.0/mux_state #(4.4) > >> > >> oh, so you're using a modified tree :-) Then we can't really help. > >> > >> > [ 41.170891] intel-cht-otg intel-cht-otg.0: p: set PERIPHERAL mode > >> > [ 41.171895] dwc3 dwc3.0.auto: DWC3 OTG Notify USB_EVENT_VBUS > >> > [ 41.187420] dwc3 dwc3.0.auto: dwc3_resume_common > >> > [ 41.191192] usb 1-1: USB disconnect, device number 3 > >> > [ 41.191284] usb 1-1.1: USB disconnect, device number 4 > >> > [ 41.218958] usb 1-1.5: USB disconnect, device number 5 > >> > [ 41.238117] android_work: sent uevent USB_STATE=CONFIGURED > >> > [ 41.240572] android_work: sent uevent USB_STATE=DISCONNECTED > >> > >> What is this android_work. That doesn't exist upstream. > >> > >> > [ 41.263285] platform dabr_udc.0: unregister gadget driver > >> > 'configfs-gadget' > >> > [ 41.263413] configfs-gadget gadget: unbind function 'Function FS > >> > Gadget'/8801db049e38 > >> > [ 41.263969] configfs-gadget gadget: unbind function > >> > 'cdc_network'/8801d8897400 > >> > [ 41.325943] dabridge 1-1.5:1.0: Port 3 VBUS OFF > >> > [ 41.720957] dabr_udc deleted > >> > [ 41.721097] dabridge 1-5 deleted > >> > > >> > The UDC and gadget will be deleted after switch role to device mode. > >> > And they will be > >> > created as new object when switching back to host mode. At this time > >> > the bind in function > >> > driver (e.g. f_ncm) will not set the new gadget. > >> > > >> > For kernel 4.19+, the role switch command will be: > >> > echo "device" > /sys/class/usb_role/intel_xhci_usb_sw-role-switch/role > >> > > >> > The latest Intel role switch kernel driver can be found here: > >> > > >> > https://elixir.bootlin.com/linux/v5.2-rc5/source/drivers/usb/roles/intel-xhci-usb-role-switch.c > >> > >> Right, please test against v5.2-rc5 and show me the problem on that > >> kernel. I can't apply patches for problems that may not even exist in > >> upstream, sorry. > >> > >> -- > >> balbi > > > > The issue exist in main line kernel, but I can not test it with > > v5.2-rc5 kernel. I tested it with 4.19 kernel, > > which of the v4.19? > > > which, for the usb gadget part, has almost the same code as v5.2. It > > is 100% reproducible with dual role > > USB controller or by removing UDC hardware. Take f_ncm for example, > > the use case as follows: > > Keep in mind that the way android handles dual-role is completely > different from what we have upstream. > Right, Our xchi controller support dual role and normally it works in host mode and the other device, e.g. phone can connect to our system as a gadget device. It works in the same way as PC. > > 1. USB controller is in host mode, f_ncm and UDC is configured in configfs. > >- The ncm is instanced and alloced when "functions/ncm.usb0" is > > created and it will be freed > > when those files are delted from configfs. > > > > 2. enable the gadget and bind it to this ncm function. > > - For the first time running, ncm_opts->bound is 0 and > > gether_set_gadget is called to set the > > gadget. The bound is set to 1 then. > > > > 3. If the UDC is disconnected from bus, then the UDC and its gadget is > > what do you mean by "disconnected from the bus"? Removing the cable > (aka, disconnect) will only cause the ->disconnect() callback to be > called. It will not result in the UDC being freed. Is this, perhaps, > something specific to android? > Our UDC is removable so we can remove the UDC from our syste
[PATCH] usb: Replace snprintf with scnprintf in gether_get_ifname
From: Daniel M German snprintf returns the actual length of the buffer created; however, this is not the case if snprintf truncates its parameter. See https://lwn.net/Articles/69419/ for a detailed explanation. The current code correctly handles this case at the expense of extra code in the return statement. scnprintf does returns the actual length of the buffer created making the ?: operator unnecessary in the return statement. This change does not alter the functionality of the code. Signed-off-by: Daniel M German --- drivers/usb/gadget/function/u_ether.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c index 737bd77a575d..329b4d2861ee 100644 --- a/drivers/usb/gadget/function/u_ether.c +++ b/drivers/usb/gadget/function/u_ether.c @@ -1004,9 +1004,9 @@ int gether_get_ifname(struct net_device *net, char *name, int len) int ret; rtnl_lock(); - ret = snprintf(name, len, "%s\n", netdev_name(net)); + ret = scnprintf(name, len, "%s\n", netdev_name(net)); rtnl_unlock(); - return ret < len ? ret : len; + return ret; } EXPORT_SYMBOL_GPL(gether_get_ifname); -- 2.20.1
Re: [PATCH] usb: gadget: avoid using gadget after freed
Hi, Lianwei Wang writes: > On Tue, Jun 18, 2019 at 11:21 PM Felipe Balbi wrote: >> >> >> Hi, >> >> Lianwei Wang writes: >> >> Lianwei Wang writes: >> >> > On Mon, Jun 17, 2019 at 5:40 AM Felipe Balbi wrote: >> >> >> >> >> >> Lianwei Wang writes: >> >> >> >> >> >> > The udc and gadget device will be deleted when udc device is >> >> >> > disconnected and the related function will be unbind with it. >> >> >> > >> >> >> > But if the configfs is not deleted, then the function object >> >> >> > will be kept and the bound status is kept true. >> >> >> > >> >> >> > Then after udc device is connected again and a new udc and >> >> >> > gadget objects will be created and passed to bind interface. >> >> >> > But because the bound is still true, the new gadget is not >> >> >> > updated to netdev and a previous freed gadget will be used >> >> >> > in netdev after bind. >> >> >> > >> >> >> > To fix this using after freed issue, always set the gadget >> >> >> > object to netdev in bind interface. >> >> >> > >> >> >> > Signed-off-by: Lianwei Wang >> >> >> >> >> >> I can't actually understand what's the problem here. The gadget is not >> >> >> deleted when we disconnect the cable. >> >> >> >> >> >> -- >> >> >> balbi >> >> > >> >> > The issue was observed with a dual-role capable USB controller (e.g. >> >> > Intel >> >> > XHCI controller), which has the ability to switch role between host and >> >> > device >> >> > mode. The gadget is deleted when we switch role to device mode from host >> >> > mode. See below log: >> >> > # echo p > >> >> > /sys/devices/pci:00/:00:15.1/intel-cht-otg.0/mux_state #(4.4) >> >> >> >> oh, so you're using a modified tree :-) Then we can't really help. >> >> >> >> > [ 41.170891] intel-cht-otg intel-cht-otg.0: p: set PERIPHERAL mode >> >> > [ 41.171895] dwc3 dwc3.0.auto: DWC3 OTG Notify USB_EVENT_VBUS >> >> > [ 41.187420] dwc3 dwc3.0.auto: dwc3_resume_common >> >> > [ 41.191192] usb 1-1: USB disconnect, device number 3 >> >> > [ 41.191284] usb 1-1.1: USB disconnect, device number 4 >> >> > [ 41.218958] usb 1-1.5: USB disconnect, device number 5 >> >> > [ 41.238117] android_work: sent uevent USB_STATE=CONFIGURED >> >> > [ 41.240572] android_work: sent uevent USB_STATE=DISCONNECTED >> >> >> >> What is this android_work. That doesn't exist upstream. >> >> >> >> > [ 41.263285] platform dabr_udc.0: unregister gadget driver >> >> > 'configfs-gadget' >> >> > [ 41.263413] configfs-gadget gadget: unbind function 'Function FS >> >> > Gadget'/8801db049e38 >> >> > [ 41.263969] configfs-gadget gadget: unbind function >> >> > 'cdc_network'/8801d8897400 >> >> > [ 41.325943] dabridge 1-1.5:1.0: Port 3 VBUS OFF >> >> > [ 41.720957] dabr_udc deleted >> >> > [ 41.721097] dabridge 1-5 deleted >> >> > >> >> > The UDC and gadget will be deleted after switch role to device mode. >> >> > And they will be >> >> > created as new object when switching back to host mode. At this time >> >> > the bind in function >> >> > driver (e.g. f_ncm) will not set the new gadget. >> >> > >> >> > For kernel 4.19+, the role switch command will be: >> >> > echo "device" > /sys/class/usb_role/intel_xhci_usb_sw-role-switch/role >> >> > >> >> > The latest Intel role switch kernel driver can be found here: >> >> > >> >> > https://elixir.bootlin.com/linux/v5.2-rc5/source/drivers/usb/roles/intel-xhci-usb-role-switch.c >> >> >> >> Right, please test against v5.2-rc5 and show me the problem on that >> >> kernel. I can't apply patches for problems that may not even exist in >> >> upstream, sorry. >> >> >> >> -- >> >> balbi >> > >> > The issue exist in main line kernel, but I can not test it with >> > v5.2-rc5 kernel. I tested it with 4.19 kernel, >> >> which of the v4.19? >> >> > which, for the usb gadget part, has almost the same code as v5.2. It >> > is 100% reproducible with dual role >> > USB controller or by removing UDC hardware. Take f_ncm for example, >> > the use case as follows: >> >> Keep in mind that the way android handles dual-role is completely >> different from what we have upstream. >> > Right, Our xchi controller support dual role and normally it works in host > mode > and the other device, e.g. phone can connect to our system as a gadget device. > It works in the same way as PC. > >> > 1. USB controller is in host mode, f_ncm and UDC is configured in configfs. >> >- The ncm is instanced and alloced when "functions/ncm.usb0" is >> > created and it will be freed >> > when those files are delted from configfs. >> > >> > 2. enable the gadget and bind it to this ncm function. >> > - For the first time running, ncm_opts->bound is 0 and >> > gether_set_gadget is called to set the >> > gadget. The bound is set to 1 then. >> > >> > 3. If the UDC is disconnected from bus, then the UDC and its gadget is >> >> what do you mean by "disconnected from the bus"? Removing the cable >> (aka, disconnect) will only cause the ->disconnect() callback to be >> called. I
Re: [PATCH] usb: gadget: avoid using gadget after freed
On Wed, Jun 19, 2019 at 10:55 PM Felipe Balbi wrote: > > > Hi, > > Lianwei Wang writes: > > > On Tue, Jun 18, 2019 at 11:21 PM Felipe Balbi wrote: > >> > >> > >> Hi, > >> > >> Lianwei Wang writes: > >> >> Lianwei Wang writes: > >> >> > On Mon, Jun 17, 2019 at 5:40 AM Felipe Balbi wrote: > >> >> >> > >> >> >> Lianwei Wang writes: > >> >> >> > >> >> >> > The udc and gadget device will be deleted when udc device is > >> >> >> > disconnected and the related function will be unbind with it. > >> >> >> > > >> >> >> > But if the configfs is not deleted, then the function object > >> >> >> > will be kept and the bound status is kept true. > >> >> >> > > >> >> >> > Then after udc device is connected again and a new udc and > >> >> >> > gadget objects will be created and passed to bind interface. > >> >> >> > But because the bound is still true, the new gadget is not > >> >> >> > updated to netdev and a previous freed gadget will be used > >> >> >> > in netdev after bind. > >> >> >> > > >> >> >> > To fix this using after freed issue, always set the gadget > >> >> >> > object to netdev in bind interface. > >> >> >> > > >> >> >> > Signed-off-by: Lianwei Wang > >> >> >> > >> >> >> I can't actually understand what's the problem here. The gadget is > >> >> >> not > >> >> >> deleted when we disconnect the cable. > >> >> >> > >> >> >> -- > >> >> >> balbi > >> >> > > >> >> > The issue was observed with a dual-role capable USB controller (e.g. > >> >> > Intel > >> >> > XHCI controller), which has the ability to switch role between host > >> >> > and device > >> >> > mode. The gadget is deleted when we switch role to device mode from > >> >> > host > >> >> > mode. See below log: > >> >> > # echo p > > >> >> > /sys/devices/pci:00/:00:15.1/intel-cht-otg.0/mux_state #(4.4) > >> >> > >> >> oh, so you're using a modified tree :-) Then we can't really help. > >> >> > >> >> > [ 41.170891] intel-cht-otg intel-cht-otg.0: p: set PERIPHERAL mode > >> >> > [ 41.171895] dwc3 dwc3.0.auto: DWC3 OTG Notify USB_EVENT_VBUS > >> >> > [ 41.187420] dwc3 dwc3.0.auto: dwc3_resume_common > >> >> > [ 41.191192] usb 1-1: USB disconnect, device number 3 > >> >> > [ 41.191284] usb 1-1.1: USB disconnect, device number 4 > >> >> > [ 41.218958] usb 1-1.5: USB disconnect, device number 5 > >> >> > [ 41.238117] android_work: sent uevent USB_STATE=CONFIGURED > >> >> > [ 41.240572] android_work: sent uevent USB_STATE=DISCONNECTED > >> >> > >> >> What is this android_work. That doesn't exist upstream. > >> >> > >> >> > [ 41.263285] platform dabr_udc.0: unregister gadget driver > >> >> > 'configfs-gadget' > >> >> > [ 41.263413] configfs-gadget gadget: unbind function 'Function FS > >> >> > Gadget'/8801db049e38 > >> >> > [ 41.263969] configfs-gadget gadget: unbind function > >> >> > 'cdc_network'/8801d8897400 > >> >> > [ 41.325943] dabridge 1-1.5:1.0: Port 3 VBUS OFF > >> >> > [ 41.720957] dabr_udc deleted > >> >> > [ 41.721097] dabridge 1-5 deleted > >> >> > > >> >> > The UDC and gadget will be deleted after switch role to device mode. > >> >> > And they will be > >> >> > created as new object when switching back to host mode. At this time > >> >> > the bind in function > >> >> > driver (e.g. f_ncm) will not set the new gadget. > >> >> > > >> >> > For kernel 4.19+, the role switch command will be: > >> >> > echo "device" > > >> >> > /sys/class/usb_role/intel_xhci_usb_sw-role-switch/role > >> >> > > >> >> > The latest Intel role switch kernel driver can be found here: > >> >> > > >> >> > https://elixir.bootlin.com/linux/v5.2-rc5/source/drivers/usb/roles/intel-xhci-usb-role-switch.c > >> >> > >> >> Right, please test against v5.2-rc5 and show me the problem on that > >> >> kernel. I can't apply patches for problems that may not even exist in > >> >> upstream, sorry. > >> >> > >> >> -- > >> >> balbi > >> > > >> > The issue exist in main line kernel, but I can not test it with > >> > v5.2-rc5 kernel. I tested it with 4.19 kernel, > >> > >> which of the v4.19? > >> > >> > which, for the usb gadget part, has almost the same code as v5.2. It > >> > is 100% reproducible with dual role > >> > USB controller or by removing UDC hardware. Take f_ncm for example, > >> > the use case as follows: > >> > >> Keep in mind that the way android handles dual-role is completely > >> different from what we have upstream. > >> > > Right, Our xchi controller support dual role and normally it works in host > > mode > > and the other device, e.g. phone can connect to our system as a gadget > > device. > > It works in the same way as PC. > > > >> > 1. USB controller is in host mode, f_ncm and UDC is configured in > >> > configfs. > >> >- The ncm is instanced and alloced when "functions/ncm.usb0" is > >> > created and it will be freed > >> > when those files are delted from configfs. > >> > > >> > 2. enable the gadget and bind it to this ncm function. > >> > - For the first time running, ncm_opts->bound is 0 an
