Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Oliver Neukum]

Am Dienstag, den 25.06.2019, 10:08 -0400 schrieb Alan Stern:
> On Tue, 25 Jun 2019, Mayuresh Kulkarni wrote:

> > Right, so user space should do the following when it determines the
> > device is idle from its point of view -
> > 
> > 1. Call ALLOW_SUSPEND ioctl

That is a race in principle. You should reverse steps 1 and 2

> > 2. Queue an URB and wait for its REAP. When the wait returns -EFAIL (or
> > something similar), that is the indication that the device is no longer
> > active (or suspended)
> > 3. Call WAIT_FOR_RESUME ioctl

It seems to me that there ought to be one API for that. Either an
ioctl or poll.

> > 4. When WAIT_FOR_RESUME ioctl returns, it is guaranteed that device is
> > active.
> > 5. Call FORBID_SUSPEND ioctl and read the cause of resume.
> > 6. Go to (1) when appropriate
> > 
> > Have I summarized this approach correctly from user-space point of view?
> 
> Yes, except for one thing: In step 4, it is _not_ guaranteed that the 
> device is active when WAIT_FOR_RESUME returns.  The only guarantee is 
> that a resume did occur sometime after step 1, but the device might 
> have gone back into suspend after that occurred.

Now is that a good API? Shouldn't we rather have an API for either
* resume the device now and bump the counter
* bump the counter when te device resumes

I don't see a value in not having a defined power state after resume.

> > 3. Extend usbdev_poll() to wait for udev->state == USB_STATE_SUSPENDED
> > when events == POLLPRI. Return POLLPRI when state = USB_STATE_SUSPENDED.
> > 4. As before, any ioctl != (ALLOW_SUSPEND or WAIT_FOR_RESUME)
> > calls usb_autoresume_device().
> 
> 3 sounds reasonable at first, but I'm not sure it would work.  
> Consider what would happen if the device is suspended very briefly and
> then wakes up.  The usbdev_poll() call might not return, because by the
> time it checks udev->state, the state has already changed back to
> USB_STATE_CONFIGURED.

Indeed. It seems to me that any power transition should be reported
back.

> In any case, we shouldn't do 4.  It would prevent the device from ever
> going into suspend, because the program would want to continue making
> usbfs ioctl calls while waiting for the suspend to occur.

Exactly.

> > The corresponding user-space calls would be -
> > A. When determined device is idle, call ALLOW_SUSPEND ioctl.
> > B. Call poll(device_fd, POLLPRI). When poll returns check revents
> > == POLLPRI.
> 
> What if the device never does go into suspend?  The poll() call
> wouldn't return and the program would just stop working.

Well, that is why you can poll for multiple events at the same
time and the syscall has a timeout.

> > 2. Is it safe to wait for udev->state to be of a particular value?
> 
> No, not really, because the state can change.

That is a general issue with poll. You cannot be sure that there
is still data to be read when you are ready to read.

Regards
Oliver

[PATCH] imx: usb: get pinctrl if it's not yet initialized

[Krzysztof Michonski]

In case usb phy mode is other than USBPHY_INTERFACE_MODE_HSIC
the pinctrl for device is not acquired. It is however used later
regardless of the mode, hence leads to requesting access to
uninitialized data.

Signed-off-by: Krzysztof Michonski 
---
 drivers/usb/chipidea/ci_hdrc_imx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c 
b/drivers/usb/chipidea/ci_hdrc_imx.c
index a4b482c3dc65..2f02b35c40b6 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -428,6 +428,9 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
}
 
+   if (!data->pinctrl)
+   data->pinctrl = devm_pinctrl_get(dev);
+
if (!IS_ERR(data->pinctrl)) {
struct pinctrl_state *state;
 
-- 
2.21.0

Le attività insolite rilevate nel tuo account confermano gentilmente il tuo account per ricevere la posta in arrivo in arrivo

[Giuliana.Bacia]

VERIFICA WEBMAIL


Gentile utente di Webmail,

Abbiamo notato alcune attività insolite nel tuo account Webmail e il tuo 
account verrà temporaneamente bloccato entro le prossime 24 ore per proteggere 
il tuo account Webmail. Ciò potrebbe essere accaduto perché qualcuno potrebbe 
aver utilizzato il tuo account per inviare un sacco di email spazzatura o 
qualcos'altro che vìola i nostri Termini di servizio.

Tutte le tue e-mail in arrivo sono state messe in sospeso, per favore 
assicurati di   FARE CLIC QUI
   per verificare il tuo account per ricevere le e-mail in arrivo.

Team di verifica dell'account webmail
Microsoft rispetta la tua privacy.

Copyright © 2019 Webmail Inc. Tutti i diritti riservati.

Re: [PATCH] gpss: core: no waiters left behind on deregister

[Oliver Neukum]

Am Dienstag, den 25.06.2019, 09:04 +0200 schrieb Johan Hovold:
> On Mon, Jun 24, 2019 at 10:33:23AM +0200, Oliver Neukum wrote:
> > If you deregister a device you need to wake up all waiters
> > as there will be no further wakeups.
> > 
> > Signed-off-by: Oliver Neukum 
> > ---
> >  drivers/gnss/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c
> > index e6f94501cb28..0d13bd2cefd5 100644
> > --- a/drivers/gnss/core.c
> > +++ b/drivers/gnss/core.c
> > @@ -303,7 +303,7 @@ void gnss_deregister_device(struct gnss_device *gdev)
> > down_write(&gdev->rwsem);
> > gdev->disconnected = true;
> > if (gdev->count) {
> > -   wake_up_interruptible(&gdev->read_queue);
> > +   wake_up_interruptible_all(&gdev->read_queue);
> 
> GNSS core doesn't have any exclusive waiters, so no need to use use the
> exclusive wake-up (all) interface.

Well, yes, but that is the problem. In gnss_read() you drop the lock:

mutex_lock(&gdev->read_mutex);

while (kfifo_is_empty(&gdev->read_fifo)) {
mutex_unlock(&gdev->read_mutex);

if (gdev->disconnected)
return 0;

if (file->f_flags & O_NONBLOCK)
return -EAGAIN;

That means that an arbitrary number of tasks can get here.

ret = wait_event_interruptible(gdev->read_queue,
gdev->disconnected ||
!kfifo_is_empty(&gdev->read_fifo));

Meaning that an arbitrary number can be sleeping here. Yet in
gnss_deregister_device() you use a simple wake_up:

void gnss_deregister_device(struct gnss_device *gdev)

{

down_write(&gdev->rwsem);
gdev->disconnected = true;
if (gdev->count) {
wake_up_interruptible(&gdev->read_queue);


wake_up_interruptible() will wake up one waiting task. But after that
the device is gone. There will be no further events. The other tasks
will sleep forever.

Regards
Oliver

Re: [PATCH] gpss: core: no waiters left behind on deregister

[Johan Hovold]

On Wed, Jun 26, 2019 at 01:04:07PM +0200, Oliver Neukum wrote:
> Am Dienstag, den 25.06.2019, 09:04 +0200 schrieb Johan Hovold:
> > On Mon, Jun 24, 2019 at 10:33:23AM +0200, Oliver Neukum wrote:
> > > If you deregister a device you need to wake up all waiters
> > > as there will be no further wakeups.
> > > 
> > > Signed-off-by: Oliver Neukum 
> > > ---
> > >  drivers/gnss/core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c
> > > index e6f94501cb28..0d13bd2cefd5 100644
> > > --- a/drivers/gnss/core.c
> > > +++ b/drivers/gnss/core.c
> > > @@ -303,7 +303,7 @@ void gnss_deregister_device(struct gnss_device *gdev)
> > >   down_write(&gdev->rwsem);
> > >   gdev->disconnected = true;
> > >   if (gdev->count) {
> > > - wake_up_interruptible(&gdev->read_queue);
> > > + wake_up_interruptible_all(&gdev->read_queue);
> > 
> > GNSS core doesn't have any exclusive waiters, so no need to use use the
> > exclusive wake-up (all) interface.
> 
> Well, yes, but that is the problem. In gnss_read() you drop the lock:

> That means that an arbitrary number of tasks can get here.
> 
> ret = wait_event_interruptible(gdev->read_queue,
> gdev->disconnected ||
> !kfifo_is_empty(&gdev->read_fifo));
> 
> Meaning that an arbitrary number can be sleeping here.

I understand wait you're getting at, but I think your mistaken regarding
exclusive wait. Note that wait_event_interruptible() uses nonexclusive
wait.

> Yet in gnss_deregister_device() you use a simple wake_up:
> 
> void gnss_deregister_device(struct gnss_device *gdev)
> 
> {
> 
> down_write(&gdev->rwsem);
> gdev->disconnected = true;
> if (gdev->count) {
> wake_up_interruptible(&gdev->read_queue);
> 
> 
> wake_up_interruptible() will wake up one waiting task. But after that
> the device is gone. There will be no further events. The other tasks
> will sleep forever.

No, wake_up_interruptible() will wake up all nonexclusive waiters,
which is all we care about here.

Johan

No carrier lost information with gadget RNDIS/ECM

[Kai Ruhnau]

Hi,

On my i.MX6 SoloX, I have configured one of the OTG ports for a combined 
RNDIS/ECM gadget. After boot, I have two network interfaces (usb0 and usb1) 
which are managed by systemd-networkd.

With kernel 4.9.153, systemd-networkd reports an immediate carrier loss when I 
pull the USB cable from a Windows or macOS host. With 4.19.53 or 5.1.15 that 
carrier loss is only reported when I re-attach the cable, meaning there is a 
"Lost carrier" for the last used interface immediately followed by a "Gained 
carrier" for the newly connected interface.

I have activated CONFIG_USB_GADGET_DEBUG_FILES, and the contents of 
/proc/driver/rndis-000 don't change when I pull the cable:
Config Nr. 0
used  : y
state : RNDIS_DATA_INITIALIZED
medium: 0x
speed : 425984000
cable : connected
vendor ID : 0x
vendor: (null)

Only when changing the host to a Mac, it's different:
Config Nr. 0
used  : y
state : RNDIS_UNINITIALIZED
medium: 0x
speed : 425984000
cable : connected
vendor ID : 0x
vendor: (null)

Thanks for any help.

Cheers,
Kai

Re: No carrier lost information with gadget RNDIS/ECM

[Felipe Balbi]

Hi,

Kai Ruhnau  writes:
> On my i.MX6 SoloX, I have configured one of the OTG ports for a
> combined RNDIS/ECM gadget. After boot, I have two network interfaces
> (usb0 and usb1) which are managed by systemd-networkd.
>
> With kernel 4.9.153, systemd-networkd reports an immediate carrier
> loss when I pull the USB cable from a Windows or macOS host. With
> 4.19.53 or 5.1.15 that carrier loss is only reported when I re-attach
> the cable, meaning there is a "Lost carrier" for the last used
> interface immediately followed by a "Gained carrier" for the newly
> connected interface.

First of all, thanks for actually testing the most recent stable
kernels. Much appreciated :-)

> I have activated CONFIG_USB_GADGET_DEBUG_FILES, and the contents of
> /proc/driver/rndis-000 don't change when I pull the cable:
>
> Config Nr. 0
> used  : y
> state : RNDIS_DATA_INITIALIZED
> medium: 0x
> speed : 425984000
> cable : connected
> vendor ID : 0x
> vendor: (null)
>
> Only when changing the host to a Mac, it's different:
> Config Nr. 0
> used  : y
> state : RNDIS_UNINITIALIZED
> medium: 0x
> speed : 425984000
> cable : connected
> vendor ID : 0x
> vendor: (null)
>
> Thanks for any help.

Which peripheral controller is this board using? Is it chipidea? dwc2?
dwc3? High Speed or Super Speed?

-- 
balbi

RE: No carrier lost information with gadget RNDIS/ECM

[Kai Ruhnau]

Hi,

Felipe Balbi writes:
> Kai Ruhnau  writes:
>> On my i.MX6 SoloX, I have configured one of the OTG ports for a 
>> combined RNDIS/ECM gadget. After boot, I have two network interfaces
>> (usb0 and usb1) which are managed by systemd-networkd.
>>
>> With kernel 4.9.153, systemd-networkd reports an immediate carrier 
>> loss when I pull the USB cable from a Windows or macOS host. With
>> 4.19.53 or 5.1.15 that carrier loss is only reported when I re-attach 
>> the cable, meaning there is a "Lost carrier" for the last used 
>> interface immediately followed by a "Gained carrier" for the newly 
>> connected interface.
>
> First of all, thanks for actually testing the most recent stable kernels. 
> Much appreciated :-)

Sure. Having so much support for the i.MX6 in mainline helps a lot :)

>> I have activated CONFIG_USB_GADGET_DEBUG_FILES, and the contents of
>> /proc/driver/rndis-000 don't change when I pull the cable:
>>
>> Config Nr. 0
>> used  : y
>> state : RNDIS_DATA_INITIALIZED
>> medium: 0x
>> speed : 425984000
>> cable : connected
>> vendor ID : 0x
>> vendor: (null)
>>
>> Only when changing the host to a Mac, it's different:
>> Config Nr. 0
>> used  : y
>> state : RNDIS_UNINITIALIZED
>> medium: 0x
>> speed : 425984000
>> cable : connected
>> vendor ID : 0x
>> vendor: (null)
>>
>> Thanks for any help.
>
> Which peripheral controller is this board using? Is it chipidea? dwc2?
> dwc3? High Speed or Super Speed?

According to the device tree it's 'fsl,imx6sx-usb' driven by 
chipidea/ci_hdrc_imx.c
When connecting to Windows, the dmesg shows:
 configfs-gadget gadget: high-speed config #2: c

Cheers,
Kai

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni]

On Tue, 2019-06-25 at 10:08 -0400, Alan Stern wrote:
> On Tue, 25 Jun 2019, Mayuresh Kulkarni wrote:
> 
> > 
> > > 
> > > There are two possible ways a userspace program can monitor the 
> > > device's state:
> > > 
> > > 1.The program can leave an URB (typically on an
> > > interrupt 
> > >   endpoint) running constantly, and the device can send its 
> > >   response to the URB whenever something happens.
> > > 
> > > 2.The program can poll the device by submitting an
> > > URB 
> > >   periodically to see if anything has happened since the last 
> > >   poll.
> > > 
> > > In case 1, the program would leave the URB running even after
> > > doing
> > > the 
> > > ALLOW_SUSPEND ioctl.  That way the program will be aware of
> > > anything 
> > > that happens to the device before it suspends.  When the device
> > > does
> > > go 
> > > into suspend, the URB will be terminated.
> > > 
> > > In case 2, the program would continue polling the device even
> > > after 
> > > doing the ALLOW_SUSPEND ioctl.  When the device does go into
> > > suspend, 
> > > the polling URB will fail.
> > > 
> > Right, so user space should do the following when it determines the
> > device is idle from its point of view -
> > 
> > 1. Call ALLOW_SUSPEND ioctl
> > 2. Queue an URB and wait for its REAP. When the wait returns -EFAIL
> > (or
> > something similar), that is the indication that the device is no
> > longer
> > active (or suspended)
> > 3. Call WAIT_FOR_RESUME ioctl
> > 4. When WAIT_FOR_RESUME ioctl returns, it is guaranteed that device
> > is
> > active.
> > 5. Call FORBID_SUSPEND ioctl and read the cause of resume.
> > 6. Go to (1) when appropriate
> > 
> > Have I summarized this approach correctly from user-space point of
> > view?
> Yes, except for one thing: In step 4, it is _not_ guaranteed that the 
> device is active when WAIT_FOR_RESUME returns.  The only guarantee is 
> that a resume did occur sometime after step 1, but the device might 
> have gone back into suspend after that occurred.
> 

Right, OK.

> And note that step 2 (queuing an URB) is something your program would
> do anyway, even if the device wasn't going to be suspended or wasn't
> idle.
> 

Yes, got it now.

> 
> > 
> > Based on discussion so far and my understanding, how about below
> > approach -
> > 
> > 1. Have ALLOW_SUSPEND and WAIT_FOR_RESUME ioctls. As before,
> > ALLOW_SUSPEND calls usb_autosuspend_device() while WAIT_FOR_RESUME
> > waits
> > for resume.
> > 2. Have usbfs_notify_suspend() and usbfs_notify_resume() as per your
> > previous patch (i.e.: system/resume callbacks at device level).
> > 3. Extend usbdev_poll() to wait for udev->state ==
> > USB_STATE_SUSPENDED
> > when events == POLLPRI. Return POLLPRI when state =
> > USB_STATE_SUSPENDED.
> > 4. As before, any ioctl != (ALLOW_SUSPEND or WAIT_FOR_RESUME)
> > calls usb_autoresume_device().
> 3 sounds reasonable at first, but I'm not sure it would work.  
> Consider what would happen if the device is suspended very briefly and
> then wakes up.  The usbdev_poll() call might not return, because by
> the
> time it checks udev->state, the state has already changed back to
> USB_STATE_CONFIGURED.
> 

I see what you mean here, could be problematic.

> In any case, we shouldn't do 4.  It would prevent the device from ever
> going into suspend, because the program would want to continue making
> usbfs ioctl calls while waiting for the suspend to occur.
> 

In poll approach, due to the contraint I mentioned, it is not expected
from user-space to interact with device *after* it calls ALLOW_SUSPEND
ioctl. This is because, it has determined that device is idle from its
point of view.

But after a while, there could be a situation where the user-space wants
to send some message to device (due to end user interaction) then,
having (4) would be handy to cancel the undone suspend or cause host-
wake.

> > 
> > The corresponding user-space calls would be -
> > A. When determined device is idle, call ALLOW_SUSPEND ioctl.
> > B. Call poll(device_fd, POLLPRI). When poll returns check revents
> > == POLLPRI.
> What if the device never does go into suspend?  The poll() call
> wouldn't return and the program would just stop working.
> 
> > 
> > C. Call WAIT_FOR_RESUME ioctl.
> > D. When WAIT_FOR_RESUME ioctl returns, read resume reason.
> > E. Go to (A).
> > 
> > The constraint on (1) above is - ALLOW_SUSPEND should be called when
> > user-space decides device is idle. I think this is not a hard
> > constraint
> > since poll() for suspend will return POLLPRI when device is
> > suspended
> > which is different from what it returns when ASYNC URB is completed.
> > 
> > Few points I am unsure of are -
> > 1. Is it OK for a driver to re-purpose POLLPRI for its own use
> > or POLLPRI has a unique meaning system wide?
> POLLPRI does not have a unique system-wide meaning.  We could use it
> to 
> indicate the device is suspended, if we want to.  But I'm not
> convinced 
> that it would be a good ide

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Wed, 26 Jun 2019, Oliver Neukum wrote:

> Am Dienstag, den 25.06.2019, 10:08 -0400 schrieb Alan Stern:
> > On Tue, 25 Jun 2019, Mayuresh Kulkarni wrote:
> 
> > > Right, so user space should do the following when it determines the
> > > device is idle from its point of view -
> > > 
> > > 1. Call ALLOW_SUSPEND ioctl
> 
> That is a race in principle. You should reverse steps 1 and 2

I disagree.  First, it's not really a race.  It doesn't matter whether
the URB error occurs during completion or during submission; either way
the program will become aware that the device has suspended.

Second, in all likelihood the program will be submitting URBs more or
less continually anyway.  Think of an HID driver, for example.  It
always keeps an interrupt URB active so that it can monitor what is
happening to the device.

If you really think it would be better, we could add an IS_SUSPENDED
ioctl so that the program can poll for state changes.  But that seems
like a pretty bad approach to me.  We could even add a WAIT_FOR_SUSPEND
ioctl, but programs basically will never want to wait for a suspend to
occur.

> > > 2. Queue an URB and wait for its REAP. When the wait returns -EFAIL (or
> > > something similar), that is the indication that the device is no longer
> > > active (or suspended)
> > > 3. Call WAIT_FOR_RESUME ioctl
> 
> It seems to me that there ought to be one API for that. Either an
> ioctl or poll.

Isn't a WAIT_FOR_RESUME ioctl one API?

In general, an ioctl is more flexible than poll -- poll can report only
a very limited number of types of state change.

> > > 4. When WAIT_FOR_RESUME ioctl returns, it is guaranteed that device is
> > > active.
> > > 5. Call FORBID_SUSPEND ioctl and read the cause of resume.
> > > 6. Go to (1) when appropriate
> > > 
> > > Have I summarized this approach correctly from user-space point of view?
> > 
> > Yes, except for one thing: In step 4, it is _not_ guaranteed that the 
> > device is active when WAIT_FOR_RESUME returns.  The only guarantee is 
> > that a resume did occur sometime after step 1, but the device might 
> > have gone back into suspend after that occurred.
> 
> Now is that a good API? Shouldn't we rather have an API for either
> * resume the device now and bump the counter
> * bump the counter when te device resumes

It makes the code cleaner: The usage counter is incremented by the
FORBID_SUSPEND ioctl and decremented by the ALLOW_SUSPEND ioctl.  
Nothing else.  Now you're suggesting that WAIT_FOR_RESUME should also
increment the usage counter upon return.  Programs may not even want
that behavior (consider the possibility that WAIT_FOR_RESUME was
interrupted by a signal).

In any case, we do need two separate APIs: one to resume the device
immediately, and one to wait for a resume to occur.  Programs will want 
to do both these things.

> I don't see a value in not having a defined power state after resume.

Well, we also don't have a defined power state after ALLOW_SUSPEND.  I
don't see this as a problem.  It's more of a necessity, in fact --
there's no way to keep a userspace program fully up to date on the
device's state, because suspend and resume events are asynchronous.

> > > 3. Extend usbdev_poll() to wait for udev->state == USB_STATE_SUSPENDED
> > > when events == POLLPRI. Return POLLPRI when state = USB_STATE_SUSPENDED.
> > > 4. As before, any ioctl != (ALLOW_SUSPEND or WAIT_FOR_RESUME)
> > > calls usb_autoresume_device().
> > 
> > 3 sounds reasonable at first, but I'm not sure it would work.  
> > Consider what would happen if the device is suspended very briefly and
> > then wakes up.  The usbdev_poll() call might not return, because by the
> > time it checks udev->state, the state has already changed back to
> > USB_STATE_CONFIGURED.
> 
> Indeed. It seems to me that any power transition should be reported
> back.

Sorry, I don't understand: Reported back where?  Keep in mind that it
is not feasible to report every transition to a userspace program.  
And even if you somehow did, the last reported value would be out of
date as soon as the program received it.

> > In any case, we shouldn't do 4.  It would prevent the device from ever
> > going into suspend, because the program would want to continue making
> > usbfs ioctl calls while waiting for the suspend to occur.
> 
> Exactly.
> 
> > > The corresponding user-space calls would be -
> > > A. When determined device is idle, call ALLOW_SUSPEND ioctl.
> > > B. Call poll(device_fd, POLLPRI). When poll returns check revents
> > > == POLLPRI.
> > 
> > What if the device never does go into suspend?  The poll() call
> > wouldn't return and the program would just stop working.
> 
> Well, that is why you can poll for multiple events at the same
> time and the syscall has a timeout.

The events that poll reports on are really targeted toward
communication channels (e.g., data available to read, high-priority
data received, channel closed), not suspend/resume state changes.  
Poll really wasn't meant fo

Re: Not enough bandwidth with Magewell XI100DUSB-HDMI

[Philipp Zabel]

On Mon, 2019-06-24 at 16:40 +0200, Philipp Zabel wrote:
> Hi,
> 
> On Mon, 2018-05-21 at 15:03 -0700, Curt Meyers wrote:
> > On 05/16/2018 04:55 AM, Mathias Nyman wrote:
> > > On 15.05.2018 12:22, Michael Tretter wrote:
> > > > Hi Mathias,
> > > > 
> > > > On Tue, 10 Apr 2018 18:22:03 +0300, Mathias Nyman wrote:
> > > > > On 09.04.2018 11:21, Michael Tretter wrote:
> > > > > > On Tue, 20 Feb 2018 15:29:28 +0200, Mathias Nyman wrote:
> > > > > > > On 16.02.2018 15:28, Michael Tretter wrote:
> > > > > > > > On Mon, 29 Jan 2018 14:02:57 +0200, Mathias Nyman wrote:
> > > > > > > > > On 19.01.2018 22:12, Philipp Zabel wrote:
> > > > > > > > > > On Fri, Jan 19, 2018 at 2:10 PM, Michael Tretter
> > > > > > > > > >  wrote:
> > > > > > > > > > > I found the old thread and it sounds exactly like my 
> > > > > > > > > > > issue. 
> > > > > > > > > > > Different
> > > > > > > > > > > camera, but same xHCI controller.
> > > > > > > > > > 
> > > > > > > > > > I have exactly the same issue with the xHCI controller of 
> > > > > > > > > > my 
> > > > > > > > > > laptop and
> > > > > > > > > > "Oculus Sensor" USB3 isochronous mostly-UVC cameras:
> > > > > > > > > > 
> > > > > > > > > > 00:14.0 USB controller: Intel Corporation Wildcat Point-LP 
> > > > > > > > > > USB 
> > > > > > > > > > xHCI
> > > > > > > > > > Controller (rev 03) (prog-if 30 [XHCI])
> > > > > > > > > >     Subsystem: Lenovo Wildcat Point-LP USB xHCI 
> > > > > > > > > > Controller
> > > > > > > > > >     Flags: bus master, medium devsel, latency 0, IRQ 44
> > > > > > > > > >     Memory at f222 (64-bit, non-prefetchable) 
> > > > > > > > > > [size=64K]
> > > > > > > > > >     Capabilities: [70] Power Management version 2
> > > > > > > > > >     Capabilities: [80] MSI: Enable+ Count=1/8 Maskable- 
> > > > > > > > > > 64bit+
> > > > > > > > > >     Kernel driver in use: xhci_hcd
> > > > > > > > > >     Kernel modules: xhci_pci
> > > > > > > > > > 
> > > > > > > > > > T:  Bus=02 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  4 Spd=5000 
> > > > > > > > > > MxCh= 0
> > > > > > > > > > D:  Ver= 3.00 Cls=ef(misc ) Sub=02 Prot=01 MxPS= 9 #Cfgs=  1
> > > > > > > > > > P:  Vendor=2833 ProdID=0211 Rev= 0.00
> > > > > > > > > > S:  Manufacturer=Oculus VR
> > > > > > > > > > S:  Product=Rift Sensor
> > > > > > > > > > S:  SerialNumber=WMTD3034300BCT
> > > > > > > > > > C:* #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=800mA
> > > > > > > > > > A:  FirstIf#= 0 IfCount= 2 Cls=ff(vend.) Sub=03 Prot=00
> > > > > > > > > > I:* If#= 0 Alt= 0 #EPs= 1 Cls=ff(vend.) Sub=01 Prot=00 
> > > > > > > > > > Driver=uvcvideo
> > > > > > > > > > E:  Ad=83(I) Atr=03(Int.) MxPS=  32 Ivl=128ms
> > > > > > > > > > I:* If#= 1 Alt= 0 #EPs= 0 Cls=ff(vend.) Sub=02 Prot=00 
> > > > > > > > > > Driver=uvcvideo
> > > > > > > > > > I:  If#= 1 Alt= 1 #EPs= 1 Cls=ff(vend.) Sub=02 Prot=00 
> > > > > > > > > > Driver=uvcvideo
> > > > > > > > > > E:  Ad=81(I) Atr=05(Isoc) MxPS=1024 Ivl=125us
> > > > > > > > > > I:  If#= 1 Alt= 2 #EPs= 1 Cls=ff(vend.) Sub=02 Prot=00 
> > > > > > > > > > Driver=uvcvideo
> > > > > > > > > > E:  Ad=81(I) Atr=05(Isoc) MxPS=1024 Ivl=125us
> > > > > > > > > > 
> > > > > > > > > > Any USB2 or USB3 device that I plug in while the first 
> > > > > > > > > > camera 
> > > > > > > > > > is streaming in
> > > > > > > > > > altsetting 1 or 2 causes the bandwidth error. The same 
> > > > > > > > > > happens 
> > > > > > > > > > when I try to
> > > > > > > > > > change the altsetting on an isochronous endpoint of an 
> > > > > > > > > > already 
> > > > > > > > > > plugged device.
> > > > > > > > > > While the camera is in altsetting 0, other devices can be 
> > > > > > > > > > probed and work.
> > > > > > > > > > > For some tests, I changed the 
> > > > > > > > > > > xhci_change_max_exit_latency() 
> > > > > > > > > > > function
> > > > > > > > > > > to ignore the requested MEL and set the MEL to 0. Now the 
> > > > > > > > > > > USB 
> > > > > > > > > > > devices
> > > > > > > > > > > are detected correctly.
> > > > > > > > > > 
> > > > > > > > > > Exactly the same thing helps here, as well. With this hack, 
> > > > > > > > > > streaming from two
> > > > > > > > > > of those cameras at the same time works without any 
> > > > > > > > > > apparent 
> > > > > > > > > > problem:
> > > > > > > > > > 
> > > > > > > > > > --8<--
> > > > > > > > > > diff --git a/drivers/usb/host/xhci.c 
> > > > > > > > > > b/drivers/usb/host/xhci.c
> > > > > > > > > > index 297536c9fd00..3cb4a60d8822 100644
> > > > > > > > > > --- a/drivers/usb/host/xhci.c
> > > > > > > > > > +++ b/drivers/usb/host/xhci.c
> > > > > > > > > > @@ -4025,7 +4025,7 @@ static int __maybe_unused
> > > > > > > > > > xhci_change_max_exit_latency(struct xhci_hcd *xhci,
> > > > > > > > > >     ctrl_ctx->add_flags |= cpu_to_le32(SLOT_FLAG);
> > > > > > > > > >     slot_ctx = xhci_get_slot_ctx(xhci, 
> > > > > > > > > > command->in_ctx);
> > > > > > > > > >     slot_ctx->dev_info2 &= cpu_t

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Wed, 26 Jun 2019, Mayuresh Kulkarni wrote:

> On Tue, 2019-06-25 at 10:08 -0400, Alan Stern wrote:

> > > Based on discussion so far and my understanding, how about below
> > > approach -
> > > 
> > > 1. Have ALLOW_SUSPEND and WAIT_FOR_RESUME ioctls. As before,
> > > ALLOW_SUSPEND calls usb_autosuspend_device() while WAIT_FOR_RESUME
> > > waits
> > > for resume.
> > > 2. Have usbfs_notify_suspend() and usbfs_notify_resume() as per your
> > > previous patch (i.e.: system/resume callbacks at device level).
> > > 3. Extend usbdev_poll() to wait for udev->state ==
> > > USB_STATE_SUSPENDED
> > > when events == POLLPRI. Return POLLPRI when state =
> > > USB_STATE_SUSPENDED.
> > > 4. As before, any ioctl != (ALLOW_SUSPEND or WAIT_FOR_RESUME)
> > > calls usb_autoresume_device().
> > 3 sounds reasonable at first, but I'm not sure it would work.  
> > Consider what would happen if the device is suspended very briefly and
> > then wakes up.  The usbdev_poll() call might not return, because by
> > the
> > time it checks udev->state, the state has already changed back to
> > USB_STATE_CONFIGURED.
> > 
> 
> I see what you mean here, could be problematic.
> 
> > In any case, we shouldn't do 4.  It would prevent the device from ever
> > going into suspend, because the program would want to continue making
> > usbfs ioctl calls while waiting for the suspend to occur.
> > 
> 
> In poll approach, due to the contraint I mentioned, it is not expected
> from user-space to interact with device *after* it calls ALLOW_SUSPEND
> ioctl. This is because, it has determined that device is idle from its
> point of view.

What if the user interacts with the device at this point?  Wouldn't 
the program want to know about it?

Even if your program doesn't care about user interaction with an idle
device, there undoubtedly will be other programs that _do_ care.  This 
mechanism we are designing needs to work with all programs.

> But after a while, there could be a situation where the user-space wants
> to send some message to device (due to end user interaction) then,
> having (4) would be handy to cancel the undone suspend or cause host-
> wake.

That's what the FORBID_SUSPEND ioctl does.  We don't need (4) for this.

> > > 2. Is it safe to wait for udev->state to be of a particular value?
> > No, not really, because the state can change.
> > 
> 
> If you remember, I had also suggested to use root-hub suspend in
> generic_suspend() to call usbfs_notify_suspend/_resume APIs. It might be
> possible to use that in usbdev_poll() and send POLLPRI when root-hub
> suspends.

I don't see how that would help.  It would just make things more 
confusing.  Forget about root-hub events for now.

> > > If this approach could work, I can spend time on this one as well.
> > What advantage do you think your proposal has over my proposal?
> > 
> 
> Not necessarily advantage(s), but few things that I could think of are -
> 
> 1. In poll based approach, user-space notion of device's state is in
> sync with actual device state.

NO!  That simply is not true.  You don't seem to be able to grasp this
point.

Consider this: Suppose the computer is busy running many other 
programs.  Your program gets swapped out because a bunch of other 
higher-priority tasks need to run.  By the time your program gets a 
chance to run again, the device could have gone through several 
suspend/resume transitions.  There's no way the program can keep track 
of all that.

If you want a more likely example, consider this: Your program calls 
ALLOW_SUSPEND and then calls poll().  The device suspends and the 
poll() returns.  But then, before your program can do anything else, 
the device resumes.  Now the device is active but your program thinks 
it is suspended -- the program is out of sync.

Face it: It is _impossible_ for a program to truly know the device's 
current state at all times (unless the device is not allowed to suspend 
at all).

> This is partially true with the 3 ioctl
> approach suggested by you (partially because resume might not be current
> device state when wait-for-resume returns).

Agreed.  But so what?  What abilities does your program lose as a 
result of the fact that the device might be suspended when 
WAIT_FOR_RESUME returns?

> 2. In 3 ioctl approach, user-space can still communicate with device
> after calling ALLOW_SUSPEND. With poll based approach, we put a
> constraint on user-space that, call ALLOW_SUSPEND only when you
> determine you are not going to interact with device.

That sounds like a disadvantage for your poll-based approach: It 
constrains the behavior of userspace programs.

> I know for (2) above, you have commented before that -
> A. It is desirable to be able to communicate with the device till it is
> actually suspended.
> B. The possibility of device not going into suspend for long time, so
> user-space will not be able to proceed.
> 
> The counter comments to them are:
> For (A), *usually*, the driver by some means determine dev

RE: [PATCH] ARM: imx25: provide a fixed regulator for usb phys

[Peter Chen]

 
> On 19-06-26 02:40, Peter Chen wrote:
> >
> > > Subject: [PATCH] ARM: imx25: provide a fixed regulator for usb phys
> > >
> > > The usb phys are internal to the SoC and so it their 5V supply. With
> > > this regulator added explicitly the following (harmless) boot messages go 
> > > away:
> > >
> > >   usb_phy_generic usbphy:usb-phy@0: usbphy:usb-phy@0 supply vcc not
> > > found, using dummy regulator
> > >   usb_phy_generic usbphy:usb-phy@1: usbphy:usb-phy@1 supply vcc not
> > > found, using dummy regulator
> > >
> >
> > To eliminate the warning message, I suggest doing below changes, as
> > vcc supply is not mandatory.
> >
> > diff --git a/drivers/usb/phy/phy-generic.c
> > b/drivers/usb/phy/phy-generic.c index a53b89be5324..01a5ff1a0515
> > 100644
> > --- a/drivers/usb/phy/phy-generic.c
> > +++ b/drivers/usb/phy/phy-generic.c
> > @@ -275,7 +275,7 @@ int usb_phy_gen_create_phy(struct device *dev, struct
> usb_phy_generic *nop,
> > }
> > }
> >
> > -   nop->vcc = devm_regulator_get(dev, "vcc");
> > +   nop->vcc = devm_regulator_get_optional(dev, "vcc");
> 
> Is the regulator optional? IMHO this shouldn't be the fix. I think the right 
> fix is Uwe's
> approach.
> 

Add Felipe.

Some USB PHY's power are from the core system's power (eg, DDR), and some are
fixed at the board and no switch for it. So, it is transparent for software at 
some cases.

Peter

> Regards,
>   Marco
> 
> > if (IS_ERR(nop->vcc)) {
> > dev_dbg(dev, "Error getting vcc regulator: %ld\n",
> > PTR_ERR(nop->vcc));
> >
> > Peter
> >
> > > Signed-off-by: Uwe Kleine-König 
> > > ---
> > > Hello,
> > >
> > > note I'm an USB noob, so please consider carefully before applying
> > > :-) I also put the regulator near the usbphy node instead of in
> > > alphabetic order. Not sure what is sensible/usual here, too.
> > >
> > > Best regards
> > > Uwe
> > >
> > >  arch/arm/boot/dts/imx25.dtsi | 9 +
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/arch/arm/boot/dts/imx25.dtsi
> > > b/arch/arm/boot/dts/imx25.dtsi
> > > --- a/arch/arm/boot/dts/imx25.dtsi
> > > +++ b/arch/arm/boot/dts/imx25.dtsi
> > > @@ -614,6 +614,11 @@
> > >   };
> > >   };
> > >
> > > + reg_usb: regulator_usbphy {
> > > + compatible = "regulator-fixed";
> > > + regulator-name = "usb-phy supply";
> > > + };
> > > +
> > >   usbphy {
> > >   compatible = "simple-bus";
> > >   #address-cells = <1>;
> > > @@ -623,12 +630,14 @@
> > >   reg = <0>;
> > >   compatible = "usb-nop-xceiv";
> > >   #phy-cells = <0>;
> > > + vcc-supply = <®_usb>;
> > >   };
> > >
> > >   usbphy1: usb-phy@1 {
> > >   reg = <1>;
> > >   compatible = "usb-nop-xceiv";
> > >   #phy-cells = <0>;
> > > + vcc-supply = <®_usb>;
> > >   };
> > >   };
> > >  };
> > > --
> > > 2.20.1
> >
> 
> --
> Pengutronix e.K.   | |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutro
> nix.de%2F&data=02%7C01%7Cpeter.chen%40nxp.com%7Cd1a839827b3a49
> 0624f508d6f9fab73f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
> 6971252538657445&sdata=kfTeGJ99AfS74BqdRAOLVJm52jIFIdNmZXXYPX
> SzAcA%3D&reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
> Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

RE: [PATCH] imx: usb: get pinctrl if it's not yet initialized

[Peter Chen]

 
> In case usb phy mode is other than USBPHY_INTERFACE_MODE_HSIC the pinctrl
> for device is not acquired. It is however used later regardless of the mode, 
> hence
> leads to requesting access to uninitialized data.
> 
> Signed-off-by: Krzysztof Michonski 
> ---
>  drivers/usb/chipidea/ci_hdrc_imx.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c 
> b/drivers/usb/chipidea/ci_hdrc_imx.c
> index a4b482c3dc65..2f02b35c40b6 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -428,6 +428,9 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>   pm_runtime_enable(dev);
>   }
> 
> + if (!data->pinctrl)
> + data->pinctrl = devm_pinctrl_get(dev);
> +
>   if (!IS_ERR(data->pinctrl)) {
>   struct pinctrl_state *state;
> 

Sorry, what kernel version you are using? The recent kernel is different with 
the patch you posted.

Peter

RE: No carrier lost information with gadget RNDIS/ECM

[Felipe Balbi]

Hi,

Kai Ruhnau  writes:
>>> I have activated CONFIG_USB_GADGET_DEBUG_FILES, and the contents of
>>> /proc/driver/rndis-000 don't change when I pull the cable:
>>>
>>> Config Nr. 0
>>> used  : y
>>> state : RNDIS_DATA_INITIALIZED
>>> medium: 0x
>>> speed : 425984000
>>> cable : connected
>>> vendor ID : 0x
>>> vendor: (null)
>>>
>>> Only when changing the host to a Mac, it's different:
>>> Config Nr. 0
>>> used  : y
>>> state : RNDIS_UNINITIALIZED
>>> medium: 0x
>>> speed : 425984000
>>> cable : connected
>>> vendor ID : 0x
>>> vendor: (null)
>>>
>>> Thanks for any help.
>>
>> Which peripheral controller is this board using? Is it chipidea? dwc2?
>> dwc3? High Speed or Super Speed?
>
> According to the device tree it's 'fsl,imx6sx-usb' driven by 
> chipidea/ci_hdrc_imx.c
> When connecting to Windows, the dmesg shows:
>  configfs-gadget gadget: high-speed config #2: c

Okay, adding Peter (chipidea maintainer) to the loop here. There are a
few changes on UDC side of chipidea between 4.9 and 5.1:

$ git --no-pager log --no-merges --oneline v4.9..v5.1 -- 
drivers/usb/chipidea/udc.c
16caf1fa37db usb: chipidea: Add dynamic pinctrl selection
51b751f112dc USB: chipidea: Remove redundant license text
5fd54ace4721 USB: add SPDX identifiers to all remaining files in drivers/usb/
fc5b920c3b9b usb: chipidea: do charger detection in vbus session
581821ae7f7e usb: chipidea: udc: Support SKB alignment quirk
734c58aefcc4 usb: chipidea: udc: compress return logic into line
aa1f058d7d92 usb: chipidea: udc: fix NULL pointer dereference if udc_start 
failed
a932a8041ff9 usb: chipidea: core: add sysfs group
aeb78cda5100 usb: chipidea: use bus->sysdev for DMA configuration
4f4555cfe704 usb: chipidea: udc: update gadget state after bus resume
afff6067b305 usb: chipidea: Drop lock across event_notify during gadget stop
732a4af85e87 usb: chipidea: Remove locking in ci_udc_start()
34445fb4333f usb: chipidea: Properly mark little endian descriptors
63b9e901e461 usb: chipidea: udc: remove unnecessary & operation
a98e25e71d11 usb: chipidea: udc: make use of new usb_endpoint_maxp_mult()

Peter, have you seen the problem described before?

-- 
balbi