Re: [PATCH] usbip: tools: Fix read_usb_vudc_device() error path handling
On Wed, Oct 16, 2019 at 01:38:25PM +0900, GwanYeong Kim wrote: > On Tue, 15 Oct 2019 17:14:32 -0600 > shuah wrote: > > > On 10/15/19 7:14 AM, GwanYeong Kim wrote: > > > cannot be less than 0 - fread() returns 0 on error. > > > > > > Signed-off-by: GwanYeong Kim > > > --- > > > tools/usb/usbip/libsrc/usbip_device_driver.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c > > > b/tools/usb/usbip/libsrc/usbip_device_driver.c index > > > 051d7d3f443b..49760b98aabc 100644 --- > > > a/tools/usb/usbip/libsrc/usbip_device_driver.c +++ > > > b/tools/usb/usbip/libsrc/usbip_device_driver.c @@ -79,7 +79,7 @@ > > > int read_usb_vudc_device(struct udev_device *sdev, struct > > > usbip_usb_device *dev) if (!fd) return -1; > > > ret = fread((char *) &descr, sizeof(descr), 1, fd); > > > - if (ret < 0) > +if (ret != sizeof(descr)) > > > > Are you sure this check is correct? fread() returns the number > > of elements read, # elements = 1 in this case. > > Thank you for pointing this out. > Sorry for my mistake. > > > fread() returns 0 when size or # of elements is 0 and in other > > error cases it will return < # of elements. I would think you > > want to check ret != 1 here. > > You're right. > This should be changed to "ret != 1". > > Should I send a new patch? If you want to have it applied, yes. thanks, greg k-h
Re: [PATCH v2] usb: usbfs: Suppress problematic bind and unbind uevents.
On Fri, Oct 11, 2019 at 01:55:18PM +0200, Ingo Rohloff wrote:
> commit 1455cf8dbfd0 ("driver core: emit uevents when device is bound
> to a driver") added bind and unbind uevents when a driver is bound or
> unbound to a physical device.
>
> For USB devices which are handled via the generic usbfs layer (via
> libusb for example), this is problematic:
> Each time a user space program calls
>ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
> and then later
>ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
> The kernel will now produce a bind or unbind event, which does not
> really contain any useful information.
>
> This allows a user space program to run a DoS attack against programs
> which listen to uevents (in particular systemd/eudev/upowerd):
> A malicious user space program just has to call in a tight loop
>
>ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
>ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
>
> With this loop the malicious user space program floods the kernel and
> all programs listening to uevents with tons of bind and unbind
> events.
>
> This patch suppresses uevents for ioctls USBDEVFS_CLAIMINTERFACE and
> USBDEVFS_RELEASEINTERFACE.
>
> Signed-off-by: Ingo Rohloff
> ---
>
> Notes:
> v2:
> Patch only single file (devio.c), try to only suppress uevents while
> usb_driver_claim_interface/usb_driver_release_interface are called.
> Try to restore old state of dev->kobj.uevent_suppress.
Thanks for cleaning this up. It looks much nicer now. I've queued it
up in my tree, let's see how testing goes :)
thanks,
greg k-h
Re: [PATCH] usb: usbfs: Suppress problematic bind and unbind uevents.
On Thu, Oct 10, 2019 at 06:48:00PM +0200, Ingo Rohloff wrote:
> commit 1455cf8dbfd0 ("driver core: emit uevents when device is bound
> to a driver") added bind and unbind uevents when a driver is bound or
> unbound to a physical device.
>
> For USB devices which are handled via the generic usbfs layer (via
> libusb for example), this is problematic:
> Each time a user space program calls
>ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
> and then later
>ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
> The kernel will now produce a bind or unbind event, which does not
> really contain any useful information.
>
> This allows a user space program to run a DoS attack against programs
> which listen to uevents (in particular systemd/eudev/upowerd):
> A malicious user space program just has to call in a tight loop
>
>ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
>ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
>
> With this loop the malicious user space program floods the kernel and
> all programs listening to uevents with tons of bind and unbind
> events.
>
> This patch suppresses uevents for ioctls USBDEVFS_CLAIMINTERFACE and
> USBDEVFS_RELEASEINTERFACE.
>
> Signed-off-by: Ingo Rohloff
> ---
> drivers/usb/core/devio.c | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
I am guessing this is a new version of a previously-submitted patch? If
so, you need to include a "version" number on it, and put what you
changed below the --- line. The kernel documentation should explain how
to do this, if not, please let us know.
Please fix this up and resend.
thanks,
greg k-h
Re: [PATCH] USB: usbfs: Suppress emission of uevents for interfaces handled via usbfs
On Wed, Oct 09, 2019 at 12:38:35PM +0200, Ingo Rohloff wrote:
> >From 17d1e75543e26cfe702e7f5b0d4e07e0e45e5250 Mon Sep 17 00:00:00 2001
> From: Ingo Rohloff
> Date: Tue, 8 Oct 2019 20:27:57 +0200
> Subject: [PATCH] USB: usbfs: Suppress emission of uevents for interfaces
> handled via usbfs.
No need for this in the changelog body :)
> commit 1455cf8dbfd0
> ("driver core: emit uevents when device is bound to a driver")
> added bind/unbind uevents when a driver is bound/unbound
> to a physical device.
You can wrap the line a bit nicer:
commit 1455cf8dbfd0 ("driver core: emit uevents when device is bound to
a driver") added bind/unbind uevents when a driver is bound/unbound to a
physical device.
> For USB devices which are handled via the generic usbfs layer
> (via libusb for example), this is problematic:
> Each time a user space program calls
>ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
> and then later
>ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
> The kernel will now produce a bind/unbind event,
> which does not really contain any useful information.
>
> This allows a user space program to run a DoS attack against
> programs which listen to uevents (in particular systemd/eudev/upowerd):
> A malicious user space program just has to call in a tight loop
>
>ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
>ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
>
> With this loop the malicious user space program floods
> the kernel and all programs listening to uevents with
> tons of bind/unbind events.
>
> This patch suppresses uevents for interfaces claimed via usbfs.
>
> Signed-off-by: Ingo Rohloff
> ---
> drivers/usb/core/devio.c | 7 ++-
> drivers/usb/core/driver.c | 2 ++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 3f899552f6e3..a1af1d9b2ae7 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -764,8 +764,13 @@ static int claimintf(struct usb_dev_state *ps, unsigned
> int ifnum)
> intf = usb_ifnum_to_if(dev, ifnum);
> if (!intf)
> err = -ENOENT;
> - else
> + else {
> + /* suppress uevents for devices handled by usbfs */
> + dev_set_uevent_suppress(&intf->dev, 1);
> err = usb_driver_claim_interface(&usbfs_driver, intf, ps);
> + if (err != 0)
Did checkpatch let this go through? Shouldn't that be:
if (err)
And did you send this patch twice?
Anyway, if you fix those minor things up, it looks good to me.
thanks,
greg k-h
Re: Unknown symbol errors in usb storage driver
On Tue, Oct 08, 2019 at 09:53:16PM +0200, Heiner Kallweit wrote:
> Since a while I see the following. I didn't bisect yet, maybe issue is caused
> by
> 32bca2df7da2 ("usb-storage: export symbols in USB_STORAGE namespace")?
>
> DEPMOD 5.4.0-rc2-next-20191008+
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_probe1
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_reset_resume
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_pre_reset
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_host_template_init
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_probe2
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_disconnect
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_control_msg
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_post_reset
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_bulk_transfer_buf
>
It's a depmod bug, see lkml for the discussion and potential fixes.
People are working on it :)
thanks,
greg k-h
Re: [PATCH] usbfs: Suppress uevents for claiminterface/releaseinterface
On Tue, Oct 08, 2019 at 09:32:10PM +0200, Ingo Rohloff wrote:
> Hello,
>
> With recent Ubuntu 18/Linux Mint 19.2 etc, lots of user space programs
> (in particular systemd/eudev/upowerd) have problems with the "BIND/UNBIND"
> events produced since kernel 4.13.
> Some problems are described, when googling for
> linux "usb" "bind event"
>
> Now this might be blamed on these particular user space programs.
> But: This also means that programs accessing a USB device via the generic
> usbfs layer can easily flood the kernel and all user space programs listening
> to uevents with tons of BIND/UNBIND events by calling
>
> ioctl(usbfd, USBDEVFS_CLAIMINTERFACE, &intf);
> ioctl(usbfd, USBDEVFS_RELEASEINTERFACE, &intf);
>
> in a tight loop.
> Of course this is an extreme example, but I have a use case where exactly
> this happens (running Linux Mint 19.2).
> The result is that "systemd-udev" needs > 100% CPU and
> upowerd spams the system log with messages about "bind/unbind" events.
>
> I am also not sure if these particular bind/unbind events contain any useful
> information; these events just mean an arbitrary user space program has
> bound/unbound from a particular USB interface.
>
> The following patch tries to suppress emission of uevents
> for USB interfaces which are claimed/released via usbfs.
>
> I am not sure if this is the right way to do it, but at least
> it seems to do what I intended...
>
> with best regards
> Ingo Rohloff
> From 57970b0a5a36809ddb8f15687c18ca2147dc73bd Mon Sep 17 00:00:00 2001
> From: Ingo Rohloff
> Date: Tue, 8 Oct 2019 20:27:57 +0200
> Subject: [PATCH] USB: usbfs: Suppress emission of uevents for interfaces
> handled via usbfs.
>
> commit 1455cf8dbfd0
> ("driver core: emit uevents when device is bound to a driver")
> added BIND and UNBIND events when a driver is bound/unbound
> to a physical device.
>
> For USB devices which are handled via the generic usbfs layer
> (via libusb for example). This is problematic:
> Each time a user space program calls
>ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
> and then later
>ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
> The kernel will now produce a BIND/UNBIND event, which
> does not really contain any useful information.
>
> Additionally this easily allows a user space program to run a
> DoS attack against programs which listen to uevents
> (in particular systemd/eudev/upowerd):
> A malicious user space program just has to call in a tight loop
>
> ioctl(usbfd, USBDEVFS_CLAIMINTERFACE, &intf);
> ioctl(usbfd, USBDEVFS_RELEASEINTERFACE, &intf);
>
> with this loop the malicious user space program floods
> the kernel and all programs listening to uevents with
> tons of BIND/UNBIND events.
>
> The following patch tries to suppress uevents for interfaces
> claimed via usbfs.
> ---
> drivers/usb/core/devio.c | 7 ++-
> drivers/usb/core/driver.c | 2 ++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 3f899552f6e3..a1af1d9b2ae7 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -764,8 +764,13 @@ static int claimintf(struct usb_dev_state *ps, unsigned
> int ifnum)
> intf = usb_ifnum_to_if(dev, ifnum);
> if (!intf)
> err = -ENOENT;
> - else
> + else {
> + /* suppress uevents for devices handled by usbfs */
> + dev_set_uevent_suppress(&intf->dev, 1);
> err = usb_driver_claim_interface(&usbfs_driver, intf, ps);
> + if (err != 0)
> + dev_set_uevent_suppress(&intf->dev, 0);
> + }
> if (err == 0)
> set_bit(ifnum, &ps->ifclaimed);
> return err;
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 2b27d232d7a7..6a15bc5c2869 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -594,6 +594,8 @@ void usb_driver_release_interface(struct usb_driver
> *driver,
>*/
> if (device_is_registered(dev)) {
> device_release_driver(dev);
> + /* make sure we allow uevents again */
> + dev_set_uevent_suppress(dev, 0);
> } else {
> device_lock(dev);
> usb_unbind_interface(dev);
> --
> 2.17.1
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- Your patch was attached, please place it inline so that it can be
applied directly from the email mess
Re: CREAD ignored by almost all USB serial drivers
On Mon, Oct 07, 2019 at 03:18:52PM +0200, Harald Welte wrote: > Hi Greg, > > On Mon, Oct 07, 2019 at 01:06:33PM +0200, Greg KH wrote: > > On Sat, Sep 28, 2019 at 10:49:55PM +0200, Harald Welte wrote: > > > It seems that a lot of Linux kernel USB serial device drivers are > > > ignoring the CREAD setting of termios.c_cflag. > > > > You just discovered something that has been broken since the first > > usb-serial driver was written, all those years ago :) > > Amazing and frightening at the same time. I would have expected > somebody had built something like a hardware test fixture to test those > drivers during all those decades. Something like a "well-known" serial > device as the tester, attaching to all the handshake etc. lines of the > "device under test" and then running through many of the possible > settings from HW to SW flow control, baud rate, parity, number of stop > bits, break characters, etc. I had a tester that was just a loopback device. Amazing what just a simple device like that found over time, these things barely work :) Anyway, yes, a "real" test setup like this would be great to have, I don't know of one around anywhere, and if you use it, I am sure you will find lots of issues. Turns out almost all usb-serial devices are used for "basic" rx/tx stuff, all of the "fancy" serial things just are not all that common anymore given that serial is not the primary way data is transferred anymore. > I have no shortage of projects to work on, but if somebody else was > interested to host a physical setup with many different [USB] serial > ports and some CI around, I might be tempted to build the actual tester > hardware and some test suite software for it. Would be nice to see :) > > I did add support for this to the digi driver, as you saw, as the > > hardware had support for it. For everything else, they are all just > > dumb uarts and do not expose that information to the host computer and I > > think everyone just forgot about this option. > > I am aware that many USB serial adapters are rather "dumb", hence my > suggestion > to add a related option to the core usb-serial, or even to the core tty/serial > layer: If the driver doesn't process CREAD, simply discard the received bytes > at this common/shared layer. Yes, that could be done in the usb-serial core probably. > > Given that you are the first to report it that I can think of, I don't > > think very many people use half-duplex protocols with a shared Rx/Tx > > (which is crazy anyway...) > > Every smart card interface [1] on this planet, including every SIM card in > every > mobile phone uses such a setup: asynchronous half-duplex communication with > shared Rx/Tx. Sure, not many people attach something like that to a > USB-Serial > adapter (as oppose to a USB-CCID reader), but I just wanted to clarify > it's not as obscure as one may think. You can actually buy > ultra-low-cost SIM card readers built that way. > > Also, I would assume that RS-485 is still used in lots of technology, > including e.g. DMX and industrial control systems. Unless you go for a > rather obscure 4-wire RS-485, then you have the same half-duplex > operation on shared medium. Please note that USB-RS485 adapters exist, > using a variety of USB-serial chipsets. 485 just got added to some tty drivers recently, so yes, it is used, but not all that common it seems. Or maybe it is and everyone "knows" to buy the one good device that supports it. thanks, greg k-h
Re: [PATCH v4] usb: hub: Check device descriptor before resusciation
On Tue, Oct 08, 2019 at 06:10:22PM +0200, David Heinzelmann wrote: > On Tue, Oct 08, 2019 at 02:55:46PM +0200, Greg KH wrote: > > What happened to Alan's ack? > > > > I'm not sure I'm allowed to add someone else's acked-by tag? If they provide it, and you don't change anything, then yes, you can. > If so I will sent v5. Please do. thanks, greg k-h
Re: [PATCH v4] usb: hub: Check device descriptor before resusciation
On Tue, Oct 08, 2019 at 10:09:01AM +0200, David Heinzelmann wrote: > If a device connected to an xHCI host controller disconnects from the USB bus > and then reconnects, e.g. triggered by a firmware update, then the host > controller automatically activates the connection and the port is enabled. The > implementation of hub_port_connect_change() assumes that if the port is > enabled then nothing has changed. There is no check if the USB descriptors > have changed. As a result, the kernel's internal copy of the descriptors ends > up being incorrect and the device doesn't work properly anymore. > > The solution to the problem is for hub_port_connect_change() always to > check whether the device's descriptors have changed before resuscitating > an enabled port. > > Signed-off-by: David Heinzelmann > --- > Changes in v4: > - changed commit description > Changes in v3: > - changed commit message and description > - fix code style > Changes in v2: > - fix logic error to handle return code from usb_get_device_descriptor() >properly > - fix line endings > --- > drivers/usb/core/hub.c | 196 +++-- > 1 file changed, 111 insertions(+), 85 deletions(-) What happened to Alan's ack? v5? thanks, greg k-h
Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
On Mon, Oct 07, 2019 at 10:01:47AM -0400, Alan Stern wrote: > On Mon, 7 Oct 2019, David Heinzelmann wrote: > > > Hi, > > > > I hope it all fits now. > > > > David > > > > > > From 8517ecfac0175aebba03bb0868dde652bc3c36e5 Mon Sep 17 00:00:00 2001 > > From: David Heinzelmann > > Date: Fri, 4 Oct 2019 12:28:36 +0200 > > Subject: [PATCH v4] usb: hub: Check device descriptor before resusciation > > > > If a device connected to an xHCI host controller disconnects from the USB > > bus > > and then reconnects, e.g. triggered by a firmware update, then the host > > controller automatically activates the connection and the port is enabled. > > The > > implementation of hub_port_connect_change() assumes that if the port is > > enabled then nothing has changed. There is no check if the USB descriptors > > have changed. As a result, the kernel's internal copy of the descriptors > > ends > > up being incorrect and the device doesn't work properly anymore. > > > > The solution to the problem is for hub_port_connect_change() always to > > check whether the device's descriptors have changed before resuscitating > > an enabled port. > > > > Signed-off-by: David Heinzelmann > > Acked-by: Alan Stern David, can you resend this in a format that I can apply it in? thanks, greg k-h
Re: CREAD ignored by almost all USB serial drivers
On Sat, Sep 28, 2019 at 10:49:55PM +0200, Harald Welte wrote: > [Copied on requst from https://bugzilla.kernel.org/show_bug.cgi?id=205033] > > It seems that a lot of Linux kernel USB serial device drivers are > ignoring the CREAD setting of termios.c_cflag. > > The man page is quite clear: >CREAD Enable receiver. > > The glibc man page at > https://ftp.gnu.org/old-gnu/Manuals/glibc-2.2.3/html_chapter/libc_17.html > states: > "Macro: tcflag_t CREAD >If this bit is set, input can be read from the terminal. >Otherwise, input is discarded when it arrives." > > When I mask this bit and then call tcsetattr(), I'm still receiving > input characters, at least on a pl2303 USB UART. Looking at the source > code of drivers/usb/serial/, the *only* driver appearing to respect > CREAD is digi_acceleport.c. All others seem to ignore it. To the > contrary, most classic serial drivers in drivers/tty/serial seem to > implement it. > > In absence of low-level support in individual drivers to actually > disable the receiver in hardware, I would have at least expected the > core tty/serial layer to drop/discard any characters received by the > hardware while CREAD is not set. But that also doesn't appear to be the > case. > > What's even more worrying is that the tcsetattr() call succeeds, i.e. it > is a silent error. I would expect the kernel to either implement the > functionalty in one way or another, or simply return tcsetattr() with > an error if an unsupported combination (i.e. CFLAG not set) is > configured. > > This is not a theoretical issue. Anyone implementing a half-duplex > protocol with shared Rx and Tx line will face the same issue. > > Am I missing something here? Please don't tell me that I just > discovered something that's broken for some 20-odd years, or at the very > least as far as normal linux.git history reaches back :/ You just discovered something that has been broken since the first usb-serial driver was written, all those years ago :) I did add support for this to the digi driver, as you saw, as the hardware had support for it. For everything else, they are all just dumb uarts and do not expose that information to the host computer and I think everyone just forgot about this option. Given that you are the first to report it that I can think of, I don't think very many people use half-duplex protocols with a shared Rx/Tx (which is crazy anyway...) > Please keep me in Cc of any responses, I'm not subscribed to linux-usb. Is there a specific usb-serial driver that you are using that you want to have this support added to? thanks, greg k-h
Re: [PATCH] drivers: musb: removed unused status variable
On Wed, Oct 02, 2019 at 11:09:13PM +0530, aliasgar.surti...@gmail.com wrote: > From: Aliasgar Surti > > Status variable is initialized and returned without updating it's value. > Removed status variable and returned value directly. > > Signed-off-by: Aliasgar Surti > --- > drivers/usb/musb/musb_gadget.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c > index ffe462a657b1..2cb31fc0cd60 100644 > --- a/drivers/usb/musb/musb_gadget.c > +++ b/drivers/usb/musb/musb_gadget.c > @@ -1085,7 +1085,6 @@ static int musb_gadget_disable(struct usb_ep *ep) > u8 epnum; > struct musb_ep *musb_ep; > void __iomem*epio; > - int status = 0; > > musb_ep = to_musb_ep(ep); > musb = musb_ep->musb; > @@ -1118,7 +1117,7 @@ static int musb_gadget_disable(struct usb_ep *ep) > > musb_dbg(musb, "%s", musb_ep->end_point.name); > > - return status; > + return 0; > } > > /* > -- > 2.17.1 > Someone else sent this same patch 2 hours before you did, sorry :(
Re: Driver for something that's neither a device nor an interface driver?
On Sat, Sep 28, 2019 at 02:37:21PM +0200, Bastien Nocera wrote: > On Sat, 2019-09-28 at 14:18 +0200, Greg KH wrote: > > Again, the power_supply api is for power going the other way in the > > system. That's not an "existing clearly defined API in kernel > > space". > > No it isn't, not since 2011. > > commit 25a0bc2dfc2ea732f40af2dae52426ead66ae76e > Author: Jeremy Fitzhardinge > Date: Wed Dec 7 11:24:20 2011 -0800 > > power_supply: add SCOPE attribute to power supplies > > This adds a "scope" attribute to a power_supply, which indicates how > much of the system it powers. It appears in sysfs as "scope" or in > the uevent file as POWER_SUPPLY_SCOPE=. There are presently three > possible values: > Unknown - unknown power topology > System - the power supply powers the whole system > Device - it powers a specific device, or tree of devices > > A power supply which doesn't have a "scope" attribute should be assumed to > have "System" scope. > > In general, usermode should assume that loss of all System-scoped power > supplies will power off the whole system, but any single one is sufficient > to power the system. > > Signed-off-by: Jeremy Fitzhardinge > Cc: Richard Hughes > Ah, ok, my fault, then ok, let's see how your kernel driver ties into this then. thanks, greg k-h
Re: Driver for something that's neither a device nor an interface driver?
On Sat, Sep 28, 2019 at 12:42:21PM +0200, Bastien Nocera wrote: > On Sat, 2019-09-28 at 09:39 +0200, Greg KH wrote: > > On Fri, Sep 27, 2019 at 10:12:14PM +0200, Bastien Nocera wrote: > > > On Fri, 2019-09-27 at 21:25 +0200, Greg KH wrote: > > > > On Fri, Sep 27, 2019 at 08:49:12PM +0200, Bastien Nocera wrote: > > > > > On Fri, 2019-09-27 at 13:56 -0400, Alan Stern wrote: > > > > > > > > > > > Is there any reason this needs to be done in a kernel driver? > > > > > > > > > > To offer a unified interface all the devices with similar > > > > > needs. > > > > > > > > What "other devices with similar needs?" > > > > > > I would expect Android phones to be able to offer up a different > > > charging method depending on policy, and wanting to be able to > > > switch > > > charging methods. > > > > I doubt it, it "should" be automatic based on the USB hardware > > configurations in the charger, not based on a random undocumented USB > > command sent from the host. The USB spec describes how to do all of > > this properly without any commands at all, why not just rely on that? > > That's not true, no. Until USB PD, there wasn't a way any ways for > devices to know that they could draw more current without either being > told so (as is done here), hardware modifications (with resistors being > wired to GND/VCC), or violating the USB spec. It is the resistor thing. And other "tricks" that hardware could play. My 3 year old phone could go into "high power" charging if it could figure out if it was connected to the special charger that came with it. All of that was done outside of the USB protocol stack, using hardware tricks. And all of that not USB PD. > In this case, Apple worked around the problem by having their OS, > running on their hardware, tell their peripherals to draw more power, > because the spec didn't allow this. And because Apple "knew" that their laptops could provide that kind of power. How do you know that a random USB hub can provide that properly? > Those 2 articles should help understand the complexities of the > problem: > https://lwn.net/Articles/693027/ > https://lwn.net/Articles/694062/ Those are all from the viewpoint of Linux running on the device itself, not Linux running on the host like you are talking about here. > > > > > > Can it > > > > > > be handled from userspace instead? > > > > > > > > > > It could, at a great infrastructure cost, trying to get buy-in > > > > > from > > > > > various distributions, at the very least. > > > > > > > > For USB devices that _can_ be handled in userspace, we ask that > > > > they > > > > be > > > > done in userspace and not with a kernel driver. Something that > > > > only > > > > does usb control messages with no other in-kernel api interfaces > > > > is > > > > ripe > > > > for a tiny userspace program using libusb. Not for an in-kernel > > > > driver. > > > > > > I don't quite understand why that would be when the kernel already > > > offers the API to be able to control it. > > > > Again, if it _can_ be done in userspace, it _should_ be done in > > userspace when it comes to USB drivers/commands. > > That's clear enough, although I still think it's wrong to try and move > to user-space things that have an existing clearly defined API in > kernel space. This would be akin to not allowing any new drivers for > webcams or USB sound cards because "they can be done in user-space". > Sure they can, but there's already an established API to handle them in > the kernel. Again, the power_supply api is for power going the other way in the system. That's not an "existing clearly defined API in kernel space". > > > > > > You said this was for a "power supply" class driver. It's > > > > > > not > > > > > > clear > > > > > > what that means -- the devices you want to communicate with > > > > > > are > > > > > > iphones, ipads, etc., not power supplies. > > > > > > > > > > There's tons of "device" scope "power_supply" devices in the > > > > > kernel, > > > > > which don't power the Linux machine they're running on. Grep > > > > > for > > >
Re: Driver for something that's neither a device nor an interface driver?
On Fri, Sep 27, 2019 at 10:12:14PM +0200, Bastien Nocera wrote: > On Fri, 2019-09-27 at 21:25 +0200, Greg KH wrote: > > On Fri, Sep 27, 2019 at 08:49:12PM +0200, Bastien Nocera wrote: > > > On Fri, 2019-09-27 at 13:56 -0400, Alan Stern wrote: > > > > > > > Is there any reason this needs to be done in a kernel driver? > > > > > > To offer a unified interface all the devices with similar needs. > > > > What "other devices with similar needs?" > > I would expect Android phones to be able to offer up a different > charging method depending on policy, and wanting to be able to switch > charging methods. I doubt it, it "should" be automatic based on the USB hardware configurations in the charger, not based on a random undocumented USB command sent from the host. The USB spec describes how to do all of this properly without any commands at all, why not just rely on that? Do you know of any Android device that requires a USB command like this? > > > > Can it > > > > be handled from userspace instead? > > > > > > It could, at a great infrastructure cost, trying to get buy-in from > > > various distributions, at the very least. > > > > For USB devices that _can_ be handled in userspace, we ask that they > > be > > done in userspace and not with a kernel driver. Something that only > > does usb control messages with no other in-kernel api interfaces is > > ripe > > for a tiny userspace program using libusb. Not for an in-kernel > > driver. > > I don't quite understand why that would be when the kernel already > offers the API to be able to control it. Again, if it _can_ be done in userspace, it _should_ be done in userspace when it comes to USB drivers/commands. > > > > You said this was for a "power supply" class driver. It's not > > > > clear > > > > what that means -- the devices you want to communicate with are > > > > iphones, ipads, etc., not power supplies. > > > > > > There's tons of "device" scope "power_supply" devices in the > > > kernel, > > > which don't power the Linux machine they're running on. Grep for > > > "POWER_SUPPLY_SCOPE_DEVICE" in the kernel, most wireless mice and > > > keyboards implement this already. > > > > Yes, but those are real devices that the "Host" uses for power or > > something else. wireless mice and keyboards already have kernel > > drivers > > so that's fine as well (but probably could be done from userspace > > too.) > > It probably couldn't when the pipes to get key presses and the battery > info are the same. Are you sure? They are really part of the USB HID spec? Do you have pointers to that, for some reason I thought those were "out-of-band" vendor specific commands. > > > > Under what circumstances would these messages need to get sent? > > > > > > User-space would control it by changing the device's > > > POWER_SUPPLY_PROP_CHARGE_TYPE to "Fast", if available. > > > > > > eg. > > > # echo "Fast" > /sys/devices/pci:00/:00:14.0/usb3/3- > > > 1/power_supply/apple_mfi_fastcharge/charge_type > > > > power_supply class is for the power supply that is charging the cpu > > you > > type that on. Not for the cpu of an attached device, right? > > Again, power_supply class has a scope attached to it, so having the > driver in the kernel would actually make it easier, with user-space not > having to care whether the device uses an "Apple" method or something > else. Again, power_supply is for the power being sent to the host computer, right? I don't think it is for power being sent from the host to another device, are you sure it is set up to control/monitor/handle that type of thing? I do know that there are some USB power control things that probably should be tied into the power_supply logic sometime soon, but I do not know if that plumbing has been put into place yet in the power_supply class code. Do you know if it has? thanks, greg k-h
Re: Driver for something that's neither a device nor an interface driver?
On Fri, Sep 27, 2019 at 08:49:12PM +0200, Bastien Nocera wrote: > On Fri, 2019-09-27 at 13:56 -0400, Alan Stern wrote: > > > > > Is there any reason this needs to be done in a kernel driver? > > To offer a unified interface all the devices with similar needs. What "other devices with similar needs?" > > Can it > > be handled from userspace instead? > > It could, at a great infrastructure cost, trying to get buy-in from > various distributions, at the very least. For USB devices that _can_ be handled in userspace, we ask that they be done in userspace and not with a kernel driver. Something that only does usb control messages with no other in-kernel api interfaces is ripe for a tiny userspace program using libusb. Not for an in-kernel driver. > > You said this was for a "power supply" class driver. It's not clear > > what that means -- the devices you want to communicate with are > > iphones, ipads, etc., not power supplies. > > There's tons of "device" scope "power_supply" devices in the kernel, > which don't power the Linux machine they're running on. Grep for > "POWER_SUPPLY_SCOPE_DEVICE" in the kernel, most wireless mice and > keyboards implement this already. Yes, but those are real devices that the "Host" uses for power or something else. wireless mice and keyboards already have kernel drivers so that's fine as well (but probably could be done from userspace too.) > > Under what circumstances would these messages need to get sent? > > User-space would control it by changing the device's > POWER_SUPPLY_PROP_CHARGE_TYPE to "Fast", if available. > > eg. > # echo "Fast" > /sys/devices/pci:00/:00:14.0/usb3/3- > 1/power_supply/apple_mfi_fastcharge/charge_type power_supply class is for the power supply that is charging the cpu you type that on. Not for the cpu of an attached device, right? thanks, greg k-h
Re: Driver for something that's neither a device nor an interface driver?
On Fri, Sep 27, 2019 at 07:44:08PM +0200, Bastien Nocera wrote: > On Fri, 2019-09-27 at 19:38 +0200, Greg KH wrote: > > What does the usb descriptors for the device look like? Is it only > > the > > "default" control endpoint and no interfaces? What does the output > > of > > 'usbdevices' show for the device? > > The device in question can be an iPhone, an iPod Classic/Nano, or an > iPad, amongst others, and they usually have useful interfaces, such as > mass storage for the older ones, or ethernet, PTP, etc. Ah. Then why do you have to do this from a kernel driver? Why can't you do this from userspace? > > Normally you just bind to the "default" interface for the device, and > > all is good, there should be a few other drivers in the tree that do > > this, but I can't think of one off the top of my head at the moment. > > All the interfaces (in the different configurations) are used for > something in the case of the iPhone 6S I'm trying to use. > > I've attached the output of "lsusb -v" for the device below. What about interface "9", the "Apple USB Multiplexor"? What driver binds to that thing? It's a vendor-specific protocol, so there shouldn't be any class driver assigned to it, unlike most of the other interfaces. thanks, greg k-h
Re: Driver for something that's neither a device nor an interface driver?
On Fri, Sep 27, 2019 at 07:29:52PM +0200, Bastien Nocera wrote: > Hey, > > I'm trying to write a "power supply" class driver for Apple MFi > devices, and struggling a little with the USB drivers. > > To ask many Apple devices to draw more power, we need to make a call to > the device using a vendor command. It doesn't go to an interface, but > to the device itself. > > The call done in the kernel would look something like: > usb_control_msg(mfi->udev, usb_sndctrlpipe(mfi->udev, 0), > 0x40, /* Vendor-defined USB get enabled capabilities request. > */ > USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > current_ma, /* wValue, current offset */ > current_ma, /* wIndex, current offset */ > NULL, 0, USB_CTRL_GET_TIMEOUT); > > But I can't figure out what type of driver I'd need to just be able to > export that power_supply interface. > > Trying to use a "struct usb_device_driver" didn't work as probe > functions were never called, and a "struct usb_driver" gets unbound > after user-space and the ipheth drivers comes around. What does the usb descriptors for the device look like? Is it only the "default" control endpoint and no interfaces? What does the output of 'usbdevices' show for the device? Normally you just bind to the "default" interface for the device, and all is good, there should be a few other drivers in the tree that do this, but I can't think of one off the top of my head at the moment. thanks, greg k-h
Re: [PATCH -next] usb: cdns3: Fix sheduling with locks held.
On Thu, Sep 26, 2019 at 08:00:30AM +0100, Pawel Laszczak wrote:
> Patch fix issue in cdns3_ep0_feature_handle_device function.
>
> The function usleep_range can't be used there because this function is
> called with locks held and IRQs disabled in
> cdns3_device_thread_irq_handler().
>
> To resolve this issue patch replaces usleep_range with mdelay.
>
> Reported-by: Dan Carpenter
> Signed-off-by: Pawel Laszczak
> Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
> ---
> drivers/usb/cdns3/ep0.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Is this v2 of the patch?
If so, it needs to be said so in the subject line, and below the ---
line describe what changed from the previous one.
That should all be described in the kernel documentation, right?
v3 please?
thanks,
greg k-h
Re: [PATCH -next] usb: cdns3: Fix sheduling with locks held.
On Thu, Sep 26, 2019 at 07:19:27AM +0100, Pawel Laszczak wrote: > Patch fix issue in cdns3_ep0_feature_handle_device function. > > The function usleep_range can't be used there because this function is > called with locks held and IRQs disabled in > cdns3_device_thread_irq_handler(). > > To resolve this issue patch replaces usleep_range with mdelay. > > Signed-off-by: Pawel Laszczak What commit does this fix? Did someone report it? If so, please properly credit them here. thanks, greg k-h
Re: [PATCH 1/1] usb: serial: option: add Telit FN980 compositions
On Mon, Sep 23, 2019 at 11:51:42AM +0200, Daniele Palmas wrote: > This patch adds the following Telit FN980 compositions: > > 0x1050: tty, adb, rmnet, tty, tty, tty, tty > 0x1051: tty, adb, mbim, tty, tty, tty, tty > 0x1052: rndis, tty, adb, tty, tty, tty, tty > 0x1053: tty, adb, ecm, tty, tty, tty, tty > --- Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - Your patch does not have a Signed-off-by: line. Please read the kernel file, Documentation/SubmittingPatches and resend it after adding that line. Note, the line needs to be in the body of the email, before the patch, not at the bottom of the patch or in the email signature. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
Re: Failed to connect to 4G modem
On Sat, Sep 21, 2019 at 08:10:30AM +1000, JH wrote: > Hi Greg, > > On 9/18/19, Greg KH wrote: > > Otherwise, just use 5.3 now and then 5.4 when it comes out in a few > > months. > > My apology for an off topic question. I discussed with my colleague to > use 5.3, if we cannot wait for 5.4 LTS release, we will do a remote > 5.4 upgrade from 5.3 on air for the product. One of my colleague > warning me that kernel 5.3 and 5.4 LTS will not be a minor upgrade, it > will be major upgrade which will have massive libraries changes. I > don't agree with it, but I could not find kernel document to state > either 5.3 and 5.4 LTS is a major or minor change. What is your > insight view? Appreciate if you could point me a reference to prove my > colleague is wrong. You should never have to update any userspace code or library if the kernel is updated as we guarantee backwards compatibility. If we did break something, please let us know and we will work very hard to fix it. This is a guarantee we made back in 2007 or so, and have been sticking to it since. There is no "major" upgrade issue here, the kernel does a new release every 3 months and changes the number, showing it is "newer" than the previous one. Hope this helps, greg k-h
Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
On Fri, Sep 20, 2019 at 03:17:26PM +0200, David Heinzelmann wrote: > Hi, > > sorry for the wrong patch format. No problem, that's normal. But please do not top-post on linux mailing lists. > I am trying to detect a change. At the moment I think the change could be > ignored if a > port connection-change occurs and the port status has again the > 'PORT_CONNECTION' bit set. > > I have a fx3 device which does a re-enumeration after a firmware download. > This is working > as expected and I am seeing a 'remove event' and a 'add event' monitoring via > udevadm. But > if I connect multiple devices at the same time via an usb hub I am sometimes > not receiving > a 'remove event' and 'add event' for a single device. Sounds like a broken hub :) > I think the problem could be that when a device disconnects and the port > connection-change > occurs and before the 'PORT_CONNECTION' bit is checked the device could > already be > reconnected and the 'PORT_CONNECTION' bit is set. Therefore I think it is not > correct to > resuscitate the exisiting device. Does your patch actually fix the issue? When a fx3 device downloads firmware and re-enumerates, it should come back as a totally different device, which will fail this check, right? So I don't see how this fixes the issues with your devices. Unless all of the devices reset themselves at the same time and the hub doesn't like that and can't notice that it happened? If you use a different hub, does that work properly? thanks, greg k-h
Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
On Fri, Sep 20, 2019 at 12:36:28PM +0200, David Heinzelmann wrote: > When a port connection-change occurs the hub driver tries to resuscitate an > existing device. > Activated from a firmware download a usb device can re-enumerate with new or > changed device > descriptors. Therefore it will be checked for changed device descriptors > before the connection > is resuscitated and the connection-change event is ignored. Please wrap your lines at 72 columns :( Anyway, what problem are you trying to solve here? What is broken with how things work today? Are you trying to ignore a change that is currently showing up as a change, or trying to do the opposite? thanks, greg k-h
Re: [PATCH] USB: serial: add port statistics
On Wed, Sep 18, 2019 at 01:51:29PM +0200, Yegor Yefremov wrote: > On Wed, Sep 18, 2019 at 1:45 PM Greg KH wrote: > > > > On Wed, Sep 18, 2019 at 01:22:42PM +0200, Yegor Yefremov wrote: > > > On Wed, Sep 18, 2019 at 1:08 PM Greg KH > > > wrote: > > > > > > > > On Wed, Sep 18, 2019 at 11:14:15AM +0200, yegorsli...@googlemail.com > > > > wrote: > > > > > From: Yegor Yefremov > > > > > > > > > > Add additional port statistics like received and transmitted bytes > > > > > the way /proc/tty/driver/serial does. > > > > > > > > > > As usbserial driver already provides USB related information and > > > > > this line is longer than 100 characters, this patch adds an > > > > > additional line with the same port number: > > > > > > > > > > 0: module:ftdi_sio name:"FTDI USB Serial Device" vendor:0403 ... > > > > > 0: tx:112 rx:0 > > > > > > > > > > Signed-off-by: Yegor Yefremov > > > > > --- > > > > > drivers/usb/serial/usb-serial.c | 22 ++ > > > > > 1 file changed, 22 insertions(+) > > > > > > > > You can't change existing proc files without having the chance that > > > > userspace tools will break. > > > > > > > > Have you tried this and seen what dies a horrible death? > > > > > > This patch is more a proof of concept (forgot to add RFC keyword). I > > > find statistics provdes by the 8250 driver very useful for debugging > > > purposes. What would be the best way to implemnt this feature for > > > usbserial driver? > > > > > > a) extend current line: > > > > > > 0: module:ftdi_sio name:"FTDI USB Serial Device" vendor:0403 ...tx:112 > > > rx:0 > > > > > > though this still can break parsing > > > > > > b) creating special entries for FTDI and other UARTs? Though it would > > > be greate to have all usbserial UART handled the same way in the same > > > file > > > > Why is any of this needed at all? Also, be very aware of the security > > issues involved here, we had to disable access of these values by > > "normal" users for other tty devices, so please don't break that by > > offering it up here again. > > > > What is going to use this information? > > This feature is not a "must have" one but it is convenient to see > transferred/received bytes and error flags from user space. If some > serial software is not working like expected and doesn't provide > enough debugging information one can quickly look at port statistics > from the console in order to check whether and how many bytes were > transferred or whether the were some communication errors. Again, it's a security issue, so be careful about this. If you _REALLY_ need it, make it a debugfs file, readble by root only. Or a tracepoint, and then you can have a userspace read the data using ebpf :) thanks, greg k-h
Re: [PATCH] USB: serial: add port statistics
On Wed, Sep 18, 2019 at 01:22:42PM +0200, Yegor Yefremov wrote: > On Wed, Sep 18, 2019 at 1:08 PM Greg KH wrote: > > > > On Wed, Sep 18, 2019 at 11:14:15AM +0200, yegorsli...@googlemail.com wrote: > > > From: Yegor Yefremov > > > > > > Add additional port statistics like received and transmitted bytes > > > the way /proc/tty/driver/serial does. > > > > > > As usbserial driver already provides USB related information and > > > this line is longer than 100 characters, this patch adds an > > > additional line with the same port number: > > > > > > 0: module:ftdi_sio name:"FTDI USB Serial Device" vendor:0403 ... > > > 0: tx:112 rx:0 > > > > > > Signed-off-by: Yegor Yefremov > > > --- > > > drivers/usb/serial/usb-serial.c | 22 ++ > > > 1 file changed, 22 insertions(+) > > > > You can't change existing proc files without having the chance that > > userspace tools will break. > > > > Have you tried this and seen what dies a horrible death? > > This patch is more a proof of concept (forgot to add RFC keyword). I > find statistics provdes by the 8250 driver very useful for debugging > purposes. What would be the best way to implemnt this feature for > usbserial driver? > > a) extend current line: > > 0: module:ftdi_sio name:"FTDI USB Serial Device" vendor:0403 ...tx:112 rx:0 > > though this still can break parsing > > b) creating special entries for FTDI and other UARTs? Though it would > be greate to have all usbserial UART handled the same way in the same > file Why is any of this needed at all? Also, be very aware of the security issues involved here, we had to disable access of these values by "normal" users for other tty devices, so please don't break that by offering it up here again. What is going to use this information? thanks, greg k-h
Re: [PATCH] USB: serial: add port statistics
On Wed, Sep 18, 2019 at 11:14:15AM +0200, yegorsli...@googlemail.com wrote: > From: Yegor Yefremov > > Add additional port statistics like received and transmitted bytes > the way /proc/tty/driver/serial does. > > As usbserial driver already provides USB related information and > this line is longer than 100 characters, this patch adds an > additional line with the same port number: > > 0: module:ftdi_sio name:"FTDI USB Serial Device" vendor:0403 ... > 0: tx:112 rx:0 > > Signed-off-by: Yegor Yefremov > --- > drivers/usb/serial/usb-serial.c | 22 ++ > 1 file changed, 22 insertions(+) You can't change existing proc files without having the chance that userspace tools will break. Have you tried this and seen what dies a horrible death? thanks, greg k-h
Re: Failed to connect to 4G modem
On Wed, Sep 18, 2019 at 07:38:14AM +1000, JH wrote: > Hi Greg, > > On 9/17/19, Greg KH wrote: > > On Tue, Sep 17, 2019 at 09:29:34PM +1000, JH wrote: > >> Hi, > >> > >> I am running kernel LTS 4.19 in an i.MX6 board with a 4G LTE modem, it > >> continually displayed following messages in a serial port tried and > >> failed to connect to LTE modem, are there any problems in kernel 4.19 > >> LTS to support usb qmi protocol and LTE modem connection? > >> > >> I did try the kernel version 5.1, it can connect to the modem, but > >> failed in couple of hours or days, when it disconnected, it got the > >> same following messages. > >> > >> Are there serious problems in kernel to support usb qmi LTE modem > >> communication and connection? > >> > >> [ 43.837243] option1 ttyUSB0: GSM modem (1-port) converter now > >> disconnected f0 > >> [ 43.882941] option 1-1:1.0: device disconnected > >> [ 43.942788] option1 ttyUSB1: GSM modem (1-port) converter now > >> disconnected f1 > >> [ 44.001445] option 1-1:1.2: device disconnected > >> [ 44.011575] qmi_wwan 1-1:1.3: nonzero urb status received: -71 > >> [ 44.017461] qmi_wwan 1-1:1.3: wdm_int_callback - 0 bytes > >> [ 44.022801] qmi_wwan 1-1:1.3: wdm_int_callback - usb_submit_urb failed > >> with 9 > >> [ 44.059958] qmi_wwan 1-1:1.3 wwan0: unregister 'qmi_wwan' > >> usb-ci_hdrc.1-1, We > >> [ 47.675604] usb 1-1: new high-speed USB device number 5 using ci_hdrc > >> [ 47.905246] usb 1-1: New USB device found, idVendor=05c6, > >> idProduct=90b2, bc0 > >> [ 47.913732] usb 1-1: New USB device strings: Mfr=3, Product=2, > >> SerialNumber=4 > >> [ 47.921099] usb 1-1: Product: Qualcomm CDMA Technologies MSM > >> [ 47.927087] usb 1-1: Manufacturer: Qualcomm, Incorporated > >> [ 47.932746] usb 1-1: SerialNumber: 5ff10299 > >> [ 47.964528] option 1-1:1.0: GSM modem (1-port) converter detected > >> [ 47.989484] usb 1-1: GSM modem (1-port) converter now attached to > >> ttyUSB0 > >> [ 48.014760] option 1-1:1.2: GSM modem (1-port) converter detected > >> [ 48.026996] usb 1-1: GSM modem (1-port) converter now attached to > >> ttyUSB1 > >> [ 48.048810] qmi_wwan 1-1:1.3: cdc-wdm0: USB WDM device > >> [ 48.082751] qmi_wwan 1-1:1.3 wwan0: register 'qmi_wwan' at > >> usb-ci_hdrc.1-1, 8 > >> [ 51.581595] usb 1-1: USB disconnect, device number 5 > >> [ 51.613737] option1 ttyUSB0: GSM modem (1-port) converter now > >> disconnected f0 > >> [ 51.644564] option 1-1:1.0: device disconnected > >> [ 51.713919] option1 ttyUSB1: GSM modem (1-port) converter now > >> disconnected f1 > >> [ 51.771139] option 1-1:1.2: device disconnected > > > > The device is disconnecting itself from the USB bus, and then connecting > > itself, and then disconnecting... > > > > Probably an electrical issue, the kernel can not disconnect a device > > directly from the USB bus, that information comes from the USB hub > > itself. > > That was always my thought until I tried kernel 5.1 under the same > platform (nothing changed except the kernel version), the kernel 5.1 > can connect to the 4G modem, I could not tell the hardware engineer if > it was hardware problem where kernel 5.1 can connect, kernel 4.19 > could not, how would you explain it? Seems some differences between > kernel 5.1 and kernel 4.19, what I could be missing? > > I cannot use kernel 5, we need kernel LTS on product, too late to wait > for 5.4 LTS. Can you use 'git bisect' to find the commit that fixes the issue? That way we can backport it to the 4.19.y tree for you. Otherwise, just use 5.3 now and then 5.4 when it comes out in a few months. good luck! greg k-h
Re: Failed to connect to 4G modem
On Tue, Sep 17, 2019 at 09:29:34PM +1000, JH wrote: > Hi, > > I am running kernel LTS 4.19 in an i.MX6 board with a 4G LTE modem, it > continually displayed following messages in a serial port tried and > failed to connect to LTE modem, are there any problems in kernel 4.19 > LTS to support usb qmi protocol and LTE modem connection? > > I did try the kernel version 5.1, it can connect to the modem, but > failed in couple of hours or days, when it disconnected, it got the > same following messages. > > Are there serious problems in kernel to support usb qmi LTE modem > communication and connection? > > [ 43.837243] option1 ttyUSB0: GSM modem (1-port) converter now disconnected > f0 > [ 43.882941] option 1-1:1.0: device disconnected > [ 43.942788] option1 ttyUSB1: GSM modem (1-port) converter now disconnected > f1 > [ 44.001445] option 1-1:1.2: device disconnected > [ 44.011575] qmi_wwan 1-1:1.3: nonzero urb status received: -71 > [ 44.017461] qmi_wwan 1-1:1.3: wdm_int_callback - 0 bytes > [ 44.022801] qmi_wwan 1-1:1.3: wdm_int_callback - usb_submit_urb failed > with 9 > [ 44.059958] qmi_wwan 1-1:1.3 wwan0: unregister 'qmi_wwan' usb-ci_hdrc.1-1, > We > [ 47.675604] usb 1-1: new high-speed USB device number 5 using ci_hdrc > [ 47.905246] usb 1-1: New USB device found, idVendor=05c6, idProduct=90b2, > bc0 > [ 47.913732] usb 1-1: New USB device strings: Mfr=3, Product=2, > SerialNumber=4 > [ 47.921099] usb 1-1: Product: Qualcomm CDMA Technologies MSM > [ 47.927087] usb 1-1: Manufacturer: Qualcomm, Incorporated > [ 47.932746] usb 1-1: SerialNumber: 5ff10299 > [ 47.964528] option 1-1:1.0: GSM modem (1-port) converter detected > [ 47.989484] usb 1-1: GSM modem (1-port) converter now attached to ttyUSB0 > [ 48.014760] option 1-1:1.2: GSM modem (1-port) converter detected > [ 48.026996] usb 1-1: GSM modem (1-port) converter now attached to ttyUSB1 > [ 48.048810] qmi_wwan 1-1:1.3: cdc-wdm0: USB WDM device > [ 48.082751] qmi_wwan 1-1:1.3 wwan0: register 'qmi_wwan' at > usb-ci_hdrc.1-1, 8 > [ 51.581595] usb 1-1: USB disconnect, device number 5 > [ 51.613737] option1 ttyUSB0: GSM modem (1-port) converter now disconnected > f0 > [ 51.644564] option 1-1:1.0: device disconnected > [ 51.713919] option1 ttyUSB1: GSM modem (1-port) converter now disconnected > f1 > [ 51.771139] option 1-1:1.2: device disconnected The device is disconnecting itself from the USB bus, and then connecting itself, and then disconnecting... Probably an electrical issue, the kernel can not disconnect a device directly from the USB bus, that information comes from the USB hub itself. Try your connections. thanks, greg k-h
Re: patch "usbip: tools: fix GCC8 warning for strncpy" added to usb-next
A: http://en.wikipedia.org/wiki/Top_post Q: Were do I find info about this thing called top-posting? A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top On Tue, Sep 17, 2019 at 09:13:15AM +0800, Liu, Changcheng wrote: > Hi Greg, >Do we have plan to merge this patch into 5.3 Release? >I hit the build problem(warning turns to be error under -Werror) when >building 5.3 version. This will be submitted for 5.4-rc1. If this is needed in 5.3, please ask sta...@vger.kernel.org to backport it once it is in Linus's tree. But we do not build the kernel with -Werror, so this should not be a "real" problem for you. thanks, greg k-h
Re: [PATCH v3 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality
On Thu, Sep 05, 2019 at 08:51:15PM -0500, Charles Hyde wrote:
> This change adds support to cdc_ncm for ACPI MAC address pass through
> functionality that also exists in the Realtek r8152 driver. This is in
> support of Dell's Universal Dock D6000, to give it the same feature
> capability as is currently available in Windows and advertized on Dell's
> product web site.
>
> Today's v3 patch series includes a function named get_ethernet_addr()
> which replaces two instances where the same code snippet was located in
> teh previous patch series. I also created a post reset function to set
> the MAC address, if there exists an ACPI MAC address pass through (MAPT)
> method. Oliver Neukum had requested a post reset function for this
> purpose.
>
> Signed-off-by: Charles Hyde
> Cc: Mario Limonciello
> Cc: chip.program...@gmail.com
> Cc: Oliver Neukum
> Cc: "Rafael J. Wysocki"
> Cc: Len Brown
> Cc: linux-usb@vger.kernel.org
> Cc: linux-a...@vger.kernel.org
> ---
> drivers/net/usb/cdc_ncm.c | 74 +--
> 1 file changed, 64 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 85093579612f..e0152d44f5af 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -52,6 +52,7 @@
> #include
> #include
> #include
> +#include
>
> #if IS_ENABLED(CONFIG_USB_NET_CDC_MBIM)
> static bool prefer_mbim = true;
> @@ -833,6 +834,45 @@ static const struct net_device_ops cdc_ncm_netdev_ops = {
> .ndo_validate_addr = eth_validate_addr,
> };
>
> +static int get_ethernet_addr(struct usb_interface *intf)
> +{
> + struct sockaddr sa;
> + struct usbnet *dev = usb_get_intfdata(intf);
> + struct cdc_ncm_ctx *ctx;
> + int ret = 0;
> +
> + if (!dev)
> + return 0;
> +
> + ctx = (struct cdc_ncm_ctx *)dev->data[0];
> + if (!ctx->ether_desc)
> + return 0;
> +
> + ret = cdc_ncm_get_ethernet_address(dev, ctx);
> + if (ret) {
> + dev_dbg(&intf->dev, "failed to get mac address\n");
> + return ret;
> + }
> +
> + /* Check for a Dell Universal Dock D6000 before checking if ACPI
> + * supports MAC address pass through.
> + */
> + if (strstr(dev->udev->product, "D6000")) {
As other people have pointed out, that's funny.
No, this is explicitly what the USB vendor/product ids are for, don't
try to make up something that will be guaranteed to not work
correctly...
> + sa.sa_family = dev->net->type;
> + if (get_acpi_mac_passthru(sa.sa_data)) {
> + if (!memcmp(dev->net->dev_addr, sa.sa_data,
> + ETH_ALEN)) {
> + if (!cdc_ncm_set_ethernet_address(dev, &sa))
> + memcpy(dev->net->dev_addr, sa.sa_data,
> +ETH_ALEN);
> + }
> + }
> + }
> + dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net->dev_addr);
If a driver is working properly, it should not spit out any kernel log
messages. Make this dev_dbg() if you need it for your own debugging
logic.
thanks,
greg k-h
Re: [PATCH 0/3] Add get/set ethernet address functions and ACPI MAC address pass through functionality to cdc_ncm driver
On Thu, Sep 05, 2019 at 09:14:05PM +, charles.h...@dellteam.com wrote: > > > -Original Message- > > > From: Hyde, Charles - Dell Team > > > Sent: Thursday, September 5, 2019 4:02 PM > > > To: Oliver Neukum; "Rafael J. Wysocki"; Len Brown > > > Cc: Limonciello, Mario; chip.program...@gmail.com; Realtek linux nic > > > maintainers; linux-usb@vger.kernel.org; linux-a...@vger.kernel.org > > > Subject: [PATCH 0/3] Add get/set ethernet address functions and ACPI > > > MAC address pass through functionality to cdc_ncm driver > > > > > > In recent testing of a Dell Universal Dock D6000, I found that MAC > > > address pass through is not supported in the Linux drivers. However, > > > this same device is supported in Windows 10 (Pro) on my personal > > > computer, in as much as I was able to tell Windows to assign a new MAC > > > address of my choosing, and I saw through wireshark the new MAC > > > address was pushed out to the device. Afterward, Windows reported a > > > new IP address and I was able to view web pages. > > > > > > This series of patches give support to cdc_ncm USB based Ethernet > > > controllers for programming a MAC address to the device, and also to > > > retrieve the device's MAC address. This patch series further adds > > > ACPI MAC address pass through support specifically for the cdc_ncm > > > driver, and generally for any other driver that may need or want it, > > > in furtherance of Dell's enterprise IT policy efforts. It was this > > > latter that I initially found lacking when testing a D6000 with a Dell > > > laptop, and then I found ifconfig was unable to set a MAC address into > > > the device. These patches bring a similar level of functionality to > > > cdc_ncm driver as is available with the Realtek r8152 driver, and is > > > available > > with Windows. > > > > > > The cdc_ncm driver limits the ACPI MAC address pass through support to > > > only the Dell Universal Dock D6000, so no other cdc_ncm device will be > > > impacted. > > > > > > Charles Hyde (3): > > > net: cdc_ncm: add get/set ethernet address functions > > > ACPI: move ACPI functionality out of r8152 driver > > > net: cdc_ncm: Add ACPI MAC address pass through functionality > > > > > > drivers/acpi/Makefile| 1 + > > > drivers/acpi/acpi_mac_passthru.c | 63 + > > > drivers/net/usb/cdc_ncm.c| 148 --- > > > drivers/net/usb/r8152.c | 44 + > > > include/acpi/acpi_mac_passthru.h | 29 ++ > > > 5 files changed, 234 insertions(+), 51 deletions(-) create mode > > > 100644 drivers/acpi/acpi_mac_passthru.c create mode 100644 > > > include/acpi/acpi_mac_passthru.h > > > > > > -- > > > 2.20.1 > > > > Typical practice is to make this new patch series prefixed with a v2. And to > > describe what has changed From v1 either in individual patches below the > > '--' > > or in the cover letter. > > > > So can you please describe what changed from previous submission in case > > it's > > not obvious to reviewers? > > > > For example: > > > > [PATCH v2 0/3] Add get/set ethernet address functions and ACPI MAC address > > pass through functionality to cdc_ncm driver > > > > Changes from v1 to v 2: > > * Changed foo to bar > > > Ah, my apologies. > > What changed with today's patch series from what I proposed on Friday, August > 30, is that I created a function named get_ethernet_addr() which replaces two > instances where the same code snippet was located in the previous patch > series. I also created a post reset function to set the MAC address, if > there exists an ACPI MAC address pass through (MAPT). Oliver Neukum had > requested a post reset function for this purpose. Please resend the series with that information in it, and also properly thread the emails so they are at least clustered together. thanks, greg k-h
Re: [PATCH] usb: devio: fix mmap() on non-coherent DMA architectures
On Mon, Aug 05, 2019 at 05:17:13PM +0200, Greg KH wrote: > On Fri, Aug 02, 2019 at 10:57:00AM -0700, Gavin Li wrote: > > usbfs mmap() looks like it was introduced for 4.6 in commit > > f7d34b445abc, so it should probably be backported to 4.9 and onwards. > > This issue has been present since the introduction of the feature. > > > > One sidenote: this patch will cause the following warning on x86 due > > to dmap_mmap_coherent() trying to map normal memory in as uncached: > > > > x86/PAT: ... map pfn RAM range req uncached-minus for [mem > > 0x77b00-0x77b210fff], got write-back > > > > This warning is harmless, as x86 is DMA coherent and the memory gets > > correctly mapped in as write-back. I will submit a patch to the DMA > > mapping team to eliminate this warning. > > Let me know what the git commit id of that patch is, I will wait for it > to hit the tree before adding this one. Dropping this patch from my queue now, please feel free to resend when you have refreshed it against the latest tree. thanks, greg k-h
Re: [PATCH] xhci: tegra: Parameterize mailbox register addresses
On Wed, Sep 04, 2019 at 09:43:08AM +0800, JC Kuo wrote:
> On 9/3/19 9:58 PM, Greg KH wrote:
> > On Mon, Sep 02, 2019 at 04:21:27PM +0800, JC Kuo wrote:
> >> Tegra194 XUSB host controller has rearranged mailbox registers. This
> >> commit makes mailbox registers address a part of "soc" data so that
> >> xhci-tegra driver can be used for Tegra194.
> >>
> >> Signed-off-by: JC Kuo
> >> ---
> >> drivers/usb/host/xhci-tegra.c | 58 +--
> >> 1 file changed, 42 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
> >> index dafc65911fc0..247b08ca49ee 100644
> >> --- a/drivers/usb/host/xhci-tegra.c
> >> +++ b/drivers/usb/host/xhci-tegra.c
> >> @@ -42,19 +42,18 @@
> >> #define XUSB_CFG_CSB_BASE_ADDR0x800
> >>
> >> /* FPCI mailbox registers */
> >> -#define XUSB_CFG_ARU_MBOX_CMD 0x0e4
> >> +/* XUSB_CFG_ARU_MBOX_CMD */
> >> #define MBOX_DEST_FALC BIT(27)
> >> #define MBOX_DEST_PMEBIT(28)
> >> #define MBOX_DEST_SMIBIT(29)
> >> #define MBOX_DEST_XHCI BIT(30)
> >> #define MBOX_INT_EN BIT(31)
> >> -#define XUSB_CFG_ARU_MBOX_DATA_IN 0x0e8
> >> +/* XUSB_CFG_ARU_MBOX_DATA_IN and XUSB_CFG_ARU_MBOX_DATA_OUT */
> >> #define CMD_DATA_SHIFT 0
> >> #define CMD_DATA_MASK0xff
> >> #define CMD_TYPE_SHIFT 24
> >> #define CMD_TYPE_MASK0xff
> >> -#define XUSB_CFG_ARU_MBOX_DATA_OUT0x0ec
> >> -#define XUSB_CFG_ARU_MBOX_OWNER 0x0f0
> >> +/* XUSB_CFG_ARU_MBOX_OWNER */
> >> #define MBOX_OWNER_NONE 0
> >> #define MBOX_OWNER_FW1
> >> #define MBOX_OWNER_SW2
> >> @@ -146,6 +145,13 @@ struct tegra_xusb_phy_type {
> >>unsigned int num;
> >> };
> >>
> >> +struct tega_xusb_mbox_regs {
> >> + unsigned int cmd;
> >> + unsigned int data_in;
> >> + unsigned int data_out;
> >> + unsigned int owner;
> >
> > Shouldn't these all be u8 values?
> >
> These data members represent register offset in Tegra XUSB FPCI area. Size of
> FPCI area is 0x1000, so it is possible for future Tegra XUSB to have mailbox
> registers allocated to somewhere > 0x100.
Ok, then u16?
> > This did not change any existing functionality, is there a follow-on
> > patch somewhere that takes advantage of this change to enable different
> > hardware? Otherwise this doesn't seem worth it.
> >
> Yes, I will submit another patch to enable Tegra194 XHCI. It will make use of
> this patch to declare Tegra194 XUSB mailbox registers as:
>
> .mbox = {
> .cmd = 0x68,
> .data_in = 0x6c,
> .data_out = 0x70,
> .owner = 0x74,
> },
Can you send that out as patch 2/2 so that we see the need for this
change?
thanks,
greg k-h
Re: [PATCH] USB: rio500: Fix lockdep violation
On Thu, Aug 15, 2019 at 10:47:45AM -0400, Alan Stern wrote:
> On Thu, 15 Aug 2019, Greg KH wrote:
>
> > On Thu, Aug 08, 2019 at 02:23:00PM -0400, Alan Stern wrote:
> > > On Thu, 8 Aug 2019, Greg KH wrote:
> > >
> > > > On Thu, Aug 08, 2019 at 01:34:08PM -0400, Alan Stern wrote:
> > > > > The syzbot fuzzer found a lockdep violation in the rio500 driver:
> > > > >
> > > > > ==
> > > > > WARNING: possible circular locking dependency detected
> > > > > 5.3.0-rc2+ #23 Not tainted
> > > > > --
> > > > > syz-executor.2/20386 is trying to acquire lock:
> > > > > 772249c6 (rio500_mutex){+.+.}, at: open_rio+0x16/0xc0
> > > > > drivers/usb/misc/rio500.c:64
> > > > >
> > > > > but task is already holding lock:
> > > > > d3e8f4b9 (minor_rwsem){}, at: usb_open+0x23/0x270
> > > > > drivers/usb/core/file.c:39
> > > > >
> > > > > which lock already depends on the new lock.
> > > > >
> > > > > The problem is that the driver's open_rio() routine is called while
> > > > > the usbcore's minor_rwsem is locked for reading, and it acquires the
> > > > > rio500_mutex; whereas conversely, probe_rio() and disconnect_rio()
> > > > > first acquire the rio500_mutex and then call usb_register_dev() or
> > > > > usb_deregister_dev(), which lock minor_rwsem for writing.
> > > > >
> > > > > The correct ordering of acquisition should be: minor_rwsem first, then
> > > > > rio500_mutex (since the locking in open_rio() cannot be changed).
> > > > > Thus, the probe and disconnect routines should avoid holding
> > > > > rio500_mutex while doing their registration and deregistration.
> > > > >
> > > > > This patch adjusts the code in those two routines to do just that. It
> > > > > also relies on the fact that the probe and disconnect routines are
> > > > > protected by the device mutex, so the initial test of rio->present
> > > > > needs no extra locking.
> > > > >
> > > > > Reported-by: syzbot+7bbcbe9c9ff0cd495...@syzkaller.appspotmail.com
> > > > > Signed-off-by: Alan Stern
> > > > > Fixes: d710734b0677 ("USB: rio500: simplify locking")
> > > > > CC: Oliver Neukum
> > > > > CC:
> > > > >
> > > > > ---
> > > > >
> > > > > This patch is different from the one I posted earlier. I realized
> > > > > that
> > > > > we don't want to register the device's char file until after the
> > > > > buffers have been allocated.
> > > >
> > > > Should I revert Oliver's patch?
> > >
> > > Sorry, I should have explained more clearly: This goes on top of
> > > Oliver's patch. In fact, Oliver's patch is the one listed in the
> > > Fixes: tag.
> > >
> > > You do not need to apply Oliver's reversion. Assuming he agrees that
> > > this patch is correct, of course.
> >
> > Ok, I applied the revert, and that's in 5.3-rc4. So of course this does
> > not apply :)
> >
> > Shoudl I revert the revert and then apply this? I will if I can get an
> > ack from Oliver...
>
> Either that or else Oliver and I can squash the two patches into one.
I've now merged both, thanks.
greg k-h
Re: [PATCH] xhci: tegra: Parameterize mailbox register addresses
On Mon, Sep 02, 2019 at 04:21:27PM +0800, JC Kuo wrote:
> Tegra194 XUSB host controller has rearranged mailbox registers. This
> commit makes mailbox registers address a part of "soc" data so that
> xhci-tegra driver can be used for Tegra194.
>
> Signed-off-by: JC Kuo
> ---
> drivers/usb/host/xhci-tegra.c | 58 +--
> 1 file changed, 42 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
> index dafc65911fc0..247b08ca49ee 100644
> --- a/drivers/usb/host/xhci-tegra.c
> +++ b/drivers/usb/host/xhci-tegra.c
> @@ -42,19 +42,18 @@
> #define XUSB_CFG_CSB_BASE_ADDR 0x800
>
> /* FPCI mailbox registers */
> -#define XUSB_CFG_ARU_MBOX_CMD0x0e4
> +/* XUSB_CFG_ARU_MBOX_CMD */
> #define MBOX_DEST_FALC BIT(27)
> #define MBOX_DEST_PME BIT(28)
> #define MBOX_DEST_SMI BIT(29)
> #define MBOX_DEST_XHCI BIT(30)
> #define MBOX_INT_EN BIT(31)
> -#define XUSB_CFG_ARU_MBOX_DATA_IN0x0e8
> +/* XUSB_CFG_ARU_MBOX_DATA_IN and XUSB_CFG_ARU_MBOX_DATA_OUT */
> #define CMD_DATA_SHIFT 0
> #define CMD_DATA_MASK 0xff
> #define CMD_TYPE_SHIFT 24
> #define CMD_TYPE_MASK 0xff
> -#define XUSB_CFG_ARU_MBOX_DATA_OUT 0x0ec
> -#define XUSB_CFG_ARU_MBOX_OWNER 0x0f0
> +/* XUSB_CFG_ARU_MBOX_OWNER */
> #define MBOX_OWNER_NONE 0
> #define MBOX_OWNER_FW 1
> #define MBOX_OWNER_SW 2
> @@ -146,6 +145,13 @@ struct tegra_xusb_phy_type {
> unsigned int num;
> };
>
> +struct tega_xusb_mbox_regs {
> + unsigned int cmd;
> + unsigned int data_in;
> + unsigned int data_out;
> + unsigned int owner;
Shouldn't these all be u8 values?
> +};
> +
> struct tegra_xusb_soc {
> const char *firmware;
> const char * const *supply_names;
> @@ -160,6 +166,8 @@ struct tegra_xusb_soc {
> } usb2, ulpi, hsic, usb3;
> } ports;
>
> + struct tega_xusb_mbox_regs mbox;
> +
> bool scale_ss_clock;
> bool has_ipfs;
> };
> @@ -395,15 +403,15 @@ static int tegra_xusb_mbox_send(struct tegra_xusb
> *tegra,
>* ACK/NAK messages.
>*/
> if (!(msg->cmd == MBOX_CMD_ACK || msg->cmd == MBOX_CMD_NAK)) {
> - value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_OWNER);
> + value = fpci_readl(tegra, tegra->soc->mbox.owner);
> if (value != MBOX_OWNER_NONE) {
> dev_err(tegra->dev, "mailbox is busy\n");
> return -EBUSY;
> }
>
> - fpci_writel(tegra, MBOX_OWNER_SW, XUSB_CFG_ARU_MBOX_OWNER);
> + fpci_writel(tegra, MBOX_OWNER_SW, tegra->soc->mbox.owner);
>
> - value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_OWNER);
> + value = fpci_readl(tegra, tegra->soc->mbox.owner);
> if (value != MBOX_OWNER_SW) {
> dev_err(tegra->dev, "failed to acquire mailbox\n");
> return -EBUSY;
> @@ -413,17 +421,17 @@ static int tegra_xusb_mbox_send(struct tegra_xusb
> *tegra,
> }
>
> value = tegra_xusb_mbox_pack(msg);
> - fpci_writel(tegra, value, XUSB_CFG_ARU_MBOX_DATA_IN);
> + fpci_writel(tegra, value, tegra->soc->mbox.data_in);
>
> - value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_CMD);
> + value = fpci_readl(tegra, tegra->soc->mbox.cmd);
> value |= MBOX_INT_EN | MBOX_DEST_FALC;
> - fpci_writel(tegra, value, XUSB_CFG_ARU_MBOX_CMD);
> + fpci_writel(tegra, value, tegra->soc->mbox.cmd);
>
> if (wait_for_idle) {
> unsigned long timeout = jiffies + msecs_to_jiffies(250);
>
> while (time_before(jiffies, timeout)) {
> - value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_OWNER);
> + value = fpci_readl(tegra, tegra->soc->mbox.owner);
> if (value == MBOX_OWNER_NONE)
> break;
>
> @@ -431,7 +439,7 @@ static int tegra_xusb_mbox_send(struct tegra_xusb *tegra,
> }
>
> if (time_after(jiffies, timeout))
> - value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_OWNER);
> + value = fpci_readl(tegra, tegra->soc->mbox.owner);
>
> if (value != MBOX_OWNER_NONE)
> return -ETIMEDOUT;
> @@ -598,16 +606,16 @@ static irqreturn_t tegra_xusb_mbox_thread(int irq, void
> *data)
>
> mutex_lock(&tegra->lock);
>
> - value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_DATA_OUT);
> + value = fpci_readl(tegra, tegra->soc->mbox.data_out);
> tegra_xusb_mbox_unpack(
Re: [PATCH RESEND] xhci: Prevent deadlock when xhci adapter breaks during init
On Thu, Aug 29, 2019 at 02:12:15PM -0400, Bill Kuzeja wrote:
> The system can hit a deadlock if xhci adapter breaks while initializing.
> The deadlock is between two threads: thread 1 is tearing down the
> adapter and is stuck in usb_unlocked_disable_lpm waiting to lock the
> hcd->handwidth_mutex. Thread 2 is holding this mutex (while still trying
> to add a usb device), but is stuck in xhci_endpoint_reset waiting for a
> stop or config command to complete. A reboot is required to resolve.
>
> It turns out when calling xhci_queue_stop_endpoint and
> xhci_queue_configure_endpoint in xhci_endpoint_reset, the return code is
> not checked for errors. If the timing is right and the adapter dies just
> before either of these commands get issued, we hang indefinitely waiting
> for a completion on a command that didn't get issued.
>
> This wasn't a problem before the following fix because we didn't send
> commands in xhci_endpoint_reset:
>
> commit f5249461b504 ("xhci: Clear the host side toggle manually when endpoint
> is soft reset")
>
> With the patch I am submitting, a duration test which breaks adapters
> during initialization (and which deadlocks with the standard kernel) runs
> without issue.
>
> Fixes: f5249461b504 ("xhci: Clear the host side toggle manually when endpoint
> is soft reset")
> Signed-off-by: Bill Kuzeja
> ---
> drivers/usb/host/xhci.c | 22 +++---
$ ./scripts/get_maintainer.pl --file drivers/usb/host/xhci.c
Mathias Nyman (supporter:USB XHCI DRIVER)
Greg Kroah-Hartman (supporter:USB SUBSYSTEM)
linux-usb@vger.kernel.org (open list:USB XHCI DRIVER)
linux-ker...@vger.kernel.org (open list)
I think you forgot to send this to the xhci driver maintainer for review
:(
Re: Adding "UAS" protocol line to usb.ids file?
On Sat, Aug 17, 2019 at 06:01:45PM -0400, Nathan Stratton Treadway wrote: > I noticed that when I use "lsusb -v" on a UAS-enabled drive enclosure, > the bInterfaceProtocol line for #80/0x50 has a "protocol name" label but the > one for #98/0x62 does not: > > > > # lsusb -v -s2:15 | grep Interface > bDeviceClass0 (Defined at Interface level) > bNumInterfaces 1 > Interface Descriptor: > bInterfaceNumber0 > bInterfaceClass 8 Mass Storage > bInterfaceSubClass 6 SCSI > bInterfaceProtocol 80 Bulk-Only > iInterface 0 > Interface Descriptor: > bInterfaceNumber0 > bInterfaceClass 8 Mass Storage > bInterfaceSubClass 6 SCSI > bInterfaceProtocol 98 > iInterface 0 > > > > > So...I was wondering if there was any particular reason that protocol > 98 isn't included in the usb.ids file? No one got around to it? Feel free to submit a patch to the web site that handles these to upate it. thanks, greg k-h
Re: [PATCH v3 2/2] USB: storage: ums-realtek: Whitelist auto-delink support
On Tue, Aug 27, 2019 at 02:56:36PM -0400, Alan Stern wrote:
> On Wed, 28 Aug 2019, Kai-Heng Feng wrote:
>
> > Auto-delink requires writing special registers to ums-realtek devices.
> > Unconditionally enable auto-delink may break newer devices.
> >
> > So only enable auto-delink by default for the original three IDs,
> > 0x0138, 0x0158 and 0x0159.
> >
> > Realtek is working on a patch to properly support auto-delink for other
> > IDs.
> >
> > BugLink: https://bugs.launchpad.net/bugs/1838886
> > Signed-off-by: Kai-Heng Feng
> > ---
> > v3:
> > - Whitelisting instead of adding new module parameter.
> > v2:
> > - Use auto_delink_support instead of auto_delink_enable.
> >
> > drivers/usb/storage/realtek_cr.c | 13 -
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/storage/realtek_cr.c
> > b/drivers/usb/storage/realtek_cr.c
> > index beaffac805af..1d9ce9cbc831 100644
> > --- a/drivers/usb/storage/realtek_cr.c
> > +++ b/drivers/usb/storage/realtek_cr.c
> > @@ -996,12 +996,15 @@ static int init_realtek_cr(struct us_data *us)
> > goto INIT_FAIL;
> > }
> >
> > - if (CHECK_FW_VER(chip, 0x5888) || CHECK_FW_VER(chip, 0x5889) ||
> > - CHECK_FW_VER(chip, 0x5901))
> > - SET_AUTO_DELINK(chip);
> > - if (STATUS_LEN(chip) == 16) {
> > - if (SUPPORT_AUTO_DELINK(chip))
> > + if (CHECK_PID(chip, 0x0138) || CHECK_PID(chip, 0x0158) ||
> > + CHECK_PID(chip, 0x0159)) {
> > + if (CHECK_FW_VER(chip, 0x5888) || CHECK_FW_VER(chip, 0x5889) ||
> > + CHECK_FW_VER(chip, 0x5901))
> > SET_AUTO_DELINK(chip);
> > + if (STATUS_LEN(chip) == 16) {
> > + if (SUPPORT_AUTO_DELINK(chip))
> > + SET_AUTO_DELINK(chip);
> > + }
> > }
> > #ifdef CONFIG_REALTEK_AUTOPM
> > if (ss_en)
>
> For both patches in this series:
>
> Acked-by: Alan Stern
>
> Shouldn't this patch go into the -stable kernels as well?
Yes, I can do that.
thanks,
greg k-h
Re: [PATCH 2/2] USB: storage: ums-realtek: Enable auto-delink optionally
On Mon, Aug 26, 2019 at 12:46:30PM +0800, Kai-Heng Feng wrote:
> Auto-delink requires writing special registers to ums-realtek device.
> Unconditionally enable auto-delink may break newer devices.
>
> So only enable auto-delink by default for the original three IDs,
> 0x0138, 0x0158 and 0x0159.
>
> Realtek is working on a patch to properly support auto-delink for other
> IDs.
>
> BugLink: https://bugs.launchpad.net/bugs/1838886
> Signed-off-by: Kai-Heng Feng
> ---
> drivers/usb/storage/realtek_cr.c | 24 +++-
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/storage/realtek_cr.c
> b/drivers/usb/storage/realtek_cr.c
> index 4d86cfcc0b40..376f41d0cbc3 100644
> --- a/drivers/usb/storage/realtek_cr.c
> +++ b/drivers/usb/storage/realtek_cr.c
> @@ -36,6 +36,10 @@ MODULE_DESCRIPTION("Driver for Realtek USB Card Reader");
> MODULE_AUTHOR("wwang ");
> MODULE_LICENSE("GPL");
>
> +static int auto_delink_enable = -1;
> +module_param(auto_delink_enable, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(auto_delink_enable, "enable auto delink (-1=auto [default],
> 0=disable, 1=enable)");
> +
> static int auto_delink_mode = 1;
> module_param(auto_delink_mode, int, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(auto_delink_mode, "auto delink mode (0=firmware, 1=software
> [default])");
> @@ -996,12 +1000,22 @@ static int init_realtek_cr(struct us_data *us)
> goto INIT_FAIL;
> }
This patch doesn't apply as I can't take your first patch. Can you redo
it and resend?
thanks,
greg k-h
Re: [PATCH 1/2] USB: storage: ums-realtek: Rename module parameter auto_delink_en to auto_delink_mode
On Mon, Aug 26, 2019 at 12:46:29PM +0800, Kai-Heng Feng wrote:
> The option named "auto_delink_en" is a bit misleading, as setting it to
> false doesn't really disable auto-delink but let auto-delink be firmware
> controlled.
>
> Rename it to reflect the real usage of this parameter.
>
> Signed-off-by: Kai-Heng Feng
> ---
> drivers/usb/storage/realtek_cr.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/storage/realtek_cr.c
> b/drivers/usb/storage/realtek_cr.c
> index cc794e25a0b6..4d86cfcc0b40 100644
> --- a/drivers/usb/storage/realtek_cr.c
> +++ b/drivers/usb/storage/realtek_cr.c
> @@ -36,9 +36,9 @@ MODULE_DESCRIPTION("Driver for Realtek USB Card Reader");
> MODULE_AUTHOR("wwang ");
> MODULE_LICENSE("GPL");
>
> -static int auto_delink_en = 1;
> -module_param(auto_delink_en, int, S_IRUGO | S_IWUSR);
> -MODULE_PARM_DESC(auto_delink_en, "enable auto delink");
> +static int auto_delink_mode = 1;
> +module_param(auto_delink_mode, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(auto_delink_mode, "auto delink mode (0=firmware, 1=software
> [default])");
We can not just rename module parameters, as that will break working
systems that have their startup scripts using those names :(
sorry,
greg k-h
Re: [Patch v5] usb: hcd: use managed device resources
On Fri, Aug 23, 2019 at 02:11:28PM +, Schmid, Carsten wrote: > Using managed device resources in usb_hcd_pci_probe() allows devm usage for > resource subranges, such as the mmio resource for the platform device > created to control host/device mode mux, which is a xhci extended > capability, and sits inside the xhci mmio region. > > If managed device resources are not used then "parent" resource > is released before subrange at driver removal as .remove callback is > called before the devres list of resources for this device is walked > and released. > > This has been observed with the xhci extended capability driver causing a > use-after-free which is now fixed. > > An additional nice benefit is that error handling on driver initialisation > is simplified much. > > Signed-off-by: Carsten Schmid > Tested-by: Carsten Schmid > --- > Rationale: > Use-after-free was reproduced on 4.14.102 and 4.14.129 kernel > using unbind mechanism. > echo :00:15.0 > /sys/bus/pci/drivers/xhci_hcd/unbind > > Upstream version of driver is identical in the affected code. > Fix was tested successfully on 4.14.129. > Provided patch applies and compiles on v5.2.8 stable. > As this is also a bugfix, please consider it to go to stable trees too. How far back should it go, just 4.14? Was this caused by a specific commit that you happened to notice? thanks, greg k-h
Re: [RFC 2/3] ACPI: move ACPI functionality out of r8152 driver
On Fri, Aug 23, 2019 at 10:28:24PM +, charles.h...@dellteam.com wrote:
> --- /dev/null
> +++ b/lib/acpi_mac_passthru.c
> @@ -0,0 +1,61 @@
> +/*
> + * Copyright (c) 2019 Dell Technology. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + */
You didn't run your patch through checkpatch.pl :(
Anyway, drop the license boilerplate please and use a SPDX line, like
checkpatch asks you to.
> +
> +#include
> +#include
> +
> +int get_acpi_mac_passthru(struct device *dev, struct sockaddr *sa)
> +{
> +#ifdef CONFIG_ACPI
> + acpi_status status;
> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> + union acpi_object *obj;
> + int ret = -EINVAL;
> + unsigned char buf[6];
> +
> + /* returns _AUXMAC_#AABBCCDDEEFF# */
> + status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer);
> + obj = (union acpi_object *)buffer.pointer;
> + if (!ACPI_SUCCESS(status))
> + return -ENODEV;
> + if (obj->type != ACPI_TYPE_BUFFER || obj->string.length != 0x17) {
> + dev_warn(dev,
> + "Invalid buffer for pass-thru MAC addr: (%d, %d)\n",
> + obj->type, obj->string.length);
> + goto amacout;
> + }
> + if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0 ||
> + strncmp(obj->string.pointer + 0x15, "#", 1) != 0) {
> + dev_warn(dev,
> + "Invalid header when reading pass-thru MAC addr\n");
> + goto amacout;
> + }
> + ret = hex2bin(buf, obj->string.pointer + 9, 6);
> + if (!(ret == 0 && is_valid_ether_addr(buf))) {
> + dev_warn(dev,
> + "Invalid MAC for pass-thru MAC addr: %d, %pM\n",
> + ret, buf);
> + ret = -EINVAL;
> + goto amacout;
> + }
> + memcpy(sa->sa_data, buf, 6);
> + dev_info(dev, "Pass-thru MAC addr %pM\n", sa->sa_data);
> +
> +amacout:
> + kfree(obj);
> + return ret;
> +
> +#else/* !CONFIG_ACPI */
> + (void)dev;
> + (void)sa;
> +
> + return -ENODEV;
No #ifdef in .c files, especially for something as trivial as this. The
#ifdef needs to be in the .h file, and don't build this unless acpi is
enabled. And then, just move this to the acpi core, not in lib/
thanks,
greg k-h
Re: [PATCH v5 1/2] usbip: Skip DMA mapping and unmapping for urb at vhci
On Fri, Aug 09, 2019 at 12:54:34AM +0900, Suwan Kim wrote: > vhci doesn’t do DMA for remote device. Actually, the real DMA > operation is done by network card driver. vhci just passes virtual > address of the buffer to the network stack, so vhci doesn’t use and > need dma address of the buffer of the URB. > > But HCD provides DMA mapping and unmapping function by default. > Moreover, it causes unnecessary DMA mapping and unmapping which > will be done again at the NIC driver and it wastes CPU cycles. > So, implement map_urb_for_dma and unmap_urb_for_dma function for > vhci in order to skip the DMA mapping and unmapping procedure. > > When it comes to supporting SG for vhci, it is useful to use native > SG list (urb->num_sgs) instead of mapped SG list because DMA mapping > fnuction can adjust the number of SG list (urb->num_mapped_sgs). > And vhci_map_urb_for_dma() prevents isoc pipe from using SG as > hcd_map_urb_for_dma() does. > > Signed-off-by: Suwan Kim Can you please redo this patch based on my usb-next branch that has Christoph's DMA changes in it? It should make your change much simpler and smaller. Please do that and resend the whole series. thanks, greg k-h
Re: problems with Edgeport/416
On Wed, Aug 21, 2019 at 03:04:48PM +0200, walter harms wrote: > > > Am 21.08.2019 14:20, schrieb Greg KH: > > On Wed, Aug 21, 2019 at 01:48:46PM +0200, walter harms wrote: > >> > >> > >> Am 21.08.2019 13:20, schrieb Greg KH: > >>> On Wed, Aug 21, 2019 at 12:27:24PM +0200, walter harms wrote: > >>>> Hello List, > >>>> does some use linux with an Edgeport/416 ? > >>>> > >>>> I have a strange problem. the device is resetting soon > >>>> after i started using it (but not immediately). > >>>> I do not see a kernel OOPS but a common pattern is: > >>>> > >>>> 2019-08-20T15:19:39.825812+00:00 omnfrmo10 kernel: [683270.658623] usb > >>>> 7-1.1.2: New USB device strings: Mfr=1, Product=2, SerialNumber=3 > >>>> 2019-08-20T15:19:39.825818+00:00 omnfrmo10 kernel: [683270.658626] usb > >>>> 7-1.1.2: Product: Edgeport/416 > >>>> 2019-08-20T15:19:39.825821+00:00 omnfrmo10 kernel: [683270.658628] usb > >>>> 7-1.1.2: Manufacturer: Digi International > >>>> 2019-08-20T15:19:39.825823+00:00 omnfrmo10 kernel: [683270.658630] usb > >>>> 7-1.1.2: SerialNumber: E63966100-1 > >>>> 2019-08-20T15:19:39.985571+00:00 omnfrmo10 kernel: [683270.817909] usb > >>>> 7-1.1.2: Edgeport TI 2 port adapter converter now attached to ttyUSB4 > >>>> 2019-08-20T15:19:39.985594+00:00 omnfrmo10 kernel: [683270.818132] usb > >>>> 7-1.1.2: Edgeport TI 2 port adapter converter now attached to ttyUSB5 > >>>> 2019-08-20T15:19:40.007943+00:00 omnfrmo10 mtp-probe: checking bus 7, > >>>> device 88: "/sys/devices/pci:00/:00:1d.1/usb7/7-1/7-1.1/7-1.1.2" > >>>> 2019-08-20T15:19:40.053750+00:00 omnfrmo10 kernel: [683270.885626] usb > >>>> 7-1.2.2: New USB device found, idVendor=1608, idProduct=0247 > >>>> 2019-08-20T15:19:40.053791+00:00 omnfrmo10 kernel: [683270.885630] usb > >>>> 7-1.2.2: New USB device strings: Mfr=1, Product=2, SerialNumber=3 > >>>> 2019-08-20T15:19:40.053797+00:00 omnfrmo10 kernel: [683270.885633] usb > >>>> 7-1.2.2: Product: Edgeport/416 > >>>> 2019-08-20T15:19:40.053800+00:00 omnfrmo10 kernel: [683270.885635] usb > >>>> 7-1.2.2: Manufacturer: Digi International > >>>> 2019-08-20T15:19:40.053803+00:00 omnfrmo10 kernel: [683270.885637] usb > >>>> 7-1.2.2: SerialNumber: E63966100-5 > >>>> 2019-08-20T15:19:40.065569+00:00 omnfrmo10 kernel: [683270.897406] usb > >>>> 7-1.1.3: new full-speed USB device number 90 using uhci_hcd > >>>> 2019-08-20T15:19:40.213569+00:00 omnfrmo10 kernel: [683271.046316] usb > >>>> 7-1.2.2: Edgeport TI 2 port adapter converter now attached to ttyUSB6 > >>>> 2019-08-20T15:19:40.213594+00:00 omnfrmo10 kernel: [683271.046782] usb > >>>> 7-1.2.2: Edgeport TI 2 port adapter converter now attached to ttyUSB7 > >>>> 2019-08-20T15:19:40.242034+00:00 omnfrmo10 mtp-probe: checking bus 7, > >>>> device 89: "/sys/devices/pci:00/:00:1d.1/usb7/7-1/7-1.2/7-1.2.2" > >>>> 2019-08-20T15:19:40.301578+00:00 omnfrmo10 kernel: [683271.133380] usb > >>>> 7-1.2.3: new full-speed USB device number 91 using uhci_hcd > >>>> 2019-08-20T15:19:40.357559+00:00 omnfrmo10 kernel: [683271.192815] usb > >>>> 7-1.1.3: New USB device found, idVendor=1608, idProduct=0247 > >>>> 2019-08-20T15:19:40.357584+00:00 omnfrmo10 kernel: [683271.192820] usb > >>>> 7-1.1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=3 > >>>> 2019-08-20T15:19:40.357588+00:00 omnfrmo10 kernel: [683271.192822] usb > >>>> 7-1.1.3: Product: Edgeport/416 > >>>> 2019-08-20T15:19:40.357591+00:00 omnfrmo10 kernel: [683271.192825] usb > >>>> 7-1.1.3: Manufacturer: Digi International > >>>> 2019-08-20T15:19:40.357593+00:00 omnfrmo10 kernel: [683271.192827] usb > >>>> 7-1.1.3: SerialNumber: E63966100-2 > >>>> 2019-08-20T15:19:40.513702+00:00 omnfrmo10 kernel: [683271.349103] usb > >>>> 7-1.1.3: Edgeport TI 2 port adapter converter now attached to ttyUSB8 > >>>> 2019-08-20T15:19:40.513725+00:00 omnfrmo10 kernel: [683271.349311] usb > >>>> 7-1.1.3: Edgeport TI 2 port adapter converter now attached to ttyUSB9 > >>>> 2019-08-20T15:19:40.537138+00:00 omnfrmo10 mtp-probe: checking bus 7, > >>>> device 90: "/sys/devices/pci000
Re: [PATCH v3 09/11] usb-storage: remove single-use define for debugging
On Wed, Aug 21, 2019 at 03:21:22PM +0200, Thomas Gleixner wrote:
> On Wed, 21 Aug 2019, Matthias Maennich wrote:
>
> > USB_STORAGE was defined as "usb-storage: " and used in a single location
> > as argument to printk. In order to be able to use the name
> > 'USB_STORAGE', drop the definition and use the string directly for the
> > printk call.
> >
> > Signed-off-by: Matthias Maennich
> > ---
> > drivers/usb/storage/debug.h| 2 --
> > drivers/usb/storage/scsiglue.c | 2 +-
> > 2 files changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/storage/debug.h b/drivers/usb/storage/debug.h
> > index 6d64f342f587..16ce06039a4d 100644
> > --- a/drivers/usb/storage/debug.h
> > +++ b/drivers/usb/storage/debug.h
> > @@ -29,8 +29,6 @@
> >
> > #include
> >
> > -#define USB_STORAGE "usb-storage: "
> > -
> > #ifdef CONFIG_USB_STORAGE_DEBUG
> > void usb_stor_show_command(const struct us_data *us, struct scsi_cmnd
> > *srb);
> > void usb_stor_show_sense(const struct us_data *us, unsigned char key,
> > diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
> > index 05b80211290d..df4de8323eff 100644
> > --- a/drivers/usb/storage/scsiglue.c
> > +++ b/drivers/usb/storage/scsiglue.c
> > @@ -379,7 +379,7 @@ static int queuecommand_lck(struct scsi_cmnd *srb,
> >
> > /* check for state-transition errors */
> > if (us->srb != NULL) {
> > - printk(KERN_ERR USB_STORAGE "Error in %s: us->srb = %p\n",
> > + printk(KERN_ERR "usb-storage: Error in %s: us->srb = %p\n",
> > __func__, us->srb);
>
> The proper fix for this is to use pr_fmt and convert the printk to pr_err().
Yeah, that's the correct long-term fix, I think someone already sent
that in for the usb tree, where I have taken this patch already.
thanks,
greg k-h
Re: [PATCH v3 10/11] RFC: usb-storage: export symbols in USB_STORAGE namespace
On Wed, Aug 21, 2019 at 12:49:25PM +0100, Matthias Maennich wrote: > Modules using these symbols are required to explicitly import the > namespace. This patch was generated with the following steps and serves > as a reference to use the symbol namespace feature: > > 1) Define DEFAULT_SYMBOL_NAMESPACE in the corresponding Makefile > 2) make (see warnings during modpost about missing imports) > 3) make nsdeps > > Instead of a DEFAULT_SYMBOL_NAMESPACE definition, the EXPORT_SYMBOL_NS > variants can be used to explicitly specify the namespace. The advantage > of the method used here is that newly added symbols are automatically > exported and existing ones are exported without touching their > respective EXPORT_SYMBOL macro expansion. > > Signed-off-by: Matthias Maennich This looks good to me. This can be included with the rest of this series when/if it goes through the kbuild or module tree: Reviewed-by: Greg Kroah-Hartman Actually, which tree will this be going through? thanks, greg k-h
Re: [PATCH v3 09/11] usb-storage: remove single-use define for debugging
On Wed, Aug 21, 2019 at 12:49:24PM +0100, Matthias Maennich wrote: > USB_STORAGE was defined as "usb-storage: " and used in a single location > as argument to printk. In order to be able to use the name > 'USB_STORAGE', drop the definition and use the string directly for the > printk call. > > Signed-off-by: Matthias Maennich As you know, this patch is already in the usb-next tree, and will be in the 5.4-rc1 merge. But, as this series will end up going through a different tree than the usb tree, here's my reviewed-by so that it can be included with the rest of these patches: Reviewed-by: Greg Kroah-Hartman
Re: f_mass_storage vs drivers/target
On Wed, Aug 21, 2019 at 01:38:49PM +1000, Benjamin Herrenschmidt wrote: > Hi folks ! > > It seems that f_mass_storage duplicates (well maybe predates too..) a > lot of what's in drivers/target. It predates it by a long time. > Anybody working on implementing a new version of f_mass_storage that > is layered upon drivers/target instead ? That would bring quite a lot > of additional functionality. Why is that needed? What functionality is missing that it will provide? Will it make the code simpler? > If not, I might look into it. Hey, we don't refuse patches, for cleaning stuff up, you know that :) thanks, greg k-h
Re: [RFC 1/4] Add usb_get_address and usb_set_address support
On Tue, Aug 20, 2019 at 10:18:42PM +, charles.h...@dellteam.com wrote:
> +int usb_get_address(struct usb_device *dev, unsigned char * mac)
> +{
> + int ret = -ENOMEM;
> + unsigned char *tbuf = kmalloc(256, GFP_NOIO);
On a technical level, why are you asking for 256 bytes here, and in the
control message, yet assuming you will only get 6 back for a correct
message? Shouldn't you be only asking for 6 bytes?
> +
> + if (!tbuf)
> + return -ENOMEM;
> +
> + ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> + USB_CDC_GET_NET_ADDRESS,
> + USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> + 0, USB_REQ_SET_ADDRESS, tbuf, 256,
> + USB_CTRL_GET_TIMEOUT);
> + if (ret == 6)
> + memcpy(mac, tbuf, 6);
> +
> + kfree(tbuf);
> + return ret;
So if 100 is returned by the device (not likely, but let's say 7), then
you return 7 bytes, yet you did not copy the data into the pointer given
to you.
SHouldn't you report a real error for when this happens (hint, it will.)
thanks,
greg k-h
Re: [RFC 1/4] Add usb_get_address and usb_set_address support
On Tue, Aug 20, 2019 at 10:18:42PM +, charles.h...@dellteam.com wrote:
> The core USB driver message.c is missing get/set address functionality
> that stops ifconfig from being able to push MAC addresses out to USB
> based ethernet devices. Without this functionality, some USB devices
> stop responding to ethernet packets when using ifconfig to change MAC
> addresses. This has been tested with a Dell Universal Dock D6000.
>
> Signed-off-by: Charles Hyde
> Cc: Mario Limonciello
> Cc: Greg Kroah-Hartman
> Cc: linux-usb@vger.kernel.org
> ---
> drivers/usb/core/message.c | 59 ++
> include/linux/usb.h| 3 ++
> 2 files changed, 62 insertions(+)
>
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 5adf489428aa..eea775234b09 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1085,6 +1085,65 @@ int usb_clear_halt(struct usb_device *dev, int pipe)
> }
> EXPORT_SYMBOL_GPL(usb_clear_halt);
>
> +/**
> + * usb_get_address -
trailing whitespace?
No description of what the function does?
> + * @dev: device whose endpoint is halted
> + * @mac: buffer for containing
Trailing whitespace?
> + * Context: !in_interrupt ()
> + *
> + * This will attempt to get the six byte MAC address from a USB device's
> + * ethernet controller using GET_NET_ADDRESS command.
> + *
> + * This call is synchronous, and may not be used in an interrupt context.
> + *
> + * Return: Zero on success, or else the status code returned by the
> + * underlying usb_control_msg() call.
> + */
> +int usb_get_address(struct usb_device *dev, unsigned char * mac)
> +{
> + int ret = -ENOMEM;
> + unsigned char *tbuf = kmalloc(256, GFP_NOIO);
> +
> + if (!tbuf)
> + return -ENOMEM;
> +
> + ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> + USB_CDC_GET_NET_ADDRESS,
> + USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> + 0, USB_REQ_SET_ADDRESS, tbuf, 256,
> + USB_CTRL_GET_TIMEOUT);
> + if (ret == 6)
> + memcpy(mac, tbuf, 6);
> +
> + kfree(tbuf);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_get_address);
This is a VERY cdc-net-specific function. It is not a "generic" USB
function at all. Why does it belong in the USB core? Shouldn't it live
in the code that handles the other cdc-net-specific logic?
thanks,
greg k-h
Re: [PATCH] usb: usb251xb: drop some unused defines
On Mon, Aug 19, 2019 at 12:02:11PM +0200, Uwe Kleine-König wrote: > Signed-off-by: Uwe Kleine-König > --- > drivers/usb/misc/usb251xb.c | 5 - > 1 file changed, 5 deletions(-) I can't take a patch without any changelog text. And you forgot to cc: the usb maintainer, so there's no way this was going to get merged :) thanks, greg k-h
[GIT PULL] USB fixes for 5.3-rc5
The following changes since commit d45331b00ddb179e291766617259261c112db872: Linux 5.3-rc4 (2019-08-11 13:26:41 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git tags/usb-5.3-rc5 for you to fetch changes up to 6a5f43d1d8bd3779e428178438caf33f60427124: Merge tag 'usb-serial-5.3-rc5' of https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial into usb-linus (2019-08-17 17:09:33 +0200) USB fixes for 5.3-rc5 Here are number of small USB fixes for 5.3-rc5. Syzbot has been on a tear recently now that it has some good USB debugging hooks integrated, so there's a number of fixes in here found by those tools for some _very_ old bugs. Also a handful of gadget driver fixes for reported issues, some hopefully-final dma fixes for host controller drivers, and some new USB serial gadget driver ids. All of these have been in linux-next this week with no reported issues (the usb-serial ones were in linux-next in its own branch, but merged into mine on Friday.) Signed-off-by: Greg Kroah-Hartman Alan Stern (1): USB: core: Fix races in character device registration and deregistraion André Draszik (1): usb: chipidea: imx: fix EPROBE_DEFER support during driver probe Benjamin Herrenschmidt (2): usb: gadget: composite: Clear "suspended" on reset/disconnect usb: gadget: mass_storage: Fix races between fsg_disable and fsg_set_alt Bob Ham (1): USB: serial: option: add the BroadMobi BM818 card Christoph Hellwig (2): usb: don't create dma pools for HCDs with a localmem_pool usb: add a hcd_uses_dma helper Greg Kroah-Hartman (2): Merge tag 'fixes-for-v5.3-rc4' of git://git.kernel.org/.../balbi/usb into usb-linus Merge tag 'usb-serial-5.3-rc5' of https://git.kernel.org/.../johan/usb-serial into usb-linus Hans Ulli Kroll (1): usb: host: fotg2: restart hcd after port reset Oliver Neukum (2): usb: cdc-acm: make sure a refcount is taken early enough USB: CDC: fix sanity checks in CDC union parser Rogan Dawes (1): USB: serial: option: add D-Link DWM-222 device ID Tony Lindgren (1): USB: serial: option: Add Motorola modem UARTs Yoshiaki Okamoto (1): USB: serial: option: Add support for ZTE MF871A Yoshihiro Shimoda (1): usb: gadget: udc: renesas_usb3: Fix sysfs interface of "role" drivers/usb/chipidea/ci_hdrc_imx.c | 19 --- drivers/usb/class/cdc-acm.c | 12 +++- drivers/usb/core/buffer.c| 10 +++--- drivers/usb/core/file.c | 10 +- drivers/usb/core/hcd.c | 4 ++-- drivers/usb/core/message.c | 4 ++-- drivers/usb/dwc2/hcd.c | 2 +- drivers/usb/gadget/composite.c | 1 + drivers/usb/gadget/function/f_mass_storage.c | 28 ++-- drivers/usb/gadget/udc/renesas_usb3.c| 5 +++-- drivers/usb/host/fotg210-hcd.c | 4 drivers/usb/serial/option.c | 10 ++ include/linux/usb.h | 2 +- include/linux/usb/hcd.h | 3 +++ 14 files changed, 72 insertions(+), 42 deletions(-)
Re: kernel 5.2.7 support for usb Apple Superdrive is broken
On Fri, Aug 16, 2019 at 02:24:18PM -0500, David Walthour wrote: > I am new to this mailing list, so please go easy on me if I'm breaking > etiquete. > > Upgraded my Fedora 30 install from linux kernel 5.2.6 to 5.2.7 (and > 5.2.8) and my apple usb superdrive stopped working. Booting up in 5.2.6 > and it works again as expected, so it appears to be due to changes in > 5.2.7. > > I am unsure of where to look to see if this is already reported and > being fixed. If not, I am willing to help out in fixing it as it seems > to be a reasonably small set of diffs between 5.2.6 and 5.2.7 that > could account for it, but I am new to patching linux, so I am unsure of > where to get started. Any help would be appreciated. I would start with filing a bug in fedora's bugzilla as the developers there can help you try to narrow things down better than we can if you can't build your own kernel images. And, what do you mean by "not working", are there kernel log messages? thanks, greg k-h
Re: [PATCH v5 0/2] usbip: Implement SG support
On Wed, Aug 14, 2019 at 06:19:51AM -0700, Christoph Hellwig wrote: > FYI, I think my > >"usb: add a HCD_DMA flag instead of guestimating DMA capabilities" > > is the proper core fix for what your patch 1 works around, as the USB > core should not assume DMA capabilities based on the presence of a DMA > mask. I agree. Let's wait for Christoph's series to be applied before taking this one. thanks, greg k-h
Re: [PATCH] usb: usbfs: only account once for mmap()'ed usb memory usage
On Wed, Aug 14, 2019 at 02:29:24PM -0700, gavi...@thegavinli.com wrote: > From: Gavin Li > > Memory usage for USB memory allocated via mmap() is already accounted > for at mmap() time; no need to account for it again at submiturb time. > > Signed-off-by: Gavin Li > --- > drivers/usb/core/devio.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) What commit does this fix? What issue does this fix, is it something that is user-visable? > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c > index bbe9c2edd3e7..9681dd55473b 100644 > --- a/drivers/usb/core/devio.c > +++ b/drivers/usb/core/devio.c > @@ -1603,7 +1603,8 @@ static int proc_do_submiturb(struct usb_dev_state *ps, > struct usbdevfs_urb *uurb > if (as->usbm) > num_sgs = 0; > > - u += sizeof(struct async) + sizeof(struct urb) + uurb->buffer_length + > + u += sizeof(struct async) + sizeof(struct urb) + > + (as->usbm ? 0 : uurb->buffer_length) + >num_sgs * sizeof(struct scatterlist); Are you sure? Where is the buffer_length being added to the size here? What am I missing? thanks, greg k-h
Re: [PATCH] USB: rio500: Fix lockdep violation
On Thu, Aug 08, 2019 at 02:23:00PM -0400, Alan Stern wrote:
> On Thu, 8 Aug 2019, Greg KH wrote:
>
> > On Thu, Aug 08, 2019 at 01:34:08PM -0400, Alan Stern wrote:
> > > The syzbot fuzzer found a lockdep violation in the rio500 driver:
> > >
> > > ==
> > > WARNING: possible circular locking dependency detected
> > > 5.3.0-rc2+ #23 Not tainted
> > > --
> > > syz-executor.2/20386 is trying to acquire lock:
> > > 772249c6 (rio500_mutex){+.+.}, at: open_rio+0x16/0xc0
> > > drivers/usb/misc/rio500.c:64
> > >
> > > but task is already holding lock:
> > > d3e8f4b9 (minor_rwsem){}, at: usb_open+0x23/0x270
> > > drivers/usb/core/file.c:39
> > >
> > > which lock already depends on the new lock.
> > >
> > > The problem is that the driver's open_rio() routine is called while
> > > the usbcore's minor_rwsem is locked for reading, and it acquires the
> > > rio500_mutex; whereas conversely, probe_rio() and disconnect_rio()
> > > first acquire the rio500_mutex and then call usb_register_dev() or
> > > usb_deregister_dev(), which lock minor_rwsem for writing.
> > >
> > > The correct ordering of acquisition should be: minor_rwsem first, then
> > > rio500_mutex (since the locking in open_rio() cannot be changed).
> > > Thus, the probe and disconnect routines should avoid holding
> > > rio500_mutex while doing their registration and deregistration.
> > >
> > > This patch adjusts the code in those two routines to do just that. It
> > > also relies on the fact that the probe and disconnect routines are
> > > protected by the device mutex, so the initial test of rio->present
> > > needs no extra locking.
> > >
> > > Reported-by: syzbot+7bbcbe9c9ff0cd495...@syzkaller.appspotmail.com
> > > Signed-off-by: Alan Stern
> > > Fixes: d710734b0677 ("USB: rio500: simplify locking")
> > > CC: Oliver Neukum
> > > CC:
> > >
> > > ---
> > >
> > > This patch is different from the one I posted earlier. I realized that
> > > we don't want to register the device's char file until after the
> > > buffers have been allocated.
> >
> > Should I revert Oliver's patch?
>
> Sorry, I should have explained more clearly: This goes on top of
> Oliver's patch. In fact, Oliver's patch is the one listed in the
> Fixes: tag.
>
> You do not need to apply Oliver's reversion. Assuming he agrees that
> this patch is correct, of course.
Ok, I applied the revert, and that's in 5.3-rc4. So of course this does
not apply :)
Shoudl I revert the revert and then apply this? I will if I can get an
ack from Oliver...
thanks,
greg k-h
Re: USB disconnect due to error from flowcontrol urb
On Thu, Aug 15, 2019 at 10:41:05AM +0200, Bjoern wrote: > Hi, > > > I'm running on Ubuntu Bionic 4.14.94-155 on an odroid-x4 (armhf). > > > I have a FT230X Basic UART (0403:6015) plugged into the exynos-ohci USB. The > device is linked via a udev rule to a "symbolic/virtual" device so that the > software FHEM can address it independent of the real ttyUSB* name. > > > I constantly see disconnects for the device in the kernel log, see log > excerpts below. I've found similar issues online going back till 2012 but > there seems to be no solution so far. I'd like to help find the cause if > possible. > > Regards, > Bjoern > > > Aug 4 11:25:11 fhem2 kernel: [2476586.963898] usb 2-1: USB disconnect, > device number 66 Your device removed itself from the system electronically, the other errors you report here are the kernel trying to clean up after itself. Try fixing the cable for the device, or the power provided to it, this is a physical issue, not a logical one that the kernel can help with, sorry. thanks, greg k-h
Re: [PATCH] From: cy_huang Subject: usb: add more vendor defined ops in tcpci
On Thu, Aug 15, 2019 at 12:19:13PM +0800, cy_huang wrote:
> diff --git a/drivers/usb/typec/tcpm/tcpci.h b/drivers/usb/typec/tcpm/tcpci.h
> index 303ebde..a6754fb 100644
> --- a/drivers/usb/typec/tcpm/tcpci.h
> +++ b/drivers/usb/typec/tcpm/tcpci.h
> @@ -130,6 +130,11 @@ struct tcpci_data {
>bool enable);
> int (*start_drp_toggling)(struct tcpci *tcpci, struct tcpci_data *data,
> enum typec_cc_status cc);
> + int (*set_vbus)(struct tcpci *tcpci,
> + struct tcpci_data *data, bool source, bool sink);
> + int (*get_current_limit)(struct tcpci *tcpci, struct tcpci_data *data);
> + int (*set_current_limit)(struct tcpci *tcpci,
> + struct tcpci_data *data, u32 max_ma, u32 mv);
> };
You are adding callbacks here with no users of them, which isn't
allowed. Please also submit the code that uses these callbacks at the
same time so we can review it all together.
thanks,
greg k-h
Re: [PATCH] From: cy_huang Subject: usb: add more vendor defined ops in tcpci
On Thu, Aug 15, 2019 at 12:19:13PM +0800, cy_huang wrote: > From: cy_huang Your subject line is a bit odd :) Also, we need a "real" name here, and in the signed-off-by line, not an email prefix. Please fix up and resend. thanks, greg k-h
Re: TL-MR3420 with OpenWRT with a huawei E353 usb dongle
On Wed, Aug 14, 2019 at 11:42:53AM -0300, Francisco Ferreiro wrote: > Hi guys, this is me trying to setup a tp-link TL-MR3420 with OpenWRT > with a huawei E353 usb dongle > > hopefully I will try to setup a multiwan along with this two more > sources to get redundant access to internet > - a cell phone (either tethering or if possible via USB (*)) > - a fiber based dsl service accesible via ethernet. > > after flashing the OpenWRT and setting up a little bit I made the > dongle work manually (via ttyUSB0) (log below) but cant get it to > automatically start connected from boot up > > help with this dongle and maybe with this (*) one too, would be highly > appreciated > have some logs below > and let me know if you need something else > > thanks in advance for your help > best > Franco > > [ 25.113754] usb 1-1: new high-speed USB device number 3 using ehci-platform > [ 25.327869] usbserial_generic 1-1:1.0: The "generic" usb-serial > driver is only for testing and one-off prototypes. > [ 25.338456] usbserial_generic 1-1:1.0: Tell > linux-usb@vger.kernel.org to add your device to a proper driver. This really is not the proper driver for this device. Why are you forcing it to use the generic one and not a "real" driver? > [ 25.348448] usbserial_generic 1-1:1.0: generic converter detected > [ 25.354938] usb 1-1: generic converter now attached to ttyUSB0 > [ 25.361410] usbserial_generic 1-1:1.1: The "generic" usb-serial > driver is only for testing and one-off prototypes. > [ 25.371975] usbserial_generic 1-1:1.1: Tell > linux-usb@vger.kernel.org to add your device to a proper driver. > [ 25.381965] usbserial_generic 1-1:1.1: device has no bulk endpoints > [ 25.388921] usbserial_generic 1-1:1.2: The "generic" usb-serial > driver is only for testing and one-off prototypes. > [ 25.399477] usbserial_generic 1-1:1.2: Tell > linux-usb@vger.kernel.org to add your device to a proper driver. > [ 25.409453] usbserial_generic 1-1:1.2: generic converter detected > [ 25.415898] usb 1-1: generic converter now attached to ttyUSB1 > [ 25.422331] usbserial_generic 1-1:1.3: The "generic" usb-serial > driver is only for testing and one-off prototypes. > [ 25.432887] usbserial_generic 1-1:1.3: Tell > linux-usb@vger.kernel.org to add your device to a proper driver. > [ 25.442869] usbserial_generic 1-1:1.3: generic converter detected > [ 25.449323] usb 1-1: generic converter now attached to ttyUSB2 > [ 25.455865] usb-storage 1-1:1.4: USB Mass Storage device detected > [ 25.923729] scsi host0: usb-storage 1-1:1.4 > [ 25.928826] usb-storage 1-1:1.5: USB Mass Storage device detected > [ 25.983950] scsi host1: usb-storage 1-1:1.5 > [ 26.986403] scsi 0:0:0:0: CD-ROMHUAWEI Mass Storage > 2.31 PQ: 0 ANSI: 2 > [ 27.074885] scsi 1:0:0:0: Direct-Access HUAWEI SD Storage > 2.31 PQ: 0 ANSI: 2 > [ 27.089310] sd 1:0:0:0: [sda] Attached SCSI removable disk Did you run the usb-switch program (or whatever that thing is called that toggles devices out of mass-storage mode back into modem mode), on this device? That should solve this issue, right? thanks, greg k-h
Re: [PATCH v2 07/10] modpost: add support for generating namespace dependencies
On Tue, Aug 13, 2019 at 01:17:04PM +0100, Matthias Maennich wrote: > This patch adds an option to modpost to generate a .ns_deps file > per module, containing the namespace dependencies for that module. > > E.g. if the linked module my-module.ko would depend on the symbol > myfunc.MY_NS in the namespace MY_NS, the my-module.ns_deps file created > by modpost would contain the entry MY_NS to express the namespace > dependency of my-module imposed by using the symbol myfunc. > > These files can subsequently be used by static analysis tools (like > coccinelle scripts) to address issues with missing namespace imports. A > later patch of this series will introduce such a script 'nsdeps' and a > corresponding make target to automatically add missing > MODULE_IMPORT_NS() definitions to the module's sources. For that it uses > the information provided in the generated .ns_deps files. > > Co-developed-by: Martijn Coenen > Signed-off-by: Martijn Coenen > Signed-off-by: Matthias Maennich > --- > scripts/mod/modpost.c | 61 +++ > scripts/mod/modpost.h | 2 ++ > 2 files changed, 58 insertions(+), 5 deletions(-) Reviewed-by: Greg Kroah-Hartman
Re: [PATCH v2 05/10] module: add config option MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS
On Tue, Aug 13, 2019 at 01:17:02PM +0100, Matthias Maennich wrote:
> If MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS is enabled (default=n), the
> requirement for modules to import all namespaces that are used by
> the module is relaxed.
>
> Enabling this option effectively allows (invalid) modules to be loaded
> while only a warning is emitted.
>
> Disabling this option keeps the enforcement at module loading time and
> loading is denied if the module's imports are not satisfactory.
>
> Reviewed-by: Martijn Coenen
> Signed-off-by: Matthias Maennich
> ---
> init/Kconfig| 14 ++
> kernel/module.c | 11 +--
> 2 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index bd7d650d4a99..b3373334cdf1 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -2119,6 +2119,20 @@ config MODULE_COMPRESS_XZ
>
> endchoice
>
> +config MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS
> + bool "Allow loading of modules with missing namespace imports"
> + default n
the default for config options is always N, no need to list it here.
> + help
> + Symbols exported with EXPORT_SYMBOL_NS*() are considered exported in
> + a namespace. A module that makes use of a symbol exported with such a
> + namespace is required to import the namespace via MODULE_IMPORT_NS().
> + This option relaxes this requirement when loading a module. While
> + technically there is no reason to enforce correct namespace imports,
> + it creates consistency between symbols defining namespaces and users
> + importing namespaces they make use of.
> +
> + If unsure, say N.
> +
> config TRIM_UNUSED_KSYMS
> bool "Trim unused exported kernel symbols"
> depends on MODULES && !UNUSED_SYMBOLS
> diff --git a/kernel/module.c b/kernel/module.c
> index 57e8253f2251..7c934aaae2d3 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1408,9 +1408,16 @@ static int verify_namespace_is_imported(const struct
> load_info *info,
> imported_namespace = get_next_modinfo(
> info, "import_ns", imported_namespace);
> }
> - pr_err("%s: module uses symbol (%s) from namespace %s, but does
> not import it.\n",
> -mod->name, kernel_symbol_name(sym), namespace);
> +#ifdef CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS
> + pr_warn(
> +#else
> + pr_err(
> +#endif
> + "%s: module uses symbol (%s) from namespace %s, but
> does not import it.\n",
> + mod->name, kernel_symbol_name(sym), namespace);
> +#ifndef CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS
> return -EINVAL;
> +#endif
This #ifdef mess is a hack, but oh well :)
If you drop the above default line, feel free to add:
Reviewed-by: Greg Kroah-Hartman
Re: [PATCH v2 06/10] export: allow definition default namespaces in Makefiles or sources
On Tue, Aug 13, 2019 at 01:17:03PM +0100, Matthias Maennich wrote:
> To avoid excessive usage of EXPORT_SYMBOL_NS(sym, MY_NAMESPACE), where
> MY_NAMESPACE will always be the namespace we are exporting to, allow
> exporting all definitions of EXPORT_SYMBOL() and friends by defining
> DEFAULT_SYMBOL_NAMESPACE.
>
> For example, to export all symbols defined in usb-common into the
> namespace USB_COMMON, add a line like this to drivers/usb/common/Makefile:
>
> ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=USB_COMMON
>
> That is equivalent to changing all EXPORT_SYMBOL(sym) definitions to
> EXPORT_SYMBOL_NS(sym, USB_COMMON). Subsequently all symbol namespaces
> functionality will apply.
>
> Another way of making use of this feature is to define the namespace
> within source or header files similar to how TRACE_SYSTEM defines are
> used:
> #undef DEFAULT_SYMBOL_NAMESPACE
> #define DEFAULT_SYMBOL_NAMESPACE USB_COMMON
>
> Please note that, as opposed to TRACE_SYSTEM, DEFAULT_SYMBOL_NAMESPACE
> has to be defined before including include/linux/export.h.
>
> If DEFAULT_SYMBOL_NAMESPACE is defined, a symbol can still be exported
> to another namespace by using EXPORT_SYMBOL_NS() and friends with
> explicitly specifying the namespace.
>
> Suggested-by: Arnd Bergmann
> Reviewed-by: Martijn Coenen
> Signed-off-by: Matthias Maennich
> ---
> include/linux/export.h | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/linux/export.h b/include/linux/export.h
> index 8e12e05444d1..1fb243abdbc4 100644
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -166,6 +166,12 @@ struct kernel_symbol {
> #define __EXPORT_SYMBOL ___EXPORT_SYMBOL
> #endif
>
> +#ifdef DEFAULT_SYMBOL_NAMESPACE
> +#undef __EXPORT_SYMBOL
> +#define __EXPORT_SYMBOL(sym, sec)\
> + __EXPORT_SYMBOL_NS(sym, sec, DEFAULT_SYMBOL_NAMESPACE)
> +#endif
> +
> #define EXPORT_SYMBOL(sym) __EXPORT_SYMBOL(sym, "")
> #define EXPORT_SYMBOL_GPL(sym) __EXPORT_SYMBOL(sym, "_gpl")
> #define EXPORT_SYMBOL_GPL_FUTURE(sym) __EXPORT_SYMBOL(sym, "_gpl_future")
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>
Reviewed-by: Greg Kroah-Hartman
Re: [PATCH v2 06/10] export: allow definition default namespaces in Makefiles or sources
On Tue, Aug 13, 2019 at 01:17:03PM +0100, Matthias Maennich wrote: > To avoid excessive usage of EXPORT_SYMBOL_NS(sym, MY_NAMESPACE), where > MY_NAMESPACE will always be the namespace we are exporting to, allow > exporting all definitions of EXPORT_SYMBOL() and friends by defining > DEFAULT_SYMBOL_NAMESPACE. > > For example, to export all symbols defined in usb-common into the > namespace USB_COMMON, add a line like this to drivers/usb/common/Makefile: > > ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=USB_COMMON I thought we were trying to get away from cflags :( > > That is equivalent to changing all EXPORT_SYMBOL(sym) definitions to > EXPORT_SYMBOL_NS(sym, USB_COMMON). Subsequently all symbol namespaces > functionality will apply. > > Another way of making use of this feature is to define the namespace > within source or header files similar to how TRACE_SYSTEM defines are > used: > #undef DEFAULT_SYMBOL_NAMESPACE > #define DEFAULT_SYMBOL_NAMESPACE USB_COMMON > > Please note that, as opposed to TRACE_SYSTEM, DEFAULT_SYMBOL_NAMESPACE > has to be defined before including include/linux/export.h. > > If DEFAULT_SYMBOL_NAMESPACE is defined, a symbol can still be exported > to another namespace by using EXPORT_SYMBOL_NS() and friends with > explicitly specifying the namespace. Ok, good, hopefully the cflags stuff will not be the default for people. thanks, greg k-h
Re: [PATCH v2 04/10] modpost: add support for symbol namespaces
On Tue, Aug 13, 2019 at 01:17:01PM +0100, Matthias Maennich wrote: > Add support for symbols that are exported into namespaces. For that, > extract any namespace suffix from the symbol name. In addition, emit a > warning whenever a module refers to an exported symbol without > explicitly importing the namespace that it is defined in. This patch > consistently adds the namespace suffix to symbol names exported into > Module.symvers. > > Example warning emitted by modpost in case of the above violation: > > WARNING: module ums-usbat uses symbol usb_stor_resume from namespace > USB_STORAGE, but does not import it. > > Co-developed-by: Martijn Coenen > Signed-off-by: Martijn Coenen > Reviewed-by: Joel Fernandes (Google) > Signed-off-by: Matthias Maennich > --- > scripts/mod/modpost.c | 91 +-- > scripts/mod/modpost.h | 7 > 2 files changed, 87 insertions(+), 11 deletions(-) Reviewed-by: Greg Kroah-Hartman
Re: [PATCH v2 03/10] module: add support for symbol namespaces.
On Tue, Aug 13, 2019 at 01:17:00PM +0100, Matthias Maennich wrote: > The EXPORT_SYMBOL_NS() and EXPORT_SYMBOL_NS_GPL() macros can be used to > export a symbol to a specific namespace. There are no _GPL_FUTURE and > _UNUSED variants because these are currently unused, and I'm not sure > they are necessary. > > I didn't add EXPORT_SYMBOL_NS() for ASM exports; this patch sets the > namespace of ASM exports to NULL by default. In case of relative > references, it will be relocatable to NULL. If there's a need, this > should be pretty easy to add. > > A module that wants to use a symbol exported to a namespace must add a > MODULE_IMPORT_NS() statement to their module code; otherwise, modpost > will complain when building the module, and the kernel module loader > will emit an error and fail when loading the module. > > MODULE_IMPORT_NS() adds a modinfo tag 'import_ns' to the module. That > tag can be observed by the modinfo command, modpost and kernel/module.c > at the time of loading the module. > > The ELF symbols are renamed to include the namespace with an asm label; > for example, symbol 'usb_stor_suspend' in namespace USB_STORAGE becomes > 'usb_stor_suspend.USB_STORAGE'. This allows modpost to do namespace > checking, without having to go through all the effort of parsing ELF and > relocation records just to get to the struct kernel_symbols. > > On x86_64 I saw no difference in binary size (compression), but at > runtime this will require a word of memory per export to hold the > namespace. An alternative could be to store namespaced symbols in their > own section and use a separate 'struct namespaced_kernel_symbol' for > that section, at the cost of making the module loader more complex. > > Co-developed-by: Martijn Coenen > Signed-off-by: Martijn Coenen > Signed-off-by: Matthias Maennich Reviewed-by: Greg Kroah-Hartman
Re: KASAN: use-after-free Read in device_release_driver_internal
On Tue, Aug 13, 2019 at 03:22:15PM +0200, Oliver Neukum wrote: > Am Dienstag, den 13.08.2019, 14:42 +0200 schrieb Andrey Konovalov: > > > > > > [..] > > > On Thu, Aug 8, 2019 at 4:00 PM Alan Stern wrote: > > > Ah, that looks right, thank you. The patch worked correctly -- good > > > work Oliver! > > > > Great! Just a reminder to submit the fix :) > > I did last week: > https://patchwork.kernel.org/patch/11084261/ Give me a chance to catch up :)
Re: [PATCH v2 09/10] usb-storage: remove single-use define for debugging
On Tue, Aug 13, 2019 at 02:42:59PM +0200, Greg KH wrote: > On Tue, Aug 13, 2019 at 01:17:06PM +0100, Matthias Maennich wrote: > > USB_STORAGE was defined as "usb-storage: " and used in a single location > > as argument to printk. In order to be able to use the name > > 'USB_STORAGE', drop the definition and use the string directly for the > > printk call. > > > > Signed-off-by: Matthias Maennich > > --- > > drivers/usb/storage/debug.h| 2 -- > > drivers/usb/storage/scsiglue.c | 2 +- > > 2 files changed, 1 insertion(+), 3 deletions(-) > > I'll go take this today. The module really should just be using > dev_err() there. It needs to be cleaned up :( Now applied to the usb git tree, thanks. greg k-h
Re: [PATCH v2 10/10] RFC: usb-storage: export symbols in USB_STORAGE namespace
On Tue, Aug 13, 2019 at 01:17:07PM +0100, Matthias Maennich wrote: > Modules using these symbols are required to explicitly import the > namespace. This patch was generated with the following steps and serves > as a reference to use the symbol namespace feature: > > 1) Define DDEFAULT_SYMBOL_NAMESPACE in the corresponding Makefile > 2) make (see warnings during modpost about missing imports) > 3) make nsdeps > > Instead of a DEFAULT_SYMBOL_NAMESPACE definition, the EXPORT_SYMBOL_NS > variants can be used to explicitly specify the namespace. The advantage > of the method used here is that newly added symbols are automatically > exported and existing ones are exported without touching their > respective EXPORT_SYMBOL macro expansion. Ok, I can't read text, this answers my previous question. But, as an example, shouldn't we also have some code here that uses the EXPORT_SYMBOL_NS() macro to ensure that it actually works? thanks, greg k-h
Re: [PATCH v2 10/10] RFC: usb-storage: export symbols in USB_STORAGE namespace
On Tue, Aug 13, 2019 at 01:17:07PM +0100, Matthias Maennich wrote: > Modules using these symbols are required to explicitly import the > namespace. This patch was generated with the following steps and serves > as a reference to use the symbol namespace feature: > > 1) Define DDEFAULT_SYMBOL_NAMESPACE in the corresponding Makefile > 2) make (see warnings during modpost about missing imports) > 3) make nsdeps > > Instead of a DEFAULT_SYMBOL_NAMESPACE definition, the EXPORT_SYMBOL_NS > variants can be used to explicitly specify the namespace. The advantage > of the method used here is that newly added symbols are automatically > exported and existing ones are exported without touching their > respective EXPORT_SYMBOL macro expansion. > > Signed-off-by: Matthias Maennich > --- > drivers/usb/storage/Makefile| 2 ++ > drivers/usb/storage/alauda.c| 1 + > drivers/usb/storage/cypress_atacb.c | 1 + > drivers/usb/storage/datafab.c | 1 + > drivers/usb/storage/ene_ub6250.c| 1 + > drivers/usb/storage/freecom.c | 1 + > drivers/usb/storage/isd200.c| 1 + > drivers/usb/storage/jumpshot.c | 1 + > drivers/usb/storage/karma.c | 1 + > drivers/usb/storage/onetouch.c | 1 + > drivers/usb/storage/realtek_cr.c| 1 + > drivers/usb/storage/sddr09.c| 1 + > drivers/usb/storage/sddr55.c| 1 + > drivers/usb/storage/shuttle_usbat.c | 1 + > drivers/usb/storage/uas.c | 1 + > 15 files changed, 16 insertions(+) > > diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile > index a67ddcbb4e24..46635fa4a340 100644 > --- a/drivers/usb/storage/Makefile > +++ b/drivers/usb/storage/Makefile > @@ -8,6 +8,8 @@ > > ccflags-y := -I $(srctree)/drivers/scsi > > +ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=USB_STORAGE Wait, we have to do this for every subsystem? I thought there was a macro we could use in the code itself for this. What changed from earlier versions, or was this always here? thanks, greg k-h
Re: [PATCH v2 08/10] scripts: Coccinelle script for namespace dependencies.
On Tue, Aug 13, 2019 at 01:17:05PM +0100, Matthias Maennich wrote: > A script that uses the '.ns_deps' file generated by modpost to > automatically add the required symbol namespace dependencies to each > module. > > Usage: > 1) Move some symbols to a namespace with EXPORT_SYMBOL_NS() or define >DEFAULT_SYMBOL_NAMESPACE > 2) Run 'make' (or 'make modules') and get warnings about modules not >importing that namespace. > 3) Run 'make nsdeps' to automatically add required import statements >to said modules. > > This makes it easer for subsystem maintainers to introduce and maintain > symbol namespaces into their codebase. > > Co-developed-by: Martijn Coenen > Signed-off-by: Martijn Coenen > Signed-off-by: Matthias Maennich > --- I really can't express just how cool this patch is. I was amazed when I first saw it in action a long time ago, and still am. Reviewed-by: Greg Kroah-Hartman
Re: [PATCH v2 09/10] usb-storage: remove single-use define for debugging
On Tue, Aug 13, 2019 at 01:17:06PM +0100, Matthias Maennich wrote: > USB_STORAGE was defined as "usb-storage: " and used in a single location > as argument to printk. In order to be able to use the name > 'USB_STORAGE', drop the definition and use the string directly for the > printk call. > > Signed-off-by: Matthias Maennich > --- > drivers/usb/storage/debug.h| 2 -- > drivers/usb/storage/scsiglue.c | 2 +- > 2 files changed, 1 insertion(+), 3 deletions(-) I'll go take this today. The module really should just be using dev_err() there. It needs to be cleaned up :( thanks, greg k-h
Re: [PATCH v2 02/10] export: explicitly align struct kernel_symbol
On Tue, Aug 13, 2019 at 01:16:59PM +0100, Matthias Maennich wrote: > This change allows growing struct kernel_symbol without wasting bytes to > alignment. It also concretized the alignment of ksymtab entries if > relative references are used for ksymtab entries. > > struct kernel_symbol was already implicitly being aligned to the word > size, except on x86_64 and m68k, where it is aligned to 16 and 2 bytes, > respectively. > > As far as I can tell there is no requirement for aligning struct > kernel_symbol to 16 bytes on x86_64, but gcc aligns structs to their > size, and the linker aligns the custom __ksymtab sections to the largest > data type contained within, so setting KSYM_ALIGN to 16 was necessary to > stay consistent with the code generated for non-ASM EXPORT_SYMBOL(). Now > that non-ASM EXPORT_SYMBOL() explicitly aligns to word size (8), > KSYM_ALIGN is no longer necessary. > > In case of relative references, the alignment has been changed > accordingly to not waste space when adding new struct members. > > As for m68k, struct kernel_symbol is aligned to 2 bytes even though the > structure itself is 8 bytes; using a 4-byte alignment shouldn't hurt. > > I manually verified the output of the __ksymtab sections didn't change > on x86, x86_64, arm, arm64 and m68k. As expected, the section contents > didn't change, and the ELF section alignment only changed on x86_64 and > m68k. Feedback from other archs more than welcome. > > Co-developed-by: Martijn Coenen > Signed-off-by: Martijn Coenen > Signed-off-by: Matthias Maennich Ick, messy, nice fix. Reviewed-by: Greg Kroah-Hartman
Re: [PATCH v2 01/10] module: support reading multiple values per modinfo tag
On Tue, Aug 13, 2019 at 01:16:58PM +0100, Matthias Maennich wrote: > Similar to modpost's get_next_modinfo(), introduce get_next_modinfo() in > kernel/module.c to acquire any further values associated with the same > modinfo tag name. That is useful for any tags that have multiple > occurrences (such as 'alias'), but is in particular introduced here as > part of the symbol namespaces patch series to read the (potentially) > multiple namespaces a module is importing. > > Reviewed-by: Joel Fernandes (Google) > Reviewed-by: Martijn Coenen > Signed-off-by: Matthias Maennich Reviewed-by: Greg Kroah-Hartman
Re: [PATCH] USB: core: Fix races in character device registration and deregistraion
On Mon, Aug 12, 2019 at 04:11:07PM -0400, Alan Stern wrote: > The syzbot fuzzer has found two (!) races in the USB character device > registration and deregistration routines. This patch fixes the races. > > The first race results from the fact that usb_deregister_dev() sets > usb_minors[intf->minor] to NULL before calling device_destroy() on the > class device. This leaves a window during which another thread can > allocate the same minor number but will encounter a duplicate name > error when it tries to register its own class device. A typical error > message in the system log would look like: > > sysfs: cannot create duplicate filename '/class/usbmisc/ldusb0' > > The patch fixes this race by destroying the class device first. > > The second race is in usb_register_dev(). When that routine runs, it > first allocates a minor number, then drops minor_rwsem, and then > creates the class device. If the device creation fails, the minor > number is deallocated and the whole routine returns an error. But > during the time while minor_rwsem was dropped, there is a window in > which the minor number is allocated and so another thread can > successfully open the device file. Typically this results in > use-after-free errors or invalid accesses when the other thread closes > its open file reference, because the kernel then tries to release > resources that were already deallocated when usb_register_dev() > failed. The patch fixes this race by keeping minor_rwsem locked > throughout the entire routine. > > Reported-and-tested-by: syzbot+30cf45ebfe0b0c484...@syzkaller.appspotmail.com > Signed-off-by: Alan Stern > CC: > > --- > > [as1907] Thanks for this, now queued up. greg k-h
Re: KASAN: use-after-free Read in ld_usb_release
On Fri, Aug 09, 2019 at 12:51:00PM -0400, Alan Stern wrote: > Greg: > > See below... > > On Fri, 9 Aug 2019, syzbot wrote: > > > Hello, > > > > syzbot found the following crash on: > > > > HEAD commit:e96407b4 usb-fuzzer: main usb gadget fuzzer driver > > git tree: https://github.com/google/kasan.git usb-fuzzer > > console output: https://syzkaller.appspot.com/x/log.txt?x=15a16f2660 > > kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e > > dashboard link: https://syzkaller.appspot.com/bug?extid=30cf45ebfe0b0c4847a1 > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1416df2660 > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11ce511c60 > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > Reported-by: syzbot+30cf45ebfe0b0c484...@syzkaller.appspotmail.com > > > > == > > BUG: KASAN: use-after-free in __mutex_lock_common > > kernel/locking/mutex.c:912 [inline] > > BUG: KASAN: use-after-free in __mutex_lock+0xf23/0x1360 > > kernel/locking/mutex.c:1077 > > Read of size 8 at addr 8881d21fc2d8 by task syz-executor834/1878 > > > > CPU: 0 PID: 1878 Comm: syz-executor834 Not tainted 5.3.0-rc2+ #25 > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > > Google 01/01/2011 > > Call Trace: > > __dump_stack lib/dump_stack.c:77 [inline] > > dump_stack+0xca/0x13e lib/dump_stack.c:113 > > print_address_description+0x6a/0x32c mm/kasan/report.c:351 > > __kasan_report.cold+0x1a/0x33 mm/kasan/report.c:482 > > kasan_report+0xe/0x12 mm/kasan/common.c:612 > > __mutex_lock_common kernel/locking/mutex.c:912 [inline] > > __mutex_lock+0xf23/0x1360 kernel/locking/mutex.c:1077 > > ld_usb_release+0xb1/0x400 drivers/usb/misc/ldusb.c:386 > > __fput+0x2d7/0x840 fs/file_table.c:280 > > task_work_run+0x13f/0x1c0 kernel/task_work.c:113 > > tracehook_notify_resume include/linux/tracehook.h:188 [inline] > > exit_to_usermode_loop+0x1d2/0x200 arch/x86/entry/common.c:163 > > prepare_exit_to_usermode arch/x86/entry/common.c:194 [inline] > > syscall_return_slowpath arch/x86/entry/common.c:274 [inline] > > do_syscall_64+0x45f/0x580 arch/x86/entry/common.c:299 > > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > RIP: 0033:0x406b31 > > Code: 75 14 b8 03 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 04 19 00 00 c3 48 > > 83 ec 08 e8 6a fc ff ff 48 89 04 24 b8 03 00 00 00 0f 05 <48> 8b 3c 24 48 > > 89 c2 e8 b3 fc ff ff 48 89 d0 48 83 c4 08 48 3d 01 > > RSP: 002b:7ffcf13bd080 EFLAGS: 0293 ORIG_RAX: 0003 > > RAX: RBX: 0005 RCX: 00406b31 > > RDX: fff7 RSI: 0080 RDI: 0004 > > RBP: 0159 R08: 0020 R09: 0020 > > R10: 7ffcf13bd0b0 R11: 0293 R12: 0001d884 > > R13: 0004 R14: 006e39ec R15: 0064 > > > > Allocated by task 1775: > > save_stack+0x1b/0x80 mm/kasan/common.c:69 > > set_track mm/kasan/common.c:77 [inline] > > __kasan_kmalloc mm/kasan/common.c:487 [inline] > > __kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:460 > > kmalloc include/linux/slab.h:552 [inline] > > kzalloc include/linux/slab.h:748 [inline] > > ld_usb_probe+0x6e/0xa65 drivers/usb/misc/ldusb.c:661 > > usb_probe_interface+0x305/0x7a0 drivers/usb/core/driver.c:361 > > really_probe+0x281/0x650 drivers/base/dd.c:548 > > driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709 > > __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816 > > bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454 > > __device_attach+0x217/0x360 drivers/base/dd.c:882 > > bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514 > > device_add+0xae6/0x16f0 drivers/base/core.c:2114 > > usb_set_configuration+0xdf6/0x1670 drivers/usb/core/message.c:2023 > > generic_probe+0x9d/0xd5 drivers/usb/core/generic.c:210 > > usb_probe_device+0x99/0x100 drivers/usb/core/driver.c:266 > > really_probe+0x281/0x650 drivers/base/dd.c:548 > > driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709 > > __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816 > > bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454 > > __device_attach+0x217/0x360 drivers/base/dd.c:882 > > bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514 > > device_add+0xae6/0x16f0 drivers/base/core.c:2114 > > usb_new_device.cold+0x6a4/0xe79 drivers/usb/core/hub.c:2536 > > hub_port_connect drivers/usb/core/hub.c:5098 [inline] > > hub_port_connect_change drivers/usb/core/hub.c:5213 [inline] > > port_event drivers/usb/core/hub.c:5359 [inline] > > hub_event+0x1b5c/0x3640 drivers/usb/core/hub.c:5441 > > process_one_work+0x92b/0x1530 kernel/workqueue.c:2269 > > worker_thread+0x96/0xe20 kernel/workqueue.c:2415 > > kthread+0x318/0x420 kernel/kthread.c:2
Re: KASAN: use-after-free Read in ld_usb_release
On Mon, Aug 12, 2019 at 10:21:14AM -0400, Alan Stern wrote: > On Mon, 12 Aug 2019, Andrey Konovalov wrote: > > > Alan, could you submit this patch (if you haven't already)? Looks like > > it fixes this bug (and might fix some others). > > I will. I was waiting to see if Greg KH had any comments. Give me a few hours, it's in my queue to review...
Re: [PATCH] xhci: wait CNR when doing xhci resume
On Mon, Aug 12, 2019 at 03:24:52PM +0800, Rick Tseng wrote:
> From: Rick
>
> NVIDIA 3.1 xHCI card would lose power when moving power state into D3Cold.
> Thus we need to wait CNR bit to clear when xhci resmue as xhci init.
>
> Signed-off-by: Rick
We need a "full" name on the from and signed-off-by lines, please.
> ---
> drivers/usb/host/xhci-pci.c | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 1e0236e..857ad8a 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
>
> #include "xhci.h"
> #include "xhci-trace.h"
> @@ -455,6 +456,19 @@ static void xhci_pme_quirk(struct usb_hcd *hcd)
> readl(reg);
> }
>
> +static int xhci_poll_cnr(struct usb_hcd *hcd)
> +{
> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> + void __iomem *reg = &xhci->op_regs->status;
> + u32 result;
> + int ret;
> +
> + ret = readl_poll_timeout_atomic(reg, result,
> + (result & STS_CNR) == 0,
> + 1, 100 * 1000);
> + return ret;
> +}
> +
> static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
> {
> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> @@ -508,6 +522,12 @@ static int xhci_pci_resume(struct usb_hcd *hcd, bool
> hibernated)
> if (pdev->vendor == PCI_VENDOR_ID_INTEL)
> usb_enable_intel_xhci_ports(pdev);
>
> + if (pdev->vendor == PCI_VENDOR_ID_NVIDIA) {
So all devices of this vendor need that? Are you _sure_? Why not just
blacklist a single device?
thanks,
greg k-h
[GIT PULL] USB fixes for 5.3-rc4
The following changes since commit 609488bc979f99f805f34e9a32c1e3b71179d10b: Linux 5.3-rc2 (2019-07-28 12:47:02 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git tags/usb-5.3-rc4 for you to fetch changes up to 27709ae4e2fe6cf7da2ae45e718e190c5433342b: usb: setup authorized_default attributes using usb_bus_notify (2019-08-08 16:07:34 +0200) USB fixes for 5.3-rc4 Here are some small USB fixes for 5.3-rc4. The "biggest" one here is moving code from one file to another in order to fix a long-standing race condition with the creation of sysfs files for USB devices. Turns out that there are now userspace tools out there that are hitting this long-known bug, so it's time to fix them. Thankfully the tool-maker in this case fixed the issue :) The other patches in here are all fixes for reported issues. Now that syzbot knows how to fuzz USB drivers better, and is starting to now fuzz the userspace facing side of them at the same time, there will be more and more small fixes like these coming, which is a good thing. All of these have been in linux-next with no reported issues. Signed-off-by: Greg Kroah-Hartman Gavin Li (1): usb: usbfs: fix double-free of usb memory upon submiturb error Guenter Roeck (2): usb: typec: tcpm: Add NULL check before dereferencing config usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests Heikki Krogerus (1): usb: typec: ucsi: ccg: Fix uninitilized symbol error Li Jun (2): usb: typec: tcpm: free log buf memory when remove debug file usb: typec: tcpm: remove tcpm dir if no children Mathias Nyman (1): xhci: Fix NULL pointer dereference at endpoint zero reset. Oliver Neukum (2): Revert "USB: rio500: simplify locking" usb: iowarrior: fix deadlock on disconnect Suzuki K Poulose (1): usb: yurex: Fix use-after-free in yurex_delete Thiébaud Weksteen (1): usb: setup authorized_default attributes using usb_bus_notify Yoshihiro Shimoda (1): usb: host: xhci-rcar: Fix timeout in xhci_suspend() drivers/usb/core/devio.c | 2 - drivers/usb/core/hcd.c| 123 -- drivers/usb/core/sysfs.c | 121 + drivers/usb/core/usb.h| 5 ++ drivers/usb/host/xhci-rcar.c | 9 ++- drivers/usb/host/xhci.c | 10 drivers/usb/misc/iowarrior.c | 7 ++- drivers/usb/misc/rio500.c | 43 - drivers/usb/misc/yurex.c | 2 +- drivers/usb/typec/tcpm/tcpm.c | 58 -- drivers/usb/typec/ucsi/ucsi_ccg.c | 2 +- 11 files changed, 217 insertions(+), 165 deletions(-)
Re: AW: AW: KASAN: use-after-free Read in usbhid_power
On Fri, Aug 09, 2019 at 01:00:25PM +, Schmid, Carsten wrote: > >> > >> @Greg: > >> I am still confident that my patch in __release_region should be taken in. > > > > Ok, submit it in a "real" way and we can consider it :) > > > > thanks, > > > > greg k-h > > Already done, linux-ker...@vger.kernel.org, see > https://www.spinics.net/lists/kernel/msg3218180.html You didn't cc: any developer, that's a sure way to get a patch ignored :( Try resending it with at least the people who get_maintainer.pl says has touched that file last in it. Also, Linus is the unofficial resource.c maintainer. I think he has a set of userspace testing scripts for changes somewhere, so you should cc: him too. And might as well add me :) thanks, greg k-h
Re: AW: AW: KASAN: use-after-free Read in usbhid_power
On Fri, Aug 09, 2019 at 12:38:35PM +, Schmid, Carsten wrote: > Hi again, > > >> > >> Hey, i did not want to trigger an eartquake in the basement of the kernel > >> ;-) > >> My intention was to prevent some crashes, and help developers to find > >> their bugs. > >> I think my patch exactly does this. > > > > Hehe, actually drivers not being able to block unbind has been bugging me > > for > > a while now, because there are cases where this would be really helpful. > >>> 1) make resources refcounted, have child resources take a ref on the > >>> parent > >>> 2) Disallow unbind on devices with bound child-devices? > >>> > >> Exactly what i was thinking of in first attempts. > >> But i fear that would break even more use cases. > >> > >> Hans, directly regarding the driver: > >> The problem i see is that the xhci_intel_unregister_pdev which is added > >> as an action with devm_add_action_or_reset() is called late by the > >> framework, > >> later than the usb_hcd_pci_remove() in xhci_pci_remove. > >> Is there any chance to trigger this before? > >> This is what Greg meant with "right order". > > > > Ah, I missed that part, sure that should be easy, just stop using > > devm_add_action_or_reset() and do the xhci_intel_unregister_pdev() > > manually at the right time. The downside of this is that you also > > need to make sure it happens at the right time from probe error-paths > > but given the bug you are hitting, I guess that is probably > > already a problem. > > > @Hans: > Sure, will have a look at this. I think i have found where to do that, > but need to check how to get the pdev pointer there > > @Greg: > I am still confident that my patch in __release_region should be taken in. Ok, submit it in a "real" way and we can consider it :) thanks, greg k-h
Re: BUG: bad usercopy in ld_usb_read
On Thu, Aug 08, 2019 at 04:06:32PM -0700, Kees Cook wrote:
> On Thu, Aug 08, 2019 at 02:46:54PM +0200, Greg KH wrote:
> > On Thu, Aug 08, 2019 at 05:38:06AM -0700, syzbot wrote:
> > > Hello,
> > >
> > > syzbot found the following crash on:
> > >
> > > HEAD commit:e96407b4 usb-fuzzer: main usb gadget fuzzer driver
> > > git tree: https://github.com/google/kasan.git usb-fuzzer
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=13aeaece60
> > > kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e
> > > dashboard link:
> > > https://syzkaller.appspot.com/bug?extid=45b2f40f0778cfa7634e
> > > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > >
> > > Unfortunately, I don't have any reproducer for this crash yet.
> > >
> > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > Reported-by: syzbot+45b2f40f0778cfa76...@syzkaller.appspotmail.com
> > >
> > > ldusb 6-1:0.124: Read buffer overflow, -131383996186150 bytes dropped
> >
> > That's a funny number :)
> >
> > Nice overflow found, I see you are now starting to fuzz the char device
> > nodes of usb drivers...
> >
> > Michael, care to fix this up?
>
> This looks like the length in the read-from-device buffer is unchecked:
>
> /* actual_buffer contains actual_length + interrupt_in_buffer */
> actual_buffer = (size_t *)(dev->ring_buffer + dev->ring_tail *
> (sizeof(size_t)+dev->interrupt_in_endpoint_size));
> bytes_to_read = min(count, *actual_buffer);
> if (bytes_to_read < *actual_buffer)
> dev_warn(&dev->intf->dev, "Read buffer overflow, %zd bytes
> dropped\n",
> *actual_buffer-bytes_to_read);
>
> /* copy one interrupt_in_buffer from ring_buffer into userspace */
> if (copy_to_user(buffer, actual_buffer+1, bytes_to_read)) {
> retval = -EFAULT;
> goto unlock_exit;
> }
>
> I assume what's stored at actual_buffer is bogus and needs validation
> somewhere before it's actually used. (If not here, maybe where ever the
> write into the buffer originally happens?)
I think it should be verified here, as that's when it is parsed. The
data is written to the buffer in ld_usb_interrupt_in_callback() but it
does not "know" how to parse it at that location.
thanks,
greg k-h
Re: KASAN: use-after-free Read in usbhid_power
On Fri, Aug 09, 2019 at 07:35:32AM +, Schmid, Carsten wrote:
> Hi all having use-after-free issues in USB shutdowns:
> I hunted for a similar case in the intel_xhci_usb_sw driver.
> What i have found and proposed is (from yesterday):
> ---
> [PATCH] kernel/resource.c: invalidate parent when freed resource has childs
>
> When a resource is freed and has children, the childrens are
> left without any hint that their parent is no more valid.
> This caused at least one use-after-free in the xhci-hcd using
> ext-caps driver when platform code released platform devices.
>
> Fix this by setting child's parent to zero and warn.
>
> Signed-off-by: Carsten Schmid
> ---
> Rationale:
> When hunting for the root cause of a crash on a 4.14.86 kernel, i
> have found the root cause and checked it being still present
> upstream. Our case:
> Having xhci-hcd and intel_xhci_usb_sw active we can see in
> /proc/meminfo: (exceirpt)
> b3c0-b3c0 : :00:15.0
> b3c0-b3c0 : xhci-hcd
> b3c08070-b3c0846f : intel_xhci_usb_sw
> intel_xhci_usb_sw being a child of xhci-hcd.
>
> Doing an unbind command
> echo :00:15.0 > /sys/bus/pci/drivers/xhci_hcd/unbind
> leads to xhci-hcd being freed in __release_region.
> The intel_xhci_usb_sw resource is accessed in platform code
> in platform_device_del with
> for (i = 0; i < pdev->num_resources; i++) {
> struct resource *r = &pdev->resource[i];
> if (r->parent)
> release_resource(r);
> }
> as the resource's parent has not been updated, the release_resource
> uses the parent:
> p = &old->parent->child;
> which is now invalid.
> Fix this by marking the parent invalid in the child and give a warning
> in dmesg.
> ---
> kernel/resource.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 158f04ec1d4f..95340cb0b1c2 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1200,6 +1200,15 @@ void __release_region(struct resource *parent,
> resource_size_t start,
> write_unlock(&resource_lock);
> if (res->flags & IORESOURCE_MUXED)
> wake_up(&muxed_resource_wait);
> +
> + write_lock(&resource_lock);
Nit, should't this be written so that you only drop/get the lock if the
above if statement is true?
> + if (res->child) {
> + printk(KERN_WARNING "__release_region: %s has
> child %s,"
> + "invalidating childs
> parent\n",
> + res->name, res->child->name);
What can userspace/anyone do about this if it triggers?
Can't we fix the root problem in the xhci driver to properly order how
it tears things down? If it has intel_xhci_usb_driver as a "child"
shouldn't that be unbound/freed before the parent is?
thanks,
greg k-h
Re: [PATCH] USB: rio500: Fix lockdep violation
On Thu, Aug 08, 2019 at 01:34:08PM -0400, Alan Stern wrote:
> The syzbot fuzzer found a lockdep violation in the rio500 driver:
>
> ==
> WARNING: possible circular locking dependency detected
> 5.3.0-rc2+ #23 Not tainted
> --
> syz-executor.2/20386 is trying to acquire lock:
> 772249c6 (rio500_mutex){+.+.}, at: open_rio+0x16/0xc0
> drivers/usb/misc/rio500.c:64
>
> but task is already holding lock:
> d3e8f4b9 (minor_rwsem){}, at: usb_open+0x23/0x270
> drivers/usb/core/file.c:39
>
> which lock already depends on the new lock.
>
> The problem is that the driver's open_rio() routine is called while
> the usbcore's minor_rwsem is locked for reading, and it acquires the
> rio500_mutex; whereas conversely, probe_rio() and disconnect_rio()
> first acquire the rio500_mutex and then call usb_register_dev() or
> usb_deregister_dev(), which lock minor_rwsem for writing.
>
> The correct ordering of acquisition should be: minor_rwsem first, then
> rio500_mutex (since the locking in open_rio() cannot be changed).
> Thus, the probe and disconnect routines should avoid holding
> rio500_mutex while doing their registration and deregistration.
>
> This patch adjusts the code in those two routines to do just that. It
> also relies on the fact that the probe and disconnect routines are
> protected by the device mutex, so the initial test of rio->present
> needs no extra locking.
>
> Reported-by: syzbot+7bbcbe9c9ff0cd495...@syzkaller.appspotmail.com
> Signed-off-by: Alan Stern
> Fixes: d710734b0677 ("USB: rio500: simplify locking")
> CC: Oliver Neukum
> CC:
>
> ---
>
> This patch is different from the one I posted earlier. I realized that
> we don't want to register the device's char file until after the
> buffers have been allocated.
Should I revert Oliver's patch?
confused,
greg k-h
Re: [PATCH v2] usbfs: Add ioctls for runtime power management
On Wed, Aug 07, 2019 at 10:29:50AM -0400, Alan Stern wrote: > It has been requested that usbfs should implement runtime power > management, instead of forcing the device to remain at full power as > long as the device file is open. This patch introduces that new > feature. > > It does so by adding three new usbfs ioctls: > > USBDEVFS_FORBID_SUSPEND: Prevents the device from going into > runtime suspend (and causes a resume if the device is already > suspended). > > USBDEVFS_ALLOW_SUSPEND: Allows the device to go into runtime > suspend. Some time may elapse before the device actually is > suspended, depending on things like the autosuspend delay. > > USBDEVFS_WAIT_FOR_RESUME: Blocks until the call is interrupted > by a signal or at least one runtime resume has occurred since > the most recent ALLOW_SUSPEND ioctl call (which may mean > immediately, even if the device is currently suspended). In > the latter case, the device is prevented from suspending again > just as if FORBID_SUSPEND was called before the ioctl returns. > > For backward compatibility, when the device file is first opened > runtime suspends are forbidden. The userspace program can then allow > suspends whenever it wants, and either resume the device directly (by > forbidding suspends again) or wait for a resume from some other source > (such as a remote wakeup). URBs submitted to a suspended device will > fail or will complete with an appropriate error code. > > This combination of ioctls is sufficient for user programs to have > nearly the same degree of control over a device's runtime power > behavior as kernel drivers do. > > Still lacking is documentation for the new ioctls. I intend to add it > later, after the existing documentation for the usbfs userspace API is > straightened out into a reasonable form. > > Suggested-by: Mayuresh Kulkarni > Signed-off-by: Alan Stern > > --- > > v2: Return -EINTR instead of -ERESTARTSYS when proc_wait_for_resume() > is interrupted by a signal. Looks great, thanks for doing this. Now queued up. greg k-h
Re: usb zero copy dma handling
On Thu, Aug 08, 2019 at 12:07:26PM +0200, Greg KH wrote:
> On Thu, Aug 08, 2019 at 10:46:24AM +0100, Robin Murphy wrote:
> > On 2019-08-08 9:58 am, Greg KH wrote:
> > > On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f...@hashmail.org
> > > wrote:
> > > > Hello linux-usb and linux-arm.
> > > >
> > > > Ccing security@ because "the kernel dma code is mapping randomish
> > > > kernel/user mem to a user process" seems to have security implications
> > > > even though i didnt research that aspect past "its a 100% reliable way
> > > > to crash a raspi from userspace".
> > > >
> > > > tried submitting this through linux-arm-kernel ~2 weeks ago but
> > > > the only "response" i got was phishing-spam.
> > > > tried to follow up through raspi-internals chat, they suggested
> > > > i try linux-usb instead, but otoh the original reporter was
> > > > deflected from -usb to "try some other mls, they might care".
> > > > https://www.spinics.net/lists/linux-usb/msg173277.html
> > > >
> > > > if i am not following some arcane ritual or indenting convention
> > > > required
> > > > by regular users of these lists i apologize in advance, but i am not a
> > > > kernel developer, i am just here as a user with a bug and a patch.
> > > > (and the vger FAQ link 404s...)
> > >
> > > The "arcane ritual" should be really well documented by now, it's in
> > > Documentation/SubmittingPatches in your kernel tree, and you can read it
> > > online at:
> > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> > >
> > >
> > > > i rediffed against HEAD even though the two weeks old patch still
> > > > applied
> > > > cleanly with +2 offset.
> > > >
> > > > # stepping off soap box # actual technical content starts here #
> > > >
> > > > this is a followup to that thread from 2018-11:
> > > > https://www.spinics.net/lists/arm-kernel/msg685598.html
> > > >
> > > > the issue was discussed in more detail than i can claim
> > > > to fully understand back then, but no fix ever merged.
> > > > but i would really like to use rtl_433 on a raspi without
> > > > having to build a custom-patched kernel first.
> > > >
> > > > the attached patch is my stripdown/cleanup of a devel-diff
> > > > provided to me by the original reporter Steve Markgraf.
> > > > credits to him for the good parts, blame to me for the bad parts.
> > > >
> > > > this does not cover the additional case of "PIO-based usb controllers"
> > > > mainly because i dont understand what that means (or how to handle it)
> > > > and if its broken right now (as the thread indicates) it might
> > > > as well stay broken until someone who understands cares enough.
> > > >
> > > > could you please get this on track for merging?
> > >
> > >
> > > >
> > > > regards,
> > > >x23
> > > >
> > > >
> > > >
> > >
> > > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > > > index b265ab5405f9..69594c2169ea 100644
> > > > --- a/drivers/usb/core/devio.c
> > > > +++ b/drivers/usb/core/devio.c
> > > > @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct
> > > > vm_area_struct *vma)
> > > > usbm->vma_use_count = 1;
> > > > INIT_LIST_HEAD(&usbm->memlist);
> > > > +#ifdef CONFIG_X86
> > > > if (remap_pfn_range(vma, vma->vm_start,
> > > > virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> > > > size, vma->vm_page_prot) < 0) {
> > > > +#else /* !CONFIG_X86 */
> > > > + if (dma_mmap_coherent(ps->dev->bus->sysdev,
> > > > + vma, mem, dma_handle, size) < 0) {
> > > > +#endif /* !CONFIG_X86 */
> > > > dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
> > > > return -EAGAIN;
> > > > }
> > >
> > > First off, we need this in a format we could apply it in (hint, read the
> > > above links).
> > >
> > > But the main issue h
Re: BUG: bad usercopy in ld_usb_read
On Thu, Aug 08, 2019 at 05:38:06AM -0700, syzbot wrote: > Hello, > > syzbot found the following crash on: > > HEAD commit:e96407b4 usb-fuzzer: main usb gadget fuzzer driver > git tree: https://github.com/google/kasan.git usb-fuzzer > console output: https://syzkaller.appspot.com/x/log.txt?x=13aeaece60 > kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e > dashboard link: https://syzkaller.appspot.com/bug?extid=45b2f40f0778cfa7634e > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > Unfortunately, I don't have any reproducer for this crash yet. > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+45b2f40f0778cfa76...@syzkaller.appspotmail.com > > ldusb 6-1:0.124: Read buffer overflow, -131383996186150 bytes dropped That's a funny number :) Nice overflow found, I see you are now starting to fuzz the char device nodes of usb drivers... Michael, care to fix this up? thanks, greg k-h
Re: usb zero copy dma handling
On Thu, Aug 08, 2019 at 10:46:24AM +0100, Robin Murphy wrote:
> On 2019-08-08 9:58 am, Greg KH wrote:
> > On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f...@hashmail.org
> > wrote:
> > > Hello linux-usb and linux-arm.
> > >
> > > Ccing security@ because "the kernel dma code is mapping randomish
> > > kernel/user mem to a user process" seems to have security implications
> > > even though i didnt research that aspect past "its a 100% reliable way
> > > to crash a raspi from userspace".
> > >
> > > tried submitting this through linux-arm-kernel ~2 weeks ago but
> > > the only "response" i got was phishing-spam.
> > > tried to follow up through raspi-internals chat, they suggested
> > > i try linux-usb instead, but otoh the original reporter was
> > > deflected from -usb to "try some other mls, they might care".
> > > https://www.spinics.net/lists/linux-usb/msg173277.html
> > >
> > > if i am not following some arcane ritual or indenting convention required
> > > by regular users of these lists i apologize in advance, but i am not a
> > > kernel developer, i am just here as a user with a bug and a patch.
> > > (and the vger FAQ link 404s...)
> >
> > The "arcane ritual" should be really well documented by now, it's in
> > Documentation/SubmittingPatches in your kernel tree, and you can read it
> > online at:
> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> >
> >
> > > i rediffed against HEAD even though the two weeks old patch still applied
> > > cleanly with +2 offset.
> > >
> > > # stepping off soap box # actual technical content starts here #
> > >
> > > this is a followup to that thread from 2018-11:
> > > https://www.spinics.net/lists/arm-kernel/msg685598.html
> > >
> > > the issue was discussed in more detail than i can claim
> > > to fully understand back then, but no fix ever merged.
> > > but i would really like to use rtl_433 on a raspi without
> > > having to build a custom-patched kernel first.
> > >
> > > the attached patch is my stripdown/cleanup of a devel-diff
> > > provided to me by the original reporter Steve Markgraf.
> > > credits to him for the good parts, blame to me for the bad parts.
> > >
> > > this does not cover the additional case of "PIO-based usb controllers"
> > > mainly because i dont understand what that means (or how to handle it)
> > > and if its broken right now (as the thread indicates) it might
> > > as well stay broken until someone who understands cares enough.
> > >
> > > could you please get this on track for merging?
> >
> >
> > >
> > > regards,
> > >x23
> > >
> > >
> > >
> >
> > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > > index b265ab5405f9..69594c2169ea 100644
> > > --- a/drivers/usb/core/devio.c
> > > +++ b/drivers/usb/core/devio.c
> > > @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct
> > > vm_area_struct *vma)
> > > usbm->vma_use_count = 1;
> > > INIT_LIST_HEAD(&usbm->memlist);
> > > +#ifdef CONFIG_X86
> > > if (remap_pfn_range(vma, vma->vm_start,
> > > virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> > > size, vma->vm_page_prot) < 0) {
> > > +#else /* !CONFIG_X86 */
> > > + if (dma_mmap_coherent(ps->dev->bus->sysdev,
> > > + vma, mem, dma_handle, size) < 0) {
> > > +#endif /* !CONFIG_X86 */
> > > dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
> > > return -EAGAIN;
> > > }
> >
> > First off, we need this in a format we could apply it in (hint, read the
> > above links).
> >
> > But the main issue here is what exactly is this "fixing"? What is wrong
> > with the existing code that non-x86 systems have such a problem with?
> > Shouldn't all of these dma issues be handled by the platform with the
> > remap_pfn_range() call itself?
>
> If usbm->mem is (or ever can be) a CPU address returned by
> dma_alloc_coherent(), then doing virt_to_phys() on it is bogus and may yield
> a nonsense 'PFN' to begin with. However, it it can can ever come from a
> regula
Re: usb zero copy dma handling
On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f...@hashmail.org wrote:
> Hello linux-usb and linux-arm.
>
> Ccing security@ because "the kernel dma code is mapping randomish
> kernel/user mem to a user process" seems to have security implications
> even though i didnt research that aspect past "its a 100% reliable way
> to crash a raspi from userspace".
>
> tried submitting this through linux-arm-kernel ~2 weeks ago but
> the only "response" i got was phishing-spam.
> tried to follow up through raspi-internals chat, they suggested
> i try linux-usb instead, but otoh the original reporter was
> deflected from -usb to "try some other mls, they might care".
> https://www.spinics.net/lists/linux-usb/msg173277.html
>
> if i am not following some arcane ritual or indenting convention required
> by regular users of these lists i apologize in advance, but i am not a
> kernel developer, i am just here as a user with a bug and a patch.
> (and the vger FAQ link 404s...)
The "arcane ritual" should be really well documented by now, it's in
Documentation/SubmittingPatches in your kernel tree, and you can read it
online at:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> i rediffed against HEAD even though the two weeks old patch still applied
> cleanly with +2 offset.
>
> # stepping off soap box # actual technical content starts here #
>
> this is a followup to that thread from 2018-11:
> https://www.spinics.net/lists/arm-kernel/msg685598.html
>
> the issue was discussed in more detail than i can claim
> to fully understand back then, but no fix ever merged.
> but i would really like to use rtl_433 on a raspi without
> having to build a custom-patched kernel first.
>
> the attached patch is my stripdown/cleanup of a devel-diff
> provided to me by the original reporter Steve Markgraf.
> credits to him for the good parts, blame to me for the bad parts.
>
> this does not cover the additional case of "PIO-based usb controllers"
> mainly because i dont understand what that means (or how to handle it)
> and if its broken right now (as the thread indicates) it might
> as well stay broken until someone who understands cares enough.
>
> could you please get this on track for merging?
>
> regards,
> x23
>
>
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index b265ab5405f9..69594c2169ea 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct
> vm_area_struct *vma)
> usbm->vma_use_count = 1;
> INIT_LIST_HEAD(&usbm->memlist);
>
> +#ifdef CONFIG_X86
> if (remap_pfn_range(vma, vma->vm_start,
> virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> size, vma->vm_page_prot) < 0) {
> +#else /* !CONFIG_X86 */
> + if (dma_mmap_coherent(ps->dev->bus->sysdev,
> + vma, mem, dma_handle, size) < 0) {
> +#endif /* !CONFIG_X86 */
> dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
> return -EAGAIN;
> }
First off, we need this in a format we could apply it in (hint, read the
above links).
But the main issue here is what exactly is this "fixing"? What is wrong
with the existing code that non-x86 systems have such a problem with?
Shouldn't all of these dma issues be handled by the platform with the
remap_pfn_range() call itself?
What is the problem that you are having?
thanks,
greg k-h
Re: [PATCH] usb: devio: fix mmap() on non-coherent DMA architectures
On Fri, Aug 02, 2019 at 10:57:00AM -0700, Gavin Li wrote: > usbfs mmap() looks like it was introduced for 4.6 in commit > f7d34b445abc, so it should probably be backported to 4.9 and onwards. > This issue has been present since the introduction of the feature. > > One sidenote: this patch will cause the following warning on x86 due > to dmap_mmap_coherent() trying to map normal memory in as uncached: > > x86/PAT: ... map pfn RAM range req uncached-minus for [mem > 0x77b00-0x77b210fff], got write-back > > This warning is harmless, as x86 is DMA coherent and the memory gets > correctly mapped in as write-back. I will submit a patch to the DMA > mapping team to eliminate this warning. Let me know what the git commit id of that patch is, I will wait for it to hit the tree before adding this one. thanks, greg k-h
Re: [PATCH v2] USB: Disable USB2 LPM at shutdown
On Mon, Aug 05, 2019 at 08:58:33PM +0800, Kai-Heng Feng wrote: > Hi Greg, > > at 17:22, Kai-Heng Feng wrote: > > > at 22:17, Alan Stern wrote: > > > > > > I agree with Kai-Heng, this seems like a fairly light-weight solution > > > to a reasonable problem. > > > > Thanks for your review. > > > > > As to the issue of how much it will slow down system shutdowns, I have > > > no idea. Probably not very much, unless somebody has an unusually > > > large number of USB devices plugged in, but only testing can give a > > > real answer. > > > > In addition to that, only USB2 devices that enable LPM will slow down > > shutdown process. > > Right now only internally connected USB2 devices enable LPM, so the > > numbers are even lower. > > > > > I suppose we could add an HCD flag for host controllers which require > > > this workaround. Either way, it's probably not a very big deal. > > > > IMO this is not necessary. Only xHCI that reports hw_lpm_support will be > > affected. At least for PC, this only became true after Whiskey Lake. > > > > Kai-Heng > > > > > Alan Stern > > This patch is included in Ubuntu’s kernel for a while now, and there’s no > regression report so far. > Please consider merge this patch. I do not see a patch here at all, sorry. Please resend it. greg k-h
Re: [PATCH] usb: devio: fix mmap() on non-coherent DMA architectures
On Thu, Aug 01, 2019 at 03:04:36PM -0700, gavi...@thegavinli.com wrote: > From: Gavin Li > > On architectures that are not (or are optionally) DMA coherent, > dma_alloc_coherent() returns an address into the vmalloc space, > and calling virt_to_phys() on this address returns an unusable > physical address. > > This patch replaces the raw remap_pfn_range() call with a call to > dmap_mmap_coherent(), which takes care of the differences between > coherent and non-coherent code paths. > > Tested on an arm64 rk3399 board. > > Signed-off-by: Gavin Li > --- > drivers/usb/core/devio.c | 7 +++ > 1 file changed, 3 insertions(+), 4 deletions(-) Should this be backported to the stable kernel trees to fix the issue on those platforms? If so, how far back? What commit caused this problem to occur? thanks, greg k-h
Re: [PATCH] HID: usbhid: Use GFP_KERNEL instead of GFP_ATOMIC when applicable
On Thu, Aug 01, 2019 at 12:06:03PM +0200, walter harms wrote: > > > Am 01.08.2019 09:47, schrieb Christophe JAILLET: > > There is no need to use GFP_ATOMIC when calling 'usb_alloc_coherent()' > > here. These calls are done from probe functions and using GFP_KERNEL should > > be safe. > > The memory itself is used within some interrupts, but it is not a > > problem, once it has been allocated. > > > > Signed-off-by: Christophe JAILLET > > --- > > drivers/hid/usbhid/usbkbd.c | 4 ++-- > > drivers/hid/usbhid/usbmouse.c | 2 +- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/hid/usbhid/usbkbd.c b/drivers/hid/usbhid/usbkbd.c > > index d5b7a696a68c..63e8ef8beb45 100644 > > --- a/drivers/hid/usbhid/usbkbd.c > > +++ b/drivers/hid/usbhid/usbkbd.c > > @@ -239,11 +239,11 @@ static int usb_kbd_alloc_mem(struct usb_device *dev, > > struct usb_kbd *kbd) > > return -1; > > if (!(kbd->led = usb_alloc_urb(0, GFP_KERNEL))) > > return -1; > > - if (!(kbd->new = usb_alloc_coherent(dev, 8, GFP_ATOMIC, &kbd->new_dma))) > > + if (!(kbd->new = usb_alloc_coherent(dev, 8, GFP_KERNEL, &kbd->new_dma))) > > return -1; > > if (!(kbd->cr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_KERNEL))) > > return -1; > > - if (!(kbd->leds = usb_alloc_coherent(dev, 1, GFP_ATOMIC, > > &kbd->leds_dma))) > > + if (!(kbd->leds = usb_alloc_coherent(dev, 1, GFP_KERNEL, > > &kbd->leds_dma))) > > return -1; > > > > the kernel style is usually: > kbd->new = usb_alloc_coherent(dev, 8, GFP_ATOMIC, &kbd->new_dma); > if (!kbd->new) > return -1; > > > in usbmouse.c this is done, any reason for the change here ? If you want to be extra-correct, don't return -1, return -ENOMEM. thanks, greg k-h
Re: [PATCH 4.19.y 0/3] usb: dwc3: Prevent requests from being queued twice
On Mon, Jul 29, 2019 at 07:13:36PM +0530, Saranya Gopal wrote: > With recent changes in AOSP, adb is now using asynchronous I/O. > While adb works good for the most part, there have been issues with > adb root/unroot commands which cause adb hang. The issue is caused > by a request being queued twice. A series of 3 patches from > Felipe Balbi in upstream tree fixes this issue. > > Felipe Balbi (3): > usb: dwc3: gadget: add dwc3_request status tracking > usb: dwc3: gadget: prevent dwc3_request from being queued twice > usb: dwc3: gadget: remove req->started flag I would like to get an ack from Felipe before I take these. thanks, greg k-h
Re: [PATCH 4.19.y 2/3] usb: dwc3: gadget: prevent dwc3_request from being queued twice
On Mon, Jul 29, 2019 at 05:06:13PM +, Gopal, Saranya wrote: > > On Mon, Jul 29, 2019 at 07:13:38PM +0530, Saranya Gopal wrote: > > > From: Felipe Balbi > > > > > > [Upstream commit b2b6d601365a1acb90b87c85197d79] > > > > > > Queueing the same request twice can introduce hard-to-debug > > > problems. At least one function driver - Android's f_mtp.c - is known > > > to cause this problem. > > > > > > While that function is out-of-tree, this is a problem that's easy > > > enough to avoid. > > > > > > Signed-off-by: Felipe Balbi > > > Signed-off-by: Saranya Gopal > > > --- > > > drivers/usb/dwc3/gadget.c | 5 + > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > > index 3f337a0..a56a92a 100644 > > > --- a/drivers/usb/dwc3/gadget.c > > > +++ b/drivers/usb/dwc3/gadget.c > > > @@ -1291,6 +1291,11 @@ static int __dwc3_gadget_ep_queue(struct > > dwc3_ep *dep, struct dwc3_request *req) > > > &req->request, req->dep->name)) > > > return -EINVAL; > > > > > > + if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED, > > > + "%s: request %pK already in flight\n", > > > + dep->name, &req->request)) > > > + return -EINVAL; > > > > So we are going to trip syzbot up on this out-of-tree driver? Brave... > > I had retained the commit message from the upstream commit. > However, without this patch, I see issues with adb as well. > Adb will hang after adb root/unroot command and needs a reboot to recover. So you see huge WARN dumps in the log? That's fine, but be aware, if userspace can trigger this, then syzbot will trigger it, and any system running 'panic on warn' will crash. If this is something that we normally have to catch and handle, WARN() is not how to do it. But we should fix that upstream, not here. thanks, greg k-h
Re: [PATCH 4.19.y 2/3] usb: dwc3: gadget: prevent dwc3_request from being queued twice
On Mon, Jul 29, 2019 at 07:13:38PM +0530, Saranya Gopal wrote: > From: Felipe Balbi > > [Upstream commit b2b6d601365a1acb90b87c85197d79] > > Queueing the same request twice can introduce hard-to-debug > problems. At least one function driver - Android's f_mtp.c - is known > to cause this problem. > > While that function is out-of-tree, this is a problem that's easy > enough to avoid. > > Signed-off-by: Felipe Balbi > Signed-off-by: Saranya Gopal > --- > drivers/usb/dwc3/gadget.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 3f337a0..a56a92a 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1291,6 +1291,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, > struct dwc3_request *req) > &req->request, req->dep->name)) > return -EINVAL; > > + if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED, > + "%s: request %pK already in flight\n", > + dep->name, &req->request)) > + return -EINVAL; So we are going to trip syzbot up on this out-of-tree driver? Brave...
Re: [PATCH 4.19.y 3/3] usb: dwc3: gadget: remove req->started flag
On Mon, Jul 29, 2019 at 07:13:39PM +0530, Saranya Gopal wrote: > From: Felipe Balbi > > [Upstream commit 7c3d7dc89e57a1d43acea935882dd8713c9e639f] > > Now that we have req->status, we don't need this extra flag > anymore. It's safe to remove it. > > Signed-off-by: Felipe Balbi > Signed-off-by: Saranya Gopal > --- > drivers/usb/dwc3/core.h | 2 -- > drivers/usb/dwc3/gadget.c | 1 - > drivers/usb/dwc3/gadget.h | 2 -- > 3 files changed, 5 deletions(-) Why is this patch needed for a stable tree? It just cleans stuff up, it doesn't actually change any functionality. thanks, greg k-h
[GIT PULL] USB fixes for 5.3-rc2
The following changes since commit 5f9e832c137075045d15cd6899ab0505cfb2ca4b: Linus 5.3-rc1 (2019-07-21 14:05:38 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git tags/usb-5.3-rc2 for you to fetch changes up to d39b5bad8658d6d94cb2d98a44a7e159db4f5030: xhci: Fix crash if scatter gather is used with Immediate Data Transfer (IDT). (2019-07-25 11:26:42 +0200) USB fixes for 5.3-rc2 Here are some small fixes for 5.3-rc2. All of these resolve some reported issues, some more than others :) Included in here is: - xhci fix for an annoying issue with odd devices - reversion of some usb251xb patches that should not have been merged - usb pci quirk additions and fixups - usb storage fix - usb host controller error test fix All of these have been in linux-next with no reported issues. Signed-off-by: Greg Kroah-Hartman Dan Carpenter (1): usb/hcd: Fix a NULL vs IS_ERR() bug in usb_hcd_setup_local_mem() Lucas Stach (3): Revert "usb: usb251xb: Add US lanes inversion dts-bindings" Revert "usb: usb251xb: Add US port lanes inversion property" usb: usb251xb: Reallow swap-dx-lanes to apply to the upstream port Mathias Nyman (1): xhci: Fix crash if scatter gather is used with Immediate Data Transfer (IDT). Phong Tran (1): usb: wusbcore: fix unbalanced get/put cluster_id Ryan Kennedy (2): usb: pci-quirks: Correct AMD PLL quirk detection usb: pci-quirks: Minor cleanup for AMD PLL quirk Yoshihiro Shimoda (1): usb-storage: Add a limitation for blk_queue_max_hw_sectors() Documentation/devicetree/bindings/usb/usb251xb.txt | 6 +-- drivers/usb/core/hcd.c | 4 +- drivers/usb/host/ehci-pci.c| 4 +- drivers/usb/host/hwa-hc.c | 2 +- drivers/usb/host/ohci-pci.c| 2 +- drivers/usb/host/pci-quirks.c | 45 +- drivers/usb/host/pci-quirks.h | 2 +- drivers/usb/host/xhci-pci.c| 2 +- drivers/usb/host/xhci.h| 3 +- drivers/usb/misc/usb251xb.c| 15 drivers/usb/storage/scsiglue.c | 11 ++ 11 files changed, 57 insertions(+), 39 deletions(-)
Re: Oops in xhci_endpoint_reset
On Fri, Jul 26, 2019 at 11:15:46PM -0400, Bob Gleitsmann wrote: > Hello, > > > I have seen kernel oopses on waking from suspend to memory. I got this > twice, one dmesg with backtrace attached. The other one had the failure > in the same place in the code. > > > This is kernel 5.3.0-rc1, patched for another problem in ethernet PHY > driver. Have not had the problem with earlier kernels. Using Gentoo > linux, amd64, but git kernel. Any chance you can run 'git bisect' to track down the offending commit? thanks, greg k-h
