Re: [RFC PATCH v1] leds: fix regression in usbport led trigger
Hello Jacek,
On Friday, December 28, 2018 5:29:56 PM CET Jacek Anaszewski wrote:
> On 12/25/18 9:49 PM, Christian Lamparter wrote:
> > The patch "usb: simplify usbport trigger" together with
> > "leds: triggers: add device attribute support" caused an
> > regression for the usbport trigger. it will no longer
> > enumerate any active usb hub ports under the "ports"
> > directory in the sysfs class directory, if the usb host
> > drivers are fully initialized before the usbport trigger
> > was loaded.
> >
> > The reason is that the usbport driver tries to register
> > the sysfs entries during the activate() callback. And this
> > will fail with -2 / ENOENT because the patch
> > "leds: triggers: add device attribute support" made it so
> > that the sysfs "ports" group was only being added after
> > the activate() callback succeeded.
> >
> > This version of the patch moves the device_add_groups()
> > in front of the call to the trigger's activate() function
> > in order to solve the problem.
> >
> > Cc: Greg Kroah-Hartman
> > Cc: Uwe Kleine-König
> > Cc: Rafał Miłecki
> > Fixes: 6f7b0bad8839 ("usb: simplify usbport trigger")
> > Signed-off-by: Christian Lamparter
> > ---
> >
> > This version of the patch ... of which will be many more?!
> >
> > yeah, device_add_groups() in front of activate() works
> > for the usbport driver since it dynamically registers
> > the entries in "ports". However, if a trigger has a
> > static list of entities this can get more complicated,
> > since I think the sysfs entries will now be available
> > before the activate() call even started and this can
> > end up badly. So, is there any better approach?
> > Introduce a "post_activate()" callback? Or use the event
> > system and make usbport trigger on the KOBJ_CHANGE? use
> > a (delayed) work_struct in usbport to register the ports
> > at a slightly later time? etc...
>
> post_activate() is an option.
I was told to wait a bit more because parts of Europe are
still enjoying the holidays. Happy New Year. :-D
> Otherwise, with your patch, in order to prevent access to the sysfs
> interface before the trigger is initialized, we would have to implement
> is_visible op for each struct attribute_group in the triggers
> with static attrs, and check led_cdev->activated there
> (it is currently unused and scheduled for removal, but we
> would have to set it after calling activate() in LED trigger core).
From what I can tell by looking at a lot of led triggers, only the usbport
led trigger is affected. So another sensible option could be to partially
revert 6f7b0bad8839 ("usb: simplify usbport trigger") and let the usbport
trigger manage its sysfs groups by itself. (see below). What do you think?
Best regards,
Christian Lamparter
---
diff --git a/drivers/usb/core/ledtrig-usbport.c
b/drivers/usb/core/ledtrig-usbport.c
index dc7f7fd71684..c12ac56606c3 100644
--- a/drivers/usb/core/ledtrig-usbport.c
+++ b/drivers/usb/core/ledtrig-usbport.c
@@ -119,11 +119,6 @@ static const struct attribute_group ports_group = {
.attrs = ports_attrs,
};
-static const struct attribute_group *ports_groups[] = {
- &ports_group,
- NULL
-};
-
/***
* Adding & removing ports
***/
@@ -307,6 +302,7 @@ static int usbport_trig_notify(struct notifier_block *nb,
unsigned long action,
static int usbport_trig_activate(struct led_classdev *led_cdev)
{
struct usbport_trig_data *usbport_data;
+ int err;
usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
if (!usbport_data)
@@ -315,6 +311,9 @@ static int usbport_trig_activate(struct led_classdev
*led_cdev)
/* List of ports */
INIT_LIST_HEAD(&usbport_data->ports);
+ err = sysfs_create_group(&led_cdev->dev->kobj, &ports_group);
+ if (err)
+ goto err_free;
usb_for_each_dev(usbport_data, usbport_trig_add_usb_dev_ports);
usbport_trig_update_count(usbport_data);
@@ -322,8 +321,11 @@ static int usbport_trig_activate(struct led_classdev
*led_cdev)
usbport_data->nb.notifier_call = usbport_trig_notify;
led_set_trigger_data(led_cdev, usbport_data);
usb_register_notify(&usbport_data->nb);
-
return 0;
+
+err_free:
+ kfree(usbport_data);
+ return err;
}
static void usbport_trig_deactivate(struct led_classdev *led_cdev)
@@ -335,6 +337,8 @@ static void usbport_trig_deactivate(struct led_classdev
*led_cdev)
usbport_trig_remove_port(usbport_data, port);
}
+ sysfs_remove_group(&led_cdev->dev->kobj, &ports_group);
+
usb_unregister_notify(&usbport_data->nb);
kfree(usbport_data);
@@ -344,7 +348,6 @@ static struct led_trigger usbport_led_trigger = {
.name = "usbport",
.activate = usbport_trig_activate,
.deactivate = usbport_trig_deactivate,
- .groups = ports_groups,
};
static int __init usbport_trig_ini
Re: [RFC PATCH v1] leds: fix regression in usbport led trigger
On 1/1/19 3:00 PM, Christian Lamparter wrote:
Hello Jacek,
On Friday, December 28, 2018 5:29:56 PM CET Jacek Anaszewski wrote:
On 12/25/18 9:49 PM, Christian Lamparter wrote:
The patch "usb: simplify usbport trigger" together with
"leds: triggers: add device attribute support" caused an
regression for the usbport trigger. it will no longer
enumerate any active usb hub ports under the "ports"
directory in the sysfs class directory, if the usb host
drivers are fully initialized before the usbport trigger
was loaded.
The reason is that the usbport driver tries to register
the sysfs entries during the activate() callback. And this
will fail with -2 / ENOENT because the patch
"leds: triggers: add device attribute support" made it so
that the sysfs "ports" group was only being added after
the activate() callback succeeded.
This version of the patch moves the device_add_groups()
in front of the call to the trigger's activate() function
in order to solve the problem.
Cc: Greg Kroah-Hartman
Cc: Uwe Kleine-König
Cc: Rafał Miłecki
Fixes: 6f7b0bad8839 ("usb: simplify usbport trigger")
Signed-off-by: Christian Lamparter
---
This version of the patch ... of which will be many more?!
yeah, device_add_groups() in front of activate() works
for the usbport driver since it dynamically registers
the entries in "ports". However, if a trigger has a
static list of entities this can get more complicated,
since I think the sysfs entries will now be available
before the activate() call even started and this can
end up badly. So, is there any better approach?
Introduce a "post_activate()" callback? Or use the event
system and make usbport trigger on the KOBJ_CHANGE? use
a (delayed) work_struct in usbport to register the ports
at a slightly later time? etc...
post_activate() is an option.
I was told to wait a bit more because parts of Europe are
still enjoying the holidays. Happy New Year. :-D
Happy New Year :-)
Otherwise, with your patch, in order to prevent access to the sysfs
interface before the trigger is initialized, we would have to implement
is_visible op for each struct attribute_group in the triggers
with static attrs, and check led_cdev->activated there
(it is currently unused and scheduled for removal, but we
would have to set it after calling activate() in LED trigger core).
From what I can tell by looking at a lot of led triggers, only the usbport
led trigger is affected. So another sensible option could be to partially
revert 6f7b0bad8839 ("usb: simplify usbport trigger") and let the usbport
trigger manage its sysfs groups by itself. (see below). What do you think?
Works for me, but you'll need the approval from the other side,
since this does not belong to the LED subsystem.
Acked-by: Jacek Anaszewski
---
diff --git a/drivers/usb/core/ledtrig-usbport.c
b/drivers/usb/core/ledtrig-usbport.c
index dc7f7fd71684..c12ac56606c3 100644
--- a/drivers/usb/core/ledtrig-usbport.c
+++ b/drivers/usb/core/ledtrig-usbport.c
@@ -119,11 +119,6 @@ static const struct attribute_group ports_group = {
.attrs = ports_attrs,
};
-static const struct attribute_group *ports_groups[] = {
- &ports_group,
- NULL
-};
-
/***
* Adding & removing ports
***/
@@ -307,6 +302,7 @@ static int usbport_trig_notify(struct notifier_block *nb,
unsigned long action,
static int usbport_trig_activate(struct led_classdev *led_cdev)
{
struct usbport_trig_data *usbport_data;
+ int err;
usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
if (!usbport_data)
@@ -315,6 +311,9 @@ static int usbport_trig_activate(struct led_classdev
*led_cdev)
/* List of ports */
INIT_LIST_HEAD(&usbport_data->ports);
+ err = sysfs_create_group(&led_cdev->dev->kobj, &ports_group);
+ if (err)
+ goto err_free;
usb_for_each_dev(usbport_data, usbport_trig_add_usb_dev_ports);
usbport_trig_update_count(usbport_data);
@@ -322,8 +321,11 @@ static int usbport_trig_activate(struct led_classdev *led_cdev)
usbport_data->nb.notifier_call = usbport_trig_notify;
led_set_trigger_data(led_cdev, usbport_data);
usb_register_notify(&usbport_data->nb);
-
return 0;
+
+err_free:
+ kfree(usbport_data);
+ return err;
}
static void usbport_trig_deactivate(struct led_classdev *led_cdev)
@@ -335,6 +337,8 @@ static void usbport_trig_deactivate(struct led_classdev
*led_cdev)
usbport_trig_remove_port(usbport_data, port);
}
+ sysfs_remove_group(&led_cdev->dev->kobj, &ports_group);
+
usb_unregister_notify(&usbport_data->nb);
kfree(usbport_data);
@@ -344,7 +348,6 @@ static struct led_trigger usbport_led_trigger = {
.name = "usbport",
.activate = usbport_trig_activate,
.deactivate = usbport_trig_deactivate,
- .groups = ports_groups,
};
static int __init usb
kernel: xhci_hcd 0000:00:14.0: ERROR unknown event type 37 - Kernel 4.19.13
Kernel 4.19.13 00:14.0 USB controller: Intel Corporation 9 Series Chipset Family USB xHCI Controller Around 400 "unknown event type 37" messages logged in a 2 second span. * Jan 01 02:08:07 computername tvheadend[2370]: linuxdvb: Auvitek AU8522 QAM/8VSB Frontend #0 : ATSC-T #0 - poll TIMEOUT Jan 01 02:08:00 computername tvheadend[2370]: linuxdvb: Auvitek AU8522 QAM/8VSB Frontend #0 : ATSC-T #0 - poll TIMEOUT Jan 01 02:07:56 computername kernel: xhci_hcd :00:14.0: ERROR unknown event type 37 ... Jan 01 02:07:55 computername kernel: xhci_hcd :00:14.0: ERROR unknown event type 37 Jan 01 02:07:55 computername kernel: xhci_hcd :00:14.0: ERROR unknown event type 37 Jan 01 02:07:52 computername tvheadend[2370]: linuxdvb: Auvitek AU8522 QAM/8VSB Frontend #0 : ATSC-T #0 - poll TIMEOUT Jan 01 02:07:44 computername tvheadend[2370]: linuxdvb: Auvitek AU8522 QAM/8VSB Frontend #0 : ATSC-T #0 - poll TIMEOUT * I question whether this also caused kemleak to crash as well (will post after this). Regarding my tv tuner, it isn't supported by the kernel specifically, but is close enough that all I have to do is alter a single source file to include my device's pid, and it works just fine almost all of the time.
Re: kernel: xhci_hcd 0000:00:14.0: ERROR unknown event type 37 - Kernel 4.19.13
Looks like this particular issue may have been due to a touchy/finicky connection. I removed my tuner from my hub and removed the hub from my motherboard's USB and put my tuner in directly. It STILL produced the error, but after I put everything back and played around a little, the errors stopped. Just to be sure, I also rebooted and it's still fine. No xhci errors at all. The only thing I've done recently (within the past few days) was play with my scanner which is also on that hub and maybe brushed my tuner cable or something. On Tue, Jan 1, 2019 at 12:57 PM Nathan Royce wrote: > > Kernel 4.19.13 > > 00:14.0 USB controller: Intel Corporation 9 Series Chipset Family USB > xHCI Controller > > Around 400 "unknown event type 37" messages logged in a 2 second span. > * > Jan 01 02:08:07 computername tvheadend[2370]: linuxdvb: Auvitek AU8522 > QAM/8VSB Frontend #0 : ATSC-T #0 - poll TIMEOUT > Jan 01 02:08:00 computername tvheadend[2370]: linuxdvb: Auvitek AU8522 > QAM/8VSB Frontend #0 : ATSC-T #0 - poll TIMEOUT > Jan 01 02:07:56 computername kernel: xhci_hcd :00:14.0: ERROR > unknown event type 37 > ... > Jan 01 02:07:55 computername kernel: xhci_hcd :00:14.0: ERROR > unknown event type 37 > Jan 01 02:07:55 computername kernel: xhci_hcd :00:14.0: ERROR > unknown event type 37 > Jan 01 02:07:52 computername tvheadend[2370]: linuxdvb: Auvitek AU8522 > QAM/8VSB Frontend #0 : ATSC-T #0 - poll TIMEOUT > Jan 01 02:07:44 computername tvheadend[2370]: linuxdvb: Auvitek AU8522 > QAM/8VSB Frontend #0 : ATSC-T #0 - poll TIMEOUT > * > > I question whether this also caused kemleak to crash as well (will > post after this). > > Regarding my tv tuner, it isn't supported by the kernel specifically, > but is close enough that all I have to do is alter a single source > file to include my device's pid, and it works just fine almost all of > the time.
Re: [PATCH] Add Prolific new chip: PL2303TB & PL2303N(G)
Hi lkp,
Do you have try new patch file for bellowing issue?
">> drivers/usb/serial/pl2303.c:352:36: warning: restricted
__le16 degrades to integer"
Charles
kbuild test robot 於 2018年12月25日 週二 下午11:39寫道：
>
> Hi Charles,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on usb-serial/usb-next]
> [also build test WARNING on v4.20 next-20181224]
> [if your patch is applied to the wrong git tree, please drop us a note to
> help improve the system]
>
> url:
> https://github.com/0day-ci/linux/commits/Charles-Yeh/Add-Prolific-new-chip-PL2303TB-PL2303N-G/20181225-220256
> base: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git
> usb-next
> config: x86_64-allmodconfig (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
> All warnings (new ones prefixed by >>):
>
>include/linux/slab.h:332:43: warning: dubious: x & !y
>include/linux/slab.h:332:43: warning: dubious: x & !y
> >> drivers/usb/serial/pl2303.c:352:36: warning: restricted __le16 degrades to
> >> integer
>include/linux/slab.h:332:43: warning: dubious: x & !y
>include/linux/slab.h:332:43: warning: dubious: x & !y
>
> vim +352 drivers/usb/serial/pl2303.c
>
>324
>325 static int pl2303_startup(struct usb_serial *serial)
>326 {
>327 struct pl2303_serial_private *spriv;
>328 enum pl2303_type type = TYPE_01;
>329 unsigned char *buf;
>330 int res;
>331
>332 spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
>333 if (!spriv)
>334 return -ENOMEM;
>335
>336 buf = kmalloc(1, GFP_KERNEL);
>337 if (!buf) {
>338 kfree(spriv);
>339 return -ENOMEM;
>340 }
>341
>342 if (serial->dev->descriptor.bDeviceClass == 0x02)
>343 type = TYPE_01; /* type 0 */
>344 else if (serial->dev->descriptor.bMaxPacketSize0 == 0x40)
>345 type = TYPE_HX;
>346 else if (serial->dev->descriptor.bDeviceClass == 0x00)
>347 type = TYPE_01; /* type 1 */
>348 else if (serial->dev->descriptor.bDeviceClass == 0xFF)
>349 type = TYPE_01; /* type 1 */
>350 dev_dbg(&serial->interface->dev, "device type: %d\n", type);
>351
> > 352 if (serial->dev->descriptor.bcdUSB == 0x0200) {
>353 res = usb_control_msg(serial->dev,
> usb_rcvctrlpipe(serial->dev, 0),
>354 VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
>355 TYPE_HX_READ_OTP_STATUS_REGISTER, 0, buf, 1,
> 100);
>356 if (res != 1)
>357 type = TYPE_HXN;
>358 }
>359
>360 spriv->type = &pl2303_type_data[type];
>361 spriv->quirks = (unsigned long)usb_get_serial_data(serial);
>362 spriv->quirks |= spriv->type->quirks;
>363
>364 usb_set_serial_data(serial, spriv);
>365
>366 if (type != TYPE_HXN) {
>367 pl2303_vendor_read(serial, 0x8484, buf);
>368 pl2303_vendor_write(serial, 0x0404, 0);
>369 pl2303_vendor_read(serial, 0x8484, buf);
>370 pl2303_vendor_read(serial, 0x8383, buf);
>371 pl2303_vendor_read(serial, 0x8484, buf);
>372 pl2303_vendor_write(serial, 0x0404, 1);
>373 pl2303_vendor_read(serial, 0x8484, buf);
>374 pl2303_vendor_read(serial, 0x8383, buf);
>375 pl2303_vendor_write(serial, 0, 1);
>376 pl2303_vendor_write(serial, 1, 0);
>377 if (spriv->quirks & PL2303_QUIRK_LEGACY)
>378 pl2303_vendor_write(serial, 2, 0x24);
>379 else
>380 pl2303_vendor_write(serial, 2, 0x44);
>381 }
>382
>383 kfree(buf);
>384
>385 return 0;
>386 }
>387
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
[PATCH v2] usb: devio: update max count of DPs per interval for ISOC
The failure happened when I tried to send up to 96DPs per an interval
for SSP ISOC transations by libusb, this is used to verify SSP ISOC
function of USB3 GEN2 controller, so update it as 96DPs.
(refer usb3.1r1.0 section 8.12.6 Isochronous Transactions)
Signed-off-by: Chunfeng Yun
---
v2: update changelog text suggested by Greg
---
drivers/usb/core/devio.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index a75bc0b8a50f..82c16210e34c 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1564,12 +1564,10 @@ static int proc_do_submiturb(struct usb_dev_state *ps,
struct usbdevfs_urb *uurb
}
for (totlen = u = 0; u < number_of_packets; u++) {
/*
-* arbitrary limit need for USB 3.0
-* bMaxBurst (0~15 allowed, 1~16 packets)
-* bmAttributes (bit 1:0, mult 0~2, 1~3 packets)
-* sizemax: 1024 * 16 * 3 = 49152
+* arbitrary limit need for USB 3.1 Gen2
+* sizemax: 96 DPs at SSP, 96 * 1024 = 98304
*/
- if (isopkt[u].length > 49152) {
+ if (isopkt[u].length > 98304) {
ret = -EINVAL;
goto error;
}
--
2.19.1
Re: [PATCH] Add Prolific new chip: PL2303TB & PL2303N(G)
On 01/02/2019 09:04 AM, Charles Yeh wrote: Hi lkp, Do you have try new patch file for bellowing issue? ">> drivers/usb/serial/pl2303.c:352:36: warning: restricted __le16 degrades to integer" Hi Charles, I didn't see the new patch. I think robot could test it automatically if there's a v2 patch in mail list, and robot could send the report to you if found any warning or error. Best Regards, Rong Chen
Re: [PATCH] Add Prolific new chip: PL2303TB & PL2303N(G)
Hi Rong Chen, Thank you for your expained. Hi Johan & Greg, Is this patch already OK? Charles. Rong Chen 於 2019年1月2日 週三 上午9:45寫道： > > > > On 01/02/2019 09:04 AM, Charles Yeh wrote: > > Hi lkp, > >Do you have try new patch file for bellowing issue? > > ">> drivers/usb/serial/pl2303.c:352:36: warning: restricted > > __le16 degrades to integer" > > > Hi Charles, > > I didn't see the new patch. I think robot could test it automatically if > there's a v2 patch in mail list, > and robot could send the report to you if found any warning or error. > > Best Regards, > Rong Chen
Re: [PATCH 1/2] dt-bindings: usb: usb251xb: add documentation for data lane swapping
Hi, On 31/12/2018 12:05, Marco Felsch wrote: Hi, On 18-12-28 17:45, Rob Herring wrote: On Wed, Dec 19, 2018 at 03:59:40PM +0100, Marco Felsch wrote: Add optional binding to allow USB differential-pair (D+/D-) data lane swapping. The swapping can be specified for each port separately, default is no swapping. Signed-off-by: Marco Felsch --- Documentation/devicetree/bindings/usb/usb251xb.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt index 168ff819e827..aec93a92870d 100644 --- a/Documentation/devicetree/bindings/usb/usb251xb.txt +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt @@ -64,6 +64,8 @@ Optional properties : - power-on-time-ms : Specifies the time it takes from the time the host initiates the power-on sequence to a port until the port has adequate power. The value is given in ms in a 0 - 510 range (default is 100ms). + - sw-dx-lanes-ports : Specifies the ports which will swap the differential-pair + (D+/D-), default is not-swapped. Perhaps 'swap-dx-ports' would be a more obvious name. What you think about 'swap-dx-lanes' since it has nothing to do with port swapping, it's rather swapping port properties internally. I'd also prefer 'swap-dx-lanes'. For me it's short and clear. The only downside is that the feature is called "PORT SWAP" in the datasheet. Therefore you should maybe mention it in a comment somewhere in the code so people know what to look for in the datasheet? regards;Richard.L
