Re: [PATCH v3 42/46] usb: gadget: move ep_matches() from epautoconf to udc-core
On 23.7.2015 21:46, Alan Stern wrote:
> On Thu, 23 Jul 2015, Petr Cvek wrote:
>
>> Hello,
>>
>> Is this:
>>
>> case USB_ENDPOINT_XFER_INT:
>> /* Bulk endpoints handle interrupt transfers,
>> * except the toggle-quirky iso-synch kind
>> */
>> if (!ep->caps.type_int && !ep->caps.type_bulk)
>> return 0;
>>
>> ... or original:
>>
>> switch (type) {
>> case USB_ENDPOINT_XFER_INT:
>> /* bulk endpoints handle interrupt transfers,
>> * except the toggle-quirky iso-synch kind
>> */
>> if ('s' == tmp[2]) {// == "-iso"
>> return 0;
>>
>> code still valid?
>
> It's hard to understand your question. Are you asking whether this
> code is still present in the current version of the kernel? You can
> find out for yourself easily enough.
I'm asking if there are still UDCs which require this quirk. If not, we should
change it to:
if ('n' != tmp[2]) {// != "-int"
so it will only return INT endpoints. Or if there are one or two which does not
support interrupt endpoints, it would be (in my opinion) more practical to make
a special case for them than force all other UDCs to use a BULK endpoint when
they have an INT endpoint.
>
>> It seems that it allows using a BULK endpoint for requested INT
>> endpoint. For my PXA27x machine the original code returns BULK EP
>> even with valid INT endpoint definition (because BULK EPs are defined
>> earlier than INT EPs).
>
> Yes, it does allow a bulk endpoint to be used when an interrupt
> endpoint was requested. However, it won't return a bulk endpoint if
> all the bulk endpoints are already in use.
A default PXA27x configuration returns BULK for requested INT. Which is
unfortunate, because PXA27x supports INT endpoints and has one predefined, but
this function find BULK first (one BULK is allocated and INT is never used).
>
>> This part of the code is from pre git era
>>
>> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2
>>
>> before pxa27x driver was written and only few chips was supported.
>> Does anyone know if the INT endpoints works now?
>
> What makes you think they might not work?
Because if they do, the ep_matches() function works poorly. It returns a BULK
for device (gadget) side, but host side (PC) thinks that this endpoint is an
INT and handles it in this way. But the PXA27x thinks the endpoint is a BULK
and handles it in its way (according to datasheet, settings for a BULK and an
INT transfers are not 100% compatible).
I cannot test "INT as BULK" behavior for the gadget functions, because all
gadgets which works on PXA27x does not use INT endpoints (some allocate the
endpoint but never use it).
>
> Alan Stern
>
Petr Cvek
--
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 42/46] usb: gadget: move ep_matches() from epautoconf to udc-core
On 23.7.2015 16:36, Felipe Balbi wrote:
> Hi,
>
> On Thu, Jul 23, 2015 at 06:40:40AM +0200, Petr Cvek wrote:
>> Hello,
>>
>> Is this:
>>
>> case USB_ENDPOINT_XFER_INT:
>> /* Bulk endpoints handle interrupt transfers,
>> * except the toggle-quirky iso-synch kind
>> */
>> if (!ep->caps.type_int && !ep->caps.type_bulk)
>> return 0;
>>
>> ... or original:
>>
>> switch (type) {
>> case USB_ENDPOINT_XFER_INT:
>> /* bulk endpoints handle interrupt transfers,
>> * except the toggle-quirky iso-synch kind
>> */
>> if ('s' == tmp[2]) {// == "-iso"
>> return 0;
>>
>> code still valid?
>>
>> It seems that it allows using a BULK endpoint for requested INT
>> endpoint. For my PXA27x machine the original code returns BULK EP even
>> with valid INT endpoint definition (because BULK EPs are defined
>> earlier than INT EPs).
>>
>> This part of the code is from pre git era
>>
>> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2
>>
>> before pxa27x driver was written and only few chips was supported.
>> Does anyone know if the INT endpoints works now?
>
> it's very difficult to read your reply when you remove all context.
>
Ah sorry, I was hacking around PXA UDC and found possible bug in one
ep_matches() function:
http://lxr.free-electrons.com/source/drivers/usb/gadget/epautoconf.c#L75
when searching for origin of this bug I have found about this new patch series
(someone could know how that part of code was created).
Petr Cvek
--
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 4/4] cp210x: Expose the part number in sysfs
On Fri, 24 Jul 2015 14:33:23 -0700 Greg Kroah-Hartman wrote: > On Fri, Jul 24, 2015 at 11:00:38PM +0200, Petr Tesarik wrote: > > On Fri, 24 Jul 2015 11:17:55 -0700 > > Greg Kroah-Hartman wrote: > > > > > On Fri, Jul 24, 2015 at 08:48:11AM +0200, Petr Tesarik wrote: > > > > From: Petr Tesarik > > > > > > > > Make it possible to read the cp210x part number from userspace by making > > > > it a sysfs attribute. > > > > > > > > Signed-off-by: Petr Tesarik > > > > > > All sysfs files need to be documented in Documentation/ABI/ > > > > Is this a recently added requirement? FWIW there are many undocumented > > sysfs attributes, even in code maintained by you. E.g. each usbserial > > (ttyUSB*) device has an attribute called "port_number" which is not > > documented. Or I'm blind... > > It's been a requirement for years. If we have missed any, please let me > know and we will add them. Sometimes we miss this when adding new > attributes, and many very old attributes never got documented. Fair enough. The example I gave is ancient... >[...] > > > > @@ -892,13 +905,19 @@ static int cp210x_startup(struct usb_serial > > > > *serial) &partnum, 1, USB_CTRL_GET_TIMEOUT); > > > > spriv->bPartNumber = partnum & 0xFF; > > > > > > > > - return 0; > > > > + result = device_create_file(&serial->interface->dev, > > > > + &dev_attr_part_number); > > > > > > You just raced with userspace, it will not properly see this attribute > > > :( > > > > Can you elaborate on this, please? AFAICS the file is created after all > > required objects had been instantiated already. Where's the race? > > That's the race. See this blog post for all the details: > > http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/ Thank you for the link! > > > Please never use device_create_file, use an attribute group assigned > > > to the tty device. Not the USB interface, that is only for USB > > > interface "things". >[...] > > OK, if the USB interface is the wrong place, what's a good place for > > such a thing? I don't insist on a sysfs attribute, but I don't agree > > with the tty device. > > Being a usb-serial driver, you don't have "access" to the main USB > device, so you are kind of violating some layering rules by taking this > on to the interface. True. This is still one of my concerns, but the GPIO functionality is minor compared to the serial bridge, so strictly following layering rules would be overkill. See Johan Hovold's answer in the thread about GPIO support for Silicon Labs cp210x USB serial: Indeed, you should just hang the gpio device directly off the usb-serial device [...] > Who / what is going to use this file? What is going to be done with > the information and to what device is that information going to be > associated with? Many thanks again. Thinking about it some more, once I'm done with my work, userspace can enumerate the gpio devices using sysfs without having to care about the specific model. The part number is only relevant internally to the cp210x driver. In short, there is in fact no user of this file, so the best option is to report the model in syslog and forget about a sysfs file. Simple. Petr -- 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 4/4] cp210x: Expose the part number in sysfs
On Fri, Jul 24, 2015 at 11:00:38PM +0200, Petr Tesarik wrote:
> On Fri, 24 Jul 2015 11:17:55 -0700
> Greg Kroah-Hartman wrote:
>
> > On Fri, Jul 24, 2015 at 08:48:11AM +0200, Petr Tesarik wrote:
> > > From: Petr Tesarik
> > >
> > > Make it possible to read the cp210x part number from userspace by making
> > > it a sysfs attribute.
> > >
> > > Signed-off-by: Petr Tesarik
> >
> > All sysfs files need to be documented in Documentation/ABI/
>
> Is this a recently added requirement? FWIW there are many undocumented
> sysfs attributes, even in code maintained by you. E.g. each usbserial
> (ttyUSB*) device has an attribute called "port_number" which is not
> documented. Or I'm blind...
It's been a requirement for years. If we have missed any, please let me
know and we will add them. Sometimes we miss this when adding new
attributes, and many very old attributes never got documented.
>
> > > ---
> > > drivers/usb/serial/cp210x.c | 21 -
> > > 1 file changed, 20 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/serial/cp210x.c
> > > b/drivers/usb/serial/cp210x.c index dbfc722..66de350 100644
> > > --- a/drivers/usb/serial/cp210x.c
> > > +++ b/drivers/usb/serial/cp210x.c
> > > @@ -871,11 +871,24 @@ static void cp210x_break_ctl(struct
> > > tty_struct *tty, int break_state) cp210x_set_config(port,
> > > CP210X_SET_BREAK, &state, 2); }
> > >
> > > +static ssize_t part_number_show(struct device *dev,
> > > + struct device_attribute *attr,
> > > char *buf) +{
> > > + struct usb_interface *intf = to_usb_interface(dev);
> > > + struct usb_serial *serial = usb_get_intfdata(intf);
> > > + struct cp210x_serial_private *spriv =
> > > usb_get_serial_data(serial); +
> > > + return sprintf(buf, "%i\n", spriv->bPartNumber);
> > > +}
> > > +
> > > +static DEVICE_ATTR_RO(part_number);
> > > +
> > > static int cp210x_startup(struct usb_serial *serial)
> > > {
> > > struct usb_host_interface *cur_altsetting;
> > > struct cp210x_serial_private *spriv;
> > > unsigned int partnum;
> > > + int result;
> > >
> > > spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
> > > if (!spriv)
> > > @@ -892,13 +905,19 @@ static int cp210x_startup(struct usb_serial
> > > *serial) &partnum, 1, USB_CTRL_GET_TIMEOUT);
> > > spriv->bPartNumber = partnum & 0xFF;
> > >
> > > - return 0;
> > > + result = device_create_file(&serial->interface->dev,
> > > + &dev_attr_part_number);
> >
> > You just raced with userspace, it will not properly see this attribute
> > :(
>
> Can you elaborate on this, please? AFAICS the file is created after all
> required objects had been instantiated already. Where's the race?
That's the race. See this blog post for all the details:
http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/
> > Please never use device_create_file, use an attribute group assigned
> > to the tty device. Not the USB interface, that is only for USB
> > interface "things".
>
> Well, I hesitated with adding it to the USB interface, but adding it to
> the tty device is definitely wrong. This is indeed an attribute of the
> device, not of the tty. If you look at the other CP210x thread, there's
> also a gpio device in the chip. I think it's totally silly to look
> inside a tty interface to see if there are any GPIO pins...
>
> OK, if the USB interface is the wrong place, what's a good place for
> such a thing? I don't insist on a sysfs attribute, but I don't agree
> with the tty device.
Being a usb-serial driver, you don't have "access" to the main USB
device, so you are kind of violating some layering rules by taking this
on to the interface.
Who / what is going to use this file? What is going to be done with
the information and to what device is that information going to be
associated with?
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 4/4] cp210x: Expose the part number in sysfs
On Fri, 24 Jul 2015 11:17:55 -0700
Greg Kroah-Hartman wrote:
> On Fri, Jul 24, 2015 at 08:48:11AM +0200, Petr Tesarik wrote:
> > From: Petr Tesarik
> >
> > Make it possible to read the cp210x part number from userspace by making
> > it a sysfs attribute.
> >
> > Signed-off-by: Petr Tesarik
>
> All sysfs files need to be documented in Documentation/ABI/
Is this a recently added requirement? FWIW there are many undocumented
sysfs attributes, even in code maintained by you. E.g. each usbserial
(ttyUSB*) device has an attribute called "port_number" which is not
documented. Or I'm blind...
> > ---
> > drivers/usb/serial/cp210x.c | 21 -
> > 1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/serial/cp210x.c
> > b/drivers/usb/serial/cp210x.c index dbfc722..66de350 100644
> > --- a/drivers/usb/serial/cp210x.c
> > +++ b/drivers/usb/serial/cp210x.c
> > @@ -871,11 +871,24 @@ static void cp210x_break_ctl(struct
> > tty_struct *tty, int break_state) cp210x_set_config(port,
> > CP210X_SET_BREAK, &state, 2); }
> >
> > +static ssize_t part_number_show(struct device *dev,
> > + struct device_attribute *attr,
> > char *buf) +{
> > + struct usb_interface *intf = to_usb_interface(dev);
> > + struct usb_serial *serial = usb_get_intfdata(intf);
> > + struct cp210x_serial_private *spriv =
> > usb_get_serial_data(serial); +
> > + return sprintf(buf, "%i\n", spriv->bPartNumber);
> > +}
> > +
> > +static DEVICE_ATTR_RO(part_number);
> > +
> > static int cp210x_startup(struct usb_serial *serial)
> > {
> > struct usb_host_interface *cur_altsetting;
> > struct cp210x_serial_private *spriv;
> > unsigned int partnum;
> > + int result;
> >
> > spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
> > if (!spriv)
> > @@ -892,13 +905,19 @@ static int cp210x_startup(struct usb_serial
> > *serial) &partnum, 1, USB_CTRL_GET_TIMEOUT);
> > spriv->bPartNumber = partnum & 0xFF;
> >
> > - return 0;
> > + result = device_create_file(&serial->interface->dev,
> > + &dev_attr_part_number);
>
> You just raced with userspace, it will not properly see this attribute
> :(
Can you elaborate on this, please? AFAICS the file is created after all
required objects had been instantiated already. Where's the race?
> Please never use device_create_file, use an attribute group assigned
> to the tty device. Not the USB interface, that is only for USB
> interface "things".
Well, I hesitated with adding it to the USB interface, but adding it to
the tty device is definitely wrong. This is indeed an attribute of the
device, not of the tty. If you look at the other CP210x thread, there's
also a gpio device in the chip. I think it's totally silly to look
inside a tty interface to see if there are any GPIO pins...
OK, if the USB interface is the wrong place, what's a good place for
such a thing? I don't insist on a sysfs attribute, but I don't agree
with the tty device.
Petr T
--
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] usb: gadget: udc: bdc: Minimal fix for a driver crash on disconnect
ep_dequeue() in bdc_ep.c was capturing the hw dequeue pointer incorrectly by reading the wrong register for the upper 32 bits. Signed-off-by: Al Cooper --- drivers/usb/gadget/udc/bdc/bdc_ep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/udc/bdc/bdc_ep.c b/drivers/usb/gadget/udc/bdc/bdc_ep.c index b04980c..1efa612 100644 --- a/drivers/usb/gadget/udc/bdc/bdc_ep.c +++ b/drivers/usb/gadget/udc/bdc/bdc_ep.c @@ -779,7 +779,7 @@ static int ep_dequeue(struct bdc_ep *ep, struct bdc_req *req) /* The current hw dequeue pointer */ tmp_32 = bdc_readl(bdc->regs, BDC_EPSTS0(0)); deq_ptr_64 = tmp_32; - tmp_32 = bdc_readl(bdc->regs, BDC_EPSTS0(1)); + tmp_32 = bdc_readl(bdc->regs, BDC_EPSTS1(0)); deq_ptr_64 |= ((u64)tmp_32 << 32); /* we have the dma addr of next bd that will be fetched by hardware */ -- 1.9.0.138.g2de3478 -- 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 V4 1/3] usb: Add Xen pvUSB protocol description
On Fri, Jul 24, 2015 at 11:57:47AM -0700, Greg KH wrote: > On Fri, Jul 24, 2015 at 05:51:04AM +0200, Juergen Gross wrote: > > On 07/23/2015 09:08 PM, Greg KH wrote: > > >On Thu, Jul 23, 2015 at 08:46:17AM +0200, Juergen Gross wrote: > > >>On 07/23/2015 06:36 AM, Greg KH wrote: > > >>>On Thu, Jul 23, 2015 at 06:04:39AM +0200, Juergen Gross wrote: > > On 07/23/2015 01:46 AM, Greg KH wrote: > > >On Tue, Jun 23, 2015 at 08:53:23AM +0200, Juergen Gross wrote: > > >>Add the definition of pvUSB protocol used between the pvUSB frontend > > >>in > > >>a Xen domU and the pvUSB backend in a Xen driver domain (usually > > >>Dom0). > > >> > > >>This header was originally provided by Fujitsu for Xen based on Linux > > >>2.6.18. > > >> > > >>Changes are: > > >>- adapt to Linux style guide > > >> > > >>Signed-off-by: Juergen Gross > > >>--- > > >> include/xen/interface/io/usbif.h | 252 > > >> +++ > > > > > >Why is this a different interface than the existing ones we have today > > >(i.e. usbip?) Where is it documented? Do the Xen developers / > > > > The interface definition is living in the Xen git repository for several > > years now: > > > > git://xenbits.xen.org/xen.git -> xen/include/public/io/usbif.h > > >>> > > >>>That's header file, not a document describing the api here. > > >> > > >>I suppose you want to tell me I should add something like: > > >> > > >>Documentation/DocBook/usb/API-struct-urb.html > > > > > >Somewhere that people can refer to that describes this public-facing API > > >that "must not ever be broken or changed". If you want to put it in a > > >documentation file, or a .h file, I don't care. > > > > > It is used e.g. in SUSE's xen kernel since 2.6.18. > > >>> > > >>>I am very aware of the amount of Xen crap in SuSE's kernel, don't use > > >>>that as an excuse for me to merge it to mainline :) > > >> > > >>:-) > > >> > > >>Wasn't meant as an excuse, just a hint why the interface can't be the > > >>same as for usbip. We have to ensure compatibility with those kernels > > > > > >This shouldn't be a kernel/kernel compability issue, as the api talks > > >between Xen and the OS, not between different OSs, right? > > > > Depends on where the backend is living. It's the backend the frontend is > > talking to. > > > > There is a backend in SUSE's kernels up to SLE12. So compatibility is > > to be maintained to those kernels. > > Note, just because a distro merged an out of tree patch, does not mean > that mainline has to accept the same api as-is :) > > > Looks as if in future there will be one in qemu. > > So there's only one other backend talking to this, in one distro? No. All distros which use QEMU will have it. > > > >>and possibly other operating systems (BSD?, Windows?) which already > > >>might be using pvUSB with a Dom0 based on the SUSE xen kernel. > > > > > >Are there other operating system drivers today that use this API? Is > > >this an API in the Xen core today that we have to support? > > > > Yes. > > Yes to both? Which other operating systems have such a driver? Windows XP, Windows 7, Windows 8, and so on using one of the Xen PV drivers. The old RHEL5 PV kernels, SLES kernels, and NetBSD [edit: Only the header file, odd] > > > >Some more background / descriptions would be nice to have. > > > > I guess a documentation file giving a brief explanation about the > > interfaces of Xen wouldn't be a bad idea. This could avoid discussions > > like this. > > Yes it would. > > > It shouldn't define each interface, but the classes of interfaces which > > are existing (between kernel and hypervisor, frontends and backends) > > and the stability requirements. Headers like the one we are discussing > > here could then refer to this document. > > Why shouldn't the protocol be documented? I presume you mean the wire spec? Like what is in the memory and exchanged between the guests? Classes sounds like an C++ thing :-) There is also the XenBus which does the negotiation of parameters (like the VirtioPCI spec) - that could be expanded to enumerate the USB drivers ones ? > > 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 V2 1/4] usb: gadget: udc: bdc: Fix a driver crash on disconnect
Hi, (please avoid top-posting) On Fri, Jul 24, 2015 at 03:20:10PM -0400, Alan Cooper wrote: > I'll send a single patch for the minimal bug fix for the rc and I'll > send the rest of the changes as a patch set for 4.3. that's all right -- balbi signature.asc Description: Digital signature
Re: [PATCH V2 1/4] usb: gadget: udc: bdc: Fix a driver crash on disconnect
I'll send a single patch for the minimal bug fix for the rc and I'll send the rest of the changes as a patch set for 4.3. Thanks Al On Thu, Jul 23, 2015 at 4:05 PM, Felipe Balbi wrote: > On Thu, Jul 23, 2015 at 03:23:26PM -0400, Alan Cooper wrote: >> On Wed, Jul 22, 2015 at 10:03 PM, Felipe Balbi wrote: >> > Hi, >> > >> > On Wed, Jul 22, 2015 at 06:27:29PM -0400, Alan Cooper wrote: >> >> On Wed, Jul 22, 2015 at 5:29 PM, Felipe Balbi wrote: >> >> > On Wed, Jul 22, 2015 at 04:26:57PM -0500, Felipe Balbi wrote: >> >> >> On Wed, Jul 22, 2015 at 04:58:07PM -0400, Al Cooper wrote: >> >> >> > V2 - Fix a compiler bug that happend when the config options >> >> >> > CONFIG_USB_GADGET_DEBUG and CONFIG_USB_GADGET_VERBOSE >> >> >> > were enabled. >> >> >> > >> >> >> > ep_dequeue() in bdc_ep.c was capturing the hw dequeue pointer >> >> >> > incorrectly by reading the wrong register for the upper 32 bits. >> >> >> > The header file defining the registers was incorrect. >> >> >> >> >> >> btw, the header file was really "incorrect" as long as you passed 0 to >> >> >> the argument :-p >> >> > >> >> > in fact, the minimal fix for this bug would be the one below: >> >> > >> >> > diff --git a/drivers/usb/gadget/udc/bdc/bdc_ep.c >> >> > b/drivers/usb/gadget/udc/bdc/bdc_ep.c >> >> > index b04980cf6dc4..1efa61265d8d 100644 >> >> > --- a/drivers/usb/gadget/udc/bdc/bdc_ep.c >> >> > +++ b/drivers/usb/gadget/udc/bdc/bdc_ep.c >> >> > @@ -779,7 +779,7 @@ static int ep_dequeue(struct bdc_ep *ep, struct >> >> > bdc_req *req) >> >> > /* The current hw dequeue pointer */ >> >> > tmp_32 = bdc_readl(bdc->regs, BDC_EPSTS0(0)); >> >> > deq_ptr_64 = tmp_32; >> >> > - tmp_32 = bdc_readl(bdc->regs, BDC_EPSTS0(1)); >> >> > + tmp_32 = bdc_readl(bdc->regs, BDC_EPSTS1(0)); >> >> > deq_ptr_64 |= ((u64)tmp_32 << 32); >> >> > >> >> > /* we have the dma addr of next bd that will be fetched by >> >> > hardware */ >> >> > >> >> > And $subject becomes a cleanup patch for v4.3. Can you make these >> >> > changes, please ? >> >> >> >> Yes, I can make these changes, but first let me explain why I did it this >> >> way. >> >> The 8 End Point Status registers are 32 bit consecutive registers, so >> >> defining them as: >> >> #define BDC_EPSTS0(n) (0x60 + (n * 0x10)) >> >> #define BDC_EPSTS1(n) (0x64 + (n * 0x10)) >> >> #define BDC_EPSTS2(n) (0x68 + (n * 0x10)) >> >> #define BDC_EPSTS3(n) (0x6c + (n * 0x10)) >> >> #define BDC_EPSTS4(n) (0x70 + (n * 0x10)) >> >> #define BDC_EPSTS5(n) (0x74 + (n * 0x10)) >> >> #define BDC_EPSTS6(n) (0x78 + (n * 0x10)) >> >> #define BDC_EPSTS7(n) (0x7c + (n * 0x10)) >> >> seems misleading, does not reflect the hardware and using anything >> >> other than (0) would get you to some other unexpected register and >> >> should be considered a coding error. >> > >> > it sure is, but the minimal patch for -rc is what I sent above :-) As >> > long as you pass 0 as parameters, all your offsets are correct, so >> > removing the parameter (which must be always zero) is, actually, >> > refactoring happening. >> > >> >> OK, I think I understand. You're asking me to split the bug fix patch >> into two patches, one is a minimal bug fix patch for -rc and the other >> is a cleanup patch (fix the header file) that will go into 4.3. Is >> that correct? > > correct. > >> >> I think the original hardware spec had each End Point Status as a >> >> block of registers but the first silicon had them as single registers >> >> and they are expected to stay that way. >> > >> > I believe they wanted to have: >> > >> > #define BDC_EPSTS(n)(0x60 + (n * 0x4)) >> > >> > and that way you can access each EP by passing the correct argument to >> > that macro. >> > >> >> Actually, the 8 EPSTS registers are all different and together contain >> the info for 1 end point. The original plan was to have 4 32bit >> registers per end point and enough sets of these registers for all >> endpoints, but this turned out to be too expensive in silicon so they >> changed it to 1 set of 8 registers that are updated when an end point >> is stopped. > > oh ok. thanks > > -- > balbi -- 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 V4 1/3] usb: Add Xen pvUSB protocol description
On Fri, Jul 24, 2015 at 05:51:04AM +0200, Juergen Gross wrote: > On 07/23/2015 09:08 PM, Greg KH wrote: > >On Thu, Jul 23, 2015 at 08:46:17AM +0200, Juergen Gross wrote: > >>On 07/23/2015 06:36 AM, Greg KH wrote: > >>>On Thu, Jul 23, 2015 at 06:04:39AM +0200, Juergen Gross wrote: > On 07/23/2015 01:46 AM, Greg KH wrote: > >On Tue, Jun 23, 2015 at 08:53:23AM +0200, Juergen Gross wrote: > >>Add the definition of pvUSB protocol used between the pvUSB frontend in > >>a Xen domU and the pvUSB backend in a Xen driver domain (usually Dom0). > >> > >>This header was originally provided by Fujitsu for Xen based on Linux > >>2.6.18. > >> > >>Changes are: > >>- adapt to Linux style guide > >> > >>Signed-off-by: Juergen Gross > >>--- > >> include/xen/interface/io/usbif.h | 252 > >> +++ > > > >Why is this a different interface than the existing ones we have today > >(i.e. usbip?) Where is it documented? Do the Xen developers / > > The interface definition is living in the Xen git repository for several > years now: > > git://xenbits.xen.org/xen.git -> xen/include/public/io/usbif.h > >>> > >>>That's header file, not a document describing the api here. > >> > >>I suppose you want to tell me I should add something like: > >> > >>Documentation/DocBook/usb/API-struct-urb.html > > > >Somewhere that people can refer to that describes this public-facing API > >that "must not ever be broken or changed". If you want to put it in a > >documentation file, or a .h file, I don't care. > > > It is used e.g. in SUSE's xen kernel since 2.6.18. > >>> > >>>I am very aware of the amount of Xen crap in SuSE's kernel, don't use > >>>that as an excuse for me to merge it to mainline :) > >> > >>:-) > >> > >>Wasn't meant as an excuse, just a hint why the interface can't be the > >>same as for usbip. We have to ensure compatibility with those kernels > > > >This shouldn't be a kernel/kernel compability issue, as the api talks > >between Xen and the OS, not between different OSs, right? > > Depends on where the backend is living. It's the backend the frontend is > talking to. > > There is a backend in SUSE's kernels up to SLE12. So compatibility is > to be maintained to those kernels. Note, just because a distro merged an out of tree patch, does not mean that mainline has to accept the same api as-is :) > Looks as if in future there will be one in qemu. So there's only one other backend talking to this, in one distro? > >>and possibly other operating systems (BSD?, Windows?) which already > >>might be using pvUSB with a Dom0 based on the SUSE xen kernel. > > > >Are there other operating system drivers today that use this API? Is > >this an API in the Xen core today that we have to support? > > Yes. Yes to both? Which other operating systems have such a driver? > >Some more background / descriptions would be nice to have. > > I guess a documentation file giving a brief explanation about the > interfaces of Xen wouldn't be a bad idea. This could avoid discussions > like this. Yes it would. > It shouldn't define each interface, but the classes of interfaces which > are existing (between kernel and hypervisor, frontends and backends) > and the stability requirements. Headers like the one we are discussing > here could then refer to this document. Why shouldn't the protocol be documented? 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 4/4] cp210x: Expose the part number in sysfs
On Fri, Jul 24, 2015 at 08:48:11AM +0200, Petr Tesarik wrote:
> From: Petr Tesarik
>
> Make it possible to read the cp210x part number from userspace by making
> it a sysfs attribute.
>
> Signed-off-by: Petr Tesarik
All sysfs files need to be documented in Documentation/ABI/
> ---
> drivers/usb/serial/cp210x.c | 21 -
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index dbfc722..66de350 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -871,11 +871,24 @@ static void cp210x_break_ctl(struct tty_struct *tty,
> int break_state)
> cp210x_set_config(port, CP210X_SET_BREAK, &state, 2);
> }
>
> +static ssize_t part_number_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct usb_interface *intf = to_usb_interface(dev);
> + struct usb_serial *serial = usb_get_intfdata(intf);
> + struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
> +
> + return sprintf(buf, "%i\n", spriv->bPartNumber);
> +}
> +
> +static DEVICE_ATTR_RO(part_number);
> +
> static int cp210x_startup(struct usb_serial *serial)
> {
> struct usb_host_interface *cur_altsetting;
> struct cp210x_serial_private *spriv;
> unsigned int partnum;
> + int result;
>
> spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
> if (!spriv)
> @@ -892,13 +905,19 @@ static int cp210x_startup(struct usb_serial *serial)
> &partnum, 1, USB_CTRL_GET_TIMEOUT);
> spriv->bPartNumber = partnum & 0xFF;
>
> - return 0;
> + result = device_create_file(&serial->interface->dev,
> + &dev_attr_part_number);
You just raced with userspace, it will not properly see this attribute
:(
Please never use device_create_file, use an attribute group assigned to
the tty device. Not the USB interface, that is only for USB interface
"things".
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: Several races in "usbnet" module (kernel 4.1.x)
21.07.2015 15:04, Oliver Neukum пишет:
On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:
Hi,
I have recently found several data races in "usbnet" module, checked on
vanilla kernel 4.1.0 on x86_64. The races do actually happen, I have
confirmed it by adding delays and using hardware breakpoints to detect
the conflicting memory accesses (with RaceHound tool,
https://github.com/winnukem/racehound).
I have not analyzed yet how harmful these races are (if they are), but
it is better to report them anyway, I think.
Everything was checked using YOTA 4G LTE Modem that works via "usbnet"
and "cdc_ether" kernel modules.
--
[Race #1]
Race on skb_queue ('next' pointer) between usbnet_stop() and rx_complete().
Reproduced that by unplugging the device while the system was
downloading a large file from the Net.
Here is part of the call stack with the code where the changes to the
queue happen:
#0 __skb_unlink (skbuff.h:1517)
prev->next = next;
#1 defer_bh (usbnet.c:430)
spin_lock_irqsave(&list->lock, flags);
old_state = entry->state;
entry->state = state;
__skb_unlink(skb, list);
spin_unlock(&list->lock);
spin_lock(&dev->done.lock);
__skb_queue_tail(&dev->done, skb);
if (dev->done.qlen == 1)
tasklet_schedule(&dev->bh);
spin_unlock_irqrestore(&dev->done.lock, flags);
#2 rx_complete (usbnet.c:640)
state = defer_bh(dev, skb, &dev->rxq, state);
At the same time, the following code repeatedly checks if the queue is
empty and reads the same values concurrently with the above changes:
#0 usbnet_terminate_urbs (usbnet.c:765)
/* maybe wait for deletions to finish. */
while (!skb_queue_empty(&dev->rxq)
&& !skb_queue_empty(&dev->txq)
&& !skb_queue_empty(&dev->done)) {
schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
set_current_state(TASK_UNINTERRUPTIBLE);
netif_dbg(dev, ifdown, dev->net,
"waited for %d urb completions\n", temp);
}
#1 usbnet_stop (usbnet.c:806)
if (!(info->flags & FLAG_AVOID_UNLINK_URBS))
usbnet_terminate_urbs(dev);
For example, it is possible that the skb is removed from dev->rxq by
__skb_unlink() before the check "!skb_queue_empty(&dev->rxq)" in
usbnet_terminate_urbs() is made. It is also possible in this case that
the skb is added to dev->done queue after "!skb_queue_empty(&dev->done)"
is checked. So usbnet_terminate_urbs() may stop waiting and return while
dev->done queue still has an item.
Hi,
your analysis is correct and it looks like in addition to your proposed
fix locking needs to be simplified and a common lock to be taken.
Suggestions?
Just an idea, I haven't tested it.
How about moving the operations with dev->done under &list->lock in
defer_bh, while keeping dev->done.lock too and changing
usbnet_terminate_urbs() as described below?
Like this:
@@ -428,12 +428,12 @@ static enum skb_state defer_bh(struct usbnet *dev,
struct sk_buff *skb,
old_state = entry->state;
entry->state = state;
__skb_unlink(skb, list);
- spin_unlock(&list->lock);
spin_lock(&dev->done.lock);
__skb_queue_tail(&dev->done, skb);
if (dev->done.qlen == 1)
tasklet_schedule(&dev->bh);
- spin_unlock_irqrestore(&dev->done.lock, flags);
+ spin_unlock(&dev->done.lock);
+ spin_unlock_irqrestore(&list->lock, flags);
return old_state;
}
---
usbnet_terminate_urbs() can then be changed as follows:
@@ -749,6 +749,20 @@ EXPORT_SYMBOL_GPL(usbnet_unlink_rx_urbs);
/*-*/
+static void wait_skb_queue_empty(struct sk_buff_head *q)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&q->lock, flags);
+ while (!skb_queue_empty(q)) {
+ spin_unlock_irqrestore(&q->lock, flags);
+ schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
+ set_current_state(TASK_UNINTERRUPTIBLE);
+ spin_lock_irqsave(&q->lock, flags);
+ }
+ spin_unlock_irqrestore(&q->lock, flags);
+}
+
// precondition: never called in_interrupt
static void usbnet_terminate_urbs(struct usbnet *dev)
{
@@ -762,14 +776,11 @@ static void usbnet_terminate_urbs(struct usbnet *dev)
unlink_urbs(dev, &dev->rxq);
/* maybe wait for deletions to finish. */
- while (!skb_queue_empty(&dev->rxq)
- && !skb_queue_empty(&dev->txq)
- && !skb_queue_empty(&dev->done)) {
- schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
- set_current_state(TASK_UNINTERRUPTIBLE);
- netif_dbg(dev, ifdown, dev->net,
- "waited for %d urb completions\n", temp);
Re: GPIO support for Silicon Labs cp210x USB serial
Petr, We will be happy to test, code review, and otherwise contribute to your effort. We're using the cp2105 dual-uart device in our product. It has two logical serial ports and two sets of GPIOs so I think its a good test case. best, John. On Fri, 2015-07-24 at 13:08 +0200, Petr Tesařík wrote: > On Thu, 23 Jul 2015 14:59:32 -0700 > "John D. Blair" > wrote: > > > On Thu, 2015-07-23 at 18:00 +0200, Johan Hovold wrote: > > > The short answer is that we do not want custom driver ioctls. Any gpio > > > implementation needs to based on gpiolib, which provides a > > > standardised > > > interface. > > > > > I understand. > > > > We will evaluate and decide if the work is worth it for us to do. > > FWIW I'm planning to do this work. I've just started cleaning up the > cp210x sources, but my goal is to get gpio support (and not using > IOCTLs). > > One of the challenges is that the CP210x device is in fact two logical > devices (the serial port and the GPIO), so theoretically, there should > be two drivers and a common module to handle resource conflicts, but I > believe this would be overkill. > > I'll see how it goes with the upstream maintainers. ;-) > > Petr T -- 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: xhci_hcd 0000:00:14.0: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 2 comp_code 1
On Friday 24 of July 2015, Mathias Nyman wrote: > On 24.07.2015 14:59, Mathias Nyman wrote: > > On 22.07.2015 17:12, Arkadiusz Miskiewicz wrote: > >> On Tuesday 21 of July 2015, Mathias Nyman wrote: > >>> On 20.07.2015 23:13, Arkadiusz Miskiewicz wrote: > On Saturday 18 of July 2015, Arkadiusz Miskiewicz wrote: > > Hi. > > > > I'm on 4.2.0-rc2-00077-gf760b87 kernel and while trying to copy some > > file from usb storage (sata disk behind sata-usb bridge or pendrive; > > hapens in > > > both cases) copying process hangs just early after start with: > Looks like suspend & resume is enough. Reloading bluetooth firmware > done by kernel triggers problem: > > [ 106.302783] rtc_cmos 00:02: System wakeup disabled by ACPI > [ 106.313280] PM: resume of devices complete after 3003.032 msecs > [ 106.314079] Restarting tasks ... done. > [ 106.326434] Bluetooth: hci0: read Intel version: 370710018002030d00 > [ 106.330422] Bluetooth: hci0: Intel Bluetooth firmware file: > intel/ibt-hw-37.7.10-fw-1.80.2.3.d.bseq [ 106.398223] xhci_hcd > :00:14.0: ERROR Transfer event TRB DMA ptr not part of current TD > ep_index 0 comp_code 1 > > > > Thanks for the logs, They show that the error is related to transfer > > descriptors that wrap around on the endpoint ring buffer by exactly one > > transfer block. > > > > I don't know yet why this happens, and I might need some help running > > additional debug patches to solve this. I'll take a more in depth look > > at the code one more time first. > > I think I found something, The recent ring segment size increase exposed an > off by one error that has been in the driver for a long time. But you need > to be unlucky and have your memory pages allocated in a specific order to > trigger it. > > small fix, looks like this: > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 94416ff..77da8fe 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -82,7 +82,7 @@ dma_addr_t xhci_trb_virt_to_dma(struct xhci_segment *seg, > return 0; > /* offset in TRBs */ > segment_offset = trb - seg->trbs; > - if (segment_offset > TRBS_PER_SEGMENT) > + if (segment_offset > TRBS_PER_SEGMENT - 1) > return 0; > return seg->dma + (segment_offset * sizeof(*trb)); > } > > > Patch attached, could you try it out? Works fine with this patch, so: Tested-by: Arkadiusz Miśkiewicz Thanks! ps. please push to stable@, too > > Thanks > -Mathias -- Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org ) -- 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
On Mon, 22 Jun 2015, James Bottomley wrote: > On Mon, 2015-06-22 at 13:48 -0400, Alan Stern wrote: > > On Mon, 22 Jun 2015, James Bottomley wrote: > > > > > On Mon, 2015-06-22 at 13:30 -0400, Alan Stern wrote: > > > > On Mon, 22 Jun 2015, James Bottomley wrote: > > > > > > > > > I'm not sure I entirely like this: we are back again treating data > > > > > corruption problems silently. > > > > > > > > > > However, I also believe treating a single flush failure as a critical > > > > > filesystem error is also wrong: The data's all there correctly; all > > > > > it > > > > > does is introduce a potential window were the FS could get corrupted > > > > > in > > > > > the unlikely event the system crashed. > > > > > > > > > > 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. > > > > > > > > How about something like this patch? It transforms FS FLUSH into a > > > > > log > > > > > warning from an error but preserves the error on any other path. > > > > > You'll > > > > > still get a fairly continuous dump of warnings for one of these > > > > > devices, > > > > > though ... do they respond to mode selects turning off the writeback? > > > > > > > > I would be very surprised if any of those drives support MODE SELECT at > > > > all. > > > > > > I assume the cache type attribute file you refer to above is just > > > pretending their cache is write through rather than actually setting it > > > to be so? > > > > Yes; I'm referring to cache_type_store() in sd.c, and writing > > "temporary write through", which does not issue a MODE SELECT command. > > It would be easy enough for people to try leaving out the "temporary", > > but I don't expect it to work. > > > > > The original IDE device had no way of turning their cache > > > types to write through either, but the manufacturers were eventually > > > convinced of the error of their ways. > > > > In this case the stupidity resides in the USB-ATA bridge. You can see > > the gory details at > > > > https://bugzilla.kernel.org/show_bug.cgi?id=89511#c19 > > OK, so that says the SAT in the bridge doesn't know what to do with > MODE_SELECT (probably unsurprising given that it's a usb bridge). the > SATA disk should respond to the ATA command SET FEATURES, though. > Presuming we can get it through the bridge. > > You can try it with > > hdparm -W 0 > > optionally with --prefer_ata_12 to do the 12 instead of 16 byte > encapsulation and see if that makes a difference. Dan Williams recently posted the content below to the Bugzilla report: > I have a drive with this problem and I tested 4.0.8 with this hunk > added to the patch above. > > UNUSUAL_DEV( 0x0bc2, 0x0888, 0x, 0x, > "Seagate", > "USB 2.0 Pocket Hard Drive", > USB_SC_DEVICE, USB_PR_DEVICE, NULL, > US_FL_NO_SYNCHRONIZE_CACHE), > > I don't have the linux-usb thread around so I can reply to > http://www.spinics.net/lists/linux-usb/msg126364.html, but here's the > result of: > > [dcbw@localhost ~]$ sudo hdparm -W 0 --prefer-ata12 /dev/sdb > > /dev/sdb: > setting drive write-caching to 0 (off) > SG_IO: bad/missing sense data, sb[]: 70 00 05 00 00 00 00 0a 00 00 00 > 00 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 In English: "Illegal Request: Invalid command operation code". Apparently there's no way to tell the drive to change its caching. > SG_IO: bad/missing sense data, sb[]: 70 00 05 00 00 00 00 0a 00 00 00 > 00 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SG_IO: bad/missing sense data, sb[]: 70 00 05 00 00 00 00 0a 00 00 00 > 00 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SG_IO: bad/missing sense data, sb[]: 70 00 05 00 00 00 00 0a 00 00 00 > 00 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SG_IO: bad/missing sense data, sb[]: 70 00 05 00 00 00 00 0a 00 00 00 > 00 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > write-caching = not supported > > What was the upstream resolution on this one? So far there isn't any. 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: Several races in "usbnet" module (kernel 4.1.x)
23.07.2015 12:15, Oliver Neukum пишет:
On Wed, 2015-07-22 at 21:33 +0300, Eugene Shatokhin wrote:
The following part is not necessary, I think. usbnet_bh() does not
touch
EVENT_NO_RUNTIME_PM bit explicitly and these bit operations are
atomic
w.r.t. each other.
+ mpn |= !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
+ /* in case the bh reset a flag */
Yes, they are atomic w.r.t. each other. And that limitation worries me.
I am considering architectures which do atomic operations with
spinlocks. And this code mixes another operation into it. Can
this happen?
CPU A CPU B
take lock
read old value
set value to 0
clear bit
write back changed value
release lock
From what I see now in Documentation/atomic_ops.txt, stores to the
properly aligned memory locations are in fact atomic.
So, I think, the situation you described above cannot happen for
dev->flags, which is good. No need to address that in the patch. The
race might be harmless after all.
If I understand the code correctly now, dev->flags is set to 0 in
usbnet_stop() so that the worker function (usbnet_deferred_kevent) would
do nothing, should it start later. If so, how about adding memory
barriers for all CPUs to see dev->flags is 0 before other things?
The patch could look like this then:
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 3c86b10..d87b9c7 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -778,7 +778,7 @@ int usbnet_stop (struct net_device *net)
{
struct usbnet *dev = netdev_priv(net);
struct driver_info *info = dev->driver_info;
- int retval, pm;
+ int retval, pm, mpn;
clear_bit(EVENT_DEV_OPEN, &dev->flags);
netif_stop_queue (net);
@@ -813,14 +813,17 @@ int usbnet_stop (struct net_device *net)
* can't flush_scheduled_work() until we drop rtnl (later),
* else workers could deadlock; so make workers a NOP.
*/
+ mpn = !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
dev->flags = 0;
+ smp_mb(); /* make sure the workers see that dev->flags == 0 */
+
del_timer_sync (&dev->delay);
tasklet_kill (&dev->bh);
+
if (!pm)
usb_autopm_put_interface(dev->intf);
- if (info->manage_power &&
- !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags))
+ if (info->manage_power && mpn)
info->manage_power(dev, 0);
else
usb_autopm_put_interface(dev->intf);
@@ -1078,6 +1081,9 @@ usbnet_deferred_kevent (struct work_struct *work)
container_of(work, struct usbnet, kevent);
int status;
+ /* See the changes in dev->flags from other CPUs. */
+ smp_mb();
+
/* usb_clear_halt() needs a thread context */
if (test_bit (EVENT_TX_HALT, &dev->flags)) {
unlink_urbs (dev, &dev->txq);
What do you think?
Regards,
Eugene
--
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: xhci_hcd 0000:00:14.0: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 2 comp_code 1
On 24.07.2015 16:53, Peter Stuge wrote: > Mathias Nyman wrote: >> +++ b/drivers/usb/host/xhci-ring.c >> @@ -82,7 +82,7 @@ dma_addr_t xhci_trb_virt_to_dma(struct xhci_segment *seg, >> return 0; >> /* offset in TRBs */ >> segment_offset = trb - seg->trbs; >> - if (segment_offset > TRBS_PER_SEGMENT) >> + if (segment_offset > TRBS_PER_SEGMENT - 1) > > Maybe change the > comparison to >= rather than add the extra "- 1"? Yes, sure, I'll change it if this really was the cause. Just happy to finally find a probable cause after staring at logs and code for some time -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: xhci_hcd 0000:00:14.0: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 2 comp_code 1
Mathias Nyman wrote: > +++ b/drivers/usb/host/xhci-ring.c > @@ -82,7 +82,7 @@ dma_addr_t xhci_trb_virt_to_dma(struct xhci_segment *seg, > return 0; > /* offset in TRBs */ > segment_offset = trb - seg->trbs; > - if (segment_offset > TRBS_PER_SEGMENT) > + if (segment_offset > TRBS_PER_SEGMENT - 1) Maybe change the > comparison to >= rather than add the extra "- 1"? //Peter -- 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: Multiple drives on JMS56x-based sata-usb docking station.
On Fri, 24 Jul 2015, Giulio Bernardi wrote: > This is the trace I got after I used this option: > options usb-storage quirks=152d:1567:u This trace is pretty clear; the system does not scan for logical units beyond LUN 0, even though the device contains two logical units. In older kernels this was controlled by CONFIG_SCSI_MULTI_LUN, but that setting was removed in the 3.17 kernel. In more recent kernels, LUN scanning is controlled by a bunch of software rules, but I don't see any reason why they would prevent LUN 1 from being scanned on your device. If you want to see what's going on, read through the scsi_sequential_lun_scan() routine in drivers/scsi/scsi_scan.c. The most important thing to check is the value of max_dev_lun for your device. Also, the BLIST_FORCELUN bit should be set in bflags. 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: xhci_hcd 0000:00:14.0: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 2 comp_code 1
On 24.07.2015 14:59, Mathias Nyman wrote: > On 22.07.2015 17:12, Arkadiusz Miskiewicz wrote: >> >> On Tuesday 21 of July 2015, Mathias Nyman wrote: >>> On 20.07.2015 23:13, Arkadiusz Miskiewicz wrote: On Saturday 18 of July 2015, Arkadiusz Miskiewicz wrote: > Hi. > > I'm on 4.2.0-rc2-00077-gf760b87 kernel and while trying to copy some > file from usb storage (sata disk behind sata-usb bridge or pendrive; > hapens in > both cases) copying process hangs just early after start with: Looks like suspend & resume is enough. Reloading bluetooth firmware done by kernel triggers problem: [ 106.302783] rtc_cmos 00:02: System wakeup disabled by ACPI [ 106.313280] PM: resume of devices complete after 3003.032 msecs [ 106.314079] Restarting tasks ... done. [ 106.326434] Bluetooth: hci0: read Intel version: 370710018002030d00 [ 106.330422] Bluetooth: hci0: Intel Bluetooth firmware file: intel/ibt-hw-37.7.10-fw-1.80.2.3.d.bseq [ 106.398223] xhci_hcd :00:14.0: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 0 comp_code 1 >>> > > Thanks for the logs, They show that the error is related to transfer > descriptors that wrap around > on the endpoint ring buffer by exactly one transfer block. > > I don't know yet why this happens, and I might need some help running > additional debug > patches to solve this. I'll take a more in depth look at the code one more > time first. > I think I found something, The recent ring segment size increase exposed an off by one error that has been in the driver for a long time. But you need to be unlucky and have your memory pages allocated in a specific order to trigger it. small fix, looks like this: diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 94416ff..77da8fe 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -82,7 +82,7 @@ dma_addr_t xhci_trb_virt_to_dma(struct xhci_segment *seg, return 0; /* offset in TRBs */ segment_offset = trb - seg->trbs; - if (segment_offset > TRBS_PER_SEGMENT) + if (segment_offset > TRBS_PER_SEGMENT - 1) return 0; return seg->dma + (segment_offset * sizeof(*trb)); } Patch attached, could you try it out? Thanks -Mathias >From 10e909ee20846793e41973941b1367e2303ec313 Mon Sep 17 00:00:00 2001 From: Mathias Nyman Date: Fri, 24 Jul 2015 15:56:23 +0300 Subject: [PATCH] xhci: fix off by one error in TRB DMA address boundary check We need to check that a TRB is part of the current segment in use before calculating its DMA address. Previously a ring segment didn't use a full memory page, and every new ring segment got a new memory page, so the off by one error in checking the upper bound was never seen. Now that we use a full memory page, 256 TRBs (4096 bytes) the off by one caused issues as it doesnt catch the case when a TRB was the first element of the next segment. This is triggered if the virtual memory pages for a ring segment are next to each in increasing order where the ring buffer wraps around and causes errors like: [ 106.398223] xhci_hcd :00:14.0: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 0 comp_code 1 [ 106.398230] xhci_hcd :00:14.0: Looking for event-dma fffd3000 trb-start fffd4fd0 trb-end fffd5000 seg-start fffd4000 seg-end fffd4ff0 the trb-end address is one outside the end-seg address. Signed-off-by: Mathias Nyman --- drivers/usb/host/xhci-ring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 94416ff..77da8fe 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -82,7 +82,7 @@ dma_addr_t xhci_trb_virt_to_dma(struct xhci_segment *seg, return 0; /* offset in TRBs */ segment_offset = trb - seg->trbs; - if (segment_offset > TRBS_PER_SEGMENT) + if (segment_offset > TRBS_PER_SEGMENT - 1) return 0; return seg->dma + (segment_offset * sizeof(*trb)); } -- 1.8.3.2
Re: [PATCH 1/1] Avoid usb reset crashes by making tty_io cdevs truly dynamic
On Thu, 23 Jul 2015, Richard Watts wrote:
> When the USB reset happens, we get a bunch of complaints from the
> kernel.
>
> Some of these are to do with races on the kobjects associated with the
> sysfs entries for the ttyACM0 device. They turn out not to be fatal,
> and have their own patch series ('Attempt to cope with device changes
> and delayed kobject deallocation' on linux-kernel).
>
> The fatal one turns out to be an execution path that goes like this:
>
> 1 USB device declares itself to be CDC
> 2 tty driver fires up and allocates a cdev for the relevant tty.
Does this happen in the same thread as 1?
> 3 driver->cdevs[0].kobj gets initialised as part of the cdev_alloc()
> 4 USB reset happens, queueing driver->cdevs[0].kobj for release.
1 and 4 should be mutually exclusive. Probing and reset both acquire
the USB device's mutex.
> 5 The tty driver calls cdev_init(&driver->cdevs[0]), which
> reinitialises driver->cdevs[0].kobj with a refcount of 1.
Presumably this happens in the same thread as 1, 2, and 3? Which means
it should be mutually exclusive with 4 -- it should happen _before_ 4,
not after.
By the way, kobjects should _never_ be reinitialized. They are
initialized just once, when they are created. If something initializes
them again afterward, that's a bug.
> 6 tty driver starts using that new cdev, queueing an operation on it.
> This causes a timer entry to be added including the kobj.
> 7 At this point, the release we scheduled in (4) happens and the
> members of kobj are deallocated.
> 8 Someone allocates the newly released memory for one of the members of
> cdriver->cdevs[0].kobj somewhere else and overwrites it.
> 9 The timer goes off.
> 10 Boom
>
> My patch (ham-fistedly) fixes this by ensuring that because we
> never reuse the cdev pointer, we are never fooled into reinitialising
> a kobject queued for deletion.
>
> I'm not all that familiar with how the locking should go here, and
> there is a definite argument that under non CONFIG_DEBUG_KOBJECT_RELEASE
> conditions, the kobject_release() would have happened by 5, and
> therefore this situation should never exist "for real".
I can tell you a little about locking in the USB subsystem (don't know
about the TTY subsystem). In particular, USB probing and reset are
mutually exclusive.
> .. but (a) that makes it rather hard to test kernels with
> CONFIG_DEBUG_KOBJECT_RELEASE, and (b) my customer's crashes have
> (allegedly) now gone away even without CONFIG_DEBUG_KOBJECT_RELEASE
> set.
In general, delaying an object's release should make no difference at
all (except for the fact that the memory is temporarily unavailable for
other purposes). Objects don't get released until their refcounts are
0, meaning that they are not in use anywhere. If an object is still in
use (being initialized, or on a list, etc.) then its refcount should
not be 0. If it is, that's a bug.
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 v2 4/6] usb: dwc3; ep0: Modify _dwc3_ep0_start_trans_ API to take 'chain' parameter
Hello. On 7/24/2015 9:47 AM, Kishon Vijay Abraham I wrote: No functional change. Added a new parameter in _dwc3_ep0_start_trans_ to indicate whether the TRB is a chained TRB or last TRB. This is in preparation for adding chained TRB support for ep0. Signed-off-by: Kishon Vijay Abraham I --- drivers/usb/dwc3/ep0.c | 15 --- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 4998074..d1a2be1 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -56,7 +56,7 @@ static const char *dwc3_ep0_state_string(enum dwc3_ep0_state state) } static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum, dma_addr_t buf_dma, - u32 len, u32 type) + u32 len, u32 type, unsigned chain) I'm seeing you always pass *false*, so why not just *bool* here? [...] 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: [PATCH v2] USB: sierra: add 1199:68AB device ID
On 24.07.2015 14:37, Johan Hovold wrote: On Fri, Jul 24, 2015 at 07:22:45PM +0700, Lars Melin wrote: The interface layout of 1199:68ab does not look like 1199:68aa does, it has instead more in common with MC879x modules 1199:683c/683d which also are composite devices with 7 interfaces (0..6) and also MDM62xx based as the AR8550. The major difference is from what I can see only the interface attributes 02/02/01 on interfaces 3 and 4 on the AR8550, they are vendor specific ff/ff/ff on MC879x modules. So the Sierra driver seems to be the right driver for the AR8550, I only find it strange that Sierra hasn't added it. Somehow I would also expect them to tell their customers (which I assume Dirkäs employer is) which driver to use.. Thanks for the input, Lars. Dirk, care to respin the patch and drop the blacklisting and direct-ip comment as already discussed? Sure, I'll do that. Thanks for the discussion! Best regards Dirk -- 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 v2] USB: sierra: add 1199:68AB device ID
On Fri, Jul 24, 2015 at 07:22:45PM +0700, Lars Melin wrote: > The interface layout of 1199:68ab does not look like 1199:68aa does, it > has instead more in common with MC879x modules 1199:683c/683d which also > are composite devices with 7 interfaces (0..6) and also MDM62xx based as > the AR8550. > The major difference is from what I can see only the interface > attributes 02/02/01 on interfaces 3 and 4 on the AR8550, they are > vendor specific ff/ff/ff on MC879x modules. > > So the Sierra driver seems to be the right driver for the AR8550, I only > find it strange that Sierra hasn't added it. > Somehow I would also expect them to tell their customers (which I assume > Dirkäs employer is) which driver to use.. Thanks for the input, Lars. Dirk, care to respin the patch and drop the blacklisting and direct-ip comment as already discussed? 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
Re: [PATCH v2] USB: sierra: add 1199:68AB device ID
On 2015-07-24 16:30, Johan Hovold wrote:
On Fri, Jul 24, 2015 at 10:59:48AM +0200, Dirk Behme wrote:
On 24.07.2015 10:45, Johan Hovold wrote:
On Fri, Jul 24, 2015 at 10:17:24AM +0200, Dirk Behme wrote:
On 23.07.2015 19:26, Johan Hovold wrote:
This is probably the right driver, but I'm confused over the DirectIP
bits. Those interfaces are lacking AFAICT and still your comment claims
it's a DirectIP modem (that would also require it to be added not the
network driver).
As we just use this device as ttyUSBx, it seems the "Direct IP modems"
comment might be wrong and a copy & paste error. I'll remove it in v3.
Ok?
Sounds good. And drop the blacklisting too.
Just for better understanding:
We took the existing 0x68AA device
{ USB_DEVICE_AND_INTERFACE_INFO(0x1199, 0x68AA, 0xFF, 0xFF, 0xFF),
.driver_info = (kernel_ulong_t)&direct_ip_interface_blacklist
as an example for our 0x68AB.
Do we have a lsusb from the 0x68AA to get an idea what's different
between the existing 0x68AA and the new 0x68AB? I.e. why 0x68AA needs
the blacklisting, but 0x68AB not?
Blacklisting the direct-ip interfaces means blacklisting the following
interfaces
static const u8 direct_ip_non_serial_ifaces[] = { 7, 8, 9, 10, 11, 19,
20 };
as they are supposed to be handled by a network driver instead.
As these are not present in your device, it's not a direct-ip device and
does not need blacklisting.
But note that if the device can be reconfigured for direct ip, without
changing product id, then the blacklisting may be needed, but I'd like
to hear what the modem people thinks about that. AFAIK, switching to
direct ip mode has switched product id in the past.
I'm sure you can find lsusb output for 0x68aa in the archives or through
a search engine.
Johan
The interface layout of 1199:68ab does not look like 1199:68aa does, it
has instead more in common with MC879x modules 1199:683c/683d which also
are composite devices with 7 interfaces (0..6) and also MDM62xx based as
the AR8550.
The major difference is from what I can see only the interface
attributes 02/02/01 on interfaces 3 and 4 on the AR8550, they are
vendor specific ff/ff/ff on MC879x modules.
So the Sierra driver seems to be the right driver for the AR8550, I only
find it strange that Sierra hasn't added it.
Somehow I would also expect them to tell their customers (which I assume
Dirkäs employer is) which driver to use..
/Lars
--
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 v2] USB: sierra: add 1199:68AB device ID
On 2015-07-24 16:30, Johan Hovold wrote:
On Fri, Jul 24, 2015 at 10:59:48AM +0200, Dirk Behme wrote:
On 24.07.2015 10:45, Johan Hovold wrote:
On Fri, Jul 24, 2015 at 10:17:24AM +0200, Dirk Behme wrote:
On 23.07.2015 19:26, Johan Hovold wrote:
This is probably the right driver, but I'm confused over the DirectIP
bits. Those interfaces are lacking AFAICT and still your comment claims
it's a DirectIP modem (that would also require it to be added not the
network driver).
As we just use this device as ttyUSBx, it seems the "Direct IP modems"
comment might be wrong and a copy & paste error. I'll remove it in v3.
Ok?
Sounds good. And drop the blacklisting too.
Just for better understanding:
We took the existing 0x68AA device
{ USB_DEVICE_AND_INTERFACE_INFO(0x1199, 0x68AA, 0xFF, 0xFF, 0xFF),
.driver_info = (kernel_ulong_t)&direct_ip_interface_blacklist
as an example for our 0x68AB.
Do we have a lsusb from the 0x68AA to get an idea what's different
between the existing 0x68AA and the new 0x68AB? I.e. why 0x68AA needs
the blacklisting, but 0x68AB not?
Blacklisting the direct-ip interfaces means blacklisting the following
interfaces
static const u8 direct_ip_non_serial_ifaces[] = { 7, 8, 9, 10, 11, 19,
20 };
as they are supposed to be handled by a network driver instead.
As these are not present in your device, it's not a direct-ip device and
does not need blacklisting.
But note that if the device can be reconfigured for direct ip, without
changing product id, then the blacklisting may be needed, but I'd like
to hear what the modem people thinks about that. AFAIK, switching to
direct ip mode has switched product id in the past.
I'm sure you can find lsusb output for 0x68aa in the archives or through
a search engine.
Johan
The interface layout of 1199:68ab does not look like 1199:68aa does, it
has instead more in common with MC879x modules 1199:683c/683d which also
are composite devices with 7 interfaces (0..6) and also MDM62xx based as
the AR8550.
The major difference is from what I can see only the interface
attributes 02/02/01 on interfaces 3 and 4 on the AR8550, they are
vendor specific ff/ff/ff on MC879x modules.
So the Sierra driver seems to be the right driver for the AR8550, I only
find it strange that Sierra hasn't added it.
Somehow I would also expect them to tell their customers (which I assume
Dirkäs employer is) which driver to use..
/Lars
--
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: xhci_hcd 0000:00:14.0: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 2 comp_code 1
On 22.07.2015 17:12, Arkadiusz Miskiewicz wrote: > > [sorry, resend from different email - vger postmaster team has stupid filters > in place] > > On Tuesday 21 of July 2015, Mathias Nyman wrote: >> On 20.07.2015 23:13, Arkadiusz Miskiewicz wrote: >>> On Saturday 18 of July 2015, Arkadiusz Miskiewicz wrote: Hi. I'm on 4.2.0-rc2-00077-gf760b87 kernel and while trying to copy some file from usb storage (sata disk behind sata-usb bridge or pendrive; hapens in >>> both cases) copying process hangs just early after start with: >>> Looks like suspend & resume is enough. Reloading bluetooth firmware done >>> by kernel triggers problem: >>> >>> [ 106.302783] rtc_cmos 00:02: System wakeup disabled by ACPI >>> [ 106.313280] PM: resume of devices complete after 3003.032 msecs >>> [ 106.314079] Restarting tasks ... done. >>> [ 106.326434] Bluetooth: hci0: read Intel version: 370710018002030d00 >>> [ 106.330422] Bluetooth: hci0: Intel Bluetooth firmware file: >>> intel/ibt-hw-37.7.10-fw-1.80.2.3.d.bseq [ 106.398223] xhci_hcd >>> :00:14.0: ERROR Transfer event TRB DMA ptr not part of current TD >>> ep_index 0 comp_code 1 >> >> Looks like we get an event for a really old transfer for some reason, it >> should probably be handled already. >> >> I got a patch that adds more paranoid checks for TRB cancel, which has been >> one major reasons for the "Transfer event TRB DMA ptr not part of current >> TD" Errors. It also adds some logging to show what's went wrong. (patch >> attached, applies on 4.2-rc3) Can you see if it helps? > > It doesn't unfortunately. 4.2.0-rc3-00017-gd725e66 + that patch > -> dmesg http://sprunge.us/PDIE > > around 91s - after resume from ram bluetooth driver reloads > > around 754s - tried to copy data from external usb disk > > >> If it doesn't, then adding xhci debugging could give us some clue: >> echo -n 'module xhci_hcd =p' > /sys/kernel/debug/dynamic_debug/control > > Ok, http://sprunge.us/GiHX > Thanks for the logs, They show that the error is related to transfer descriptors that wrap around on the endpoint ring buffer by exactly one transfer block. I don't know yet why this happens, and I might need some help running additional debug patches to solve this. I'll take a more in depth look at the code one more time first. A short explanation of the error, mostly for myself: To transfer data we have a ring buffer that holds transfer requests blocks (TRBs). The ring buffer is made up of two segments, the last TRB in each segment is a special link TRB that points to the next segment. Segment A: 0x3000 - 0x3ff0 (where link TRB at 0x3ff0 points to Segment B, 0x4000) Segment B: 0x4000 - 0x4ff0 (where link TRB at 0x4ff0 points back at Segment A, 0x3000) A tranfer descriptor (TD) can consist of many TRBs, in this case three TRBs. When a TD is completed we will get an event telling where the last transferred TRB of the TD was. So in this case the error: xhci_hcd :00:14.0: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 0 comp_code 1 xhci_hcd :00:14.0: Looking for event-dma fffd3000 trb-start fffd4fd0 trb-end fffd5000 seg-start fffd4000 seg-end fffd4ff0 tells us the first TRB of the TD (trb-start) is at 4fd0, The second TRB is at 4fe0, Then we have the special link TRB be at 4ff0, pointing us back to the first segment. The third and final TRB should be back at the first segment at 3000. We get an event for the last TRB at 3000 and all should be fine, but driver claims the TDs TRBs start at 4fd0, and the last TRB is at 5000, missing the link TRB wrapping us back to the first segment. -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: GPIO support for Silicon Labs cp210x USB serial
On Fri, Jul 24, 2015 at 01:08:57PM +0200, Petr Tesařík wrote: > On Thu, 23 Jul 2015 14:59:32 -0700 > "John D. Blair" > wrote: > > > On Thu, 2015-07-23 at 18:00 +0200, Johan Hovold wrote: > > > The short answer is that we do not want custom driver ioctls. Any gpio > > > implementation needs to based on gpiolib, which provides a > > > standardised > > > interface. > > > > > I understand. > > > > We will evaluate and decide if the work is worth it for us to do. > > FWIW I'm planning to do this work. I've just started cleaning up the > cp210x sources, but my goal is to get gpio support (and not using > IOCTLs). That sounds great. > One of the challenges is that the CP210x device is in fact two logical > devices (the serial port and the GPIO), so theoretically, there should > be two drivers and a common module to handle resource conflicts, but I > believe this would be overkill. Indeed, you should just hang the gpio device directly off the usb-serial device (the interface, not the port). You can keep the implementation in its own file if it makes sense (e.g. to avoid ifdefs), but no need to try to force it to live under drivers/gpio. > I'll see how it goes with the upstream maintainers. ;-) We've pretty much agreed on the above by now. 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
Re: GPIO support for Silicon Labs cp210x USB serial
On Thu, 23 Jul 2015 14:59:32 -0700 "John D. Blair" wrote: > On Thu, 2015-07-23 at 18:00 +0200, Johan Hovold wrote: > > The short answer is that we do not want custom driver ioctls. Any gpio > > implementation needs to based on gpiolib, which provides a > > standardised > > interface. > > > I understand. > > We will evaluate and decide if the work is worth it for us to do. FWIW I'm planning to do this work. I've just started cleaning up the cp210x sources, but my goal is to get gpio support (and not using IOCTLs). One of the challenges is that the CP210x device is in fact two logical devices (the serial port and the GPIO), so theoretically, there should be two drivers and a common module to handle resource conflicts, but I believe this would be overkill. I'll see how it goes with the upstream maintainers. ;-) Petr T -- 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 v2] USB: sierra: add 1199:68AB device ID
On Fri, Jul 24, 2015 at 10:59:48AM +0200, Dirk Behme wrote:
> On 24.07.2015 10:45, Johan Hovold wrote:
> > On Fri, Jul 24, 2015 at 10:17:24AM +0200, Dirk Behme wrote:
> >> On 23.07.2015 19:26, Johan Hovold wrote:
> >
> >>> This is probably the right driver, but I'm confused over the DirectIP
> >>> bits. Those interfaces are lacking AFAICT and still your comment claims
> >>> it's a DirectIP modem (that would also require it to be added not the
> >>> network driver).
> >>
> >>
> >> As we just use this device as ttyUSBx, it seems the "Direct IP modems"
> >> comment might be wrong and a copy & paste error. I'll remove it in v3.
> >>
> >> Ok?
> >
> > Sounds good. And drop the blacklisting too.
>
>
> Just for better understanding:
>
> We took the existing 0x68AA device
>
> { USB_DEVICE_AND_INTERFACE_INFO(0x1199, 0x68AA, 0xFF, 0xFF, 0xFF),
>.driver_info = (kernel_ulong_t)&direct_ip_interface_blacklist
>
> as an example for our 0x68AB.
>
> Do we have a lsusb from the 0x68AA to get an idea what's different
> between the existing 0x68AA and the new 0x68AB? I.e. why 0x68AA needs
> the blacklisting, but 0x68AB not?
Blacklisting the direct-ip interfaces means blacklisting the following
interfaces
static const u8 direct_ip_non_serial_ifaces[] = { 7, 8, 9, 10, 11, 19,
20 };
as they are supposed to be handled by a network driver instead.
As these are not present in your device, it's not a direct-ip device and
does not need blacklisting.
But note that if the device can be reconfigured for direct ip, without
changing product id, then the blacklisting may be needed, but I'd like
to hear what the modem people thinks about that. AFAIK, switching to
direct ip mode has switched product id in the past.
I'm sure you can find lsusb output for 0x68aa in the archives or through
a search engine.
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] udc:Remove no longer required varaiable in the function udc_set_halt
ACK and thanks! for your patches. Am 6/27/2015 8:45 PM, schrieb Nicholas Krause: This removes the no longer required variable retval in the function udc_set_halt now due to this variable never being set to a return value after calling another function inside the function udc_set_halt and only being used unchanged at the end of this function's body. Due to this remove the no longer required variable retval and return the value zero directly if we arrive at the end of this particular function's body without any internal errors occurring. Signed-off-by: Nicholas Krause --- drivers/usb/gadget/udc/amd5536udc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/usb/gadget/udc/amd5536udc.c b/drivers/usb/gadget/udc/amd5536udc.c index de7e5e2..5b99e53 100644 --- a/drivers/usb/gadget/udc/amd5536udc.c +++ b/drivers/usb/gadget/udc/amd5536udc.c @@ -1307,7 +1307,6 @@ udc_set_halt(struct usb_ep *usbep, int halt) struct udc_ep *ep; u32 tmp; unsigned long iflags; - int retval = 0; if (!usbep) return -EINVAL; @@ -1360,7 +1359,7 @@ udc_set_halt(struct usb_ep *usbep, int halt) } } spin_unlock_irqrestore(&udc_stall_spinlock, iflags); - return retval; + return 0; } /* gadget interface */ -- 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 v2] USB: sierra: add 1199:68AB device ID
On 24.07.2015 10:45, Johan Hovold wrote:
On Fri, Jul 24, 2015 at 10:17:24AM +0200, Dirk Behme wrote:
On 23.07.2015 19:26, Johan Hovold wrote:
This is probably the right driver, but I'm confused over the DirectIP
bits. Those interfaces are lacking AFAICT and still your comment claims
it's a DirectIP modem (that would also require it to be added not the
network driver).
As we just use this device as ttyUSBx, it seems the "Direct IP modems"
comment might be wrong and a copy & paste error. I'll remove it in v3.
Ok?
Sounds good. And drop the blacklisting too.
Just for better understanding:
We took the existing 0x68AA device
{ USB_DEVICE_AND_INTERFACE_INFO(0x1199, 0x68AA, 0xFF, 0xFF, 0xFF),
.driver_info = (kernel_ulong_t)&direct_ip_interface_blacklist
as an example for our 0x68AB.
Do we have a lsusb from the 0x68AA to get an idea what's different
between the existing 0x68AA and the new 0x68AB? I.e. why 0x68AA needs
the blacklisting, but 0x68AB not?
Best regards
Dirk
--
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 v2] USB: sierra: add 1199:68AB device ID
On Fri, Jul 24, 2015 at 10:17:24AM +0200, Dirk Behme wrote: > On 23.07.2015 19:26, Johan Hovold wrote: > > This is probably the right driver, but I'm confused over the DirectIP > > bits. Those interfaces are lacking AFAICT and still your comment claims > > it's a DirectIP modem (that would also require it to be added not the > > network driver). > > > As we just use this device as ttyUSBx, it seems the "Direct IP modems" > comment might be wrong and a copy & paste error. I'll remove it in v3. > > Ok? Sounds good. And drop the blacklisting too. 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
RE: [PATCH] usb/gadget: make composite gadget meet usb compliance for vbus draw
Thanks for explanation. I am agree with you that separate changes into two
patches.
Will send out new patch soon.
Regards
Du, Changbin
> From: Andrzej Pietrasiewicz [mailto:andrze...@samsung.com]
> Hi,
>
> W dniu 24.07.2015 o 06:11, Du, Changbin pisze:
> > Thanks, Pietrasiewicz.
> >> From: Andrzej Pietrasiewicz [mailto:andrze...@samsung.com]
> >> W dniu 23.07.2015 o 14:34, Du, Changbin pisze:
> >>> >From 0a8e0d63a9887735c6782d7b0c15c2c1fdf1952a Mon Sep 17
> 00:00:00
> >>>void composite_disconnect(struct usb_gadget *gadget)
> >>>{
> >>> struct usb_composite_dev*cdev = get_gadget_data(gadget);
> >>> @@ -2095,7 +2119,7 @@ void composite_suspend(struct usb_gadget
> >> *gadget)
> >>>
> >>> cdev->suspended = 1;
> >>>
> >>> - usb_gadget_vbus_draw(gadget, 2);
> >>> + usb_gadget_vbus_draw(gadget, USB_VBUS_DRAW_SUSPEND);
> >>>}
> >>
> >> This looks like an unrelated change. I think it should go first
> >> in a separate patch which eliminates usage of "magic" numbers.
> >>
> > This change does make sense. As you know, when device is reset, it is in a
> 'unconfigured'
> > state. Compliance Test equipment may also measure vbus current at this
> moment.
>
> I am not questioning the change itself.
>
> What I mean is that in my opinion it should be done in a separate patch,
> because the newly introduced USB_VBUS_DRAW_SUSPEND is not used
> anywhere else in your patch. The meaning of this change is "use a symbolic
> name rather than an explicit number" and it is unrelated to
> making composite gadget meet usb compliance for vbus draw. In other
> words,
> if you don't do this change at all the compliance is still maintained,
> because the value of USB_VBUS_DRAW_SUSPEND is 2 anyway, so what the
> compiler eventually sees is the same whether the change is made or not.
>
> My idea:
>
> [PATCHv2 0/2] usb gadget vbus draw compilance
>[PATCHv2 1/2] usb: gadget: composite: avoid using a magic number
>>> substituting an explicit "2" with USB_VBUS_DRAW_SUSPEND goes
> here <<
>[PATCHv2 2/2] usb: gadget: composite: meet usb compliance for vbus draw
>>> the rest of your changes go here <<
>
> >
> >>>}
> >>> @@ -2132,7 +2157,7 @@ static const struct usb_gadget_driver
> >> composite_driver_template = {
> >>> .unbind = composite_unbind,
> >>>
> >>> .setup = composite_setup,
> >>> - .reset = composite_disconnect,
> >>> + .reset = composite_reset,
> >>> .disconnect = composite_disconnect,
> >>>
> >>
> >> A similar "template" is in drivers/usb/gadget/configfs.c. Shouldn't the
> "reset"
> >> method be changed there as well?
> >>
> > Yes, it also need to change. I will change it as well.
> >
> >>
> >>>
> >>> +/* USB2 compliance requires that un-configured current draw <=
> 100mA,
> >>> + * USB3 requires it <= 150mA, OTG requires it <= 2.5mA.
> >>> + */
> >>> +#define USB2_VBUS_DRAW_UNCONF100
> >>> +#define USB3_VBUS_DRAW_UNCONF150
> >>> +#define USB_OTG_VBUS_DRAW_UNCONF 2
> >>
> >>
> >>> +#define USB_VBUS_DRAW_SUSPEND2
> >>
> >> separate patch
> >>
> > Sorry, I didn't get your idea. Why separate these macros definition?
>
> Please see above.
>
> Andrzej
--
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 v2] USB: sierra: add 1199:68AB device ID
On 23.07.2015 19:26, Johan Hovold wrote:
On Thu, Jul 23, 2015 at 02:58:03PM +0200, Dirk Behme wrote:
On 23.07.2015 12:36, Johan Hovold wrote:
On Thu, Jul 23, 2015 at 12:31:55PM +0200, Dirk Behme wrote:
Hi Johan,
On 20.07.2015 11:24, Johan Hovold wrote:
[ +CC: Bjørn and Dan ]
On Mon, Jul 20, 2015 at 08:14:19AM +0200, Dirk Behme wrote:
Add support for the Sierra Wireless AR8550 device with
USB descriptor 0x1199, 0x68AB. For this, lsusb reports:
Thanks for the patch. This modem business is a bit of a mess and it's
not always apparent what driver a device id belongs to.
Bus 001 Device 004: ID 1199:68ab Sierra Wireless, Inc.
Device Descriptor:
bLength18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass0 (Defined at Interface level)
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize064
idVendor 0x1199 Sierra Wireless, Inc.
idProduct 0x68ab
bcdDevice0.06
iManufacturer 3 Sierra Wireless, Incorporated
iProduct2 AR8550
iSerial 0
bNumConfigurations 1
Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 198
bNumInterfaces 7
[...]
Signed-off-by: Dirk Behme
---
Changes in v2: Improve the commit message.
drivers/usb/serial/sierra.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c
index 46179a0..4122e4f 100644
--- a/drivers/usb/serial/sierra.c
+++ b/drivers/usb/serial/sierra.c
@@ -289,6 +289,9 @@ static const struct usb_device_id id_table[] = {
{ USB_DEVICE_AND_INTERFACE_INFO(0x1199, 0x68AA, 0xFF, 0xFF, 0xFF),
.driver_info = (kernel_ulong_t)&direct_ip_interface_blacklist
},
+ { USB_DEVICE(0x1199, 0x68AB), /* Sierra Wireless Direct IP modems */
+ .driver_info = (kernel_ulong_t)&direct_ip_interface_blacklist
+ },
This device has seven interfaces (0..6) so why do you use the direct-ip
interface blacklisting, which only blacklists interfaces >= 7?
Using it the same way like the quite similar ID 0x1199, 0x68AA above
'just works for us'. Not being the experts on this, do you like to
propose anything else you like us to try?
Also do you notice any delays when connecting the device (which could
indicate that sierra is not the right driver)?
As above, do you like us to try anything else?
Could you post a log from when connecting the device with you patch
applied?
Are you looking for something like this?
[ 102.948830] usb 1-1: new high-speed USB device number 4 using ci_hdrc
[ 103.101172] usb 1-1: New USB device found, idVendor=1199, idProduct=68ab
[ 103.101239] usb 1-1: New USB device strings: Mfr=3, Product=2,
SerialNumber=0
[ 103.101270] usb 1-1: Product: AR8550
[ 103.101297] usb 1-1: Manufacturer: Sierra Wireless, Incorporated
[ 103.195766] usbcore: registered new interface driver usbserial
[ 103.204471] usbcore: registered new interface driver sierra
[ 103.204692] usbserial: USB Serial support registered for Sierra USB modem
[ 103.204861] sierra 1-1:1.0: Sierra USB modem converter detected
[ 103.218385] usb 1-1: Sierra USB modem converter now attached to ttyUSB0
[ 103.220304] sierra 1-1:1.1: Sierra USB modem converter detected
[ 103.227186] usb 1-1: Sierra USB modem converter now attached to ttyUSB1
[ 103.227721] sierra 1-1:1.2: Sierra USB modem converter detected
[ 103.234287] usb 1-1: Sierra USB modem converter now attached to ttyUSB2
[ 103.234533] sierra 1-1:1.3: Sierra USB modem converter detected
[ 103.236469] usb 1-1: Sierra USB modem converter now attached to ttyUSB3
[ 103.236663] sierra 1-1:1.4: Sierra USB modem converter detected
[ 103.237712] usb 1-1: Sierra USB modem converter now attached to ttyUSB4
[ 103.237873] sierra 1-1:1.5: Sierra USB modem converter detected
[ 103.247026] usb 1-1: Sierra USB modem converter now attached to ttyUSB5
[ 103.247327] sierra 1-1:1.6: Sierra USB modem converter detected
[ 103.251378] usb 1-1: Sierra USB modem converter now attached to ttyUSB6
Yes, we've seen lengthy timeouts during probe for some Sierra devices
when incorrectly driven by the sierra driver, but that's not the case
here.
This is probably the right driver, but I'm confused over the DirectIP
bits. Those interfaces are lacking AFAICT and still your comment claims
it's a DirectIP modem (that would also require it to be added not the
network driver).
As we just use this device as ttyUSBx, it seems the "Direct IP modems"
comment might be wrong and a copy & paste error. I'll remove it in v3.
Ok?
Best regards
Dirk
--
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: Multiple drives on JMS56x-based sata-usb docking station.
This is the trace I got after I used this option: options usb-storage quirks=152d:1567:u 8<--- 33c03f00 0.013617 C Ii:2:001:1 0:2048 2 = 0400 33c03f00 0.013643 S Ii:2:001:1 -:2048 4 < de84e900 0.013877 S Ci:2:001:0 s a3 00 0002 0004 4 < de84e900 0.013923 C Ci:2:001:0 0 4 = 01050100 de84e900 0.013934 S Co:2:001:0 s 23 01 0010 0002 0 de84e900 0.013945 C Co:2:001:0 0 0 de84e900 0.013954 S Ci:2:001:0 s a3 00 0002 0004 4 < de84e900 0.013963 C Ci:2:001:0 0 4 = 0105 de84e900 0.039324 S Ci:2:001:0 s a3 00 0002 0004 4 < de84e900 0.039344 C Ci:2:001:0 0 4 = 0105 de84e900 0.065360 S Ci:2:001:0 s a3 00 0002 0004 4 < de84e900 0.065387 C Ci:2:001:0 0 4 = 0105 de84e900 0.091205 S Ci:2:001:0 s a3 00 0002 0004 4 < de84e900 0.091226 C Ci:2:001:0 0 4 = 0105 de84e900 0.117226 S Ci:2:001:0 s a3 00 0002 0004 4 < de84e900 0.117254 C Ci:2:001:0 0 4 = 0105 de84e900 0.117285 S Co:2:001:0 s 23 03 0004 0002 0 de84e900 0.117303 C Co:2:001:0 0 0 de84e900 0.168564 S Ci:2:001:0 s a3 00 0002 0004 4 < de84e900 0.169004 C Ci:2:001:0 0 4 = 03051000 de84e900 0.219220 S Co:2:001:0 s 23 01 0014 0002 0 de84e900 0.219252 C Co:2:001:0 0 0 de84e900 0.219281 S Ci:2:000:0 s 80 06 0100 0040 64 < de84e900 0.258554 C Ci:2:000:0 0 18 = 12011002 0040 2d156715 01010102 0501 . . . . . . . @ - . g . . . . . . . de84e900 0.258654 S Co:2:001:0 s 23 03 0004 0002 0 de84e900 0.258670 C Co:2:001:0 0 0 de84e900 0.309328 S Ci:2:001:0 s a3 00 0002 0004 4 < de84e900 0.309658 C Ci:2:001:0 0 4 = 03051000 de84e900 0.360576 S Co:2:001:0 s 23 01 0014 0002 0 de84e900 0.360613 C Co:2:001:0 0 0 de84e900 0.360629 S Co:2:000:0 s 00 05 0005 0 de84e900 0.403062 C Co:2:000:0 0 0 de84e480 0.415186 S Ci:2:005:0 s 80 06 0100 0012 18 < de84e480 0.415406 C Ci:2:005:0 0 18 = 12011002 0040 2d156715 01010102 0501 . . . . . . . @ - . g . . . . . . . de84e480 0.415420 S Ci:2:005:0 s 80 06 0f00 0005 5 < de84e480 0.415662 C Ci:2:005:0 0 5 = 050f1600 02 de84e480 0.415676 S Ci:2:005:0 s 80 06 0f00 0016 22 < de84e480 0.415911 C Ci:2:005:0 0 22 = 050f1600 02071002 0e0f 0a100300 0e00010a 2000 . . . . . . . . . . . . . . . . . . . .. de84e480 0.415979 S Ci:2:005:0 s 80 06 0200 0009 9 < de84e480 0.416166 C Ci:2:005:0 0 9 = 09025500 010104c0 01 . . U . . . . . . de84e480 0.416179 S Ci:2:005:0 s 80 06 0200 0055 85 < de84e480 0.416524 C Ci:2:005:0 0 85 = 09025500 010104c0 01090400 00020806 50060705 81020002 00070502 02000200 . . U . . . . . . . . . . . . . P . . . . . . . . . . . . . . . de84e0c0 0.416543 S Ci:2:005:0 s 80 06 0300 00ff 255 < de84e0c0 0.416647 C Ci:2:005:0 0 4 = 04030904 de84e0c0 0.416660 S Ci:2:005:0 s 80 06 0302 0409 00ff 255 < de84e0c0 0.416897 C Ci:2:005:0 0 28 = 1c034a00 4d005300 35003600 78002000 53006500 72006900 65007300 . . J . M . S . 5 . 6 . x . . S . e . r . i . e . s . de84e0c0 0.416911 S Ci:2:005:0 s 80 06 0301 0409 00ff 255 < de84e0c0 0.417022 C Ci:2:005:0 0 16 = 10034a00 4d006900 63007200 6f006e00 . . J . M . i . c . r . o . n . de84e0c0 0.417035 S Ci:2:005:0 s 80 06 0305 0409 00ff 255 < de84e0c0 0.417313 C Ci:2:005:0 0 32 = 20034400 42003900 38003700 36003500 34003300 32003100 31003100 36003600 . D . B . 9 . 8 . 7 . 6 . 5 . 4 . 3 . 2 . 1 . 1 . 1 . 6 . 6 . de84e300 0.417601 S Co:2:005:0 s 00 09 0001 0 de84e300 0.417773 C Co:2:005:0 0 0 de84e300 0.417785 S Ci:2:005:0 s 80 06 0304 0409 00ff 255 < de84e300 0.418022 C Ci:2:005:0 0 34 = 22035500 53004200 20004d00 61007300 73002000 53007400 6f007200 61006700 " . U . S . B .. M . a . s . s . . S . t . o . r . a . g . de84ecc0 0.418071 S Ci:2:005:0 s 80 06 0306 0409 00ff 255 < de84ecc0 0.418273 C Ci:2:005:0 0 46 = 2e034d00 53004300 20004200 75006c00 6b002d00 4f006e00 6c007900 20005400 . . M . S . C .. B . u . l . k . - . O . n . l . y .. T . 33833e40 1.472370 S Ci:2:005:0 s a1 fe 0001 1 < 33833e40 1.472539 C Ci:2:005:0 0 1 = 01 33833e40 1.472826 S Bo:2:005:2 - 31 = 55534243 0100 2400 8612 0024 00 U S B C . . . . $ . . . . . . . . . . $ . . . . . . . . . . . 33833e40 1.472884 C Bo:2:005:2 0 31 > a5e3dcc0 1.473029 S Bi:2:005:1 - 36 < a5e3dcc0 1.473134 C Bi:2:005:1 0 36 = 0612 5b00 496e6174 65636b20 . . . . [ . . . I n a t e c k. . . . . . . . . . . . . . . . 33833e40 1.473161 S Bi:2:005:1 - 13 < 33833e40 1.473259 C Bi:2:005:1 0 13 = 55534253 0100 00 U S B S . . . . . . . . . 33833e40 1.473818 S Bo:2:005:2 - 31 = 55534243 0200 0600 00 U S B C . . . . . . . . . . . . . . . . . . . . . . . . . . . 33833e
[PATCHv2 2/2] usb: gadget: f_printer: actually limit the number of instances
There is a predefined maximum number of printer instances, currently 4.
A chrdev region is allocated accordingly, but with configfs the user
can create as many printer function directories as they like. To make the
number of printer instances consistent with the number of allocated
minors, the limit is enforced at directory creation time.
Signed-off-by: Andrzej Pietrasiewicz
---
drivers/usb/gadget/function/f_printer.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_printer.c
b/drivers/usb/gadget/function/f_printer.c
index 44173df..357f63f 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -1248,7 +1248,15 @@ static struct config_item_type printer_func_type = {
static inline int gprinter_get_minor(void)
{
- return ida_simple_get(&printer_ida, 0, 0, GFP_KERNEL);
+ int ret;
+
+ ret = ida_simple_get(&printer_ida, 0, 0, GFP_KERNEL);
+ if (ret >= PRINTER_MINORS) {
+ ida_simple_remove(&printer_ida, ret);
+ ret = -ENODEV;
+ }
+
+ return ret;
}
static inline void gprinter_put_minor(int minor)
--
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
[PATCHv2 1/2] usb: gadget: f_hid: actually limit the number of instances
There is a predefined maximum number of hid instances, currently 4.
A chrdev region is allocated accordingly, but with configfs the user
can create as many hid function directories as they like. To make
the number of hid instances consistent with the number of allocated minors,
the limit is enforced at directory creation time.
Signed-off-by: Andrzej Pietrasiewicz
---
drivers/usb/gadget/function/f_hid.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/gadget/function/f_hid.c
b/drivers/usb/gadget/function/f_hid.c
index f7f35a3..6df9715 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -699,6 +699,10 @@ static inline int hidg_get_minor(void)
int ret;
ret = ida_simple_get(&hidg_ida, 0, 0, GFP_KERNEL);
+ if (ret >= HIDG_MINORS) {
+ ida_simple_remove(&hidg_ida, ret);
+ ret = -ENODEV;
+ }
return ret;
}
--
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
[PATCHv2 0/2] hid & printer fixes
v1..v2: - addressed Sergei's comment Users should not be able to create more function instances than minor numbers allocated for the functions. The problem is in hid and printer functions. This short series fixes it by enforcing the maximum number of function directories created. Andrzej Pietrasiewicz (2): usb: gadget: f_hid: actually limit the number of instances usb: gadget: f_printer: actually limit the number of instances drivers/usb/gadget/function/f_hid.c | 4 drivers/usb/gadget/function/f_printer.c | 10 +- 2 files changed, 13 insertions(+), 1 deletion(-) -- 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
Re: [PATCH] usb/gadget: make composite gadget meet usb compliance for vbus draw
Hi,
W dniu 24.07.2015 o 06:11, Du, Changbin pisze:
Thanks, Pietrasiewicz.
From: Andrzej Pietrasiewicz [mailto:andrze...@samsung.com]
W dniu 23.07.2015 o 14:34, Du, Changbin pisze:
>From 0a8e0d63a9887735c6782d7b0c15c2c1fdf1952a Mon Sep 17 00:00:00
void composite_disconnect(struct usb_gadget *gadget)
{
struct usb_composite_dev*cdev = get_gadget_data(gadget);
@@ -2095,7 +2119,7 @@ void composite_suspend(struct usb_gadget
*gadget)
cdev->suspended = 1;
- usb_gadget_vbus_draw(gadget, 2);
+ usb_gadget_vbus_draw(gadget, USB_VBUS_DRAW_SUSPEND);
}
This looks like an unrelated change. I think it should go first
in a separate patch which eliminates usage of "magic" numbers.
This change does make sense. As you know, when device is reset, it is in a
'unconfigured'
state. Compliance Test equipment may also measure vbus current at this moment.
I am not questioning the change itself.
What I mean is that in my opinion it should be done in a separate patch,
because the newly introduced USB_VBUS_DRAW_SUSPEND is not used
anywhere else in your patch. The meaning of this change is "use a symbolic
name rather than an explicit number" and it is unrelated to
making composite gadget meet usb compliance for vbus draw. In other words,
if you don't do this change at all the compliance is still maintained,
because the value of USB_VBUS_DRAW_SUSPEND is 2 anyway, so what the
compiler eventually sees is the same whether the change is made or not.
My idea:
[PATCHv2 0/2] usb gadget vbus draw compilance
[PATCHv2 1/2] usb: gadget: composite: avoid using a magic number
>> substituting an explicit "2" with USB_VBUS_DRAW_SUSPEND goes here <<
[PATCHv2 2/2] usb: gadget: composite: meet usb compliance for vbus draw
>> the rest of your changes go here <<
}
@@ -2132,7 +2157,7 @@ static const struct usb_gadget_driver
composite_driver_template = {
.unbind = composite_unbind,
.setup = composite_setup,
- .reset = composite_disconnect,
+ .reset = composite_reset,
.disconnect = composite_disconnect,
A similar "template" is in drivers/usb/gadget/configfs.c. Shouldn't the "reset"
method be changed there as well?
Yes, it also need to change. I will change it as well.
+/* USB2 compliance requires that un-configured current draw <= 100mA,
+ * USB3 requires it <= 150mA, OTG requires it <= 2.5mA.
+ */
+#define USB2_VBUS_DRAW_UNCONF 100
+#define USB3_VBUS_DRAW_UNCONF 150
+#define USB_OTG_VBUS_DRAW_UNCONF 2
+#define USB_VBUS_DRAW_SUSPEND 2
separate patch
Sorry, I didn't get your idea. Why separate these macros definition?
Please see above.
Andrzej
--
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
