Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices

[Jiri Kosina]

On Fri, 26 Jun 2015, Greg Kroah-Hartman wrote:

> > I thought udev used a whitelist of devices known to work okay with 
> > autosuspend.  Does it really turn on autosuspend for _every_ USB HID 
> > device that is marked as removable?
> 
> Yes, it had a tiny whitelist of 3-4 devices, and then would turn on
> autosuspend for anything not marked as removable or unknown.  Look at
> /usr/lib/udev/rules.d/42-usb-hid-pm.rules on your system for them, it's
> these lines:
>   ACTION=="add", SUBSYSTEM=="usb", SUBSYSTEMS=="usb", 
> ATTR{../removable}=="removable", GOTO="usb_hid_pm_end"
>   ACTION=="add", SUBSYSTEM=="usb", SUBSYSTEMS=="usb", 
> ATTR{../removable}=="unknown", GOTO="usb_hid_pm_end"
> 
> > (Come to think of it, given the bug in the hub driver, no device 
> > attached directly to the root hub will _ever_ be marked as removable 
> > AFAICS.  So maybe that bug is masking possible regressions.)
> 
> Maybe that's the issue, don't know, it would be good to figure out as
> upstream udev just deleted that whole rules file :)

Last time we were testing this, autosuspend for USB HID devices was quite 
a disaster.

Do you have any idea whether udev developers tested the "autosuspend on by 
default for USB HID devices" on reasonable set of devices?

The culrpits that I remember from top of my head (it's been long time 
ago):

- the LEDs for suspended device go off. This is very confusing at least on 
  keyboards, and brings really bad user experience

- many keyboards were losing first keystroke when waking up from suspend. 
  We've been debugging this with Alan, and never root-caused it to a 
  problem in our code, it seems to be the property of the HW

I really do want to keep the autosuspend disabled by default on USB HID 
devices (at least keyboards), and enable it based on whitelist. If udev is 
not going to do this any more, I am afraid we'll have to move the default 
into the kernel.

-- 
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack

[Enrico Mioso]

Hi Oliver!
I wish sincerely to thank you again for your time and patience.

On Fri, 26 Jun 2015, Oliver Neukum wrote:


Date: Fri, 26 Jun 2015 10:14:02
From: Oliver Neukum 
To: Enrico Mioso 
Cc: linux-usb@vger.kernel.org, net...@vger.kernel.org
Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack

On Thu, 2015-06-25 at 17:19 +0200, Enrico Mioso wrote:

Hi Oliver! And thank you again.

I like / recommend the usage of open messaging standards: my preferred XMPP ID
(JID) is: mrk...@jit.si.

On Thu, 25 Jun 2015, Oliver Neukum wrote:


Date: Thu, 25 Jun 2015 15:38:46
From: Oliver Neukum 
To: Enrico Mioso 
Cc: linux-usb@vger.kernel.org, net...@vger.kernel.org
Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack

On Thu, 2015-06-25 at 13:44 +0200, Enrico Mioso wrote:


On Thu, 25 Jun 2015, Oliver Neukum wrote:



Is there any advantage in keeping this in a single function?


I did this choice in the light of the fact I think the tx_fixup function will
become more complex than it is now, when aggregating frames.


Yes, but that is a reason to split the helpers up not the opposite.

Right - I understood only now your observation.

the only reason to write the manager that way was that it shouldn't become very
complex - it should simply do things to frames, helping out in building valid
NCM packages.




I answer here your other message to make it more convenient to read: my
intention for the tx_fixup function would be to:
- aggregate frames
- send them out when:
- a timer expires


How would you do that in tx_fixup()? If a timer is required then you
need a separate function.


Sure. I meant: I will adapt it in case needed, and expectin the code to become
a little bit more convoluted.


You cannot become any more convoluted even if you try.


OR
- we have enough data in the aggregate, and cannot add more.


Yes.

You need a third case:
- the interface is taken down.

But in general the logic for that is already there. So can you explain
what additional goals you have?

regarding the "timer logic" I saw it in cdc_ncm.c, but I should study it in
more detail to understand it and implement it here where needed in case.


It is involved. Basically a timer for transmission creates locking
issues. cdc-ncm uses an hrtimer which calls a bottom half which
calls xmit with a NULL skb.

   /* if there is a remaining skb, it gets priority */
   if (skb != NULL) {
   swap(skb, ctx->tx_rem_skb);
   swap(sign, ctx->tx_rem_sign);
   } else {
   ready2send = 1;
   }

The else branch here is the timer triggering.
The actual transmission is done in usbnet if cdc_ncm_fill_tx_frame()
returns an skb.

I doubt you can can come up with anything more convoluted
but it simplifies locking.


Well, no, but it supposes a matched commit phase. Can you guarantee
that? I was under the oppression that in that phase you want to actually
give a frame over to the hardware.

No. When Italk about committing, I think about "writing things to out skb".
another reason why i found confortable writing the code this way was to
maintain a kind of statefullness in a more "clean" way.
But I understand this is kind of agruable, and I can if needed break it up as
desired.

Regarding the commit phase - I am not sure I understand your comment, sorry
about that.
However, my intention would be to allow the caller to do calls in sequences
like:
- init frame
- ncm update
- ncm update
- ncm update
...
- ncm update
- ncm commit

(to work in "huawei" mode)

OR

- ncm init frame
- ncm commit
- ncm update
- ncm update
- ncm update
- ncm update
- finalize nth

(to work in "cdc_ncm.c"-mode)..


Sounds like a plan.


But to prevent usbnet from submittinx RX'd packets, I should be doing something
nasty here. And unfortunately don't understand why.


   // some devices want funky USB-level framing, for
   // win32 driver (usually) and/or hardware quirks
   if (info->tx_fixup) {
   skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
   if (!skb) {
   /* packet collected; minidriver waiting for more
*/
   if (info->flags & FLAG_MULTI_PACKET)
   goto not_drop;

So you just return NULL if you are ready to queue more. But you
need the FLAG_MULTI_PACKET flag.


Unfortuantely I might have not explained myself better.
I should thank you for this illumination on the timer part: since I know I'll 
need this.
But what's stopping me right now is actually the RX, not the TX part.
Sure, the RX function I am using comes from cdc_ncm.c and works fine: 
unfortunately, usbnet will not call it.
Infact, my dmesg ends up FULL of 
"kevent 2 may have been dropped" messages.
And looking around I discovered I end up triggering a memory check in usbnet.c, 
at line 475


skb = __netdev_alloc_skb_ip_align(dev->net, size, flags);
if (!skb) { /* triggered */
netif

Re: 64 byte EP0 OUT data transfer issue on Chipidea highspeed dual role controller

[Jayan John]

On Sat, Jun 27, 2015 at 2:21 AM, Alan Stern  wrote:
> On Sat, 27 Jun 2015, Jayan John wrote:
>
>> On Fri, Jun 26, 2015 at 8:44 PM, David Laight  
>> wrote:
>> > From: Steve Calfee
>> >> Sent: 26 June 2015 15:59
>> >> > On the host (Wandboard iMX6q) the test app opens /dev/hidraw0 and
>> >> > write 64 bytes with report ID (1). The HID device has no Interrupt OUT
>> >> > ep, therefore uses control endpoint ep0 for the 64 bytes transfer to
>> >> > gadget (Wandboard iMX6q) using set_report.
>> >> >
>> >> > The setup phase is OK and the 64 bytes is written to gadget. However
>> >> > the chipidea interrupt (ci_irq()) and resulting udc interrupt
>> >> > (udc_irq()) is invoked. This indicates that the 64 bytes transaction
>> >> > is not completed over ep0 from host to gadget.
>> >> >
>> >> > **this issue is reproducible for all data transfers that aligns on 64 
>> >> > bytes**
>> >> >
>> >> > ~jayan
>> >> >
>> >> Hi Jayan,
>> >>
>> >> This sounds like a Zero Length Transfer issue. This applies to any
>> >> endpoint including EP0.
>> >>
>> >> A ZLT is needed to end any transfer IFF the length is not already
>> >> known by the protocol. So if the hid URB requests say 1024 bytes and
>> >> the gadget only has 64, it must first send the 64 byte maxpacket and
>> >> then send a zero length packet. This way the host knows the transfer
>> >> is complete.
>> >
>> > Is this using the xhci driver?
>> > The ZLP sending code used to be broken.
>> > A couple of patches have been submitted but I don't remember one being 
>> > applied.
>> >
>> > David
>> >
>>
>> Thanks for the pointers. This is not using the xhci driver.
>>
>> - This is a ep0 OUT transfer. Does the host need to add a ZLP for 64
>> byte aligned data transfer?
>
> I'm not sure what you mean here by "aligned", but in general the answer
> is No.
>
>>  - Is it not expected for the completion interrupt be raised on
>> receiving the 64 bytes of data, irrespective of whether or not  a ZLP
>> has been sent.
>
> It is expected.  Did you check the wLength value in the Setup packet?
> Is it equal to 64?  Does the hid gadget driver then queue a request for
> a 64-byte OUT transfer on ep0?
>
> If it does then there's a bug in the UDC driver or hardware.
>
> Alan Stern
>

Thanks.

Yes, the wLength value in the Setup packet is equal to 64. "Aligned"
was the wrong term, multiple of 64 would be more appropriate :).

The hid gadget driver queues a request for the transfer. Please see below logs..
...
HID: drivers/usb/gadget/f_hid_meu.c:366 - hidg_setup()
HID: hid_setup crtl_request : bRequestType:0x21 bRequest:0x9 Value:0x200
HID: drivers/usb/chipidea/udc.c:1337 - ep_queue()
HID: drivers/usb/chipidea/udc.c:794 - _ep_queue()
HID: drivers/usb/chipidea/udc.c:467 - _hardware_enqueue()
HID: drivers/usb/chipidea/udc.c:395 - add_td_to_list()
HID: drivers/usb/chipidea/udc.c:61 - hw_ep_bit()
HID: drivers/usb/chipidea/udc.c:209 - hw_ep_prime()
..


In the 64 bytes case, the following logs are missing (indicating
transfer complete interrupt):
...
HID: drivers/usb/chipidea/core.c:368 - ci_irq()
HID: drivers/usb/chipidea/udc.c:1786 - udc_irq()
HID: drivers/usb/chipidea/udc.c:282 - hw_read_intr_status()
HID: drivers/usb/chipidea/udc.c:271 - hw_read_intr_enable()
HID: drivers/usb/chipidea/udc.c:309 - hw_test_and_clear_intr_active()
HID: drivers/usb/chipidea/udc.c:992 - isr_tr_complete_handler()
HID: drivers/usb/chipidea/udc.c:295 - hw_test_and_clear_complete()
HID: drivers/usb/chipidea/udc.c:68 - ep_to_bit()
HID: drivers/usb/chipidea/udc.c:957 - isr_tr_complete_low()
HID: drivers/usb/chipidea/udc.c:583 - _hardware_dequeue()
HID: drivers/usb/gadget/f_hid_meu.c:322 - hidg_set_report_complete()
...

~Jayan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices

[Greg Kroah-Hartman]

On Fri, Jun 26, 2015 at 09:20:19PM -0400, Alan Stern wrote:
> On Fri, 26 Jun 2015, Greg Kroah-Hartman wrote:
> 
> > > This doesn't do what the patch title says.  USB_DEVICE_FIXED means that
> > > the device can't be unplugged from its upstream port.  It doesn't mean
> > > the device is internal to the computer.
> > > 
> > > As an example, consider a composite Apple keyboard, which has an
> > > internal 3-port USB hub where two of the hub's ports are exposed on the
> > > edge of the keyboard case and the keyboard controller is permanently
> > > attached to the third hub port.  Then the controller device would be
> > > marked USB_DEVICE_FIXED, even though the whole thing is external to
> > > the computer and can be unplugged.
> > > 
> > 
> > Is that really how those devices are marked?  I can't find any of my
> > keyboard with hubs that mark things that way.
> 
> My Apple keyboard isn't here at the moment, and I don't remember
> exactly what its hub descriptor contains.  In theory, it _should_ mark
> the permanently attached port as non-removable.
> 
> I can test it next week, if you would like to see the actual values.

That would be great.

> > > Also, are you really certain this is safe?  Aren't there a number of 
> > > built-in keyboards that will work badly if you allow them to 
> > > autosuspend?
> > 
> > udev has been doing this for a while now by default, with no reports of
> > problems, so I think we should be safe.
> 
> I thought udev used a whitelist of devices known to work okay with 
> autosuspend.  Does it really turn on autosuspend for _every_ USB HID 
> device that is marked as removable?

Yes, it had a tiny whitelist of 3-4 devices, and then would turn on
autosuspend for anything not marked as removable or unknown.  Look at
/usr/lib/udev/rules.d/42-usb-hid-pm.rules on your system for them, it's
these lines:
ACTION=="add", SUBSYSTEM=="usb", SUBSYSTEMS=="usb", 
ATTR{../removable}=="removable", GOTO="usb_hid_pm_end"
ACTION=="add", SUBSYSTEM=="usb", SUBSYSTEMS=="usb", 
ATTR{../removable}=="unknown", GOTO="usb_hid_pm_end"

> (Come to think of it, given the bug in the hub driver, no device 
> attached directly to the root hub will _ever_ be marked as removable 
> AFAICS.  So maybe that bug is masking possible regressions.)

Maybe that's the issue, don't know, it would be good to figure out as
upstream udev just deleted that whole rules file :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices

[Alan Stern]

On Fri, 26 Jun 2015, Greg Kroah-Hartman wrote:

> > This doesn't do what the patch title says.  USB_DEVICE_FIXED means that
> > the device can't be unplugged from its upstream port.  It doesn't mean
> > the device is internal to the computer.
> > 
> > As an example, consider a composite Apple keyboard, which has an
> > internal 3-port USB hub where two of the hub's ports are exposed on the
> > edge of the keyboard case and the keyboard controller is permanently
> > attached to the third hub port.  Then the controller device would be
> > marked USB_DEVICE_FIXED, even though the whole thing is external to
> > the computer and can be unplugged.
> > 
> 
> Is that really how those devices are marked?  I can't find any of my
> keyboard with hubs that mark things that way.

My Apple keyboard isn't here at the moment, and I don't remember
exactly what its hub descriptor contains.  In theory, it _should_ mark
the permanently attached port as non-removable.

I can test it next week, if you would like to see the actual values.

> > Also, are you really certain this is safe?  Aren't there a number of 
> > built-in keyboards that will work badly if you allow them to 
> > autosuspend?
> 
> udev has been doing this for a while now by default, with no reports of
> problems, so I think we should be safe.

I thought udev used a whitelist of devices known to work okay with 
autosuspend.  Does it really turn on autosuspend for _every_ USB HID 
device that is marked as removable?

(Come to think of it, given the bug in the hub driver, no device 
attached directly to the root hub will _ever_ be marked as removable 
AFAICS.  So maybe that bug is masking possible regressions.)

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices

[Greg Kroah-Hartman]

On Fri, Jun 26, 2015 at 04:08:20PM -0400, Alan Stern wrote:
> On Fri, 26 Jun 2015, Tom Gundersen wrote:
> 
> > This policy used to be unconditionally applied by udev, but there
> > is no reason to make userspace be involved in this and in the future
> > udev will not be doing it by default.
> > 
> > See: .
> > 
> > Signed-off-by: Tom Gundersen 
> > Cc: Jiri Kosina 
> > Cc: Greg Kroah-Hartman 
> > ---
> > 
> > Hi,
> > 
> > I don't have the right hardware for this, so it has only been 
> > compile-tested.
> > I'm therefore sending it as an RFC only. Mainly I want to bring it to 
> > people's
> > attention that it would be great to get this feature into the kernel as we 
> > want
> > to drop it from udev.
> > 
> > Cheers,
> > 
> > Tom
> > 
> >  drivers/hid/usbhid/hid-core.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > index bfbe1be..af80700 100644
> > --- a/drivers/hid/usbhid/hid-core.c
> > +++ b/drivers/hid/usbhid/hid-core.c
> > @@ -1358,6 +1358,9 @@ static int usbhid_probe(struct usb_interface *intf, 
> > const struct usb_device_id *
> > setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
> > spin_lock_init(&usbhid->lock);
> >  
> > +   if (dev->removable == USB_DEVICE_FIXED)
> > +   usb_enable_autosuspend(dev);
> 
> This doesn't do what the patch title says.  USB_DEVICE_FIXED means that
> the device can't be unplugged from its upstream port.  It doesn't mean
> the device is internal to the computer.
> 
> As an example, consider a composite Apple keyboard, which has an
> internal 3-port USB hub where two of the hub's ports are exposed on the
> edge of the keyboard case and the keyboard controller is permanently
> attached to the third hub port.  Then the controller device would be
> marked USB_DEVICE_FIXED, even though the whole thing is external to
> the computer and can be unplugged.
> 

Is that really how those devices are marked?  I can't find any of my
keyboard with hubs that mark things that way.

> A reasonable compromise might be
> 
>   /*
>* Enable autosuspend for devices permanently attached
>* to the root hub.
>*/
>   if (!dev->parent->parent && dev->removable == USB_DEVICE_FIXED)
>   usb_enable_autosuspend(dev);
> 
> But this doesn't work if there's a permanently attached hub and a
> device permanently attached to that hub.  To do this thoroughly, you
> have to iterate up the dev->parent chain, making sure at each step that 
> the ->removable value is USB_DEVICE_FIXED.
> 
> Also, are you really certain this is safe?  Aren't there a number of 
> built-in keyboards that will work badly if you allow them to 
> autosuspend?

udev has been doing this for a while now by default, with no reports of
problems, so I think we should be safe.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 64 byte EP0 OUT data transfer issue on Chipidea highspeed dual role controller

[Alan Stern]

On Sat, 27 Jun 2015, Jayan John wrote:

> On Fri, Jun 26, 2015 at 8:44 PM, David Laight  wrote:
> > From: Steve Calfee
> >> Sent: 26 June 2015 15:59
> >> > On the host (Wandboard iMX6q) the test app opens /dev/hidraw0 and
> >> > write 64 bytes with report ID (1). The HID device has no Interrupt OUT
> >> > ep, therefore uses control endpoint ep0 for the 64 bytes transfer to
> >> > gadget (Wandboard iMX6q) using set_report.
> >> >
> >> > The setup phase is OK and the 64 bytes is written to gadget. However
> >> > the chipidea interrupt (ci_irq()) and resulting udc interrupt
> >> > (udc_irq()) is invoked. This indicates that the 64 bytes transaction
> >> > is not completed over ep0 from host to gadget.
> >> >
> >> > **this issue is reproducible for all data transfers that aligns on 64 
> >> > bytes**
> >> >
> >> > ~jayan
> >> >
> >> Hi Jayan,
> >>
> >> This sounds like a Zero Length Transfer issue. This applies to any
> >> endpoint including EP0.
> >>
> >> A ZLT is needed to end any transfer IFF the length is not already
> >> known by the protocol. So if the hid URB requests say 1024 bytes and
> >> the gadget only has 64, it must first send the 64 byte maxpacket and
> >> then send a zero length packet. This way the host knows the transfer
> >> is complete.
> >
> > Is this using the xhci driver?
> > The ZLP sending code used to be broken.
> > A couple of patches have been submitted but I don't remember one being 
> > applied.
> >
> > David
> >
> 
> Thanks for the pointers. This is not using the xhci driver.
> 
> - This is a ep0 OUT transfer. Does the host need to add a ZLP for 64
> byte aligned data transfer?

I'm not sure what you mean here by "aligned", but in general the answer 
is No.

>  - Is it not expected for the completion interrupt be raised on
> receiving the 64 bytes of data, irrespective of whether or not  a ZLP
> has been sent.

It is expected.  Did you check the wLength value in the Setup packet?  
Is it equal to 64?  Does the hid gadget driver then queue a request for 
a 64-byte OUT transfer on ep0?

If it does then there's a bug in the UDC driver or hardware.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 64 byte EP0 OUT data transfer issue on Chipidea highspeed dual role controller

[Jayan John]

On Fri, Jun 26, 2015 at 8:44 PM, David Laight  wrote:
> From: Steve Calfee
>> Sent: 26 June 2015 15:59
>> > On the host (Wandboard iMX6q) the test app opens /dev/hidraw0 and
>> > write 64 bytes with report ID (1). The HID device has no Interrupt OUT
>> > ep, therefore uses control endpoint ep0 for the 64 bytes transfer to
>> > gadget (Wandboard iMX6q) using set_report.
>> >
>> > The setup phase is OK and the 64 bytes is written to gadget. However
>> > the chipidea interrupt (ci_irq()) and resulting udc interrupt
>> > (udc_irq()) is invoked. This indicates that the 64 bytes transaction
>> > is not completed over ep0 from host to gadget.
>> >
>> > **this issue is reproducible for all data transfers that aligns on 64 
>> > bytes**
>> >
>> > ~jayan
>> >
>> Hi Jayan,
>>
>> This sounds like a Zero Length Transfer issue. This applies to any
>> endpoint including EP0.
>>
>> A ZLT is needed to end any transfer IFF the length is not already
>> known by the protocol. So if the hid URB requests say 1024 bytes and
>> the gadget only has 64, it must first send the 64 byte maxpacket and
>> then send a zero length packet. This way the host knows the transfer
>> is complete.
>
> Is this using the xhci driver?
> The ZLP sending code used to be broken.
> A couple of patches have been submitted but I don't remember one being 
> applied.
>
> David
>

Thanks for the pointers. This is not using the xhci driver.

- This is a ep0 OUT transfer. Does the host need to add a ZLP for 64
byte aligned data transfer?
 - Is it not expected for the completion interrupt be raised on
receiving the 64 bytes of data, irrespective of whether or not  a ZLP
has been sent.

~Jayan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices

[Alan Stern]

On Fri, 26 Jun 2015, Alan Stern wrote:

> On Fri, 26 Jun 2015, Tom Gundersen wrote:
> 
> > This policy used to be unconditionally applied by udev, but there
> > is no reason to make userspace be involved in this and in the future
> > udev will not be doing it by default.
> > 
> > See: .
> > 
> > Signed-off-by: Tom Gundersen 
> > Cc: Jiri Kosina 
> > Cc: Greg Kroah-Hartman 
> > ---
> > 
> > Hi,
> > 
> > I don't have the right hardware for this, so it has only been 
> > compile-tested.
> > I'm therefore sending it as an RFC only. Mainly I want to bring it to 
> > people's
> > attention that it would be great to get this feature into the kernel as we 
> > want
> > to drop it from udev.
> > 
> > Cheers,
> > 
> > Tom
> > 
> >  drivers/hid/usbhid/hid-core.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > index bfbe1be..af80700 100644
> > --- a/drivers/hid/usbhid/hid-core.c
> > +++ b/drivers/hid/usbhid/hid-core.c
> > @@ -1358,6 +1358,9 @@ static int usbhid_probe(struct usb_interface *intf, 
> > const struct usb_device_id *
> > setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
> > spin_lock_init(&usbhid->lock);
> >  
> > +   if (dev->removable == USB_DEVICE_FIXED)
> > +   usb_enable_autosuspend(dev);
> 
> This doesn't do what the patch title says.  USB_DEVICE_FIXED means that
> the device can't be unplugged from its upstream port.  It doesn't mean
> the device is internal to the computer.
> 
> As an example, consider a composite Apple keyboard, which has an

Oops -- I meant "compound", not "composite".  A compound USB device
actually has multiple devices (one of which is a hub) in a single 
package.

> internal 3-port USB hub where two of the hub's ports are exposed on the
> edge of the keyboard case and the keyboard controller is permanently
> attached to the third hub port.  Then the controller device would be
> marked USB_DEVICE_FIXED, even though the whole thing is external to
> the computer and can be unplugged.

Another problem is that the set_usb_port_removable() routine in
drivers/usb/core/hub.c (the routine that sets dev->removable in the
first place) contains this code:

hub = usb_hub_to_struct_hub(udev->parent);

wHubCharacteristics = le16_to_cpu(hub->descriptor->wHubCharacteristics);

if (!(wHubCharacteristics & HUB_CHAR_COMPOUND))
return;

I guess the assumption was that all of a hub's ports would be
unpluggable if the hub wasn't part of a compound device.  But that's
not correct for root hubs.  This test should be deleted.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices

[Alan Stern]

On Fri, 26 Jun 2015, Tom Gundersen wrote:

> This policy used to be unconditionally applied by udev, but there
> is no reason to make userspace be involved in this and in the future
> udev will not be doing it by default.
> 
> See: .
> 
> Signed-off-by: Tom Gundersen 
> Cc: Jiri Kosina 
> Cc: Greg Kroah-Hartman 
> ---
> 
> Hi,
> 
> I don't have the right hardware for this, so it has only been compile-tested.
> I'm therefore sending it as an RFC only. Mainly I want to bring it to people's
> attention that it would be great to get this feature into the kernel as we 
> want
> to drop it from udev.
> 
> Cheers,
> 
> Tom
> 
>  drivers/hid/usbhid/hid-core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index bfbe1be..af80700 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1358,6 +1358,9 @@ static int usbhid_probe(struct usb_interface *intf, 
> const struct usb_device_id *
>   setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
>   spin_lock_init(&usbhid->lock);
>  
> + if (dev->removable == USB_DEVICE_FIXED)
> + usb_enable_autosuspend(dev);

This doesn't do what the patch title says.  USB_DEVICE_FIXED means that
the device can't be unplugged from its upstream port.  It doesn't mean
the device is internal to the computer.

As an example, consider a composite Apple keyboard, which has an
internal 3-port USB hub where two of the hub's ports are exposed on the
edge of the keyboard case and the keyboard controller is permanently
attached to the third hub port.  Then the controller device would be
marked USB_DEVICE_FIXED, even though the whole thing is external to
the computer and can be unplugged.

A reasonable compromise might be

/*
 * Enable autosuspend for devices permanently attached
 * to the root hub.
 */
if (!dev->parent->parent && dev->removable == USB_DEVICE_FIXED)
usb_enable_autosuspend(dev);

But this doesn't work if there's a permanently attached hub and a
device permanently attached to that hub.  To do this thoroughly, you
have to iterate up the dev->parent chain, making sure at each step that 
the ->removable value is USB_DEVICE_FIXED.

Also, are you really certain this is safe?  Aren't there a number of 
built-in keyboards that will work badly if you allow them to 
autosuspend?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: common code for CDC drivers

[Tal Shorer]

It won't break, but this will make me compile something I don't need
and may not want.
Wouldn't it make more sense to put this unified code in a new file
(maybe drivers/usb/class/cdc.c) and have all related drivers depend on
that instead of on usbnet?
The less code that is compiled but never runs, the happier I am.

On Fri, Jun 26, 2015 at 7:59 PM, Greg KH  wrote:
> On Fri, Jun 26, 2015 at 07:47:47PM +0300, Tal Shorer wrote:
>> As someone who has cdc-acm enabled and usbnet disabled on my project,
>> I'd rather you don't break my configuration >:
>
> configurations will not break, they will be updated to pull in usbnet if
> needed.
>
> thanks,
>
> greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices

[Greg Kroah-Hartman]

On Fri, Jun 26, 2015 at 09:24:07PM +0200, Tom Gundersen wrote:
> This policy used to be unconditionally applied by udev, but there
> is no reason to make userspace be involved in this and in the future
> udev will not be doing it by default.
> 
> See: .
> 
> Signed-off-by: Tom Gundersen 
> Cc: Jiri Kosina 
> Cc: Greg Kroah-Hartman 
> ---
> 
> Hi,
> 
> I don't have the right hardware for this, so it has only been compile-tested.
> I'm therefore sending it as an RFC only. Mainly I want to bring it to people's
> attention that it would be great to get this feature into the kernel as we 
> want
> to drop it from udev.
> 
> Cheers,
> 
> Tom
> 
>  drivers/hid/usbhid/hid-core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index bfbe1be..af80700 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1358,6 +1358,9 @@ static int usbhid_probe(struct usb_interface *intf, 
> const struct usb_device_id *
>   setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
>   spin_lock_init(&usbhid->lock);
>  
> + if (dev->removable == USB_DEVICE_FIXED)
> + usb_enable_autosuspend(dev);
> +

As this duplicates what udev has been doing for a while now, we should
be safe here.

thanks for the patch.

Acked-by: Greg Kroah-Hartman 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[GIT PULL] USB driver patches for 4.2-rc1

[Greg KH]

The following changes since commit d4a4f75cd8f29cd9464a5a32e9224a91571d6649:

  Linux 4.1-rc7 (2015-06-07 20:23:50 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/ tags/usb-4.2-rc1

for you to fetch changes up to 50641056d833813b71b0ad51823f7ded8dd62e7f:

  usb: dwc3: Use ASCII space in Kconfig (2015-06-12 17:42:31 -0700)


USB patches for 4.2-rc1

Here's the big USB patchset for 4.2-rc1.  As is normal these days, the
majority of changes are in the gadget drivers, with a bunch of other
small driver changes.

All of these have been in linux-next with no reported issues.

Signed-off-by: Greg Kroah-Hartman 


Abhishek Bist (1):
  USB: hcd.h : Removed an unnecessary function prototype 
usb_find_interface_driver()

Akinobu Mita (1):
  usb: storage: fix module reference for scsi host

Alan Stern (1):
  USB: don't build PCI quirks if USB support isn't configured

Alexey Sokolov (1):
  cdc-acm: Add support of ATOL FPrint fiscal printers

Andrzej Pietrasiewicz (6):
  usb: gadget: rndis: use rndis_params instead of configNr
  usb: gadget: rndis: style correction
  usb: gadget: rndis: remove the limit of available rndis connections
  usb: gadget: rndis: change the value passed to rndis_signal_(dis)connect()
  usb: gadget: rndis: don't duplicate the "i" variable
  usb: gadget: rndis: use signed type for a signed value

Arnd Bergmann (2):
  usb: renesas_usbhs: avoid uninitialized variable use
  usb: phy: add static inline wrapper for devm_usb_get_phy_by_node

Arun Ramamurthy (3):
  phy: core: Add devm_of_phy_get_by_index to phy-core
  usb: ehci-platform: Use devm_of_phy_get_by_index
  usb: ohci-platform: Use devm_of_phy_get_by_index

Axel Lin (1):
  phy: core: Check requested PHY status in _of_phy_get()

Bin Liu (2):
  usb: musb: only set test mode once
  usb: musb: add softconnect for host mode

Brian Norris (2):
  Documentation: devicetree: add Broadcom SATA PHY binding
  phy: add Broadcom SATA3 PHY driver for Broadcom STB SoCs

Chris Bainbridge (1):
  usb: host: xhci: remove incorrect comment about mutex

Colin Ian King (1):
  usb: isp1760: check for null return from kzalloc

Dan Carpenter (1):
  USB: devio: fix a condition in async_completed()

Dmitry Torokhov (1):
  phy: phy-core: allow specifying supply at port level

Fabian Frederick (1):
  cdc-acm: use swap() in acm_probe()

Fabio Estevam (1):
  usb: chipidea: usbmisc_imx: Remove unneeded semicolon

Felipe Balbi (8):
  phy: miphy28lp: fix sparse warnings
  phy: miphy365x: fix sparse warnings
  usb: dwc2: hcd: fix build warning
  usb: gadget: atmel: fix build warning
  usb: musb: am35x: fix build warnings
  usb: musb: ux500: fix build warnings
  usb: gadget: atmel: fix build warnings
  usb: dwc3: gadget: don't clear EP_BUSY too early

Geert Uytterhoeven (3):
  usb: phy: Remove the phy-rcar-gen2-usb driver
  usb: phy: Allow compile test of GPIO consumers if !GPIOLIB
  usb: phy: Remove the phy-rcar-gen2-usb driver

Greg Kroah-Hartman (25):
  USB: ehci-dbg.c: move assignment out of if () block
  USB: fusbh200-hcd.c: move assignment out of if () block
  USB: hcd.c: move assignment out of if () block
  USB: hub.c: move assignment out of if () block
  USB: inode.c: move assignment out of if () block
  USB: isp116x-hcd.c: move assignment out of if () block
  USB: mon_bin.c: move assignment out of if () block
  USB: mon_main.c: move assignment out of if () block
  USB: mon_stat.c: move assignment out of if () block
  USB: ohci-dbg.c: move assignment out of if () block
  USB: ohci-hcd.c: move assignment out of if () block
  USB: ohci-q.c: move assignment out of if () block
  USB: sisusb.c: move assignment out of if () block
  USB: sisusb_con.c: move assignment out of if () block
  USB: speedtch.c: move assignment out of if () block
  USB: usbatm.c: move assignment out of if () block
  USB: usblp.c: move assignment out of if () block
  USB: uss720.c: move assignment out of if () block
  USB: xusbatm.c: move assignment out of if () block
  Merge 4.1-rc4 into usb-next
  Merge tag 'usb-for-v4.2' of git://git.kernel.org/.../balbi/usb into 
usb-next
  Merge tag 'phy-for-v4.2' of git://git.kernel.org/.../kishon/linux-phy 
into usb-next
  Merge tag 'usb-serial-4.2-rc1' of 
git://git.kernel.org/.../johan/usb-serial into usb-next
  Merge 4.1-rc7 into usb-next
  Merge tag 'usb-ci-v4.2-rc1' of git://git.kernel.org/.../peter.chen/usb 
into usb-work

Gregory Herrero (15):
  usb: dwc2: add controller hibernation support
  usb: dwc2: implement hibernation during bus suspend/resume
  usb: dwc2: controller must update lx_state before releasing l

[PATCH][RFC] usbhid: enable autosuspend for internal devices

[Tom Gundersen]

This policy used to be unconditionally applied by udev, but there
is no reason to make userspace be involved in this and in the future
udev will not be doing it by default.

See: .

Signed-off-by: Tom Gundersen 
Cc: Jiri Kosina 
Cc: Greg Kroah-Hartman 
---

Hi,

I don't have the right hardware for this, so it has only been compile-tested.
I'm therefore sending it as an RFC only. Mainly I want to bring it to people's
attention that it would be great to get this feature into the kernel as we want
to drop it from udev.

Cheers,

Tom

 drivers/hid/usbhid/hid-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index bfbe1be..af80700 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1358,6 +1358,9 @@ static int usbhid_probe(struct usb_interface *intf, const 
struct usb_device_id *
setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
spin_lock_init(&usbhid->lock);
 
+   if (dev->removable == USB_DEVICE_FIXED)
+   usb_enable_autosuspend(dev);
+
ret = hid_add_device(hid);
if (ret) {
if (ret != -ENODEV)
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3][v2] drivers:usb:fsl: Replace macros with enumerated type

[gre...@linuxfoundation.org]

On Fri, Jun 26, 2015 at 05:12:04PM +, Badola Nikhil wrote:
> > -Original Message-
> > From: Nikhil Badola [mailto:nikhil.bad...@freescale.com]
> > Sent: Monday, June 15, 2015 3:47 PM



> Could you please provide comments for this patch and the subsequent
> patches in this patch set, if any.

It's the middle of the merge window right now, there's nothing I can do
with this until after it is over.  Please wait until then to expect a
response, and even then, give me a week or so to dig out from under the
pending-patch load.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xhci:Change function name from xhci_configure_endpoint_result to xhci_trace_endpoint_result

[Greg KH]

On Fri, Jun 26, 2015 at 02:25:48PM -0400, Nicholas Krause wrote:
> This changes the name of the function xhci_configure_endpoint_result
> to xhci_trace_endpoint_result to more clearly state the actual
> intended use of this function which is tracing the passed command
> status, on the passed usb/xhci structure pointers of udev and xhci
> respectfully.
> 
> Signed-off-by: Nicholas Krause 

Dammit, my filters broke, I need to go fix them now...

> ---
>  drivers/usb/host/xhci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 36bf089..2e086ef 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1839,7 +1839,7 @@ static void xhci_zero_in_ctx(struct xhci_hcd *xhci, 
> struct xhci_virt_device *vir
>   }
>  }
>  
> -static int xhci_configure_endpoint_result(struct xhci_hcd *xhci,
> +static int xhci_trace_endpoint_result(struct xhci_hcd *xhci,
>   struct usb_device *udev, u32 *cmd_status)
>  {
>   int ret;
> @@ -2683,7 +2683,7 @@ static int xhci_configure_endpoint(struct xhci_hcd 
> *xhci,
>   wait_for_completion(command->completion);
>  
>   if (!ctx_change)
> - ret = xhci_configure_endpoint_result(xhci, udev,
> + ret = xhci_trace_endpoint_result(xhci, udev,
>&command->status);

No, please stop, this is doing nothing but unneeded code churn, and do
absolutely nothing.

Because you are banned from vger, these patches are not being sent to
the public, so I will not accept them either.

I will not be accepting any patches from you for at least a year, maybe
two, so don't waste your time.  Please go work on some other open source
project, if you really want to contribute to the Linux ecosystem, as
long as it is not the kernel.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Xenta Mouse Wireless sometimes not working in Linux Kernel 4.0.5

[Greg KH]

On Fri, Jun 19, 2015 at 02:58:27PM -0500, José David León Rodríguez wrote:
> Sometimes generic mice usb wireless not work with usb_modeswitch enabled.
> 
> dmesg | grep usb output when kernel not find device descriptors for mouse:
> 
> [  154.988937] usb 2-3: new full-speed USB device number 9 using xhci_hcd
> [  160.159445] usb 2-3: unable to read config index 0 descriptor/start: -110
> [  160.159450] usb 2-3: can't read configurations, error -110
> [  160.319415] usb 2-3: new full-speed USB device number 10 using xhci_hcd
> [  165.489805] usb 2-3: unable to read config index 0 descriptor/start: -110
> [  165.489815] usb 2-3: can't read configurations, error -110
> 
> after I disable usb_modeswitch or unplug and plug mouse several times...

usb_modeswitch shouldn't care about this, the device isn't communicating
well with the system.

> [  165.649708] usb 2-3: new full-speed USB device number 11 using xhci_hcd
> [  165.705290] usbcore: registered new interface driver usbhid
> [  165.705293] usbhid: USB HID core driver
> [  165.739693] input: HID Wireless Mouse HID Wireless Mouse as
> /devices/pci:00/:00:14.0/usb2/2-3/2-3:1.0/0003:1D57:0016.0001/input/input17
> [  165.739838] hid-generic 0003:1D57:0016.0001: input,hidraw0: USB HID
> v1.10 Mouse [HID Wireless Mouse HID Wireless Mouse] on
> usb-:00:14.0-3/input0

So it works properly now?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/3][v2] drivers:usb:fsl: Replace macros with enumerated type

[Alan Stern]

On Fri, 26 Jun 2015, Badola Nikhil wrote:

> Hi Greg/Alan,
> 
> Could you please provide comments for this patch and the subsequent
> patches in this patch set, if any.

You shouldn't ask Greg and me; you should ask the Freescale maintainer.  
He is the person who has to approve your patches.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] usb: storage : Remove c99 style commenting

[Sunny Kumar]

On Fri, Jun 26, 2015 at 10:08:42AM -0400, Alan Stern wrote:
> On Fri, 26 Jun 2015, Sunny Kumar wrote:
> 
> > This patch fixes checkpatch.pl warning c99 style commenting.
> > 
> > Signed-off-by: Sunny Kumar 
> > ---
> >  drivers/usb/storage/sddr55.c | 16 
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c
> > index aacedef..54d0a59 100644
> > --- a/drivers/usb/storage/sddr55.c
> > +++ b/drivers/usb/storage/sddr55.c
> > @@ -209,10 +209,10 @@ static int sddr55_read_data(struct us_data *us,
> > unsigned int len, offset;
> > struct scatterlist *sg;
> >  
> > -   // Since we only read in one block at a time, we have to create
> > -   // a bounce buffer and move the data a piece at a time between the
> > -   // bounce buffer and the actual transfer buffer.
> > -
> > +   /* Since we only read in one block at a time, we have to create
> > +* a bounce buffer and move the data a piece at a time between the
> > +* bounce buffer and the actual transfer buffer.
> > +*/
> > len = min((unsigned int) sectors, (unsigned int) info->blocksize >>
> > info->smallpageshift) * PAGESIZE;
> > buffer = kmalloc(len, GFP_NOIO);
> > @@ -336,10 +336,10 @@ static int sddr55_write_data(struct us_data *us,
> > return USB_STOR_TRANSPORT_FAILED;
> > }
> >  
> > -   // Since we only write one block at a time, we have to create
> > -   // a bounce buffer and move the data a piece at a time between the
> > -   // bounce buffer and the actual transfer buffer.
> > -
> > +   /* Since we only write one block at a time, we have to create
> > +* a bounce buffer and move the data a piece at a time between the
> > +* bounce buffer and the actual transfer buffer.
> > +*/
> > len = min((unsigned int) sectors, (unsigned int) info->blocksize >>
> > info->smallpageshift) * PAGESIZE;
> > buffer = kmalloc(len, GFP_NOIO);
> 
> Why did you fix just these two sites?  There are lots of other places 
> in usb-storage that use C99-style comments:
> 
> $ cd drivers/usb/storage
> $ egrep '[^:]//' *.[ch] | wc
> 17716359562
> 
> (The [^:] is to avoid matching things like "http://";, and as a result
> this misses the four places where a // comment starts at the beginning
> of a line.)
> 
> Why not fix all of them?
These were C99 muliline comments so thought of fixing ..
> 
> Alan Stern
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/3][v2] drivers:usb:fsl: Replace macros with enumerated type

[Badola Nikhil]

> -Original Message-
> From: Nikhil Badola [mailto:nikhil.bad...@freescale.com]
> Sent: Monday, June 15, 2015 3:47 PM
> To: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org
> Cc: gre...@linuxfoundation.org; st...@rowland.harvard.edu; Badola Nikhil-
> B46172
> Subject: [PATCH 1/3][v2] drivers:usb:fsl: Replace macros with enumerated
> type
> 
> Replace macros with enumerated type to represent usb ip controller version
> 
> Signed-off-by: Nikhil Badola 
> ---
> Changes for v2 :
>   - Assigned value to each enumerator
>   - Changed return type of function that returns
> controller version
>   - Introduced FSL_USB_VER_NONE for invalid controller version
> 
>  drivers/usb/host/fsl-mph-dr-of.c |  8 
>  include/linux/fsl_devices.h  | 16 ++--
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-
> of.c
> index 5e0d600..2195956 100644
> --- a/drivers/usb/host/fsl-mph-dr-of.c
> +++ b/drivers/usb/host/fsl-mph-dr-of.c
> @@ -119,9 +119,9 @@ error:
> 
>  static const struct of_device_id fsl_usb2_mph_dr_of_match[];
> 
> -static int usb_get_ver_info(struct device_node *np)
> +static enum fsl_usb2_controller_ver usb_get_ver_info(struct device_node
> +*np)
>  {
> - int ver = -1;
> + enum fsl_usb2_controller_ver ver = FSL_USB_VER_NONE;
> 
>   /*
>* returns 1 for usb controller version 1.6 @@ -142,7 +142,7 @@ static
> int usb_get_ver_info(struct device_node *np)
>   else /* for previous controller versions */
>   ver = FSL_USB_VER_OLD;
> 
> - if (ver > -1)
> + if (ver > FSL_USB_VER_NONE)
>   return ver;
>   }
> 
> @@ -215,7 +215,7 @@ static int fsl_usb2_mph_dr_of_probe(struct
> platform_device *ofdev)
>   pdata->controller_ver = usb_get_ver_info(np);
> 
>   if (pdata->have_sysif_regs) {
> - if (pdata->controller_ver < 0) {
> + if (pdata->controller_ver == FSL_USB_VER_NONE) {
>   dev_warn(&ofdev->dev, "Could not get controller
> version\n");
>   return -ENODEV;
>   }
> diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h index
> 2a2f56b..0d4855cd 100644
> --- a/include/linux/fsl_devices.h
> +++ b/include/linux/fsl_devices.h
> @@ -20,11 +20,6 @@
>  #define FSL_UTMI_PHY_DLY 10  /*As per P1010RM, delay for UTMI
>   PHY CLK to become stable - 10ms*/
>  #define FSL_USB_PHY_CLK_TIMEOUT  1   /* uSec */
> -#define FSL_USB_VER_OLD  0
> -#define FSL_USB_VER_1_6  1
> -#define FSL_USB_VER_2_2  2
> -#define FSL_USB_VER_2_4  3
> -#define FSL_USB_VER_2_5  4
> 
>  #include 
> 
> @@ -52,6 +47,15 @@
>   *
>   */
> 
> +enum fsl_usb2_controller_ver {
> + FSL_USB_VER_NONE = -1,
> + FSL_USB_VER_OLD = 0,
> + FSL_USB_VER_1_6 = 1,
> + FSL_USB_VER_2_2 = 2,
> + FSL_USB_VER_2_4 = 3,
> + FSL_USB_VER_2_5 = 4,
> +};
> +
>  enum fsl_usb2_operating_modes {
>   FSL_USB2_MPH_HOST,
>   FSL_USB2_DR_HOST,
> @@ -72,7 +76,7 @@ struct platform_device;
> 
>  struct fsl_usb2_platform_data {
>   /* board specific information */
> - int controller_ver;
> + enum fsl_usb2_controller_vercontroller_ver;
>   enum fsl_usb2_operating_modes   operating_mode;
>   enum fsl_usb2_phy_modes phy_mode;
>   unsigned intport_enables;
> --

Hi Greg/Alan,

Could you please provide comments for this patch and the subsequent
patches in this patch set, if any.

Thanks,
Nikhil 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: common code for CDC drivers

[Greg KH]

On Fri, Jun 26, 2015 at 07:47:47PM +0300, Tal Shorer wrote:
> As someone who has cdc-acm enabled and usbnet disabled on my project,
> I'd rather you don't break my configuration >:

configurations will not break, they will be updated to pull in usbnet if
needed.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: common code for CDC drivers

[Tal Shorer]

As someone who has cdc-acm enabled and usbnet disabled on my project,
I'd rather you don't break my configuration >:

On Fri, Jun 26, 2015 at 4:40 PM, Oliver Neukum  wrote:
> Hi,
>
> I am looking at all the CDC drivers. They all have a parser for
> the extra CDC headers. If I unify this would it be acceptable
> for cdc-acm to depend on usbnet? Or should I put the code into
> a header?
>
> Regards
> Oliver
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: common code for CDC drivers

[Bjørn Mork]

On June 26, 2015 3:40:18 PM CEST, Oliver Neukum  wrote:
>Hi,
>
>I am looking at all the CDC drivers. They all have a parser for
>the extra CDC headers. If I unify this would it be acceptable
>for cdc-acm to depend on usbnet? Or should I put the code into
>a header?

I don't have any strong feelings.  Either way is fine. 

And thanks for doing this. I've often thought about it, but always been too 
lazy to actually do anything about it. 

I believe cdc-wdm also has one of these parsers and should be unified.



Bjørn

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [EDT][PATCH] XHCI: Fix memory leak in error condition

[Greg KH]

On Fri, Jun 26, 2015 at 07:46:00AM +, Vivek Kumar Bhagat wrote:
> EP-EC562D6B53594479BCA6FC73F17DEE54
> In error condition, td buffer is not freed which can lead
> to memory leak.
> 
> Signed-off-by: Vivek Kumar Bhagat 
> ---
>  drivers/usb/host/xhci.c |1 +
>  1 file changed, 1 insertion(+)

Also, please use scripts/get_maintainer.pl to determine who to send
patches to.  Hint, I'm not the one for this patch, I can't do anything
with it...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [EDT][PATCH] XHCI: Fix memory leak in error condition

[Greg KH]

On Fri, Jun 26, 2015 at 07:46:00AM +, Vivek Kumar Bhagat wrote:
> EP-EC562D6B53594479BCA6FC73F17DEE54

What is that crazy line for?  It doesn't belong here.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: 64 byte EP0 OUT data transfer issue on Chipidea highspeed dual role controller

[David Laight]

From: Steve Calfee
> Sent: 26 June 2015 15:59
> > On the host (Wandboard iMX6q) the test app opens /dev/hidraw0 and
> > write 64 bytes with report ID (1). The HID device has no Interrupt OUT
> > ep, therefore uses control endpoint ep0 for the 64 bytes transfer to
> > gadget (Wandboard iMX6q) using set_report.
> >
> > The setup phase is OK and the 64 bytes is written to gadget. However
> > the chipidea interrupt (ci_irq()) and resulting udc interrupt
> > (udc_irq()) is invoked. This indicates that the 64 bytes transaction
> > is not completed over ep0 from host to gadget.
> >
> > **this issue is reproducible for all data transfers that aligns on 64 
> > bytes**
> >
> > ~jayan
> >
> Hi Jayan,
> 
> This sounds like a Zero Length Transfer issue. This applies to any
> endpoint including EP0.
> 
> A ZLT is needed to end any transfer IFF the length is not already
> known by the protocol. So if the hid URB requests say 1024 bytes and
> the gadget only has 64, it must first send the 64 byte maxpacket and
> then send a zero length packet. This way the host knows the transfer
> is complete.

Is this using the xhci driver?
The ZLP sending code used to be broken.
A couple of patches have been submitted but I don't remember one being applied.

David

Re: 64 byte EP0 OUT data transfer issue on Chipidea highspeed dual role controller

[Steve Calfee]

On Fri, Jun 26, 2015 at 5:01 AM, Jayan John  wrote:
> Thanks Alex. I appreciate you introducing me to Peter. Any help is 
> appreciated.
>
> On the host (Wandboard iMX6q) the test app opens /dev/hidraw0 and
> write 64 bytes with report ID (1). The HID device has no Interrupt OUT
> ep, therefore uses control endpoint ep0 for the 64 bytes transfer to
> gadget (Wandboard iMX6q) using set_report.
>
> The setup phase is OK and the 64 bytes is written to gadget. However
> the chipidea interrupt (ci_irq()) and resulting udc interrupt
> (udc_irq()) is invoked. This indicates that the 64 bytes transaction
> is not completed over ep0 from host to gadget.
>
> **this issue is reproducible for all data transfers that aligns on 64 bytes**
>
> ~jayan
>
Hi Jayan,

This sounds like a Zero Length Transfer issue. This applies to any
endpoint including EP0.

A ZLT is needed to end any transfer IFF the length is not already
known by the protocol. So if the hid URB requests say 1024 bytes and
the gadget only has 64, it must first send the 64 byte maxpacket and
then send a zero length packet. This way the host knows the transfer
is complete.

If there is no ZLT, the host hardware will keep requesting the rest of
the message and the gadget which thinks it is done will keep
NAKing Forever.

If however the transfer is known to be exactly some multiple (1, 2
...) of the maxpacketsize, no ZLT is needed since the host will ask
for exactly that size. I believe for HID the packet size is determined
by the report size in the HID descriptor.

Good Luck, Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] usb: storage : Remove c99 style commenting

[Alan Stern]

On Fri, 26 Jun 2015, Sunny Kumar wrote:

> This patch fixes checkpatch.pl warning c99 style commenting.
> 
> Signed-off-by: Sunny Kumar 
> ---
>  drivers/usb/storage/sddr55.c | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c
> index aacedef..54d0a59 100644
> --- a/drivers/usb/storage/sddr55.c
> +++ b/drivers/usb/storage/sddr55.c
> @@ -209,10 +209,10 @@ static int sddr55_read_data(struct us_data *us,
>   unsigned int len, offset;
>   struct scatterlist *sg;
>  
> - // Since we only read in one block at a time, we have to create
> - // a bounce buffer and move the data a piece at a time between the
> - // bounce buffer and the actual transfer buffer.
> -
> + /* Since we only read in one block at a time, we have to create
> +  * a bounce buffer and move the data a piece at a time between the
> +  * bounce buffer and the actual transfer buffer.
> +  */
>   len = min((unsigned int) sectors, (unsigned int) info->blocksize >>
>   info->smallpageshift) * PAGESIZE;
>   buffer = kmalloc(len, GFP_NOIO);
> @@ -336,10 +336,10 @@ static int sddr55_write_data(struct us_data *us,
>   return USB_STOR_TRANSPORT_FAILED;
>   }
>  
> - // Since we only write one block at a time, we have to create
> - // a bounce buffer and move the data a piece at a time between the
> - // bounce buffer and the actual transfer buffer.
> -
> + /* Since we only write one block at a time, we have to create
> +  * a bounce buffer and move the data a piece at a time between the
> +  * bounce buffer and the actual transfer buffer.
> +  */
>   len = min((unsigned int) sectors, (unsigned int) info->blocksize >>
>   info->smallpageshift) * PAGESIZE;
>   buffer = kmalloc(len, GFP_NOIO);

Why did you fix just these two sites?  There are lots of other places 
in usb-storage that use C99-style comments:

$ cd drivers/usb/storage
$ egrep '[^:]//' *.[ch] | wc
17716359562

(The [^:] is to avoid matching things like "http://";, and as a result
this misses the four places where a // comment starts at the beginning
of a line.)

Why not fix all of them?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] USB: mos7720: rename registers

[Sudip Mukherjee]

On Fri, Jun 26, 2015 at 09:46:43AM +0200, Johan Hovold wrote:
> On Tue, Jun 23, 2015 at 06:59:40PM +0530, Sudip Mukherjee wrote:
> > Some of the register names defined here are matching with registers
> > defined in other places. Like DCR is defined here and DCR is also
> > a register in mn10300 architecture. So when we are building this with
> > mn10300, build fails.
> > To avoid we rename all the registers.
> > 
> > Signed-off-by: Sudip Mukherjee 
> 
> Looks good. I'll queue this up for 4.2.
Thanks Johan.

David,
This series works on next-20150618. But next-20150625 again fails in
allmodconfig of mn10300. I did a bisect and the first bad commit is:
db8786a2829c ("Merge remote-tracking branch 'dt-rh/for-next'").
And reverting it solved the problem. I will find out what was the exact
problem in that merge, but after I find out should i send a new series
or another patch?

regards
sudip

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] usb: chipidea: Reduce ULPI PHY reset pulse to datasheet spec of 1us

[Mike Looijmans]

The datasheet for the 334x PHY mentions that a reset can be performed:
"... by bringing the pin low for a minimum of 1 microsecond and
then high."
A delay of 5ms to implement that seems overly long, so reduce it to
just 1us.
As for the delay after reset, the datasheet only mentioned that the
chip will assert the DIR output. 1ms seems like a safe time to wait
for that to happen, so no change there.

Signed-off-by: Mike Looijmans 
---
 drivers/usb/chipidea/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index e970863..c865abe 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -664,7 +664,7 @@ static int ci_hdrc_create_ulpi_phy(struct device *dev, 
struct ci_hdrc *ci)
dev_err(dev, "Failed to request ULPI reset gpio: %d\n", 
ret);
return ret;
}
-   msleep(5);
+   udelay(1);
gpio_set_value_cansleep(reset_gpio, 1);
msleep(1);
}
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] usb: chipidea: Wait 50 ms before reading ID bit

[Mike Looijmans]

The datasheet for the USB343x PHY mentions a 50ms wait time before
reading back the ID bit after enabling the internal pull-up or a
reset:
"To monitor the status of the ID pin, the Link activates the IdPullup
bit in the OTG Control register, waits 50mS and then reads the status
of the IdGnd bit in the USB Interrupt Status register."
Implement this by adding a 50ms sleep at the only point in the code
where the ID status is being read without IRQ trigger.

When starting the board with a USB cable connected to a PC, the
system would activate host mode, then in ~20ms get an ID IRQ and
attempt to switch to gadget mode. This then failed because the
VBUS will not drop to zero (because the host is supplying it).

After this patch, the system starts up correctly and selects
gadget mode immediately, and the USB link works. It also fixes
the issue that the VBUS supply was being activated while already
being supplied from the host PC.

Signed-off-by: Mike Looijmans 
---
 drivers/usb/chipidea/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index c865abe..4c6cf48 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -801,6 +801,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 
if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
if (ci->is_otg) {
+   msleep(50); /* Datasheet: Wait 50ms to read ID */
ci->role = ci_otg_role(ci);
/* Enable ID change irq */
hw_write_otgsc(ci, OTGSC_IDIE, OTGSC_IDIE);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

common code for CDC drivers

[Oliver Neukum]

Hi,

I am looking at all the CDC drivers. They all have a parser for
the extra CDC headers. If I unify this would it be acceptable
for cdc-acm to depend on usbnet? Or should I put the code into
a header?

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] usb: storage : Remove c99 style commenting

[Sergei Shtylyov]

Hello.

On 6/26/2015 9:14 AM, Sunny Kumar wrote:


This patch fixes checkpatch.pl warning c99 style commenting.



Signed-off-by: Sunny Kumar 
---
  drivers/usb/storage/sddr55.c | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)



diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c
index aacedef..54d0a59 100644
--- a/drivers/usb/storage/sddr55.c
+++ b/drivers/usb/storage/sddr55.c
@@ -209,10 +209,10 @@ static int sddr55_read_data(struct us_data *us,
unsigned int len, offset;
struct scatterlist *sg;

-   // Since we only read in one block at a time, we have to create
-   // a bounce buffer and move the data a piece at a time between the
-   // bounce buffer and the actual transfer buffer.
-
+   /* Since we only read in one block at a time, we have to create
+* a bounce buffer and move the data a piece at a time between the
+* bounce buffer and the actual transfer buffer.
+*/


   This style of commenting is preferred only in the networking code. The 
other parts of the kernel prefer this style:


/*
 * bla
 * bla
 */

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 64 byte EP0 OUT data transfer issue on Chipidea highspeed dual role controller

[Jayan John]

Thanks Alex. I appreciate you introducing me to Peter. Any help is appreciated.

On the host (Wandboard iMX6q) the test app opens /dev/hidraw0 and
write 64 bytes with report ID (1). The HID device has no Interrupt OUT
ep, therefore uses control endpoint ep0 for the 64 bytes transfer to
gadget (Wandboard iMX6q) using set_report.

The setup phase is OK and the 64 bytes is written to gadget. However
the chipidea interrupt (ci_irq()) and resulting udc interrupt
(udc_irq()) is invoked. This indicates that the 64 bytes transaction
is not completed over ep0 from host to gadget.

**this issue is reproducible for all data transfers that aligns on 64 bytes**

~jayan

On Fri, Jun 26, 2015 at 5:10 PM, Alexander Shishkin
 wrote:
> Jayan John  writes:
>
>> I am developing a custom USB device on a iMX6q platform (Wandboard)
>> Chipidea HDRC (highspeed dual role controller). The HID interface
>> consists of a single Interrupt IN ep and ep0. It is required to send
>> HID reports from Host to Gadget over ep0 (with set_report cmd on
>> hidraw interface) in OUT direction. The size of each HID report is 64
>> bytes.
>>
>> The Chipidea HDRC is unable to receive/ process the 64 bytes of data
>> over ep0 (OUT). The setup stage is fine. No UDC interrupt is raised by
>> controller to indicate completion of transaction. The same data
>> transfer works fine for <64 bytes (63, 63 etc) and >64 bytes (65, 66
>> etc) where the transfers are split as 64+n. Analyzer logs attached.
>>
>> 1. Is there an issue with Chipidea HDRC receiving wMaxPacketSize (i.e.
>> 64 byte) aligned data transfers on ep0 in OUT direction?
>> 2. Anything that needs to be configured/ added in descriptor or source
>> to have completion interrupt hit on 64 bytes data transfer on ep0 OUT?
>
> Let's add Peter to CC as he's the maintainer for Chipidea now. How do
> you generate this traffic? Are you using the zero gadget or some other
> kind of test?
>
> Regards,
> --
> Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 64 byte EP0 OUT data transfer issue on Chipidea highspeed dual role controller

[Alexander Shishkin]

Jayan John  writes:

> I am developing a custom USB device on a iMX6q platform (Wandboard)
> Chipidea HDRC (highspeed dual role controller). The HID interface
> consists of a single Interrupt IN ep and ep0. It is required to send
> HID reports from Host to Gadget over ep0 (with set_report cmd on
> hidraw interface) in OUT direction. The size of each HID report is 64
> bytes.
>
> The Chipidea HDRC is unable to receive/ process the 64 bytes of data
> over ep0 (OUT). The setup stage is fine. No UDC interrupt is raised by
> controller to indicate completion of transaction. The same data
> transfer works fine for <64 bytes (63, 63 etc) and >64 bytes (65, 66
> etc) where the transfers are split as 64+n. Analyzer logs attached.
>
> 1. Is there an issue with Chipidea HDRC receiving wMaxPacketSize (i.e.
> 64 byte) aligned data transfers on ep0 in OUT direction?
> 2. Anything that needs to be configured/ added in descriptor or source
> to have completion interrupt hit on 64 bytes data transfer on ep0 OUT?

Let's add Peter to CC as he's the maintainer for Chipidea now. How do
you generate this traffic? Are you using the zero gadget or some other
kind of test?

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[Stefan Richter]

On Jun 26 James Bottomley wrote:
> On Fri, 2015-06-26 at 11:43 +0200, Stefan Richter wrote:
> > On Jun 22 James Bottomley wrote:
[...]
> > > Perhaps it might be wise to do this to every USB device ... for external
> > > devices, the small performance gain doesn't really make up for the
> > > potential data loss.
> > 
> > Just a small note on the assumption of externally (and in extension,
> > temporarily) attached devices:  Not all USB-attached devices are external,
> > and not all external devices are used as removable devices.
> 
> The problems don't depend on the connection type: internal devices which
> have a writeback cache and don't accept flush commands have data
> integrity problems too.  I can't really think of many situations where
> you'd be willing to sacrifice data integrity for performance.

Sure; writeback caches are prone to more issues besides sudden connection
loss.  (E.g. sudden power loss is but one of several more potential issues
of course.)

I merely wanted to remind that the bus type of a device (USB or whatever)
does not say much about the risk of sudden connection loss to that device,
since such devices may have physical protection against sudden connection
loss too.  I was not particularly commenting on the topic at hand, i.e.
on presence of writeback cache without working flush command.
-- 
Stefan Richter
-=-= -==- ==-=-
http://arcgraph.de/sr/
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[James Bottomley]

On Fri, 2015-06-26 at 11:43 +0200, Stefan Richter wrote:
> On Jun 22 James Bottomley wrote:
> > On Mon, 2015-06-22 at 13:30 -0400, Alan Stern wrote:
> > > On Mon, 22 Jun 2015, James Bottomley wrote:
> > > > Obviously, for a disk with a writeback cache that can't do flush, that
> > > > window is much wider and the real solution should be to try to switch
> > > > the cache to write through.
> > > 
> > > I agree.  Doing the switch manually (by writing to the "cache_type" 
> > > attribute file) works, but it's a nuisance to do this when you have a 
> > > portable USB drive that gets moved among a bunch of machines.
> > 
> > Perhaps it might be wise to do this to every USB device ... for external
> > devices, the small performance gain doesn't really make up for the
> > potential data loss.
> 
> Just a small note on the assumption of externally (and in extension,
> temporarily) attached devices:  Not all USB-attached devices are external,
> and not all external devices are used as removable devices.

The problems don't depend on the connection type: internal devices which
have a writeback cache and don't accept flush commands have data
integrity problems too.  I can't really think of many situations where
you'd be willing to sacrifice data integrity for performance.

James

> (For example, I am using 2 internal CD-ROMs via USB-2 + usb-storage and 2
> internal HDDs via USB 3 + uas in a PC with too few SATA ports and no PCIe
> slot to spare for a controller add-on card, but plenty of USB headers
> available on the mainboard.  Similarly, some NASes have their operating
> system located on a USB-attached device.  Small offices use USB-attached
> disks for backup and won't detach such a disk until rotation for off-site
> deposit.  Not to mention embedded computers with USB-attached but fixed
> disks.)



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb:host:Make the function room_on_ring bool

[Mathias Nyman]

On 25.06.2015 19:27, Nicholas Krause wrote:
> This makes the function room_on_ring bool now due to this particular
> function only ever returning either one or zero as its return value.
> 
> Signed-off-by: Nicholas Krause 

Thanks, 

Looks good, but could you start the subject with 
"usb: xhci:", or just "xhci:" for xhci specific changes

-Mathias 


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH TRIVIAL] drivers/usb/gadget/composite.c: i18n is not an acronym

[Borislav Petkov]

On Fri, Jun 26, 2015 at 11:43:25AM +0200, Jiri Kosina wrote:
> On Thu, 25 Jun 2015, Diego Viola wrote:
> 
> > Ping?
> 
> Diego, you seem to be very anxious about your trivial patches.
> 
> Please understand, that those of the patches which are not really 
> super-important, might sit in the queue for long time, because they are 
> preempted by more important stuff.
> 
> Your patch hasn't been lost, it's still in my trivial-todo inbox, and will 
> eventually be processed.

I'm pretty sure if Diego tried fixing some real bugs, people would
respond much faster...

:-)

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[Stefan Richter]

On Jun 22 James Bottomley wrote:
> On Mon, 2015-06-22 at 13:30 -0400, Alan Stern wrote:
> > On Mon, 22 Jun 2015, James Bottomley wrote:
> > > Obviously, for a disk with a writeback cache that can't do flush, that
> > > window is much wider and the real solution should be to try to switch
> > > the cache to write through.
> > 
> > I agree.  Doing the switch manually (by writing to the "cache_type" 
> > attribute file) works, but it's a nuisance to do this when you have a 
> > portable USB drive that gets moved among a bunch of machines.
> 
> Perhaps it might be wise to do this to every USB device ... for external
> devices, the small performance gain doesn't really make up for the
> potential data loss.

Just a small note on the assumption of externally (and in extension,
temporarily) attached devices:  Not all USB-attached devices are external,
and not all external devices are used as removable devices.

(For example, I am using 2 internal CD-ROMs via USB-2 + usb-storage and 2
internal HDDs via USB 3 + uas in a PC with too few SATA ports and no PCIe
slot to spare for a controller add-on card, but plenty of USB headers
available on the mainboard.  Similarly, some NASes have their operating
system located on a USB-attached device.  Small offices use USB-attached
disks for backup and won't detach such a disk until rotation for off-site
deposit.  Not to mention embedded computers with USB-attached but fixed
disks.)
-- 
Stefan Richter
-=-= -==- ==-=-
http://arcgraph.de/sr/
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH TRIVIAL] drivers/usb/gadget/composite.c: i18n is not an acronym

[Jiri Kosina]

On Thu, 25 Jun 2015, Diego Viola wrote:

> Ping?

Diego, you seem to be very anxious about your trivial patches.

Please understand, that those of the patches which are not really 
super-important, might sit in the queue for long time, because they are 
preempted by more important stuff.

Your patch hasn't been lost, it's still in my trivial-todo inbox, and will 
eventually be processed.

Thanks,

-- 
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: about [PATCH] net:usb:cdc_ncm: fix that tag Huawei devices as wwan

[Bjørn Mork]

"Chenqi (jakio)"  writes:

> Hi, All,
>
> I'm from Huawei Technologies Co., Ltd, working in mobile broadband
> devices department.
>
> Huawei's broadband card don't support wwan in linux OS, there are
> several historical reasons:
>
> 1.  Huawei produced dozens of broadband card types in past years, and
> our customers/users integrate our products in many different OS
> versions, We need give customer/user a continuous method to let them
> use convenience.
>
> 2.  Wwan connection need application support, while we didn't test
> them for compatibility.
>
> Meanwhile, we have provided serial port to let other existing
> applications dial-up the connection
>
> I understand that you guys have some confusion why the Huawei
> broadband cards support wwan in windows OS but not in linux OS with
> the same device ID.  Since wwan standard is used firstly in windows
> OS, when we pronounced to support this feature, wwan is not enabled in
> linux.  So we keep the card's behavior in linux same as before.

The Linux kernel/drivers/userspace always will attempt to use hardware
in the same way Windows use it, or at least as close as we are able to
get.  The reason is simple: A billion (or whatever) Windows users is the
best test lab you can get.

You may not agree with this strategy, but as a hardware vendor you have
to expect it to happen.  The community will continue to develop support
based on that assumption, even if you try to accommodate Linux by adding
addiotional "Linux specific" features or modes.

> This wwan patch has great influence on our broadband cards, we have
> tested many our broadband cards with this kernel version, and this
> compatible issue is common.  So please approve our modification for
> this problem.

If noone else has any comments or objections here, then I am certainly
not going to object to the proposed patch.

Please go ahead and submit the patch formally.  I am going to keep my
big mouth shut :)

> I think the root cause of this issue is this kernel version trait the
> NCM as wwan devtype, maybe we can add a flag in usb descriptor in
> future to indicate whether the device support wwan or not, then Huawei
> can support wwan feature later for the newest product without
> introducing compatible issues.
>
> BTW: if we want to join the usb-ethernet kernel evolution in linux,
> which forum or organization should we join in?  We can participate in
> it to avoid similar problems.

USB networking drivers are discussed, along with other network drivers,
on the net...@vger.kernel.org mailing list.  Preferable with a copy to
the linux-usb@vger.kernel.org mailing list for USB expert review.

But as you point out above:  wwan connections need application support.
Userspace projects like ModemManager, oFono, libmbim, libqmi, umbim,
uqmi and probably more, each have their own development forums/mailing
lists.  This is where the most important part of the wwan developemt
takes place.

Huawei have already made important contributions to the development of
both Linux wwan drivers and userspace applications.  I am sure all those
projects will appreciate any further help Huawei are able to offer.

Thanks a lot for your detailed introduction and explanation.


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack

[Oliver Neukum]

On Thu, 2015-06-25 at 17:19 +0200, Enrico Mioso wrote:
> Hi Oliver! And thank you again.
> 
> I like / recommend the usage of open messaging standards: my preferred XMPP ID
> (JID) is: mrk...@jit.si.
> 
> On Thu, 25 Jun 2015, Oliver Neukum wrote:
> 
> > Date: Thu, 25 Jun 2015 15:38:46
> > From: Oliver Neukum 
> > To: Enrico Mioso 
> > Cc: linux-usb@vger.kernel.org, net...@vger.kernel.org
> > Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
> > 
> > On Thu, 2015-06-25 at 13:44 +0200, Enrico Mioso wrote:
> >
> >> On Thu, 25 Jun 2015, Oliver Neukum wrote:
> >
> >>> Is there any advantage in keeping this in a single function?
> >>>
> >> I did this choice in the light of the fact I think the tx_fixup function 
> >> will
> >> become more complex than it is now, when aggregating frames.
> >
> > Yes, but that is a reason to split the helpers up not the opposite.
> Right - I understood only now your observation.
> 
> the only reason to write the manager that way was that it shouldn't become 
> very 
> complex - it should simply do things to frames, helping out in building valid 
> NCM packages.
> 
> >
> >> I answer here your other message to make it more convenient to read: my
> >> intention for the tx_fixup function would be to:
> >> - aggregate frames
> >> - send them out when:
> >>- a timer expires
> >
> > How would you do that in tx_fixup()? If a timer is required then you
> > need a separate function.
> >
> Sure. I meant: I will adapt it in case needed, and expectin the code to 
> become 
> a little bit more convoluted.

You cannot become any more convoluted even if you try.

> >>OR
> >>- we have enough data in the aggregate, and cannot add more.
> >
> > Yes.
> >
> > You need a third case:
> > - the interface is taken down.
> >
> > But in general the logic for that is already there. So can you explain
> > what additional goals you have?
> regarding the "timer logic" I saw it in cdc_ncm.c, but I should study it in 
> more detail to understand it and implement it here where needed in case.

It is involved. Basically a timer for transmission creates locking
issues. cdc-ncm uses an hrtimer which calls a bottom half which
calls xmit with a NULL skb.

/* if there is a remaining skb, it gets priority */
if (skb != NULL) {
swap(skb, ctx->tx_rem_skb);
swap(sign, ctx->tx_rem_sign);
} else {
ready2send = 1;
}

The else branch here is the timer triggering.
The actual transmission is done in usbnet if cdc_ncm_fill_tx_frame()
returns an skb.

I doubt you can can come up with anything more convoluted
but it simplifies locking.

> > Well, no, but it supposes a matched commit phase. Can you guarantee
> > that? I was under the oppression that in that phase you want to actually
> > give a frame over to the hardware.
> No. When Italk about committing, I think about "writing things to out skb".
> another reason why i found confortable writing the code this way was to 
> maintain a kind of statefullness in a more "clean" way.
> But I understand this is kind of agruable, and I can if needed break it up as 
> desired.
> 
> Regarding the commit phase - I am not sure I understand your comment, sorry
> about that.
> However, my intention would be to allow the caller to do calls in sequences 
> like:
> - init frame
> - ncm update
> - ncm update
> - ncm update
> ...
> - ncm update
> - ncm commit
> 
> (to work in "huawei" mode)
> 
> OR
> 
> - ncm init frame
> - ncm commit
> - ncm update
> - ncm update
> - ncm update
> - ncm update
> - finalize nth
> 
> (to work in "cdc_ncm.c"-mode)..

Sounds like a plan.

> But to prevent usbnet from submittinx RX'd packets, I should be doing 
> something 
> nasty here. And unfortunately don't understand why.

// some devices want funky USB-level framing, for
// win32 driver (usually) and/or hardware quirks
if (info->tx_fixup) {
skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
if (!skb) {
/* packet collected; minidriver waiting for more
*/
if (info->flags & FLAG_MULTI_PACKET)
goto not_drop;

So you just return NULL if you are ready to queue more. But you
need the FLAG_MULTI_PACKET flag.

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [EDT][PATCH] XHCI: Fix memory leak in error condition

[Roger Quadros]

On 26/06/15 10:46, Vivek Kumar Bhagat wrote:

EP-EC562D6B53594479BCA6FC73F17DEE54
In error condition, td buffer is not freed which can lead
to memory leak.

Signed-off-by: Vivek Kumar Bhagat 
---
  drivers/usb/host/xhci.c |1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 36bf089..dc02532 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1438,6 +1438,7 @@ dying:
ret = -ESHUTDOWN;
  free_priv:
xhci_urb_free_priv(urb_priv);
+   kfree(buffer);
urb->hcpriv = NULL;
spin_unlock_irqrestore(&xhci->lock, flags);
return ret;



Do you need to fix up xhci_urb_free_priv() as well?

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] USB: mos7720: rename registers

[Johan Hovold]

On Tue, Jun 23, 2015 at 06:59:40PM +0530, Sudip Mukherjee wrote:
> Some of the register names defined here are matching with registers
> defined in other places. Like DCR is defined here and DCR is also
> a register in mn10300 architecture. So when we are building this with
> mn10300, build fails.
> To avoid we rename all the registers.
> 
> Signed-off-by: Sudip Mukherjee 

Looks good. I'll queue this up for 4.2.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[EDT][PATCH] XHCI: Fix memory leak in error condition

[Vivek Kumar Bhagat]

EP-EC562D6B53594479BCA6FC73F17DEE54
In error condition, td buffer is not freed which can lead
to memory leak.

Signed-off-by: Vivek Kumar Bhagat 
---
 drivers/usb/host/xhci.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 36bf089..dc02532 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1438,6 +1438,7 @@ dying:
ret = -ESHUTDOWN;
 free_priv:
xhci_urb_free_priv(urb_priv);
+   kfree(buffer);
urb->hcpriv = NULL;
spin_unlock_irqrestore(&xhci->lock, flags);
return ret;
-- 
1.7.9.5

Re: [PATCH] USB: cp210x: add ID for Aruba Networks controllers

[Johan Hovold]

On Thu, Jun 25, 2015 at 05:40:05PM -0700, Peter Sanford wrote:
> Add the USB serial console device ID for Aruba Networks 7xxx series
> controllers which have a USB port for their serial console.
> 
> Signed-off-by: Peter Sanford 

Thanks for the patch. I'll queue it up for 4.2.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/3] USB: io_ti: Move request_firmware() calls out of download_fw()

[Johan Hovold]

On Wed, Jun 24, 2015 at 12:18:27AM -0500, Peter Berger wrote:
> On Mon, 2015-06-22 at 11:43 +0200, Johan Hovold wrote:
> > On Thu, Jun 18, 2015 at 06:43:35AM -0500, Peter E. Berger wrote:
> > > From: "Peter E. Berger" 
> > > 
> > > The io_ti driver fails to download firmware to Edgeport
> > > devices such as the EP/416, even when the on-disk firmware image
> > > (/lib/firmware/edgeport/down3.bin) is more current than the version
> > > on the EP/416.  The current download code is broken in a few ways.
> > > Notably it mis-uses global variables OperationalMajorVersion and
> > > OperationalMinorVersion (reading their values before they've been
> > > properly initialized and subsequently initializing them multiple times
> > > without synchronization).  This patch drops the global variables and
> > > replaces the redundant calls to request_firmware()/release_firmware()
> > > in download_fw() with a single call pair in edge_startup(); the firmware
> > > image pointer is then passed to download_fw() and build_i2c_fw_hdr().
> > >
> > > Also, the firmware has a "build_number" field, though it apparently is
> > > unused (according to observations of the three firmware images I have
> > > seen and confirmed by Digi Tech Support).  This comment describes its
> > > structure, in case it is populated in a future release.
> > >
> > > Signed-off-by: Peter E. Berger 
> > > ---
> > >  drivers/usb/serial/io_ti.c | 100 
> > > +++--
> > >  1 file changed, 51 insertions(+), 49 deletions(-)
> > > 
> > > diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
> > > index 69378a7..c76820b 100644
> > > --- a/drivers/usb/serial/io_ti.c
> > > +++ b/drivers/usb/serial/io_ti.c
> > > @@ -101,6 +101,7 @@ struct edgeport_serial {
> > >   struct mutex es_lock;
> > >   int num_ports_open;
> > >   struct usb_serial *serial;
> > > + int fw_version;
> > >  };
> > >  
> > >  
> > > @@ -187,10 +188,6 @@ static const struct usb_device_id 
> > > id_table_combined[] = {
> > >  
> > >  MODULE_DEVICE_TABLE(usb, id_table_combined);
> > >  
> > > -static unsigned char OperationalMajorVersion;
> > > -static unsigned char OperationalMinorVersion;
> > > -static unsigned short OperationalBuildNumber;
> > > -
> > >  static int closing_wait = EDGE_CLOSING_WAIT;
> > >  static bool ignore_cpu_rev;
> > >  static int default_uart_mode;/* RS232 */
> > > @@ -751,18 +748,18 @@ exit:
> > >  }
> > >  
> > >  /* Build firmware header used for firmware update */
> > > -static int build_i2c_fw_hdr(__u8 *header, struct device *dev)
> > > +static int build_i2c_fw_hdr(u8 *header, struct device *dev,
> > > + const struct firmware *fw)
> > >  {
> > >   __u8 *buffer;
> > >   int buffer_size;
> > >   int i;
> > > - int err;
> > >   __u8 cs = 0;
> > >   struct ti_i2c_desc *i2c_header;
> > >   struct ti_i2c_image_header *img_header;
> > >   struct ti_i2c_firmware_rec *firmware_rec;
> > > - const struct firmware *fw;
> > > - const char *fw_name = "edgeport/down3.bin";
> > > + u8 fw_major_version = fw->data[0];
> > > + u8 fw_minor_version = fw->data[1];
> > >  
> > >   /* In order to update the I2C firmware we must change the type 2 record
> > >* to type 0xF2.  This will force the UMP to come up in Boot Mode.
> > > @@ -785,24 +782,11 @@ static int build_i2c_fw_hdr(__u8 *header, struct 
> > > device *dev)
> > >   // Set entire image of 0xffs
> > >   memset(buffer, 0xff, buffer_size);
> > >  
> > > - err = request_firmware(&fw, fw_name, dev);
> > > - if (err) {
> > > - dev_err(dev, "Failed to load image \"%s\" err %d\n",
> > > - fw_name, err);
> > > - kfree(buffer);
> > > - return err;
> > > - }
> > > -
> > > - /* Save Download Version Number */
> > > - OperationalMajorVersion = fw->data[0];
> > > - OperationalMinorVersion = fw->data[1];
> > > - OperationalBuildNumber = fw->data[2] | (fw->data[3] << 8);
> > > -
> > >   /* Copy version number into firmware record */
> > >   firmware_rec = (struct ti_i2c_firmware_rec *)buffer;
> > >  
> > > - firmware_rec->Ver_Major = OperationalMajorVersion;
> > > - firmware_rec->Ver_Minor = OperationalMinorVersion;
> > > + firmware_rec->Ver_Major = fw_major_version;
> > > + firmware_rec->Ver_Minor = fw_minor_version;
> > >  
> > >   /* Pointer to fw_down memory image */
> > >   img_header = (struct ti_i2c_image_header *)&fw->data[4];
> > 
> > Note that sanity checks on the firmware size are missing here; you could
> > fix that as a follow up.
> 
> I did some digging and it looks like we can actually do several sanity
> checks on the firmware image.  It seems that the Edgeport firmware
> images have a 7-byte header:
>  u8 major_version;
>  u8 minor_version;
>  le16 build_number;
>  le16 length;
>  u8 checksum;
>  
> "length" appears to be the number of bytes of actual data following the
> header.
>  
> "checksum" is apparently (from the images I have seen) the low order
> byte resulting from adding the values of all

Re: [PATCH v6 1/3] USB: io_ti: Increase insufficient timeout for firmware downloads

[Johan Hovold]

On Wed, Jun 24, 2015 at 12:39:05AM -0500, Peter Berger wrote:
> On Mon, 2015-06-22 at 10:45 +0200, Johan Hovold wrote:
> > On Thu, Jun 18, 2015 at 06:43:34AM -0500, Peter E. Berger wrote:
> > > From: "Peter E. Berger" 
> > > 
> > > The io_ti driver fails to download firmware to Edgeport devices such as
> > > the EP/416.  One of the problems is that the default 1 second timeout
> > > in ti_vsend_sync() is insufficient for download operations.  This patch
> > > increases the download timeout to 10 seconds.
> > 
> > Patch looks good now.
> > 
> > What happens when download fails? Does the device still work (e.g. this
> > is only needed to support newer on-disk firmware)? Perhaps you can
> > mention that in the commit log as well.
> 
> It looks like some Edgeports (models like the EP/416 with on-board
> E2PROM) may be able to function even if the on-disk firmware image is
> bad or missing, iff their local E2PROM versions are valid.  But most
> Edgeport models (I've tried EP/1 and EP/4) do not appear to have this
> capability and they always rely on the on-disk firmware image.
> 
> I'm testing an implementation that calls the new check_fw_sanity()
> function at the top of download_fw() and, rather than simply returning
> an error if the firmware image is bad or missing, it saves the result
> and defers the decision until later when it may find that it is running
> on a E2PROM-equipped device with a valid image.  But I think this is
> messier than it is worth (adding still more messiness to the already
> very messy download_fw()) for such a marginal possible benefit.  I'm
> leaning towards the simpler approach of returning an error whenever
> check_fw_sanity() indicates a bad on-disk firmware image.  Do you agree?

Yes, that sounds reasonable.

Perhaps such a feature can be added later after a much-needed clean up
of download_fw, if ever.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html