[PATCH v2 1/2] usb: gadget: ether: Add \n to each attribute of ethernet functions
Generally in SysFS and ConfigFS files are new line terminated.
Also most of USB functions adds a trailing newline to each attribute.
Let's follow this convention also in ethernet functions.
Signed-off-by: Krzysztof Opasiak
---
drivers/usb/gadget/function/u_ether.c | 24
drivers/usb/gadget/function/u_ether_configfs.h | 2 +-
2 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/function/u_ether.c
b/drivers/usb/gadget/function/u_ether.c
index 5d1bd13a56c1..8a50d1ccab5d 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -918,9 +918,16 @@ EXPORT_SYMBOL_GPL(gether_set_dev_addr);
int gether_get_dev_addr(struct net_device *net, char *dev_addr, int len)
{
struct eth_dev *dev;
+ int ret;
dev = netdev_priv(net);
- return get_ether_addr_str(dev->dev_mac, dev_addr, len);
+ ret = get_ether_addr_str(dev->dev_mac, dev_addr, len);
+ if (ret + 1 < len) {
+ dev_addr[ret++] = '\n';
+ dev_addr[ret] = '\0';
+ }
+
+ return ret;
}
EXPORT_SYMBOL_GPL(gether_get_dev_addr);
@@ -940,9 +947,16 @@ EXPORT_SYMBOL_GPL(gether_set_host_addr);
int gether_get_host_addr(struct net_device *net, char *host_addr, int len)
{
struct eth_dev *dev;
+ int ret;
dev = netdev_priv(net);
- return get_ether_addr_str(dev->host_mac, host_addr, len);
+ ret = get_ether_addr_str(dev->host_mac, host_addr, len);
+ if (ret + 1 < len) {
+ host_addr[ret++] = '\n';
+ host_addr[ret] = '\0';
+ }
+
+ return ret;
}
EXPORT_SYMBOL_GPL(gether_get_host_addr);
@@ -989,10 +1003,12 @@ EXPORT_SYMBOL_GPL(gether_get_qmult);
int gether_get_ifname(struct net_device *net, char *name, int len)
{
+ int ret;
+
rtnl_lock();
- strlcpy(name, netdev_name(net), len);
+ ret = snprintf(name, len, "%s\n", netdev_name(net));
rtnl_unlock();
- return strlen(name);
+ return ret < len ? ret : len;
}
EXPORT_SYMBOL_GPL(gether_get_ifname);
diff --git a/drivers/usb/gadget/function/u_ether_configfs.h
b/drivers/usb/gadget/function/u_ether_configfs.h
index 4f47289fcf7c..c71133de17e7 100644
--- a/drivers/usb/gadget/function/u_ether_configfs.h
+++ b/drivers/usb/gadget/function/u_ether_configfs.h
@@ -108,7 +108,7 @@
mutex_lock(&opts->lock);\
qmult = gether_get_qmult(opts->net);\
mutex_unlock(&opts->lock); \
- return sprintf(page, "%d", qmult); \
+ return sprintf(page, "%d\n", qmult);\
} \
\
static ssize_t _f_##_opts_qmult_store(struct config_item *item, \
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] usb: gadget: printer: Remove pnp_string static buffer
pnp string is usually much shorter than 1k so let's stop wasting 1k of
memory for its buffer and make it dynamically alocated.
This also removes 1k len limitation for pnp_string and
adds a new line after string content if required.
Signed-off-by: Krzysztof Opasiak
---
Changes since v1:
- assignt true instead of statement with no effect (reported by kbuild)
---
drivers/usb/gadget/function/f_printer.c | 57 +
drivers/usb/gadget/function/u_printer.h | 5 ++-
drivers/usb/gadget/legacy/printer.c | 28 +---
3 files changed, 62 insertions(+), 28 deletions(-)
diff --git a/drivers/usb/gadget/function/f_printer.c
b/drivers/usb/gadget/function/f_printer.c
index 0de36cda6e41..36fe181e72c5 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -49,7 +49,6 @@
#include "u_printer.h"
-#define PNP_STRING_LEN 1024
#define PRINTER_MINORS 4
#define GET_DEVICE_ID 0
#define GET_PORT_STATUS1
@@ -907,8 +906,7 @@ static bool gprinter_req_match(struct usb_function *f,
switch (ctrl->bRequest) {
case GET_DEVICE_ID:
w_index >>= 8;
- if (w_length <= PNP_STRING_LEN &&
- (USB_DIR_IN & ctrl->bRequestType))
+ if (USB_DIR_IN & ctrl->bRequestType)
break;
return false;
case GET_PORT_STATUS:
@@ -937,6 +935,7 @@ static int printer_func_setup(struct usb_function *f,
struct printer_dev *dev = func_to_printer(f);
struct usb_composite_dev *cdev = f->config->cdev;
struct usb_request *req = cdev->req;
+ u8 *buf = req->buf;
int value = -EOPNOTSUPP;
u16 wIndex = le16_to_cpu(ctrl->wIndex);
u16 wValue = le16_to_cpu(ctrl->wValue);
@@ -953,10 +952,16 @@ static int printer_func_setup(struct usb_function *f,
if ((wIndex>>8) != dev->interface)
break;
- value = (dev->pnp_string[0] << 8) | dev->pnp_string[1];
- memcpy(req->buf, dev->pnp_string, value);
+ if (!dev->pnp_string) {
+ value = 0;
+ break;
+ }
+ value = strlen(dev->pnp_string);
+ buf[0] = (value >> 8) & 0xFF;
+ buf[1] = value & 0xFF;
+ memcpy(buf + 2, dev->pnp_string, value);
DBG(dev, "1284 PNP String: %x %s\n", value,
- &dev->pnp_string[2]);
+ dev->pnp_string);
break;
case GET_PORT_STATUS: /* Get Port Status */
@@ -964,7 +969,7 @@ static int printer_func_setup(struct usb_function *f,
if (wIndex != dev->interface)
break;
- *(u8 *)req->buf = dev->printer_status;
+ buf[0] = dev->printer_status;
value = min_t(u16, wLength, 1);
break;
@@ -1157,10 +1162,21 @@ static ssize_t f_printer_opts_pnp_string_show(struct
config_item *item,
char *page)
{
struct f_printer_opts *opts = to_f_printer_opts(item);
- int result;
+ int result = 0;
mutex_lock(&opts->lock);
- result = strlcpy(page, opts->pnp_string + 2, PNP_STRING_LEN - 2);
+ if (!opts->pnp_string)
+ goto unlock;
+
+ result = strlcpy(page, opts->pnp_string, PAGE_SIZE);
+ if (result >= PAGE_SIZE) {
+ result = PAGE_SIZE;
+ } else if (page[result - 1] != '\n' && result + 1 < PAGE_SIZE) {
+ page[result++] = '\n';
+ page[result] = '\0';
+ }
+
+unlock:
mutex_unlock(&opts->lock);
return result;
@@ -1170,13 +1186,24 @@ static ssize_t f_printer_opts_pnp_string_store(struct
config_item *item,
const char *page, size_t len)
{
struct f_printer_opts *opts = to_f_printer_opts(item);
- int result, l;
+ char *new_pnp;
+ int result;
mutex_lock(&opts->lock);
- result = strlcpy(opts->pnp_string + 2, page, PNP_STRING_LEN - 2);
- l = strlen(opts->pnp_string + 2) + 2;
- opts->pnp_string[0] = (l >> 8) & 0xFF;
- opts->pnp_string[1] = l & 0xFF;
+
+ new_pnp = kstrndup(page, len, GFP_KERNEL);
+ if (!new_pnp) {
+ result = -ENOMEM;
+ goto unlock;
+ }
+
+ if (opts->pnp_string_allocated)
+ kfree(opts->pnp_string);
+
+ opts->pnp_string_allocated = true;
+ opts->pnp_string = new_pnp;
+ result = len;
+unlock:
mutex_unlock(&opts->lock);
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On 21 December 2016 at 11:48, NeilBrown wrote:
> On Wed, Dec 21 2016, Baolin Wang wrote:
>
>> Hi,
>>
>> On 21 December 2016 at 06:07, NeilBrown wrote:
>>> On Tue, Dec 20 2016, Baolin Wang wrote:
>>>
Hi Neil,
On 3 November 2016 at 09:25, NeilBrown wrote:
> On Tue, Nov 01 2016, Baolin Wang wrote:
>
>
>>> So I won't be responding on this topic any further until I see a genuine
>>> attempt to understand and resolve the inconsistencies with
>>> usb_register_notifier().
>>
>> Any better solution?
>
> I'm not sure exactly what you are asking, so I'll assume you are asking
> the question I want to answer :-)
>
> 1/ Liase with the extcon developers to resolve the inconsistencies
> with USB connector types.
> e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
> which both seem to suggest a standard downstream port. There is no
> documentation describing how these relate, and no consistent practice
> to copy.
> I suspect the intention is that
> EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
> the cable, while EXTCON_CHG_USB* indicate the power capabilities of
> the cable.
> So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
> while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
> would normally appear with EXTCON_USB_HOST (I think).
> Some drivers follow this model, particularly extcon-max14577.c
> but it is not consistent.
>
> This policy should be well documented and possibly existing drivers
> should be updated to follow it.
>
> At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
> and EXTCON_CHG_USB_FAST. These names don't mean much.
> They were recently removed from drivers/power/axp288_charger.c
> which is good, but are still used in drivers/extcon/extcon-max*
> Possibly they should be changed to names from the standard, or
> possibly they should be renamed to identify the current they are
> expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
Now I am creating the new patchset with fixing and converting exist
drivers.
>>>
>>> Thanks!
>>>
I did some investigation about EXTCON subsystem. From your suggestion:
1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
After checking, now all extcon drivers were following this rule.
>>>
>>> what about extcon-axp288.c ?
>>> axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
>>> never sets EXTCON_USB.
>>> Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.
>>
>> Ha, sorry, I missed these 2 files, and I will fix them.
>>
>>>
2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
change.
>>>
>>> Agreed.
>>>
3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
There are no model that shows the slow charger should be 500mA
and fast charger is 1A. (In extcon-max77693.c file, the fast charger
can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
I think.
>>>
>>> Leaving the names a SLOW/FAST is less useful as those names don't *mean*
>>> anything.
>>> The only place where the cable types are registered are in
>>> extcon-max{14577,77693,77843,8997}.c
>>>
>>> In each case, the code strongly suggests that the meaning is that "slow"
>>> means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
>>>
>>> With names like "fast" and "slow", any common changer framework cannot
>>> make use of these cable types as the name doesn't mean anything.
>>> If the names were changed to 500MA/1A, then common code could reasonably
>>> assume how much current can safely be drawn from each.
>>
>> As I know, some fast charger can be drawn 5A, then do we need another
>> macro named 5A? then will introduce more macros in future, I am not
>> true this is helpful.
>
> It isn't really a question of what the charger can provide. It is a
> question of what the cable reports to the phy.
Yes, there is no spec to describe fast/slow charger type and how much
current fast/slow charger can provide. Maybe some fast charger can
provide 1A/2A, others can provide 5A, which depends on users'
platform. If we change to EXTCON_CHG_USB_500MA/1A and some fast
charger can provide 1.5A on user's platform, will it report the fast
charger type by EXTCON_CHG_USB_1A on user's platform (but it can
provide 1.5A)? So what I mean, can we keep EXTCON_CHG_USB_SLOW/FAST as
they were, and maybe fix them in future? (BTW, I've fixed issue 1 and
maintainer has applied them).
>
> Unfortunately I cannot find any datasheets on line to verify how these
> devices work, but the code seems to suggest that they can detect and
> report special charger types that provide 500mA and 1A respect
Re: JMS56x not working reliably with uas driver
On Wed, 2016-12-21 at 17:09 +0530, George Cherian wrote: > Hi Oliver, > > I was working with this JMicron device and using the uas driver. > I am seeing the following 2 issues. > > 1) On connect I see the following messages. Thanks. Do you want to submit it to Greg? The patch is fine. > 2) On disconnect I am seeing the following issue > > scsi host4: uas_post_reset: alloc streams error -19 after reset > sd 4:0:0:0: [sdb] Synchronizing SCSI cache > > This is more fatal because after these messages the USB port becomes > unusable. Even an lsusb invocation hangs for ever. Ouch. That points to a logic error. We should not reset if a device is gone. Could you send dmesg of such a case? 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: JMS56x not working reliably with uas driver
Hi, On 21-12-16 12:39, George Cherian wrote: Hi Oliver, I was working with this JMicron device and using the uas driver. I am seeing the following 2 issues. 1) On connect I see the following messages. xhci_hcd :00:11.0: ERROR Transfer event for disabled endpoint or incorrect stream ring This was eliminated using the following scissor patch. -8< [PATCH] usb: storage: unusual_uas: Add JMicron JMS56x to unusual device This device gives the following error on detection. xhci_hcd :00:11.0: ERROR Transfer event for disabled endpoint or incorrect stream ring The same error is not seen when it is added to unusual_device list with US_FL_NO_REPORT_OPCODES passed. Signed-off-by: George Cherian --- drivers/usb/storage/unusual_uas.h | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/usb/storage/unusual_uas.h b/drivers/usb/storage/unusual_uas.h index cbea9f3..d292299 100644 --- a/drivers/usb/storage/unusual_uas.h +++ b/drivers/usb/storage/unusual_uas.h @@ -142,6 +142,13 @@ UNUSUAL_DEV(0x152d, 0x0567, 0x, 0x, USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_BROKEN_FUA | US_FL_NO_REPORT_OPCODES), +/* Reported-by George Cherian */ +UNUSUAL_DEV(0x152d, 0x9561, 0x, 0x, +"JMicron", +"JMS56x", +USB_SC_DEVICE, USB_PR_DEVICE, NULL, +US_FL_NO_REPORT_OPCODES), + /* Reported-by: Hans de Goede */ UNUSUAL_DEV(0x2109, 0x0711, 0x, 0x, "VIA", ->8 2) On disconnect I am seeing the following issue scsi host4: uas_post_reset: alloc streams error -19 after reset sd 4:0:0:0: [sdb] Synchronizing SCSI cache This is more fatal because after these messages the USB port becomes unusable. Even an lsusb invocation hangs for ever. Also please note that the device works fine with usb-storage driver. I am attaching the usbmon capture of disconnect using uas and usb-storage driver. Any help in this regard is highly appreciated. Are you still seeen this second problem with the first patch applied ? Is this after an actual disconnect, or after the kernel seeing a disconnect without the device being actually disconnected. This second problem sounds like it is an issue with your xhci controller. Can you try this on another motherboard (with another xhci controller) ? 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: JMS56x not working reliably with uas driver
Hi, On 21-12-16 12:42, Oliver Neukum wrote: On Wed, 2016-12-21 at 17:09 +0530, George Cherian wrote: Hi Oliver, I was working with this JMicron device and using the uas driver. I am seeing the following 2 issues. 1) On connect I see the following messages. Thanks. Do you want to submit it to Greg? The patch is fine. 2) On disconnect I am seeing the following issue scsi host4: uas_post_reset: alloc streams error -19 after reset sd 4:0:0:0: [sdb] Synchronizing SCSI cache This is more fatal because after these messages the USB port becomes unusable. Even an lsusb invocation hangs for ever. Ouch. That points to a logic error. We should not reset if a device is gone. Ah good point, so instead of just seeing a disconnect, something is triggering a reset, and then we try to alloc stream on the disconnected usb port and that apparently makes the xhci controller quite unhappy. 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: JMS56x not working reliably with uas driver
On Wed, 2016-12-21 at 12:54 +0100, Hans de Goede wrote: > Hi, > > On 21-12-16 12:42, Oliver Neukum wrote: > > On Wed, 2016-12-21 at 17:09 +0530, George Cherian wrote: > >> Hi Oliver, > >> > >> I was working with this JMicron device and using the uas driver. > >> I am seeing the following 2 issues. > >> > >> 1) On connect I see the following messages. > > > > Thanks. Do you want to submit it to Greg? > > The patch is fine. > > > >> 2) On disconnect I am seeing the following issue > >> > >> scsi host4: uas_post_reset: alloc streams error -19 after reset > >> sd 4:0:0:0: [sdb] Synchronizing SCSI cache > >> > >> This is more fatal because after these messages the USB port becomes > >> unusable. Even an lsusb invocation hangs for ever. > > > > Ouch. That points to a logic error. We should not reset if > > a device is gone. > > Ah good point, so instead of just seeing a disconnect, something > is triggering a reset, and then we try to alloc stream on the > disconnected usb port and that apparently makes the xhci controller > quite unhappy. It is problematic because there is a window we cannot avoid. This needs a full dmesg to say anything more about it. 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
JMS56x not working reliably with uas driver
Hi Oliver, I was working with this JMicron device and using the uas driver. I am seeing the following 2 issues. 1) On connect I see the following messages. xhci_hcd :00:11.0: ERROR Transfer event for disabled endpoint or incorrect stream ring This was eliminated using the following scissor patch. -8< [PATCH] usb: storage: unusual_uas: Add JMicron JMS56x to unusual device This device gives the following error on detection. xhci_hcd :00:11.0: ERROR Transfer event for disabled endpoint or incorrect stream ring The same error is not seen when it is added to unusual_device list with US_FL_NO_REPORT_OPCODES passed. Signed-off-by: George Cherian --- drivers/usb/storage/unusual_uas.h | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/usb/storage/unusual_uas.h b/drivers/usb/storage/unusual_uas.h index cbea9f3..d292299 100644 --- a/drivers/usb/storage/unusual_uas.h +++ b/drivers/usb/storage/unusual_uas.h @@ -142,6 +142,13 @@ UNUSUAL_DEV(0x152d, 0x0567, 0x, 0x, USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_BROKEN_FUA | US_FL_NO_REPORT_OPCODES), +/* Reported-by George Cherian */ +UNUSUAL_DEV(0x152d, 0x9561, 0x, 0x, +"JMicron", +"JMS56x", +USB_SC_DEVICE, USB_PR_DEVICE, NULL, +US_FL_NO_REPORT_OPCODES), + /* Reported-by: Hans de Goede */ UNUSUAL_DEV(0x2109, 0x0711, 0x, 0x, "VIA", ->8 2) On disconnect I am seeing the following issue scsi host4: uas_post_reset: alloc streams error -19 after reset sd 4:0:0:0: [sdb] Synchronizing SCSI cache This is more fatal because after these messages the USB port becomes unusable. Even an lsusb invocation hangs for ever. Also please note that the device works fine with usb-storage driver. I am attaching the usbmon capture of disconnect using uas and usb-storage driver. Any help in this regard is highly appreciated. Regards, -George 801f5efb8a00 57530621 C Ii:4:002:1 0:128 1 = 08 801f5efb8a00 57530654 S Ii:4:002:1 -115:128 2 < 801f61285a00 57530677 S Ci:4:002:0 s a3 00 0003 0004 4 < 801f61285a00 57531618 C Ci:4:002:0 0 4 = c1024000 801f61285a00 57531634 S Co:4:002:0 s 23 01 0019 0003 0 801f61285a00 57531992 C Co:4:002:0 0 0 801f61285a00 57532225 S Ci:4:002:0 s a3 00 0003 0004 4 < 801f61285a00 57533010 C Ci:4:002:0 0 4 = c102 801f61285a00 57533022 S Co:4:002:0 s 23 03 001c 0003 0 801f61285a00 57533405 C Co:4:002:0 0 0 801f61285a00 57553165 S Ci:4:002:0 s a3 00 0003 0004 4 < 801f61285a00 57554174 C Ci:4:002:0 0 4 = b102 801f61285a00 57573164 S Ci:4:002:0 s a3 00 0003 0004 4 < 801f61285a00 57574064 C Ci:4:002:0 0 4 = b102 801f61285a00 57593169 S Ci:4:002:0 s a3 00 0003 0004 4 < 801f61285a00 57594214 C Ci:4:002:0 0 4 = b102 801f5efb8a00 57642612 C Ii:4:002:1 0:128 1 = 08 801f5efb8a00 57642621 S Ii:4:002:1 -115:128 2 < 801f5efb8a00 57658612 C Ii:4:002:1 0:128 1 = 08 801f5efb8a00 57658618 S Ii:4:002:1 -115:128 2 < 801f5efb8a00 57674611 C Ii:4:002:1 0:128 1 = 08 801f5efb8a00 57674617 S Ii:4:002:1 -115:128 2 < 801f5efb8a00 57690610 C Ii:4:002:1 0:128 1 = 08 801f5efb8a00 57690615 S Ii:4:002:1 -115:128 2 < 801f5efb8a00 57706609 C Ii:4:002:1 0:128 1 = 08 801f5efb8a00 57706615 S Ii:4:002:1 -115:128 2 < 801f5efb8a00 57722609 C Ii:4:002:1 0:128 1 = 08 801f5efb8a00 57722614 S Ii:4:002:1 -115:128 2 < 801f5efb8a00 57738611 C Ii:4:002:1 0:128 1 = 08 801f5efb8a00 57738616 S Ii:4:002:1 -115:128 2 < 801f5efb8a00 57754610 C Ii:4:002:1 0:128 1 = 08 801f5efb8a00 57754615 S Ii:4:002:1 -115:128 2 < 801f5efb8a00 57770607 C Ii:4:002:1 0:128 1 = 08 801f5efb8a00 57770612 S Ii:4:002:1 -115:128 2 < 801f5efb8a00 57786609 C Ii:4:002:1 0:128 1 = 08 801f5efb8a00 57786614 S Ii:4:002:1 -115:128 2 < 801f5efb8a00 57802608 C Ii:4:002:1 0:128 1 = 08 801f5efb8a00 57802613 S Ii:4:002:1 -115:128 2 < 801f61285a00 57803198 S Ci:4:002:0 s a3 00 0003 0004 4 < 801f61285a00 57804109 C Ci:4:002:0 0 4 = a0020100 801f61285a00 57804122 S Co:4:002:0 s 23 01 0014 0003 0 801f61285a00 57804539 C Co:4:002:0 0 0 801f61285a00 57804553 S Co:4:002:0 s 23 01 001d 0003 0 801f61285a00 57804876 C Co:4:002:0 0 0 801f61285a00 57804890 S Co:4:002:0 s 23 01 0019 0003 0 801f61285a00 57805185 C Co:4:002:0 0 0 801f61285a00 57805199 S Co:4:002:0 s 23 01 0010 0003 0 801f61285a00 57805735 C Co:4:002:0 0 0 801f61285a00 57805749 S Ci:4:002:0 s a3 00 0003 0004 4 < 801f61285a00 57806491 C Ci:4:002:0 0 4 = a002 801f61285a00 57806506 S Ci:4:002:0 s a3 00 0003 0004 4 < 801f61285a00 57806895 C Ci:4:002:0 0 4 = a002 801f61285a00 57806910 S Ci:4:002:0 s a3
Re: JMS56x not working reliably with uas driver
On Wed, 2016-12-21 at 17:37 +0530, George Cherian wrote: > > On 12/21/2016 05:12 PM, Oliver Neukum wrote: > > On Wed, 2016-12-21 at 17:09 +0530, George Cherian wrote: > >> Hi Oliver, > >> > >> I was working with this JMicron device and using the uas driver. > >> I am seeing the following 2 issues. > >> > >> 1) On connect I see the following messages. > > > > Thanks. Do you want to submit it to Greg? > > The patch is fine. > Yes please!!! So you want me to submit it? It would be your chance to get a patch upstream. > >> 2) On disconnect I am seeing the following issue > >> > >>scsi host4: uas_post_reset: alloc streams error -19 after reset > >>sd 4:0:0:0: [sdb] Synchronizing SCSI cache > >> > >> This is more fatal because after these messages the USB port becomes > >> unusable. Even an lsusb invocation hangs for ever. > > > > Ouch. That points to a logic error. We should not reset if > > a device is gone. > > Could you send dmesg of such a case? > here is the dmesg!! > [ 203.475382] usb 4-1.3: new SuperSpeed USB device number 3 using xhci_hcd > [ 203.496172] usb 4-1.3: New USB device found, idVendor=152d, > idProduct=9561 > [ 203.503037] usb 4-1.3: New USB device strings: Mfr=1, Product=2, > SerialNumber=5 > [ 203.510352] usb 4-1.3: Product: JMS56x Series > [ 203.514698] usb 4-1.3: Manufacturer: JMicron > [ 203.518966] usb 4-1.3: SerialNumber: > [ 203.594383] usbcore: registered new interface driver usb-storage > [ 203.612425] scsi host4: uas > [ 203.615418] usbcore: registered new interface driver uas > [ 203.620979] scsi 4:0:0:0: Direct-Access ST4000NM 0033-9ZM170 > 0001 PQ: 0 ANSI: 6 > [ 203.630240] sd 4:0:0:0: Attached scsi generic sg1 type 0 > [ 203.630382] sd 4:0:0:0: [sdb] 7814037168 512-byte logical blocks: > (4.00 TB/3.63 TiB) > [ 203.631338] sd 4:0:0:0: [sdb] Write Protect is off > [ 203.631342] sd 4:0:0:0: [sdb] Mode Sense: 67 00 10 08 > [ 203.631734] sd 4:0:0:0: [sdb] Write cache: enabled, read cache: > enabled, supports DPO and FUA > [ 203.631899] xhci_hcd :00:11.0: ERROR Transfer event for disabled > endpoint or incorrect stream ring > [ 203.631904] xhci_hcd :00:11.0: @001f610a1c10 > 1b00 03078001 state 14 ep_info 9403 > [ 203.631906] xhci_hcd :00:11.0: No epring > [ 203.674546] sdb: sdb1 > [ 203.676639] sd 4:0:0:0: [sdb] Attached SCSI disk > [ 213.222913] scsi host4: uas_post_reset: alloc streams error -19 after > reset > [ 213.230548] sd 4:0:0:0: [sdb] Synchronizing SCSI cache > > Above is the dmesg without the unusual_uas patch applied. > Do you need me to enable any specific dev_dbg and then the dmesg output? Damn, that is a strange error. Do you know whether it happens on other XHCI controllers? Which controller are you using and can you please also post "lsusb -v"? 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: JMS56x not working reliably with uas driver
Hi, On 21-12-16 13:07, George Cherian wrote: On 12/21/2016 05:12 PM, Oliver Neukum wrote: On Wed, 2016-12-21 at 17:09 +0530, George Cherian wrote: Hi Oliver, I was working with this JMicron device and using the uas driver. I am seeing the following 2 issues. 1) On connect I see the following messages. Thanks. Do you want to submit it to Greg? The patch is fine. Yes please!!! 2) On disconnect I am seeing the following issue scsi host4: uas_post_reset: alloc streams error -19 after reset sd 4:0:0:0: [sdb] Synchronizing SCSI cache This is more fatal because after these messages the USB port becomes unusable. Even an lsusb invocation hangs for ever. Ouch. That points to a logic error. We should not reset if a device is gone. Could you send dmesg of such a case? here is the dmesg!! [ 203.475382] usb 4-1.3: new SuperSpeed USB device number 3 using xhci_hcd [ 203.496172] usb 4-1.3: New USB device found, idVendor=152d, idProduct=9561 [ 203.503037] usb 4-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=5 [ 203.510352] usb 4-1.3: Product: JMS56x Series [ 203.514698] usb 4-1.3: Manufacturer: JMicron [ 203.518966] usb 4-1.3: SerialNumber: [ 203.594383] usbcore: registered new interface driver usb-storage [ 203.612425] scsi host4: uas [ 203.615418] usbcore: registered new interface driver uas [ 203.620979] scsi 4:0:0:0: Direct-Access ST4000NM 0033-9ZM170 0001 PQ: 0 ANSI: 6 [ 203.630240] sd 4:0:0:0: Attached scsi generic sg1 type 0 [ 203.630382] sd 4:0:0:0: [sdb] 7814037168 512-byte logical blocks: (4.00 TB/3.63 TiB) [ 203.631338] sd 4:0:0:0: [sdb] Write Protect is off [ 203.631342] sd 4:0:0:0: [sdb] Mode Sense: 67 00 10 08 [ 203.631734] sd 4:0:0:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA [ 203.631899] xhci_hcd :00:11.0: ERROR Transfer event for disabled endpoint or incorrect stream ring [ 203.631904] xhci_hcd :00:11.0: @001f610a1c10 1b00 03078001 state 14 ep_info 9403 [ 203.631906] xhci_hcd :00:11.0: No epring [ 203.674546] sdb: sdb1 [ 203.676639] sd 4:0:0:0: [sdb] Attached SCSI disk [ 213.222913] scsi host4: uas_post_reset: alloc streams error -19 after reset [ 213.230548] sd 4:0:0:0: [sdb] Synchronizing SCSI cache Above is the dmesg without the unusual_uas patch applied. Do you need me to enable any specific dev_dbg and then the dmesg output? Can you get us a dmesg with the unusual_uas patch applied? Usually once things go foobar because of issue-ing a command which the device does not understand, more things typically come down as both the device and the host may be in an undefined state then. 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 2/2] usb: host: xhci: Handle the right timeout command
On 21.12.2016 08:17, Lu Baolu wrote:
Hi Mathias,
I have some comments for the implementation of xhci_abort_cmd_ring() below.
On 12/20/2016 11:13 PM, Mathias Nyman wrote:
On 20.12.2016 09:30, Baolin Wang wrote:
...
Alright, I gathered all current work related to xhci races and timeouts
and put them into a branch:
git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git timeout_race_fixes
Its based on 4.9
It includes a few other patches just to avoid conflicts and make my life easier
Interesting patches are:
ee4eb91 xhci: remove unnecessary check for pending timer
0cba67d xhci: detect stop endpoint race using pending timer instead of counter.
4f2535f xhci: Handle command completion and timeout race
b9d00d7 usb: host: xhci: Fix possible wild pointer when handling abort command
529a5a0 usb: xhci: fix possible wild pointer
4766555 xhci: Fix race related to abort operation
de834a3 xhci: Use delayed_work instead of timer for command timeout
69973b8 Linux 4.9
The fixes for command queue races will go to usb-linus and stable, the
reworks for stop ep watchdog timer will go to usb-next.
Still completely untested, (well it compiles)
Felipe gave instructions how to modify dwc3 driver to timeout on address
devicecommands to test these, I'll try to set that up.
All additional testing is welcome, especially if you can trigger timeouts
and races
-Mathias
Below is the latest code. I put my comments in line.
322 static int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
323 {
324 u64 temp_64;
325 int ret;
326
327 xhci_dbg(xhci, "Abort command ring\n");
328
329 reinit_completion(&xhci->cmd_ring_stop_completion);
330
331 temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
332 xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
333 &xhci->op_regs->cmd_ring);
We should hold xhci->lock when we are modifying xhci registers
at runtime.
Makes sense, but we need to unlock it before sleeping or waiting for completion.
I need to look into that in more detail.
But this was an issue already before these changes.
The retry of setting CMD_RING_ABORT is not necessary according to
previous discussion. We have cleaned code for second try in
xhci_handle_command_timeout(). Need to clean up here as well.
Yes it can be cleaned up as well, but the two cases are a bit different.
The cleaned up one was about command ring not starting again after it was
stopped.
This second try is a workaround for what we thought was the command ring failing
to stop in the first place, but is most likely due to the race that OGAWA
Hirofumi
fixed. It races if the stop command ring interrupt happens between writing the
abort
bit and polling for the ring stopped bit. The interrupt hander may start the
command
ring again, and we would believe we failed to stop it in the first place.
This race could probably be fixed by just extending the lock (and preventing
interrupts) to cover both writing the abort bit and polling for the command ring
running bit, as you pointed out here previously.
But then again I really like OGAWA Hiroumi's solution that separates the
command ring stopping from aborting commands and restarting the ring.
The current way of always restarting the command ring as a response to
a stop command ring command really limits its usage.
So, with this in mind most reasonable would be to
1. fix the lock to cover abort+CRR check, and send it to usb-linus +stable
2. rebase OGAWA Hirofumi's changes on top of that, and send to usb-linus only
3. remove unnecessary second abort try as a separate patch, send to usb-next
4. remove polling for the Command ring running (CRR), waiting for completion
is enough, if completion times out then we can check CRR. for usb-next
I'll fix the typos these patches would introduce. Fixing old typos can be done as separate
patches later.
-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: JMS56x not working reliably with uas driver
On 12/21/2016 05:50 PM, Hans de Goede wrote: Hi, On 21-12-16 13:07, George Cherian wrote: On 12/21/2016 05:12 PM, Oliver Neukum wrote: On Wed, 2016-12-21 at 17:09 +0530, George Cherian wrote: Hi Oliver, I was working with this JMicron device and using the uas driver. I am seeing the following 2 issues. 1) On connect I see the following messages. Thanks. Do you want to submit it to Greg? The patch is fine. Yes please!!! 2) On disconnect I am seeing the following issue scsi host4: uas_post_reset: alloc streams error -19 after reset sd 4:0:0:0: [sdb] Synchronizing SCSI cache This is more fatal because after these messages the USB port becomes unusable. Even an lsusb invocation hangs for ever. Ouch. That points to a logic error. We should not reset if a device is gone. Could you send dmesg of such a case? here is the dmesg!! [ 203.475382] usb 4-1.3: new SuperSpeed USB device number 3 using xhci_hcd [ 203.496172] usb 4-1.3: New USB device found, idVendor=152d, idProduct=9561 [ 203.503037] usb 4-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=5 [ 203.510352] usb 4-1.3: Product: JMS56x Series [ 203.514698] usb 4-1.3: Manufacturer: JMicron [ 203.518966] usb 4-1.3: SerialNumber: [ 203.594383] usbcore: registered new interface driver usb-storage [ 203.612425] scsi host4: uas [ 203.615418] usbcore: registered new interface driver uas [ 203.620979] scsi 4:0:0:0: Direct-Access ST4000NM 0033-9ZM170 0001 PQ: 0 ANSI: 6 [ 203.630240] sd 4:0:0:0: Attached scsi generic sg1 type 0 [ 203.630382] sd 4:0:0:0: [sdb] 7814037168 512-byte logical blocks: (4.00 TB/3.63 TiB) [ 203.631338] sd 4:0:0:0: [sdb] Write Protect is off [ 203.631342] sd 4:0:0:0: [sdb] Mode Sense: 67 00 10 08 [ 203.631734] sd 4:0:0:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA [ 203.631899] xhci_hcd :00:11.0: ERROR Transfer event for disabled endpoint or incorrect stream ring [ 203.631904] xhci_hcd :00:11.0: @001f610a1c10 1b00 03078001 state 14 ep_info 9403 [ 203.631906] xhci_hcd :00:11.0: No epring [ 203.674546] sdb: sdb1 [ 203.676639] sd 4:0:0:0: [sdb] Attached SCSI disk [ 213.222913] scsi host4: uas_post_reset: alloc streams error -19 after reset [ 213.230548] sd 4:0:0:0: [sdb] Synchronizing SCSI cache Above is the dmesg without the unusual_uas patch applied. Do you need me to enable any specific dev_dbg and then the dmesg output? Can you get us a dmesg with the unusual_uas patch applied? Usually once things go foobar because of issue-ing a command which the device does not understand, more things typically come down as both the device and the host may be in an undefined state then. Here is the lsusb -t lsusb -t /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/1p, 5000M |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M |__ Port 3: Dev 4, If 0, Class=Mass Storage, Driver=uas, 5000M /: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/1p, 480M |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/1p, 5000M /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/1p, 480M lsusb Bus 003 Device 002: ID 0bda:5401 Realtek Semiconductor Corp. RTL 8153 USB 3.0 hub with gigabit ethernet Bus 004 Device 002: ID 0bda:0401 Realtek Semiconductor Corp. Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub Bus 004 Device 004: ID 152d:9561 JMicron Technology Corp. / JMicron USA Technology Corp. lsusb -v Bus 004 Device 004: ID 152d:9561 JMicron Technology Corp. / JMicron USA Technology Corp. Device Descriptor: bLength18 bDescriptorType 1 bcdUSB 3.00 bDeviceClass0 (Defined at Interface level) bDeviceSubClass 0 bDeviceProtocol 0 bMaxPacketSize0 9 idVendor 0x152d JMicron Technology Corp. / JMicron USA Technology Corp. idProduct 0x9561 bcdDevice0.01 iManufacturer 1 JMicron iProduct2 JMS56x Series iSerial 5 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 121 bNumInterfaces 1 bConfigurationValue 1 iConfiguration 4 USB Mass Storage bmAttributes 0xc0 Self Powered MaxPower2mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber0 bAlternateSetting 0 bNumEndpoints 2 bInterfaceClass 8 Mass Storage
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
On 21.12.2016 08:57, Lu Baolu wrote:
Hi Mathias,
I have some comments for the implementation of
xhci_handle_command_timeout() as well.
On 12/20/2016 11:13 PM, Mathias Nyman wrote:
On 20.12.2016 09:30, Baolin Wang wrote:
...
Alright, I gathered all current work related to xhci races and timeouts
and put them into a branch:
git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git timeout_race_fixes
Its based on 4.9
It includes a few other patches just to avoid conflicts and make my life easier
Interesting patches are:
ee4eb91 xhci: remove unnecessary check for pending timer
0cba67d xhci: detect stop endpoint race using pending timer instead of counter.
4f2535f xhci: Handle command completion and timeout race
b9d00d7 usb: host: xhci: Fix possible wild pointer when handling abort command
529a5a0 usb: xhci: fix possible wild pointer
4766555 xhci: Fix race related to abort operation
de834a3 xhci: Use delayed_work instead of timer for command timeout
69973b8 Linux 4.9
The fixes for command queue races will go to usb-linus and stable, the
reworks for stop ep watchdog timer will go to usb-next.
Still completely untested, (well it compiles)
Felipe gave instructions how to modify dwc3 driver to timeout on address
devicecommands to test these, I'll try to set that up.
All additional testing is welcome, especially if you can trigger timeouts
and races
-Mathias
I post the code below and add my comments in line.
1276 void xhci_handle_command_timeout(struct work_struct *work)
1277 {
1278 struct xhci_hcd *xhci;
1279 int ret;
1280 unsigned long flags;
1281 u64 hw_ring_state;
1282
1283 xhci = container_of(to_delayed_work(work), struct xhci_hcd,
cmd_timer);
1284
1285 spin_lock_irqsave(&xhci->lock, flags);
1286
1287 /*
1288 * If timeout work is pending, or current_cmd is NULL, it means we
1289 * raced with command completion. Command is handled so just
return.
1290 */
1291 if (!xhci->current_cmd || delayed_work_pending(&xhci->cmd_timer)) {
1292 spin_unlock_irqrestore(&xhci->lock, flags);
1293 return;
1294 }
1295 /* mark this command to be cancelled */
1296 xhci->current_cmd->status = COMP_CMD_ABORT;
1297
1298 /* Make sure command ring is running before aborting it */
1299 hw_ring_state = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
1300 if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) &&
1301 (hw_ring_state & CMD_RING_RUNNING)) {
1302 /* Prevent new doorbell, and start command abort */
1303 xhci->cmd_ring_state = CMD_RING_STATE_ABORTED;
1304 spin_unlock_irqrestore(&xhci->lock, flags);
1305 xhci_dbg(xhci, "Command timeout\n");
1306 ret = xhci_abort_cmd_ring(xhci);
1307 if (unlikely(ret == -ESHUTDOWN)) {
1308 xhci_err(xhci, "Abort command ring failed\n");
1309 xhci_cleanup_command_queue(xhci);
1310 usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
1311 xhci_dbg(xhci, "xHCI host controller is dead.\n");
1312 }
1313 return;
1314 }
1315
1316 /* host removed. Bail out */
1317 if (xhci->xhc_state & XHCI_STATE_REMOVING) {
1318 spin_unlock_irqrestore(&xhci->lock, flags);
1319 xhci_dbg(xhci, "host removed, ring start fail?\n");
1320 xhci_cleanup_command_queue(xhci);
1321 return;
1322 }
I think this part of code should be moved up to line 1295.
The XHCI_STATE_REMOVING and XHCI_STATE_DYING needs a rework,
I'm working on that.
Basically we want XHCI_STATE_REMOVING to mean that all devices are going,
away and driver will be removed. Don't bother with re-calculating available
bandwidths after every device removal, but do use xhci hardware to disable
devices cleanly etc.
XHCI_STATE_DYING should mean hardware is not working/responding. Don't
bother writing any registers or queuing anything. Just return all
pending and cancelled URBs, notify core we died, and free all allocated memory.
1323
1324 /* command timeout on stopped ring, ring can't be aborted */
1325 xhci_dbg(xhci, "Command timeout on stopped ring\n");
1326 xhci_handle_stopped_cmd_ring(xhci, xhci->current_cmd);
1327 spin_unlock_irqrestore(&xhci->lock, flags);
This part of code is tricky. I have no idea about in which case should this
code be executed? Anyway, we shouldn't call xhci_handle_stopped_cmd_ring()
here, right?
This isn't changed it these patches.
It will remove the aborted commands and restart the ring. It's useful if we
want to abort a command but command ring was not running. (if for some
unkown reason it was stopped, or forgot to restart.
-Mathias
--
To unsubscribe from this list: send the line "unsubsc
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
On 21.12.2016 04:22, Baolin Wang wrote: Hi Mathias, On 20 December 2016 at 23:13, Mathias Nyman wrote: On 20.12.2016 09:30, Baolin Wang wrote: ... Alright, I gathered all current work related to xhci races and timeouts and put them into a branch: git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git timeout_race_fixes Its based on 4.9 It includes a few other patches just to avoid conflicts and make my life easier Interesting patches are: ee4eb91 xhci: remove unnecessary check for pending timer 0cba67d xhci: detect stop endpoint race using pending timer instead of counter. 4f2535f xhci: Handle command completion and timeout race b9d00d7 usb: host: xhci: Fix possible wild pointer when handling abort command 529a5a0 usb: xhci: fix possible wild pointer 4766555 xhci: Fix race related to abort operation de834a3 xhci: Use delayed_work instead of timer for command timeout 69973b8 Linux 4.9 The fixes for command queue races will go to usb-linus and stable, the reworks for stop ep watchdog timer will go to usb-next. How about applying below patch in your 'timeout_race_fixes' branch? [PATCH] usb: host: xhci: Clean up commands when stop endpoint command is timeout https://lkml.org/lkml/2016/12/13/94 usb_hc_died() should eventyally end up calling xhci_mem_cleanup() which will cleanup the command queue. But I need to look into that -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] usbip: vudc: check for NULL before use
On Tue, Dec 20, 2016 at 07:31:44AM -0700, Shuah Khan wrote: > On 12/18/2016 03:44 PM, Sudip Mukherjee wrote: > > to_vep() is doing a container_of() on _ep. It is better to do the NULL > > check first and then use it. > > > > Signed-off-by: Sudip Mukherjee > > --- > > drivers/usb/usbip/vudc_dev.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c > > index 968471b..32ea604 100644 > > --- a/drivers/usb/usbip/vudc_dev.c > > +++ b/drivers/usb/usbip/vudc_dev.c > > @@ -388,10 +388,10 @@ static int vep_dequeue(struct usb_ep *_ep, struct > > usb_request *_req) > > unsigned long flags; > > int ret = 0; > > > > - ep = to_vep(_ep); > > if (!_ep) > > return -EINVAL; > > Hmm. Linus's latest checks _ep and _req. Are you sure you are working > with the latest tree? I checked with next-20161221 and its still there. 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: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Mathias Nyman writes:
>> Below is the latest code. I put my comments in line.
>>
>> 322 static int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
>> 323 {
>> 324 u64 temp_64;
>> 325 int ret;
>> 326
>> 327 xhci_dbg(xhci, "Abort command ring\n");
>> 328
>> 329 reinit_completion(&xhci->cmd_ring_stop_completion);
>> 330
>> 331 temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
>> 332 xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
>> 333 &xhci->op_regs->cmd_ring);
>>
>> We should hold xhci->lock when we are modifying xhci registers
>> at runtime.
>>
>
> Makes sense, but we need to unlock it before sleeping or waiting for
> completion. I need to look into that in more detail.
>
> But this was an issue already before these changes.
We set CMD_RING_STATE_ABORTED state under locking. I'm not checking what
is for taking lock for register though, I guess it should be enough just
lock around of read=>write of ->cmd_ring if need lock.
[Rather ->cmd_ring user should check CMD_RING_STATE_ABORTED state.]
> But then again I really like OGAWA Hiroumi's solution that separates the
> command ring stopping from aborting commands and restarting the ring.
>
> The current way of always restarting the command ring as a response to
> a stop command ring command really limits its usage.
>
> So, with this in mind most reasonable would be to
> 1. fix the lock to cover abort+CRR check, and send it to usb-linus +stable
> 2. rebase OGAWA Hirofumi's changes on top of that, and send to usb-linus only
> 3. remove unnecessary second abort try as a separate patch, send to usb-next
> 4. remove polling for the Command ring running (CRR), waiting for completion
> is enough, if completion times out then we can check CRR. for usb-next
I think we should check both of CRR and even of stop completion. Because
CRR and stop completion was not same time (both can be first).
Thanks.
--
OGAWA Hirofumi
--
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] usbip: vudc: check for NULL before use
Hi Sudip,
On Wed, Dec 21, 2016 at 6:33 AM, Sudip Mukherjee
wrote:
> On Tue, Dec 20, 2016 at 07:31:44AM -0700, Shuah Khan wrote:
>> On 12/18/2016 03:44 PM, Sudip Mukherjee wrote:
>> > to_vep() is doing a container_of() on _ep. It is better to do the NULL
>> > check first and then use it.
>> >
>> > Signed-off-by: Sudip Mukherjee
>> > ---
>> > drivers/usb/usbip/vudc_dev.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
>> > index 968471b..32ea604 100644
>> > --- a/drivers/usb/usbip/vudc_dev.c
>> > +++ b/drivers/usb/usbip/vudc_dev.c
>> > @@ -388,10 +388,10 @@ static int vep_dequeue(struct usb_ep *_ep, struct
>> > usb_request *_req)
>> > unsigned long flags;
>> > int ret = 0;
>> >
>> > - ep = to_vep(_ep);
>> > if (!_ep)
>> > return -EINVAL;
>>
>> Hmm. Linus's latest checks _ep and _req. Are you sure you are working
>> with the latest tree?
>
> I checked with next-20161221 and its still there.
This is for vep_dequeue() - Are you sure both linux-next and Linus's tree show
the following:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/usbip/vudc_dev.c
static int vep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct vep *ep;
struct vrequest *req;
struct vudc *udc;
struct vrequest *lst;
unsigned long flags;
int ret = -EINVAL;
if (!_ep || !_req)
return ret;
ep = to_vep(_ep);
req = to_vrequest(_req);
udc = req->udc;
if (!udc->driver)
return -ESHUTDOWN;
There are other routines in this file that don't check null. I am confused.
thanks,
-- Shuah
--
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: JMS56x not working reliably with uas driver
On 12/21/2016 05:12 PM, Oliver Neukum wrote: On Wed, 2016-12-21 at 17:09 +0530, George Cherian wrote: Hi Oliver, I was working with this JMicron device and using the uas driver. I am seeing the following 2 issues. 1) On connect I see the following messages. Thanks. Do you want to submit it to Greg? The patch is fine. Yes please!!! 2) On disconnect I am seeing the following issue scsi host4: uas_post_reset: alloc streams error -19 after reset sd 4:0:0:0: [sdb] Synchronizing SCSI cache This is more fatal because after these messages the USB port becomes unusable. Even an lsusb invocation hangs for ever. Ouch. That points to a logic error. We should not reset if a device is gone. Could you send dmesg of such a case? here is the dmesg!! [ 203.475382] usb 4-1.3: new SuperSpeed USB device number 3 using xhci_hcd [ 203.496172] usb 4-1.3: New USB device found, idVendor=152d, idProduct=9561 [ 203.503037] usb 4-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=5 [ 203.510352] usb 4-1.3: Product: JMS56x Series [ 203.514698] usb 4-1.3: Manufacturer: JMicron [ 203.518966] usb 4-1.3: SerialNumber: [ 203.594383] usbcore: registered new interface driver usb-storage [ 203.612425] scsi host4: uas [ 203.615418] usbcore: registered new interface driver uas [ 203.620979] scsi 4:0:0:0: Direct-Access ST4000NM 0033-9ZM170 0001 PQ: 0 ANSI: 6 [ 203.630240] sd 4:0:0:0: Attached scsi generic sg1 type 0 [ 203.630382] sd 4:0:0:0: [sdb] 7814037168 512-byte logical blocks: (4.00 TB/3.63 TiB) [ 203.631338] sd 4:0:0:0: [sdb] Write Protect is off [ 203.631342] sd 4:0:0:0: [sdb] Mode Sense: 67 00 10 08 [ 203.631734] sd 4:0:0:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA [ 203.631899] xhci_hcd :00:11.0: ERROR Transfer event for disabled endpoint or incorrect stream ring [ 203.631904] xhci_hcd :00:11.0: @001f610a1c10 1b00 03078001 state 14 ep_info 9403 [ 203.631906] xhci_hcd :00:11.0: No epring [ 203.674546] sdb: sdb1 [ 203.676639] sd 4:0:0:0: [sdb] Attached SCSI disk [ 213.222913] scsi host4: uas_post_reset: alloc streams error -19 after reset [ 213.230548] sd 4:0:0:0: [sdb] Synchronizing SCSI cache Above is the dmesg without the unusual_uas patch applied. Do you need me to enable any specific dev_dbg and then the dmesg output? 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: JMS56x not working reliably with uas driver
On Wed, 2016-12-21 at 18:17 +0530, George Cherian wrote:
> [ 843.149653] scsi host5: uas_post_reset: alloc streams error -19
> after
> reset
That would mean the endpoints are gone. Which is odd.
> [ 843.157268] sd 5:0:0:0: [sdb] Synchronizing SCSI cache
Could you try the attached patch and do a SCSI log of the enumeration?
Regards
Oliver
From d4ddac88bbf9cb15e7d8638582f96d31e245f15b Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Wed, 21 Dec 2016 15:34:54 +0100
Subject: [PATCH] uas: device crashes on reset
We avoid resetting it.
Signed-off-by: Oliver Neukum
---
drivers/usb/core/quirks.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index d2e50a2..52483fb 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -194,6 +194,8 @@ static const struct usb_device_id usb_quirk_list[] = {
{ USB_DEVICE(0x1532, 0x0116), .driver_info =
USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL },
+ { USB_DEVICE(0x152d, 0x9561), .driver_info = USB_QUIRK_RESET },
+
/* BUILDWIN Photo Frame */
{ USB_DEVICE(0x1908, 0x1315), .driver_info =
USB_QUIRK_HONOR_BNUMINTERFACES },
--
2.1.4
Re: [PATCH] usbip: vudc: check for NULL before use
Hi,
On 12/21/2016 03:38 PM, Shuah Khan wrote:
> Hi Sudip,
>
> On Wed, Dec 21, 2016 at 6:33 AM, Sudip Mukherjee
> wrote:
>> On Tue, Dec 20, 2016 at 07:31:44AM -0700, Shuah Khan wrote:
>>> On 12/18/2016 03:44 PM, Sudip Mukherjee wrote:
>>>> to_vep() is doing a container_of() on _ep. It is better to do the NULL
>>>> check first and then use it.
>>>>
>>>> Signed-off-by: Sudip Mukherjee
>>>> ---
>>>> drivers/usb/usbip/vudc_dev.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
>>>> index 968471b..32ea604 100644
>>>> --- a/drivers/usb/usbip/vudc_dev.c
>>>> +++ b/drivers/usb/usbip/vudc_dev.c
>>>> @@ -388,10 +388,10 @@ static int vep_dequeue(struct usb_ep *_ep, struct
>>>> usb_request *_req)
>>>> unsigned long flags;
>>>> int ret = 0;
>>>>
>>>> - ep = to_vep(_ep);
>>>> if (!_ep)
>>>> return -EINVAL;
>>>
>>> Hmm. Linus's latest checks _ep and _req. Are you sure you are working
>>> with the latest tree?
>>
>> I checked with next-20161221 and its still there.
>
> This is for vep_dequeue() - Are you sure both linux-next and Linus's tree
> show
> the following:
>
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/usbip/vudc_dev.c
>
> static int vep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> {
> struct vep *ep;
> struct vrequest *req;
> struct vudc *udc;
> struct vrequest *lst;
> unsigned long flags;
> int ret = -EINVAL;
>
> if (!_ep || !_req)
> return ret;
>
> ep = to_vep(_ep);
> req = to_vrequest(_req);
> udc = req->udc;
>
> if (!udc->driver)
> return -ESHUTDOWN;
>
> There are other routines in this file that don't check null. I am confused.
>
generally this file contains implementation of gadget/endpoint
callbacks. Those methods will be called by udc-core with values passed
from USB gadget (usually directly from function).
I check other udc in kernel and there is no agreement if we should check
in those callbacks if our params are NULL or not.
I have also run through udc-core implementation and generally it doesn't
check if params are NULL or not and just dereference some of them and
pass them to our callbacks.
I think that the best option is just to ask Felipe (USB gadget
maintainer) if we should check this or not.
@Felipe
Should each UDC check if values passed to gadget/endpoint callbacks is
not NULL or just simply dereference them?
Best regards,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
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 2/2] usb: host: xhci: Handle the right timeout command
On 21.12.2016 16:10, OGAWA Hirofumi wrote:
Mathias Nyman writes:
Below is the latest code. I put my comments in line.
322 static int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
323 {
324 u64 temp_64;
325 int ret;
326
327 xhci_dbg(xhci, "Abort command ring\n");
328
329 reinit_completion(&xhci->cmd_ring_stop_completion);
330
331 temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
332 xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
333 &xhci->op_regs->cmd_ring);
We should hold xhci->lock when we are modifying xhci registers
at runtime.
Makes sense, but we need to unlock it before sleeping or waiting for
completion. I need to look into that in more detail.
But this was an issue already before these changes.
We set CMD_RING_STATE_ABORTED state under locking. I'm not checking what
is for taking lock for register though, I guess it should be enough just
lock around of read=>write of ->cmd_ring if need lock.
After your patch it should be enough to have the lock only while
reading and writing the cmd_ring register.
If we want a locking fix that applies more easily to older stable releases
before
your change then the lock needs to cover set CMD_RING_STATE_ABORT, read cmd_reg,
write cmd_reg and busiloop checking CRR bit. Otherwise the stop cmd ring
interrupt
handler may restart the ring just before we start checing CRR. The stop cmd ring
interrupt will set the CMD_RING_STATE_ABORTED to CMD_RING_STATE_RUNNING so
ring will really restart in the interrupt handler.
[Rather ->cmd_ring user should check CMD_RING_STATE_ABORTED state.]
But then again I really like OGAWA Hiroumi's solution that separates the
command ring stopping from aborting commands and restarting the ring.
The current way of always restarting the command ring as a response to
a stop command ring command really limits its usage.
So, with this in mind most reasonable would be to
1. fix the lock to cover abort+CRR check, and send it to usb-linus +stable
2. rebase OGAWA Hirofumi's changes on top of that, and send to usb-linus only
3. remove unnecessary second abort try as a separate patch, send to usb-next
4. remove polling for the Command ring running (CRR), waiting for completion
is enough, if completion times out then we can check CRR. for usb-next
I think we should check both of CRR and even of stop completion. Because
CRR and stop completion was not same time (both can be first).
We can keep both, maybe change the order and do the busylooping-checking after
waiting for completion, but thats a optimization for usb-next sometimes later.
Thanks
-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 2/2] usb: host: xhci: Handle the right timeout command
Mathias Nyman writes: >> We set CMD_RING_STATE_ABORTED state under locking. I'm not checking what >> is for taking lock for register though, I guess it should be enough just >> lock around of read=>write of ->cmd_ring if need lock. > > After your patch it should be enough to have the lock only while > reading and writing the cmd_ring register. > > If we want a locking fix that applies more easily to older stable > releases before your change then the lock needs to cover set > CMD_RING_STATE_ABORT, read cmd_reg, write cmd_reg and busiloop > checking CRR bit. Otherwise the stop cmd ring interrupt handler may > restart the ring just before we start checing CRR. The stop cmd ring > interrupt will set the CMD_RING_STATE_ABORTED to > CMD_RING_STATE_RUNNING so ring will really restart in the interrupt > handler. Just for record (no chance to make patch I myself for now, sorry), while checking locking slightly, I noticed unrelated missing locking. xhci_cleanup_command_queue() We are calling it without locking, but we need to lock for accessing list. Thanks. -- OGAWA Hirofumi -- 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] usbip: vudc: check for NULL before use
On Wed, Dec 21, 2016 at 07:38:21AM -0700, Shuah Khan wrote: > Hi Sudip, > > On Wed, Dec 21, 2016 at 6:33 AM, Sudip Mukherjee > wrote: > > On Tue, Dec 20, 2016 at 07:31:44AM -0700, Shuah Khan wrote: > >> On 12/18/2016 03:44 PM, Sudip Mukherjee wrote: > >> > to_vep() is doing a container_of() on _ep. It is better to do the NULL > >> > check first and then use it. > >> > > >> > Signed-off-by: Sudip Mukherjee > >> > --- > >> > drivers/usb/usbip/vudc_dev.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c > >> > index 968471b..32ea604 100644 > >> > --- a/drivers/usb/usbip/vudc_dev.c > >> > +++ b/drivers/usb/usbip/vudc_dev.c > >> > @@ -388,10 +388,10 @@ static int vep_dequeue(struct usb_ep *_ep, struct > >> > usb_request *_req) > >> > unsigned long flags; > >> > int ret = 0; > >> > > >> > - ep = to_vep(_ep); > >> > if (!_ep) > >> > return -EINVAL; > >> > >> Hmm. Linus's latest checks _ep and _req. Are you sure you are working > >> with the latest tree? > > > > I checked with next-20161221 and its still there. > > This is for vep_dequeue() - Are you sure both linux-next and Linus's tree > show > the following: This is for vep_set_halt_and_wedge(). I do not have any idea why the patch says its vep_dequeue(). I tried generating the patch again and it still shows as vep_dequeue(). But the line number 388 is correct and if you try to apply, it applies correctly. 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: [PATCH v2 1/1] Increase USB transfer limit
Hi Greg, On 15 Dec 2016, at 11:07, Greg KH wrote: [..] Sorry, I wasn't quite sure how handle this. Documentation/SubmittingPatches should describe how to do this. Please do so on your next submission so I know what patch to apply. Ok, thanks for clarification. I’ll follow this next time. With regard to this patch, would you like me to edit it and resubmit with corrected version or any other steps for me to follow or are you just waiting for merge window to open? Thank you, Mateusz Berezecki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] Increase USB transfer limit
On Wed, Dec 21, 2016 at 08:44:30AM -0800, Mateusz Berezecki wrote: > Hi Greg, > > On 15 Dec 2016, at 11:07, Greg KH wrote: > > [..] > > > > > > > Sorry, I wasn't quite sure how handle this. > > > > Documentation/SubmittingPatches should describe how to do this. Please > > do so on your next submission so I know what patch to apply. > > > Ok, thanks for clarification. I’ll follow this next time. With regard to > this patch, would you like me to edit it and resubmit with corrected version > or any other steps for me to follow or are you just waiting for merge window > to open? I am waiting for it to open, but if you want to resend with the latest correct version, that would help me out a lot. 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
[PATCH v3 1/1] Increase USB transfer limit
Promote a variable keeping track of USB transfer memory usage to a
wider data type and allow for higher bandwidth transfers from a large
number of USB devices connected to a single host.
Signed-off-by: Mateusz Berezecki
---
drivers/usb/core/devio.c | 43 ---
1 file changed, 16 insertions(+), 27 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 4016dae..52747b6 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -134,42 +134,35 @@ enum snoop_when {
#define USB_DEVICE_DEV MKDEV(USB_DEVICE_MAJOR, 0)
/* Limit on the total amount of memory we can allocate for transfers */
-static unsigned usbfs_memory_mb = 16;
+static u32 usbfs_memory_mb = 16;
module_param(usbfs_memory_mb, uint, 0644);
MODULE_PARM_DESC(usbfs_memory_mb,
"maximum MB allowed for usbfs buffers (0 = no limit)");
-/* Hard limit, necessary to avoid arithmetic overflow */
-#define USBFS_XFER_MAX (UINT_MAX / 2 - 100)
-
-static atomic_t usbfs_memory_usage;/* Total memory currently allocated */
+static atomic64_t usbfs_memory_usage; /* Total memory currently allocated */
/* Check whether it's okay to allocate more memory for a transfer */
-static int usbfs_increase_memory_usage(unsigned amount)
+static int usbfs_increase_memory_usage(u64 amount)
{
- unsigned lim;
+ u64 lim;
- /*
-* Convert usbfs_memory_mb to bytes, avoiding overflows.
-* 0 means use the hard limit (effectively unlimited).
-*/
lim = ACCESS_ONCE(usbfs_memory_mb);
- if (lim == 0 || lim > (USBFS_XFER_MAX >> 20))
- lim = USBFS_XFER_MAX;
- else
- lim <<= 20;
+ lim <<= 20;
- atomic_add(amount, &usbfs_memory_usage);
- if (atomic_read(&usbfs_memory_usage) <= lim)
- return 0;
- atomic_sub(amount, &usbfs_memory_usage);
- return -ENOMEM;
+ atomic64_add(amount, &usbfs_memory_usage);
+
+ if (lim > 0 && atomic64_read(&usbfs_memory_usage) > lim) {
+ atomic64_sub(amount, &usbfs_memory_usage);
+ return -ENOMEM;
+ }
+
+ return 0;
}
/* Memory for a transfer is being deallocated */
-static void usbfs_decrease_memory_usage(unsigned amount)
+static void usbfs_decrease_memory_usage(u64 amount)
{
- atomic_sub(amount, &usbfs_memory_usage);
+ atomic64_sub(amount, &usbfs_memory_usage);
}
static int connected(struct usb_dev_state *ps)
@@ -1191,7 +1184,7 @@ static int proc_bulk(struct usb_dev_state *ps, void
__user *arg)
if (!usb_maxpacket(dev, pipe, !(bulk.ep & USB_DIR_IN)))
return -EINVAL;
len1 = bulk.len;
- if (len1 >= USBFS_XFER_MAX)
+ if (len1 >= (INT_MAX - sizeof(struct urb)))
return -EINVAL;
ret = usbfs_increase_memory_usage(len1 + sizeof(struct urb));
if (ret)
@@ -1584,10 +1577,6 @@ static int proc_do_submiturb(struct usb_dev_state *ps,
struct usbdevfs_urb *uurb
return -EINVAL;
}
- if (uurb->buffer_length >= USBFS_XFER_MAX) {
- ret = -EINVAL;
- goto error;
- }
if (uurb->buffer_length > 0 &&
!access_ok(is_in ? VERIFY_WRITE : VERIFY_READ,
uurb->buffer, uurb->buffer_length)) {
--
2.7.4
This is a re-send of a previously sent patch, acked-by Alan Stern. This
version uses a bumped up patch version number to ease the identification
of the patch, as previously, I sent multiple versions with the same number.
--
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
Using multiple USB3380 PCIe cards on a single Linux system
Hi Linux-USB members, I currently have a Intel Xeon based Dell poweredge T320 server setup running Ubuntu server 14.04. I would like to add multiple USB3 devices (peripheral mode) to the system using multiple PLX based USB3380 PCIe cards. I would like to associate USB mass storage gadget with all the multiple USB3 devices. Before I go ahead and buy multiple USB3380 PCIe cards, I would like to know if the net2280/usb3380 driver + usb mass storage gadget driver supports multiple USB3 devices operation at the same time? Thanks in advance for your time! Thanks, Karthik. -- 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 v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Wed, Dec 21 2016, Baolin Wang wrote:
> On 21 December 2016 at 11:48, NeilBrown wrote:
>> On Wed, Dec 21 2016, Baolin Wang wrote:
>>
>>> Hi,
>>>
>>> On 21 December 2016 at 06:07, NeilBrown wrote:
On Tue, Dec 20 2016, Baolin Wang wrote:
> Hi Neil,
>
> On 3 November 2016 at 09:25, NeilBrown wrote:
>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>
>>
So I won't be responding on this topic any further until I see a
genuine
attempt to understand and resolve the inconsistencies with
usb_register_notifier().
>>>
>>> Any better solution?
>>
>> I'm not sure exactly what you are asking, so I'll assume you are asking
>> the question I want to answer :-)
>>
>> 1/ Liase with the extcon developers to resolve the inconsistencies
>> with USB connector types.
>> e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>> which both seem to suggest a standard downstream port. There is no
>> documentation describing how these relate, and no consistent practice
>> to copy.
>> I suspect the intention is that
>> EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>> the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>> the cable.
>> So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>> while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>> would normally appear with EXTCON_USB_HOST (I think).
>> Some drivers follow this model, particularly extcon-max14577.c
>> but it is not consistent.
>>
>> This policy should be well documented and possibly existing drivers
>> should be updated to follow it.
>>
>> At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>> and EXTCON_CHG_USB_FAST. These names don't mean much.
>> They were recently removed from drivers/power/axp288_charger.c
>> which is good, but are still used in drivers/extcon/extcon-max*
>> Possibly they should be changed to names from the standard, or
>> possibly they should be renamed to identify the current they are
>> expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>
> Now I am creating the new patchset with fixing and converting exist
> drivers.
Thanks!
>
> I did some investigation about EXTCON subsystem. From your suggestion:
> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
> After checking, now all extcon drivers were following this rule.
what about extcon-axp288.c ?
axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
never sets EXTCON_USB.
Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.
>>>
>>> Ha, sorry, I missed these 2 files, and I will fix them.
>>>
>
> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
> Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
> change.
Agreed.
>
> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
> There are no model that shows the slow charger should be 500mA
> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
> I think.
Leaving the names a SLOW/FAST is less useful as those names don't *mean*
anything.
The only place where the cable types are registered are in
extcon-max{14577,77693,77843,8997}.c
In each case, the code strongly suggests that the meaning is that "slow"
means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
With names like "fast" and "slow", any common changer framework cannot
make use of these cable types as the name doesn't mean anything.
If the names were changed to 500MA/1A, then common code could reasonably
assume how much current can safely be drawn from each.
>>>
>>> As I know, some fast charger can be drawn 5A, then do we need another
>>> macro named 5A? then will introduce more macros in future, I am not
>>> true this is helpful.
>>
>> It isn't really a question of what the charger can provide. It is a
>> question of what the cable reports to the phy.
>
> Yes, there is no spec to describe fast/slow charger type and how much
> current fast/slow charger can provide. Maybe some fast charger can
> provide 1A/2A, others can provide 5A, which depends on users'
> platform. If we change to EXTCON_CHG_USB_500MA/1A and some fast
> charger can provide 1.5A on user's platform, will it report the fast
> charger type by EXTCON_CHG_USB_1A on user's platform (but it can
> provide 1.5A)? So what I mean, can we keep EXTCON_CHG_USB_SLOW/FAST as
> they were, and maybe fix them in future? (BTW, I've fixed issue 1 and
> maintainer has applied them).
I said "It isn't really a question of
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Hi,
On 12/21/2016 08:57 PM, Mathias Nyman wrote:
> On 21.12.2016 08:57, Lu Baolu wrote:
>> Hi Mathias,
>>
>> I have some comments for the implementation of
>> xhci_handle_command_timeout() as well.
>>
>> On 12/20/2016 11:13 PM, Mathias Nyman wrote:
>>> On 20.12.2016 09:30, Baolin Wang wrote:
>>> ...
>>>
>>> Alright, I gathered all current work related to xhci races and timeouts
>>> and put them into a branch:
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
>>> timeout_race_fixes
>>>
>>> Its based on 4.9
>>> It includes a few other patches just to avoid conflicts and make my life
>>> easier
>>>
>>> Interesting patches are:
>>>
>>> ee4eb91 xhci: remove unnecessary check for pending timer
>>> 0cba67d xhci: detect stop endpoint race using pending timer instead of
>>> counter.
>>> 4f2535f xhci: Handle command completion and timeout race
>>> b9d00d7 usb: host: xhci: Fix possible wild pointer when handling abort
>>> command
>>> 529a5a0 usb: xhci: fix possible wild pointer
>>> 4766555 xhci: Fix race related to abort operation
>>> de834a3 xhci: Use delayed_work instead of timer for command timeout
>>> 69973b8 Linux 4.9
>>>
>>> The fixes for command queue races will go to usb-linus and stable, the
>>> reworks for stop ep watchdog timer will go to usb-next.
>>>
>>> Still completely untested, (well it compiles)
>>>
>>> Felipe gave instructions how to modify dwc3 driver to timeout on address
>>> devicecommands to test these, I'll try to set that up.
>>>
>>> All additional testing is welcome, especially if you can trigger timeouts
>>> and races
>>>
>>> -Mathias
>>>
>>>
>>
>> I post the code below and add my comments in line.
>>
>> 1276 void xhci_handle_command_timeout(struct work_struct *work)
>> 1277 {
>> 1278 struct xhci_hcd *xhci;
>> 1279 int ret;
>> 1280 unsigned long flags;
>> 1281 u64 hw_ring_state;
>> 1282
>> 1283 xhci = container_of(to_delayed_work(work), struct xhci_hcd,
>> cmd_timer);
>> 1284
>> 1285 spin_lock_irqsave(&xhci->lock, flags);
>> 1286
>> 1287 /*
>> 1288 * If timeout work is pending, or current_cmd is NULL, it means
>> we
>> 1289 * raced with command completion. Command is handled so just
>> return.
>> 1290 */
>> 1291 if (!xhci->current_cmd ||
>> delayed_work_pending(&xhci->cmd_timer)) {
>> 1292 spin_unlock_irqrestore(&xhci->lock, flags);
>> 1293 return;
>> 1294 }
>> 1295 /* mark this command to be cancelled */
>> 1296 xhci->current_cmd->status = COMP_CMD_ABORT;
>> 1297
>> 1298 /* Make sure command ring is running before aborting it */
>> 1299 hw_ring_state = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
>> 1300 if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) &&
>> 1301 (hw_ring_state & CMD_RING_RUNNING)) {
>> 1302 /* Prevent new doorbell, and start command abort */
>> 1303 xhci->cmd_ring_state = CMD_RING_STATE_ABORTED;
>> 1304 spin_unlock_irqrestore(&xhci->lock, flags);
>> 1305 xhci_dbg(xhci, "Command timeout\n");
>> 1306 ret = xhci_abort_cmd_ring(xhci);
>> 1307 if (unlikely(ret == -ESHUTDOWN)) {
>> 1308 xhci_err(xhci, "Abort command ring failed\n");
>> 1309 xhci_cleanup_command_queue(xhci);
>> 1310 usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
>> 1311 xhci_dbg(xhci, "xHCI host controller is
>> dead.\n");
>> 1312 }
>> 1313 return;
>> 1314 }
>> 1315
>> 1316 /* host removed. Bail out */
>> 1317 if (xhci->xhc_state & XHCI_STATE_REMOVING) {
>> 1318 spin_unlock_irqrestore(&xhci->lock, flags);
>> 1319 xhci_dbg(xhci, "host removed, ring start fail?\n");
>> 1320 xhci_cleanup_command_queue(xhci);
>> 1321 return;
>> 1322 }
>>
>> I think this part of code should be moved up to line 1295.
>
> The XHCI_STATE_REMOVING and XHCI_STATE_DYING needs a rework,
> I'm working on that.
>
> Basically we want XHCI_STATE_REMOVING to mean that all devices are going,
> away and driver will be removed. Don't bother with re-calculating available
> bandwidths after every device removal, but do use xhci hardware to disable
> devices cleanly etc.
>
> XHCI_STATE_DYING should mean hardware is not working/responding. Don't
> bother writing any registers or queuing anything. Just return all
> pending and cancelled URBs, notify core we died, and free all allocated
> memory.
Okay, thanks for the information.
>
>>
>> 1323
>> 1324 /* command timeout on stopped ring, ring can't be aborted */
>> 1325 xhci_dbg(xhci, "Command timeout on stopped ring\n");
>> 1326 xhci_handle_stopped_cmd_ring(xhci, xhci->current_cmd);
>> 1327 spin_unlock_irqrestore(&xhci->lock, flags);
>>
>> This
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Hi,
On 12/21/2016 08:48 PM, Mathias Nyman wrote:
> On 21.12.2016 08:17, Lu Baolu wrote:
>> Hi Mathias,
>>
>> I have some comments for the implementation of xhci_abort_cmd_ring() below.
>>
>> On 12/20/2016 11:13 PM, Mathias Nyman wrote:
>>> On 20.12.2016 09:30, Baolin Wang wrote:
>>> ...
>>>
>>> Alright, I gathered all current work related to xhci races and timeouts
>>> and put them into a branch:
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
>>> timeout_race_fixes
>>>
>>> Its based on 4.9
>>> It includes a few other patches just to avoid conflicts and make my life
>>> easier
>>>
>>> Interesting patches are:
>>>
>>> ee4eb91 xhci: remove unnecessary check for pending timer
>>> 0cba67d xhci: detect stop endpoint race using pending timer instead of
>>> counter.
>>> 4f2535f xhci: Handle command completion and timeout race
>>> b9d00d7 usb: host: xhci: Fix possible wild pointer when handling abort
>>> command
>>> 529a5a0 usb: xhci: fix possible wild pointer
>>> 4766555 xhci: Fix race related to abort operation
>>> de834a3 xhci: Use delayed_work instead of timer for command timeout
>>> 69973b8 Linux 4.9
>>>
>>> The fixes for command queue races will go to usb-linus and stable, the
>>> reworks for stop ep watchdog timer will go to usb-next.
>>>
>>> Still completely untested, (well it compiles)
>>>
>>> Felipe gave instructions how to modify dwc3 driver to timeout on address
>>> devicecommands to test these, I'll try to set that up.
>>>
>>> All additional testing is welcome, especially if you can trigger timeouts
>>> and races
>>>
>>> -Mathias
>>>
>>>
>>
>> Below is the latest code. I put my comments in line.
>>
>> 322 static int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
>> 323 {
>> 324 u64 temp_64;
>> 325 int ret;
>> 326
>> 327 xhci_dbg(xhci, "Abort command ring\n");
>> 328
>> 329 reinit_completion(&xhci->cmd_ring_stop_completion);
>> 330
>> 331 temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
>> 332 xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
>> 333 &xhci->op_regs->cmd_ring);
>>
>> We should hold xhci->lock when we are modifying xhci registers
>> at runtime.
>>
>
> Makes sense, but we need to unlock it before sleeping or waiting for
> completion.
> I need to look into that in more detail.
>
> But this was an issue already before these changes.
>
>> The retry of setting CMD_RING_ABORT is not necessary according to
>> previous discussion. We have cleaned code for second try in
>> xhci_handle_command_timeout(). Need to clean up here as well.
>>
>
> Yes it can be cleaned up as well, but the two cases are a bit different.
> The cleaned up one was about command ring not starting again after it was
> stopped.
>
> This second try is a workaround for what we thought was the command ring
> failing
> to stop in the first place, but is most likely due to the race that OGAWA
> Hirofumi
> fixed. It races if the stop command ring interrupt happens between writing
> the abort
> bit and polling for the ring stopped bit. The interrupt hander may start the
> command
> ring again, and we would believe we failed to stop it in the first place.
>
> This race could probably be fixed by just extending the lock (and preventing
> interrupts) to cover both writing the abort bit and polling for the command
> ring
> running bit, as you pointed out here previously.
>
> But then again I really like OGAWA Hiroumi's solution that separates the
> command ring stopping from aborting commands and restarting the ring.
>
> The current way of always restarting the command ring as a response to
> a stop command ring command really limits its usage.
>
> So, with this in mind most reasonable would be to
> 1. fix the lock to cover abort+CRR check, and send it to usb-linus +stable
> 2. rebase OGAWA Hirofumi's changes on top of that, and send to usb-linus only
> 3. remove unnecessary second abort try as a separate patch, send to usb-next
> 4. remove polling for the Command ring running (CRR), waiting for completion
>is enough, if completion times out then we can check CRR. for usb-next
>I'll fix the typos these patches would introduce. Fixing old typos can be
> done as separate
> patches later.
This is exactly the same as what I am thinking of. I will submit the patches
later.
Best regards,
Lu Baolu
--
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 2/2] usb: host: xhci: Handle the right timeout command
Hi, On 12/21/2016 11:18 PM, OGAWA Hirofumi wrote: > Mathias Nyman writes: > >>> We set CMD_RING_STATE_ABORTED state under locking. I'm not checking what >>> is for taking lock for register though, I guess it should be enough just >>> lock around of read=>write of ->cmd_ring if need lock. >> After your patch it should be enough to have the lock only while >> reading and writing the cmd_ring register. >> >> If we want a locking fix that applies more easily to older stable >> releases before your change then the lock needs to cover set >> CMD_RING_STATE_ABORT, read cmd_reg, write cmd_reg and busiloop >> checking CRR bit. Otherwise the stop cmd ring interrupt handler may >> restart the ring just before we start checing CRR. The stop cmd ring >> interrupt will set the CMD_RING_STATE_ABORTED to >> CMD_RING_STATE_RUNNING so ring will really restart in the interrupt >> handler. > Just for record (no chance to make patch I myself for now, sorry), while > checking locking slightly, I noticed unrelated missing locking. > > xhci_cleanup_command_queue() > > We are calling it without locking, but we need to lock for accessing list. Yeah. I can make the patch. Best regards, Lu Baolu -- 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: JMS56x not working reliably with uas driver
Hi Oliver, I will try it out and update you!! Regards, -George On Wednesday 21 December 2016 08:09 PM, Oliver Neukum wrote: On Wed, 2016-12-21 at 18:17 +0530, George Cherian wrote: [ 843.149653] scsi host5: uas_post_reset: alloc streams error -19 after reset That would mean the endpoints are gone. Which is odd. [ 843.157268] sd 5:0:0:0: [sdb] Synchronizing SCSI cache Could you try the attached patch and do a SCSI log of the enumeration? 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
USB 3.1 controller does weird magic: appears after Windows 10, disappears otherwise
Hi, I have motherboard GA-Z170X-UD5 TH with 2 USB Type-C (USB 3.1 Gen 2/Thunderbolt 3) ports, BIOS version F20b (latest as of today). Both ports are "sleeping" on cold start - I can't boot from USB device plugged into any and both of these 2 USB Type-C ports, Linux kernel 4.9 doesn't see anything (no messages in dmesg). But magically situation changes when involve Windows 10 to the party. I boot into Windows PE based on Windows 10 and it happily see flash drive and my Nexus 6P connected to both ports. After this without unplugging mentioned devices I reboot into Linux with kernel 4.9 and "magic" - both flash drive and Nexus 6P work fine through these 2 USB Type-C ports. Even though it works in Linux, BIOS (UEFI) still doesn't see anything, can't boot from them. lspci -d ::0c03 -k 00:14.0 USB controller: Intel Corporation Sunrise Point-H USB 3.0 xHCI Controller (rev 31) Subsystem: Gigabyte Technology Co., Ltd Sunrise Point-H USB 3.0 xHCI Controller Kernel driver in use: xhci_hcd 0b:00.0 USB controller: Intel Corporation DSL6540 USB 3.1 Controller [Alpine Ridge] Subsystem: Device : Kernel driver in use: xhci_hcd But things are even more interesting: when I unplug either Nexus 6P or flash drive and then plug back - they still work fine. However as soon as I unplug both of them - Linux doesn't see anything and doesn't post anything into dmesg until I boot into Windows 10 again. Interestingly, USB 3.1 controller also disappears: lspci -d ::0c03 -k 00:14.0 USB controller: Intel Corporation Sunrise Point-H USB 3.0 xHCI Controller (rev 31) Subsystem: Gigabyte Technology Co., Ltd Sunrise Point-H USB 3.0 xHCI Controller Kernel driver in use: xhci_hcd Power is always delivered to both ports, fast charging for Nexus 6P works fine. cat /proc/version Linux version 4.9.0-040900-lowlatency (kernel@tangerine) (gcc version 6.2.0 20161005 (Ubuntu 6.2.0-5ubuntu12) ) #201612111631 SMP PREEMPT Sun Dec 11 21:39:11 UTC 2016 -- 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 3.1 controller does weird magic: appears after Windows 10,
22.12.16 07:37, Oliver Neukum пише: > On Thu, 2016-12-22 at 04:14 +0200, Nazar Mokrynskyi wrote: >> Hi, >> >> I have motherboard GA-Z170X-UD5 TH with 2 USB Type-C (USB 3.1 Gen >> 2/Thunderbolt 3) ports, BIOS version F20b (latest as of today). >> >> Both ports are "sleeping" on cold start - I can't boot from USB device >> plugged into any and both of these 2 USB Type-C ports, Linux kernel >> 4.9 doesn't see anything (no messages in dmesg). > For those controllers it is normal to virtually disconnect if no device > is connected to them. They should undergo a virtual plug event > themselves as soon as a device is plugged in. On other laptops they > in fact do so. > > The failure to do so even for the firmware indicates that your laptop > has faulty ACPI tables and WinPE has a fix for that flaw. > You need to talk to your vendor and the ACPI list. > > Regards > Oliver > > This is a desktop motherboard, I'll contact GIGABYTE and send them your comment about this issue, let's see what they answer. Thank you for this information! smime.p7s Description: Кріптографічний підпис S/MIME
Re: [PATCH v5 0/6] inherit dma configuration from parent dev
On Thu, Nov 17, 2016 at 5:13 PM, Sriram Dash wrote: > For xhci-hcd platform device, all the DMA parameters are not > configured properly, notably dma ops for dwc3 devices. > > The idea here is that you pass in the parent of_node along > with the child device pointer, so it would behave exactly > like the parent already does. The difference is that it also > handles all the other attributes besides the mask. > > Arnd Bergmann (6): > usb: separate out sysdev pointer from usb_bus > usb: chipidea: use bus->sysdev for DMA configuration > usb: ehci: fsl: use bus->sysdev for DMA configuration > usb: xhci: use bus->sysdev for DMA configuration > usb: dwc3: use bus->sysdev for DMA configuration > usb: dwc3: Do not set dma coherent mask Tested patches 1, 4 & 5 on db820c platform with required set of patches [1] for phy. Tested-by: Vivek Gautam for the above mentioned patches. [1] https://lkml.org/lkml/2016/12/20/392 -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- 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 v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On 22 December 2016 at 07:47, NeilBrown wrote:
> On Wed, Dec 21 2016, Baolin Wang wrote:
>
>> On 21 December 2016 at 11:48, NeilBrown wrote:
>>> On Wed, Dec 21 2016, Baolin Wang wrote:
>>>
Hi,
On 21 December 2016 at 06:07, NeilBrown wrote:
> On Tue, Dec 20 2016, Baolin Wang wrote:
>
>> Hi Neil,
>>
>> On 3 November 2016 at 09:25, NeilBrown wrote:
>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>
>>>
> So I won't be responding on this topic any further until I see a
> genuine
> attempt to understand and resolve the inconsistencies with
> usb_register_notifier().
Any better solution?
>>>
>>> I'm not sure exactly what you are asking, so I'll assume you are asking
>>> the question I want to answer :-)
>>>
>>> 1/ Liase with the extcon developers to resolve the inconsistencies
>>> with USB connector types.
>>> e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>>> which both seem to suggest a standard downstream port. There is no
>>> documentation describing how these relate, and no consistent practice
>>> to copy.
>>> I suspect the intention is that
>>> EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>>> the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>>> the cable.
>>> So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>>> while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>>> would normally appear with EXTCON_USB_HOST (I think).
>>> Some drivers follow this model, particularly extcon-max14577.c
>>> but it is not consistent.
>>>
>>> This policy should be well documented and possibly existing drivers
>>> should be updated to follow it.
>>>
>>> At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>>> and EXTCON_CHG_USB_FAST. These names don't mean much.
>>> They were recently removed from drivers/power/axp288_charger.c
>>> which is good, but are still used in drivers/extcon/extcon-max*
>>> Possibly they should be changed to names from the standard, or
>>> possibly they should be renamed to identify the current they are
>>> expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>>
>> Now I am creating the new patchset with fixing and converting exist
>> drivers.
>
> Thanks!
>
>>
>> I did some investigation about EXTCON subsystem. From your suggestion:
>> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
>> After checking, now all extcon drivers were following this rule.
>
> what about extcon-axp288.c ?
> axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
> never sets EXTCON_USB.
> Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.
Ha, sorry, I missed these 2 files, and I will fix them.
>
>>
>> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
>> Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
>> change.
>
> Agreed.
>
>>
>> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
>> There are no model that shows the slow charger should be 500mA
>> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
>> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
>> I think.
>
> Leaving the names a SLOW/FAST is less useful as those names don't *mean*
> anything.
> The only place where the cable types are registered are in
> extcon-max{14577,77693,77843,8997}.c
>
> In each case, the code strongly suggests that the meaning is that "slow"
> means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
>
> With names like "fast" and "slow", any common changer framework cannot
> make use of these cable types as the name doesn't mean anything.
> If the names were changed to 500MA/1A, then common code could reasonably
> assume how much current can safely be drawn from each.
As I know, some fast charger can be drawn 5A, then do we need another
macro named 5A? then will introduce more macros in future, I am not
true this is helpful.
>>>
>>> It isn't really a question of what the charger can provide. It is a
>>> question of what the cable reports to the phy.
>>
>> Yes, there is no spec to describe fast/slow charger type and how much
>> current fast/slow charger can provide. Maybe some fast charger can
>> provide 1A/2A, others can provide 5A, which depends on users'
>> platform. If we change to EXTCON_CHG_USB_500MA/1A and some fast
>> charger can provide 1.5A on user's platform, will it report the fast
>> charger type by EXTCON_CHG_USB_1A on user's platform (but it can
>> provide 1.5A)? So what I mean, can we keep EXTCON_CHG
