RE: [RFC PATCH balbi-usb] usb: renesas_usbhs: usbhs_rcar3_notifier() can be static
Hi, Yoshihiro Shimodawrites: > Hi, > >> From: kbuild test robot, Sent: Thursday, December 14, 2017 2:21 AM >> >> Fixes: 3a7cce26122e ("usb: renesas_usbhs: add extcon notifier to set mode >> for non-otg channel") >> Signed-off-by: Fengguang Wu > > Thank you for the patch! > > Acked-by: Yoshihiro Shimoda amended to patch which introduced the problem, thanks -- balbi signature.asc Description: PGP signature
[PATCH] usb: core: add support for USB_REQ_SET_ISOCH_DELAY
USB SS and SSP hubs provide wHubDelay values on their hub descriptor which we should inform the USB Device about. The USB Specification 3.0 explains, on section 9.4.11, how to calculate the value and how to issue the request. Note that a USB_REQ_SET_ISOCH_DELAY is valid on all device states (Default, Address, Configured), we just *chose* to issue it from Address state right after successfully fetching the USB Device Descriptor. Signed-off-by: Felipe Balbi--- This has been sitting in Intel's test farm for a while now. Me and Mathias have also booted machines ourselves and couldn't notice any problems. It may be a good idea to leave this in next for as long as possible to make sure it gets as much testing as we can. drivers/usb/core/hub.c | 30 ++ drivers/usb/core/message.c | 24 drivers/usb/core/usb.h | 1 + include/linux/usb.h| 6 ++ 4 files changed, 61 insertions(+) diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index cf7bbcb9a63c..ff0e614e06ff 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -38,6 +38,9 @@ #define USB_VENDOR_GENESYS_LOGIC 0x05e3 #define HUB_QUIRK_CHECK_PORT_AUTOSUSPEND 0x01 +#define USB_TP_TRANSMISSION_DELAY 40 /* ns */ +#define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */ + /* Protect struct usb_device->state and ->children members * Note: Both are also protected by ->dev.sem, except that ->state can * change to USB_STATE_NOTATTACHED even when the semaphore isn't held. */ @@ -1352,6 +1355,20 @@ static int hub_configure(struct usb_hub *hub, goto fail; } + /* +* Accumulate wHubDelay + 40ns for every hub in the tree of devices. +* The resulting value will be used for SetIsochDelay() request. +*/ + if (hub_is_superspeed(hdev) || hub_is_superspeedplus(hdev)) { + u32 delay = __le16_to_cpu(hub->descriptor->u.ss.wHubDelay); + + if (hdev->parent) + delay += hdev->parent->hub_delay; + + delay += USB_TP_TRANSMISSION_DELAY; + hdev->hub_delay = min_t(u32, delay, USB_TP_TRANSMISSION_DELAY_MAX); + } + maxchild = hub->descriptor->bNbrPorts; dev_info(hub_dev, "%d port%s detected\n", maxchild, (maxchild == 1) ? "" : "s"); @@ -4599,7 +4616,20 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, if (retval >= 0) retval = -EMSGSIZE; } else { + u32 delay; + retval = 0; + + delay = udev->parent->hub_delay; + udev->hub_delay = min_t(u32, delay, + USB_TP_TRANSMISSION_DELAY_MAX); + retval = usb_set_isoch_delay(udev); + if (retval) { + dev_dbg(>dev, + "Failed set isoch delay, error %d\n", + retval); + retval = 0; + } break; } } diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c index 77001bcfc504..af0d708829dd 100644 --- a/drivers/usb/core/message.c +++ b/drivers/usb/core/message.c @@ -915,6 +915,30 @@ int usb_get_device_descriptor(struct usb_device *dev, unsigned int size) return ret; } +/* + * usb_set_isoch_delay - informs the device of the packet transmit delay + * @dev: the device whose delay is to be informed + * Context: !in_interrupt() + * + * Since this is an optional request, we don't bother if it fails. + */ +int usb_set_isoch_delay(struct usb_device *dev) +{ + /* skip hub devices */ + if (dev->descriptor.bDeviceClass == USB_CLASS_HUB) + return 0; + + /* skip non-SS/non-SSP devices */ + if (dev->speed < USB_SPEED_SUPER) + return 0; + + return usb_control_msg(dev, usb_sndctrlpipe(dev, 0), + USB_REQ_SET_ISOCH_DELAY, + USB_DIR_OUT | USB_TYPE_STANDARD | USB_RECIP_DEVICE, + cpu_to_le16(dev->hub_delay), 0, NULL, 0, + USB_CTRL_SET_TIMEOUT); +} + /** * usb_get_status - issues a GET_STATUS call * @dev: the device whose status is being checked diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h index 2bee08d084ae..149cc7480971 100644 --- a/drivers/usb/core/usb.h +++ b/drivers/usb/core/usb.h @@ -40,6 +40,7 @@ extern int usb_remove_device(struct usb_device *udev); extern int usb_get_device_descriptor(struct usb_device *dev, unsigned int size); +extern int usb_set_isoch_delay(struct usb_device *dev); extern int usb_get_bos_descriptor(struct usb_device *dev); extern
Re: [BUG] usb/io_edgeport: a possible sleep-in-atomic bug in edge_bulk_in_callback
Okay, I had submitted a patch yesterday. You can have a look :) Thanks, Jia-Ju Bai On 2017/12/13 19:38, Johan Hovold wrote: [ +CC: linux-usb] On Wed, Dec 13, 2017 at 06:22:26PM +0800, Jia-Ju Bai wrote: According to drivers/usb/serial/io_edgeport.c, the driver may sleep under a spinlock. The function call path is: edge_bulk_in_callback (acquire the spinlock) process_rcvd_data process_rcvd_status change_port_settings send_iosp_ext_cmd write_cmd_usb usb_kill_urb --> may sleep I do not find a good way to fix it, so I only report. This possible bug is found by my static analysis tool (DSAC) and checked by my code review. Good catch! Fortunately, the fix here is just to remove that usb_kill_urb() from the error path in write_cmd_usb() after usb_submit_urb() fails. Care to submit a patch for that? Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH balbi-usb] usb: renesas_usbhs: usbhs_rcar3_notifier() can be static
Hi, > From: kbuild test robot, Sent: Thursday, December 14, 2017 2:21 AM > > Fixes: 3a7cce26122e ("usb: renesas_usbhs: add extcon notifier to set mode for > non-otg channel") > Signed-off-by: Fengguang WuThank you for the patch! Acked-by: Yoshihiro Shimoda Best regards, Yoshihiro Shimoda -- 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: Dual-role behavior with USB-C?
On Wed, Dec 13, 2017 at 02:37:14PM +0200, Heikki Krogerus wrote: > Hi guys, > > > > I think the USB-OTG PHY is part of i.MX6UL SoC? > > > > Yes, but it is just USB PHY, not PD PHY. You need external pure CC-logic > > chip (like PTN5150), or Type-C PD chip (like PTN5110) to support CC > > or PD event. > > Oh cool, PTP5150 seems to function autonomously if needed! So it would > have worked even without driver support initially. > Yes, if you configure PTN5150 as DRP, the ID pin for SoC will change accordingly. -- Best Regards, Peter Chen -- 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: Dual-role behavior with USB-C?
Hello. Now I am waiting for my USB-C breakout board to just re-using it for connecting to conventional micr-A/B receptacle. BTW, Looking into schematic, another board HiKey960 with USB-C receptacle seems to have USB-C controller in its circuit. >OK, from those schematics we can clearly see that the CC lines are >really connected to the USB_OTG_ID signal. I guess the idea has been >to attempt to get the USB_OTG_ID signal pulled down to ground when the >partner is UFP (device) and has the CC line connected to the ground >with the resistor Rd. That should put the board into host mode, but >I guess it's not working. -- 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] phy: rockchip-typec: Try to turn the PHY on several times
Hi, On Wed, Dec 13, 2017 at 4:41 AM, Enric Balletbo Serrawrote: > Hi Doug, > > 2017-12-11 22:45 GMT+01:00 Douglas Anderson : >> Bind / unbind stress testing of the USB controller on rk3399 found >> that we'd often end up with lots of failures that looked like this: >> >> phy phy-ff80.phy.9: phy poweron failed --> -110 >> dwc3 fe90.dwc3: failed to initialize core >> dwc3: probe of fe90.dwc3 failed with error -110 >> >> Those errors were sometimes seen at bootup too, in which case USB >> peripherals wouldn't work until unplugged and re-plugged in. >> >> I spent some time trying to figure out why the PHY was failing to >> power on but I wasn't able to. Possibly this has to do with the fact >> that the PHY docs say that the USB controller "needs to be held in >> reset to hold pipe power state in P2 before initializing the Type C >> PHY" but that doesn't appear to be easy to do with the dwc3 driver >> today. Messing around with the ordering of the reset vs. the PHY >> initialization in the dwc3 driver didn't seem to fix things. >> >> I did, however, find that if I simply retry the power on it seems to >> have a good chance of working. So let's add some retries. I ran a >> pretty tight bind/unbind loop overnight. When I did so, I found that >> I need to retry between 1% and 2% of the time. Overnight I found only >> a small handful of times where I needed 2 retries. I never found a >> case where I needed 3 retries. >> >> I'm completely aware of the fact that this is quite an ugly hack and I >> wish I didn't have to resort to it, but I have no other real idea how >> to make this hardware reliable. If Rockchip in the future can come up >> with a solution we can always revert this hack. Until then, let's at >> least have something that works. >> >> This patch is tested atop Enric's latest dwc3 patch series ending at: >> https://patchwork.kernel.org/patch/10095527/ >> ...but it could be applied independently of that series without any >> bad effects. >> >> For some more details on this bug, you can refer to: >> https://bugs.chromium.org/p/chromium/issues/detail?id=783464 >> >> Signed-off-by: Douglas Anderson >> --- >> >> drivers/phy/rockchip/phy-rockchip-typec.c | 24 ++-- >> 1 file changed, 22 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/phy/rockchip/phy-rockchip-typec.c >> b/drivers/phy/rockchip/phy-rockchip-typec.c >> index ee85fa0ca4b0..5c2157156ce1 100644 >> --- a/drivers/phy/rockchip/phy-rockchip-typec.c >> +++ b/drivers/phy/rockchip/phy-rockchip-typec.c >> @@ -349,6 +349,8 @@ >> #define MODE_DFP_USB BIT(1) >> #define MODE_DFP_DPBIT(2) >> >> +#define POWER_ON_TRIES 5 >> + > > I did the test of increase the number of tries to 100 because > unfortunately, even with this patch applied, I can see the problem on > my kevin with current mainline. > > [ 244.309094] rockchip-typec-phy ff80.phy: Turn on failed after 100 loops > > That's an extra debug print ^ > > [ 244.317019] phy phy-ff80.phy.8: phy poweron failed --> -110 > [ 244.323824] dwc3 fe90.dwc3: failed to initialize core > [ 244.330057] dwc3: probe of fe90.dwc3 failed with error -110 > > So I'm wondering if there is something else that I need to apply to > really fix this as you didn't reproduce the issue doing lots of tests > and I can reproduce the issue very easily. Ah! I added that message to the top of my upstream series and, indeed, I sometimes see the PHY fail to turn on. Doh. OK, so here's what I've done: * The place where I ran the overnight loops was actually the Chrome OS 4.4 kernel. In that kernel I had a message very similar to yours and I didn't hit it. I just re-ran this for 20 minutes now and I can re-confirm. In the Chrome OS kernel I never see it needing more than a 1 (or 2) loops and it doesn't ever get into the "totally failed" case. * Previously I ran ~10 minutes with the upstream kernel, but at the time I didn't have your printout. After 10 minutes I checked my logs and I definitely saw the "Needed 1 loops to turn on", so I knew my patch was doing something useful. It didn't occur to me to re-confirm that I didn't get the "totally failed" upstream, though now that I say it out loud it's stupid that I didn't think to do this. * Previously when playing with patches on the upstream kernel I saw lots of problems powering on the PHY and I thought my patch was helping, but that was all very non-scientific. So to say it shortly: * For me, my patch makes things a slightly better even on the upstream kernel (I do sometimes see the "turned on after 1 tries") * I can confirm that my patch doesn't fix everything upstream, so there's something different about the Chrome OS tree still. --- I also picked all the local patches from the Chrome OS kernel to the PHY driver and now my PHY driver in the upstream and downstream trees
Re: [PATCH net,stable] net: qmi_wwan: add Sierra EM7565 1199:9091
From: ssjoh...@mac.com Date: Mon, 11 Dec 2017 21:51:14 +0100 > From: Sebastian Sjoholm> > Sierra Wireless EM7565 is an Qualcomm MDM9x50 based M.2 modem. > The USB id is added to qmi_wwan.c to allow QMI communication > with the EM7565. > > Signed-off-by: Sebastian Sjoholm > Acked-by: Bjørn Mork Applied and queued up for -stable. -- 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: fix usbip bind writing random string after command in match_busid
On 12/13/2017 04:07 AM, Juan Zea wrote: > usbip bind writes commands followed by random string when writing to > match_busid attribute in sysfs, caused by using full variable size > instead of string length. > > Signed-off-by: Juan Zea> --- > tools/usb/usbip/src/utils.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/usb/usbip/src/utils.c b/tools/usb/usbip/src/utils.c > index 2b3d6d2..ea1a1af 100644 > --- a/tools/usb/usbip/src/utils.c > +++ b/tools/usb/usbip/src/utils.c > @@ -42,7 +42,7 @@ int modify_match_busid(char *busid, int add) > snprintf(command, SYSFS_BUS_ID_SIZE + 4, "del %s", busid); > > rc = write_sysfs_attribute(match_busid_attr_path, command, > - sizeof(command)); > + strlen(command)); > if (rc < 0) { > dbg("failed to write match_busid: %s", strerror(errno)); > return -1; > Why not use the return value from snprintf() for length, instead of calling strlen(command)? 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
[balbi-usb:testing/next 25/25] drivers/usb/renesas_usbhs/rcar3.c:115:5: sparse: symbol 'usbhs_rcar3_notifier' was not declared. Should it be static?
tree: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next head: 3a7cce26122e3925993ee2ac0b45ac6b9bf6f65f commit: 3a7cce26122e3925993ee2ac0b45ac6b9bf6f65f [25/25] usb: renesas_usbhs: add extcon notifier to set mode for non-otg channel reproduce: # apt-get install sparse git checkout 3a7cce26122e3925993ee2ac0b45ac6b9bf6f65f make ARCH=x86_64 allmodconfig make C=1 CF=-D__CHECK_ENDIAN__ sparse warnings: (new ones prefixed by >>) Please review and possibly fold the followup patch. --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation -- 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
[RFC PATCH balbi-usb] usb: renesas_usbhs: usbhs_rcar3_notifier() can be static
Fixes: 3a7cce26122e ("usb: renesas_usbhs: add extcon notifier to set mode for non-otg channel") Signed-off-by: Fengguang Wu--- rcar3.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/renesas_usbhs/rcar3.c b/drivers/usb/renesas_usbhs/rcar3.c index d657309..d0ea4ff 100644 --- a/drivers/usb/renesas_usbhs/rcar3.c +++ b/drivers/usb/renesas_usbhs/rcar3.c @@ -112,8 +112,8 @@ static int usbhs_rcar3_get_id(struct platform_device *pdev) return USBHS_GADGET; } -int usbhs_rcar3_notifier(struct notifier_block *nb, unsigned long event, -void *data) +static int usbhs_rcar3_notifier(struct notifier_block *nb, unsigned long event, + void *data) { struct usbhs_priv *priv = container_of(nb, struct usbhs_priv, nb); -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: ark3116.c: Remove unused TIOCSSERIAL case from ioctl
On Wed, 13 Dec 2017 15:39:21 +0100 Oliver Neukumwrote: > But: > > device->maxbaudrate = 38400 > > is better than > > device->maxbaudrate = MAX_BAUD > > You see the point? Yes, I see. This is better, because it's more important to know =, but not =. thank you Oliver. --- Mikhail -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: core: prevent malicious bNumInterfaces overflow
On Wed, 13 Dec 2017, Greg KH wrote: > > > --- usb-4.x.orig/drivers/usb/core/config.c > > > +++ usb-4.x/drivers/usb/core/config.c > > > @@ -555,6 +555,9 @@ static int usb_parse_configuration(struc > > > unsigned iad_num = 0; > > > > > > memcpy(>desc, buffer, USB_DT_CONFIG_SIZE); > > > + nintf = nintf_orig = config->desc.bNumInterfaces; > > > + config->desc.bNumInterfaces = 0;// Adjusted later > > > + > > > > The comment format? > > Is fine, I've given up that fight :) In fact, Linus posted an email sometime in the last few weeks, in which he said that he had changed his mind about // comments. He called it the one thing that C++ got right! Also, checkpatch didn't complain. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: ark3116.c: Remove unused TIOCSSERIAL case from ioctl
Am Mittwoch, den 13.12.2017, 15:30 +0300 schrieb Mikhail Zaytsev: > On Wed, 13 Dec 2017 12:40:48 +0100 Oliver Neukumwrote: > > > > > They give you nothing. If you are looking at a vendor ID nothing but the > > bare number makes sense. You are just making peoples' life harder when > > they have to look up that definition. A symbolic name is fine if it gives > > meaning. Even if the information you give is that the value is magic > > and therefore not understood. But a vendor ID is an arbitrary yet > > meaningful number. There is no point in hiding it. > > Thanks. I hear you, Oliver. What about: > > - serstruct.baud_base = 460800; > > Is it a magic number? I think yes. > Hi, yes sure. That is a candidate for a symbolic name. Though if you use it once, I see no benefit, but it does not hurt either. The member is named and that is the important thing. A line like if (rate > 38400) return -EINVAL; is not so good if (rate > MAX_BAUD) return -EINVAL; better But: device->maxbaudrate = 38400 is better than device->maxbaudrate = MAX_BAUD You see the point? 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
[PATCH 1/2] USB: serial: ark3116.c: Remove unused TIOCSSERIAL ioctl case.
The patch removes unused TIOCSSERIAL ioctl case and adds the default block to the switch. Signed-off-by: Mikhail Zaytsev--- drivers/usb/serial/ark3116.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c index 23d46ef87..2e957c76f 100644 --- a/drivers/usb/serial/ark3116.c +++ b/drivers/usb/serial/ark3116.c @@ -418,10 +418,8 @@ static int ark3116_ioctl(struct tty_struct *tty, return -EFAULT; return 0; - case TIOCSSERIAL: - if (copy_from_user(, user_arg, sizeof(serstruct))) - return -EFAULT; - return 0; + default: + break; } return -ENOIOCTLCMD; -- 2.11.0 -- 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 0/2] USB: serial: ark3116.c: ioctl changes
The patch removes unused TIOCSSERIAL case from ioctl. TIOCGSERIAL case moves to the get_serial_info() function. Some magic numbers moves to #define directives. Mikhail Zaytsev (2): USB: serial: ark3116.c: Remove unused TIOCSSERIAL ioctl case. USB: serial: ark3116.c: Move TIOCGSERIAL ioctl case to function. drivers/usb/serial/ark3116.c | 41 +++-- 1 file changed, 23 insertions(+), 18 deletions(-) -- 2.11.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] USB: serial: ark3116.c: Move TIOCGSERIAL ioctl case to function.
The patch moves TIOCGSERIAL ioctl case to get_serial_info function. Signed-off-by: Mikhail Zaytsev--- drivers/usb/serial/ark3116.c | 35 +-- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c index 2e957c76f..2ce8fe10e 100644 --- a/drivers/usb/serial/ark3116.c +++ b/drivers/usb/serial/ark3116.c @@ -36,6 +36,7 @@ #define DRIVER_DESC "USB ARK3116 serial/IrDA driver" #define DRIVER_DEV_DESC "ARK3116 RS232/IrDA" #define DRIVER_NAME "ark3116" +#define ARK3116_BAUDRATE 460800 /* usb timeout of 1 second */ #define ARK_TIMEOUT 1000 @@ -397,27 +398,33 @@ static int ark3116_open(struct tty_struct *tty, struct usb_serial_port *port) return result; } +static int ark3116_get_serial_info(struct usb_serial_port *port, + unsigned long arg) +{ + struct serial_struct ser; + + memset(, 0, sizeof(ser)); + + ser.type = PORT_16654; + ser.line = port->minor; + ser.port = port->port_number; + ser.custom_divisor = 0; + ser.baud_base = ARK3116_BAUDRATE; + + if (copy_to_user((void __user *)arg, , sizeof(ser))) + return -EFAULT; + + return 0; +} + static int ark3116_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg) { struct usb_serial_port *port = tty->driver_data; - struct serial_struct serstruct; - void __user *user_arg = (void __user *)arg; switch (cmd) { case TIOCGSERIAL: - /* XXX: Some of these values are probably wrong. */ - memset(, 0, sizeof(serstruct)); - serstruct.type = PORT_16654; - serstruct.line = port->minor; - serstruct.port = port->port_number; - serstruct.custom_divisor = 0; - serstruct.baud_base = 460800; - - if (copy_to_user(user_arg, , sizeof(serstruct))) - return -EFAULT; - - return 0; + return ark3116_get_serial_info(port, arg); default: break; } -- 2.11.0 -- 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] phy: rockchip-typec: Try to turn the PHY on several times
Hi Doug, 2017-12-11 22:45 GMT+01:00 Douglas Anderson: > Bind / unbind stress testing of the USB controller on rk3399 found > that we'd often end up with lots of failures that looked like this: > > phy phy-ff80.phy.9: phy poweron failed --> -110 > dwc3 fe90.dwc3: failed to initialize core > dwc3: probe of fe90.dwc3 failed with error -110 > > Those errors were sometimes seen at bootup too, in which case USB > peripherals wouldn't work until unplugged and re-plugged in. > > I spent some time trying to figure out why the PHY was failing to > power on but I wasn't able to. Possibly this has to do with the fact > that the PHY docs say that the USB controller "needs to be held in > reset to hold pipe power state in P2 before initializing the Type C > PHY" but that doesn't appear to be easy to do with the dwc3 driver > today. Messing around with the ordering of the reset vs. the PHY > initialization in the dwc3 driver didn't seem to fix things. > > I did, however, find that if I simply retry the power on it seems to > have a good chance of working. So let's add some retries. I ran a > pretty tight bind/unbind loop overnight. When I did so, I found that > I need to retry between 1% and 2% of the time. Overnight I found only > a small handful of times where I needed 2 retries. I never found a > case where I needed 3 retries. > > I'm completely aware of the fact that this is quite an ugly hack and I > wish I didn't have to resort to it, but I have no other real idea how > to make this hardware reliable. If Rockchip in the future can come up > with a solution we can always revert this hack. Until then, let's at > least have something that works. > > This patch is tested atop Enric's latest dwc3 patch series ending at: > https://patchwork.kernel.org/patch/10095527/ > ...but it could be applied independently of that series without any > bad effects. > > For some more details on this bug, you can refer to: > https://bugs.chromium.org/p/chromium/issues/detail?id=783464 > > Signed-off-by: Douglas Anderson > --- > > drivers/phy/rockchip/phy-rockchip-typec.c | 24 ++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/drivers/phy/rockchip/phy-rockchip-typec.c > b/drivers/phy/rockchip/phy-rockchip-typec.c > index ee85fa0ca4b0..5c2157156ce1 100644 > --- a/drivers/phy/rockchip/phy-rockchip-typec.c > +++ b/drivers/phy/rockchip/phy-rockchip-typec.c > @@ -349,6 +349,8 @@ > #define MODE_DFP_USB BIT(1) > #define MODE_DFP_DPBIT(2) > > +#define POWER_ON_TRIES 5 > + I did the test of increase the number of tries to 100 because unfortunately, even with this patch applied, I can see the problem on my kevin with current mainline. [ 244.309094] rockchip-typec-phy ff80.phy: Turn on failed after 100 loops That's an extra debug print ^ [ 244.317019] phy phy-ff80.phy.8: phy poweron failed --> -110 [ 244.323824] dwc3 fe90.dwc3: failed to initialize core [ 244.330057] dwc3: probe of fe90.dwc3 failed with error -110 So I'm wondering if there is something else that I need to apply to really fix this as you didn't reproduce the issue doing lots of tests and I can reproduce the issue very easily. > struct usb3phy_reg { > u32 offset; > u32 enable_bit; > @@ -818,9 +820,8 @@ static int tcphy_get_mode(struct rockchip_typec_phy > *tcphy) > return mode; > } > > -static int rockchip_usb3_phy_power_on(struct phy *phy) > +static int _rockchip_usb3_phy_power_on(struct rockchip_typec_phy *tcphy) > { > - struct rockchip_typec_phy *tcphy = phy_get_drvdata(phy); > struct rockchip_usb3phy_port_cfg *cfg = >port_cfgs; > const struct usb3phy_reg *reg = >pipe_status; > int timeout, new_mode, ret = 0; > @@ -867,6 +868,25 @@ static int rockchip_usb3_phy_power_on(struct phy *phy) > return ret; > } > > +static int rockchip_usb3_phy_power_on(struct phy *phy) > +{ > + struct rockchip_typec_phy *tcphy = phy_get_drvdata(phy); > + int ret; > + int tries; > + > + for (tries = 0; tries < POWER_ON_TRIES; tries++) { > + ret = _rockchip_usb3_phy_power_on(tcphy); > + if (!ret) > + break; > + } > + > + if (tries && !ret) > + dev_info(tcphy->dev, "Needed %d loops to turn on\n", tries); > + It's curious that in my case I never see this message, or it works or it fails after 100 retries. I'll do more longer tests and continue investigating a little bit. Regards, Enric > + return ret; > +} > + > + > static int rockchip_usb3_phy_power_off(struct phy *phy) > { > struct rockchip_typec_phy *tcphy = phy_get_drvdata(phy); > -- > 2.15.1.424.g9478a66081-goog > > -- > 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
Re: Dual-role behavior with USB-C?
Hi guys, On Wed, Dec 13, 2017 at 10:43:50AM +0800, Peter Chen wrote: > On Wed, Dec 13, 2017 at 01:17:23AM +, Takashi Matsuzawa wrote: > > Hello. > > > > >If you have a Type-C connector on your board, then you also should > > >have a USB Type-C PHY that takes care of CC logic. The host-to-device > > >relationship is determined using the Configuration Channel (CC) that > > >goes through the USB Type-C cable. Note that CC is not the same as ID! > > > > I am playing with i.MX6 pico board, which has USB-OTG receptacle with USB-C > > physical shape. > > http://download.wandboard.org/hobbitboard-imx6ul/documentation/pico-imx6ul-emmc-hobbit-reva1-hardware-manual-20160328.pdf OK, from those schematics we can clearly see that the CC lines are really connected to the USB_OTG_ID signal. I guess the idea has been to attempt to get the USB_OTG_ID signal pulled down to ground when the partner is UFP (device) and has the CC line connected to the ground with the resistor Rd. That should put the board into host mode, but I guess it's not working. With Type-C to Type-C connections, in case the partner is DFP (host) or DRP (dual-role), that configuration will just confuse the partner. The partner will now see that the CC is pulled high and probable think that your board is also DFP (host), i.e. your board and the partner connected to it, both think they need to be in device mode. I don't think there are actually any guarantees that connecting the CC lines to the ID input on the board will work in any scenario. There really should be a CC-logic chip if dual-role support was needed. For device mode only, simply connecting the CC lines to the ground from the connector would have been enough. > > I think the USB-OTG PHY is part of i.MX6UL SoC? > > Yes, but it is just USB PHY, not PD PHY. You need external pure CC-logic > chip (like PTN5150), or Type-C PD chip (like PTN5110) to support CC > or PD event. Oh cool, PTP5150 seems to function autonomously if needed! So it would have worked even without driver support initially. Cheers, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb/io_edgeport: Fix a possible sleep-in-atomic bug in edge_bulk_in_callback
According to drivers/usb/serial/io_edgeport.c, the driver may sleep under a spinlock. The function call path is: edge_bulk_in_callback (acquire the spinlock) process_rcvd_data process_rcvd_status change_port_settings send_iosp_ext_cmd write_cmd_usb usb_kill_urb --> may sleep To fix it, usb_kill_urb() is removed from the error path after usb_submit_urb() fails. This possible bug is found by my static analysis tool (DSAC) and checked by my code review. Signed-off-by: Jia-Ju Bai--- drivers/usb/serial/io_edgeport.c |1 - 1 file changed, 1 deletion(-) diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c index 219265c..17283f4 100644 --- a/drivers/usb/serial/io_edgeport.c +++ b/drivers/usb/serial/io_edgeport.c @@ -2282,7 +2282,6 @@ static int write_cmd_usb(struct edgeport_port *edge_port, /* something went wrong */ dev_err(dev, "%s - usb_submit_urb(write command) failed, status = %d\n", __func__, status); - usb_kill_urb(urb); usb_free_urb(urb); atomic_dec(); return status; -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: ark3116.c: Remove unused TIOCSSERIAL case from ioctl
On Wed, 13 Dec 2017 12:40:48 +0100 Oliver Neukumwrote: > They give you nothing. If you are looking at a vendor ID nothing but the > bare number makes sense. You are just making peoples' life harder when > they have to look up that definition. A symbolic name is fine if it gives > meaning. Even if the information you give is that the value is magic > and therefore not understood. But a vendor ID is an arbitrary yet > meaningful number. There is no point in hiding it. Thanks. I hear you, Oliver. What about: - serstruct.baud_base = 460800; Is it a magic number? I think yes. -- Mikhail -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] Staging: rtl8723bs: Replace true with x and false with !x
On Wed, Dec 13, 2017 at 05:25:42PM +0530, Shreeya Patel wrote: > Replace true and false keywords with "x" and "!x" > respectively to follow the kernel coding style. > > Signed-off-by: Shreeya Patel> --- > drivers/staging/rtl8723bs/hal/sdio_ops.c | 30 ++ > 1 file changed, 14 insertions(+), 16 deletions(-) $ ./scripts/get_maintainer.pl --file drivers/staging/rtl8723bs/hal/sdio_ops.c Greg Kroah-Hartman (supporter:STAGING SUBSYSTEM,commit_signer:4/4=100%) Larry Finger (commit_signer:1/4=25%) Aishwarya Pant (commit_signer:1/4=25%,authored:1/4=25%,removed_lines:4/10=40%) Himanshu Jha (commit_signer:1/4=25%,authored:1/4=25%,removed_lines:1/10=10%) Hans de Goede (commit_signer:1/4=25%,authored:1/4=25%,added_lines:1296/1304=99%) Joe Perches (authored:1/4=25%,removed_lines:5/10=50%) de...@driverdev.osuosl.org (open list:STAGING SUBSYSTEM) linux-ker...@vger.kernel.org (open list) You got the wrong list, and wrong person, and didn't send all 4 patches :( Please fix up and try again. 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 2/4] Staging: rtl8723bs: Change names to conform to the kernel code
Change names of some variables and functions to conform to the kernel coding style. Signed-off-by: Shreeya Patel--- drivers/staging/rtl8723bs/hal/sdio_ops.c | 186 +++ 1 file changed, 93 insertions(+), 93 deletions(-) diff --git a/drivers/staging/rtl8723bs/hal/sdio_ops.c b/drivers/staging/rtl8723bs/hal/sdio_ops.c index aa52c31..e72f80f 100644 --- a/drivers/staging/rtl8723bs/hal/sdio_ops.c +++ b/drivers/staging/rtl8723bs/hal/sdio_ops.c @@ -29,34 +29,34 @@ /* */ static void HalSdioGetCmdAddr8723BSdio( struct adapter *padapter, - u8 DeviceID, - u32 Addr, - u32 *pCmdAddr + u8 device_id, + u32 addr, + u32 *pcmdaddr ) { - switch (DeviceID) { + switch (device_id) { case SDIO_LOCAL_DEVICE_ID: - *pCmdAddr = ((SDIO_LOCAL_DEVICE_ID << 13) | (Addr & SDIO_LOCAL_MSK)); + *pcmdaddr = ((SDIO_LOCAL_DEVICE_ID << 13) | (addr & SDIO_LOCAL_MSK)); break; case WLAN_IOREG_DEVICE_ID: - *pCmdAddr = ((WLAN_IOREG_DEVICE_ID << 13) | (Addr & WLAN_IOREG_MSK)); + *pcmdaddr = ((WLAN_IOREG_DEVICE_ID << 13) | (addr & WLAN_IOREG_MSK)); break; case WLAN_TX_HIQ_DEVICE_ID: - *pCmdAddr = ((WLAN_TX_HIQ_DEVICE_ID << 13) | (Addr & WLAN_FIFO_MSK)); + *pcmdaddr = ((WLAN_TX_HIQ_DEVICE_ID << 13) | (addr & WLAN_FIFO_MSK)); break; case WLAN_TX_MIQ_DEVICE_ID: - *pCmdAddr = ((WLAN_TX_MIQ_DEVICE_ID << 13) | (Addr & WLAN_FIFO_MSK)); + *pcmdaddr = ((WLAN_TX_MIQ_DEVICE_ID << 13) | (addr & WLAN_FIFO_MSK)); break; case WLAN_TX_LOQ_DEVICE_ID: - *pCmdAddr = ((WLAN_TX_LOQ_DEVICE_ID << 13) | (Addr & WLAN_FIFO_MSK)); + *pcmdaddr = ((WLAN_TX_LOQ_DEVICE_ID << 13) | (addr & WLAN_FIFO_MSK)); break; case WLAN_RX0FF_DEVICE_ID: - *pCmdAddr = ((WLAN_RX0FF_DEVICE_ID << 13) | (Addr & WLAN_RX0FF_MSK)); + *pcmdaddr = ((WLAN_RX0FF_DEVICE_ID << 13) | (addr & WLAN_RX0FF_MSK)); break; default: @@ -66,64 +66,64 @@ static void HalSdioGetCmdAddr8723BSdio( static u8 get_deviceid(u32 addr) { - u8 devideId; - u16 pseudoId; + u8 devide_id; + u16 pseudo_id; - pseudoId = (u16)(addr >> 16); - switch (pseudoId) { + pseudo_id = (u16)(addr >> 16); + switch (pseudo_id) { case 0x1025: - devideId = SDIO_LOCAL_DEVICE_ID; + devide_id = SDIO_LOCAL_DEVICE_ID; break; case 0x1026: - devideId = WLAN_IOREG_DEVICE_ID; + devide_id = WLAN_IOREG_DEVICE_ID; break; /* case 0x1027: */ -/* devideId = SDIO_FIRMWARE_FIFO; */ +/* devide_id = SDIO_FIRMWARE_FIFO; */ /* break; */ case 0x1031: - devideId = WLAN_TX_HIQ_DEVICE_ID; + devide_id = WLAN_TX_HIQ_DEVICE_ID; break; case 0x1032: - devideId = WLAN_TX_MIQ_DEVICE_ID; + devide_id = WLAN_TX_MIQ_DEVICE_ID; break; case 0x1033: - devideId = WLAN_TX_LOQ_DEVICE_ID; + devide_id = WLAN_TX_LOQ_DEVICE_ID; break; case 0x1034: - devideId = WLAN_RX0FF_DEVICE_ID; + devide_id = WLAN_RX0FF_DEVICE_ID; break; default: -/* devideId = (u8)((addr >> 13) & 0xF); */ - devideId = WLAN_IOREG_DEVICE_ID; +/* devide_id = (u8)((addr >> 13) & 0xF); */ + devide_id = WLAN_IOREG_DEVICE_ID; break; } - return devideId; + return devide_id; } /* * Ref: *HalSdioGetCmdAddr8723BSdio() */ -static u32 _cvrt2ftaddr(const u32 addr, u8 *pdeviceId, u16 *poffset) +static u32 _cvrt2ftaddr(const u32 addr, u8 *pdevice_id, u16 *poffset) { - u8 deviceId; + u8 device_id; u16 offset; u32 ftaddr; - deviceId = get_deviceid(addr); + device_id = get_deviceid(addr); offset = 0; - switch (deviceId) { + switch (device_id) { case SDIO_LOCAL_DEVICE_ID: offset = addr & SDIO_LOCAL_MSK; break; @@ -140,14 +140,14 @@ static u32 _cvrt2ftaddr(const u32 addr, u8 *pdeviceId, u16 *poffset) case WLAN_IOREG_DEVICE_ID: default: - deviceId = WLAN_IOREG_DEVICE_ID; + device_id = WLAN_IOREG_DEVICE_ID; offset = addr & WLAN_IOREG_MSK; break; } - ftaddr = (deviceId << 13) | offset; + ftaddr = (device_id << 13) | offset; - if (pdeviceId) - *pdeviceId = deviceId; + if (pdevice_id) +
[PATCH 4/4] Staging: rtl8723bs: Use !x instead of NULL comparison
If "x" is compared to NULL, use "!x" instead of it, so as to follow the kernel coding style. Signed-off-by: Shreeya Patel--- drivers/staging/rtl8723bs/hal/sdio_ops.c | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/staging/rtl8723bs/hal/sdio_ops.c b/drivers/staging/rtl8723bs/hal/sdio_ops.c index 67c4cca..d6a4572 100644 --- a/drivers/staging/rtl8723bs/hal/sdio_ops.c +++ b/drivers/staging/rtl8723bs/hal/sdio_ops.c @@ -215,7 +215,7 @@ static u32 sdio_read32(struct intf_hdl *pintfhdl, u32 addr) u8 *ptmpbuf; ptmpbuf = rtw_malloc(8); - if (NULL == ptmpbuf) { + if (!ptmpbuf) { DBG_8192C(KERN_ERR "%s: Allocate memory FAIL!(size =8) addr = 0x%x\n", __func__, addr); return SDIO_ERR_VAL32; } @@ -264,7 +264,7 @@ static s32 sdio_readN(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *pbuf) ftaddr &= ~(u16)0x3; n = cnt + shift; ptmpbuf = rtw_malloc(n); - if (NULL == ptmpbuf) + if (!ptmpbuf) return -1; err = sd_read(pintfhdl, ftaddr, n, ptmpbuf); @@ -367,7 +367,7 @@ static s32 sdio_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *pbuf) ftaddr &= ~(u16)0x3; n = cnt + shift; ptmpbuf = rtw_malloc(n); - if (NULL == ptmpbuf) + if (!ptmpbuf) return -1; err = sd_read(pintfhdl, ftaddr, 4, ptmpbuf); if (err) { @@ -457,7 +457,7 @@ static u32 sdio_read_port( #ifdef SDIO_DYNAMIC_ALLOC_MEM oldmem = mem; mem = rtw_malloc(cnt); - if (mem == NULL) { + if (!mem) { DBG_8192C(KERN_WARNING "%s: allocate memory %d bytes fail!\n", __func__, cnt); mem = oldmem; oldmem = NULL; @@ -745,7 +745,7 @@ static s32 ReadInterrupt8723BSdio(struct adapter *padapter, u32 *phisr) u8 val8, hisr_len; - if (phisr == NULL) + if (!phisr) return false; himr = GET_HAL_DATA(padapter)->sdio_himr; @@ -969,13 +969,13 @@ static struct recv_buf *sd_recv_rxfifo(struct adapter *padapter, u32 size) /* 3 1. alloc recvbuf */ precvpriv = >recvpriv; precvbuf = rtw_dequeue_recvbuf(>free_recv_buf_queue); - if (precvbuf == NULL) { + if (!precvbuf) { DBG_871X_LEVEL(_drv_err_, "%s: alloc recvbuf FAIL!\n", __func__); return NULL; } /* 3 2. alloc skb */ - if (precvbuf->pskb == NULL) { + if (!precvbuf->pskb) { SIZE_PTR tmpaddr = 0; SIZE_PTR alignment = 0; @@ -989,7 +989,7 @@ static struct recv_buf *sd_recv_rxfifo(struct adapter *padapter, u32 size) skb_reserve(precvbuf->pskb, (RECVBUFF_ALIGN_SZ - alignment)); } - if (precvbuf->pskb == NULL) { + if (!precvbuf->pskb) { DBG_871X("%s: alloc_skb fail! read =%d\n", __func__, readsize); return NULL; } @@ -1247,7 +1247,7 @@ u8 RecvOnePkt(struct adapter *padapter, u32 size) DBG_871X("+%s: size: %d+\n", __func__, size); - if (padapter == NULL) { + if (!padapter) { DBG_871X(KERN_ERR "%s: padapter is NULL!\n", __func__); return false; } -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] Staging: rtl8723bs: Change condition to assignment
Change the conditional operator to assignment as it is not a conditional statement. Signed-off-by: Shreeya Patel--- drivers/staging/rtl8723bs/hal/sdio_ops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/rtl8723bs/hal/sdio_ops.c b/drivers/staging/rtl8723bs/hal/sdio_ops.c index e72f80f..67c4cca 100644 --- a/drivers/staging/rtl8723bs/hal/sdio_ops.c +++ b/drivers/staging/rtl8723bs/hal/sdio_ops.c @@ -460,7 +460,7 @@ static u32 sdio_read_port( if (mem == NULL) { DBG_8192C(KERN_WARNING "%s: allocate memory %d bytes fail!\n", __func__, cnt); mem = oldmem; - oldmem == NULL; + oldmem = NULL; } #else /* in this case, caller should gurante the buffer is big enough */ -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] Staging: rtl8723bs: Replace true with x and false with !x
Replace true and false keywords with "x" and "!x" respectively to follow the kernel coding style. Signed-off-by: Shreeya Patel--- drivers/staging/rtl8723bs/hal/sdio_ops.c | 30 ++ 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/drivers/staging/rtl8723bs/hal/sdio_ops.c b/drivers/staging/rtl8723bs/hal/sdio_ops.c index 93ac083..aa52c31 100644 --- a/drivers/staging/rtl8723bs/hal/sdio_ops.c +++ b/drivers/staging/rtl8723bs/hal/sdio_ops.c @@ -191,8 +191,8 @@ static u32 sdio_read32(struct intf_hdl *pintfhdl, u32 addr) rtw_hal_get_hwreg(padapter, HW_VAR_APFM_ON_MAC, ); if ( ((deviceId == WLAN_IOREG_DEVICE_ID) && (offset < 0x100)) || - (false == bMacPwrCtrlOn) || - (true == adapter_to_pwrctl(padapter)->bFwCurrentInPSMode) + (!bMacPwrCtrlOn) || + (adapter_to_pwrctl(padapter)->bFwCurrentInPSMode) ) { err = sd_cmd52_read(pintfhdl, ftaddr, 4, (u8 *)_tmp); #ifdef SDIO_DEBUG_IO @@ -248,8 +248,8 @@ static s32 sdio_readN(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *pbuf) rtw_hal_get_hwreg(padapter, HW_VAR_APFM_ON_MAC, ); if ( ((deviceId == WLAN_IOREG_DEVICE_ID) && (offset < 0x100)) || - (false == bMacPwrCtrlOn) || - (true == adapter_to_pwrctl(padapter)->bFwCurrentInPSMode) + (!bMacPwrCtrlOn) || + (adapter_to_pwrctl(padapter)->bFwCurrentInPSMode) ) return sd_cmd52_read(pintfhdl, ftaddr, cnt, pbuf); @@ -352,8 +352,8 @@ static s32 sdio_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *pbuf) rtw_hal_get_hwreg(padapter, HW_VAR_APFM_ON_MAC, ); if ( ((deviceId == WLAN_IOREG_DEVICE_ID) && (offset < 0x100)) || - (false == bMacPwrCtrlOn) || - (true == adapter_to_pwrctl(padapter)->bFwCurrentInPSMode) + (!bMacPwrCtrlOn) || + (adapter_to_pwrctl(padapter)->bFwCurrentInPSMode) ) return sd_cmd52_write(pintfhdl, ftaddr, cnt, pbuf); @@ -513,7 +513,7 @@ static u32 sdio_write_port( padapter = pintfhdl->padapter; psdio = _to_dvobj(padapter)->intf_data; - if (padapter->hw_init_completed == false) { + if (!padapter->hw_init_completed) { DBG_871X("%s [addr = 0x%x cnt =%d] padapter->hw_init_completed == false\n", __func__, addr, cnt); return _FAIL; } @@ -577,7 +577,7 @@ static s32 _sdio_local_read( HalSdioGetCmdAddr8723BSdio(padapter, SDIO_LOCAL_DEVICE_ID, addr, ); rtw_hal_get_hwreg(padapter, HW_VAR_APFM_ON_MAC, ); - if (false == bMacPwrCtrlOn) + if (!bMacPwrCtrlOn) return _sd_cmd52_read(pintfhdl, addr, cnt, pbuf); n = RND4(cnt); @@ -616,8 +616,8 @@ s32 sdio_local_read( rtw_hal_get_hwreg(padapter, HW_VAR_APFM_ON_MAC, ); if ( - (false == bMacPwrCtrlOn) || - (true == adapter_to_pwrctl(padapter)->bFwCurrentInPSMode) + (!bMacPwrCtrlOn) || + (adapter_to_pwrctl(padapter)->bFwCurrentInPSMode) ) return sd_cmd52_read(pintfhdl, addr, cnt, pbuf); @@ -662,8 +662,8 @@ s32 sdio_local_write( rtw_hal_get_hwreg(padapter, HW_VAR_APFM_ON_MAC, ); if ( - (false == bMacPwrCtrlOn) || - (true == adapter_to_pwrctl(padapter)->bFwCurrentInPSMode) + (!bMacPwrCtrlOn) || + (adapter_to_pwrctl(padapter)->bFwCurrentInPSMode) ) return sd_cmd52_write(pintfhdl, addr, cnt, pbuf); @@ -843,8 +843,7 @@ void ClearInterrupt8723BSdio(struct adapter *padapter) struct hal_com_data *pHalData; u8 *clear; - - if (true == padapter->bSurpriseRemoved) + if (padapter->bSurpriseRemoved) return; pHalData = GET_HAL_DATA(padapter); @@ -1161,8 +1160,7 @@ void sd_int_hdl(struct adapter *padapter) if ( - (padapter->bDriverStopped == true) || - (padapter->bSurpriseRemoved == true) + (padapter->bDriverStopped) || (padapter->bSurpriseRemoved) ) return; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] Remove checkpatch warnings
This patchset removes some warnings generated by checkpatch for cleanup of the rtl8723bs driver. Also some additional cleanups are introduced in the *[1/4] and *[3/4] patches to make the code according to the kernel coding style. Shreeya Patel (4): Staging: rtl8723bs: Replace true with x and false with !x Staging: rtl8723bs: Change names to conform to the kernel code Staging: rtl8723bs: Change condition to assignment Staging: rtl8723bs: Use !x instead of NULL comparison drivers/staging/rtl8723bs/hal/sdio_ops.c | 224 +++ 1 file changed, 111 insertions(+), 113 deletions(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: ti_usb_3410_5052: Avoid goto in bulk in callback
On Wed, Dec 13, 2017 at 12:32:47PM +0100, Ladislav Michl wrote: > On Wed, Dec 13, 2017 at 12:16:05PM +0100, Johan Hovold wrote: > > On Mon, Dec 11, 2017 at 01:44:57PM +0100, Ladislav Michl wrote: > > > On Mon, Dec 11, 2017 at 12:52:46PM +0100, Johan Hovold wrote: > > > > On Mon, Dec 11, 2017 at 12:32:49PM +0100, Ladislav Michl wrote: > > > > > On Mon, Dec 11, 2017 at 11:10:35AM +0100, Johan Hovold wrote: > > > [snip] > > > > > > I'm afraid I don't consider this an improvement. I prefer using > > > > > > gotos > > > > > > for error paths, while keeping the success path out of the status > > > > > > switch. > > > > > > > > > > > > Furthermore, this isn't functionally equivalent as we'd not longer > > > > > > log > > > > > > an error for -EPIPE. > > > > > > > > > > Yes, you are right... Now, shouldn't we react somehow to stalled > > > > > endpoint? > > > > > Tty side seems to be unaware of it. > > > > > > > > Recovering from a stalled endpoint is a bit involved, so for now we > > > > typically just log an error an bail out (forcing the user to reopen the > > > > port). This seems to work well enough as this condition should be rare. > > > > > > I just do not see this in code. I would expect pending tty I/O operation > > > would fail once USB device errors out with -EPIPE, so tty side consumer > > > gets > > > notified about error. Either it is not there or I did not look hard > > > enough :) > > > > No, we do not provide any error notification besides logging an error > > when an endpoint has been stalled (i.e. in case of a read, you would not > > receive any more data before the halt condition has been cleared). > > Hmm, could we make 1aba579f3cf5 a bit more generic then? I'm not sure this is worth the complexity it adds, or that it really solves anything as the fundamental problem remains; simply clearing the Halt is not sufficient unless the underlying reason for the condition triggering the stall has first been removed (and that is going to be device specific). > > Given that there's been no reports about this being an issue (for the > > past ten years and that I can recall), this crude handling appears to > > suffice. > > Because embedded people tend to power cycle stuck device on timeout. > That involves project specific hacks in userspace, which could be > probably handled better. I haven't seen any reports of this being an issue (and your stalled CDC device was really due to a hardware/power issue IIRC) so again, I'm not convinced this gives us anything. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: ark3116.c: Remove unused TIOCSSERIAL case from ioctl
Am Mittwoch, den 13.12.2017, 14:31 +0300 schrieb Mikhail Zaytsev: > On Wed, 13 Dec 2017 11:17:28 +0100 Oliver Neukumwrote: > > > > > Am Mittwoch, den 13.12.2017, 12:30 +0300 schrieb Mikhail Zaytsev: > > > > > > +#define RS232_VENDOR 0x6547 > > > +#define RS232_PRODUCT 0x0232 > > > +#define IRDA_VENDOR 0x18ec > > > +#define IRDA_PRODUCT 0x3118 > > > > > > /* usb timeout of 1 second */ > > > #define ARK_TIMEOUT 1000 > > > > > > static const struct usb_device_id id_table[] = { > > > - { USB_DEVICE(0x6547, 0x0232) }, > > > - { USB_DEVICE(0x18ec, 0x3118) }, /* USB to IrDA adapter */ > > > + { USB_DEVICE(RS232_VENDOR, RS232_PRODUCT) }, > > > + { USB_DEVICE(IRDA_VENDOR, IRDA_PRODUCT) }, /* USB to IrDA > > > adapter */ > > > > Hi, > > > > what is the purpose of this change? It just makes it harder to grep. > > The constants are arbitrary and they are clearly device IDs. > > The constants are using in several places. > I think the names easier to read. They give you nothing. If you are looking at a vendor ID nothing but the bare number makes sense. You are just making peoples' life harder when they have to look up that definition. A symbolic name is fine if it gives meaning. Even if the information you give is that the value is magic and therefore not understood. But a vendor ID is an arbitrary yet meaningful number. There is no point in hiding 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
Re: [BUG] usb/io_edgeport: a possible sleep-in-atomic bug in edge_bulk_in_callback
[ +CC: linux-usb] On Wed, Dec 13, 2017 at 06:22:26PM +0800, Jia-Ju Bai wrote: > According to drivers/usb/serial/io_edgeport.c, the driver may sleep > under a spinlock. > The function call path is: > edge_bulk_in_callback (acquire the spinlock) >process_rcvd_data > process_rcvd_status >change_port_settings > send_iosp_ext_cmd >write_cmd_usb > usb_kill_urb --> may sleep > > I do not find a good way to fix it, so I only report. > This possible bug is found by my static analysis tool (DSAC) and checked > by my code review. Good catch! Fortunately, the fix here is just to remove that usb_kill_urb() from the error path in write_cmd_usb() after usb_submit_urb() fails. Care to submit a patch for that? Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: ti_usb_3410_5052: Avoid goto in bulk in callback
On Wed, Dec 13, 2017 at 12:16:05PM +0100, Johan Hovold wrote: > On Mon, Dec 11, 2017 at 01:44:57PM +0100, Ladislav Michl wrote: > > On Mon, Dec 11, 2017 at 12:52:46PM +0100, Johan Hovold wrote: > > > On Mon, Dec 11, 2017 at 12:32:49PM +0100, Ladislav Michl wrote: > > > > On Mon, Dec 11, 2017 at 11:10:35AM +0100, Johan Hovold wrote: > > [snip] > > > > > I'm afraid I don't consider this an improvement. I prefer using gotos > > > > > for error paths, while keeping the success path out of the status > > > > > switch. > > > > > > > > > > Furthermore, this isn't functionally equivalent as we'd not longer log > > > > > an error for -EPIPE. > > > > > > > > Yes, you are right... Now, shouldn't we react somehow to stalled > > > > endpoint? > > > > Tty side seems to be unaware of it. > > > > > > Recovering from a stalled endpoint is a bit involved, so for now we > > > typically just log an error an bail out (forcing the user to reopen the > > > port). This seems to work well enough as this condition should be rare. > > > > I just do not see this in code. I would expect pending tty I/O operation > > would fail once USB device errors out with -EPIPE, so tty side consumer gets > > notified about error. Either it is not there or I did not look hard enough > > :) > > No, we do not provide any error notification besides logging an error > when an endpoint has been stalled (i.e. in case of a read, you would not > receive any more data before the halt condition has been cleared). Hmm, could we make 1aba579f3cf5 a bit more generic then? > Given that there's been no reports about this being an issue (for the > past ten years and that I can recall), this crude handling appears to > suffice. Because embedded people tend to power cycle stuck device on timeout. That involves project specific hacks in userspace, which could be probably handled better. Best regards, ladis -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: ark3116.c: Remove unused TIOCSSERIAL case from ioctl
On Wed, 13 Dec 2017 11:17:28 +0100 Oliver Neukumwrote: > Am Mittwoch, den 13.12.2017, 12:30 +0300 schrieb Mikhail Zaytsev: > > +#define RS232_VENDOR 0x6547 > > +#define RS232_PRODUCT 0x0232 > > +#define IRDA_VENDOR 0x18ec > > +#define IRDA_PRODUCT 0x3118 > > > > /* usb timeout of 1 second */ > > #define ARK_TIMEOUT 1000 > > > > static const struct usb_device_id id_table[] = { > > - { USB_DEVICE(0x6547, 0x0232) }, > > - { USB_DEVICE(0x18ec, 0x3118) }, /* USB to IrDA adapter */ > > + { USB_DEVICE(RS232_VENDOR, RS232_PRODUCT) }, > > + { USB_DEVICE(IRDA_VENDOR, IRDA_PRODUCT) }, /* USB to IrDA adapter > > */ > > Hi, > > what is the purpose of this change? It just makes it harder to grep. > The constants are arbitrary and they are clearly device IDs. The constants are using in several places. I think the names easier to read. -- Mikhail -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: core: prevent malicious bNumInterfaces overflow
On Tue, Dec 12, 2017 at 02:25:13PM -0500, Alan Stern wrote: > A malicious USB device with crafted descriptors can cause the kernel > to access unallocated memory by setting the bNumInterfaces value too > high in a configuration descriptor. Although the value is adjusted > during parsing, this adjustment is skipped in one of the error return > paths. > > This patch prevents the problem by setting bNumInterfaces to 0 > initially. The existing code already sets it to the proper value > after parsing is complete. > > Signed-off-by: Alan Stern> Reported-by: Andrey Konovalov > CC: > > --- > > > [as1855] > > > drivers/usb/core/config.c |4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > Index: usb-4.x/drivers/usb/core/config.c > === > --- usb-4.x.orig/drivers/usb/core/config.c > +++ usb-4.x/drivers/usb/core/config.c > @@ -555,6 +555,9 @@ static int usb_parse_configuration(struc > unsigned iad_num = 0; > > memcpy(>desc, buffer, USB_DT_CONFIG_SIZE); > + nintf = nintf_orig = config->desc.bNumInterfaces; > + config->desc.bNumInterfaces = 0;// Adjusted later > + > if (config->desc.bDescriptorType != USB_DT_CONFIG || > config->desc.bLength < USB_DT_CONFIG_SIZE || > config->desc.bLength > size) { > @@ -568,7 +571,6 @@ static int usb_parse_configuration(struc > buffer += config->desc.bLength; > size -= config->desc.bLength; > > - nintf = nintf_orig = config->desc.bNumInterfaces; Ugh, I tried to find this place to do this, but couldn't. Nice job, I'll revert my patch and apply yours instead, thanks for this. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: ark3116.c: Remove unused TIOCSSERIAL case from ioctl
On Wed, Dec 13, 2017 at 12:30:04PM +0300, Mikhail Zaytsev wrote: > The patch removes unused TIOCSSERIAL case from ioctl. TIOCGSERIAL case > moves to the get_serial_info() function. Some magic numbers moves to > #define directives. You need to split logical changes up in separate patches, and there's at least three things being done here. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: ti_usb_3410_5052: Avoid goto in bulk in callback
On Mon, Dec 11, 2017 at 01:44:57PM +0100, Ladislav Michl wrote: > On Mon, Dec 11, 2017 at 12:52:46PM +0100, Johan Hovold wrote: > > On Mon, Dec 11, 2017 at 12:32:49PM +0100, Ladislav Michl wrote: > > > On Mon, Dec 11, 2017 at 11:10:35AM +0100, Johan Hovold wrote: > [snip] > > > > I'm afraid I don't consider this an improvement. I prefer using gotos > > > > for error paths, while keeping the success path out of the status > > > > switch. > > > > > > > > Furthermore, this isn't functionally equivalent as we'd not longer log > > > > an error for -EPIPE. > > > > > > Yes, you are right... Now, shouldn't we react somehow to stalled endpoint? > > > Tty side seems to be unaware of it. > > > > Recovering from a stalled endpoint is a bit involved, so for now we > > typically just log an error an bail out (forcing the user to reopen the > > port). This seems to work well enough as this condition should be rare. > > I just do not see this in code. I would expect pending tty I/O operation > would fail once USB device errors out with -EPIPE, so tty side consumer gets > notified about error. Either it is not there or I did not look hard enough :) No, we do not provide any error notification besides logging an error when an endpoint has been stalled (i.e. in case of a read, you would not receive any more data before the halt condition has been cleared). Given that there's been no reports about this being an issue (for the past ten years and that I can recall), this crude handling appears to suffice. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usbip: fix usbip bind writing random string after command in match_busid
usbip bind writes commands followed by random string when writing to match_busid attribute in sysfs, caused by using full variable size instead of string length. Signed-off-by: Juan Zea--- tools/usb/usbip/src/utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/usb/usbip/src/utils.c b/tools/usb/usbip/src/utils.c index 2b3d6d2..ea1a1af 100644 --- a/tools/usb/usbip/src/utils.c +++ b/tools/usb/usbip/src/utils.c @@ -42,7 +42,7 @@ int modify_match_busid(char *busid, int add) snprintf(command, SYSFS_BUS_ID_SIZE + 4, "del %s", busid); rc = write_sysfs_attribute(match_busid_attr_path, command, - sizeof(command)); + strlen(command)); if (rc < 0) { dbg("failed to write match_busid: %s", strerror(errno)); return -1; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: xhci: Add XHCI_TRUST_TX_LENGTH for Renesas uPD720201
On 12.12.2017 18:54, Ard Biesheuvel wrote: On 12 December 2017 at 16:47, Daniel Thompsonwrote: When plugging in a USB webcam I see the following message: xhci_hcd :04:00.0: WARN Successful completion on short TX: needs XHCI_TRUST_TX_LENGTH quirk? handle_tx_event: 913 callbacks suppressed All is quiet again with this patch (and I've done a fair but of soak testing with the camera since). Cc: Signed-off-by: Daniel Thompson I have been setting this quirk manually for ages on my kernel command line, for the same reason (MS HD Webcam), and with the same positive result, so Acked-by: Ard Biesheuvel Thanks, adding -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] xhci: Fix use-after-free in xhci debugfs
On 11.12.2017 00:14, Alexander Kappner wrote: Trying to read from debugfs after the system has resumed from hibernate causes a use-after-free and thus a protection fault. Steps to reproduce: Hibernate system, resume from hibernate, then run $ cat /sys/kernel/debug/usb/xhci/*/command-ring/enqueue dmesg below: [ 3902.765086] general protection fault: [#1] PREEMPT SMP [ 3902.765095] Modules linked in: ipheth nvidia_modeset(PO) iwlmvm mac80211 nvidia(PO) iwlwifi qmi_wwan cfg80211 thinkpad_acpi rfkill [ 3902.765118] CPU: 4 PID: 3591 Comm: cat Tainted: P O 4.14.0.1-12769-g1deab8c #1 [ 3902.765121] Hardware name: LENOVO 20ENCTO1WW/20ENCTO1WW, BIOS N1EET62W (1.35 ) 11/10/2016 [ 3902.765125] task: 88100263d040 task.stack: c90006e5 [ 3902.765136] RIP: 0010:xhci_trb_virt_to_dma.part.50+0x5/0x30 [ 3902.765140] RSP: 0018:c90006e53da8 EFLAGS: 00010286 [ 3902.765144] RAX: 88100a57e680 RBX: 8810032f1900 RCX: 881013800900 [ 3902.765147] RDX: RSI: 881000abf488 RDI: 0d0c0a0bb1637008 [ 3902.765151] RBP: c90006e53f00 R08: R09: [ 3902.765154] R10: 7ffcd73165e0 R11: 88100263d040 R12: 880bd5afcc00 [ 3902.765157] R13: 0002 R14: 0001 R15: 8810032f1900 [ 3902.765161] FS: 7f6aa6cec700() GS:881053d0() knlGS: [ 3902.765165] CS: 0010 DS: ES: CR0: 80050033 [ 3902.765168] CR2: 7f6aa6d08008 CR3: 0010033f9003 CR4: 003606e0 [ 3902.765172] DR0: DR1: DR2: [ 3902.765175] DR3: DR6: fffe0ff0 DR7: 0400 [ 3902.765178] Call Trace: [ 3902.765188] xhci_ring_enqueue_show+0x1e/0x40 [ 3902.765197] seq_read+0xdb/0x3a0 [ 3902.765204] ? __handle_mm_fault+0x5fb/0x1210 [ 3902.765211] full_proxy_read+0x4a/0x70 [ 3902.765219] __vfs_read+0x23/0x120 [ 3902.765228] vfs_read+0x8e/0x130 [ 3902.765235] SyS_read+0x42/0x90 [ 3902.765242] do_syscall_64+0x6b/0x290 [ 3902.765251] entry_SYSCALL64_slow_path+0x25/0x25 [ 3902.765256] RIP: 0033:0x7f6aa683cba0 [ 3902.765260] RSP: 002b:7ffcd7316818 EFLAGS: 0246 ORIG_RAX: [ 3902.765264] RAX: ffda RBX: 0002 RCX: 7f6aa683cba0 [ 3902.765267] RDX: 0002 RSI: 7f6aa6d09000 RDI: 0003 [ 3902.765270] RBP: 0002 R08: R09: [ 3902.765274] R10: 7ffcd73165e0 R11: 0246 R12: 7f6aa6d09000 [ 3902.765277] R13: 0003 R14: R15: 0002 [ 3902.765282] Code: 0f 44 c0 49 8b 04 24 52 48 c7 c2 30 cd cb 81 48 8d b0 98 00 00 00 31 c0 e8 39 e6 dc ff 58 e9 75 ff ff ff 0f 1f 00 0f 1f 44 00 00 <48> 8b 17 31 c0 48 39 f2 77 13 48 29 d6 48 81 fe ff 0f 00 00 77 [ 3902.765371] RIP: xhci_trb_virt_to_dma.part.50+0x5/0x30 RSP: c90006e53da8 [ 3902.765376] ---[ end trace 9ee53de1dccf7d8e ]--- The issue is caused by the xhci ring structures being reallocated when the system is resumed, but pointers to the old structures being retained in the debugfs files "private" field: (gdb) list *(xhci_ring_enqueue_show+0x1e) 0x8170403e is in xhci_ring_enqueue_show (drivers/usb/host/xhci-debugfs.c:168). 163 { 164 dma_addr_t dma; 165 struct xhci_ring*ring = s->private; 166 167 dma = xhci_trb_virt_to_dma(ring->enq_seg, ring->enqueue); 168 seq_printf(s, "%pad\n", ); 169 170 return 0; 171 } 172 The proposed patch fixes this issue by storing a pointer to the xhci_ring field in the xhci device structure in debugfs rather than directly storing a pointer to the xhci_ring. Signed-off-by: Alexander Kappner--- Thanks, that should work. I'll try it out, and shorten the commit message a bit. If everything works I'll apply it to my for-usb-linus branch -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: fix usbip bind writing random string after command in match_busid
On Wed, Dec 13, 2017 at 11:16:03AM +0100, Juan Zea wrote: > usbip bind writes commands followed by random string when writing to > match_busid attribute in sysfs, caused by using full variable size > instead of string length. > > Signed-off-by: Juan Zea> --- > tools/usb/usbip/src/utils.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) You forgot to cc: the maintainers of this file: $ ./scripts/get_maintainer.pl --file tools/usb/usbip/src/utils.c Valentina Manea (maintainer:USB OVER IP DRIVER) Shuah Khan (maintainer:USB OVER IP DRIVER) linux-usb@vger.kernel.org (open list:USB OVER IP DRIVER) linux-ker...@vger.kernel.org (open list) -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: ark3116.c: Remove unused TIOCSSERIAL case from ioctl
Am Mittwoch, den 13.12.2017, 12:30 +0300 schrieb Mikhail Zaytsev: > +#define RS232_VENDOR 0x6547 > +#define RS232_PRODUCT 0x0232 > +#define IRDA_VENDOR 0x18ec > +#define IRDA_PRODUCT 0x3118 > > /* usb timeout of 1 second */ > #define ARK_TIMEOUT 1000 > > static const struct usb_device_id id_table[] = { > - { USB_DEVICE(0x6547, 0x0232) }, > - { USB_DEVICE(0x18ec, 0x3118) }, /* USB to IrDA adapter */ > + { USB_DEVICE(RS232_VENDOR, RS232_PRODUCT) }, > + { USB_DEVICE(IRDA_VENDOR, IRDA_PRODUCT) }, /* USB to IrDA adapter */ Hi, what is the purpose of this change? It just makes it harder to grep. The constants are arbitrary and they are clearly device IDs. 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
[PATCH] usbip: fix usbip bind writing random string after command in match_busid
usbip bind writes commands followed by random string when writing to match_busid attribute in sysfs, caused by using full variable size instead of string length. Signed-off-by: Juan Zea--- tools/usb/usbip/src/utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/usb/usbip/src/utils.c b/tools/usb/usbip/src/utils.c index 2b3d6d2..ea1a1af 100644 --- a/tools/usb/usbip/src/utils.c +++ b/tools/usb/usbip/src/utils.c @@ -42,7 +42,7 @@ int modify_match_busid(char *busid, int add) snprintf(command, SYSFS_BUS_ID_SIZE + 4, "del %s", busid); rc = write_sysfs_attribute(match_busid_attr_path, command, - sizeof(command)); + strlen(command)); if (rc < 0) { dbg("failed to write match_busid: %s", strerror(errno)); return -1; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: configfs: Disallow empty function instance name
On 12/13/2017 10:29 AM, Felipe Balbi wrote: Hi, Alan Sternwrites: Krzysztof Opasiak writes: On 12/12/2017 01:31 PM, Felipe Balbi wrote: Hi, Krzysztof Opasiak writes: Every function should have a type and instance name. Unfortunately in most cases instance name was left unused and unchecked. This may lead to situations like FunctionFS device name identified by "" or some misleading debug messages from TCM like: tcm: Activating To avoid this let's add a check that instance name should have at least one character. Reported-by: Stefan Agner Signed-off-by: Krzysztof Opasiak --- drivers/usb/gadget/configfs.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c index aeb9f3c40521..bdc9ec597d6a 100644 --- a/drivers/usb/gadget/configfs.c +++ b/drivers/usb/gadget/configfs.c @@ -548,6 +548,11 @@ static struct config_group *function_make( *instance_name = '\0'; instance_name++; + if (*instance_name == '\0') { + pr_err("Instance name (after .) should not be empty\n"); + return ERR_PTR(-EINVAL); + } aand just like that you break potentially existing scripts :-) We need to find a better way of enforcing a name which doesn't break existing users. I'm really open for suggestions how to enforce this without breaking those scripts ;) The origin of this commit is github issue for libusbgx[1]. So the problem is that library allows to create a function with empty name (because I mistakenly assumed that kernel rejects this) but then it is unable to reinitialize the ConfigFS state because there is a check that disallows this. From my point of view I'd be happy to disallow empty names because it causes some problems (f_fs) or weird debug messages (f_tcm) so is generally inconvenient and seems to be unintentional. But I would like to keep this consistent with kernel policy. I think we need to first fix libusbgx to prevent empty names. I don't want to be the one hearing from Linus that "we don't break userspace". It's clear that empty names shouldn't be allowed, but they _are_ allowed as of today, so how can we cause a regression all of a sudden? Alan, Greg, any suggestions? You could do some silly name munging, like changing an empty name to " " whenever you encounter it. Or adding an '_' to the end of any name that consists of nothing but '_' characters. Hmm, that could be done. So everytime userspace gives us an empty name, we would convert to '_'. That still doesn't solve the problems of mounting functionfs, though. But I guess there's nothing that can be done in that case. How is it different from disallowing empty name? It may also cause some "broken" scripts stop working. Isn't it going to introduce some weird problems like: mkdir g1/function/ffs._ mkdir g2/function/ffs. -EBUSY Best regards, -- Krzysztof Opasiak Samsung R 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] usb: gadget: configfs: Disallow empty function instance name
Hi, Alan Sternwrites: >> Krzysztof Opasiak writes: >> > On 12/12/2017 01:31 PM, Felipe Balbi wrote: >> >> >> >> Hi, >> >> >> >> Krzysztof Opasiak writes: >> >>> Every function should have a type and instance name. >> >>> Unfortunately in most cases instance name was left unused and unchecked. >> >>> This may lead to situations like FunctionFS device name identified by "" >> >>> or some misleading debug messages from TCM like: >> >>> >> >>> tcm: Activating >> >>> >> >>> To avoid this let's add a check that instance name should have at least >> >>> one character. >> >>> >> >>> Reported-by: Stefan Agner >> >>> Signed-off-by: Krzysztof Opasiak >> >>> --- >> >>> drivers/usb/gadget/configfs.c | 5 + >> >>> 1 file changed, 5 insertions(+) >> >>> >> >>> diff --git a/drivers/usb/gadget/configfs.c >> >>> b/drivers/usb/gadget/configfs.c >> >>> index aeb9f3c40521..bdc9ec597d6a 100644 >> >>> --- a/drivers/usb/gadget/configfs.c >> >>> +++ b/drivers/usb/gadget/configfs.c >> >>> @@ -548,6 +548,11 @@ static struct config_group *function_make( >> >>> *instance_name = '\0'; >> >>> instance_name++; >> >>> >> >>> +if (*instance_name == '\0') { >> >>> +pr_err("Instance name (after .) should not be empty\n"); >> >>> +return ERR_PTR(-EINVAL); >> >>> +} >> >> >> >> aand just like that you break potentially existing scripts :-) >> >> >> >> We need to find a better way of enforcing a name which doesn't break >> >> existing users. >> > >> > I'm really open for suggestions how to enforce this without breaking >> > those scripts ;) >> > >> > The origin of this commit is github issue for libusbgx[1]. >> > So the problem is that library allows to create a function with empty >> > name (because I mistakenly assumed that kernel rejects this) but then it >> > is unable to reinitialize the ConfigFS state because there is a check >> > that disallows this. From my point of view I'd be happy to disallow >> > empty names because it causes some problems (f_fs) or weird debug >> > messages (f_tcm) so is generally inconvenient and seems to be >> > unintentional. But I would like to keep this consistent with kernel policy. >> >> I think we need to first fix libusbgx to prevent empty names. >> >> I don't want to be the one hearing from Linus that "we don't break >> userspace". It's clear that empty names shouldn't be allowed, but they >> _are_ allowed as of today, so how can we cause a regression all of a >> sudden? >> >> Alan, Greg, any suggestions? > > You could do some silly name munging, like changing an empty name to > " " whenever you encounter it. Or adding an '_' to the end of any name > that consists of nothing but '_' characters. Hmm, that could be done. So everytime userspace gives us an empty name, we would convert to '_'. That still doesn't solve the problems of mounting functionfs, though. But I guess there's nothing that can be done in that case. -- balbi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: serial: ark3116.c: Remove unused TIOCSSERIAL case from ioctl
The patch removes unused TIOCSSERIAL case from ioctl. TIOCGSERIAL case moves to the get_serial_info() function. Some magic numbers moves to #define directives. Signed-off-by: Mikhail Zaytsev--- drivers/usb/serial/ark3116.c | 54 ++-- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c index 23d46ef87..f45f69e18 100644 --- a/drivers/usb/serial/ark3116.c +++ b/drivers/usb/serial/ark3116.c @@ -36,13 +36,19 @@ #define DRIVER_DESC "USB ARK3116 serial/IrDA driver" #define DRIVER_DEV_DESC "ARK3116 RS232/IrDA" #define DRIVER_NAME "ark3116" +#define ARK3116_BAUDRATE 460800 + +#define RS232_VENDOR 0x6547 +#define RS232_PRODUCT 0x0232 +#define IRDA_VENDOR 0x18ec +#define IRDA_PRODUCT 0x3118 /* usb timeout of 1 second */ #define ARK_TIMEOUT 1000 static const struct usb_device_id id_table[] = { - { USB_DEVICE(0x6547, 0x0232) }, - { USB_DEVICE(0x18ec, 0x3118) }, /* USB to IrDA adapter */ + { USB_DEVICE(RS232_VENDOR, RS232_PRODUCT) }, + { USB_DEVICE(IRDA_VENDOR, IRDA_PRODUCT) }, /* USB to IrDA adapter */ { }, }; MODULE_DEVICE_TABLE(usb, id_table); @@ -50,8 +56,8 @@ MODULE_DEVICE_TABLE(usb, id_table); static int is_irda(struct usb_serial *serial) { struct usb_device *dev = serial->dev; - if (le16_to_cpu(dev->descriptor.idVendor) == 0x18ec && - le16_to_cpu(dev->descriptor.idProduct) == 0x3118) + if (le16_to_cpu(dev->descriptor.idVendor) == IRDA_VENDOR && + le16_to_cpu(dev->descriptor.idProduct) == IRDA_PRODUCT) return 1; return 0; } @@ -397,31 +403,35 @@ static int ark3116_open(struct tty_struct *tty, struct usb_serial_port *port) return result; } +static int ark3116_get_serial_info(struct usb_serial_port *port, + unsigned long arg) +{ + struct serial_struct ser; + + memset(, 0, sizeof(ser)); + + ser.type = PORT_16654; + ser.line = port->minor; + ser.port = port->port_number; + ser.custom_divisor = 0; + ser.baud_base = ARK3116_BAUDRATE; + + if (copy_to_user((void __user *)arg, , sizeof(ser))) + return -EFAULT; + + return 0; +} + static int ark3116_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg) { struct usb_serial_port *port = tty->driver_data; - struct serial_struct serstruct; - void __user *user_arg = (void __user *)arg; switch (cmd) { case TIOCGSERIAL: - /* XXX: Some of these values are probably wrong. */ - memset(, 0, sizeof(serstruct)); - serstruct.type = PORT_16654; - serstruct.line = port->minor; - serstruct.port = port->port_number; - serstruct.custom_divisor = 0; - serstruct.baud_base = 460800; - - if (copy_to_user(user_arg, , sizeof(serstruct))) - return -EFAULT; - - return 0; - case TIOCSSERIAL: - if (copy_from_user(, user_arg, sizeof(serstruct))) - return -EFAULT; - return 0; + return ark3116_get_serial_info(port, arg); + default: + break; } return -ENOIOCTLCMD; -- 2.11.0 -- 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: [BUG] kaweth: a possible sleep-in-atomic bug in kaweth_start_xmit
Am Mittwoch, den 13.12.2017, 16:57 +0800 schrieb Jia-Ju Bai: > According to drivers/net/usb/kaweth.c, the driver may sleep under a > spinlock. > The function call path is: > kaweth_start_xmit (acquire the spinlock) >kaweth_async_set_rx_mode > kaweth_control >kaweth_internal_control_msg > usb_start_wait_urb >wait_event_timeout --> may sleep >usb_kill_urb --> may sleep > > I do not find a good way to fix it, so I only report. > This possible bug is found by my static analysis tool (DSAC) and checked > by my code review. > Hi, thanks for reporting. I need to get out my old test device. It will take a few days. The obvious fix would be to set this filter only on initialization. Unfortunately this needs to be tested. 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
[BUG] kaweth: a possible sleep-in-atomic bug in kaweth_start_xmit
According to drivers/net/usb/kaweth.c, the driver may sleep under a spinlock. The function call path is: kaweth_start_xmit (acquire the spinlock) kaweth_async_set_rx_mode kaweth_control kaweth_internal_control_msg usb_start_wait_urb wait_event_timeout --> may sleep usb_kill_urb --> may sleep I do not find a good way to fix it, so I only report. This possible bug is found by my static analysis tool (DSAC) and checked by my code review. Thanks, Jia-Ju Bai -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: serial: ark3116.c: Remove unused TIOCSSERIAL case from ioctl
The patch removes unused TIOCSSERIAL case from ioctl. TIOCGSERIAL case moves to the get_serial_info() function. Any magic numbers moves to #define directives. Signed-off-by: Mikhail Zaytsev--- drivers/usb/serial/ark3116.c | 54 ++-- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c index 23d46ef87..f45f69e18 100644 --- a/drivers/usb/serial/ark3116.c +++ b/drivers/usb/serial/ark3116.c @@ -36,13 +36,19 @@ #define DRIVER_DESC "USB ARK3116 serial/IrDA driver" #define DRIVER_DEV_DESC "ARK3116 RS232/IrDA" #define DRIVER_NAME "ark3116" +#define ARK3116_BAUDRATE 460800 + +#define RS232_VENDOR 0x6547 +#define RS232_PRODUCT 0x0232 +#define IRDA_VENDOR 0x18ec +#define IRDA_PRODUCT 0x3118 /* usb timeout of 1 second */ #define ARK_TIMEOUT 1000 static const struct usb_device_id id_table[] = { - { USB_DEVICE(0x6547, 0x0232) }, - { USB_DEVICE(0x18ec, 0x3118) }, /* USB to IrDA adapter */ + { USB_DEVICE(RS232_VENDOR, RS232_PRODUCT) }, + { USB_DEVICE(IRDA_VENDOR, IRDA_PRODUCT) }, /* USB to IrDA adapter */ { }, }; MODULE_DEVICE_TABLE(usb, id_table); @@ -50,8 +56,8 @@ MODULE_DEVICE_TABLE(usb, id_table); static int is_irda(struct usb_serial *serial) { struct usb_device *dev = serial->dev; - if (le16_to_cpu(dev->descriptor.idVendor) == 0x18ec && - le16_to_cpu(dev->descriptor.idProduct) == 0x3118) + if (le16_to_cpu(dev->descriptor.idVendor) == IRDA_VENDOR && + le16_to_cpu(dev->descriptor.idProduct) == IRDA_PRODUCT) return 1; return 0; } @@ -397,31 +403,35 @@ static int ark3116_open(struct tty_struct *tty, struct usb_serial_port *port) return result; } +static int ark3116_get_serial_info(struct usb_serial_port *port, + unsigned long arg) +{ + struct serial_struct ser; + + memset(, 0, sizeof(ser)); + + ser.type = PORT_16654; + ser.line = port->minor; + ser.port = port->port_number; + ser.custom_divisor = 0; + ser.baud_base = ARK3116_BAUDRATE; + + if (copy_to_user((void __user *)arg, , sizeof(ser))) + return -EFAULT; + + return 0; +} + static int ark3116_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg) { struct usb_serial_port *port = tty->driver_data; - struct serial_struct serstruct; - void __user *user_arg = (void __user *)arg; switch (cmd) { case TIOCGSERIAL: - /* XXX: Some of these values are probably wrong. */ - memset(, 0, sizeof(serstruct)); - serstruct.type = PORT_16654; - serstruct.line = port->minor; - serstruct.port = port->port_number; - serstruct.custom_divisor = 0; - serstruct.baud_base = 460800; - - if (copy_to_user(user_arg, , sizeof(serstruct))) - return -EFAULT; - - return 0; - case TIOCSSERIAL: - if (copy_from_user(, user_arg, sizeof(serstruct))) - return -EFAULT; - return 0; + return ark3116_get_serial_info(port, arg); + default: + break; } return -ENOIOCTLCMD; -- 2.11.0 -- 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: Add your device DWM-222 to a proper driver
On 12/13/2017 14:18, martin.er...@centrum.cz wrote: Dear linux development, please to add device to a proper driver. I wrote this requirement according log list message. My USB modem is D-link DWM-222 Vendor 3G Standard Default Id Modem Id User Modem Storage Diag Protocol(s) DWM-222 A1 LTE CAT4 2001:ab00 2001:7e35 2 1 5 0 serial & qmi Dear linux user, DWM-222 A1 with USB Id 2001:7e35 was added to the option driver 4-5 month ago so it is either a matter of loading the driver or updating your kernel to a more recent version. rgds Lars -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html