[PATCH 1/1] usb: storage : Remove c99 style commenting
This patch fixes checkpatch.pl warning c99 style commenting. Signed-off-by: Sunny Kumar sunny.kumar@gmail.com --- drivers/usb/storage/sddr55.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c index aacedef..54d0a59 100644 --- a/drivers/usb/storage/sddr55.c +++ b/drivers/usb/storage/sddr55.c @@ -209,10 +209,10 @@ static int sddr55_read_data(struct us_data *us, unsigned int len, offset; struct scatterlist *sg; - // Since we only read in one block at a time, we have to create - // a bounce buffer and move the data a piece at a time between the - // bounce buffer and the actual transfer buffer. - + /* Since we only read in one block at a time, we have to create +* a bounce buffer and move the data a piece at a time between the +* bounce buffer and the actual transfer buffer. +*/ len = min((unsigned int) sectors, (unsigned int) info-blocksize info-smallpageshift) * PAGESIZE; buffer = kmalloc(len, GFP_NOIO); @@ -336,10 +336,10 @@ static int sddr55_write_data(struct us_data *us, return USB_STOR_TRANSPORT_FAILED; } - // Since we only write one block at a time, we have to create - // a bounce buffer and move the data a piece at a time between the - // bounce buffer and the actual transfer buffer. - + /* Since we only write one block at a time, we have to create +* a bounce buffer and move the data a piece at a time between the +* bounce buffer and the actual transfer buffer. +*/ len = min((unsigned int) sectors, (unsigned int) info-blocksize info-smallpageshift) * PAGESIZE; buffer = kmalloc(len, GFP_NOIO); -- 2.1.4 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/3] USB: io_ti: Increase insufficient timeout for firmware downloads
On Wed, Jun 24, 2015 at 12:39:05AM -0500, Peter Berger wrote: On Mon, 2015-06-22 at 10:45 +0200, Johan Hovold wrote: On Thu, Jun 18, 2015 at 06:43:34AM -0500, Peter E. Berger wrote: From: Peter E. Berger pber...@brimson.com The io_ti driver fails to download firmware to Edgeport devices such as the EP/416. One of the problems is that the default 1 second timeout in ti_vsend_sync() is insufficient for download operations. This patch increases the download timeout to 10 seconds. Patch looks good now. What happens when download fails? Does the device still work (e.g. this is only needed to support newer on-disk firmware)? Perhaps you can mention that in the commit log as well. It looks like some Edgeports (models like the EP/416 with on-board E2PROM) may be able to function even if the on-disk firmware image is bad or missing, iff their local E2PROM versions are valid. But most Edgeport models (I've tried EP/1 and EP/4) do not appear to have this capability and they always rely on the on-disk firmware image. I'm testing an implementation that calls the new check_fw_sanity() function at the top of download_fw() and, rather than simply returning an error if the firmware image is bad or missing, it saves the result and defers the decision until later when it may find that it is running on a E2PROM-equipped device with a valid image. But I think this is messier than it is worth (adding still more messiness to the already very messy download_fw()) for such a marginal possible benefit. I'm leaning towards the simpler approach of returning an error whenever check_fw_sanity() indicates a bad on-disk firmware image. Do you agree? Yes, that sounds reasonable. Perhaps such a feature can be added later after a much-needed clean up of download_fw, if ever. Thanks, Johan -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/3] USB: io_ti: Move request_firmware() calls out of download_fw()
On Wed, Jun 24, 2015 at 12:18:27AM -0500, Peter Berger wrote:
On Mon, 2015-06-22 at 11:43 +0200, Johan Hovold wrote:
On Thu, Jun 18, 2015 at 06:43:35AM -0500, Peter E. Berger wrote:
From: Peter E. Berger pber...@brimson.com
The io_ti driver fails to download firmware to Edgeport
devices such as the EP/416, even when the on-disk firmware image
(/lib/firmware/edgeport/down3.bin) is more current than the version
on the EP/416. The current download code is broken in a few ways.
Notably it mis-uses global variables OperationalMajorVersion and
OperationalMinorVersion (reading their values before they've been
properly initialized and subsequently initializing them multiple times
without synchronization). This patch drops the global variables and
replaces the redundant calls to request_firmware()/release_firmware()
in download_fw() with a single call pair in edge_startup(); the firmware
image pointer is then passed to download_fw() and build_i2c_fw_hdr().
Also, the firmware has a build_number field, though it apparently is
unused (according to observations of the three firmware images I have
seen and confirmed by Digi Tech Support). This comment describes its
structure, in case it is populated in a future release.
Signed-off-by: Peter E. Berger pber...@brimson.com
---
drivers/usb/serial/io_ti.c | 100
+++--
1 file changed, 51 insertions(+), 49 deletions(-)
diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index 69378a7..c76820b 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -101,6 +101,7 @@ struct edgeport_serial {
struct mutex es_lock;
int num_ports_open;
struct usb_serial *serial;
+ int fw_version;
};
@@ -187,10 +188,6 @@ static const struct usb_device_id
id_table_combined[] = {
MODULE_DEVICE_TABLE(usb, id_table_combined);
-static unsigned char OperationalMajorVersion;
-static unsigned char OperationalMinorVersion;
-static unsigned short OperationalBuildNumber;
-
static int closing_wait = EDGE_CLOSING_WAIT;
static bool ignore_cpu_rev;
static int default_uart_mode;/* RS232 */
@@ -751,18 +748,18 @@ exit:
}
/* Build firmware header used for firmware update */
-static int build_i2c_fw_hdr(__u8 *header, struct device *dev)
+static int build_i2c_fw_hdr(u8 *header, struct device *dev,
+ const struct firmware *fw)
{
__u8 *buffer;
int buffer_size;
int i;
- int err;
__u8 cs = 0;
struct ti_i2c_desc *i2c_header;
struct ti_i2c_image_header *img_header;
struct ti_i2c_firmware_rec *firmware_rec;
- const struct firmware *fw;
- const char *fw_name = edgeport/down3.bin;
+ u8 fw_major_version = fw-data[0];
+ u8 fw_minor_version = fw-data[1];
/* In order to update the I2C firmware we must change the type 2 record
* to type 0xF2. This will force the UMP to come up in Boot Mode.
@@ -785,24 +782,11 @@ static int build_i2c_fw_hdr(__u8 *header, struct
device *dev)
// Set entire image of 0xffs
memset(buffer, 0xff, buffer_size);
- err = request_firmware(fw, fw_name, dev);
- if (err) {
- dev_err(dev, Failed to load image \%s\ err %d\n,
- fw_name, err);
- kfree(buffer);
- return err;
- }
-
- /* Save Download Version Number */
- OperationalMajorVersion = fw-data[0];
- OperationalMinorVersion = fw-data[1];
- OperationalBuildNumber = fw-data[2] | (fw-data[3] 8);
-
/* Copy version number into firmware record */
firmware_rec = (struct ti_i2c_firmware_rec *)buffer;
- firmware_rec-Ver_Major = OperationalMajorVersion;
- firmware_rec-Ver_Minor = OperationalMinorVersion;
+ firmware_rec-Ver_Major = fw_major_version;
+ firmware_rec-Ver_Minor = fw_minor_version;
/* Pointer to fw_down memory image */
img_header = (struct ti_i2c_image_header *)fw-data[4];
Note that sanity checks on the firmware size are missing here; you could
fix that as a follow up.
I did some digging and it looks like we can actually do several sanity
checks on the firmware image. It seems that the Edgeport firmware
images have a 7-byte header:
u8 major_version;
u8 minor_version;
le16 build_number;
le16 length;
u8 checksum;
length appears to be the number of bytes of actual data following the
header.
checksum is apparently (from the images I have seen) the low order
byte resulting from adding the values of all the data bytes.
So, I'm testing a new ck_fw_sanity() function that checks for these
error conditions:
1. NULL (missing) firmware image
2. Incomplete firmware header:
#define FW_HEADER_SIZE 7
fw-size FW_HEADER_SIZE
3. Bad firmware size:
fw-size != FW_HEADER_SIZE +
Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Thu, 2015-06-25 at 17:19 +0200, Enrico Mioso wrote:
Hi Oliver! And thank you again.
I like / recommend the usage of open messaging standards: my preferred XMPP ID
(JID) is: mrk...@jit.si.
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Date: Thu, 25 Jun 2015 15:38:46
From: Oliver Neukum oneu...@suse.de
To: Enrico Mioso mrkiko...@gmail.com
Cc: linux-usb@vger.kernel.org, net...@vger.kernel.org
Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Thu, 2015-06-25 at 13:44 +0200, Enrico Mioso wrote:
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Is there any advantage in keeping this in a single function?
I did this choice in the light of the fact I think the tx_fixup function
will
become more complex than it is now, when aggregating frames.
Yes, but that is a reason to split the helpers up not the opposite.
Right - I understood only now your observation.
the only reason to write the manager that way was that it shouldn't become
very
complex - it should simply do things to frames, helping out in building valid
NCM packages.
I answer here your other message to make it more convenient to read: my
intention for the tx_fixup function would be to:
- aggregate frames
- send them out when:
- a timer expires
How would you do that in tx_fixup()? If a timer is required then you
need a separate function.
Sure. I meant: I will adapt it in case needed, and expectin the code to
become
a little bit more convoluted.
You cannot become any more convoluted even if you try.
OR
- we have enough data in the aggregate, and cannot add more.
Yes.
You need a third case:
- the interface is taken down.
But in general the logic for that is already there. So can you explain
what additional goals you have?
regarding the timer logic I saw it in cdc_ncm.c, but I should study it in
more detail to understand it and implement it here where needed in case.
It is involved. Basically a timer for transmission creates locking
issues. cdc-ncm uses an hrtimer which calls a bottom half which
calls xmit with a NULL skb.
/* if there is a remaining skb, it gets priority */
if (skb != NULL) {
swap(skb, ctx-tx_rem_skb);
swap(sign, ctx-tx_rem_sign);
} else {
ready2send = 1;
}
The else branch here is the timer triggering.
The actual transmission is done in usbnet if cdc_ncm_fill_tx_frame()
returns an skb.
I doubt you can can come up with anything more convoluted
but it simplifies locking.
Well, no, but it supposes a matched commit phase. Can you guarantee
that? I was under the oppression that in that phase you want to actually
give a frame over to the hardware.
No. When Italk about committing, I think about writing things to out skb.
another reason why i found confortable writing the code this way was to
maintain a kind of statefullness in a more clean way.
But I understand this is kind of agruable, and I can if needed break it up as
desired.
Regarding the commit phase - I am not sure I understand your comment, sorry
about that.
However, my intention would be to allow the caller to do calls in sequences
like:
- init frame
- ncm update
- ncm update
- ncm update
...
- ncm update
- ncm commit
(to work in huawei mode)
OR
- ncm init frame
- ncm commit
- ncm update
- ncm update
- ncm update
- ncm update
- finalize nth
(to work in cdc_ncm.c-mode)..
Sounds like a plan.
But to prevent usbnet from submittinx RX'd packets, I should be doing
something
nasty here. And unfortunately don't understand why.
// some devices want funky USB-level framing, for
// win32 driver (usually) and/or hardware quirks
if (info-tx_fixup) {
skb = info-tx_fixup (dev, skb, GFP_ATOMIC);
if (!skb) {
/* packet collected; minidriver waiting for more
*/
if (info-flags FLAG_MULTI_PACKET)
goto not_drop;
So you just return NULL if you are ready to queue more. But you
need the FLAG_MULTI_PACKET flag.
Regards
Oliver
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[EDT][PATCH] XHCI: Fix memory leak in error condition
EP-EC562D6B53594479BCA6FC73F17DEE54 In error condition, td buffer is not freed which can lead to memory leak. Signed-off-by: Vivek Kumar Bhagat vivek.bha...@samsung.com --- drivers/usb/host/xhci.c |1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 36bf089..dc02532 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -1438,6 +1438,7 @@ dying: ret = -ESHUTDOWN; free_priv: xhci_urb_free_priv(urb_priv); + kfree(buffer); urb-hcpriv = NULL; spin_unlock_irqrestore(xhci-lock, flags); return ret; -- 1.7.9.5
Re: [EDT][PATCH] XHCI: Fix memory leak in error condition
On Fri, Jun 26, 2015 at 07:46:00AM +, Vivek Kumar Bhagat wrote: EP-EC562D6B53594479BCA6FC73F17DEE54 In error condition, td buffer is not freed which can lead to memory leak. Signed-off-by: Vivek Kumar Bhagat vivek.bha...@samsung.com --- drivers/usb/host/xhci.c |1 + 1 file changed, 1 insertion(+) Also, please use scripts/get_maintainer.pl to determine who to send patches to. Hint, I'm not the one for this patch, I can't do anything with it... -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [EDT][PATCH] XHCI: Fix memory leak in error condition
On Fri, Jun 26, 2015 at 07:46:00AM +, Vivek Kumar Bhagat wrote: EP-EC562D6B53594479BCA6FC73F17DEE54 What is that crazy line for? It doesn't belong here. -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: common code for CDC drivers
On Fri, Jun 26, 2015 at 07:47:47PM +0300, Tal Shorer wrote: As someone who has cdc-acm enabled and usbnet disabled on my project, I'd rather you don't break my configuration : configurations will not break, they will be updated to pull in usbnet if needed. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: common code for CDC drivers
As someone who has cdc-acm enabled and usbnet disabled on my project, I'd rather you don't break my configuration : On Fri, Jun 26, 2015 at 4:40 PM, Oliver Neukum oneu...@suse.com wrote: Hi, I am looking at all the CDC drivers. They all have a parser for the extra CDC headers. If I unify this would it be acceptable for cdc-acm to depend on usbnet? Or should I put the code into a header? Regards Oliver -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: common code for CDC drivers
On June 26, 2015 3:40:18 PM CEST, Oliver Neukum oneu...@suse.com wrote: Hi, I am looking at all the CDC drivers. They all have a parser for the extra CDC headers. If I unify this would it be acceptable for cdc-acm to depend on usbnet? Or should I put the code into a header? I don't have any strong feelings. Either way is fine. And thanks for doing this. I've often thought about it, but always been too lazy to actually do anything about it. I believe cdc-wdm also has one of these parsers and should be unified. Bjørn -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] USB: mos7720: rename registers
On Fri, Jun 26, 2015 at 09:46:43AM +0200, Johan Hovold wrote: On Tue, Jun 23, 2015 at 06:59:40PM +0530, Sudip Mukherjee wrote: Some of the register names defined here are matching with registers defined in other places. Like DCR is defined here and DCR is also a register in mn10300 architecture. So when we are building this with mn10300, build fails. To avoid we rename all the registers. Signed-off-by: Sudip Mukherjee su...@vectorindia.org Looks good. I'll queue this up for 4.2. Thanks Johan. David, This series works on next-20150618. But next-20150625 again fails in allmodconfig of mn10300. I did a bisect and the first bad commit is: db8786a2829c (Merge remote-tracking branch 'dt-rh/for-next'). And reverting it solved the problem. I will find out what was the exact problem in that merge, but after I find out should i send a new series or another patch? regards sudip -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 64 byte EP0 OUT data transfer issue on Chipidea highspeed dual role controller
On Fri, Jun 26, 2015 at 5:01 AM, Jayan John jayanjoh...@gmail.com wrote: Thanks Alex. I appreciate you introducing me to Peter. Any help is appreciated. On the host (Wandboard iMX6q) the test app opens /dev/hidraw0 and write 64 bytes with report ID (1). The HID device has no Interrupt OUT ep, therefore uses control endpoint ep0 for the 64 bytes transfer to gadget (Wandboard iMX6q) using set_report. The setup phase is OK and the 64 bytes is written to gadget. However the chipidea interrupt (ci_irq()) and resulting udc interrupt (udc_irq()) is invoked. This indicates that the 64 bytes transaction is not completed over ep0 from host to gadget. **this issue is reproducible for all data transfers that aligns on 64 bytes** ~jayan Hi Jayan, This sounds like a Zero Length Transfer issue. This applies to any endpoint including EP0. A ZLT is needed to end any transfer IFF the length is not already known by the protocol. So if the hid URB requests say 1024 bytes and the gadget only has 64, it must first send the 64 byte maxpacket and then send a zero length packet. This way the host knows the transfer is complete. If there is no ZLT, the host hardware will keep requesting the rest of the message and the gadget which thinks it is done will keep NAKing Forever. If however the transfer is known to be exactly some multiple (1, 2 ...) of the maxpacketsize, no ZLT is needed since the host will ask for exactly that size. I believe for HID the packet size is determined by the report size in the HID descriptor. Good Luck, Steve -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: 64 byte EP0 OUT data transfer issue on Chipidea highspeed dual role controller
From: Steve Calfee Sent: 26 June 2015 15:59 On the host (Wandboard iMX6q) the test app opens /dev/hidraw0 and write 64 bytes with report ID (1). The HID device has no Interrupt OUT ep, therefore uses control endpoint ep0 for the 64 bytes transfer to gadget (Wandboard iMX6q) using set_report. The setup phase is OK and the 64 bytes is written to gadget. However the chipidea interrupt (ci_irq()) and resulting udc interrupt (udc_irq()) is invoked. This indicates that the 64 bytes transaction is not completed over ep0 from host to gadget. **this issue is reproducible for all data transfers that aligns on 64 bytes** ~jayan Hi Jayan, This sounds like a Zero Length Transfer issue. This applies to any endpoint including EP0. A ZLT is needed to end any transfer IFF the length is not already known by the protocol. So if the hid URB requests say 1024 bytes and the gadget only has 64, it must first send the 64 byte maxpacket and then send a zero length packet. This way the host knows the transfer is complete. Is this using the xhci driver? The ZLP sending code used to be broken. A couple of patches have been submitted but I don't remember one being applied. David
Re: [PATCH 1/1] usb: storage : Remove c99 style commenting
On Fri, 26 Jun 2015, Sunny Kumar wrote: This patch fixes checkpatch.pl warning c99 style commenting. Signed-off-by: Sunny Kumar sunny.kumar@gmail.com --- drivers/usb/storage/sddr55.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c index aacedef..54d0a59 100644 --- a/drivers/usb/storage/sddr55.c +++ b/drivers/usb/storage/sddr55.c @@ -209,10 +209,10 @@ static int sddr55_read_data(struct us_data *us, unsigned int len, offset; struct scatterlist *sg; - // Since we only read in one block at a time, we have to create - // a bounce buffer and move the data a piece at a time between the - // bounce buffer and the actual transfer buffer. - + /* Since we only read in one block at a time, we have to create + * a bounce buffer and move the data a piece at a time between the + * bounce buffer and the actual transfer buffer. + */ len = min((unsigned int) sectors, (unsigned int) info-blocksize info-smallpageshift) * PAGESIZE; buffer = kmalloc(len, GFP_NOIO); @@ -336,10 +336,10 @@ static int sddr55_write_data(struct us_data *us, return USB_STOR_TRANSPORT_FAILED; } - // Since we only write one block at a time, we have to create - // a bounce buffer and move the data a piece at a time between the - // bounce buffer and the actual transfer buffer. - + /* Since we only write one block at a time, we have to create + * a bounce buffer and move the data a piece at a time between the + * bounce buffer and the actual transfer buffer. + */ len = min((unsigned int) sectors, (unsigned int) info-blocksize info-smallpageshift) * PAGESIZE; buffer = kmalloc(len, GFP_NOIO); Why did you fix just these two sites? There are lots of other places in usb-storage that use C99-style comments: $ cd drivers/usb/storage $ egrep '[^:]//' *.[ch] | wc 17716359562 (The [^:] is to avoid matching things like http://;, and as a result this misses the four places where a // comment starts at the beginning of a line.) Why not fix all of them? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] usb: storage : Remove c99 style commenting
On Fri, Jun 26, 2015 at 10:08:42AM -0400, Alan Stern wrote: On Fri, 26 Jun 2015, Sunny Kumar wrote: This patch fixes checkpatch.pl warning c99 style commenting. Signed-off-by: Sunny Kumar sunny.kumar@gmail.com --- drivers/usb/storage/sddr55.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c index aacedef..54d0a59 100644 --- a/drivers/usb/storage/sddr55.c +++ b/drivers/usb/storage/sddr55.c @@ -209,10 +209,10 @@ static int sddr55_read_data(struct us_data *us, unsigned int len, offset; struct scatterlist *sg; - // Since we only read in one block at a time, we have to create - // a bounce buffer and move the data a piece at a time between the - // bounce buffer and the actual transfer buffer. - + /* Since we only read in one block at a time, we have to create +* a bounce buffer and move the data a piece at a time between the +* bounce buffer and the actual transfer buffer. +*/ len = min((unsigned int) sectors, (unsigned int) info-blocksize info-smallpageshift) * PAGESIZE; buffer = kmalloc(len, GFP_NOIO); @@ -336,10 +336,10 @@ static int sddr55_write_data(struct us_data *us, return USB_STOR_TRANSPORT_FAILED; } - // Since we only write one block at a time, we have to create - // a bounce buffer and move the data a piece at a time between the - // bounce buffer and the actual transfer buffer. - + /* Since we only write one block at a time, we have to create +* a bounce buffer and move the data a piece at a time between the +* bounce buffer and the actual transfer buffer. +*/ len = min((unsigned int) sectors, (unsigned int) info-blocksize info-smallpageshift) * PAGESIZE; buffer = kmalloc(len, GFP_NOIO); Why did you fix just these two sites? There are lots of other places in usb-storage that use C99-style comments: $ cd drivers/usb/storage $ egrep '[^:]//' *.[ch] | wc 17716359562 (The [^:] is to avoid matching things like http://;, and as a result this misses the four places where a // comment starts at the beginning of a line.) Why not fix all of them? These were C99 muliline comments so thought of fixing .. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/3][v2] drivers:usb:fsl: Replace macros with enumerated type
On Fri, 26 Jun 2015, Badola Nikhil wrote: Hi Greg/Alan, Could you please provide comments for this patch and the subsequent patches in this patch set, if any. You shouldn't ask Greg and me; you should ask the Freescale maintainer. He is the person who has to approve your patches. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/3][v2] drivers:usb:fsl: Replace macros with enumerated type
-Original Message-
From: Nikhil Badola [mailto:nikhil.bad...@freescale.com]
Sent: Monday, June 15, 2015 3:47 PM
To: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org
Cc: gre...@linuxfoundation.org; st...@rowland.harvard.edu; Badola Nikhil-
B46172
Subject: [PATCH 1/3][v2] drivers:usb:fsl: Replace macros with enumerated
type
Replace macros with enumerated type to represent usb ip controller version
Signed-off-by: Nikhil Badola nikhil.bad...@freescale.com
---
Changes for v2 :
- Assigned value to each enumerator
- Changed return type of function that returns
controller version
- Introduced FSL_USB_VER_NONE for invalid controller version
drivers/usb/host/fsl-mph-dr-of.c | 8
include/linux/fsl_devices.h | 16 ++--
2 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-
of.c
index 5e0d600..2195956 100644
--- a/drivers/usb/host/fsl-mph-dr-of.c
+++ b/drivers/usb/host/fsl-mph-dr-of.c
@@ -119,9 +119,9 @@ error:
static const struct of_device_id fsl_usb2_mph_dr_of_match[];
-static int usb_get_ver_info(struct device_node *np)
+static enum fsl_usb2_controller_ver usb_get_ver_info(struct device_node
+*np)
{
- int ver = -1;
+ enum fsl_usb2_controller_ver ver = FSL_USB_VER_NONE;
/*
* returns 1 for usb controller version 1.6 @@ -142,7 +142,7 @@ static
int usb_get_ver_info(struct device_node *np)
else /* for previous controller versions */
ver = FSL_USB_VER_OLD;
- if (ver -1)
+ if (ver FSL_USB_VER_NONE)
return ver;
}
@@ -215,7 +215,7 @@ static int fsl_usb2_mph_dr_of_probe(struct
platform_device *ofdev)
pdata-controller_ver = usb_get_ver_info(np);
if (pdata-have_sysif_regs) {
- if (pdata-controller_ver 0) {
+ if (pdata-controller_ver == FSL_USB_VER_NONE) {
dev_warn(ofdev-dev, Could not get controller
version\n);
return -ENODEV;
}
diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h index
2a2f56b..0d4855cd 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -20,11 +20,6 @@
#define FSL_UTMI_PHY_DLY 10 /*As per P1010RM, delay for UTMI
PHY CLK to become stable - 10ms*/
#define FSL_USB_PHY_CLK_TIMEOUT 1 /* uSec */
-#define FSL_USB_VER_OLD 0
-#define FSL_USB_VER_1_6 1
-#define FSL_USB_VER_2_2 2
-#define FSL_USB_VER_2_4 3
-#define FSL_USB_VER_2_5 4
#include linux/types.h
@@ -52,6 +47,15 @@
*
*/
+enum fsl_usb2_controller_ver {
+ FSL_USB_VER_NONE = -1,
+ FSL_USB_VER_OLD = 0,
+ FSL_USB_VER_1_6 = 1,
+ FSL_USB_VER_2_2 = 2,
+ FSL_USB_VER_2_4 = 3,
+ FSL_USB_VER_2_5 = 4,
+};
+
enum fsl_usb2_operating_modes {
FSL_USB2_MPH_HOST,
FSL_USB2_DR_HOST,
@@ -72,7 +76,7 @@ struct platform_device;
struct fsl_usb2_platform_data {
/* board specific information */
- int controller_ver;
+ enum fsl_usb2_controller_vercontroller_ver;
enum fsl_usb2_operating_modes operating_mode;
enum fsl_usb2_phy_modes phy_mode;
unsigned intport_enables;
--
Hi Greg/Alan,
Could you please provide comments for this patch and the subsequent
patches in this patch set, if any.
Thanks,
Nikhil
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 64 byte EP0 OUT data transfer issue on Chipidea highspeed dual role controller
On Fri, Jun 26, 2015 at 8:44 PM, David Laight david.lai...@aculab.com wrote: From: Steve Calfee Sent: 26 June 2015 15:59 On the host (Wandboard iMX6q) the test app opens /dev/hidraw0 and write 64 bytes with report ID (1). The HID device has no Interrupt OUT ep, therefore uses control endpoint ep0 for the 64 bytes transfer to gadget (Wandboard iMX6q) using set_report. The setup phase is OK and the 64 bytes is written to gadget. However the chipidea interrupt (ci_irq()) and resulting udc interrupt (udc_irq()) is invoked. This indicates that the 64 bytes transaction is not completed over ep0 from host to gadget. **this issue is reproducible for all data transfers that aligns on 64 bytes** ~jayan Hi Jayan, This sounds like a Zero Length Transfer issue. This applies to any endpoint including EP0. A ZLT is needed to end any transfer IFF the length is not already known by the protocol. So if the hid URB requests say 1024 bytes and the gadget only has 64, it must first send the 64 byte maxpacket and then send a zero length packet. This way the host knows the transfer is complete. Is this using the xhci driver? The ZLP sending code used to be broken. A couple of patches have been submitted but I don't remember one being applied. David Thanks for the pointers. This is not using the xhci driver. - This is a ep0 OUT transfer. Does the host need to add a ZLP for 64 byte aligned data transfer? - Is it not expected for the completion interrupt be raised on receiving the 64 bytes of data, irrespective of whether or not a ZLP has been sent. ~Jayan -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 64 byte EP0 OUT data transfer issue on Chipidea highspeed dual role controller
On Sat, 27 Jun 2015, Jayan John wrote: On Fri, Jun 26, 2015 at 8:44 PM, David Laight david.lai...@aculab.com wrote: From: Steve Calfee Sent: 26 June 2015 15:59 On the host (Wandboard iMX6q) the test app opens /dev/hidraw0 and write 64 bytes with report ID (1). The HID device has no Interrupt OUT ep, therefore uses control endpoint ep0 for the 64 bytes transfer to gadget (Wandboard iMX6q) using set_report. The setup phase is OK and the 64 bytes is written to gadget. However the chipidea interrupt (ci_irq()) and resulting udc interrupt (udc_irq()) is invoked. This indicates that the 64 bytes transaction is not completed over ep0 from host to gadget. **this issue is reproducible for all data transfers that aligns on 64 bytes** ~jayan Hi Jayan, This sounds like a Zero Length Transfer issue. This applies to any endpoint including EP0. A ZLT is needed to end any transfer IFF the length is not already known by the protocol. So if the hid URB requests say 1024 bytes and the gadget only has 64, it must first send the 64 byte maxpacket and then send a zero length packet. This way the host knows the transfer is complete. Is this using the xhci driver? The ZLP sending code used to be broken. A couple of patches have been submitted but I don't remember one being applied. David Thanks for the pointers. This is not using the xhci driver. - This is a ep0 OUT transfer. Does the host need to add a ZLP for 64 byte aligned data transfer? I'm not sure what you mean here by aligned, but in general the answer is No. - Is it not expected for the completion interrupt be raised on receiving the 64 bytes of data, irrespective of whether or not a ZLP has been sent. It is expected. Did you check the wLength value in the Setup packet? Is it equal to 64? Does the hid gadget driver then queue a request for a 64-byte OUT transfer on ep0? If it does then there's a bug in the UDC driver or hardware. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices
On Fri, Jun 26, 2015 at 09:24:07PM +0200, Tom Gundersen wrote: This policy used to be unconditionally applied by udev, but there is no reason to make userspace be involved in this and in the future udev will not be doing it by default. See: https://github.com/systemd/systemd/pull/353. Signed-off-by: Tom Gundersen t...@jklm.no Cc: Jiri Kosina jkos...@suse.cz Cc: Greg Kroah-Hartman gre...@linuxfoundation.org --- Hi, I don't have the right hardware for this, so it has only been compile-tested. I'm therefore sending it as an RFC only. Mainly I want to bring it to people's attention that it would be great to get this feature into the kernel as we want to drop it from udev. Cheers, Tom drivers/hid/usbhid/hid-core.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index bfbe1be..af80700 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -1358,6 +1358,9 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id * setup_timer(usbhid-io_retry, hid_retry_timeout, (unsigned long) hid); spin_lock_init(usbhid-lock); + if (dev-removable == USB_DEVICE_FIXED) + usb_enable_autosuspend(dev); + As this duplicates what udev has been doing for a while now, we should be safe here. thanks for the patch. Acked-by: Greg Kroah-Hartman gre...@linuxfoundation.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][RFC] usbhid: enable autosuspend for internal devices
On Fri, 26 Jun 2015, Tom Gundersen wrote: This policy used to be unconditionally applied by udev, but there is no reason to make userspace be involved in this and in the future udev will not be doing it by default. See: https://github.com/systemd/systemd/pull/353. Signed-off-by: Tom Gundersen t...@jklm.no Cc: Jiri Kosina jkos...@suse.cz Cc: Greg Kroah-Hartman gre...@linuxfoundation.org --- Hi, I don't have the right hardware for this, so it has only been compile-tested. I'm therefore sending it as an RFC only. Mainly I want to bring it to people's attention that it would be great to get this feature into the kernel as we want to drop it from udev. Cheers, Tom drivers/hid/usbhid/hid-core.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index bfbe1be..af80700 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -1358,6 +1358,9 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id * setup_timer(usbhid-io_retry, hid_retry_timeout, (unsigned long) hid); spin_lock_init(usbhid-lock); + if (dev-removable == USB_DEVICE_FIXED) + usb_enable_autosuspend(dev); This doesn't do what the patch title says. USB_DEVICE_FIXED means that the device can't be unplugged from its upstream port. It doesn't mean the device is internal to the computer. As an example, consider a composite Apple keyboard, which has an internal 3-port USB hub where two of the hub's ports are exposed on the edge of the keyboard case and the keyboard controller is permanently attached to the third hub port. Then the controller device would be marked USB_DEVICE_FIXED, even though the whole thing is external to the computer and can be unplugged. A reasonable compromise might be /* * Enable autosuspend for devices permanently attached * to the root hub. */ if (!dev-parent-parent dev-removable == USB_DEVICE_FIXED) usb_enable_autosuspend(dev); But this doesn't work if there's a permanently attached hub and a device permanently attached to that hub. To do this thoroughly, you have to iterate up the dev-parent chain, making sure at each step that the -removable value is USB_DEVICE_FIXED. Also, are you really certain this is safe? Aren't there a number of built-in keyboards that will work badly if you allow them to autosuspend? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] USB driver patches for 4.2-rc1
The following changes since commit d4a4f75cd8f29cd9464a5a32e9224a91571d6649: Linux 4.1-rc7 (2015-06-07 20:23:50 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/ tags/usb-4.2-rc1 for you to fetch changes up to 50641056d833813b71b0ad51823f7ded8dd62e7f: usb: dwc3: Use ASCII space in Kconfig (2015-06-12 17:42:31 -0700) USB patches for 4.2-rc1 Here's the big USB patchset for 4.2-rc1. As is normal these days, the majority of changes are in the gadget drivers, with a bunch of other small driver changes. All of these have been in linux-next with no reported issues. Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org Abhishek Bist (1): USB: hcd.h : Removed an unnecessary function prototype usb_find_interface_driver() Akinobu Mita (1): usb: storage: fix module reference for scsi host Alan Stern (1): USB: don't build PCI quirks if USB support isn't configured Alexey Sokolov (1): cdc-acm: Add support of ATOL FPrint fiscal printers Andrzej Pietrasiewicz (6): usb: gadget: rndis: use rndis_params instead of configNr usb: gadget: rndis: style correction usb: gadget: rndis: remove the limit of available rndis connections usb: gadget: rndis: change the value passed to rndis_signal_(dis)connect() usb: gadget: rndis: don't duplicate the i variable usb: gadget: rndis: use signed type for a signed value Arnd Bergmann (2): usb: renesas_usbhs: avoid uninitialized variable use usb: phy: add static inline wrapper for devm_usb_get_phy_by_node Arun Ramamurthy (3): phy: core: Add devm_of_phy_get_by_index to phy-core usb: ehci-platform: Use devm_of_phy_get_by_index usb: ohci-platform: Use devm_of_phy_get_by_index Axel Lin (1): phy: core: Check requested PHY status in _of_phy_get() Bin Liu (2): usb: musb: only set test mode once usb: musb: add softconnect for host mode Brian Norris (2): Documentation: devicetree: add Broadcom SATA PHY binding phy: add Broadcom SATA3 PHY driver for Broadcom STB SoCs Chris Bainbridge (1): usb: host: xhci: remove incorrect comment about mutex Colin Ian King (1): usb: isp1760: check for null return from kzalloc Dan Carpenter (1): USB: devio: fix a condition in async_completed() Dmitry Torokhov (1): phy: phy-core: allow specifying supply at port level Fabian Frederick (1): cdc-acm: use swap() in acm_probe() Fabio Estevam (1): usb: chipidea: usbmisc_imx: Remove unneeded semicolon Felipe Balbi (8): phy: miphy28lp: fix sparse warnings phy: miphy365x: fix sparse warnings usb: dwc2: hcd: fix build warning usb: gadget: atmel: fix build warning usb: musb: am35x: fix build warnings usb: musb: ux500: fix build warnings usb: gadget: atmel: fix build warnings usb: dwc3: gadget: don't clear EP_BUSY too early Geert Uytterhoeven (3): usb: phy: Remove the phy-rcar-gen2-usb driver usb: phy: Allow compile test of GPIO consumers if !GPIOLIB usb: phy: Remove the phy-rcar-gen2-usb driver Greg Kroah-Hartman (25): USB: ehci-dbg.c: move assignment out of if () block USB: fusbh200-hcd.c: move assignment out of if () block USB: hcd.c: move assignment out of if () block USB: hub.c: move assignment out of if () block USB: inode.c: move assignment out of if () block USB: isp116x-hcd.c: move assignment out of if () block USB: mon_bin.c: move assignment out of if () block USB: mon_main.c: move assignment out of if () block USB: mon_stat.c: move assignment out of if () block USB: ohci-dbg.c: move assignment out of if () block USB: ohci-hcd.c: move assignment out of if () block USB: ohci-q.c: move assignment out of if () block USB: sisusb.c: move assignment out of if () block USB: sisusb_con.c: move assignment out of if () block USB: speedtch.c: move assignment out of if () block USB: usbatm.c: move assignment out of if () block USB: usblp.c: move assignment out of if () block USB: uss720.c: move assignment out of if () block USB: xusbatm.c: move assignment out of if () block Merge 4.1-rc4 into usb-next Merge tag 'usb-for-v4.2' of git://git.kernel.org/.../balbi/usb into usb-next Merge tag 'phy-for-v4.2' of git://git.kernel.org/.../kishon/linux-phy into usb-next Merge tag 'usb-serial-4.2-rc1' of git://git.kernel.org/.../johan/usb-serial into usb-next Merge 4.1-rc7 into usb-next Merge tag 'usb-ci-v4.2-rc1' of git://git.kernel.org/.../peter.chen/usb into usb-work Gregory Herrero (15): usb: dwc2: add controller hibernation support usb: dwc2: implement hibernation during bus suspend/resume usb: dwc2: controller must update
Re: common code for CDC drivers
It won't break, but this will make me compile something I don't need and may not want. Wouldn't it make more sense to put this unified code in a new file (maybe drivers/usb/class/cdc.c) and have all related drivers depend on that instead of on usbnet? The less code that is compiled but never runs, the happier I am. On Fri, Jun 26, 2015 at 7:59 PM, Greg KH g...@kroah.com wrote: On Fri, Jun 26, 2015 at 07:47:47PM +0300, Tal Shorer wrote: As someone who has cdc-acm enabled and usbnet disabled on my project, I'd rather you don't break my configuration : configurations will not break, they will be updated to pull in usbnet if needed. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices
On Fri, 26 Jun 2015, Alan Stern wrote: On Fri, 26 Jun 2015, Tom Gundersen wrote: This policy used to be unconditionally applied by udev, but there is no reason to make userspace be involved in this and in the future udev will not be doing it by default. See: https://github.com/systemd/systemd/pull/353. Signed-off-by: Tom Gundersen t...@jklm.no Cc: Jiri Kosina jkos...@suse.cz Cc: Greg Kroah-Hartman gre...@linuxfoundation.org --- Hi, I don't have the right hardware for this, so it has only been compile-tested. I'm therefore sending it as an RFC only. Mainly I want to bring it to people's attention that it would be great to get this feature into the kernel as we want to drop it from udev. Cheers, Tom drivers/hid/usbhid/hid-core.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index bfbe1be..af80700 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -1358,6 +1358,9 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id * setup_timer(usbhid-io_retry, hid_retry_timeout, (unsigned long) hid); spin_lock_init(usbhid-lock); + if (dev-removable == USB_DEVICE_FIXED) + usb_enable_autosuspend(dev); This doesn't do what the patch title says. USB_DEVICE_FIXED means that the device can't be unplugged from its upstream port. It doesn't mean the device is internal to the computer. As an example, consider a composite Apple keyboard, which has an Oops -- I meant compound, not composite. A compound USB device actually has multiple devices (one of which is a hub) in a single package. internal 3-port USB hub where two of the hub's ports are exposed on the edge of the keyboard case and the keyboard controller is permanently attached to the third hub port. Then the controller device would be marked USB_DEVICE_FIXED, even though the whole thing is external to the computer and can be unplugged. Another problem is that the set_usb_port_removable() routine in drivers/usb/core/hub.c (the routine that sets dev-removable in the first place) contains this code: hub = usb_hub_to_struct_hub(udev-parent); wHubCharacteristics = le16_to_cpu(hub-descriptor-wHubCharacteristics); if (!(wHubCharacteristics HUB_CHAR_COMPOUND)) return; I guess the assumption was that all of a hub's ports would be unpluggable if the hub wasn't part of a compound device. But that's not correct for root hubs. This test should be deleted. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3][v2] drivers:usb:fsl: Replace macros with enumerated type
On Fri, Jun 26, 2015 at 05:12:04PM +, Badola Nikhil wrote: -Original Message- From: Nikhil Badola [mailto:nikhil.bad...@freescale.com] Sent: Monday, June 15, 2015 3:47 PM snip Could you please provide comments for this patch and the subsequent patches in this patch set, if any. It's the middle of the merge window right now, there's nothing I can do with this until after it is over. Please wait until then to expect a response, and even then, give me a week or so to dig out from under the pending-patch load. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xhci:Change function name from xhci_configure_endpoint_result to xhci_trace_endpoint_result
On Fri, Jun 26, 2015 at 02:25:48PM -0400, Nicholas Krause wrote:
This changes the name of the function xhci_configure_endpoint_result
to xhci_trace_endpoint_result to more clearly state the actual
intended use of this function which is tracing the passed command
status, on the passed usb/xhci structure pointers of udev and xhci
respectfully.
Signed-off-by: Nicholas Krause xerofo...@gmail.com
Dammit, my filters broke, I need to go fix them now...
---
drivers/usb/host/xhci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 36bf089..2e086ef 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1839,7 +1839,7 @@ static void xhci_zero_in_ctx(struct xhci_hcd *xhci,
struct xhci_virt_device *vir
}
}
-static int xhci_configure_endpoint_result(struct xhci_hcd *xhci,
+static int xhci_trace_endpoint_result(struct xhci_hcd *xhci,
struct usb_device *udev, u32 *cmd_status)
{
int ret;
@@ -2683,7 +2683,7 @@ static int xhci_configure_endpoint(struct xhci_hcd
*xhci,
wait_for_completion(command-completion);
if (!ctx_change)
- ret = xhci_configure_endpoint_result(xhci, udev,
+ ret = xhci_trace_endpoint_result(xhci, udev,
command-status);
No, please stop, this is doing nothing but unneeded code churn, and do
absolutely nothing.
Because you are banned from vger, these patches are not being sent to
the public, so I will not accept them either.
I will not be accepting any patches from you for at least a year, maybe
two, so don't waste your time. Please go work on some other open source
project, if you really want to contribute to the Linux ecosystem, as
long as it is not the kernel.
greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Xenta Mouse Wireless sometimes not working in Linux Kernel 4.0.5
On Fri, Jun 19, 2015 at 02:58:27PM -0500, José David León Rodríguez wrote: Sometimes generic mice usb wireless not work with usb_modeswitch enabled. dmesg | grep usb output when kernel not find device descriptors for mouse: [ 154.988937] usb 2-3: new full-speed USB device number 9 using xhci_hcd [ 160.159445] usb 2-3: unable to read config index 0 descriptor/start: -110 [ 160.159450] usb 2-3: can't read configurations, error -110 [ 160.319415] usb 2-3: new full-speed USB device number 10 using xhci_hcd [ 165.489805] usb 2-3: unable to read config index 0 descriptor/start: -110 [ 165.489815] usb 2-3: can't read configurations, error -110 after I disable usb_modeswitch or unplug and plug mouse several times... usb_modeswitch shouldn't care about this, the device isn't communicating well with the system. [ 165.649708] usb 2-3: new full-speed USB device number 11 using xhci_hcd [ 165.705290] usbcore: registered new interface driver usbhid [ 165.705293] usbhid: USB HID core driver [ 165.739693] input: HID Wireless Mouse HID Wireless Mouse as /devices/pci:00/:00:14.0/usb2/2-3/2-3:1.0/0003:1D57:0016.0001/input/input17 [ 165.739838] hid-generic 0003:1D57:0016.0001: input,hidraw0: USB HID v1.10 Mouse [HID Wireless Mouse HID Wireless Mouse] on usb-:00:14.0-3/input0 So it works properly now? thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH TRIVIAL] drivers/usb/gadget/composite.c: i18n is not an acronym
On Thu, 25 Jun 2015, Diego Viola wrote: Ping? Diego, you seem to be very anxious about your trivial patches. Please understand, that those of the patches which are not really super-important, might sit in the queue for long time, because they are preempted by more important stuff. Your patch hasn't been lost, it's still in my trivial-todo inbox, and will eventually be processed. Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: storage: add no SYNCHRONIZE CACHE quirk
On Jun 22 James Bottomley wrote: On Mon, 2015-06-22 at 13:30 -0400, Alan Stern wrote: On Mon, 22 Jun 2015, James Bottomley wrote: Obviously, for a disk with a writeback cache that can't do flush, that window is much wider and the real solution should be to try to switch the cache to write through. I agree. Doing the switch manually (by writing to the cache_type attribute file) works, but it's a nuisance to do this when you have a portable USB drive that gets moved among a bunch of machines. Perhaps it might be wise to do this to every USB device ... for external devices, the small performance gain doesn't really make up for the potential data loss. Just a small note on the assumption of externally (and in extension, temporarily) attached devices: Not all USB-attached devices are external, and not all external devices are used as removable devices. (For example, I am using 2 internal CD-ROMs via USB-2 + usb-storage and 2 internal HDDs via USB 3 + uas in a PC with too few SATA ports and no PCIe slot to spare for a controller add-on card, but plenty of USB headers available on the mainboard. Similarly, some NASes have their operating system located on a USB-attached device. Small offices use USB-attached disks for backup and won't detach such a disk until rotation for off-site deposit. Not to mention embedded computers with USB-attached but fixed disks.) -- Stefan Richter -=-= -==- ==-=- http://arcgraph.de/sr/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH TRIVIAL] drivers/usb/gadget/composite.c: i18n is not an acronym
On Fri, Jun 26, 2015 at 11:43:25AM +0200, Jiri Kosina wrote: On Thu, 25 Jun 2015, Diego Viola wrote: Ping? Diego, you seem to be very anxious about your trivial patches. Please understand, that those of the patches which are not really super-important, might sit in the queue for long time, because they are preempted by more important stuff. Your patch hasn't been lost, it's still in my trivial-todo inbox, and will eventually be processed. I'm pretty sure if Diego tried fixing some real bugs, people would respond much faster... :-) -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb:host:Make the function room_on_ring bool
On 25.06.2015 19:27, Nicholas Krause wrote: This makes the function room_on_ring bool now due to this particular function only ever returning either one or zero as its return value. Signed-off-by: Nicholas Krause xerofo...@gmail.com Thanks, Looks good, but could you start the subject with usb: xhci:, or just xhci: for xhci specific changes -Mathias -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: storage: add no SYNCHRONIZE CACHE quirk
On Fri, 2015-06-26 at 11:43 +0200, Stefan Richter wrote: On Jun 22 James Bottomley wrote: On Mon, 2015-06-22 at 13:30 -0400, Alan Stern wrote: On Mon, 22 Jun 2015, James Bottomley wrote: Obviously, for a disk with a writeback cache that can't do flush, that window is much wider and the real solution should be to try to switch the cache to write through. I agree. Doing the switch manually (by writing to the cache_type attribute file) works, but it's a nuisance to do this when you have a portable USB drive that gets moved among a bunch of machines. Perhaps it might be wise to do this to every USB device ... for external devices, the small performance gain doesn't really make up for the potential data loss. Just a small note on the assumption of externally (and in extension, temporarily) attached devices: Not all USB-attached devices are external, and not all external devices are used as removable devices. The problems don't depend on the connection type: internal devices which have a writeback cache and don't accept flush commands have data integrity problems too. I can't really think of many situations where you'd be willing to sacrifice data integrity for performance. James (For example, I am using 2 internal CD-ROMs via USB-2 + usb-storage and 2 internal HDDs via USB 3 + uas in a PC with too few SATA ports and no PCIe slot to spare for a controller add-on card, but plenty of USB headers available on the mainboard. Similarly, some NASes have their operating system located on a USB-attached device. Small offices use USB-attached disks for backup and won't detach such a disk until rotation for off-site deposit. Not to mention embedded computers with USB-attached but fixed disks.) -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices
On Fri, 26 Jun 2015, Greg Kroah-Hartman wrote: This doesn't do what the patch title says. USB_DEVICE_FIXED means that the device can't be unplugged from its upstream port. It doesn't mean the device is internal to the computer. As an example, consider a composite Apple keyboard, which has an internal 3-port USB hub where two of the hub's ports are exposed on the edge of the keyboard case and the keyboard controller is permanently attached to the third hub port. Then the controller device would be marked USB_DEVICE_FIXED, even though the whole thing is external to the computer and can be unplugged. Is that really how those devices are marked? I can't find any of my keyboard with hubs that mark things that way. My Apple keyboard isn't here at the moment, and I don't remember exactly what its hub descriptor contains. In theory, it _should_ mark the permanently attached port as non-removable. I can test it next week, if you would like to see the actual values. Also, are you really certain this is safe? Aren't there a number of built-in keyboards that will work badly if you allow them to autosuspend? udev has been doing this for a while now by default, with no reports of problems, so I think we should be safe. I thought udev used a whitelist of devices known to work okay with autosuspend. Does it really turn on autosuspend for _every_ USB HID device that is marked as removable? (Come to think of it, given the bug in the hub driver, no device attached directly to the root hub will _ever_ be marked as removable AFAICS. So maybe that bug is masking possible regressions.) Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices
On Fri, Jun 26, 2015 at 09:20:19PM -0400, Alan Stern wrote:
On Fri, 26 Jun 2015, Greg Kroah-Hartman wrote:
This doesn't do what the patch title says. USB_DEVICE_FIXED means that
the device can't be unplugged from its upstream port. It doesn't mean
the device is internal to the computer.
As an example, consider a composite Apple keyboard, which has an
internal 3-port USB hub where two of the hub's ports are exposed on the
edge of the keyboard case and the keyboard controller is permanently
attached to the third hub port. Then the controller device would be
marked USB_DEVICE_FIXED, even though the whole thing is external to
the computer and can be unplugged.
Is that really how those devices are marked? I can't find any of my
keyboard with hubs that mark things that way.
My Apple keyboard isn't here at the moment, and I don't remember
exactly what its hub descriptor contains. In theory, it _should_ mark
the permanently attached port as non-removable.
I can test it next week, if you would like to see the actual values.
That would be great.
Also, are you really certain this is safe? Aren't there a number of
built-in keyboards that will work badly if you allow them to
autosuspend?
udev has been doing this for a while now by default, with no reports of
problems, so I think we should be safe.
I thought udev used a whitelist of devices known to work okay with
autosuspend. Does it really turn on autosuspend for _every_ USB HID
device that is marked as removable?
Yes, it had a tiny whitelist of 3-4 devices, and then would turn on
autosuspend for anything not marked as removable or unknown. Look at
/usr/lib/udev/rules.d/42-usb-hid-pm.rules on your system for them, it's
these lines:
ACTION==add, SUBSYSTEM==usb, SUBSYSTEMS==usb,
ATTR{../removable}==removable, GOTO=usb_hid_pm_end
ACTION==add, SUBSYSTEM==usb, SUBSYSTEMS==usb,
ATTR{../removable}==unknown, GOTO=usb_hid_pm_end
(Come to think of it, given the bug in the hub driver, no device
attached directly to the root hub will _ever_ be marked as removable
AFAICS. So maybe that bug is masking possible regressions.)
Maybe that's the issue, don't know, it would be good to figure out as
upstream udev just deleted that whole rules file :)
thanks,
greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices
On Fri, Jun 26, 2015 at 04:08:20PM -0400, Alan Stern wrote: On Fri, 26 Jun 2015, Tom Gundersen wrote: This policy used to be unconditionally applied by udev, but there is no reason to make userspace be involved in this and in the future udev will not be doing it by default. See: https://github.com/systemd/systemd/pull/353. Signed-off-by: Tom Gundersen t...@jklm.no Cc: Jiri Kosina jkos...@suse.cz Cc: Greg Kroah-Hartman gre...@linuxfoundation.org --- Hi, I don't have the right hardware for this, so it has only been compile-tested. I'm therefore sending it as an RFC only. Mainly I want to bring it to people's attention that it would be great to get this feature into the kernel as we want to drop it from udev. Cheers, Tom drivers/hid/usbhid/hid-core.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index bfbe1be..af80700 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -1358,6 +1358,9 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id * setup_timer(usbhid-io_retry, hid_retry_timeout, (unsigned long) hid); spin_lock_init(usbhid-lock); + if (dev-removable == USB_DEVICE_FIXED) + usb_enable_autosuspend(dev); This doesn't do what the patch title says. USB_DEVICE_FIXED means that the device can't be unplugged from its upstream port. It doesn't mean the device is internal to the computer. As an example, consider a composite Apple keyboard, which has an internal 3-port USB hub where two of the hub's ports are exposed on the edge of the keyboard case and the keyboard controller is permanently attached to the third hub port. Then the controller device would be marked USB_DEVICE_FIXED, even though the whole thing is external to the computer and can be unplugged. Is that really how those devices are marked? I can't find any of my keyboard with hubs that mark things that way. A reasonable compromise might be /* * Enable autosuspend for devices permanently attached * to the root hub. */ if (!dev-parent-parent dev-removable == USB_DEVICE_FIXED) usb_enable_autosuspend(dev); But this doesn't work if there's a permanently attached hub and a device permanently attached to that hub. To do this thoroughly, you have to iterate up the dev-parent chain, making sure at each step that the -removable value is USB_DEVICE_FIXED. Also, are you really certain this is safe? Aren't there a number of built-in keyboards that will work badly if you allow them to autosuspend? udev has been doing this for a while now by default, with no reports of problems, so I think we should be safe. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: about [PATCH] net:usb:cdc_ncm: fix that tag Huawei devices as wwan
Chenqi (jakio) cqi.c...@huawei.com writes: Hi, All, I'm from Huawei Technologies Co., Ltd, working in mobile broadband devices department. Huawei's broadband card don't support wwan in linux OS, there are several historical reasons: 1. Huawei produced dozens of broadband card types in past years, and our customers/users integrate our products in many different OS versions, We need give customer/user a continuous method to let them use convenience. 2. Wwan connection need application support, while we didn't test them for compatibility. Meanwhile, we have provided serial port to let other existing applications dial-up the connection I understand that you guys have some confusion why the Huawei broadband cards support wwan in windows OS but not in linux OS with the same device ID. Since wwan standard is used firstly in windows OS, when we pronounced to support this feature, wwan is not enabled in linux. So we keep the card's behavior in linux same as before. The Linux kernel/drivers/userspace always will attempt to use hardware in the same way Windows use it, or at least as close as we are able to get. The reason is simple: A billion (or whatever) Windows users is the best test lab you can get. You may not agree with this strategy, but as a hardware vendor you have to expect it to happen. The community will continue to develop support based on that assumption, even if you try to accommodate Linux by adding addiotional Linux specific features or modes. This wwan patch has great influence on our broadband cards, we have tested many our broadband cards with this kernel version, and this compatible issue is common. So please approve our modification for this problem. If noone else has any comments or objections here, then I am certainly not going to object to the proposed patch. Please go ahead and submit the patch formally. I am going to keep my big mouth shut :) I think the root cause of this issue is this kernel version trait the NCM as wwan devtype, maybe we can add a flag in usb descriptor in future to indicate whether the device support wwan or not, then Huawei can support wwan feature later for the newest product without introducing compatible issues. BTW: if we want to join the usb-ethernet kernel evolution in linux, which forum or organization should we join in? We can participate in it to avoid similar problems. USB networking drivers are discussed, along with other network drivers, on the net...@vger.kernel.org mailing list. Preferable with a copy to the linux-usb@vger.kernel.org mailing list for USB expert review. But as you point out above: wwan connections need application support. Userspace projects like ModemManager, oFono, libmbim, libqmi, umbim, uqmi and probably more, each have their own development forums/mailing lists. This is where the most important part of the wwan developemt takes place. Huawei have already made important contributions to the development of both Linux wwan drivers and userspace applications. I am sure all those projects will appreciate any further help Huawei are able to offer. Thanks a lot for your detailed introduction and explanation. Bjørn -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 64 byte EP0 OUT data transfer issue on Chipidea highspeed dual role controller
On Sat, Jun 27, 2015 at 2:21 AM, Alan Stern st...@rowland.harvard.edu wrote: On Sat, 27 Jun 2015, Jayan John wrote: On Fri, Jun 26, 2015 at 8:44 PM, David Laight david.lai...@aculab.com wrote: From: Steve Calfee Sent: 26 June 2015 15:59 On the host (Wandboard iMX6q) the test app opens /dev/hidraw0 and write 64 bytes with report ID (1). The HID device has no Interrupt OUT ep, therefore uses control endpoint ep0 for the 64 bytes transfer to gadget (Wandboard iMX6q) using set_report. The setup phase is OK and the 64 bytes is written to gadget. However the chipidea interrupt (ci_irq()) and resulting udc interrupt (udc_irq()) is invoked. This indicates that the 64 bytes transaction is not completed over ep0 from host to gadget. **this issue is reproducible for all data transfers that aligns on 64 bytes** ~jayan Hi Jayan, This sounds like a Zero Length Transfer issue. This applies to any endpoint including EP0. A ZLT is needed to end any transfer IFF the length is not already known by the protocol. So if the hid URB requests say 1024 bytes and the gadget only has 64, it must first send the 64 byte maxpacket and then send a zero length packet. This way the host knows the transfer is complete. Is this using the xhci driver? The ZLP sending code used to be broken. A couple of patches have been submitted but I don't remember one being applied. David Thanks for the pointers. This is not using the xhci driver. - This is a ep0 OUT transfer. Does the host need to add a ZLP for 64 byte aligned data transfer? I'm not sure what you mean here by aligned, but in general the answer is No. - Is it not expected for the completion interrupt be raised on receiving the 64 bytes of data, irrespective of whether or not a ZLP has been sent. It is expected. Did you check the wLength value in the Setup packet? Is it equal to 64? Does the hid gadget driver then queue a request for a 64-byte OUT transfer on ep0? If it does then there's a bug in the UDC driver or hardware. Alan Stern Thanks. Yes, the wLength value in the Setup packet is equal to 64. Aligned was the wrong term, multiple of 64 would be more appropriate :). The hid gadget driver queues a request for the transfer. Please see below logs.. ... HID: drivers/usb/gadget/f_hid_meu.c:366 - hidg_setup() HID: hid_setup crtl_request : bRequestType:0x21 bRequest:0x9 Value:0x200 HID: drivers/usb/chipidea/udc.c:1337 - ep_queue() HID: drivers/usb/chipidea/udc.c:794 - _ep_queue() HID: drivers/usb/chipidea/udc.c:467 - _hardware_enqueue() HID: drivers/usb/chipidea/udc.c:395 - add_td_to_list() HID: drivers/usb/chipidea/udc.c:61 - hw_ep_bit() HID: drivers/usb/chipidea/udc.c:209 - hw_ep_prime() .. In the 64 bytes case, the following logs are missing (indicating transfer complete interrupt): ... HID: drivers/usb/chipidea/core.c:368 - ci_irq() HID: drivers/usb/chipidea/udc.c:1786 - udc_irq() HID: drivers/usb/chipidea/udc.c:282 - hw_read_intr_status() HID: drivers/usb/chipidea/udc.c:271 - hw_read_intr_enable() HID: drivers/usb/chipidea/udc.c:309 - hw_test_and_clear_intr_active() HID: drivers/usb/chipidea/udc.c:992 - isr_tr_complete_handler() HID: drivers/usb/chipidea/udc.c:295 - hw_test_and_clear_complete() HID: drivers/usb/chipidea/udc.c:68 - ep_to_bit() HID: drivers/usb/chipidea/udc.c:957 - isr_tr_complete_low() HID: drivers/usb/chipidea/udc.c:583 - _hardware_dequeue() HID: drivers/usb/gadget/f_hid_meu.c:322 - hidg_set_report_complete() ... ~Jayan -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
Hi Oliver!
I wish sincerely to thank you again for your time and patience.
On Fri, 26 Jun 2015, Oliver Neukum wrote:
Date: Fri, 26 Jun 2015 10:14:02
From: Oliver Neukum oneu...@suse.com
To: Enrico Mioso mrkiko...@gmail.com
Cc: linux-usb@vger.kernel.org, net...@vger.kernel.org
Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Thu, 2015-06-25 at 17:19 +0200, Enrico Mioso wrote:
Hi Oliver! And thank you again.
I like / recommend the usage of open messaging standards: my preferred XMPP ID
(JID) is: mrk...@jit.si.
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Date: Thu, 25 Jun 2015 15:38:46
From: Oliver Neukum oneu...@suse.de
To: Enrico Mioso mrkiko...@gmail.com
Cc: linux-usb@vger.kernel.org, net...@vger.kernel.org
Subject: Re: [PATCH RFC] 2/2 huawei_cdc_ncm: introduce new TX ncm stack
On Thu, 2015-06-25 at 13:44 +0200, Enrico Mioso wrote:
On Thu, 25 Jun 2015, Oliver Neukum wrote:
Is there any advantage in keeping this in a single function?
I did this choice in the light of the fact I think the tx_fixup function will
become more complex than it is now, when aggregating frames.
Yes, but that is a reason to split the helpers up not the opposite.
Right - I understood only now your observation.
the only reason to write the manager that way was that it shouldn't become very
complex - it should simply do things to frames, helping out in building valid
NCM packages.
I answer here your other message to make it more convenient to read: my
intention for the tx_fixup function would be to:
- aggregate frames
- send them out when:
- a timer expires
How would you do that in tx_fixup()? If a timer is required then you
need a separate function.
Sure. I meant: I will adapt it in case needed, and expectin the code to become
a little bit more convoluted.
You cannot become any more convoluted even if you try.
OR
- we have enough data in the aggregate, and cannot add more.
Yes.
You need a third case:
- the interface is taken down.
But in general the logic for that is already there. So can you explain
what additional goals you have?
regarding the timer logic I saw it in cdc_ncm.c, but I should study it in
more detail to understand it and implement it here where needed in case.
It is involved. Basically a timer for transmission creates locking
issues. cdc-ncm uses an hrtimer which calls a bottom half which
calls xmit with a NULL skb.
/* if there is a remaining skb, it gets priority */
if (skb != NULL) {
swap(skb, ctx-tx_rem_skb);
swap(sign, ctx-tx_rem_sign);
} else {
ready2send = 1;
}
The else branch here is the timer triggering.
The actual transmission is done in usbnet if cdc_ncm_fill_tx_frame()
returns an skb.
I doubt you can can come up with anything more convoluted
but it simplifies locking.
Well, no, but it supposes a matched commit phase. Can you guarantee
that? I was under the oppression that in that phase you want to actually
give a frame over to the hardware.
No. When Italk about committing, I think about writing things to out skb.
another reason why i found confortable writing the code this way was to
maintain a kind of statefullness in a more clean way.
But I understand this is kind of agruable, and I can if needed break it up as
desired.
Regarding the commit phase - I am not sure I understand your comment, sorry
about that.
However, my intention would be to allow the caller to do calls in sequences
like:
- init frame
- ncm update
- ncm update
- ncm update
...
- ncm update
- ncm commit
(to work in huawei mode)
OR
- ncm init frame
- ncm commit
- ncm update
- ncm update
- ncm update
- ncm update
- finalize nth
(to work in cdc_ncm.c-mode)..
Sounds like a plan.
But to prevent usbnet from submittinx RX'd packets, I should be doing something
nasty here. And unfortunately don't understand why.
// some devices want funky USB-level framing, for
// win32 driver (usually) and/or hardware quirks
if (info-tx_fixup) {
skb = info-tx_fixup (dev, skb, GFP_ATOMIC);
if (!skb) {
/* packet collected; minidriver waiting for more
*/
if (info-flags FLAG_MULTI_PACKET)
goto not_drop;
So you just return NULL if you are ready to queue more. But you
need the FLAG_MULTI_PACKET flag.
Unfortuantely I might have not explained myself better.
I should thank you for this illumination on the timer part: since I know I'll
need this.
But what's stopping me right now is actually the RX, not the TX part.
Sure, the RX function I am using comes from cdc_ncm.c and works fine:
unfortunately, usbnet will not call it.
Infact, my dmesg ends up FULL of
kevent 2 may have been dropped messages.
And looking around I discovered I end up triggering a memory check in usbnet.c,
at line 475
skb = __netdev_alloc_skb_ip_align(dev-net, size, flags);
[PATCH 1/2] usb: chipidea: Reduce ULPI PHY reset pulse to datasheet spec of 1us
The datasheet for the 334x PHY mentions that a reset can be performed: ... by bringing the pin low for a minimum of 1 microsecond and then high. A delay of 5ms to implement that seems overly long, so reduce it to just 1us. As for the delay after reset, the datasheet only mentioned that the chip will assert the DIR output. 1ms seems like a safe time to wait for that to happen, so no change there. Signed-off-by: Mike Looijmans mike.looijm...@topic.nl --- drivers/usb/chipidea/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index e970863..c865abe 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -664,7 +664,7 @@ static int ci_hdrc_create_ulpi_phy(struct device *dev, struct ci_hdrc *ci) dev_err(dev, Failed to request ULPI reset gpio: %d\n, ret); return ret; } - msleep(5); + udelay(1); gpio_set_value_cansleep(reset_gpio, 1); msleep(1); } -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] usb: chipidea: Wait 50 ms before reading ID bit
The datasheet for the USB343x PHY mentions a 50ms wait time before
reading back the ID bit after enabling the internal pull-up or a
reset:
To monitor the status of the ID pin, the Link activates the IdPullup
bit in the OTG Control register, waits 50mS and then reads the status
of the IdGnd bit in the USB Interrupt Status register.
Implement this by adding a 50ms sleep at the only point in the code
where the ID status is being read without IRQ trigger.
When starting the board with a USB cable connected to a PC, the
system would activate host mode, then in ~20ms get an ID IRQ and
attempt to switch to gadget mode. This then failed because the
VBUS will not drop to zero (because the host is supplying it).
After this patch, the system starts up correctly and selects
gadget mode immediately, and the USB link works. It also fixes
the issue that the VBUS supply was being activated while already
being supplied from the host PC.
Signed-off-by: Mike Looijmans mike.looijm...@topic.nl
---
drivers/usb/chipidea/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index c865abe..4c6cf48 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -801,6 +801,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
if (ci-roles[CI_ROLE_HOST] ci-roles[CI_ROLE_GADGET]) {
if (ci-is_otg) {
+ msleep(50); /* Datasheet: Wait 50ms to read ID */
ci-role = ci_otg_role(ci);
/* Enable ID change irq */
hw_write_otgsc(ci, OTGSC_IDIE, OTGSC_IDIE);
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 64 byte EP0 OUT data transfer issue on Chipidea highspeed dual role controller
Thanks Alex. I appreciate you introducing me to Peter. Any help is appreciated. On the host (Wandboard iMX6q) the test app opens /dev/hidraw0 and write 64 bytes with report ID (1). The HID device has no Interrupt OUT ep, therefore uses control endpoint ep0 for the 64 bytes transfer to gadget (Wandboard iMX6q) using set_report. The setup phase is OK and the 64 bytes is written to gadget. However the chipidea interrupt (ci_irq()) and resulting udc interrupt (udc_irq()) is invoked. This indicates that the 64 bytes transaction is not completed over ep0 from host to gadget. **this issue is reproducible for all data transfers that aligns on 64 bytes** ~jayan On Fri, Jun 26, 2015 at 5:10 PM, Alexander Shishkin alexander.shish...@linux.intel.com wrote: Jayan John jayanjoh...@gmail.com writes: I am developing a custom USB device on a iMX6q platform (Wandboard) Chipidea HDRC (highspeed dual role controller). The HID interface consists of a single Interrupt IN ep and ep0. It is required to send HID reports from Host to Gadget over ep0 (with set_report cmd on hidraw interface) in OUT direction. The size of each HID report is 64 bytes. The Chipidea HDRC is unable to receive/ process the 64 bytes of data over ep0 (OUT). The setup stage is fine. No UDC interrupt is raised by controller to indicate completion of transaction. The same data transfer works fine for 64 bytes (63, 63 etc) and 64 bytes (65, 66 etc) where the transfers are split as 64+n. Analyzer logs attached. 1. Is there an issue with Chipidea HDRC receiving wMaxPacketSize (i.e. 64 byte) aligned data transfers on ep0 in OUT direction? 2. Anything that needs to be configured/ added in descriptor or source to have completion interrupt hit on 64 bytes data transfer on ep0 OUT? Let's add Peter to CC as he's the maintainer for Chipidea now. How do you generate this traffic? Are you using the zero gadget or some other kind of test? Regards, -- Alex -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
common code for CDC drivers
Hi, I am looking at all the CDC drivers. They all have a parser for the extra CDC headers. If I unify this would it be acceptable for cdc-acm to depend on usbnet? Or should I put the code into a header? Regards Oliver -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] usb: storage : Remove c99 style commenting
Hello. On 6/26/2015 9:14 AM, Sunny Kumar wrote: This patch fixes checkpatch.pl warning c99 style commenting. Signed-off-by: Sunny Kumar sunny.kumar@gmail.com --- drivers/usb/storage/sddr55.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c index aacedef..54d0a59 100644 --- a/drivers/usb/storage/sddr55.c +++ b/drivers/usb/storage/sddr55.c @@ -209,10 +209,10 @@ static int sddr55_read_data(struct us_data *us, unsigned int len, offset; struct scatterlist *sg; - // Since we only read in one block at a time, we have to create - // a bounce buffer and move the data a piece at a time between the - // bounce buffer and the actual transfer buffer. - + /* Since we only read in one block at a time, we have to create +* a bounce buffer and move the data a piece at a time between the +* bounce buffer and the actual transfer buffer. +*/ This style of commenting is preferred only in the networking code. The other parts of the kernel prefer this style: /* * bla * bla */ WBR, Sergei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: storage: add no SYNCHRONIZE CACHE quirk
On Jun 26 James Bottomley wrote: On Fri, 2015-06-26 at 11:43 +0200, Stefan Richter wrote: On Jun 22 James Bottomley wrote: [...] Perhaps it might be wise to do this to every USB device ... for external devices, the small performance gain doesn't really make up for the potential data loss. Just a small note on the assumption of externally (and in extension, temporarily) attached devices: Not all USB-attached devices are external, and not all external devices are used as removable devices. The problems don't depend on the connection type: internal devices which have a writeback cache and don't accept flush commands have data integrity problems too. I can't really think of many situations where you'd be willing to sacrifice data integrity for performance. Sure; writeback caches are prone to more issues besides sudden connection loss. (E.g. sudden power loss is but one of several more potential issues of course.) I merely wanted to remind that the bus type of a device (USB or whatever) does not say much about the risk of sudden connection loss to that device, since such devices may have physical protection against sudden connection loss too. I was not particularly commenting on the topic at hand, i.e. on presence of writeback cache without working flush command. -- Stefan Richter -=-= -==- ==-=- http://arcgraph.de/sr/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 64 byte EP0 OUT data transfer issue on Chipidea highspeed dual role controller
Jayan John jayanjoh...@gmail.com writes: I am developing a custom USB device on a iMX6q platform (Wandboard) Chipidea HDRC (highspeed dual role controller). The HID interface consists of a single Interrupt IN ep and ep0. It is required to send HID reports from Host to Gadget over ep0 (with set_report cmd on hidraw interface) in OUT direction. The size of each HID report is 64 bytes. The Chipidea HDRC is unable to receive/ process the 64 bytes of data over ep0 (OUT). The setup stage is fine. No UDC interrupt is raised by controller to indicate completion of transaction. The same data transfer works fine for 64 bytes (63, 63 etc) and 64 bytes (65, 66 etc) where the transfers are split as 64+n. Analyzer logs attached. 1. Is there an issue with Chipidea HDRC receiving wMaxPacketSize (i.e. 64 byte) aligned data transfers on ep0 in OUT direction? 2. Anything that needs to be configured/ added in descriptor or source to have completion interrupt hit on 64 bytes data transfer on ep0 OUT? Let's add Peter to CC as he's the maintainer for Chipidea now. How do you generate this traffic? Are you using the zero gadget or some other kind of test? Regards, -- Alex -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
