Re: [PATCH] USB: core: Improve unlocking of a mutex in two functions
On Sun, Nov 05, 2017 at 12:25:19AM +0100, Geert Uytterhoeven wrote: > On Sat, Nov 4, 2017 at 9:12 PM, SF Markus Elfring > wrote: > > From: Markus Elfring > > Date: Sat, 4 Nov 2017 21:00:46 +0100 > > > > * Add jump targets so that a call of the function "mutex_unlock" is stored > > only twice in these function implementations. > > > > * Replace five calls by goto statements. > > > > This issue was detected by using the Coccinelle software. > > > > Signed-off-by: Markus Elfring > > NAKed-by: Geert Uytterhoeven Don't worry, or waste your time, I don't take patches from this author as they are in my blacklist. 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] USB: core: Improve unlocking of a mutex in two functions
>> @@ -5529,8 +5528,7 @@ static int usb_reset_and_verify_device(struct >> usb_device *udev) >> dev_err(&udev->dev, >> "can't restore configuration #%d (error=%d)\n", >> udev->actconfig->desc.bConfigurationValue, ret); >> - mutex_unlock(hcd->bandwidth_mutex); >> - goto re_enumerate; >> + goto unlock; >> } >> mutex_unlock(hcd->bandwidth_mutex); >> usb_set_device_state(udev, USB_STATE_CONFIGURED); >> @@ -5583,6 +5581,8 @@ static int usb_reset_and_verify_device(struct >> usb_device *udev) >> udev->bos = bos; >> return 0; >> >> +unlock: >> + mutex_unlock(hcd->bandwidth_mutex); > > This makes it harder for the reader, I am curious if the view on the preferred code readability can be clarified further. > as the mutex_unlock() is now far below the block > of code that's protected by the lock. I got an other software development opinion for this aspect. Can the label be clear enough about the shown purpose already? Regards, Markus -- 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: USB: core: Improve unlocking of a mutex in two functions
> Don't worry, or waste your time, I don't take patches from this author
> as they are in my blacklist.
I am curious if our dialogue can become more constructive again. 🌈
I can offer another bit of information for this software development
discussion. 💭
The affected source files can be compiled for the processor architecture
“x86_64”
by a tool like “GCC 6.4.1+r251631-1.3” from the software distribution
“openSUSE Tumbleweed” with the following command example.
my_cc=/usr/bin/gcc-6 \
&& my_module1=drivers/usb/core/hub.o \
&& my_module2=drivers/usb/core/message.o \
&& git checkout next-20171102 \
&& make -j4 CC="${my_cc}" HOSTCC="${my_cc}" allmodconfig "${my_module1}"
"${my_module2}" \
&& size "${my_module1}" "${my_module2}" \
&& git checkout ':/^USB: core: Improve unlocking of a mutex in two functions' \
&& make -j4 CC="${my_cc}" HOSTCC="${my_cc}" allmodconfig "${my_module1}"
"${my_module2}" \
&& size "${my_module1}" "${my_module2}"
🔮 Do you find the following differences useful for further clarification?
╔═╤══╗
║ module │ text ║
╠═╪══╣
║ hub │ -16 ║
║ message │ -48 ║
╚═╧══╝
Regards,
Markus
--
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
Seagate Backup Plus UAS patch
Hello. Please, add Seagate Backup Plus entry to unusual devices list. diff -ur linux-4.9.orig/drivers/usb/storage/unusual_uas.h linux-4.9/drivers/usb/storage/unusual_uas.h --- linux-4.9.orig/drivers/usb/storage/unusual_uas.h 2016-12-11 22:17:54.0 +0300 +++ linux-4.9/drivers/usb/storage/unusual_uas.h 2017-11-05 14:55:58.703334883 +0300 @@ -121,6 +121,13 @@ USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_NO_ATA_1X), +/* Reported-by: Andrey Astafyev */ +UNUSUAL_DEV(0x0bc2, 0xab30, 0x, 0x, + "Seagate", + "BUP BK", + USB_SC_DEVICE, USB_PR_DEVICE, NULL, + US_FL_NO_ATA_1X), + /* Reported-by: Benjamin Tissoires */ UNUSUAL_DEV(0x13fd, 0x3940, 0x, 0x, "Initio Corporation", Bus 001 Device 007: ID 0bc2:ab30 Seagate RSS LLC Device Descriptor: bLength18 bDescriptorType 1 bcdUSB 2.10 bDeviceClass0 (Defined at Interface level) bDeviceSubClass 0 bDeviceProtocol 0 bMaxPacketSize064 idVendor 0x0bc2 Seagate RSS LLC idProduct 0xab30 bcdDevice1.08 iManufacturer 1 Seagate iProduct2 BUP BK iSerial 3 NA7TWXT4 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 85 bNumInterfaces 1 bConfigurationValue 1 iConfiguration 0 bmAttributes 0x80 (Bus Powered) MaxPower 500mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber0 bAlternateSetting 0 bNumEndpoints 2 bInterfaceClass 8 Mass Storage bInterfaceSubClass 6 SCSI bInterfaceProtocol 80 Bulk-Only iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes2 Transfer TypeBulk Synch Type None Usage Type Data wMaxPacketSize 0x0200 1x 512 bytes bInterval 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x02 EP 2 OUT bmAttributes2 Transfer TypeBulk Synch Type None Usage Type Data wMaxPacketSize 0x0200 1x 512 bytes bInterval 0 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber0 bAlternateSetting 1 bNumEndpoints 4 bInterfaceClass 8 Mass Storage bInterfaceSubClass 6 SCSI bInterfaceProtocol 98 iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x01 EP 1 OUT bmAttributes2 Transfer TypeBulk Synch Type None Usage Type Data wMaxPacketSize 0x0200 1x 512 bytes bInterval 0 Command pipe (0x01) Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x82 EP 2 IN bmAttributes2 Transfer TypeBulk Synch Type None Usage Type Data wMaxPacketSize 0x0200 1x 512 bytes bInterval 0 Status pipe (0x02) Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x83 EP 3 IN bmAttributes2 Transfer TypeBulk Synch Type None Usage Type Data wMaxPacketSize 0x0200 1x 512 bytes bInterval 0 Data-in pipe (0x03) Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x04 EP 4 OUT bmAttributes2 Transfer TypeBulk Synch Type None Usage Type Data wMaxPacketSize 0x0200 1x 512 bytes bInterval 0 Data-out pipe (0x04) Binary Object Store Descriptor: bLength 5 bDescriptorType15 wTotalLength 22 bNumDeviceCaps 2 USB 2.0 Extension Device Capability: bLength 7 bDescriptorType16 bDevCapabilityType 2 bmAttributes 0x0f0e Link Power Management (LPM) Supported SuperSpeed USB Device Capability: bLength10 bDescript
Re: Seagate Backup Plus UAS patch
On Sun, Nov 05, 2017 at 03:00:59PM +0300, Andrey Astafyev wrote: > Hello. Please, add Seagate Backup Plus entry to unusual devices list. > > > diff -ur linux-4.9.orig/drivers/usb/storage/unusual_uas.h > linux-4.9/drivers/usb/storage/unusual_uas.h > --- linux-4.9.orig/drivers/usb/storage/unusual_uas.h 2016-12-11 > 22:17:54.0 +0300 > +++ linux-4.9/drivers/usb/storage/unusual_uas.h 2017-11-05 > 14:55:58.703334883 +0300 > @@ -121,6 +121,13 @@ > USB_SC_DEVICE, USB_PR_DEVICE, NULL, > US_FL_NO_ATA_1X), > > +/* Reported-by: Andrey Astafyev */ > +UNUSUAL_DEV(0x0bc2, 0xab30, 0x, 0x, > + "Seagate", > + "BUP BK", > + USB_SC_DEVICE, USB_PR_DEVICE, NULL, > + US_FL_NO_ATA_1X), > + > /* Reported-by: Benjamin Tissoires */ > UNUSUAL_DEV(0x13fd, 0x3940, 0x, 0x, > "Initio Corporation", Can you resend this in a format we can apply it in (i.e. a signed-off-by line and other information as described in Documentation/SubmittingPatches)? 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] USB: add SPDX identifiers to all remaining files in drivers/usb/
On Sat, Nov 4, 2017 at 11:40 AM, Greg Kroah-Hartman wrote: > On Fri, Nov 03, 2017 at 05:53:01PM +0100, Johan Hovold wrote: >> On Fri, Nov 03, 2017 at 11:28:30AM +0100, Greg Kroah-Hartman wrote: >> > It's good to have SPDX identifiers in all files to make it easier to >> > audit the kernel tree for correct licenses. >> > >> > Update the drivers/usb/ and include/linux/usb* files with the correct >> > SPDX license identifier based on the license text in the file itself. >> > The SPDX identifier is a legally binding shorthand, which can be used >> > instead of the full boiler plate text. >> > >> > This work is based on a script and data from Thomas Gleixner, Philippe >> > Ombredanne, and Kate Stewart. >> > >> > Cc: Thomas Gleixner >> > Cc: Kate Stewart >> > Cc: Philippe Ombredanne >> > Signed-off-by: Greg Kroah-Hartman >> >> I noticed several MODULE_LICENSE macros which did not match the headers >> (e.g. "GPL" being used for version 2 only modules) for which I'll send a >> follow-up patch. >> >> Someone should probably write a script for that once the SPDX >> identifiers are in. > > Yes, I think that someone might have a script for that, it will be much > easier to detect these things now. The issue is that the "v2" marking > came after the original "GPL" marking for MODULE_LICENSE() from what I > remember, so many of those will be wrong. If this can help my [1] tool can detect both header-level licenses-in-comments as well as MODULE_LICENSE macros. Based on that we could reasonably easily craft a script that scans a file and report discrepancies between the two. FWIW this is the same tool that has been used to provide some input to Greg to clean things up here. [1] https://github.com/nexB/scancode-toolkit -- Cordially Philippe Ombredanne -- 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: add SPDX identifiers to all remaining files in drivers/usb/
On Sun, Nov 05, 2017 at 01:53:54PM +0100, Philippe Ombredanne wrote: > On Sat, Nov 4, 2017 at 11:40 AM, Greg Kroah-Hartman > wrote: > > On Fri, Nov 03, 2017 at 05:53:01PM +0100, Johan Hovold wrote: > >> On Fri, Nov 03, 2017 at 11:28:30AM +0100, Greg Kroah-Hartman wrote: > >> > It's good to have SPDX identifiers in all files to make it easier to > >> > audit the kernel tree for correct licenses. > >> > > >> > Update the drivers/usb/ and include/linux/usb* files with the correct > >> > SPDX license identifier based on the license text in the file itself. > >> > The SPDX identifier is a legally binding shorthand, which can be used > >> > instead of the full boiler plate text. > >> > > >> > This work is based on a script and data from Thomas Gleixner, Philippe > >> > Ombredanne, and Kate Stewart. > >> > > >> > Cc: Thomas Gleixner > >> > Cc: Kate Stewart > >> > Cc: Philippe Ombredanne > >> > Signed-off-by: Greg Kroah-Hartman > >> > >> I noticed several MODULE_LICENSE macros which did not match the headers > >> (e.g. "GPL" being used for version 2 only modules) for which I'll send a > >> follow-up patch. > >> > >> Someone should probably write a script for that once the SPDX > >> identifiers are in. > > > > Yes, I think that someone might have a script for that, it will be much > > easier to detect these things now. The issue is that the "v2" marking > > came after the original "GPL" marking for MODULE_LICENSE() from what I > > remember, so many of those will be wrong. > > If this can help my [1] tool can detect both header-level licenses-in-comments > as well as MODULE_LICENSE macros. Based on that we could reasonably > easily craft a script that scans a file and report discrepancies > between the two. That would be great, as there are going to be a lot of these showing up soon, as we start adding the SPDX identifiers to the files based on the license text and the mis-matches become obvious. 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] USB: add SPDX identifiers to all remaining files in drivers/usb/
On Sun, Nov 5, 2017 at 2:51 PM, Greg Kroah-Hartman wrote: > On Sun, Nov 05, 2017 at 01:53:54PM +0100, Philippe Ombredanne wrote: >> On Sat, Nov 4, 2017 at 11:40 AM, Greg Kroah-Hartman >> wrote: >> > On Fri, Nov 03, 2017 at 05:53:01PM +0100, Johan Hovold wrote: >> >> On Fri, Nov 03, 2017 at 11:28:30AM +0100, Greg Kroah-Hartman wrote: >> >> > It's good to have SPDX identifiers in all files to make it easier to >> >> > audit the kernel tree for correct licenses. >> >> > >> >> > Update the drivers/usb/ and include/linux/usb* files with the correct >> >> > SPDX license identifier based on the license text in the file itself. >> >> > The SPDX identifier is a legally binding shorthand, which can be used >> >> > instead of the full boiler plate text. >> >> > >> >> > This work is based on a script and data from Thomas Gleixner, Philippe >> >> > Ombredanne, and Kate Stewart. >> >> > >> >> > Cc: Thomas Gleixner >> >> > Cc: Kate Stewart >> >> > Cc: Philippe Ombredanne >> >> > Signed-off-by: Greg Kroah-Hartman >> >> >> >> I noticed several MODULE_LICENSE macros which did not match the headers >> >> (e.g. "GPL" being used for version 2 only modules) for which I'll send a >> >> follow-up patch. >> >> >> >> Someone should probably write a script for that once the SPDX >> >> identifiers are in. >> > >> > Yes, I think that someone might have a script for that, it will be much >> > easier to detect these things now. The issue is that the "v2" marking >> > came after the original "GPL" marking for MODULE_LICENSE() from what I >> > remember, so many of those will be wrong. >> >> If this can help my [1] tool can detect both header-level >> licenses-in-comments >> as well as MODULE_LICENSE macros. Based on that we could reasonably >> easily craft a script that scans a file and report discrepancies >> between the two. > > That would be great, as there are going to be a lot of these showing up > soon, as we start adding the SPDX identifiers to the files based on the > license text and the mis-matches become obvious. I can run a scancode scan to list modules with a license that does not match their MODULE_LICENSE (irrespective of whether they have an SPDX id already or not) I can then either provide a CSV (or provide an eventually big patch). Which do you prefer? What should be the tree to run this on: Yours? usb? Linus's? tip of the tree or a tag? If you prefer a patch, what should be the rationale when licenses do not match? I guess update the MODULE_LICENSE to match the license comment? -- Cordially Philippe Ombredanne -- 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: Seagate Backup Plus UAS patch
Hi, On 05-11-17 13:00, Andrey Astafyev wrote: Hello. Please, add Seagate Backup Plus entry to unusual devices list. As Greg already mentioned you need to submit this as a proper patch following the kernel's patch-submission guidelines. If you don't know how to do that I can do this for you, but I need your Signed-off-by to indicate that you are ok with this patch being added to the kernel under the GPL. It would be best if you just submit the patch yourself, but if you want me to do it and you're ok with this patch being added to the kernel under the GPL, please reply to this mail with: "Signed-off-by: Andrey Astafyev <1...@246060.ru>" Regards, Hans -- 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] usb: hub: Cycle HUB power when initialization fails
On 03-11-17 18:27, Alan Stern wrote:
On Fri, 3 Nov 2017, Mike Looijmans wrote:
Sometimes the USB device gets confused about the state of the initialization and
the connection fails. In particular, the device thinks that it's already set up
and running while the host thinks the device still needs to be configured. To
How do you know that this is really the issue? How can the device
think it's already set if it doesn't have an assigned address?
It seems to me that the device just doesn't react at all on the host
requests to assign an address. I've seen this happen with various custom
mass-storage like appliances, but also DVB tuners and such. The device
won't return to a working state until you unplug it and put it back, or,
and that's what the patch does, just power-cycle the USB port, which has
the same effect.
work around this issue, power-cycle the hub's output to issue a sort of "reset"
to the device. This makes the device restart its state machine and then the
initialization succeeds.
This fixes problems where the kernel reports a list of errors like this:
usb 1-1.3: device not accepting address 19, error -71
The end result is a non-functioning device. After this patch, the sequence
becomes like this:
usb 1-1.3: new high-speed USB device number 18 using ci_hdrc
usb 1-1.3: device not accepting address 18, error -71
usb 1-1.3: new high-speed USB device number 19 using ci_hdrc
usb 1-1.3: device not accepting address 19, error -71
usb 1-1-port3: attempt power cycle
usb 1-1.3: new high-speed USB device number 21 using ci_hdrc
usb-storage 1-1.3:1.2: USB Mass Storage device detected
Signed-off-by: Mike Looijmans
---
This is a fix I did for a customer which might be appropriate for upstream.
What do you think?
drivers/usb/core/hub.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e9ce6bb..a30c1e7 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2611,7 +2611,7 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
#define PORT_RESET_TRIES 5
#define SET_ADDRESS_TRIES 2
#define GET_DESCRIPTOR_TRIES 2
-#define SET_CONFIG_TRIES (2 * (use_both_schemes + 1))
+#define SET_CONFIG_TRIES (4 * (use_both_schemes + 1))
We already have too many retry loops. I am not keen on the idea of
adding even more. How about leaving this value the same and adding the
power cycle?
I'm fine with that as well.
The ideal, however, would be to find out what is wrong with the device
and see what needs to be done to fix it properly. This change won't
work on many computers (desktops and laptops) because they don't have
real USB port-power switching. A lot of hubs don't have it either.
I'm pretty sure it's the device's fault, but they're out there and
probably not upgradable anyway if you could get the manufacturer to pick
up the phone.
On desktop/laptop machines the problem isn't as pressing since there's a
often a user there who can unplug the thing. It's really nasty on
embedded systems, and they tend to have the USB power wired through a
supply limiter/switch.
I'm thinking worst that could happen on desktops is that the patch won't
have any effect at all.
So what do you think, is this worth a v2 patch for general consumption
or should I keep this a "private" patch for systems that have
demonstrated to benefit from it?
#define USE_NEW_SCHEME(i) ((i) / 2 == (int)old_scheme_first)
#define HUB_ROOT_RESET_TIME 60 /* times are in msec */
@@ -4805,7 +4805,6 @@ static void hub_port_connect(struct usb_hub *hub, int
port1, u16 portstatus,
status = 0;
for (i = 0; i < SET_CONFIG_TRIES; i++) {
-
Gratuitous whitespace change.
/* reallocate for each attempt, since references
* to the previous one can escape in various ways
*/
@@ -4935,6 +4934,15 @@ static void hub_port_connect(struct usb_hub *hub, int
port1, u16 portstatus,
usb_put_dev(udev);
if ((status == -ENOTCONN) || (status == -ENOTSUPP))
break;
+
+ /* When halfway through our retry count, power-cycle the port */
+ if (i == (SET_CONFIG_TRIES / 2) - 1) {
+ dev_info(&port_dev->dev, "attempt power cycle\n");
+ usb_hub_set_port_power(hdev, hub, port1, false);
+ msleep(800);
+ usb_hub_set_port_power(hdev, hub, port1, true);
+ msleep(hub_power_on_good_delay(hub));
+ }
}
if (hub->hdev->parent ||
!hcd->driver->port_handed_over ||
@@ -5476,7 +5484,6 @@ static int usb_reset_and_verify_device(struct usb_device
*udev)
udev->bos = NULL;
for (i = 0; i < SET_CONFIG_TRIES; ++i) {
-
Another gratuitous change.
/* ep0 maxpacket size may change; let the HCD know about it.
Re: [PATCH][RFC] usb: hub: Cycle HUB power when initialization fails
On 11/05/2017 10:41 AM, Mike Looijmans wrote:
On 03-11-17 18:27, Alan Stern wrote:
On Fri, 3 Nov 2017, Mike Looijmans wrote:
Sometimes the USB device gets confused about the state of the initialization and
the connection fails. In particular, the device thinks that it's already set up
and running while the host thinks the device still needs to be configured. To
How do you know that this is really the issue? How can the device
think it's already set if it doesn't have an assigned address?
It seems to me that the device just doesn't react at all on the host requests
to assign an address. I've seen this happen with various custom mass-storage
like appliances, but also DVB tuners and such. The device won't return to a
working state until you unplug it and put it back, or, and that's what the
patch does, just power-cycle the USB port, which has the same effect.
We have seen this problem with Ethernet dongles left in a bad/hung state during
a previous boot, so it definitely does happen. This happened, for example, with
the r8152 driver with upstream commit 2f25abe6bac ("r8152: prevent the driver
from transmitting packets with carrier off") missing.
Problem though, as mentioned, is that many hubs don't implement this feature,
and if I understand correctly root hubs don't implement it either.
Guenter
work around this issue, power-cycle the hub's output to issue a sort of "reset"
to the device. This makes the device restart its state machine and then the
initialization succeeds.
This fixes problems where the kernel reports a list of errors like this:
usb 1-1.3: device not accepting address 19, error -71
The end result is a non-functioning device. After this patch, the sequence
becomes like this:
usb 1-1.3: new high-speed USB device number 18 using ci_hdrc
usb 1-1.3: device not accepting address 18, error -71
usb 1-1.3: new high-speed USB device number 19 using ci_hdrc
usb 1-1.3: device not accepting address 19, error -71
usb 1-1-port3: attempt power cycle
usb 1-1.3: new high-speed USB device number 21 using ci_hdrc
usb-storage 1-1.3:1.2: USB Mass Storage device detected
Signed-off-by: Mike Looijmans
---
This is a fix I did for a customer which might be appropriate for upstream.
What do you think?
drivers/usb/core/hub.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e9ce6bb..a30c1e7 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2611,7 +2611,7 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
#define PORT_RESET_TRIES 5
#define SET_ADDRESS_TRIES 2
#define GET_DESCRIPTOR_TRIES 2
-#define SET_CONFIG_TRIES (2 * (use_both_schemes + 1))
+#define SET_CONFIG_TRIES (4 * (use_both_schemes + 1))
We already have too many retry loops. I am not keen on the idea of
adding even more. How about leaving this value the same and adding the
power cycle?
I'm fine with that as well.
The ideal, however, would be to find out what is wrong with the device
and see what needs to be done to fix it properly. This change won't
work on many computers (desktops and laptops) because they don't have
real USB port-power switching. A lot of hubs don't have it either.
I'm pretty sure it's the device's fault, but they're out there and probably not
upgradable anyway if you could get the manufacturer to pick up the phone.
On desktop/laptop machines the problem isn't as pressing since there's a often
a user there who can unplug the thing. It's really nasty on embedded systems,
and they tend to have the USB power wired through a supply limiter/switch.
I'm thinking worst that could happen on desktops is that the patch won't have
any effect at all.
So what do you think, is this worth a v2 patch for general consumption or should I keep
this a "private" patch for systems that have demonstrated to benefit from it?
#define USE_NEW_SCHEME(i) ((i) / 2 == (int)old_scheme_first)
#define HUB_ROOT_RESET_TIME 60 /* times are in msec */
@@ -4805,7 +4805,6 @@ static void hub_port_connect(struct usb_hub *hub, int
port1, u16 portstatus,
status = 0;
for (i = 0; i < SET_CONFIG_TRIES; i++) {
-
Gratuitous whitespace change.
/* reallocate for each attempt, since references
* to the previous one can escape in various ways
*/
@@ -4935,6 +4934,15 @@ static void hub_port_connect(struct usb_hub *hub, int
port1, u16 portstatus,
usb_put_dev(udev);
if ((status == -ENOTCONN) || (status == -ENOTSUPP))
break;
+
+ /* When halfway through our retry count, power-cycle the port */
+ if (i == (SET_CONFIG_TRIES / 2) - 1) {
+ dev_info(&port_dev->dev, "attempt power cycle\n");
+ usb_hub_set_port_power(hdev, hub, port1, false);
+ msleep(800);
+ usb_hub_set_port_power(hdev, hub, port1, true);
+ msleep(hub_power_on_good_delay(hub));
+ }
}
Re: [1/2] mm: drop migrate type checks from has_unmovable_pages
Hi Michal, the dwc2 USB driver on BCM2835 in linux-next is affected by the CMA allocation issue. A quick web search guide me to your patch, which avoid the issue. Since the patch wasn't accepted, i want to know is there another solution? Is this an issue in dwc2? Best regards Stefan -- 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 2/3] usb: xhci: Add DbC support in xHCI driver
Hi, On 11/03/2017 02:27 PM, Greg Kroah-Hartman wrote: > On Fri, Nov 03, 2017 at 08:45:46AM +0800, Lu Baolu wrote: >> Hi, >> >> On 11/03/2017 12:51 AM, Greg Kroah-Hartman wrote: >>> On Thu, Nov 02, 2017 at 12:38:57PM +0200, Felipe Balbi wrote: > Hi, > > Greg Kroah-Hartman writes: > Greg Kroah-Hartman writes: >>> xHCI compatible USB host controllers(i.e. super-speed USB3 >>> controllers) >>> can be implemented with the Debug Capability(DbC). It presents >>> a debug >>> device which is fully compliant with the USB framework and >>> provides the >>> equivalent of a very high performance full-duplex serial link. >>> The debug >>> capability operation model and registers interface are defined >>> in 7.6.8 >>> of the xHCI specification, revision 1.1. >>> >>> The DbC debug device shares a root port with the xHCI host. By >>> default, >>> the debug capability is disabled and the root port is assigned >>> to xHCI. >>> When the DbC is enabled, the root port will be assigned to the >>> DbC debug >>> device, and the xHCI sees nothing on this port. This >>> implementation uses >>> a sysfs node named under the xHCI device to manage the >>> enabling >>> and disabling of the debug capability. >>> >>> When the debug capability is enabled, it will present a debug >>> device >>> through the debug port. This debug device is fully compliant >>> with the >>> USB3 framework, and it can be enumerated by a debug host on the >>> other >>> end of the USB link. As soon as the debug device is configured, >>> a TTY >>> serial device named /dev/ttyDBC0 will be created. >>> >>> One use of this link is running a login service on the debug >>> target. >>> Hence it can be remote accessed by a debug host. Another use >>> case can >>> probably be found in servers. It provides a peer-to-peer USB >>> link >>> between two host-only machines. This provides a reasonable >>> out-of-band >>> communication method between two servers. >>> >>> Signed-off-by: Lu Baolu >>> --- >>> .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd | 25 + >>> drivers/usb/host/Kconfig |9 + >>> drivers/usb/host/Makefile |5 + >>> drivers/usb/host/xhci-dbgcap.c | 1016 >>> >>> drivers/usb/host/xhci-dbgcap.h | 247 + >>> drivers/usb/host/xhci-dbgtty.c | 586 >>> +++ >>> drivers/usb/host/xhci-trace.h | 60 ++ >>> drivers/usb/host/xhci.c| 10 + >>> drivers/usb/host/xhci.h|1 + >>> 9 files changed, 1959 insertions(+) >>> create mode 100644 >>> Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd >>> create mode 100644 drivers/usb/host/xhci-dbgcap.c >>> create mode 100644 drivers/usb/host/xhci-dbgcap.h >>> create mode 100644 drivers/usb/host/xhci-dbgtty.c >>> > [snip] > >>> +#define DBC_VENDOR_ID 0x1d6b /* Linux >>> Foundation 0x1d6b */ >>> +#define DBC_PRODUCT_ID 0x0004 /* device 0004 >>> */ >>> > The DbC (xHCI DeBug Capability) is an optional functionality in > some xHCI host controllers. It will present a super-speed debug > device through the debug port after it is enabled. > > The DbC register set defines an interface for system software > to specify the vendor id and product id of the debug device. > These two values will be presented by the debug device in its > device descriptor idVendor and idProduct fields. > > Microsoft Windows have a well established protocol for > debugging over DbC. And it assigns below values for its use. > > USB\VID_045E&PID_062D.DeviceDesc="Microsoft USB Debug Target" > > I'm going to use 0x1d6b/0x0004 value pair for DbC use in > Linux. Do you approve me to do so? >>> No. Why can't you use the same ids as Windows? This is >>> implementing >>> the same protocol, right?
[PATCH] usb:xhci fix panic in xhci_free_virt_devices_depth_first
From: Yu Chen
Check vdev->real_port 0 to avoid panic
[9.261347] []
xhci_free_virt_devices_depth_first+0x58/0x108
[9.261352] [] xhci_mem_cleanup+0x1bc/0x570
[9.261355] [] xhci_stop+0x140/0x1c8
[9.261365] [] usb_remove_hcd+0xfc/0x1d0
[9.261369] [] xhci_plat_remove+0x6c/0xa8
[9.261377] [] platform_drv_remove+0x2c/0x70
[9.261384] [] __device_release_driver+0x80/0x108
[9.261387] [] device_release_driver+0x2c/0x40
[9.261392] [] bus_remove_device+0xe0/0x120
[9.261396] [] device_del+0x114/0x210
[9.261399] [] platform_device_del+0x30/0xa0
[9.261403] [] dwc3_otg_work+0x204/0x488
[9.261407] [] event_work+0x304/0x5b8
[9.261414] [] process_one_work+0x148/0x490
[9.261417] [] worker_thread+0x50/0x4a0
[9.261421] [] kthread+0xe8/0x100
[9.261427] [] ret_from_fork+0x10/0x50
The problem can occur if xhci_plat_remove() is called shortly after
xhci_plat_probe(). While xhci_free_virt_devices_depth_first been
called before the device has been setup and get real_port initialized.
The problem occurred on Hikey960 and was reproduced by Guenter Roeck
on Kevin with chromeos-4.4.
Cc: Guenter Roeck
Signed-off-by: Fan Ning
Signed-off-by: Li Rui
Signed-off-by: yangdi
Signed-off-by: Yu Chen
---
drivers/usb/host/xhci-mem.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 2a82c927ded2..295789d993b0 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -947,6 +947,11 @@ void xhci_free_virt_devices_depth_first(struct xhci_hcd
*xhci, int slot_id)
if (!vdev)
return;
+ if (WARN_ON(!vdev->real_port)) {
+ xhci_warn(xhci, "Bad vdev->real_port\n");
+ goto out;
+ }
+
tt_list_head = &(xhci->rh_bw[vdev->real_port - 1].tts);
list_for_each_entry_safe(tt_info, next, tt_list_head, tt_list) {
/* is this a hub device that added a tt_info to the tts list */
@@ -960,6 +965,7 @@ void xhci_free_virt_devices_depth_first(struct xhci_hcd
*xhci, int slot_id)
}
}
}
+out:
/* we are now at a leaf device */
xhci_free_virt_device(xhci, slot_id);
}
--
2.15.0-rc2
--
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: [1/2] mm: drop migrate type checks from has_unmovable_pages
On 11/05/2017 10:56 PM, Stefan Wahren wrote: > Hi Michal, > > the dwc2 USB driver on BCM2835 in linux-next is affected by the CMA > allocation issue. A quick web search guide me to your patch, which avoid the > issue. > > Since the patch wasn't accepted, i want to know is there another solution? AFAIK it was accepted: http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-drop-migrate-type-checks-from-has_unmovable_pages.patch So I'd expect it to be in current linux-next as well. > Is this an issue in dwc2? > > Best regards > Stefan > -- 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 WARN Event TRB for slot ep with no TDs queued?
Hi, This is the same trace with kernel 4.12.9: https://pastebin.com/280Xeu8X I'll try to do a "git bisect" with kernel today. Thanks. Regards. 2017-11-02 18:18 GMT+01:00 Mathias Nyman : > On 02.11.2017 16:31, Juan Simón wrote: >> >> Hi, >> This is the trace output: https://pastebin.com/apt56yGe > > > Thanks > xhci logs look pretty good to me. A interrupt transfer TRB is queued > asking for 32 bytes from the mouse, and mouse replies with 15 bytes. > After this and we immediately queue the next TRB. > > Are you sure this generates the "WARN TRB for slot x ep with no TDs queued"? > can you check dmesg if those really appear? > In the logs we always have a TRB (TD) queued when we receive events. > > The mouse is however constantly responding with 15 bytes of data, > as if it's constantly moving, sending data. > > Not sure how HID devices normally behave, but my guess is that they would > respond with a NAK to the interrupt transfer if no data is pending. > > git bisect would show when it stopped working, could be a change > somewhere else as well, hid maybe? > > -Mathias > > (leaving rest of message for reference) >> >> >> I'm going to ask in Arch forums to see how I can get the differences >> of the xhci_hcd module in both versions of the kernel. >> Thanks. >> Regards. >> >> 2017-11-01 12:11 GMT+01:00 Mathias Nyman : >>> >>> On 31.10.2017 16:45, Juan Simón wrote: Hi, Thanks but that patch doesn't work for me. The warnings in system log aren't the problem. In my case, with 4.13.x kernels, the problem is that the system continuously receives mouse signals.How do I detect it? Because when I put a video on YouTube the bar with the controls never disappears even though the mouse is still. The same goes for Netflix videos. And when I run a command in a terminal emulator with a long output and scroll to read the initial output I can't because the terminal receives a signal from the mouse continuously that makes it returns to the end. At first I thought the problem was the type of mouse I was using: Logitech MX Master. But I've tested with a simple wired mouse and it also fails. On the other hand, I bought this tower from an online store that is specialized in selling computers with Linux compatible hardware. And all its components are fully compatible with Linux but it turns out that now I have this problem and I don't know how to fix it. Is Arch Linux a problem? Is it a kernel problem? Is it a hardware problem? With the exception of the network card, which is from Realtek, the rest of the hardware is from Intel. Isn't Intel hardware supposed to be Linux-friendly? >>> >>> >>> >>> Thats why I'm here helping you debug this ;) >>> >>> If the problem started when upgrading the kernel from 4.12.x to 4.13.x >>> then I'd start looking there. As Greg said, git bisect is the best way >>> to find which change caused this regression. >>> >>> second best way is to show me xhci traces of the this. >>> xhci traces can be taken with: >>> >>> mount -t debugfs none /sys/kernel/debug >>> echo 81920 > /sys/kernel/debug/tracing/buffer_size_kb >>> echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable >>> < do whatever is needed to trigger issue> >>> then send me content of /sys/kernel/debug/tracing/trace >>> >>> -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 >> > -- 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
