Re: [PATCH] [PATCH v8] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)
On Tue, Sep 24, 2019 at 08:14:00PM +0800, Charles Yeh wrote: > Prolific has developed a new USB to UART chip: PL2303HXN > PL2303HXN : PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE/PL2303GB > The Vendor request used by the PL2303HXN (TYPE_HXN) is different from > the existing PL2303 series (TYPE_HX & TYPE_01). > Therefore, different Vendor requests are used to issue related commands. > > 1. Added a new TYPE_HXN type in pl2303_type_data, and then executes >new Vendor request,new flow control and other related instructions >if TYPE_HXN is recognized. > > 2. Because the new PL2303HXN only accept the new Vendor request, >the old Vendor request cannot be accepted (the error message >will be returned) >So first determine the TYPE_HX or TYPE_HXN through >PL2303_READ_TYPE_HX_STATUS in pl2303_startup. > > 2.1 If the return message is "1", then the PL2303 is the existing > TYPE_HX/ TYPE_01 series. > The other settings in pl2303_startup are to continue execution. > 2.2 If the return message is "not 1", then the PL2303 is the new > TYPE_HXN series. > The other settings in pl2303_startup are ignored. > (PL2303HXN will directly use the default value in the hardware, >no need to add additional settings through the software) > > 3. In pl2303_open: Because TYPE_HXN is different from the instruction of reset >down/up stream used by TYPE_HX. >Therefore, we will also execute different instructions here. > > 4. In pl2303_set_termios: The UART flow control instructions used by >TYPE_HXN/TYPE_HX/TYPE_01 are different. >Therefore, we will also execute different instructions here. > > 5. In pl2303_vendor_read & pl2303_vendor_write, since TYPE_HXN is different >from the vendor request instruction used by TYPE_HX/TYPE_01, >it will also execute different instructions here. > > 6. In pl2303_update_reg: TYPE_HXN used different register for flow control. >Therefore, we will also execute different instructions here. > > Signed-off-by: Charles Yeh Now applied, thanks. Johan
[GIT PULL] USB-serial fixes for 5.4-rc4
The following changes since commit 4f5cafb5cb8471e54afdc9054d973535614f7675: Linux 5.4-rc3 (2019-10-13 16:37:36 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-5.4-rc4 for you to fetch changes up to bc25770f00d3f4e7482278f9823c2c2793605484: USB: serial: ti_usb_3410_5052: clean up serial data access (2019-10-16 10:29:23 +0200) USB-serial fixes for 5.4-rc4 Here's a fix for a long-standing locking bug in ti_usb_3410_5052 and related clean up. Both have been in linux-next with no reported issues. Signed-off-by: Johan Hovold Johan Hovold (2): USB: serial: ti_usb_3410_5052: fix port-close races USB: serial: ti_usb_3410_5052: clean up serial data access drivers/usb/serial/ti_usb_3410_5052.c | 12 1 file changed, 4 insertions(+), 8 deletions(-)
Re: [RFT PATCH] xhci: Fix use-after-free regression in xhci clear hub TT implementation
On Fri, Oct 11, 2019 at 03:58:42PM +0300, Mathias Nyman wrote:
> commit ef513be0a905 ("usb: xhci: Add Clear_TT_Buffer") schedules work
> to clear TT buffer, but causes a use-after-free regression at the same time
>
> Make sure hub_tt_work finishes before endpoint is disabled, otherwise
> the work will dereference already freed endpoint and device related
> pointers.
>
> This was triggered when usb core failed to read the configuration
> descriptor of a FS/LS device during enumeration.
> xhci driver queued clear_tt_work while usb core freed and reallocated
> a new device for the next enumeration attempt.
>
> EHCI driver implents ehci_endpoint_disable() that makes sure
> clear_tt_work has finished before it returns, but xhci lacks this support.
> usb core will call hcd->driver->endpoint_disable() callback before
> disabling endpoints, so we want this in xhci as well.
>
> The added xhci_endpoint_disable() is based on ehci_endpoint_disable()
>
> Fixes: ef513be0a905 ("usb: xhci: Add Clear_TT_Buffer")
> Cc: # v5.3
> Reported-by: Johan Hovold
> Signed-off-by: Mathias Nyman
> ---
> drivers/usb/host/xhci.c | 43 +++
> 1 file changed, 43 insertions(+)
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 5cfbf9a04494..6e817686d04f 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -3071,6 +3071,48 @@ void xhci_cleanup_stalled_ring(struct xhci_hcd *xhci,
> unsigned int ep_index,
> }
> }
>
> +static void xhci_endpoint_disable(struct usb_hcd *hcd,
> + struct usb_host_endpoint *host_ep)
> +{
> + struct xhci_hcd *xhci;
> + struct xhci_virt_device *vdev;
> + struct xhci_virt_ep *ep;
> + struct usb_device *udev;
> + unsigned long flags;
> + unsigned intep_index;
> +
> + xhci = hcd_to_xhci(hcd);
> +rescan:
> + spin_lock_irqsave(&xhci->lock, flags);
> +
> + udev = (struct usb_device *)host_ep->hcpriv;
> + if (!udev || !udev->slot_id)
> + goto done;
> +
> + vdev = xhci->devs[udev->slot_id];
> + if (!vdev)
> + goto done;
> +
> + ep_index = xhci_get_endpoint_index(&host_ep->desc);
> + ep = &vdev->eps[ep_index];
> + if (!ep)
> + goto done;
> +
> + /* wait for hub_tt_work to finish clearing hub TT */
> + if (ep->ep_state & EP_CLEARING_TT) {
> + spin_unlock_irqrestore(&xhci->lock, flags);
> + schedule_timeout_uninterruptible(1);
> + goto rescan;
> + }
> +
> + if (ep->ep_state)
> + xhci_dbg(xhci, "endpoint disable with ep_state 0x%x\n",
> + ep->ep_state);
> +done:
> + host_ep->hcpriv = NULL;
> + spin_unlock_irqrestore(&xhci->lock, flags);
> +}
> +
I used essentially the same reproducer as you did for debugging this
after I first hit it with an actually stalled control endpoint, and this
patch works also with my fault-injection hack.
I've reviewed the code and it looks good to me except for one mostly
theoretical issue. You need to check ep->hc_priv while holding the
xhci->lock in xhci_clear_tt_buffer_complete() or you could end up having
xhci_endpoint_disable() reschedule indefinitely while waiting for
EP_CLEARING_TT to be cleared on a sufficiently weakly ordered
system.
Since cfbb8a84c2d2 ("xhci: Fix NULL pointer dereference in
xhci_clear_tt_buffer_complete()") isn't needed anymore and is slightly
misleading, I suggest amending the patch with the following:
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 9b1e15fe2c8e..6c17e3fe181a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -5280,20 +5280,13 @@ static void xhci_clear_tt_buffer_complete(struct
usb_hcd *hcd,
unsigned int ep_index;
unsigned long flags;
- /*
-* udev might be NULL if tt buffer is cleared during a failed device
-* enumeration due to a halted control endpoint. Usb core might
-* have allocated a new udev for the next enumeration attempt.
-*/
-
xhci = hcd_to_xhci(hcd);
+
+ spin_lock_irqsave(&xhci->lock, flags);
udev = (struct usb_device *)ep->hcpriv;
- if (!udev)
- return;
slot_id = udev->slot_id;
ep_index = xhci_get_endpoint_index(&ep->desc);
- spin_lock_irqsave(&xhci->lock, flags);
xhci->devs[slot_id]->eps[ep_index].ep_state &= ~EP_CLEARING_TT;
xhci_ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
spin_unlock_irqrestore(&xhci->lock, flags);
Feel free to add my:
Suggested-by: Johan Hovold
Reviewed-by: Johan Hovold
Tested-by: Johan Hovold
Johan
Re: [PATCH v2] USB: legousbtower: fix a signedness bug in tower_probe()
On Fri, Oct 11, 2019 at 05:11:15PM +0300, Dan Carpenter wrote:
> The problem is that sizeof() is unsigned long so negative error codes
> are type promoted to high positive values and the condition becomes
> false.
>
> Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe")
> Signed-off-by: Dan Carpenter
Acked-by: Johan Hovold
> ---
> v2: style improvement suggested by Walter Harms.
>
> drivers/usb/misc/legousbtower.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
> index 9d4c52a7ebe0..9bd240df8f4c 100644
> --- a/drivers/usb/misc/legousbtower.c
> +++ b/drivers/usb/misc/legousbtower.c
> @@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface *interface,
> const struct usb_device
> get_version_reply,
> sizeof(*get_version_reply),
> 1000);
> - if (result < sizeof(*get_version_reply)) {
> + if (result != sizeof(*get_version_reply)) {
> if (result >= 0)
> result = -EIO;
> dev_err(idev, "get version request failed: %d\n", result);
Re: [PATCH] USB: legousbtower: fix a signedness bug in tower_probe()
On Fri, Oct 11, 2019 at 04:58:56PM +0300, Dan Carpenter wrote:
> On Fri, Oct 11, 2019 at 03:51:26PM +0200, walter harms wrote:
> >
> >
> > Am 11.10.2019 15:35, schrieb Dan Carpenter:
> > > The problem is that sizeof() is unsigned long so negative error codes
> > > are type promoted to high positive values and the condition becomes
> > > false.
> > >
> > > Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe")
> > > Signed-off-by: Dan Carpenter
> > > ---
> > > drivers/usb/misc/legousbtower.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/misc/legousbtower.c
> > > b/drivers/usb/misc/legousbtower.c
> > > index 9d4c52a7ebe0..835908fe1e65 100644
> > > --- a/drivers/usb/misc/legousbtower.c
> > > +++ b/drivers/usb/misc/legousbtower.c
> > > @@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface
> > > *interface, const struct usb_device
> > > get_version_reply,
> > > sizeof(*get_version_reply),
> > > 1000);
> > > - if (result < sizeof(*get_version_reply)) {
> > > + if (result < 0 || result < sizeof(*get_version_reply)) {
> > > if (result >= 0)
> > > result = -EIO;
> > > dev_err(idev, "get version request failed: %d\n", result);
> >
> > i am not an USB expert but it seems that this is a complicated way
> > to check for result != sizeof(*get_version_reply).
>
> Yeah. You're right. That would look nicer. I will resend.
Your version, or adding an explicit cast to int, may be preferred as
they document that there's something to watch out for here.
Either way you have my ack.
Johan
Re: [PATCH] USB: legousbtower: fix a signedness bug in tower_probe()
On Fri, Oct 11, 2019 at 04:35:25PM +0300, Dan Carpenter wrote:
> The problem is that sizeof() is unsigned long so negative error codes
> are type promoted to high positive values and the condition becomes
> false.
>
> Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe")
> Signed-off-by: Dan Carpenter
> ---
> drivers/usb/misc/legousbtower.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
> index 9d4c52a7ebe0..835908fe1e65 100644
> --- a/drivers/usb/misc/legousbtower.c
> +++ b/drivers/usb/misc/legousbtower.c
> @@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface *interface,
> const struct usb_device
> get_version_reply,
> sizeof(*get_version_reply),
> 1000);
> - if (result < sizeof(*get_version_reply)) {
> + if (result < 0 || result < sizeof(*get_version_reply)) {
> if (result >= 0)
> result = -EIO;
> dev_err(idev, "get version request failed: %d\n", result);
Bah, I should have noticed.
Thanks for fixing this!
Acked-by: Johan Hovold
Johan
Re: [PATCH 2/2] USB: serial: ti_usb_3410_5052: clean up serial data access
On Fri, Oct 11, 2019 at 11:57:36AM +0200, Johan Hovold wrote:
> Use the tdev pointer directly instead of going through the port data
> when accessing the serial data in open().
hmm, s/open/close/.
> Signed-off-by: Johan Hovold
> ---
> drivers/usb/serial/ti_usb_3410_5052.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
> b/drivers/usb/serial/ti_usb_3410_5052.c
> index 9174ba2e06da..ef23acc9b9ce 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -800,8 +800,8 @@ static void ti_close(struct usb_serial_port *port)
> , __func__, status);
>
> mutex_lock(&tdev->td_open_close_lock);
> - --tport->tp_tdev->td_open_port_count;
> - if (tport->tp_tdev->td_open_port_count == 0) {
> + --tdev->td_open_port_count;
> + if (tdev->td_open_port_count == 0) {
> /* last port is closed, shut down interrupt urb */
> usb_kill_urb(port->serial->port[0]->interrupt_in_urb);
> }
Johan
[PATCH 1/2] USB: serial: ti_usb_3410_5052: fix port-close races
Fix races between closing a port and opening or closing another port on
the same device which could lead to a failure to start or stop the
shared interrupt URB. The latter could potentially cause a
use-after-free or worse in the completion handler on driver unbind.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable
Signed-off-by: Johan Hovold
---
drivers/usb/serial/ti_usb_3410_5052.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
b/drivers/usb/serial/ti_usb_3410_5052.c
index dd0ad67aa71e..9174ba2e06da 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -776,7 +776,6 @@ static void ti_close(struct usb_serial_port *port)
struct ti_port *tport;
int port_number;
int status;
- int do_unlock;
unsigned long flags;
tdev = usb_get_serial_data(port->serial);
@@ -800,16 +799,13 @@ static void ti_close(struct usb_serial_port *port)
"%s - cannot send close port command, %d\n"
, __func__, status);
- /* if mutex_lock is interrupted, continue anyway */
- do_unlock = !mutex_lock_interruptible(&tdev->td_open_close_lock);
+ mutex_lock(&tdev->td_open_close_lock);
--tport->tp_tdev->td_open_port_count;
- if (tport->tp_tdev->td_open_port_count <= 0) {
+ if (tport->tp_tdev->td_open_port_count == 0) {
/* last port is closed, shut down interrupt urb */
usb_kill_urb(port->serial->port[0]->interrupt_in_urb);
- tport->tp_tdev->td_open_port_count = 0;
}
- if (do_unlock)
- mutex_unlock(&tdev->td_open_close_lock);
+ mutex_unlock(&tdev->td_open_close_lock);
}
--
2.23.0
[PATCH 2/2] USB: serial: ti_usb_3410_5052: clean up serial data access
Use the tdev pointer directly instead of going through the port data
when accessing the serial data in open().
Signed-off-by: Johan Hovold
---
drivers/usb/serial/ti_usb_3410_5052.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
b/drivers/usb/serial/ti_usb_3410_5052.c
index 9174ba2e06da..ef23acc9b9ce 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -800,8 +800,8 @@ static void ti_close(struct usb_serial_port *port)
, __func__, status);
mutex_lock(&tdev->td_open_close_lock);
- --tport->tp_tdev->td_open_port_count;
- if (tport->tp_tdev->td_open_port_count == 0) {
+ --tdev->td_open_port_count;
+ if (tdev->td_open_port_count == 0) {
/* last port is closed, shut down interrupt urb */
usb_kill_urb(port->serial->port[0]->interrupt_in_urb);
}
--
2.23.0
Re: [PATCH 5/5] USB: yurex: fix NULL-derefs on disconnect
On Wed, Oct 09, 2019 at 05:38:48PM +0200, Johan Hovold wrote:
> The driver was using its struct usb_interface pointer as an inverted
> disconnected flag, but was setting it to NULL without making sure all
> code paths that used it were done with it.
>
> Before commit ef61eb43ada6 ("USB: yurex: Fix protection fault after
> device removal") this included the interrupt-in completion handler, but
> there are further accesses in dev_err and dev_dbg statements in
> yurex_write() and the driver-data destructor (sic!).
>
> Fix this by unconditionally stopping also the control URB at disconnect
> and by using a dedicated disconnected flag.
>
> Note that we need to take a reference to the struct usb_interface to
> avoid a use-after-free in the destructor whenever the device was
> disconnected while the character device was still open.
>
> Fixes: aadd6472d904 ("USB: yurex.c: remove dbg() usage")
> Fixes: 45714104b9e8 ("USB: yurex.c: remove err() usage")
> Cc: stable # 3.5: ef61eb43ada6
> Signed-off-by: Johan Hovold
Greg, I noticed that you picked up all patches in this series except
this last one.
Was that one purpose or by mistake?
Johan
[PATCH 3/3] USB: usb-skeleton: drop redundant in-urb check
The driver bails out at probe if we can't find a bulk-in endpoint or if we fail to allocate the URB, so drop the check in read(). Signed-off-by: Johan Hovold --- drivers/usb/usb-skeleton.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c index be311787403e..2dc58766273a 100644 --- a/drivers/usb/usb-skeleton.c +++ b/drivers/usb/usb-skeleton.c @@ -230,8 +230,7 @@ static ssize_t skel_read(struct file *file, char *buffer, size_t count, dev = file->private_data; - /* if we cannot read at all, return EOF */ - if (!dev->bulk_in_urb || !count) + if (!count) return 0; /* no concurrent readers */ -- 2.23.0
[PATCH 2/3] USB: usb-skeleton: fix use-after-free after driver unbind
The driver failed to stop its read URB on disconnect, something which
could lead to a use-after-free in the completion handler after driver
unbind in case the character device has been closed.
Fixes: e7389cc9a7ff ("USB: skel_read really sucks royally")
Signed-off-by: Johan Hovold
---
drivers/usb/usb-skeleton.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index c2843fcfa52d..be311787403e 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -575,6 +575,7 @@ static void skel_disconnect(struct usb_interface *interface)
dev->disconnected = 1;
mutex_unlock(&dev->io_mutex);
+ usb_kill_urb(dev->bulk_in_urb);
usb_kill_anchored_urbs(&dev->submitted);
/* decrement our usage count */
--
2.23.0
[PATCH 1/3] USB: usb-skeleton: fix NULL-deref on disconnect
The driver was using its struct usb_interface pointer as an inverted
disconnected flag and was setting it to NULL before making sure all
completion handlers had run. This could lead to NULL-pointer
dereferences in the dev_err() statements in the completion handlers
which relies on said pointer.
Fix this by using a dedicated disconnected flag.
Note that this is also addresses a NULL-pointer dereference at release()
and a struct usb_interface reference leak introduced by a recent runtime
PM fix, which depends on and should have been submitted together with
this patch.
Fixes: 4212cd74ca6f ("USB: usb-skeleton.c: remove err() usage")
Fixes: 5c290a5e42c3 ("USB: usb-skeleton: fix runtime PM after driver unbind")
Cc: stable
Signed-off-by: Johan Hovold
---
drivers/usb/usb-skeleton.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 8001d6384c73..c2843fcfa52d 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -61,6 +61,7 @@ struct usb_skel {
spinlock_t err_lock; /* lock for errors */
struct kref kref;
struct mutexio_mutex; /* synchronize I/O with
disconnect */
+ unsigned long disconnected:1;
wait_queue_head_t bulk_in_wait; /* to wait for an
ongoing read */
};
#define to_skel_dev(d) container_of(d, struct usb_skel, kref)
@@ -238,7 +239,7 @@ static ssize_t skel_read(struct file *file, char *buffer,
size_t count,
if (rv < 0)
return rv;
- if (!dev->interface) { /* disconnect() was called */
+ if (dev->disconnected) {/* disconnect() was called */
rv = -ENODEV;
goto exit;
}
@@ -420,7 +421,7 @@ static ssize_t skel_write(struct file *file, const char
*user_buffer,
/* this lock makes sure we don't submit URBs to gone devices */
mutex_lock(&dev->io_mutex);
- if (!dev->interface) { /* disconnect() was called */
+ if (dev->disconnected) {/* disconnect() was called */
mutex_unlock(&dev->io_mutex);
retval = -ENODEV;
goto error;
@@ -571,7 +572,7 @@ static void skel_disconnect(struct usb_interface *interface)
/* prevent more I/O from starting */
mutex_lock(&dev->io_mutex);
- dev->interface = NULL;
+ dev->disconnected = 1;
mutex_unlock(&dev->io_mutex);
usb_kill_anchored_urbs(&dev->submitted);
--
2.23.0
[PATCH 0/3] USB: usb-skeleton: regression fix
I messed up when submitting the runtime PM fixes last week and failed to notice that the change to usb-skeleton depended on another fix I already had in my tree (I did notice the conflict, but rebased and sent a v2 also without the prerequisite patch). So here's a regression fix to a commit in usb-linus for usb-skeleton. :/ Included are also a use-after-free fix and a related clean up. Johan Johan Hovold (3): USB: usb-skeleton: fix NULL-deref on disconnect USB: usb-skeleton: fix use-after-free after driver unbind USB: usb-skeleton: drop redundant in-urb check drivers/usb/usb-skeleton.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) -- 2.23.0
[PATCH 0/5] USB: misc: fix disconnect bugs
This series fixes a number of issues introduced primarily with the conversion to dev_err() and dev_dbg(), which failed to notice that the drivers where using their USB interface and device pointers as disconnected flags (leading to NULL derefs) and that they did not hold references to the structures (leading to use-after-free on release()). I've already fixed up a few of these USB character device drivers separately, and the uss720 driver has similar bugs that remain to be fixed. Johan Johan Hovold (5): USB: adutux: fix use-after-free on release USB: chaoskey: fix use-after-free on release USB: ldusb: fix NULL-derefs on driver unbind USB: legousbtower: fix use-after-free on release USB: yurex: fix NULL-derefs on disconnect drivers/usb/misc/adutux.c | 3 ++- drivers/usb/misc/chaoskey.c | 5 +++-- drivers/usb/misc/ldusb.c| 24 drivers/usb/misc/legousbtower.c | 3 ++- drivers/usb/misc/yurex.c| 11 +++ 5 files changed, 26 insertions(+), 20 deletions(-) -- 2.23.0
[PATCH 2/5] USB: chaoskey: fix use-after-free on release
The driver was accessing its struct usb_interface in its release()
callback without holding a reference. This would lead to a
use-after-free whenever the device was disconnected while the character
device was still open.
Fixes: 66e3e591891d ("usb: Add driver for Altus Metrum ChaosKey device (v2)")
Cc: stable # 4.1
Signed-off-by: Johan Hovold
---
drivers/usb/misc/chaoskey.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c
index cf5828ce927a..34e6cd6f40d3 100644
--- a/drivers/usb/misc/chaoskey.c
+++ b/drivers/usb/misc/chaoskey.c
@@ -98,6 +98,7 @@ static void chaoskey_free(struct chaoskey *dev)
usb_free_urb(dev->urb);
kfree(dev->name);
kfree(dev->buf);
+ usb_put_intf(dev->interface);
kfree(dev);
}
}
@@ -145,6 +146,8 @@ static int chaoskey_probe(struct usb_interface *interface,
if (dev == NULL)
goto out;
+ dev->interface = usb_get_intf(interface);
+
dev->buf = kmalloc(size, GFP_KERNEL);
if (dev->buf == NULL)
@@ -174,8 +177,6 @@ static int chaoskey_probe(struct usb_interface *interface,
goto out;
}
- dev->interface = interface;
-
dev->in_ep = in_ep;
if (le16_to_cpu(udev->descriptor.idVendor) != ALEA_VENDOR_ID)
--
2.23.0
[PATCH 4/5] USB: legousbtower: fix use-after-free on release
The driver was accessing its struct usb_device in its release()
callback without holding a reference. This would lead to a
use-after-free whenever the device was disconnected while the character
device was still open.
Fixes: fef526cae700 ("USB: legousbtower: remove custom debug macro")
Cc: stable # 3.12
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 44d6a3381804..9d4c52a7ebe0 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -296,6 +296,7 @@ static inline void tower_delete (struct lego_usb_tower *dev)
kfree (dev->read_buffer);
kfree (dev->interrupt_in_buffer);
kfree (dev->interrupt_out_buffer);
+ usb_put_dev(dev->udev);
kfree (dev);
}
@@ -810,7 +811,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
mutex_init(&dev->lock);
- dev->udev = udev;
+ dev->udev = usb_get_dev(udev);
dev->open_count = 0;
dev->disconnected = 0;
--
2.23.0
[PATCH 3/5] USB: ldusb: fix NULL-derefs on driver unbind
The driver was using its struct usb_interface pointer as an inverted
disconnected flag, but was setting it to NULL before making sure all
completion handlers had run. This could lead to a NULL-pointer
dereference in a number of dev_dbg, dev_warn and dev_err statements in
the completion handlers which relies on said pointer.
Fix this by unconditionally stopping all I/O and preventing
resubmissions by poisoning the interrupt URBs at disconnect and using a
dedicated disconnected flag.
This also makes sure that all I/O has completed by the time the
disconnect callback returns.
Fixes: 2824bd250f0b ("[PATCH] USB: add ldusb driver")
Cc: stable # 2.6.13
Signed-off-by: Johan Hovold
---
drivers/usb/misc/ldusb.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
index 6581774bdfa4..f3108d85e768 100644
--- a/drivers/usb/misc/ldusb.c
+++ b/drivers/usb/misc/ldusb.c
@@ -153,6 +153,7 @@ MODULE_PARM_DESC(min_interrupt_out_interval, "Minimum
interrupt out interval in
struct ld_usb {
struct mutexmutex; /* locks this structure */
struct usb_interface*intf; /* save off the usb interface
pointer */
+ unsigned long disconnected:1;
int open_count; /* number of times this port
has been opened */
@@ -192,12 +193,10 @@ static void ld_usb_abort_transfers(struct ld_usb *dev)
/* shutdown transfer */
if (dev->interrupt_in_running) {
dev->interrupt_in_running = 0;
- if (dev->intf)
- usb_kill_urb(dev->interrupt_in_urb);
+ usb_kill_urb(dev->interrupt_in_urb);
}
if (dev->interrupt_out_busy)
- if (dev->intf)
- usb_kill_urb(dev->interrupt_out_urb);
+ usb_kill_urb(dev->interrupt_out_urb);
}
/**
@@ -205,8 +204,6 @@ static void ld_usb_abort_transfers(struct ld_usb *dev)
*/
static void ld_usb_delete(struct ld_usb *dev)
{
- ld_usb_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev->interrupt_in_urb);
usb_free_urb(dev->interrupt_out_urb);
@@ -263,7 +260,7 @@ static void ld_usb_interrupt_in_callback(struct urb *urb)
resubmit:
/* resubmit if we're still running */
- if (dev->interrupt_in_running && !dev->buffer_overflow && dev->intf) {
+ if (dev->interrupt_in_running && !dev->buffer_overflow) {
retval = usb_submit_urb(dev->interrupt_in_urb, GFP_ATOMIC);
if (retval) {
dev_err(&dev->intf->dev,
@@ -392,7 +389,7 @@ static int ld_usb_release(struct inode *inode, struct file
*file)
retval = -ENODEV;
goto unlock_exit;
}
- if (dev->intf == NULL) {
+ if (dev->disconnected) {
/* the device was unplugged before the file was released */
mutex_unlock(&dev->mutex);
/* unlock here as ld_usb_delete frees dev */
@@ -423,7 +420,7 @@ static __poll_t ld_usb_poll(struct file *file, poll_table
*wait)
dev = file->private_data;
- if (!dev->intf)
+ if (dev->disconnected)
return EPOLLERR | EPOLLHUP;
poll_wait(file, &dev->read_wait, wait);
@@ -462,7 +459,7 @@ static ssize_t ld_usb_read(struct file *file, char __user
*buffer, size_t count,
}
/* verify that the device wasn't unplugged */
- if (dev->intf == NULL) {
+ if (dev->disconnected) {
retval = -ENODEV;
printk(KERN_ERR "ldusb: No device or device unplugged %d\n",
retval);
goto unlock_exit;
@@ -542,7 +539,7 @@ static ssize_t ld_usb_write(struct file *file, const char
__user *buffer,
}
/* verify that the device wasn't unplugged */
- if (dev->intf == NULL) {
+ if (dev->disconnected) {
retval = -ENODEV;
printk(KERN_ERR "ldusb: No device or device unplugged %d\n",
retval);
goto unlock_exit;
@@ -764,6 +761,9 @@ static void ld_usb_disconnect(struct usb_interface *intf)
/* give back our minor */
usb_deregister_dev(intf, &ld_usb_class);
+ usb_poison_urb(dev->interrupt_in_urb);
+ usb_poison_urb(dev->interrupt_out_urb);
+
mutex_lock(&dev->mutex);
/* if the device is not opened, then we clean up right now */
@@ -771,7 +771,7 @@ static void ld_usb_disconnect(struct usb_interface *intf)
mutex_unlock(&dev->mutex);
ld_usb_delete(dev);
} else {
- dev->intf = NULL;
+ dev->disconnected = 1;
/* wake up pollers */
wake_up_interruptible_all(&dev->read_wait);
wake_up_interruptible_all(&dev->write_wait);
--
2.23.0
[PATCH 5/5] USB: yurex: fix NULL-derefs on disconnect
The driver was using its struct usb_interface pointer as an inverted
disconnected flag, but was setting it to NULL without making sure all
code paths that used it were done with it.
Before commit ef61eb43ada6 ("USB: yurex: Fix protection fault after
device removal") this included the interrupt-in completion handler, but
there are further accesses in dev_err and dev_dbg statements in
yurex_write() and the driver-data destructor (sic!).
Fix this by unconditionally stopping also the control URB at disconnect
and by using a dedicated disconnected flag.
Note that we need to take a reference to the struct usb_interface to
avoid a use-after-free in the destructor whenever the device was
disconnected while the character device was still open.
Fixes: aadd6472d904 ("USB: yurex.c: remove dbg() usage")
Fixes: 45714104b9e8 ("USB: yurex.c: remove err() usage")
Cc: stable # 3.5: ef61eb43ada6
Signed-off-by: Johan Hovold
---
drivers/usb/misc/yurex.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/misc/yurex.c b/drivers/usb/misc/yurex.c
index 8d52d4336c29..be0505b8b5d4 100644
--- a/drivers/usb/misc/yurex.c
+++ b/drivers/usb/misc/yurex.c
@@ -60,6 +60,7 @@ struct usb_yurex {
struct kref kref;
struct mutexio_mutex;
+ unsigned long disconnected:1;
struct fasync_struct*async_queue;
wait_queue_head_t waitq;
@@ -107,6 +108,7 @@ static void yurex_delete(struct kref *kref)
dev->int_buffer, dev->urb->transfer_dma);
usb_free_urb(dev->urb);
}
+ usb_put_intf(dev->interface);
usb_put_dev(dev->udev);
kfree(dev);
}
@@ -205,7 +207,7 @@ static int yurex_probe(struct usb_interface *interface,
const struct usb_device_
init_waitqueue_head(&dev->waitq);
dev->udev = usb_get_dev(interface_to_usbdev(interface));
- dev->interface = interface;
+ dev->interface = usb_get_intf(interface);
/* set up the endpoint information */
iface_desc = interface->cur_altsetting;
@@ -316,8 +318,9 @@ static void yurex_disconnect(struct usb_interface
*interface)
/* prevent more I/O from starting */
usb_poison_urb(dev->urb);
+ usb_poison_urb(dev->cntl_urb);
mutex_lock(&dev->io_mutex);
- dev->interface = NULL;
+ dev->disconnected = 1;
mutex_unlock(&dev->io_mutex);
/* wakeup waiters */
@@ -405,7 +408,7 @@ static ssize_t yurex_read(struct file *file, char __user
*buffer, size_t count,
dev = file->private_data;
mutex_lock(&dev->io_mutex);
- if (!dev->interface) { /* already disconnected */
+ if (dev->disconnected) {/* already disconnected */
mutex_unlock(&dev->io_mutex);
return -ENODEV;
}
@@ -440,7 +443,7 @@ static ssize_t yurex_write(struct file *file, const char
__user *user_buffer,
goto error;
mutex_lock(&dev->io_mutex);
- if (!dev->interface) { /* already disconnected */
+ if (dev->disconnected) {/* already disconnected */
mutex_unlock(&dev->io_mutex);
retval = -ENODEV;
goto error;
--
2.23.0
[PATCH 1/5] USB: adutux: fix use-after-free on release
The driver was accessing its struct usb_device in its release()
callback without holding a reference. This would lead to a
use-after-free whenever the device was disconnected while the character
device was still open.
Fixes: 66d4bc30d128 ("USB: adutux: remove custom debug macro")
Cc: stable # 3.12
Signed-off-by: Johan Hovold
---
drivers/usb/misc/adutux.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index f9efec719359..6f5edb9fc61e 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -149,6 +149,7 @@ static void adu_delete(struct adu_device *dev)
kfree(dev->read_buffer_secondary);
kfree(dev->interrupt_in_buffer);
kfree(dev->interrupt_out_buffer);
+ usb_put_dev(dev->udev);
kfree(dev);
}
@@ -664,7 +665,7 @@ static int adu_probe(struct usb_interface *interface,
mutex_init(&dev->mtx);
spin_lock_init(&dev->buflock);
- dev->udev = udev;
+ dev->udev = usb_get_dev(udev);
init_waitqueue_head(&dev->read_wait);
init_waitqueue_head(&dev->write_wait);
--
2.23.0
[PATCH] USB: core: drop OOM message
Drop redundant OOM message on allocation failures which would already
have been logged by the allocator. This also allows us to clean up the
error paths somewhat.
Signed-off-by: Johan Hovold
---
drivers/usb/core/config.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index 151a74a54386..ff9f50f7218f 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -800,10 +800,10 @@ int usb_get_configuration(struct usb_device *dev)
{
struct device *ddev = &dev->dev;
int ncfg = dev->descriptor.bNumConfigurations;
- int result = -ENOMEM;
unsigned int cfgno, length;
unsigned char *bigbuffer;
struct usb_config_descriptor *desc;
+ int result;
if (ncfg > USB_MAXCONFIG) {
dev_warn(ddev, "too many configurations: %d, "
@@ -819,16 +819,16 @@ int usb_get_configuration(struct usb_device *dev)
length = ncfg * sizeof(struct usb_host_config);
dev->config = kzalloc(length, GFP_KERNEL);
if (!dev->config)
- goto err2;
+ return -ENOMEM;
length = ncfg * sizeof(char *);
dev->rawdescriptors = kzalloc(length, GFP_KERNEL);
if (!dev->rawdescriptors)
- goto err2;
+ return -ENOMEM;
desc = kmalloc(USB_DT_CONFIG_SIZE, GFP_KERNEL);
if (!desc)
- goto err2;
+ return -ENOMEM;
for (cfgno = 0; cfgno < ncfg; cfgno++) {
/* We grab just the first descriptor so we know how long
@@ -890,9 +890,7 @@ int usb_get_configuration(struct usb_device *dev)
err:
kfree(desc);
dev->descriptor.bNumConfigurations = cfgno;
-err2:
- if (result == -ENOMEM)
- dev_err(ddev, "out of memory\n");
+
return result;
}
--
2.23.0
Re: [PATCH 8/8] xhci: Fix NULL pointer dereference in xhci_clear_tt_buffer_complete()
[ +CC: Alan ]
On Fri, Oct 04, 2019 at 02:59:33PM +0300, Mathias Nyman wrote:
> udev stored in ep->hcpriv might be NULL if tt buffer is cleared
> due to a halted control endpoint during device enumeration
>
> xhci_clear_tt_buffer_complete is called by hub_tt_work() once it's
> scheduled, and by then usb core might have freed and allocated a
> new udev for the next enumeration attempt.
>
> Fixes: ef513be0a905 ("usb: xhci: Add Clear_TT_Buffer")
> Cc: # v5.3
> Reported-by: Johan Hovold
> Signed-off-by: Mathias Nyman
> ---
> drivers/usb/host/xhci.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 00f3804f7aa7..517ec3206f6e 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -5238,8 +5238,16 @@ static void xhci_clear_tt_buffer_complete(struct
> usb_hcd *hcd,
> unsigned int ep_index;
> unsigned long flags;
>
> + /*
> + * udev might be NULL if tt buffer is cleared during a failed device
> + * enumeration due to a halted control endpoint. Usb core might
> + * have allocated a new udev for the next enumeration attempt.
> + */
> +
> xhci = hcd_to_xhci(hcd);
> udev = (struct usb_device *)ep->hcpriv;
> + if (!udev)
> + return;
I didn't have time to look into this myself last week, or comment on the
patch before Greg picked it up, but this clearly isn't the right fix.
As your comment suggests, ep->hcpriv may indeed be NULL here if USB core
have allocated a new udev. But this only happens after USB has freed the
old usb_device and the new one happens to get the same address.
Note that even the usb_host_endpoint itself (ep) has then been freed and
reallocated since it is member of struct usb_device, and it is the
use-after-free that needs fixing.
I've even been able to trigger another NULL-deref in this function
before a new udev has been allocated, due to the virt dev having been
freed by xhci_free_dev as part of usb_release_dev:
[ 19.627771] usb 2-2.4: unable to read config index 0 descriptor/start: -32
[ 19.627966] usb 2-2.4: chopping to 0 config(s)
[ 19.628133] usb 2-2.4: can't read configurations, error -32
[ 19.629017] usb 2-2.4: usb_release_dev - udev = 930b14d82000
udev is freed here
[ 19.629258] usb 2-2-port4: attempt power cycle
[ 19.629461] xhci_clear_tt_buffer_complete - udev = 930b14d82000
use-after-free when tt work is scheduled (note than udev is non-NULL
since udev hasn't been reallocated and initialised yet):
[ 19.629643] xhci_clear_tt_buffer_complete - xhci->devs[4] =
virt dev is NULL after having been freed by xhci_free_dev()
[ 19.629876] BUG: kernel NULL pointer dereference, address: 0030
and is later dereferenced
[ 19.630034] #PF: supervisor write access in kernel mode
[ 19.630155] #PF: error_code(0x0002) - not-present page
[ 19.630270] PGD 0 P4D 0
[ 19.630341] Oops: 0002 [#1] SMP
[ 19.630425] CPU: 2 PID: 110 Comm: kworker/2:2 Not tainted 5.4.0-rc1 #28
[ 19.630572] Hardware name: /D34010WYK, BIOS
WYLPT10H.86A.0051.2019.0322.1320 03/22/2019
[ 19.636141] Workqueue: events hub_tt_work
[ 19.638125] RIP: 0010:xhci_clear_tt_buffer_complete.cold.69+0x9b/0xcd
It seems the xhci clear-tt implementation was incomplete since it did
not take care to wait for any ongoing work before disabling the
endpoint. EHCI does this in ehci_endpoint_disable(), but xhci doesn't
even implement that callback.
As this may be something you could end up hitting in other paths as
well, perhaps we should even consider reverting the offending commit
pending a more complete implementation?
> slot_id = udev->slot_id;
> ep_index = xhci_get_endpoint_index(&ep->desc);
For reference, my original report is here:
https://lkml.kernel.org/r/20190930103107.GC13531@localhost
Johan
[GIT PULL] USB-serial fixes for 5.4-rc2
The following changes since commit 54ecb8f7028c5eb3d740bb82b0f1d90f2df63c5c: Linux 5.4-rc1 (2019-09-30 10:35:40 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-5.4-rc2 for you to fetch changes up to 7d7e21fafdbc7fcf0854b877bd0975b487ed2717: USB: serial: keyspan: fix NULL-derefs on open() and write() (2019-10-04 10:57:19 +0200) USB-serial fixes for 5.4-rc2 Here's a fix for a long-standing issue in the keyspan driver which could lead to NULL-pointer dereferences when a device had unexpected endpoint descriptors. Included are also some new device IDs. All but the last two commits have been in linux-next with no reported issues. Signed-off-by: Johan Hovold Beni Mahler (1): USB: serial: ftdi_sio: add device IDs for Sienna and Echelon PL-20 Daniele Palmas (1): USB: serial: option: add Telit FN980 compositions Johan Hovold (1): USB: serial: keyspan: fix NULL-derefs on open() and write() Reinhard Speyerer (1): USB: serial: option: add support for Cinterion CLS8 devices drivers/usb/serial/ftdi_sio.c | 3 +++ drivers/usb/serial/ftdi_sio_ids.h | 9 + drivers/usb/serial/keyspan.c | 4 ++-- drivers/usb/serial/option.c | 11 +++ 4 files changed, 25 insertions(+), 2 deletions(-)
Re: [PATCH] USB: serial: option: add support for Cinterion CLS8 devices
On Thu, Oct 03, 2019 at 06:53:21PM +0200, Reinhard Speyerer wrote: > Add support for the serial ports of Cinterion CLS8 devices. > > T: Bus=01 Lev=03 Prnt=05 Port=01 Cnt=02 Dev#= 25 Spd=480 MxCh= 0 > D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1 > P: Vendor=1e2d ProdID=00b0 Rev= 3.18 > S: Manufacturer=GEMALTO > S: Product=USB Modem > C:* #Ifs= 5 Cfg#= 1 Atr=80 MxPwr=500mA > I:* If#= 0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=42 Prot=01 Driver=(none) > E: Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=81(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 1 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option > E: Ad=83(I) Atr=03(Int.) MxPS= 10 Ivl=32ms > E: Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option > E: Ad=85(I) Atr=03(Int.) MxPS= 10 Ivl=32ms > E: Ad=84(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=03(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option > E: Ad=87(I) Atr=03(Int.) MxPS= 10 Ivl=32ms > E: Ad=86(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=04(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 4 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=qmi_wwan > E: Ad=89(I) Atr=03(Int.) MxPS= 8 Ivl=32ms > E: Ad=88(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=05(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > > Signed-off-by: Reinhard Speyerer Applied, thanks. Johan
[PATCH] USB: serial: keyspan: fix NULL-derefs on open() and write()
Fix NULL-pointer dereferences on open() and write() which can be
triggered by a malicious USB device.
The current URB allocation helper would fail to initialise the newly
allocated URB if the device has unexpected endpoint descriptors,
something which could lead NULL-pointer dereferences in a number of
open() and write() paths when accessing the URB. For example:
BUG: kernel NULL pointer dereference, address:
...
RIP: 0010:usb_clear_halt+0x11/0xc0
...
Call Trace:
? tty_port_open+0x4d/0xd0
keyspan_open+0x70/0x160 [keyspan]
serial_port_activate+0x5b/0x80 [usbserial]
tty_port_open+0x7b/0xd0
? check_tty_count+0x43/0xa0
tty_open+0xf1/0x490
BUG: kernel NULL pointer dereference, address:
...
RIP: 0010:keyspan_write+0x14e/0x1f3 [keyspan]
...
Call Trace:
serial_write+0x43/0xa0 [usbserial]
n_tty_write+0x1af/0x4f0
? do_wait_intr_irq+0x80/0x80
? process_echoes+0x60/0x60
tty_write+0x13f/0x2f0
BUG: kernel NULL pointer dereference, address:
...
RIP: 0010:keyspan_usa26_send_setup+0x298/0x305 [keyspan]
...
Call Trace:
keyspan_open+0x10f/0x160 [keyspan]
serial_port_activate+0x5b/0x80 [usbserial]
tty_port_open+0x7b/0xd0
? check_tty_count+0x43/0xa0
tty_open+0xf1/0x490
Fixes: fdcba53e2d58 ("fix for bugzilla #7544 (keyspan USB-to-serial converter)")
Cc: stable # 2.6.21
Signed-off-by: Johan Hovold
---
drivers/usb/serial/keyspan.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/keyspan.c b/drivers/usb/serial/keyspan.c
index d34779fe4a8d..e66a59ef43a1 100644
--- a/drivers/usb/serial/keyspan.c
+++ b/drivers/usb/serial/keyspan.c
@@ -1741,8 +1741,8 @@ static struct urb *keyspan_setup_urb(struct usb_serial
*serial, int endpoint,
ep_desc = find_ep(serial, endpoint);
if (!ep_desc) {
- /* leak the urb, something's wrong and the callers don't care */
- return urb;
+ usb_free_urb(urb);
+ return NULL;
}
if (usb_endpoint_xfer_int(ep_desc)) {
ep_type_name = "INT";
--
2.23.0
[PATCH] USB: microtek: fix info-leak at probe
Add missing bulk-in endpoint sanity check to prevent uninitialised stack
data from being reported to the system log and used as endpoint
addresses.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable
Reported-by: syzbot+5630ca7c3b2be5c9d...@syzkaller.appspotmail.com
Signed-off-by: Johan Hovold
---
drivers/usb/image/microtek.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/image/microtek.c b/drivers/usb/image/microtek.c
index 0a57c2cc8e5a..7a6b122c833f 100644
--- a/drivers/usb/image/microtek.c
+++ b/drivers/usb/image/microtek.c
@@ -716,6 +716,10 @@ static int mts_usb_probe(struct usb_interface *intf,
}
+ if (ep_in_current != &ep_in_set[2]) {
+ MTS_WARNING("couldn't find two input bulk endpoints. Bailing
out.\n");
+ return -ENODEV;
+ }
if ( ep_out == -1 ) {
MTS_WARNING( "couldn't find an output bulk endpoint. Bailing
out.\n" );
--
2.23.0
Re: [PATCH v2 1/1] usb: serial: option: add Telit FN980 compositions
On Mon, Sep 23, 2019 at 12:23:28PM +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 > > Signed-off-by: Daniele Palmas > --- > Sorry, forgot to add Signed-off-by tag... > > v2: added Signed-off-by tag Now applied, thanks. Johan
Re: [PATCH] USB: serial: FTDI: Add device IDs for Sienna and Echelon PL-20
On Thu, Sep 05, 2019 at 12:26:20AM +0200, beni.mah...@gmx.net wrote: > From: Beni Mahler > > Both devices added here have a FTDI chip inside. The device from Echelon > is called 'Network Interface' it is actually a LON network gateway. > > ID 0403:8348 Future Technology Devices International, Ltd > > https://www.eltako.com/fileadmin/downloads/de/datenblatt/Datenblatt_PL-SW-PROF.pdf > > ID 0920:7500 Network Interface > https://www.echelon.com/products/u20-usb-network-interface It wasn't obvious to me that these two are essentially the same product (?) using different IDs so at first I wondered why you included them in the same patch. > Signed-off-by: Beni Mahler Now applied, thanks. Johan
Re: musb: cppi41: broken high speed FTDI functionality when connected to musb directly
On Mon, Sep 30, 2019 at 09:54:11PM +0200, Sebastian Reichel wrote: > Hi, > > On Mon, Sep 30, 2019 at 08:23:30AM -0700, Tony Lindgren wrote: > > Actually playing with the cppi41 timeout might be more suitable here, > > they use the same module clock from what I remember though. So > > maybe increase the cppi41 autosuspend_timeout from 100 ms to 500 ms > > or higher: > > > > # echo 500 > > > /sys/bus/platform/drivers/cppi41-dma-engine/4740.dma-controller/power/autosuspend_delay_ms > > > > If changing the autosuspend_timeout_ms value does not help, then > > try setting control to on there. > > I did not check the details, but from the cover-letter this might be > woth looking into: > > https://lore.kernel.org/lkml/20190930161205.18803-1-jo...@kernel.org/ No, that one should be unrelated as it would only prevent later suspends after a driver has been unbound (and rebound). Johan signature.asc Description: PGP signature
NULL-deref in xhci_clear_tt_buffer_complete()
Hi Mathias,
I hit this NULL-deref in xhci_clear_tt_buffer_complete() with usb-next
after an external HS hub with a connected FS device got into some weird
state this morning:
[ 66.833702] usb 2-2.4: USB disconnect, device number 5
[ 66.834756] usblcd 2-2.4:1.0: USB LCD #144 now disconnected
[ 67.774259] usb 2-2.4: new full-speed USB device number 6 using xhci_hcd
[ 67.855160] usb 2-2.4: unable to read config index 0 descriptor/start: -32
[ 67.855306] usb 2-2.4: chopping to 0 config(s)
[ 67.855401] usb 2-2.4: can't read configurations, error -32
[ 67.856455] BUG: kernel NULL pointer dereference, address: 06d8
[ 67.856554] #PF: supervisor read access in kernel mode
[ 67.856635] #PF: error_code(0x) - not-present page
[ 67.856712] PGD 0 P4D 0
[ 67.856760] Oops: [#1] SMP
[ 67.856815] CPU: 2 PID: 97 Comm: kworker/2:2 Not tainted 5.3.0-rc7 #4
[ 67.856904] Hardware name: /D34010WYK, BIOS
WYLPT10H.86A.0051.2019.0322.1320 03/22/2019
[ 67.857017] Workqueue: events hub_tt_work
[ 67.857089] RIP: 0010:xhci_clear_tt_buffer_complete+0x2b/0xb0
[ 67.857173] Code: 57 41 56 41 55 49 89 f5 41 54 55 53 48 89 fb e8 db 94 fd
ff 85 c0 75 07 48 8b 9b 58 03 00 00 49 8b 45 28 4c 8d a3 90 03 00 00 <8b> a8 d8
06 00 00 41 f6 45 03 03 75 60 45 0f b6 75 02 41 83 e6 0f
[ 67.857404] RSP: 0018:a6020029fde8 EFLAGS: 00010202
[ 67.857482] RAX: RBX: 94cd55358000 RCX: 01f3
[ 67.857577] RDX: 01f2 RSI: 94cd50db2850 RDI: 94cd55358000
[ 67.857672] RBP: 94cd55372000 R08: R09:
[ 67.857767] R10: R11: R12: 94cd55358390
[ 67.857860] R13: 94cd50db2850 R14: dead0122 R15: dead0100
[ 67.857956] FS: () GS:94cd5790()
knlGS:
[ 67.858060] CS: 0010 DS: ES: CR0: 80050033
[ 67.858141] CR2: 06d8 CR3: 000213baa001 CR4: 001606e0
[ 67.858236] Call Trace:
[ 67.858287] hub_tt_work+0x154/0x190
[ 67.858353] process_one_work+0x2a0/0x600
[ 67.858425] worker_thread+0x34/0x3d0
[ 67.858490] ? process_one_work+0x600/0x600
[ 67.858558] kthread+0x118/0x130
[ 67.858614] ? kthread_create_on_node+0x60/0x60
[ 67.858688] ret_from_fork+0x3a/0x50
[ 67.858753] Modules linked in: netconsole ftdi_sio x86_pkg_temp_thermal
usbserial usblcd
[ 67.858865] CR2: 06d8
[ 67.858922] ---[ end trace 7fb6e59f68b07112 ]---
Address 06d8 is udev->slot_id (line 5203) so apparently
udev = (struct usb_device *)ep->hcpriv;
can be NULL here.
While the hub/device was in this state, the bug appeared to be perfectly
reproducable and prevented the machine from booting. Disconnecting and
reconnecting the hub made the problem go away.
Also adding Jim Lin who introduced this code in commit ef513be0a905
("usb: xhci: Add Clear_TT_Buffer") in v5.2.
Johan
Re: [PATCH 0/4] USB: usblcd: disconnect fix and locking clean ups
On Thu, Sep 26, 2019 at 11:12:24AM +0200, Johan Hovold wrote: > This series fixes a failure to stop I/O on disconnect() in the usblcd > driver. Turns out there was a lot of legacy cruft in this driver which > could simply be removed. My apologies for the double post. Johan
[PATCH 1/4] USB: usblcd: fix I/O after disconnect
Make sure to stop all I/O on disconnect by adding a disconnected flag
which is used to prevent new I/O from being started and by stopping all
ongoing I/O before returning.
This also fixes a potential use-after-free on driver unbind in case the
driver data is freed before the completion handler has run.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable # 7bbe990c989e
Signed-off-by: Johan Hovold
---
drivers/usb/misc/usblcd.c | 33 +++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/usblcd.c b/drivers/usb/misc/usblcd.c
index 9ba4a4e68d91..aa982d3ca36b 100644
--- a/drivers/usb/misc/usblcd.c
+++ b/drivers/usb/misc/usblcd.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -57,6 +58,8 @@ struct usb_lcd {
using up all RAM */
struct usb_anchor submitted; /* URBs to wait for
before suspend */
+ struct rw_semaphore io_rwsem;
+ unsigned long disconnected:1;
};
#define to_lcd_dev(d) container_of(d, struct usb_lcd, kref)
@@ -142,6 +145,13 @@ static ssize_t lcd_read(struct file *file, char __user *
buffer,
dev = file->private_data;
+ down_read(&dev->io_rwsem);
+
+ if (dev->disconnected) {
+ retval = -ENODEV;
+ goto out_up_io;
+ }
+
/* do a blocking bulk read to get data from the device */
retval = usb_bulk_msg(dev->udev,
usb_rcvbulkpipe(dev->udev,
@@ -158,6 +168,9 @@ static ssize_t lcd_read(struct file *file, char __user *
buffer,
retval = bytes_read;
}
+out_up_io:
+ up_read(&dev->io_rwsem);
+
return retval;
}
@@ -237,11 +250,18 @@ static ssize_t lcd_write(struct file *file, const char
__user * user_buffer,
if (r < 0)
return -EINTR;
+ down_read(&dev->io_rwsem);
+
+ if (dev->disconnected) {
+ retval = -ENODEV;
+ goto err_up_io;
+ }
+
/* create a urb, and a buffer for it, and copy the data to the urb */
urb = usb_alloc_urb(0, GFP_KERNEL);
if (!urb) {
retval = -ENOMEM;
- goto err_no_buf;
+ goto err_up_io;
}
buf = usb_alloc_coherent(dev->udev, count, GFP_KERNEL,
@@ -278,6 +298,7 @@ static ssize_t lcd_write(struct file *file, const char
__user * user_buffer,
the USB core will eventually free it entirely */
usb_free_urb(urb);
+ up_read(&dev->io_rwsem);
exit:
return count;
error_unanchor:
@@ -285,7 +306,8 @@ static ssize_t lcd_write(struct file *file, const char
__user * user_buffer,
error:
usb_free_coherent(dev->udev, count, buf, urb->transfer_dma);
usb_free_urb(urb);
-err_no_buf:
+err_up_io:
+ up_read(&dev->io_rwsem);
up(&dev->limit_sem);
return retval;
}
@@ -325,6 +347,7 @@ static int lcd_probe(struct usb_interface *interface,
kref_init(&dev->kref);
sema_init(&dev->limit_sem, USB_LCD_CONCURRENT_WRITES);
+ init_rwsem(&dev->io_rwsem);
init_usb_anchor(&dev->submitted);
dev->udev = usb_get_dev(interface_to_usbdev(interface));
@@ -422,6 +445,12 @@ static void lcd_disconnect(struct usb_interface *interface)
/* give back our minor */
usb_deregister_dev(interface, &lcd_class);
+ down_write(&dev->io_rwsem);
+ dev->disconnected = 1;
+ up_write(&dev->io_rwsem);
+
+ usb_kill_anchored_urbs(&dev->submitted);
+
/* decrement our usage count */
kref_put(&dev->kref, lcd_delete);
--
2.23.0
[PATCH 1/4] USB: usblcd: fix I/O after disconnect
Make sure to stop all I/O on disconnect by adding a disconnected flag
which is used to prevent new I/O from being started and by stopping all
ongoing I/O before returning.
This also fixes a potential use-after-free on driver unbind in case the
driver data is freed before the completion handler has run.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable # 7bbe990c989e
Signed-off-by: Johan Hovold
---
drivers/usb/misc/usblcd.c | 33 +++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/usblcd.c b/drivers/usb/misc/usblcd.c
index 9ba4a4e68d91..aa982d3ca36b 100644
--- a/drivers/usb/misc/usblcd.c
+++ b/drivers/usb/misc/usblcd.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -57,6 +58,8 @@ struct usb_lcd {
using up all RAM */
struct usb_anchor submitted; /* URBs to wait for
before suspend */
+ struct rw_semaphore io_rwsem;
+ unsigned long disconnected:1;
};
#define to_lcd_dev(d) container_of(d, struct usb_lcd, kref)
@@ -142,6 +145,13 @@ static ssize_t lcd_read(struct file *file, char __user *
buffer,
dev = file->private_data;
+ down_read(&dev->io_rwsem);
+
+ if (dev->disconnected) {
+ retval = -ENODEV;
+ goto out_up_io;
+ }
+
/* do a blocking bulk read to get data from the device */
retval = usb_bulk_msg(dev->udev,
usb_rcvbulkpipe(dev->udev,
@@ -158,6 +168,9 @@ static ssize_t lcd_read(struct file *file, char __user *
buffer,
retval = bytes_read;
}
+out_up_io:
+ up_read(&dev->io_rwsem);
+
return retval;
}
@@ -237,11 +250,18 @@ static ssize_t lcd_write(struct file *file, const char
__user * user_buffer,
if (r < 0)
return -EINTR;
+ down_read(&dev->io_rwsem);
+
+ if (dev->disconnected) {
+ retval = -ENODEV;
+ goto err_up_io;
+ }
+
/* create a urb, and a buffer for it, and copy the data to the urb */
urb = usb_alloc_urb(0, GFP_KERNEL);
if (!urb) {
retval = -ENOMEM;
- goto err_no_buf;
+ goto err_up_io;
}
buf = usb_alloc_coherent(dev->udev, count, GFP_KERNEL,
@@ -278,6 +298,7 @@ static ssize_t lcd_write(struct file *file, const char
__user * user_buffer,
the USB core will eventually free it entirely */
usb_free_urb(urb);
+ up_read(&dev->io_rwsem);
exit:
return count;
error_unanchor:
@@ -285,7 +306,8 @@ static ssize_t lcd_write(struct file *file, const char
__user * user_buffer,
error:
usb_free_coherent(dev->udev, count, buf, urb->transfer_dma);
usb_free_urb(urb);
-err_no_buf:
+err_up_io:
+ up_read(&dev->io_rwsem);
up(&dev->limit_sem);
return retval;
}
@@ -325,6 +347,7 @@ static int lcd_probe(struct usb_interface *interface,
kref_init(&dev->kref);
sema_init(&dev->limit_sem, USB_LCD_CONCURRENT_WRITES);
+ init_rwsem(&dev->io_rwsem);
init_usb_anchor(&dev->submitted);
dev->udev = usb_get_dev(interface_to_usbdev(interface));
@@ -422,6 +445,12 @@ static void lcd_disconnect(struct usb_interface *interface)
/* give back our minor */
usb_deregister_dev(interface, &lcd_class);
+ down_write(&dev->io_rwsem);
+ dev->disconnected = 1;
+ up_write(&dev->io_rwsem);
+
+ usb_kill_anchored_urbs(&dev->submitted);
+
/* decrement our usage count */
kref_put(&dev->kref, lcd_delete);
--
2.23.0
[PATCH 3/4] USB: usblcd: drop redundant lcd mutex
Drop the redundant lcd mutex introduced by commit 925ce689bb31 ("USB:
autoconvert trivial BKL users to private mutex") which replaced an
earlier BKL use.
The lock serialised calls to open() against other open() and a custom
ioctl() returning the bcdDevice (sic!), but neither is needed.
Signed-off-by: Johan Hovold
---
drivers/usb/misc/usblcd.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/usb/misc/usblcd.c b/drivers/usb/misc/usblcd.c
index b898650a5570..732eb1f81368 100644
--- a/drivers/usb/misc/usblcd.c
+++ b/drivers/usb/misc/usblcd.c
@@ -30,7 +30,6 @@
#define IOCTL_GET_DRV_VERSION 2
-static DEFINE_MUTEX(lcd_mutex);
static const struct usb_device_id id_table[] = {
{ .idVendor = 0x10D2, .match_flags = USB_DEVICE_ID_MATCH_VENDOR, },
{ },
@@ -81,12 +80,10 @@ static int lcd_open(struct inode *inode, struct file *file)
struct usb_interface *interface;
int subminor, r;
- mutex_lock(&lcd_mutex);
subminor = iminor(inode);
interface = usb_find_interface(&lcd_driver, subminor);
if (!interface) {
- mutex_unlock(&lcd_mutex);
printk(KERN_ERR "USBLCD: %s - error, can't find device for
minor %d\n",
__func__, subminor);
return -ENODEV;
@@ -101,13 +98,11 @@ static int lcd_open(struct inode *inode, struct file *file)
r = usb_autopm_get_interface(interface);
if (r < 0) {
kref_put(&dev->kref, lcd_delete);
- mutex_unlock(&lcd_mutex);
return r;
}
/* save our object in the file's private structure */
file->private_data = dev;
- mutex_unlock(&lcd_mutex);
return 0;
}
@@ -176,14 +171,12 @@ static long lcd_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
switch (cmd) {
case IOCTL_GET_HARD_VERSION:
- mutex_lock(&lcd_mutex);
bcdDevice = le16_to_cpu((dev->udev)->descriptor.bcdDevice);
sprintf(buf, "%1d%1d.%1d%1d",
(bcdDevice & 0xF000)>>12,
(bcdDevice & 0xF00)>>8,
(bcdDevice & 0xF0)>>4,
(bcdDevice & 0xF));
- mutex_unlock(&lcd_mutex);
if (copy_to_user((void __user *)arg, buf, strlen(buf)) != 0)
return -EFAULT;
break;
--
2.23.0
[PATCH 4/4] USB: usblcd: use pr_err()
Replace the one remaining printk with pr_err().
Signed-off-by: Johan Hovold
---
drivers/usb/misc/usblcd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/misc/usblcd.c b/drivers/usb/misc/usblcd.c
index 732eb1f81368..61e9e987fe4a 100644
--- a/drivers/usb/misc/usblcd.c
+++ b/drivers/usb/misc/usblcd.c
@@ -84,7 +84,7 @@ static int lcd_open(struct inode *inode, struct file *file)
interface = usb_find_interface(&lcd_driver, subminor);
if (!interface) {
- printk(KERN_ERR "USBLCD: %s - error, can't find device for
minor %d\n",
+ pr_err("USBLCD: %s - error, can't find device for minor %d\n",
__func__, subminor);
return -ENODEV;
}
--
2.23.0
[PATCH 0/4] USB: usblcd: disconnect fix and locking clean ups
This series fixes a failure to stop I/O on disconnect() in the usblcd driver. Turns out there was a lot of legacy cruft in this driver which could simply be removed. The first patch is marked for stable and could go into v5.4 while the rest is v5.5 material. Posting all at once for completeness. I was tempted to rip out the custom ioctls() used to retrieve the driver version and bcdDevice (sic!), but decided to leave them in. I doubt anyone would miss them though so perhaps we should give it a go? Tested using a mockup device. Johan Johan Hovold (4): USB: usblcd: fix I/O after disconnect USB: usblcd: drop redundant disconnect mutex USB: usblcd: drop redundant lcd mutex USB: usblcd: use pr_err() drivers/usb/misc/usblcd.c | 60 +-- 1 file changed, 33 insertions(+), 27 deletions(-) -- 2.23.0
[PATCH 3/4] USB: usblcd: drop redundant lcd mutex
Drop the redundant lcd mutex introduced by commit 925ce689bb31 ("USB:
autoconvert trivial BKL users to private mutex") which replaced an
earlier BKL use.
The lock serialised calls to open() against other open() and a custom
ioctl() returning the bcdDevice (sic!), but neither is needed.
Signed-off-by: Johan Hovold
---
drivers/usb/misc/usblcd.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/usb/misc/usblcd.c b/drivers/usb/misc/usblcd.c
index b898650a5570..732eb1f81368 100644
--- a/drivers/usb/misc/usblcd.c
+++ b/drivers/usb/misc/usblcd.c
@@ -30,7 +30,6 @@
#define IOCTL_GET_DRV_VERSION 2
-static DEFINE_MUTEX(lcd_mutex);
static const struct usb_device_id id_table[] = {
{ .idVendor = 0x10D2, .match_flags = USB_DEVICE_ID_MATCH_VENDOR, },
{ },
@@ -81,12 +80,10 @@ static int lcd_open(struct inode *inode, struct file *file)
struct usb_interface *interface;
int subminor, r;
- mutex_lock(&lcd_mutex);
subminor = iminor(inode);
interface = usb_find_interface(&lcd_driver, subminor);
if (!interface) {
- mutex_unlock(&lcd_mutex);
printk(KERN_ERR "USBLCD: %s - error, can't find device for
minor %d\n",
__func__, subminor);
return -ENODEV;
@@ -101,13 +98,11 @@ static int lcd_open(struct inode *inode, struct file *file)
r = usb_autopm_get_interface(interface);
if (r < 0) {
kref_put(&dev->kref, lcd_delete);
- mutex_unlock(&lcd_mutex);
return r;
}
/* save our object in the file's private structure */
file->private_data = dev;
- mutex_unlock(&lcd_mutex);
return 0;
}
@@ -176,14 +171,12 @@ static long lcd_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
switch (cmd) {
case IOCTL_GET_HARD_VERSION:
- mutex_lock(&lcd_mutex);
bcdDevice = le16_to_cpu((dev->udev)->descriptor.bcdDevice);
sprintf(buf, "%1d%1d.%1d%1d",
(bcdDevice & 0xF000)>>12,
(bcdDevice & 0xF00)>>8,
(bcdDevice & 0xF0)>>4,
(bcdDevice & 0xF));
- mutex_unlock(&lcd_mutex);
if (copy_to_user((void __user *)arg, buf, strlen(buf)) != 0)
return -EFAULT;
break;
--
2.23.0
[PATCH 0/4] USB: usblcd: disconnect fix and locking clean ups
This series fixes a failure to stop I/O on disconnect() in the usblcd driver. Turns out there was a lot of legacy cruft in this driver which could simply be removed. The first patch is marked for stable and could go into v5.4 while the rest is v5.5 material. Posting all at once for completeness. I was tempted to rip out the custom ioctls() used to retrieve the driver version and bcdDevice (sic!), but decided to leave them in. I doubt anyone would miss them though so perhaps we should give it a go? Tested using a mockup device. Johan Johan Hovold (4): USB: usblcd: fix I/O after disconnect USB: usblcd: drop redundant disconnect mutex USB: usblcd: drop redundant lcd mutex USB: usblcd: use pr_err() drivers/usb/misc/usblcd.c | 60 +-- 1 file changed, 33 insertions(+), 27 deletions(-) -- 2.23.0
[PATCH 2/4] USB: usblcd: drop redundant disconnect mutex
Drop the redundant disconnect mutex which was introduced after the
open-disconnect race had been addressed generally in USB core by commit
d4ead16f50f9 ("USB: prevent char device open/deregister race").
Specifically, the rw-semaphore in core guarantees that all calls to
open() will have completed and that no new calls to open() will occur
after usb_deregister_dev() returns. Hence there is no need use the
driver data as an inverted disconnected flag.
Signed-off-by: Johan Hovold
---
drivers/usb/misc/usblcd.c | 18 +-
1 file changed, 1 insertion(+), 17 deletions(-)
diff --git a/drivers/usb/misc/usblcd.c b/drivers/usb/misc/usblcd.c
index aa982d3ca36b..b898650a5570 100644
--- a/drivers/usb/misc/usblcd.c
+++ b/drivers/usb/misc/usblcd.c
@@ -37,9 +37,6 @@ static const struct usb_device_id id_table[] = {
};
MODULE_DEVICE_TABLE(usb, id_table);
-static DEFINE_MUTEX(open_disc_mutex);
-
-
struct usb_lcd {
struct usb_device *udev; /* init: probe_lcd */
struct usb_interface*interface; /* the interface for
@@ -95,17 +92,10 @@ static int lcd_open(struct inode *inode, struct file *file)
return -ENODEV;
}
- mutex_lock(&open_disc_mutex);
dev = usb_get_intfdata(interface);
- if (!dev) {
- mutex_unlock(&open_disc_mutex);
- mutex_unlock(&lcd_mutex);
- return -ENODEV;
- }
/* increment our usage count for the device */
kref_get(&dev->kref);
- mutex_unlock(&open_disc_mutex);
/* grab a power reference */
r = usb_autopm_get_interface(interface);
@@ -388,7 +378,6 @@ static int lcd_probe(struct usb_interface *interface,
/* something prevented us from registering this driver */
dev_err(&interface->dev,
"Not able to get a minor for this device.\n");
- usb_set_intfdata(interface, NULL);
goto error;
}
@@ -434,14 +423,9 @@ static int lcd_resume(struct usb_interface *intf)
static void lcd_disconnect(struct usb_interface *interface)
{
- struct usb_lcd *dev;
+ struct usb_lcd *dev = usb_get_intfdata(interface);
int minor = interface->minor;
- mutex_lock(&open_disc_mutex);
- dev = usb_get_intfdata(interface);
- usb_set_intfdata(interface, NULL);
- mutex_unlock(&open_disc_mutex);
-
/* give back our minor */
usb_deregister_dev(interface, &lcd_class);
--
2.23.0
[PATCH 2/4] USB: usblcd: drop redundant disconnect mutex
Drop the redundant disconnect mutex which was introduced after the
open-disconnect race had been addressed generally in USB core by commit
d4ead16f50f9 ("USB: prevent char device open/deregister race").
Specifically, the rw-semaphore in core guarantees that all calls to
open() will have completed and that no new calls to open() will occur
after usb_deregister_dev() returns. Hence there is no need use the
driver data as an inverted disconnected flag.
Signed-off-by: Johan Hovold
---
drivers/usb/misc/usblcd.c | 18 +-
1 file changed, 1 insertion(+), 17 deletions(-)
diff --git a/drivers/usb/misc/usblcd.c b/drivers/usb/misc/usblcd.c
index aa982d3ca36b..b898650a5570 100644
--- a/drivers/usb/misc/usblcd.c
+++ b/drivers/usb/misc/usblcd.c
@@ -37,9 +37,6 @@ static const struct usb_device_id id_table[] = {
};
MODULE_DEVICE_TABLE(usb, id_table);
-static DEFINE_MUTEX(open_disc_mutex);
-
-
struct usb_lcd {
struct usb_device *udev; /* init: probe_lcd */
struct usb_interface*interface; /* the interface for
@@ -95,17 +92,10 @@ static int lcd_open(struct inode *inode, struct file *file)
return -ENODEV;
}
- mutex_lock(&open_disc_mutex);
dev = usb_get_intfdata(interface);
- if (!dev) {
- mutex_unlock(&open_disc_mutex);
- mutex_unlock(&lcd_mutex);
- return -ENODEV;
- }
/* increment our usage count for the device */
kref_get(&dev->kref);
- mutex_unlock(&open_disc_mutex);
/* grab a power reference */
r = usb_autopm_get_interface(interface);
@@ -388,7 +378,6 @@ static int lcd_probe(struct usb_interface *interface,
/* something prevented us from registering this driver */
dev_err(&interface->dev,
"Not able to get a minor for this device.\n");
- usb_set_intfdata(interface, NULL);
goto error;
}
@@ -434,14 +423,9 @@ static int lcd_resume(struct usb_interface *intf)
static void lcd_disconnect(struct usb_interface *interface)
{
- struct usb_lcd *dev;
+ struct usb_lcd *dev = usb_get_intfdata(interface);
int minor = interface->minor;
- mutex_lock(&open_disc_mutex);
- dev = usb_get_intfdata(interface);
- usb_set_intfdata(interface, NULL);
- mutex_unlock(&open_disc_mutex);
-
/* give back our minor */
usb_deregister_dev(interface, &lcd_class);
--
2.23.0
[PATCH 4/4] USB: usblcd: use pr_err()
Replace the one remaining printk with pr_err().
Signed-off-by: Johan Hovold
---
drivers/usb/misc/usblcd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/misc/usblcd.c b/drivers/usb/misc/usblcd.c
index 732eb1f81368..61e9e987fe4a 100644
--- a/drivers/usb/misc/usblcd.c
+++ b/drivers/usb/misc/usblcd.c
@@ -84,7 +84,7 @@ static int lcd_open(struct inode *inode, struct file *file)
interface = usb_find_interface(&lcd_driver, subminor);
if (!interface) {
- printk(KERN_ERR "USBLCD: %s - error, can't find device for
minor %d\n",
+ pr_err("USBLCD: %s - error, can't find device for minor %d\n",
__func__, subminor);
return -ENODEV;
}
--
2.23.0
Re: [PATCH] [PATCH v7] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)
On Wed, Sep 25, 2019 at 05:36:23PM +0800, Charles Yeh wrote: > Johan Hovold 於 2019年9月25日 週三 下午4:06寫道： > > Meanwhile you can double check that you've considered all > > review-feedback you've gotten so far. > > > > I don't think you ever replied to my last comment about the reset > > register and why I thought using plain write (not read, mask, write) was > > the right thing to do. > > > > Does the register always read back as 0, or does it read back as the > > last value written? > > Charles Ans: > I just asked my colleague, who is RD of design PL2303 hardware, > His answer is to read 0 forever. > > Does the register always read back as 0, or does it read back as the > last value written? > Ans: Yes, the register"#define PL2303_HXN_RESET_REG 0x07" always > read back as 0. > > I hope the above content has an answer to your question: > If there are other questions, please try to raise it.. thanks It does, thanks for confirming. Johan
[PATCH 2/2] USB: adutux: fix NULL-derefs on disconnect
The driver was using its struct usb_device pointer as an inverted
disconnected flag, but was setting it to NULL before making sure all
completion handlers had run. This could lead to a NULL-pointer
dereference in a number of dev_dbg statements in the completion handlers
which relies on said pointer.
The pointer was also dereferenced unconditionally in a dev_dbg statement
release() something which would lead to a NULL-deref whenever a device
was disconnected before the final character-device close if debugging
was enabled.
Fix this by unconditionally stopping all I/O and preventing
resubmissions by poisoning the interrupt URBs at disconnect and using a
dedicated disconnected flag.
This also makes sure that all I/O has completed by the time the
disconnect callback returns.
Fixes: 1ef37c6047fe ("USB: adutux: remove custom debug macro and module
parameter")
Fixes: 66d4bc30d128 ("USB: adutux: remove custom debug macro")
Cc: stable # 3.12
Signed-off-by: Johan Hovold
---
drivers/usb/misc/adutux.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index bcc138990e2f..f9efec719359 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -75,6 +75,7 @@ struct adu_device {
charserial_number[8];
int open_count; /* number of times this port has
been opened */
+ unsigned long disconnected:1;
char*read_buffer_primary;
int read_buffer_length;
@@ -116,7 +117,7 @@ static void adu_abort_transfers(struct adu_device *dev)
{
unsigned long flags;
- if (dev->udev == NULL)
+ if (dev->disconnected)
return;
/* shutdown transfer */
@@ -243,7 +244,7 @@ static int adu_open(struct inode *inode, struct file *file)
}
dev = usb_get_intfdata(interface);
- if (!dev || !dev->udev) {
+ if (!dev) {
retval = -ENODEV;
goto exit_no_device;
}
@@ -326,7 +327,7 @@ static int adu_release(struct inode *inode, struct file
*file)
}
adu_release_internal(dev);
- if (dev->udev == NULL) {
+ if (dev->disconnected) {
/* the device was unplugged before the file was released */
if (!dev->open_count) /* ... and we're the last user */
adu_delete(dev);
@@ -354,7 +355,7 @@ static ssize_t adu_read(struct file *file, __user char
*buffer, size_t count,
return -ERESTARTSYS;
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL) {
+ if (dev->disconnected) {
retval = -ENODEV;
pr_err("No device or device unplugged %d\n", retval);
goto exit;
@@ -518,7 +519,7 @@ static ssize_t adu_write(struct file *file, const __user
char *buffer,
goto exit_nolock;
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL) {
+ if (dev->disconnected) {
retval = -ENODEV;
pr_err("No device or device unplugged %d\n", retval);
goto exit;
@@ -764,11 +765,14 @@ static void adu_disconnect(struct usb_interface
*interface)
usb_deregister_dev(interface, &adu_class);
+ usb_poison_urb(dev->interrupt_in_urb);
+ usb_poison_urb(dev->interrupt_out_urb);
+
mutex_lock(&adutux_mutex);
usb_set_intfdata(interface, NULL);
mutex_lock(&dev->mtx); /* not interruptible */
- dev->udev = NULL; /* poison */
+ dev->disconnected = 1;
mutex_unlock(&dev->mtx);
/* if the device is not opened, then we clean up right now */
--
2.23.0
[PATCH 1/2] USB: adutux: fix use-after-free on disconnect
The driver was clearing its struct usb_device pointer, which it used as
an inverted disconnected flag, before deregistering the character device
and without serialising against racing release().
This could lead to a use-after-free if a racing release() callback
observes the cleared pointer and frees the driver data before
disconnect() is finished with it.
This could also lead to NULL-pointer dereferences in a racing open().
Fixes: f08812d5eb8f ("USB: FIx locks and urb->status in adutux (updated)")
Cc: stable # 2.6.24
Reported-by: syzbot+0243cb250a51eeefb...@syzkaller.appspotmail.com
Tested-by: syzbot+0243cb250a51eeefb...@syzkaller.appspotmail.com
Signed-off-by: Johan Hovold
---
drivers/usb/misc/adutux.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index 344d523b0502..bcc138990e2f 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -762,14 +762,15 @@ static void adu_disconnect(struct usb_interface
*interface)
dev = usb_get_intfdata(interface);
- mutex_lock(&dev->mtx); /* not interruptible */
- dev->udev = NULL; /* poison */
usb_deregister_dev(interface, &adu_class);
- mutex_unlock(&dev->mtx);
mutex_lock(&adutux_mutex);
usb_set_intfdata(interface, NULL);
+ mutex_lock(&dev->mtx); /* not interruptible */
+ dev->udev = NULL; /* poison */
+ mutex_unlock(&dev->mtx);
+
/* if the device is not opened, then we clean up right now */
if (!dev->open_count)
adu_delete(dev);
--
2.23.0
Re: [PATCH] [PATCH v7] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)
On Wed, Sep 25, 2019 at 09:20:07AM +0800, Charles Yeh wrote: > Johan Hovold 於 2019年9月23日 週一 下午9:08寫道： > > > > Yes, the above looks good. > > > > Thank you for your reply > > I have already written a new patch[v8] file, > if you have free time. Please check as soon as possible...thanks! I'll start processing patches for 5.4 next week when the merge window closes. Meanwhile you can double check that you've considered all review-feedback you've gotten so far. I don't think you ever replied to my last comment about the reset register and why I thought using plain write (not read, mask, write) was the right thing to do. Does the register always read back as 0, or does it read back as the last value written? Johan
Re: [PATCH] [PATCH v7] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)
On Mon, Sep 23, 2019 at 06:35:19PM +0800, Charles Yeh wrote: > Johan Hovold 於 2019年9月23日 週一 下午6:24寫道： > > That looks much better. But please move the reset defines above the > > flow control ones to keep the registers sorted by address (0x7 < 0xa). > > Thank you for your reply > > Charles Ans: > The new define is follows > > #define PL2303_READ_TYPE_HX_STATUS0x8080 > > #define PL2303_HXN_RESET_REG0x07 > #define PL2303_HXN_RESET_UPSTREAM_PIPE0x02 > #define PL2303_HXN_RESET_DOWNSTREAM_PIPE0x01 > > #define PL2303_HXN_FLOWCTRL_REG0x0A > #define PL2303_HXN_FLOWCTRL_MASK0x1C > #define PL2303_HXN_FLOWCTRL_NONE0x1C > #define PL2303_HXN_FLOWCTRL_RTS_CTS0x18 > #define PL2303_HXN_FLOWCTRL_XON_XOFF0x0C > > > > Also looks good, thanks. Just move the reset define block as mentioned > > above. > > Thank you for your reply > > > Please confirm the above new define > If there is no problem.. I will write a new Patch file. Yes, the above looks good. Johan
Re: [PATCH] [PATCH v7] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)
On Mon, Sep 23, 2019 at 05:53:34PM +0800, Charles Yeh wrote: > Johan Hovold 於 2019年9月20日 週五 下午3:56寫道： > > Yes, that's better, but you're mixing register addresses, bit values and > > masks above. Perhaps things would be more clear if you but a _REG suffix > > on the register defines and order things as follows: > > > > #define PL2303_HXN__REG 0xX1 > > #define PL2303_HXN___MASK 0xY1 > > #define PL2303_HXN___ 0xZ1 > > > > #define PL2303_HXN__REG 0xX2 > > #define PL2303_HXN___MASK 0xY2 > > #define PL2303_HXN___ 0xZ2 > > > > The idea is simply to keep related defines together and not mix > > register address, masks and value defines. > > > > Keep registers sorted by address, and bit masks and values by bit order > > (e.g. MSB first). > > Thank you for your reply > > Charles Ans: > The new define is follows > > #define PL2303_READ_TYPE_HX_STATUS0x8080 > > #define PL2303_HXN_FLOWCTRL_REG0x0A > #define PL2303_HXN_FLOWCTRL_MASK0x1C > #define PL2303_HXN_FLOWCTRL_NONE0x1C > #define PL2303_HXN_FLOWCTRL_RTS_CTS0x18 > #define PL2303_HXN_FLOWCTRL_XON_XOFF0x0C > > #define PL2303_HXN_RESET_REG0x07 > #define PL2303_HXN_RESET_UPSTREAM_PIPE0x02 > #define PL2303_HXN_RESET_DOWNSTREAM_PIPE0x01 That looks much better. But please move the reset defines above the flow control ones to keep the registers sorted by address (0x7 < 0xa). > > Yes, but that doesn't imply that you need to read back the old value. > > > > I'm assuming it would either always read back as 0, or you would read > > back the previous value written, which means you could end up resetting > > something you did not intend. > > > > Either way, you should not read back the current value when resetting > > the data pipes. > > > > Thank you for your reply > > Charles Ans: > The new code is follows > > pl2303_vendor_write(serial, > PL2303_HXN_RESET_REG, > PL2303_HXN_RESET_UPSTREAM_PIPE | > PL2303_HXN_RESET_DOWNSTREAM_PIPE); > > > Please confirm the above new define & code.. > If there is no problem.. I will write a new Patch file. Also looks good, thanks. Just move the reset define block as mentioned above. Johan
Re: Linux Keyspan USB serial driver ignoring XOFF
On Fri, Sep 20, 2019 at 02:49:51PM -0500, John Goerzen wrote: > Hello, > > I have narrowed down the issue I'm about to describe to keyspan.c; a > Digi Edgeport/1 with identical configuration works fine. > > I am configuring a Raspberry Pi running 4.19.66 (though keyspan.c hasn't > changed since 2017) to talk to a real-live vt420. Configuring agetty > with systemd worked easy enough, but I found that XON/XOFF wasn't > working. stty -a shows ixon and ixoff as appropriate, but sending > Ctrl-S (tested from multiple ways of sending) had no effect on output in > bash, or scrolling output. (Emacs, though, recognized it as the start > of a search, so I knew it was getting down the line.) > > > After a great deal of head-scratching on this, I went to look at the > kernel source and found that keyspan.c does not appear to be honoring > XOFF. I also have a Digi Edgeport/1 on hand (which uses io_ti.c), and > when I swapped to that, everything worked fine - Ctrl-S caused the > expected pause. > > As far as I can tell, keyspan.c simply never implemented handling of > XOFF, but you guys are the experts there. That's correct, the driver does not support software flow control even if the hardware seems to have some support for assisted XON/XOFF. Would you mind testing the below patch which may be enough to turn sw-flow control always on. If that works, I can probably find time to cook up a proper patch to make this configurable later this week. Johan >From 55b46d78fe63f182923e4674659fa18f4624d6b8 Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Mon, 23 Sep 2019 12:14:56 +0200 Subject: [PATCH] USB: serial: keyspan: enable XON flow control Signed-off-by: Johan Hovold --- drivers/usb/serial/keyspan.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/usb/serial/keyspan.c b/drivers/usb/serial/keyspan.c index d34779fe4a8d..934430cbdfc4 100644 --- a/drivers/usb/serial/keyspan.c +++ b/drivers/usb/serial/keyspan.c @@ -2118,7 +2118,7 @@ static int keyspan_usa26_send_setup(struct usb_serial *serial, msg.setLcr = 0xff; msg.ctsFlowControl = (p_priv->flow_control == flow_cts); - msg.xonFlowControl = 0; + msg.xonFlowControl = 1; msg.setFlowControl = 0xff; msg.forwardingLength = 16; msg.xonChar = 17; @@ -2234,7 +2234,7 @@ static int keyspan_usa28_send_setup(struct usb_serial *serial, msg.parity = 0; /* XXX for now */ msg.ctsFlowControl = (p_priv->flow_control == flow_cts); - msg.xonFlowControl = 0; + msg.xonFlowControl = 1; /* Do handshaking outputs, DTR is inverted relative to RTS */ msg.rts = p_priv->rts_state; @@ -2388,7 +2388,7 @@ static int keyspan_usa49_send_setup(struct usb_serial *serial, msg.setLcr = 0xff; msg.ctsFlowControl = (p_priv->flow_control == flow_cts); - msg.xonFlowControl = 0; + msg.xonFlowControl = 1; msg.setFlowControl = 0xff; msg.forwardingLength = 16; @@ -2690,7 +2690,7 @@ static int keyspan_usa67_send_setup(struct usb_serial *serial, msg.setLcr = 0xff; msg.ctsFlowControl = (p_priv->flow_control == flow_cts); - msg.xonFlowControl = 0; + msg.xonFlowControl = 1; msg.setFlowControl = 0xff; msg.forwardingLength = 16; msg.xonChar = 17; -- 2.23.0
Re: [PATCH] [PATCH v7] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)
On Tue, Aug 27, 2019 at 04:40:39PM +0800, Charles Yeh wrote: > Johan Hovold 於 2019年7月16日 週二 下午4:49寫道： > > > #define PL2303_FLOWCTRL_MASK 0xf0 > > > +#define PL2303_HXN_FLOWCTRL_MASK 0x1C > > > +#define PL2303_HXN_FLOWCTRL 0x0A > > > > I asked you to keep related defines together (and to move the mask where > > the register define was, not the other way round). Please move these to > > the other HXN defines below, and keep the register address defines > > before the corresponding bit defines. > > Charles Ans: [ You don't need to prefix your replies like this, I can tell from the number of > signs. ] > I am not 100% sure what you mean, please see if it is defined below > > #define PL2303_FLOWCTRL_MASK0xf0 > > #define PL2303_READ_TYPE_HX_STATUS0x8080 > > #define PL2303_HXN_CTRL_XON_XOFF0x0C > #define PL2303_HXN_CTRL_RTS_CTS0x18 > #define PL2303_HXN_CTRL_NONE0x1C > #define PL2303_HXN_FLOWCTRL_MASK0x1C > #define PL2303_HXN_FLOWCTRL0x0A Yes, that's better, but you're mixing register addresses, bit values and masks above. Perhaps things would be more clear if you but a _REG suffix on the register defines and order things as follows: #define PL2303_HXN__REG 0xX1 #define PL2303_HXN___MASK 0xY1 #define PL2303_HXN___ 0xZ1 #define PL2303_HXN__REG 0xX2 #define PL2303_HXN___MASK 0xY2 #define PL2303_HXN___ 0xZ2 The idea is simply to keep related defines together and not mix register address, masks and value defines. Keep registers sorted by address, and bit masks and values by bit order (e.g. MSB first). > #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE0x03 > #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE_MASK0x03 > #define PL2303_HXN_RESET_CONTROL0x07 > > > > + > > > +#define PL2303_HXN_RESET_CONTROL_MASK0x03 > > This makes no sense. The whole register is used for reset. If you want a > > define that can be used for resetting both pipes then add two separate > > defines for up and down respectively, and add a third define for > > resetting both buffer as a bitwise OR of the two. > > Charles Ans: > Yes,The whole register is used for reset. > Bit 0 and bit 1 are used for up & downstream data pipe, > Bit 2 for interface reset > Bit 4 for chip reset. > > But I only reset bit 0 & bit 1. Yes, but you need to reflect that in how you name your defines. Add two separate defines for up and downstream data pipe reset. If you want you add a third as the bitwise-OR of the two as well (perhaps with a _BOTH suffix in the name). > > Also move this one after the corresponding register address define > > below. > > > > > +#define PL2303_HXN_RESET_CONTROL 0x07 > > > +#define PL2303_HXN_CTRL_XON_XOFF 0x0C > > > +#define PL2303_HXN_CTRL_RTS_CTS 0x18 > > > +#define PL2303_HXN_CTRL_NONE 0x1C > > Charles Ans: > I am not 100% sure what you mean, please see if it is defined below > > #define PL2303_FLOWCTRL_MASK0xf0 > > #define PL2303_READ_TYPE_HX_STATUS0x8080 > > #define PL2303_HXN_CTRL_XON_XOFF0x0C > #define PL2303_HXN_CTRL_RTS_CTS0x18 > #define PL2303_HXN_CTRL_NONE0x1C > #define PL2303_HXN_FLOWCTRL_MASK0x1C > #define PL2303_HXN_FLOWCTRL0x0A > > #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE0x03 > #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE_MASK0x03 > #define PL2303_HXN_RESET_CONTROL0x07 I meant that you should move the bit values (masks) after the register address that they apply to (as also mentioned above). For example, #define PL2303_HXN_RESET_REG0x07 #define PL2303_HXN_RESET_UPSTREAM_PIPE 0x02 #define PL2303_HXN_RESET_DOWNSTREAM_PIPE0x01 > > > + pl2303_update_reg(serial, PL2303_HXN_RESET_CONTROL, > > > + PL2303_HXN_RESET_CONTROL_MASK, 0x03); > > > > So two things; first, do you really need to read back the current value? > > I would assume that it always reads back as 0 and that writing 0x03 in > > this case would be sufficient to reset both buffers. > > > > Charles Ans: > Yes, I want to read back the current value. > because the whole register is used for reset. > Bit 0 and bit 1 are used for up & downstream data pipe, > Bit 2 for interface reset > Bit 4 for chip reset. > > But I only reset bit 0 & bit 1. Yes, but that doesn't imply that you need to read back the old value. I'm assuming it would eithe
[PATCH RESEND 2/4] USB: legousbtower: fix deadlock on disconnect
Fix a potential deadlock if disconnect races with open.
Since commit d4ead16f50f9 ("USB: prevent char device open/deregister
race") core holds an rw-semaphore while open is called and when
releasing the minor number during deregistration. This can lead to an
ABBA deadlock if a driver takes a lock in open which it also holds
during deregistration.
This effectively reverts commit 78663ecc344b ("USB: disconnect open race
in legousbtower") which needlessly introduced this issue after a generic
fix for this race had been added to core by commit d4ead16f50f9 ("USB:
prevent char device open/deregister race").
Fixes: 78663ecc344b ("USB: disconnect open race in legousbtower")
Cc: stable # 2.6.24
Reported-by: syzbot+f9549f5ee8a5416f0...@syzkaller.appspotmail.com
Tested-by: syzbot+f9549f5ee8a5416f0...@syzkaller.appspotmail.com
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 19 ++-
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 1db07d4dc738..773e4188f336 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -179,7 +179,6 @@ static const struct usb_device_id tower_table[] = {
};
MODULE_DEVICE_TABLE (usb, tower_table);
-static DEFINE_MUTEX(open_disc_mutex);
#define LEGO_USB_TOWER_MINOR_BASE 160
@@ -332,18 +331,14 @@ static int tower_open (struct inode *inode, struct file
*file)
goto exit;
}
- mutex_lock(&open_disc_mutex);
dev = usb_get_intfdata(interface);
-
if (!dev) {
- mutex_unlock(&open_disc_mutex);
retval = -ENODEV;
goto exit;
}
/* lock this device */
if (mutex_lock_interruptible(&dev->lock)) {
- mutex_unlock(&open_disc_mutex);
retval = -ERESTARTSYS;
goto exit;
}
@@ -351,12 +346,10 @@ static int tower_open (struct inode *inode, struct file
*file)
/* allow opening only once */
if (dev->open_count) {
- mutex_unlock(&open_disc_mutex);
retval = -EBUSY;
goto unlock_exit;
}
dev->open_count = 1;
- mutex_unlock(&open_disc_mutex);
/* reset the tower */
result = usb_control_msg (dev->udev,
@@ -423,10 +416,9 @@ static int tower_release (struct inode *inode, struct file
*file)
if (dev == NULL) {
retval = -ENODEV;
- goto exit_nolock;
+ goto exit;
}
- mutex_lock(&open_disc_mutex);
if (mutex_lock_interruptible(&dev->lock)) {
retval = -ERESTARTSYS;
goto exit;
@@ -456,10 +448,7 @@ static int tower_release (struct inode *inode, struct file
*file)
unlock_exit:
mutex_unlock(&dev->lock);
-
exit:
- mutex_unlock(&open_disc_mutex);
-exit_nolock:
return retval;
}
@@ -912,7 +901,6 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
if (retval) {
/* something prevented us from registering this driver */
dev_err(idev, "Not able to get a minor for this device.\n");
- usb_set_intfdata (interface, NULL);
goto error;
}
dev->minor = interface->minor;
@@ -944,16 +932,13 @@ static void tower_disconnect (struct usb_interface
*interface)
int minor;
dev = usb_get_intfdata (interface);
- mutex_lock(&open_disc_mutex);
- usb_set_intfdata (interface, NULL);
minor = dev->minor;
- /* give back our minor */
+ /* give back our minor and prevent further open() */
usb_deregister_dev (interface, &tower_class);
mutex_lock(&dev->lock);
- mutex_unlock(&open_disc_mutex);
/* if the device is not opened, then we clean up right now */
if (!dev->open_count) {
--
2.23.0
[PATCH RESEND 3/4] USB: legousbtower: fix potential NULL-deref on disconnect
The driver is using its struct usb_device pointer as an inverted
disconnected flag, but was setting it to NULL before making sure all
completion handlers had run. This could lead to a NULL-pointer
dereference in a number of dev_dbg and dev_err statements in the
completion handlers which relies on said pointer.
Fix this by unconditionally stopping all I/O and preventing
resubmissions by poisoning the interrupt URBs at disconnect and using a
dedicated disconnected flag.
This also makes sure that all I/O has completed by the time the
disconnect callback returns.
Fixes: 9d974b2a06e3 ("USB: legousbtower.c: remove err() usage")
Fixes: fef526cae700 ("USB: legousbtower: remove custom debug macro")
Fixes: 4dae99638097 ("USB: legotower: remove custom debug macro and module
parameter")
Cc: stable # 3.5
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 26 +++---
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 773e4188f336..4fa999882635 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -190,6 +190,7 @@ struct lego_usb_tower {
unsigned char minor; /* the starting minor number
for this device */
int open_count; /* number of times this port
has been opened */
+ unsigned long disconnected:1;
char* read_buffer;
size_t read_buffer_length; /* this much came in */
@@ -289,8 +290,6 @@ static inline void lego_usb_tower_debug_data(struct device
*dev,
*/
static inline void tower_delete (struct lego_usb_tower *dev)
{
- tower_abort_transfers (dev);
-
/* free data structures */
usb_free_urb(dev->interrupt_in_urb);
usb_free_urb(dev->interrupt_out_urb);
@@ -430,7 +429,8 @@ static int tower_release (struct inode *inode, struct file
*file)
retval = -ENODEV;
goto unlock_exit;
}
- if (dev->udev == NULL) {
+
+ if (dev->disconnected) {
/* the device was unplugged before the file was released */
/* unlock here as tower_delete frees dev */
@@ -466,10 +466,9 @@ static void tower_abort_transfers (struct lego_usb_tower
*dev)
if (dev->interrupt_in_running) {
dev->interrupt_in_running = 0;
mb();
- if (dev->udev)
- usb_kill_urb (dev->interrupt_in_urb);
+ usb_kill_urb(dev->interrupt_in_urb);
}
- if (dev->interrupt_out_busy && dev->udev)
+ if (dev->interrupt_out_busy)
usb_kill_urb(dev->interrupt_out_urb);
}
@@ -505,7 +504,7 @@ static __poll_t tower_poll (struct file *file, poll_table
*wait)
dev = file->private_data;
- if (!dev->udev)
+ if (dev->disconnected)
return EPOLLERR | EPOLLHUP;
poll_wait(file, &dev->read_wait, wait);
@@ -552,7 +551,7 @@ static ssize_t tower_read (struct file *file, char __user
*buffer, size_t count,
}
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL) {
+ if (dev->disconnected) {
retval = -ENODEV;
pr_err("No device or device unplugged %d\n", retval);
goto unlock_exit;
@@ -638,7 +637,7 @@ static ssize_t tower_write (struct file *file, const char
__user *buffer, size_t
}
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL) {
+ if (dev->disconnected) {
retval = -ENODEV;
pr_err("No device or device unplugged %d\n", retval);
goto unlock_exit;
@@ -748,7 +747,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
resubmit:
/* resubmit if we're still running */
- if (dev->interrupt_in_running && dev->udev) {
+ if (dev->interrupt_in_running) {
retval = usb_submit_urb (dev->interrupt_in_urb, GFP_ATOMIC);
if (retval)
dev_err(&dev->udev->dev,
@@ -813,6 +812,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
dev->udev = udev;
dev->open_count = 0;
+ dev->disconnected = 0;
dev->read_buffer = NULL;
dev->read_buffer_length = 0;
@@ -938,6 +938,10 @@ static void tower_disconnect (struct usb_interface
*interface)
/* give back our minor and prevent further open() */
usb_deregister_dev (interface, &tower_class);
+ /* stop I/O */
+ usb_poison_urb(dev->interrupt_in_urb);
+ usb_poison_urb(dev->interrupt_out_urb);
+
mutex_lock(&dev->
[PATCH RESEND 4/4] USB: legousbtower: fix open after failed reset request
The driver would return with a nonzero open count in case the reset
control request failed. This would prevent any further attempts to open
the char dev until the device was disconnected.
Fix this by incrementing the open count only on successful open.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 4fa999882635..44d6a3381804 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -348,7 +348,6 @@ static int tower_open (struct inode *inode, struct file
*file)
retval = -EBUSY;
goto unlock_exit;
}
- dev->open_count = 1;
/* reset the tower */
result = usb_control_msg (dev->udev,
@@ -388,13 +387,14 @@ static int tower_open (struct inode *inode, struct file
*file)
dev_err(&dev->udev->dev,
"Couldn't submit interrupt_in_urb %d\n", retval);
dev->interrupt_in_running = 0;
- dev->open_count = 0;
goto unlock_exit;
}
/* save device in the file's private structure */
file->private_data = dev;
+ dev->open_count = 1;
+
unlock_exit:
mutex_unlock(&dev->lock);
--
2.23.0
[PATCH RESEND 1/4] USB: legousbtower: fix slab info leak at probe
Make sure to check for short transfers when retrieving the version
information at probe to avoid leaking uninitialised slab data when
logging it.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 006cf13b2199..1db07d4dc738 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -891,8 +891,10 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
get_version_reply,
sizeof(*get_version_reply),
1000);
- if (result < 0) {
- dev_err(idev, "LEGO USB Tower get version control request
failed\n");
+ if (result < sizeof(*get_version_reply)) {
+ if (result >= 0)
+ result = -EIO;
+ dev_err(idev, "get version request failed: %d\n", result);
retval = result;
goto error;
}
--
2.23.0
[PATCH RESEND 0/4] USB: legousbtower: misc fixes
[ Resend with Juergen on CC ] This series fixes a few issues found in the legousbtower driver. The potential deadlock issue was reported by syzbot, and the rest was found through inspection. I have bunch of clean ups for this driver as well that I'll post once these are in Linus's tree. Johan Johan Hovold (4): USB: legousbtower: fix slab info leak at probe USB: legousbtower: fix deadlock on disconnect USB: legousbtower: fix potential NULL-deref on disconnect USB: legousbtower: fix open after failed reset request drivers/usb/misc/legousbtower.c | 55 ++--- 1 file changed, 23 insertions(+), 32 deletions(-) -- 2.23.0
[PATCH 3/4] USB: legousbtower: fix potential NULL-deref on disconnect
The driver is using its struct usb_device pointer as an inverted
disconnected flag, but was setting it to NULL before making sure all
completion handlers had run. This could lead to a NULL-pointer
dereference in a number of dev_dbg and dev_err statements in the
completion handlers which relies on said pointer.
Fix this by unconditionally stopping all I/O and preventing
resubmissions by poisoning the interrupt URBs at disconnect and using a
dedicated disconnected flag.
This also makes sure that all I/O has completed by the time the
disconnect callback returns.
Fixes: 9d974b2a06e3 ("USB: legousbtower.c: remove err() usage")
Fixes: fef526cae700 ("USB: legousbtower: remove custom debug macro")
Fixes: 4dae99638097 ("USB: legotower: remove custom debug macro and module
parameter")
Cc: stable # 3.5
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 26 +++---
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 773e4188f336..4fa999882635 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -190,6 +190,7 @@ struct lego_usb_tower {
unsigned char minor; /* the starting minor number
for this device */
int open_count; /* number of times this port
has been opened */
+ unsigned long disconnected:1;
char* read_buffer;
size_t read_buffer_length; /* this much came in */
@@ -289,8 +290,6 @@ static inline void lego_usb_tower_debug_data(struct device
*dev,
*/
static inline void tower_delete (struct lego_usb_tower *dev)
{
- tower_abort_transfers (dev);
-
/* free data structures */
usb_free_urb(dev->interrupt_in_urb);
usb_free_urb(dev->interrupt_out_urb);
@@ -430,7 +429,8 @@ static int tower_release (struct inode *inode, struct file
*file)
retval = -ENODEV;
goto unlock_exit;
}
- if (dev->udev == NULL) {
+
+ if (dev->disconnected) {
/* the device was unplugged before the file was released */
/* unlock here as tower_delete frees dev */
@@ -466,10 +466,9 @@ static void tower_abort_transfers (struct lego_usb_tower
*dev)
if (dev->interrupt_in_running) {
dev->interrupt_in_running = 0;
mb();
- if (dev->udev)
- usb_kill_urb (dev->interrupt_in_urb);
+ usb_kill_urb(dev->interrupt_in_urb);
}
- if (dev->interrupt_out_busy && dev->udev)
+ if (dev->interrupt_out_busy)
usb_kill_urb(dev->interrupt_out_urb);
}
@@ -505,7 +504,7 @@ static __poll_t tower_poll (struct file *file, poll_table
*wait)
dev = file->private_data;
- if (!dev->udev)
+ if (dev->disconnected)
return EPOLLERR | EPOLLHUP;
poll_wait(file, &dev->read_wait, wait);
@@ -552,7 +551,7 @@ static ssize_t tower_read (struct file *file, char __user
*buffer, size_t count,
}
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL) {
+ if (dev->disconnected) {
retval = -ENODEV;
pr_err("No device or device unplugged %d\n", retval);
goto unlock_exit;
@@ -638,7 +637,7 @@ static ssize_t tower_write (struct file *file, const char
__user *buffer, size_t
}
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL) {
+ if (dev->disconnected) {
retval = -ENODEV;
pr_err("No device or device unplugged %d\n", retval);
goto unlock_exit;
@@ -748,7 +747,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
resubmit:
/* resubmit if we're still running */
- if (dev->interrupt_in_running && dev->udev) {
+ if (dev->interrupt_in_running) {
retval = usb_submit_urb (dev->interrupt_in_urb, GFP_ATOMIC);
if (retval)
dev_err(&dev->udev->dev,
@@ -813,6 +812,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
dev->udev = udev;
dev->open_count = 0;
+ dev->disconnected = 0;
dev->read_buffer = NULL;
dev->read_buffer_length = 0;
@@ -938,6 +938,10 @@ static void tower_disconnect (struct usb_interface
*interface)
/* give back our minor and prevent further open() */
usb_deregister_dev (interface, &tower_class);
+ /* stop I/O */
+ usb_poison_urb(dev->interrupt_in_urb);
+ usb_poison_urb(dev->interrupt_out_urb);
+
mutex_lock(&dev->
[PATCH 1/4] USB: legousbtower: fix slab info leak at probe
Make sure to check for short transfers when retrieving the version
information at probe to avoid leaking uninitialised slab data when
logging it.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 006cf13b2199..1db07d4dc738 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -891,8 +891,10 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
get_version_reply,
sizeof(*get_version_reply),
1000);
- if (result < 0) {
- dev_err(idev, "LEGO USB Tower get version control request
failed\n");
+ if (result < sizeof(*get_version_reply)) {
+ if (result >= 0)
+ result = -EIO;
+ dev_err(idev, "get version request failed: %d\n", result);
retval = result;
goto error;
}
--
2.23.0
[PATCH 0/4] USB: legousbtower: misc fixes
This series fixes a few issues found in the legousbtower driver. The potential deadlock issue was reported by syzbot, and the rest was found through inspection. I have bunch of clean ups for this driver as well that I'll post once these are in Linus's tree. Johan Johan Hovold (4): USB: legousbtower: fix slab info leak at probe USB: legousbtower: fix deadlock on disconnect USB: legousbtower: fix potential NULL-deref on disconnect USB: legousbtower: fix open after failed reset request drivers/usb/misc/legousbtower.c | 55 ++--- 1 file changed, 23 insertions(+), 32 deletions(-) -- 2.23.0
[PATCH 2/4] USB: legousbtower: fix deadlock on disconnect
Fix a potential deadlock if disconnect races with open.
Since commit d4ead16f50f9 ("USB: prevent char device open/deregister
race") core holds an rw-semaphore while open is called and when
releasing the minor number during deregistration. This can lead to an
ABBA deadlock if a driver takes a lock in open which it also holds
during deregistration.
This effectively reverts commit 78663ecc344b ("USB: disconnect open race
in legousbtower") which needlessly introduced this issue after a generic
fix for this race had been added to core by commit d4ead16f50f9 ("USB:
prevent char device open/deregister race").
Fixes: 78663ecc344b ("USB: disconnect open race in legousbtower")
Cc: stable # 2.6.24
Reported-by: syzbot+f9549f5ee8a5416f0...@syzkaller.appspotmail.com
Tested-by: syzbot+f9549f5ee8a5416f0...@syzkaller.appspotmail.com
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 19 ++-
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 1db07d4dc738..773e4188f336 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -179,7 +179,6 @@ static const struct usb_device_id tower_table[] = {
};
MODULE_DEVICE_TABLE (usb, tower_table);
-static DEFINE_MUTEX(open_disc_mutex);
#define LEGO_USB_TOWER_MINOR_BASE 160
@@ -332,18 +331,14 @@ static int tower_open (struct inode *inode, struct file
*file)
goto exit;
}
- mutex_lock(&open_disc_mutex);
dev = usb_get_intfdata(interface);
-
if (!dev) {
- mutex_unlock(&open_disc_mutex);
retval = -ENODEV;
goto exit;
}
/* lock this device */
if (mutex_lock_interruptible(&dev->lock)) {
- mutex_unlock(&open_disc_mutex);
retval = -ERESTARTSYS;
goto exit;
}
@@ -351,12 +346,10 @@ static int tower_open (struct inode *inode, struct file
*file)
/* allow opening only once */
if (dev->open_count) {
- mutex_unlock(&open_disc_mutex);
retval = -EBUSY;
goto unlock_exit;
}
dev->open_count = 1;
- mutex_unlock(&open_disc_mutex);
/* reset the tower */
result = usb_control_msg (dev->udev,
@@ -423,10 +416,9 @@ static int tower_release (struct inode *inode, struct file
*file)
if (dev == NULL) {
retval = -ENODEV;
- goto exit_nolock;
+ goto exit;
}
- mutex_lock(&open_disc_mutex);
if (mutex_lock_interruptible(&dev->lock)) {
retval = -ERESTARTSYS;
goto exit;
@@ -456,10 +448,7 @@ static int tower_release (struct inode *inode, struct file
*file)
unlock_exit:
mutex_unlock(&dev->lock);
-
exit:
- mutex_unlock(&open_disc_mutex);
-exit_nolock:
return retval;
}
@@ -912,7 +901,6 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
if (retval) {
/* something prevented us from registering this driver */
dev_err(idev, "Not able to get a minor for this device.\n");
- usb_set_intfdata (interface, NULL);
goto error;
}
dev->minor = interface->minor;
@@ -944,16 +932,13 @@ static void tower_disconnect (struct usb_interface
*interface)
int minor;
dev = usb_get_intfdata (interface);
- mutex_lock(&open_disc_mutex);
- usb_set_intfdata (interface, NULL);
minor = dev->minor;
- /* give back our minor */
+ /* give back our minor and prevent further open() */
usb_deregister_dev (interface, &tower_class);
mutex_lock(&dev->lock);
- mutex_unlock(&open_disc_mutex);
/* if the device is not opened, then we clean up right now */
if (!dev->open_count) {
--
2.23.0
[PATCH 4/4] USB: legousbtower: fix open after failed reset request
The driver would return with a nonzero open count in case the reset
control request failed. This would prevent any further attempts to open
the char dev until the device was disconnected.
Fix this by incrementing the open count only on successful open.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 4fa999882635..44d6a3381804 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -348,7 +348,6 @@ static int tower_open (struct inode *inode, struct file
*file)
retval = -EBUSY;
goto unlock_exit;
}
- dev->open_count = 1;
/* reset the tower */
result = usb_control_msg (dev->udev,
@@ -388,13 +387,14 @@ static int tower_open (struct inode *inode, struct file
*file)
dev_err(&dev->udev->dev,
"Couldn't submit interrupt_in_urb %d\n", retval);
dev->interrupt_in_running = 0;
- dev->open_count = 0;
goto unlock_exit;
}
/* save device in the file's private structure */
file->private_data = dev;
+ dev->open_count = 1;
+
unlock_exit:
mutex_unlock(&dev->lock);
--
2.23.0
[GIT PULL] USB-serial updates for 5.4-rc1
The following changes since commit d1abaeb3be7b5fa6d7a1fbbd2e14e3310005c4c1: Linux 5.3-rc5 (2019-08-18 14:31:08 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-5.4-rc1 for you to fetch changes up to 7a786b84790789eff5bad49e3f6c15f75b7bf691: USB: serial: ftdi_sio: add support for FT232H CBUS gpios (2019-08-28 15:35:33 +0200) USB-serial updates for 5.4-rc1 Here are the USB-serial updates for 5.4-rc1, which this time is just a single commit adding support for the CBUS GPIOs on FT232H devices. This change has spent a week in linux-next with no reported issues. Signed-off-by: Johan Hovold Matthew Michilot (1): USB: serial: ftdi_sio: add support for FT232H CBUS gpios drivers/usb/serial/ftdi_sio.c | 43 +++ 1 file changed, 43 insertions(+)
[GIT PULL] USB-serial fixes for 5.3-rc5
The following changes since commit e21a712a9685488f5ce80495b37b9fdbe96c230d: Linux 5.3-rc3 (2019-08-04 18:40:12 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-5.3-rc5 for you to fetch changes up to e5d8badf37e6b547842f2fcde10361b29e08bd36: USB: serial: option: add the BroadMobi BM818 card (2019-08-15 13:46:22 +0200) USB-serial fixes for 5.3-rc5 Here are some new modem device ids. All have been in linux-next with no reported issues. Signed-off-by: Johan Hovold Bob Ham (1): USB: serial: option: add the BroadMobi BM818 card 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 drivers/usb/serial/option.c | 10 ++ 1 file changed, 10 insertions(+)
Re: [PATCH 1/2] usb: serial: option: Add the BroadMobi BM818 card
On Thu, Aug 15, 2019 at 01:19:19PM +0100, Bob Ham wrote: > On 15/08/2019 12:49, Johan Hovold wrote: > > On Mon, Aug 05, 2019 at 03:44:30PM +0100, Bob Ham wrote: > >> On 05/08/2019 12:47, Johan Hovold wrote: > >>> On Wed, Jul 24, 2019 at 07:52:26AM -0700, Angus Ainslie (Purism) wrote: > >>>> From: Bob Ham > >>>> > >>>> Add a VID:PID for the BroadModi BM818 M.2 card > >>> > >>> Would you mind posting the output of usb-devices (or lsusb -v) for this > >>> device? > >> > >> T: Bus=01 Lev=03 Prnt=40 Port=03 Cnt=01 Dev#= 44 Spd=480 MxCh= 0 > >> D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1 > >> P: Vendor=2020 ProdID=2060 Rev=00.00 > >> S: Manufacturer=Qualcomm, Incorporated > >> S: Product=Qualcomm CDMA Technologies MSM > >> C: #Ifs= 5 Cfg#= 1 Atr=e0 MxPwr=500mA > >> I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none) > >> I: If#=0x1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none) > >> I: If#=0x2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none) > >> I: If#=0x3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=fe Prot=ff Driver=(none) > >> I: If#=0x4 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none) > > > > I amended the commit message with the above, switched to > > USB_DEVICE_INTERFACE_CLASS(), fixed the comment and moved the entry > > to the other 0x2020 entries before applying. > > Sorry I should probably have mentioned this before but Angus has been on > vacation, hence the silence on the other matters. Regardless, thanks. Ok, no worries. Johan signature.asc Description: PGP signature
Re: [PATCH 1/2] usb: serial: option: Add the BroadMobi BM818 card
On Mon, Aug 05, 2019 at 03:44:30PM +0100, Bob Ham wrote: > On 05/08/2019 12:47, Johan Hovold wrote: > > On Wed, Jul 24, 2019 at 07:52:26AM -0700, Angus Ainslie (Purism) wrote: > >> From: Bob Ham > >> > >> Add a VID:PID for the BroadModi BM818 M.2 card > > > > Would you mind posting the output of usb-devices (or lsusb -v) for this > > device? > > T: Bus=01 Lev=03 Prnt=40 Port=03 Cnt=01 Dev#= 44 Spd=480 MxCh= 0 > D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1 > P: Vendor=2020 ProdID=2060 Rev=00.00 > S: Manufacturer=Qualcomm, Incorporated > S: Product=Qualcomm CDMA Technologies MSM > C: #Ifs= 5 Cfg#= 1 Atr=e0 MxPwr=500mA > I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none) > I: If#=0x1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none) > I: If#=0x2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none) > I: If#=0x3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=fe Prot=ff Driver=(none) > I: If#=0x4 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none) I amended the commit message with the above, switched to USB_DEVICE_INTERFACE_CLASS(), fixed the comment and moved the entry to the other 0x2020 entries before applying. Johan signature.asc Description: PGP signature
Re: [PATCHv2] USB: serial: option: Add Motorola modem UARTs
On Thu, Aug 15, 2019 at 01:26:02AM -0700, Tony Lindgren wrote: > On Motorola Mapphone devices such as Droid 4 there are five USB ports > that do not use the same layout as Gobi 1K/2K/etc devices listed in > qcserial.c. So we should use qcaux.c or option.c as noted by > Dan Williams . > > As the Motorola USB serial ports have an interrupt endpoint as shown > with lsusb -v, we should use option.c instead of qcaux.c as pointed out > by Johan Hovold . > > The ff/ff/ff interfaces seem to always be UARTs on Motorola devices. > For the other interfaces, class 0x0a (CDC Data) should not in general > be added as they are typically part of a multi-interface function as > noted earlier by Bjørn Mork . > > However, looking at the Motorola mapphone kernel code, the mdm6600 0x0a > class is only used for flashing the modem firmware, and there are no > other interfaces. So I've added that too with more details below as it > works just fine. > > The ttyUSB ports on Droid 4 are: > > ttyUSB0 DIAG, CQDM-capable > ttyUSB1 MUX or NMEA, no response > ttyUSB2 MUX or NMEA, no response > ttyUSB3 TCMD > ttyUSB4 AT-capable > Tested-by: Pavel Machek > Signed-off-by: Tony Lingren I fixed up the typo in your name here, which checkpatch caught. > --- > > Changes since v1: > - Leave out defines as suggested by Lars Thanks, Tony. Now applied. Johan
Re: Fwd: Re: New USB Device
On Wed, Jul 31, 2019 at 07:32:29PM +0200, Markus Breunig wrote: > > > Am 16.07.2019 um 11:23 schrieb Johan Hovold: > > [ Pleas avoid top posting. ] > > > > On Sun, Jul 07, 2019 at 09:38:00PM +0200, Markus Breunig wrote: > >> Hi Greg, > >> > >> also the company GNS has a fragmented homepage, the handbook ist > >> available here: > >> http://www.servicedocs.com/ARTIKELEN/7200284490001.pdf > >> habe a look to page 10 "Remarks to Linux" > >> > >> This is the log of "lsusb -v" (full scan result attached): > >> > >> Bus 001 Device 004: ID 04d8:f8e8 Microchip Technology, Inc. Harmony > >> 300/350 Remote > > > > Are you sure this is the right device? This looks like a remote control, > > and one that should be using the cdc-acm driver. > > > > The output of lsusb before plugging the GNS5890 device into the USB-port: > > Bus 001 Device 005: ID 046d:c05a Logitech, Inc. M90/M100 Optical Mouse > Bus 001 Device 004: ID 046a:0001 Cherry GmbH Keyboard > Bus 001 Device 003: ID 0424:ec00 Standard Microsystems Corp. > SMSC9512/9514 Fast Ethernet Adapter > Bus 001 Device 002: ID 0424:9514 Standard Microsystems Corp. SMC9514 Hub > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > and the result of the lsusb after plugging the GNS5890 device into the > USB-port: > > Bus 001 Device 005: ID 046d:c05a Logitech, Inc. M90/M100 Optical Mouse > Bus 001 Device 004: ID 046a:0001 Cherry GmbH Keyboard > Bus 001 Device 006: ID 04d8:f8e8 Microchip Technology, Inc. Harmony > 300/350 Remote > Bus 001 Device 003: ID 0424:ec00 Standard Microsystems Corp. > SMSC9512/9514 Fast Ethernet Adapter > Bus 001 Device 002: ID 0424:9514 Standard Microsystems Corp. SMC9514 Hub > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub Ok, thanks for confirming. Based on the below descriptors, this device should be handled by the cdc-acm driver and show up as a /dev/ttyACMn. Do you have that driver enabled? Can you enable debugging in that driver and post the syslog from when plugging the device in if it still doesn't work? You can enable debugging using modprobe cdc-acm dyndbg==p or through sysfs, see Documentation/admin-guide/dynamic-debug-howto.rst > >> Device Descriptor: > >> bLength18 > >> bDescriptorType 1 > >> bcdUSB 2.00 > >> bDeviceClass 255 Vendor Specific Class > >> bDeviceSubClass 0 > >> bDeviceProtocol 0 > >> bMaxPacketSize0 8 > >> idVendor 0x04d8 Microchip Technology, Inc. > >> idProduct 0xf8e8 Harmony 300/350 Remote > >> bcdDevice 48.12 > >> iManufacturer 1 > >> iProduct2 > >> iSerial 3 > >> bNumConfigurations 1 > >> Configuration Descriptor: > >> bLength 9 > >> bDescriptorType 2 > >> wTotalLength 67 > >> bNumInterfaces 2 > >> bConfigurationValue 1 > >> iConfiguration 0 > >> bmAttributes 0xc0 > >> Self Powered > >> MaxPower 100mA > >> Interface Descriptor: > >> bLength 9 > >> bDescriptorType 4 > >> bInterfaceNumber0 > >> bAlternateSetting 0 > >> bNumEndpoints 1 > >> bInterfaceClass 2 Communications > >> bInterfaceSubClass 2 Abstract (modem) > >> bInterfaceProtocol 1 AT-commands (v.25ter) > >> iInterface 0 > >> CDC Header: > >> bcdCDC 1.10 > >> CDC ACM: > >> bmCapabilities 0x02 > >> line coding and serial state > >> CDC Union: > >> bMasterInterface0 > >> bSlaveInterface 1 > >> CDC Call Management: > >> bmCapabilities 0x00 > >> bDataInterface 1 > >> Endpoint Descriptor: > >> bLength 7 > >> bDescriptorType 5 > >> bEndpointAddress 0x82 EP 2 IN > >> bmAttributes3 > >> Transfer TypeInterrupt > >> Synch Type None > >> Usage Type Data > >> wMaxPacketSize 0x0008 1x 8 bytes > >> bInterval 2 > >> Interface Descriptor: Johan
Re: [PATCH] USB: serial: ftdi_sio: add support for FT232H CBUS gpios
On Thu, Aug 08, 2019 at 10:23:48PM +, Matthew Michilot wrote: > Enable support for cbus gpios on FT232H. The cbus configuration is > stored in one word in the EEPROM at byte-offset 0x1a with the mux > config for ACBUS5, ACBUS6, ACBUS8 and ACBUS9 (only pins that can be > configured as I/O mode). > > Tested using FT232H by configuring one ACBUS pin at a time. > > Review-by: Tim Harvey > Signed-off-by: Matthew Michilot Also make sure your SoB matches the From line. Johan
Re: [PATCH] USB: serial: ftdi_sio: add support for FT232H CBUS gpios
On Thu, Aug 08, 2019 at 10:23:48PM +, Matthew Michilot wrote:
> Enable support for cbus gpios on FT232H. The cbus configuration is
> stored in one word in the EEPROM at byte-offset 0x1a with the mux
It seems to be stored in two words.
> config for ACBUS5, ACBUS6, ACBUS8 and ACBUS9 (only pins that can be
> configured as I/O mode).
>
> Tested using FT232H by configuring one ACBUS pin at a time.
>
> Review-by: Tim Harvey
typo: Reviewed-by
> Signed-off-by: Matthew Michilot
> ---
> drivers/usb/serial/ftdi_sio.c | 43 +++
> 1 file changed, 43 insertions(+)
>
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 4b3a049561f3..c8d35faa8f61 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -2023,6 +2023,46 @@ static int ftdi_read_eeprom(struct usb_serial *serial,
> void *dst, u16 addr,
> return 0;
> }
>
> +static int ftdi_gpio_init_ft232h(struct usb_serial_port *port)
> +{
> + struct ftdi_private *priv = usb_get_serial_port_data(port);
> + u8 *buf;
> + u16 cbus_config;
> + int ret;
> + int i;
> +
> + buf = kmalloc(2, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = ftdi_read_eeprom(port->serial, buf, 0x1A, 4);
You allocate 2 byte above, but write 4 bytes into buf here.
Please also use lower-case hex notation consistently throughout.
> + if (ret < 0)
> + goto out_free;
> +
> + /*
> + * FT232H CBUS Memory Map
> + *
> + * 0x1A: 8X (upper nibble -> AC5)
> + * 0x1B: X8 (lower nibble -> AC6)
> + * 0x1C: 88 (upper nibble -> AC8 | lower nibble -> AC9)
> + */
> + cbus_config = (((buf[0] & 0xf0) | (buf[1] & 0xf)) << 8 | buf[2]);
Have you verified the order you use here by enabling one gpio at a time
and toggling it? The reason I ask is that the above would give a
mapping of
gpio0 -> AC9
gpio1 -> AC8
gpio2 -> AC6
gpio4 -> AC5
which looks backwards but may be correct.
Also please drop the outer parentheses in the above expression.
> +
> + priv->gc.ngpio = 4;
> + priv->gpio_altfunc = 0xff;
> +
> + for (i = 0; i < priv->gc.ngpio; ++i) {
> + if ((cbus_config & 0xf) == FTDI_FTX_CBUS_MUX_GPIO)
> + priv->gpio_altfunc &= ~BIT(i);
> + cbus_config >>= 4;
> + }
> +
> +out_free:
> + kfree(buf);
> +
> + return ret;
> +}
> +
> static int ftdi_gpio_init_ft232r(struct usb_serial_port *port)
> {
> struct ftdi_private *priv = usb_get_serial_port_data(port);
> @@ -2104,6 +2144,9 @@ static int ftdi_gpio_init(struct usb_serial_port *port)
> case FTX:
> result = ftdi_gpio_init_ftx(port);
> break;
> + case FT232H:
> + result = ftdi_gpio_init_ft232h(port);
> + break;
Please keep the case labels sorted alphabetically.
> default:
> return 0;
> }
Johan
Re: [PATCH 1/2] usb: serial: option: Add the BroadMobi BM818 card
On Wed, Jul 24, 2019 at 07:52:26AM -0700, Angus Ainslie (Purism) wrote:
> From: Bob Ham
>
> Add a VID:PID for the BroadModi BM818 M.2 card
Would you mind posting the output of usb-devices (or lsusb -v) for this
device?
> Signed-off-by: Bob Ham
> Signed-off-by: Angus Ainslie (Purism)
> ---
> drivers/usb/serial/option.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
> index c1582fbd1150..674a68ee9564 100644
> --- a/drivers/usb/serial/option.c
> +++ b/drivers/usb/serial/option.c
> @@ -1975,6 +1975,8 @@ static const struct usb_device_id option_ids[] = {
> .driver_info = RSVD(4) | RSVD(5) },
> { USB_DEVICE_INTERFACE_CLASS(0x2cb7, 0x0105, 0xff),
> /* Fibocom NL678 series */
> .driver_info = RSVD(6) },
> + { USB_DEVICE(0x2020, 0x2060),
> /* BroadMobi */
Looks like you forgot to include the model in the comment here.
And please move this one after the other 0x2020 (PID 0x2031) entry.
Should you also be using USB_DEVICE_INTERFACE_CLASS() (e.g. to avoid
matching a mass-storage interface)?
> + .driver_info = RSVD(4) },
> { } /* Terminating entry */
> };
> MODULE_DEVICE_TABLE(usb, option_ids);
Johan
Re: [PATCH v6] USB: serial: option: add D-Link DWM-222 device ID
On Wed, Jul 17, 2019 at 11:11:34AM +0200, Rogan Dawes wrote: > Add device id for D-Link DWM-222 A2. > > MI_00 D-Link HS-USB Diagnostics > MI_01 D-Link HS-USB Modem > MI_02 D-Link HS-USB AT Port > MI_03 D-Link HS-USB NMEA > MI_04 D-Link HS-USB WWAN Adapter (qmi_wwan) > MI_05 USB Mass Storage Device > > Cc: sta...@vger.kernel.org > Signed-Off-By: Rogan Dawes Now applied, thanks. Johan
Re: [PATCH v3] USB: serial: option: Add support for ZTE MF871A
On Sat, Jul 20, 2019 at 10:23:18PM +0900, Yoshiaki Okamoto wrote: > This patch adds support for MF871A USB modem (aka Speed USB STICK U03) > to option driver. This modem is manufactured by ZTE corporation, and > sold by KDDI. > Co-developed-by: Hiroyuki Yamamoto > Signed-off-by: Hiroyuki Yamamoto > Signed-off-by: Yoshiaki Okamoto > --- > > Changes in v3: > - Change used macro to USB_DEVICE_AND_INTERFACE_INFO. > > Changes in v2: > - Add Co-developed-by tag. > - Move away product-id define and add short comment after the entry. Now applied, thanks. Johan
[PATCH] NFC: nfcmrvl: fix gpio-handling regression
Fix two reset-gpio sanity checks which were never converted to use
gpio_is_valid(), and make sure to use -EINVAL to indicate a missing
reset line also for the UART-driver module parameter and for the USB
driver.
This specifically prevents the UART and USB drivers from incidentally
trying to request and use gpio 0, and also avoids triggering a WARN() in
gpio_to_desc() during probe when no valid reset line has been specified.
Fixes: e33a3f84f88f ("NFC: nfcmrvl: allow gpio 0 for reset signalling")
Cc: stable # 4.13
Reported-by: syzbot+cf35b76f35e068a11...@syzkaller.appspotmail.com
Tested-by: syzbot+cf35b76f35e068a11...@syzkaller.appspotmail.com
Signed-off-by: Johan Hovold
---
drivers/nfc/nfcmrvl/main.c | 4 ++--
drivers/nfc/nfcmrvl/uart.c | 4 ++--
drivers/nfc/nfcmrvl/usb.c | 1 +
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index e65d027b91fa..529be35ac178 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -244,7 +244,7 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
/* Reset possible fault of previous session */
clear_bit(NFCMRVL_PHY_ERROR, &priv->flags);
- if (priv->config.reset_n_io) {
+ if (gpio_is_valid(priv->config.reset_n_io)) {
nfc_info(priv->dev, "reset the chip\n");
gpio_set_value(priv->config.reset_n_io, 0);
usleep_range(5000, 1);
@@ -255,7 +255,7 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
void nfcmrvl_chip_halt(struct nfcmrvl_private *priv)
{
- if (priv->config.reset_n_io)
+ if (gpio_is_valid(priv->config.reset_n_io))
gpio_set_value(priv->config.reset_n_io, 0);
}
diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index 9a22056e8d9e..e5a622ce4b95 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -26,7 +26,7 @@
static unsigned int hci_muxed;
static unsigned int flow_control;
static unsigned int break_control;
-static unsigned int reset_n_io;
+static int reset_n_io = -EINVAL;
/*
** NFCMRVL NCI OPS
@@ -231,5 +231,5 @@ MODULE_PARM_DESC(break_control, "Tell if UART driver must
drive break signal.");
module_param(hci_muxed, uint, 0);
MODULE_PARM_DESC(hci_muxed, "Tell if transport is muxed in HCI one.");
-module_param(reset_n_io, uint, 0);
+module_param(reset_n_io, int, 0);
MODULE_PARM_DESC(reset_n_io, "GPIO that is wired to RESET_N signal.");
diff --git a/drivers/nfc/nfcmrvl/usb.c b/drivers/nfc/nfcmrvl/usb.c
index 945cc903d8f1..888e298f610b 100644
--- a/drivers/nfc/nfcmrvl/usb.c
+++ b/drivers/nfc/nfcmrvl/usb.c
@@ -305,6 +305,7 @@ static int nfcmrvl_probe(struct usb_interface *intf,
/* No configuration for USB */
memset(&config, 0, sizeof(config));
+ config.reset_n_io = -EINVAL;
nfc_info(&udev->dev, "intf %p id %p\n", intf, id);
--
2.22.0
Re: [PATCH v6] USB: serial: option: add D-Link DWM-222 device ID
On Wed, Jul 17, 2019 at 11:11:34AM +0200, Rogan Dawes wrote: > Add device id for D-Link DWM-222 A2. > > MI_00 D-Link HS-USB Diagnostics > MI_01 D-Link HS-USB Modem > MI_02 D-Link HS-USB AT Port > MI_03 D-Link HS-USB NMEA > MI_04 D-Link HS-USB WWAN Adapter (qmi_wwan) > MI_05 USB Mass Storage Device > > Cc: sta...@vger.kernel.org > Signed-Off-By: Rogan Dawes Perfect, thanks for sticking with it. I'll queue this is up after the merge window closes. Johan
Re: WARNING in gpio_to_desc
On Tue, Jul 16, 2019 at 11:52:19PM +0200, Linus Walleij wrote:
> On Wed, Jul 10, 2019 at 1:07 PM syzbot
> wrote:
>
> > HEAD commit:7829a896 usb-fuzzer: main usb gadget fuzzer driver
> (...)
> > __gpio_set_value include/asm-generic/gpio.h:104 [inline]
> > gpio_set_value include/linux/gpio.h:71 [inline]
> > nfcmrvl_chip_halt+0x4e/0x70 drivers/nfc/nfcmrvl/main.c:259
> > nfcmrvl_nci_register_dev+0x2d4/0x378 drivers/nfc/nfcmrvl/main.c:176
> > nfcmrvl_probe+0x4e9/0x5e0 drivers/nfc/nfcmrvl/usb.c:344
>
> This bug is somewhere in the drivers/nfc/nfcmrvl* code handling
> GPIOs.
Right, and it's my bug.
> It should be converted to GPIO descriptors and fixed up, see
> drivers/gpio/TODO for details on how to do this.
Conversion will have to wait, let's fix the regression first. :)
> Johan/Vincent, tell me if you want me to forward the full fuzzing
> robot crash dump.
No need, thanks. I got it the report.
Something like the below compiles and should fix it. Vacation starts
today so I'll revisit and send a proper patch in a couple of weeks.
Perhaps someone can feed it to the bot meanwhile (no time to play with
it right now).
Note that this issue has been there since 4.12, so guess no one uses
these devices...
Johan
>From e9d9d0ef5ffd6b306cffb2f4e2514f503aa626a5 Mon Sep 17 00:00:00 2001
From: Johan Hovold
Date: Wed, 17 Jul 2019 11:07:13 +0200
Subject: [PATCH] NFC: nfcmrvl: fix gpio-handling regression
FIXME
Fixes: e33a3f84f88f ("NFC: nfcmrvl: allow gpio 0 for reset signalling")
Not-Signed-off-by: Johan Hovold
---
drivers/nfc/nfcmrvl/main.c | 4 ++--
drivers/nfc/nfcmrvl/usb.c | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index e65d027b91fa..529be35ac178 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -244,7 +244,7 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
/* Reset possible fault of previous session */
clear_bit(NFCMRVL_PHY_ERROR, &priv->flags);
- if (priv->config.reset_n_io) {
+ if (gpio_is_valid(priv->config.reset_n_io)) {
nfc_info(priv->dev, "reset the chip\n");
gpio_set_value(priv->config.reset_n_io, 0);
usleep_range(5000, 1);
@@ -255,7 +255,7 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
void nfcmrvl_chip_halt(struct nfcmrvl_private *priv)
{
- if (priv->config.reset_n_io)
+ if (gpio_is_valid(priv->config.reset_n_io))
gpio_set_value(priv->config.reset_n_io, 0);
}
diff --git a/drivers/nfc/nfcmrvl/usb.c b/drivers/nfc/nfcmrvl/usb.c
index 945cc903d8f1..888e298f610b 100644
--- a/drivers/nfc/nfcmrvl/usb.c
+++ b/drivers/nfc/nfcmrvl/usb.c
@@ -305,6 +305,7 @@ static int nfcmrvl_probe(struct usb_interface *intf,
/* No configuration for USB */
memset(&config, 0, sizeof(config));
+ config.reset_n_io = -EINVAL;
nfc_info(&udev->dev, "intf %p id %p\n", intf, id);
--
2.22.0
Re: [PATCH v2] USB: serial: option: Add support for ZTE MF871A
On Wed, Jul 17, 2019 at 02:40:16PM +0900, Yoshiaki Okamoto wrote: > This patch adds support for MF871A USB modem (aka Speed USB STICK U03) > to option driver. This modem is manufactured by ZTE corporation, and > sold by KDDI. > > Interface layout: > 0: AT > 1: MODEM > > usb-devices output: > T: Bus=01 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#= 9 Spd=480 MxCh= 0 > D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1 > P: Vendor=19d2 ProdID=1481 Rev=52.87 > S: Manufacturer=ZTE,Incorporated > S: Product=ZTE Technologies MSM > S: SerialNumber=1234567890ABCDEF > C: #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=500mA > I: If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option > I: If#= 1 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option > > Co-developed-by: Hiroyuki Yamamoto > Signed-off-by: Hiroyuki Yamamoto > Signed-off-by: Yoshiaki Okamoto > --- > > Changes in v2: > - Add Co-developed-by tag. > - Move away product-id define and add short comment after the entry. Perfect, thanks for the update. I'll queue this one up after the merge window closes. Johan
Re: [PATCH v5] USB: serial/qmi_wwan: add D-Link DWM-222 device ID
On Tue, Jul 16, 2019 at 08:43:52PM -0500, Dan Williams wrote:
> On Tue, 2019-07-16 at 21:12 +0200, Rogan Dawes wrote:
> > Add device id for D-Link DWM-222.
> > @@ -1951,6 +1951,8 @@ static const struct usb_device_id option_ids[] = {
> > .driver_info = RSVD(4) },
> > { USB_DEVICE_INTERFACE_CLASS(0x2001, 0x7e35, 0xff),
> > /* D-Link DWM-222 */
> > .driver_info = RSVD(4) },
> > + { USB_DEVICE_INTERFACE_CLASS(0x2001, 0x7e3d, 0xff),
> > /* D-Link DWM-222 A2 */
> > + .driver_info = RSVD(0) | RSVD(4) },
>
> I don't think we actually do need to blacklist interface 0, since
> that's likely a QCDM/DIAG interface and that does need a serial driver
> like option or qcserial.
>
> Johan, any reason why you thought we should blacklist 0?
It was just me being confused about the Qualcomm protocols and how the
diag port was accessed from user space. Thanks, Dan.
Rogan, let's keep interface 0 and only blacklist interface 4 as you did
originally. Sorry about the confusion.
Johan
Re: [PATCH v5] USB: serial/qmi_wwan: add D-Link DWM-222 device ID
On Tue, Jul 16, 2019 at 09:12:05PM +0200, Rogan Dawes wrote: > Add device id for D-Link DWM-222. > > Cc: sta...@vger.kernel.org > --- > Also add the qmi_wwan entry, since it was blacklisted already in option > > Apologies for the spam! No worries, you're getting there. :) > drivers/net/usb/qmi_wwan.c | 1 + > drivers/usb/serial/option.c | 2 ++ > 2 files changed, 3 insertions(+) But please split this up in two patches and run get_maintainer.pl on each. The qmi_wwan one will go through the network tree. Also include the interface layout that Lars provided in the USB serial commit message. And don't forget the SoB line as Greg pointed out. Thanks, Johan
Re: [PATCH] USB: serial: option: add D-Link DWM-222 device ID [version 2]
On Tue, Jul 16, 2019 at 09:34:56PM +0700, Lars Melin wrote: > On 7/16/2019 18:17, Johan Hovold wrote: > > snip > > > Ok, thanks. Do you have any idea what all those vendor interface are > > for? Some of the other D-Link entries blacklist a speech and debug port > > for example. > > > > We can always do that later if we need to, but perhaps you or someone > > already know (also adding Lars and Dan on CC). > > > > Johan > > > > This is the interface composition: > > MI_00 D-Link HS-USB Diagnostics > MI_01 D-Link HS-USB Modem > MI_02 D-Link HS-USB AT Port > MI_03 D-Link HS-USB NMEA > MI_04 D-Link HS-USB WWAN Adapter (qmi_wwan) > MI_05 USB Mass Storage Device Thanks, Lars. Then interface 0 and 4 should be blacklisted, right? Rogan, would you mind doing that in a v3? You can include above composition details in the commit message as well. Thanks, Johan
Re: [PATCH] USB: serial: option: add D-Link DWM-222 device ID [version 2]
On Tue, Jul 16, 2019 at 12:55:17PM +0200, Rogan Dawes wrote: > After binding the Option driver, lsusb -v -d 2001:7e3d > > Bus 001 Device 062: ID 2001:7e3d D-Link Corp. Mobile Connect > Device Descriptor: > bLength18 > bDescriptorType 1 > bcdUSB 2.01 > bDeviceClass0 > bDeviceSubClass 0 > bDeviceProtocol 0 > bMaxPacketSize064 > idVendor 0x2001 D-Link Corp. > idProduct 0x7e3d > bcdDevice2.28 > iManufacturer 1 Mobile Connect > iProduct2 Mobile Connect > iSerial 3 0123456789ABCDEF > bNumConfigurations 1 > Configuration Descriptor: > bLength 9 > bDescriptorType 2 > wTotalLength 0x00e8 > bNumInterfaces 6 > bConfigurationValue 1 > iConfiguration 0 > bmAttributes 0x80 > (Bus Powered) > MaxPower 500mA > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber0 > bAlternateSetting 0 > bNumEndpoints 2 > bInterfaceClass 255 Vendor Specific Class > bInterfaceSubClass255 Vendor Specific Subclass > bInterfaceProtocol255 Vendor Specific Protocol > iInterface 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x81 EP 1 IN > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0200 1x 512 bytes > bInterval 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x01 EP 1 OUT > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0200 1x 512 bytes > bInterval 0 > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber1 > bAlternateSetting 0 > bNumEndpoints 3 > bInterfaceClass 255 Vendor Specific Class > bInterfaceSubClass 0 > bInterfaceProtocol 0 > iInterface 0 > ** UNRECOGNIZED: 05 24 00 10 01 > ** UNRECOGNIZED: 05 24 01 00 00 > ** UNRECOGNIZED: 04 24 02 02 > ** UNRECOGNIZED: 05 24 06 00 00 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x83 EP 3 IN > bmAttributes3 > Transfer TypeInterrupt > Synch Type None > Usage Type Data > wMaxPacketSize 0x000a 1x 10 bytes > bInterval 9 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x82 EP 2 IN > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0200 1x 512 bytes > bInterval 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x02 EP 2 OUT > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0200 1x 512 bytes > bInterval 0 > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber2 > bAlternateSetting 0 > bNumEndpoints 3 > bInterfaceClass 255 Vendor Specific Class > bInterfaceSubClass 0 > bInterfaceProtocol 0 > iInterface 0 > ** UNRECOGNIZED: 05 24 00 10 01 > ** UNRECOGNIZED: 05 24 01 00 00 > ** UNRECOGNIZED: 04 24 02 02 > ** UNRECOGNIZED: 05 24 06 00 00 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x85 EP 5 IN > bmAttributes3 > Transfer TypeInterrupt > Synch Type None > Usage Type Data > wMaxPacketSize 0x000a 1x 10 bytes > bInterval 9 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x84 EP 4 IN > bmAttributes2 >
Re: Fwd: Re: New USB Device
[ Pleas avoid top posting. ] On Sun, Jul 07, 2019 at 09:38:00PM +0200, Markus Breunig wrote: > Hi Greg, > > also the company GNS has a fragmented homepage, the handbook ist > available here: > http://www.servicedocs.com/ARTIKELEN/7200284490001.pdf > habe a look to page 10 "Remarks to Linux" > > This is the log of "lsusb -v" (full scan result attached): > > Bus 001 Device 004: ID 04d8:f8e8 Microchip Technology, Inc. Harmony > 300/350 Remote Are you sure this is the right device? This looks like a remote control, and one that should be using the cdc-acm driver. > Device Descriptor: >bLength18 >bDescriptorType 1 >bcdUSB 2.00 >bDeviceClass 255 Vendor Specific Class >bDeviceSubClass 0 >bDeviceProtocol 0 >bMaxPacketSize0 8 >idVendor 0x04d8 Microchip Technology, Inc. >idProduct 0xf8e8 Harmony 300/350 Remote >bcdDevice 48.12 >iManufacturer 1 >iProduct2 >iSerial 3 >bNumConfigurations 1 >Configuration Descriptor: > bLength 9 > bDescriptorType 2 > wTotalLength 67 > bNumInterfaces 2 > bConfigurationValue 1 > iConfiguration 0 > bmAttributes 0xc0 >Self Powered > MaxPower 100mA > Interface Descriptor: >bLength 9 >bDescriptorType 4 >bInterfaceNumber0 >bAlternateSetting 0 >bNumEndpoints 1 >bInterfaceClass 2 Communications >bInterfaceSubClass 2 Abstract (modem) >bInterfaceProtocol 1 AT-commands (v.25ter) >iInterface 0 >CDC Header: > bcdCDC 1.10 >CDC ACM: > bmCapabilities 0x02 >line coding and serial state >CDC Union: > bMasterInterface0 > bSlaveInterface 1 >CDC Call Management: > bmCapabilities 0x00 > bDataInterface 1 >Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x82 EP 2 IN > bmAttributes3 >Transfer TypeInterrupt >Synch Type None >Usage Type Data > wMaxPacketSize 0x0008 1x 8 bytes > bInterval 2 > Interface Descriptor: >bLength 9 >bDescriptorType 4 >bInterfaceNumber1 >bAlternateSetting 0 >bNumEndpoints 2 >bInterfaceClass10 CDC Data >bInterfaceSubClass 0 Unused >bInterfaceProtocol 0 >iInterface 0 >Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x03 EP 3 OUT > bmAttributes2 >Transfer TypeBulk >Synch Type None >Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 >Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x83 EP 3 IN > bmAttributes2 >Transfer TypeBulk >Synch Type None >Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 > Am 05.07.2019 07:21, schrieb Greg KH: > > On Thu, Jul 04, 2019 at 10:47:47PM +0200, Markus Breunig wrote: > >> Hi Greg, > >> > >> using a serial device driver is the idea of the manufacturer > >> "www.gns-gmbh.com". In the LINUX instructions of the ADS-B receiver some > >> hints to use the device are given via usbserial. > > > > Any pointers to those instructions? > > > >> In practice the "GNS 5890 ADS-B Receiver" is similare to some GPS > >> Receivers with NMEA 0183 interface starting to send information on the > >> serial interface after power on and signal availabillity (with 115200 > >> boud data rate). Johan
Re: [PATCH] USB: serial: option: add D-Link DWM-222 device ID [version 2]
On Thu, Jul 11, 2019 at 12:34:57PM +0200, Rogan Dawes wrote: > Add device id for D-Link DWM-222. > > Cc: sta...@vger.kernel.org > Signed-Off-By: Rogan Dawes > --- > Apologies, a typo crept in when submitting this previously. Thanks for the patch, all looks good, but next time add the version information inside the "[PATCH v2]" prefix so that it doesn't end up the commit logs. Would you mind posting also the output of usb-devices (or lsusb -v) for this device for completeness? The former is compact enough to be included in the commit message. No need to resend unless you prefer to. I'll apply this one after the merge window closes. Thanks, Johan
Re: [PATCH] USB: serial: option: Add support for ZTE MF871A
On Thu, Jul 11, 2019 at 03:53:24PM +0900, Yoshiaki Okamoto wrote:
> This patch adds support for MF871A USB modem (aka Speed USB STICK U03)
> to option driver. This modem is manufactured by ZTE corporation, and
> sold by KDDI.
>
> Interface layout:
> 0: AT
> 1: MODEM
>
> usb-devices output:
> T: Bus=01 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#= 9 Spd=480 MxCh= 0
> D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
> P: Vendor=19d2 ProdID=1481 Rev=52.87
> S: Manufacturer=ZTE,Incorporated
> S: Product=ZTE Technologies MSM
> S: SerialNumber=1234567890ABCDEF
> C: #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=500mA
> I: If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
> I: If#= 1 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
Thanks for the patch. Looks good, but please fix up the two minor issues
pointed out below and resend, and I'll apply it after the merge window
closes.
> Signed-off-by: Yoshiaki Okamoto
> Signed-off-by: Hiroyuki Yamamoto
Since you are the one submitting the patch your SoB should go last. We
have a Co-developed-by tag which can you use to indicate co-authorship
(the second SoB is still required). The documentation for this was
recently updated in commit
24a2bb90741b ("docs: Clarify the usage and sign-off requirements for
Co-developed-by")
> ---
> drivers/usb/serial/option.c |2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
> index a0aaf0635359..e11ae2092229 100644
> --- a/drivers/usb/serial/option.c
> +++ b/drivers/usb/serial/option.c
> @@ -308,6 +308,7 @@ static void option_instat_callback(struct urb *urb);
> #define ZTE_PRODUCT_ME3620_MBIM 0x0426
> #define ZTE_PRODUCT_ME3620_X 0x1432
> #define ZTE_PRODUCT_ME3620_L 0x1433
> +#define ZTE_PRODUCT_MF871A 0x1481
> #define ZTE_PRODUCT_AC2726 0xfff1
> #define ZTE_PRODUCT_MG8800xfffd
> #define ZTE_PRODUCT_CDMA_TECH0xfffe
> @@ -1548,6 +1549,7 @@ static const struct usb_device_id option_ids[] = {
> { USB_DEVICE_AND_INTERFACE_INFO(ZTE_VENDOR_ID, 0x1428, 0xff, 0xff,
> 0xff), /* Telewell TW-LTE 4G v2 */
> .driver_info = RSVD(2) },
> { USB_DEVICE_INTERFACE_CLASS(ZTE_VENDOR_ID, 0x1476, 0xff) },/*
> GosunCn ZTE WeLink ME3630 (ECM/NCM mode) */
> + { USB_DEVICE_INTERFACE_CLASS(ZTE_VENDOR_ID, ZTE_PRODUCT_MF871A, 0xff) },
We're trying to move away from adding product-id defines, so please just
use a constant here as most ZTE entries do and add a short comment after
the entry. It's fine to go above 80 columns here even if checkpatch.pl
complains.
> { USB_DEVICE_AND_INTERFACE_INFO(ZTE_VENDOR_ID, 0x1533, 0xff, 0xff,
> 0xff) },
> { USB_DEVICE_AND_INTERFACE_INFO(ZTE_VENDOR_ID, 0x1534, 0xff, 0xff,
> 0xff) },
> { USB_DEVICE_AND_INTERFACE_INFO(ZTE_VENDOR_ID, 0x1535, 0xff, 0xff,
> 0xff) },
Thanks,
Johan
Re: [PATCH] [PATCH v7] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)
On Tue, Jul 02, 2019 at 08:30:06PM +0800, Charles Yeh wrote:
> Prolific has developed a new USB to UART chip: PL2303HXN
> PL2303HXN : PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE/PL2303GB
> The Vendor request used by the PL2303HXN (TYPE_HXN) is different from
> the existing PL2303 series (TYPE_HX & TYPE_01).
> Therefore, different Vendor requests are used to issue related commands.
> Signed-off-by: Charles Yeh
> ---
> changelog:
> v7:
> 1. Add PL2303_HXN_RESET_CONTROL_MASK define.
> 2. In pl2303_open,use PL2303_HXN_RESET_CONTROL_MASK & PL2303_HXN_RESET_CONTROL
>to reset the upstream and downstream pipe data
> 3. Ignore "WARNING: line over 80 characters" at #776,#782,#790
> #define PL2303_FLOWCTRL_MASK 0xf0
> +#define PL2303_HXN_FLOWCTRL_MASK 0x1C
> +#define PL2303_HXN_FLOWCTRL 0x0A
I asked you to keep related defines together (and to move the mask where
the register define was, not the other way round). Please move these to
the other HXN defines below, and keep the register address defines
before the corresponding bit defines.
> +#define PL2303_READ_TYPE_HX_STATUS 0x8080
> +
> +#define PL2303_HXN_RESET_CONTROL_MASK0x03
This makes no sense. The whole register is used for reset. If you want a
define that can be used for resetting both pipes then add two separate
defines for up and down respectively, and add a third define for
resetting both buffer as a bitwise OR of the two.
Remember that the code should be self-documenting as far as possible so
picking descriptive names is important.
Also move this one after the corresponding register address define
below.
> +#define PL2303_HXN_RESET_CONTROL 0x07
> +#define PL2303_HXN_CTRL_XON_XOFF 0x0C
> +#define PL2303_HXN_CTRL_RTS_CTS 0x18
> +#define PL2303_HXN_CTRL_NONE 0x1C
> @@ -765,8 +835,11 @@ static int pl2303_open(struct tty_struct *tty, struct
> usb_serial_port *port)
> if (spriv->quirks & PL2303_QUIRK_LEGACY) {
> usb_clear_halt(serial->dev, port->write_urb->pipe);
> usb_clear_halt(serial->dev, port->read_urb->pipe);
> - } else {
> + } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> /* reset upstream data pipes */
This comment belongs in the last else block. Your new code shouldn't
need one.
> + pl2303_update_reg(serial, PL2303_HXN_RESET_CONTROL,
> + PL2303_HXN_RESET_CONTROL_MASK, 0x03);
So two things; first, do you really need to read back the current value?
I would assume that it always reads back as 0 and that writing 0x03 in
this case would be sufficient to reset both buffers.
Second, please use a define for 0x03; no magic constants, as we have
discussed before. You don't need a separate mask define if you're always
resetting both buffers together (just use the same value define twice).
> + } else {
> pl2303_vendor_write(serial, 8, 0);
> pl2303_vendor_write(serial, 9, 0);
> }
Johan
Re: [PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)
[ Please do not top post, reply inline and trim irrelevant context.
Specifically do not copy context to the top of the mail.
Reordering your reply below. ]
On Wed, Jul 03, 2019 at 12:22:45AM +0800, Charles Yeh wrote:
> Johan Hovold 於 2019年7月1日 週一 下午11:29寫道：
> > On Mon, Jul 01, 2019 at 11:11:02PM +0800, Charles Yeh wrote:
> > > > } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> > > > > + pl2303_vendor_write(serial,
> > > > > PL2303_HXN_RESET_CONTROL,
> > > > > + 0);
> > > >
> > > > You again completely ignored my question about why you're wring 0
> > > > instead of 3 here.
> > > >
> > > > I'll ignore your patch until you explain.
> > >
> > > 3. In pl2303_open: Because TYPE_HXN is different from the instruction of
> > > reset
> > >down/up stream used by TYPE_HX.
> > >Therefore, we will also execute different instructions here.
> > >The default of chip Reset Control is 0xFF(TYPE_HXN), therefore we will
> > >write 0x00 to reset down/up stream(TYPE_HXN).
> >
> > I'm asking why you write the value 0 instead of 3 (or say, 0xfc)? Your
> > documentation said bit 0 and 1 are used to reset the up and downstream
> > pipes.
> >
> > To be more specific; what happens if I
> >
> > 1. set bit 0
> > 2. clear bit 0?
> >
> > and leave the other bits alone (write back the same value, e.g. 0xfe).
> You are right..
> set "1" is reset.
> set "0" is nothing.
>
> I have used pl2303_update_reg instead pl2303_vendor_write which to reset
> the upstream and downstream pipe data
Ok, thanks for confirming. Note that I asked you about this back in
April (15 April).
In that very same mail thread I also pointed out that you must not
ignore review feedback. Even if for some reason disagree with it you
should at least explain why.
Johan
[GIT PULL] USB-serial updates for 5.3-rc1
The following changes since commit 9e0babf2c06c73cda2c0cd37a1653d823adb40ec: Linux 5.2-rc5 (2019-06-16 08:49:45 -1000) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-5.3-rc1 for you to fetch changes up to f8377eff548170e8ea8022c067a1fbdf9e1c46a8: USB: serial: ftdi_sio: add ID for isodebug v1 (2019-06-28 17:12:35 +0200) USB-serial updates for 5.3-rc1 Here are the USB-serial updates for 5.3-rc1; just some new device ids this time. All have been in linux-next with no reported issues. Signed-off-by: Johan Hovold Andreas Fritiofson (1): USB: serial: ftdi_sio: add ID for isodebug v1 Jörgen Storvist (1): USB: serial: option: add support for GosunCn ME3630 RNDIS mode drivers/usb/serial/ftdi_sio.c | 1 + drivers/usb/serial/ftdi_sio_ids.h | 6 ++ drivers/usb/serial/option.c | 1 + 3 files changed, 8 insertions(+)
Re: [PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)
On Mon, Jul 01, 2019 at 11:11:02PM +0800, Charles Yeh wrote:
> > > + if (spriv->quirks & PL2303_QUIRK_LEGACY) {
> > > + pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
> > > + 0x40);
> >
> > No need to break this line even if you end up with slightly more than 80
> > chars.
> >
> OK. I will ignore "WARNING: line over 80 characters".
Yeah, it's ok to go slightly above 80 chars when it improves readability.
> > > + } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> > > + pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
> > > + PL2303_HXN_FLOWCTRL_MASK,
> > > + PL2303_HXN_CTRL_RTS_CTS);
> >
> > Again, continuation lines should be indented at least two tabs further
> > (you only use one tab now).
> >
>
> I have done it the way you do today...but after checking
> ./scripts/checkpatch.pl.. I got another warning message...
> So I am a little confused now...
> Previously before submitting.. must first pass ./scripts/checkpatch.pl
> check. No ERROR, or WARRING message...
>
> I will return to the office tomorrow ... I will post another warning
> message (according to the way you mentioned)
checkpatch isn't always right. Just remember to indent continuation
lines at least two tabs further, such as
pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
PL2303_HXN_FLOWCTRL_MASK,
PL2303_HXN_CTRL_RTS_CTS);
> > } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> >
> > > + pl2303_vendor_write(serial,
> > > PL2303_HXN_RESET_CONTROL,
> > > + 0);
> >
> > You again completely ignored my question about why you're wring 0
> > instead of 3 here.
> >
> > I'll ignore your patch until you explain.
>
> 3. In pl2303_open: Because TYPE_HXN is different from the instruction of reset
>down/up stream used by TYPE_HX.
>Therefore, we will also execute different instructions here.
>The default of chip Reset Control is 0xFF(TYPE_HXN), therefore we will
>write 0x00 to reset down/up stream(TYPE_HXN).
I'm asking why you write the value 0 instead of 3 (or say, 0xfc)? Your
documentation said bit 0 and 1 are used to reset the up and downstream
pipes.
To be more specific; what happens if I
1. set bit 0
2. clear bit 0?
and leave the other bits alone (write back the same value, e.g. 0xfe).
Johan
Re: [PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)
On Mon, Jul 01, 2019 at 03:32:33PM +0200, Johan Hovold wrote: > On Mon, Jul 01, 2019 at 08:21:14PM +0800, Charles Yeh wrote: > > #define PL2303_FLOWCTRL_MASK 0xf0 > > +#define PL2303_HXN_FLOWCTRL_MASK 0x1C > > Move after PL2303_HXN_RESET_CONTROL as I suggested. Sorry, I meant PL2303_HXN_FLOWCTRL here. > > +#define PL2303_READ_TYPE_HX_STATUS 0x8080 > > + > > +#define PL2303_HXN_RESET_CONTROL 0x07 > > +#define PL2303_HXN_FLOWCTRL0x0A > > +#define PL2303_HXN_CTRL_XON_XOFF 0x0C > > +#define PL2303_HXN_CTRL_RTS_CTS0x18 > > +#define PL2303_HXN_CTRL_NONE 0x1C Johan
Re: [PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)
On Mon, Jul 01, 2019 at 08:21:14PM +0800, Charles Yeh wrote:
> Prolific has developed a new USB to UART chip: PL2303HXN
> PL2303HXN : PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE/PL2303GB
> The Vendor request used by the PL2303HXN (TYPE_HXN) is different from
> the existing PL2303 series (TYPE_HX & TYPE_01).
> Therefore, different Vendor requests are used to issue related commands.
>
> 1. Added a new TYPE_HXN type in pl2303_type_data, and then executes
>new Vendor request,new flow control and other related instructions
>if TYPE_HXN is recognized.
>
> 2. Because the new PL2303HXN only accept the new Vendor request,
>the old Vendor request cannot be accepted (the error message
>will be returned)
>So first determine the TYPE_HX or TYPE_HXN through
>PL2303_READ_TYPE_HX_STATUS in pl2303_startup.
>
> 2.1 If the return message is "1", then the PL2303 is the existing
> TYPE_HX/ TYPE_01 series.
> The other settings in pl2303_startup are to continue execution.
> 2.2 If the return message is "not 1", then the PL2303 is the new
> TYPE_HXN series.
> The other settings in pl2303_startup are ignored.
> (PL2303HXN will directly use the default value in the hardware,
>no need to add additional settings through the software)
>
> 3. In pl2303_open: Because TYPE_HXN is different from the instruction of reset
>down/up stream used by TYPE_HX.
>Therefore, we will also execute different instructions here.
>The default of chip Reset Control is 0xFF(TYPE_HXN), therefore we will
>write 0x00 to reset down/up stream(TYPE_HXN).
>
> 4. In pl2303_set_termios: The UART flow control instructions used by
>TYPE_HXN/TYPE_HX/TYPE_01 are different.
>Therefore, we will also execute different instructions here.
>
> 5. In pl2303_vendor_read & pl2303_vendor_write, since TYPE_HXN is different
>from the vendor request instruction used by TYPE_HX/TYPE_01,
>it will also execute different instructions here.
>
> 6. In pl2303_update_reg: TYPE_HXN used different register for flow control.
>Therefore, we will also execute different instructions here.
>
> Signed-off-by: Charles Yeh
> ---
> changelog:
> v6:
> 1. Modify pl2303_update_reg:TYPE_HXN used different register for flow control.
>Therefore, we will also execute different instructions here.
> 2. Modify define name: PL2303_HXN_RESET_DOWN_UPSTREAM to
>PL2303_HXN_RESET_CONTROL
> 3. Re-Sorting flow-control register definition by address.
> 4. Indent continuation lines at least tw tabs.
>
> v5:
> 1. Modify pl2303_update_reg
> 2. add a patch version on subject
> 3. add a space after each colon at subject line
> ---
> drivers/usb/serial/pl2303.c | 127 +---
> drivers/usb/serial/pl2303.h | 7 +-
> 2 files changed, 108 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index d7abde14b3cf..d36d53b234c4 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -47,6 +47,12 @@ static const struct usb_device_id id_table[] = {
> { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_MOTOROLA) },
> { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_ZTEK) },
> { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_TB) },
> + { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GC) },
> + { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GB) },
> + { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GT) },
> + { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GL) },
> + { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GE) },
> + { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GS) },
> { USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID) },
> { USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID_RSAQ5) },
> { USB_DEVICE(ATEN_VENDOR_ID, ATEN_PRODUCT_ID),
> @@ -130,9 +136,11 @@ MODULE_DEVICE_TABLE(usb, id_table);
>
> #define VENDOR_WRITE_REQUEST_TYPE0x40
> #define VENDOR_WRITE_REQUEST 0x01
> +#define VENDOR_WRITE_NREQUEST0x80
>
> #define VENDOR_READ_REQUEST_TYPE 0xc0
> #define VENDOR_READ_REQUEST 0x01
> +#define VENDOR_READ_NREQUEST 0x81
>
> #define UART_STATE_INDEX 8
> #define UART_STATE_MSR_MASK 0x8b
> @@ -147,12 +155,22 @@ MODULE_DEVICE_TABLE(usb, id_table);
> #define UART_CTS 0x80
>
> #define PL2303_FLOWCTRL_MASK 0xf0
> +#define PL2303_HXN_FLOWCTRL_MASK 0x1C
Move after PL2303_HXN_RESET_CONTROL as I suggested.
> +#define PL2303_READ_TYPE_HX_STATUS 0x8080
> +
> +#define PL2303_HXN_RESET_CONTROL 0x07
> +#define PL2303_HXN_FLOWCTRL 0x0A
> +#define PL2303_HXN_CTRL_XON_XOFF 0x0C
> +#define PL2303_HXN_CTRL_RTS_CTS 0x18
> +#define PL2303_HXN_CTRL_NONE 0x1C
> @@ -719,14 +771,34 @@ static void pl2303_set_termios(struct tty_struct *tty,
> }
>
> if (C_CRTSCTS(tty)) {
> - if (spriv->
Re: [PATCH] [PATCH v5] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)
On Thu, Jun 13, 2019 at 09:45:44PM +0800, Charles Yeh wrote:
> Prolific has developed a new USB to UART chip: PL2303HXN
> PL2303HXN : PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE/PL2303GB
> The Vendor request used by the PL2303HXN (TYPE_HXN) is different from
> the existing PL2303 series (TYPE_HX & TYPE_01).
> Therefore, different Vendor requests are used to issue related commands.
>
> 1. Added a new TYPE_HXN type in pl2303_type_data, and then executes
>new Vendor request,new flow control and other related instructions
>if TYPE_HXN is recognized.
>
> 2. Because the new PL2303HXN only accept the new Vendor request,
>the old Vendor request cannot be accepted (the error message
>will be returned)
>So first determine the TYPE_HX or TYPE_HXN through
>PL2303_READ_TYPE_HX_STATUS in pl2303_startup.
>
> 2.1 If the return message is "1", then the PL2303 is the existing
> TYPE_HX/ TYPE_01 series.
> The other settings in pl2303_startup are to continue execution.
> 2.2 If the return message is "not 1", then the PL2303 is the new
> TYPE_HXN series.
> The other settings in pl2303_startup are ignored.
> (PL2303HXN will directly use the default value in the hardware,
>no need to add additional settings through the software)
>
> 3. In pl2303_open: Because TYPE_HXN is different from the instruction of reset
>down/up stream used by TYPE_HX.
>Therefore, we will also execute different instructions here.
>
> 4. In pl2303_set_termios: The UART flow control instructions used by
>TYPE_HXN/TYPE_HX/TYPE_01 are different.
>Therefore, we will also execute different instructions here.
>
> 5. In pl2303_vendor_read & pl2303_vendor_write, since TYPE_HXN is different
>from the vendor request instruction used by TYPE_HX/TYPE_01,
>it will also execute different instructions here.
>
> 6. In pl2303_update_reg: TYPE_HXN used different register for flow control.
>Therefore, we will also execute different instructions here.
>
> Signed-off-by: Charles Yeh
Sorry for not getting back to you on the clean ups yet. Just really
short on time these last few months.
We can merge your patch and I can just add those clean ups on top later.
But please fix the below first.
> ---
> changelog:
> v5:
> 1. Modify pl2303_update_reg
Surely you did more than just modify pl2303_update_reg (and that doesn't
explain how or why you did it).
Please be more specific in your changelogs.
> 2. Add a patch version on subject
> 3. Add a space after each colon at subject line
> ---
> drivers/usb/serial/pl2303.c | 113 +---
> drivers/usb/serial/pl2303.h | 7 ++-
> 2 files changed, 97 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index 55122ac84518..22ad82aa3894 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -47,6 +47,12 @@ static const struct usb_device_id id_table[] = {
> { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_MOTOROLA) },
> { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_ZTEK) },
> { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_TB) },
> + { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GC) },
> + { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GB) },
> + { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GT) },
> + { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GL) },
> + { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GE) },
> + { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GS) },
> { USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID) },
> { USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID_RSAQ5) },
> { USB_DEVICE(ATEN_VENDOR_ID, ATEN_PRODUCT_ID),
> @@ -129,9 +135,11 @@ MODULE_DEVICE_TABLE(usb, id_table);
>
> #define VENDOR_WRITE_REQUEST_TYPE0x40
> #define VENDOR_WRITE_REQUEST 0x01
> +#define VENDOR_WRITE_NREQUEST0x80
>
> #define VENDOR_READ_REQUEST_TYPE 0xc0
> #define VENDOR_READ_REQUEST 0x01
> +#define VENDOR_READ_NREQUEST 0x81
>
> #define UART_STATE_INDEX 8
> #define UART_STATE_MSR_MASK 0x8b
> @@ -146,12 +154,20 @@ MODULE_DEVICE_TABLE(usb, id_table);
> #define UART_CTS 0x80
>
> #define PL2303_FLOWCTRL_MASK 0xf0
> +#define PL2303_HXN_FLOWCTRL_MASK 0x1C
> +#define PL2303_READ_TYPE_HX_STATUS 0x8080
Add a newline here to separate the HXN register defines, and...
> +#define PL2303_HXN_FLOWCTRL 0x0A
...I'd move the HXN flow control mask here.
> +#define PL2303_HXN_CTRL_RTS_CTS 0x18
> +#define PL2303_HXN_CTRL_XON_XOFF 0x0C
> +#define PL2303_HXN_CTRL_NONE 0x1C
> +#define PL2303_HXN_RESET_DOWN_UPSTREAM 0x07
This register does more than reset the up- and downstream buffers. Your
documentation calls it "Chip reset control", so PL2303_HXN_RESET_CONTROL
might be a better name.
And move it before the flow-contro
Re: [PATCH] gpss: core: no waiters left behind on deregister
On Wed, Jun 26, 2019 at 01:04:07PM +0200, Oliver Neukum wrote:
> Am Dienstag, den 25.06.2019, 09:04 +0200 schrieb Johan Hovold:
> > On Mon, Jun 24, 2019 at 10:33:23AM +0200, Oliver Neukum wrote:
> > > If you deregister a device you need to wake up all waiters
> > > as there will be no further wakeups.
> > >
> > > Signed-off-by: Oliver Neukum
> > > ---
> > > drivers/gnss/core.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c
> > > index e6f94501cb28..0d13bd2cefd5 100644
> > > --- a/drivers/gnss/core.c
> > > +++ b/drivers/gnss/core.c
> > > @@ -303,7 +303,7 @@ void gnss_deregister_device(struct gnss_device *gdev)
> > > down_write(&gdev->rwsem);
> > > gdev->disconnected = true;
> > > if (gdev->count) {
> > > - wake_up_interruptible(&gdev->read_queue);
> > > + wake_up_interruptible_all(&gdev->read_queue);
> >
> > GNSS core doesn't have any exclusive waiters, so no need to use use the
> > exclusive wake-up (all) interface.
>
> Well, yes, but that is the problem. In gnss_read() you drop the lock:
> That means that an arbitrary number of tasks can get here.
>
> ret = wait_event_interruptible(gdev->read_queue,
> gdev->disconnected ||
> !kfifo_is_empty(&gdev->read_fifo));
>
> Meaning that an arbitrary number can be sleeping here.
I understand wait you're getting at, but I think your mistaken regarding
exclusive wait. Note that wait_event_interruptible() uses nonexclusive
wait.
> Yet in gnss_deregister_device() you use a simple wake_up:
>
> void gnss_deregister_device(struct gnss_device *gdev)
>
> {
>
> down_write(&gdev->rwsem);
> gdev->disconnected = true;
> if (gdev->count) {
> wake_up_interruptible(&gdev->read_queue);
>
>
> wake_up_interruptible() will wake up one waiting task. But after that
> the device is gone. There will be no further events. The other tasks
> will sleep forever.
No, wake_up_interruptible() will wake up all nonexclusive waiters,
which is all we care about here.
Johan
Re: [PATCH] gpss: core: no waiters left behind on deregister
On Mon, Jun 24, 2019 at 10:33:23AM +0200, Oliver Neukum wrote:
> If you deregister a device you need to wake up all waiters
> as there will be no further wakeups.
>
> Signed-off-by: Oliver Neukum
> ---
> drivers/gnss/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c
> index e6f94501cb28..0d13bd2cefd5 100644
> --- a/drivers/gnss/core.c
> +++ b/drivers/gnss/core.c
> @@ -303,7 +303,7 @@ void gnss_deregister_device(struct gnss_device *gdev)
> down_write(&gdev->rwsem);
> gdev->disconnected = true;
> if (gdev->count) {
> - wake_up_interruptible(&gdev->read_queue);
> + wake_up_interruptible_all(&gdev->read_queue);
GNSS core doesn't have any exclusive waiters, so no need to use use the
exclusive wake-up (all) interface.
> gdev->ops->close(gdev);
> }
> up_write(&gdev->rwsem);
Thanks,
Johan
Re: [usb:usb-testing 46/46] drivers/usb//misc/adutux.c:375:4: warning: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'size_t {aka unsigned int}'
On Thu, Jun 20, 2019 at 08:01:30AM -0700, dmg wrote:
>
> kbuild test robot writes:
>
> [...]
> >
> > All warnings (new ones prefixed by >>):
> >
> >In file included from include/linux/printk.h:332:0,
> > from include/linux/kernel.h:15,
> > from drivers/usb//misc/adutux.c:19:
> >drivers/usb//misc/adutux.c: In function 'adu_read':
> >>> drivers/usb//misc/adutux.c:375:4: warning: format '%lu' expects argument
> >>> of type 'long unsigned int', but argument 5 has type 'size_t {aka
> >>> unsigned int}' [-Wformat=]
> >"%s : while, data_in_secondary=%lu, status=%d\n",
>
> I am not sure what is the best way to address this warning.
>
> size_t seems to be architecture dependent. On my computer (intel64)
> size_t is long unsigned int, but in this test it is int unsigned.
>
> Are there any suggestions on what is the best way to deal with this?
You should use %zu.
Johan
Re: [PATCH] USB: serial: ch341: fix wrong baud rate setting calculation
On Sat, Jun 08, 2019 at 05:13:09PM +1200, jontio wrote:
> For some wanted baud rates ch341_set_baudrate_lcr() calculates the "a"
> value such that it produces a significantly different baud rate than the
> desired one. This means some hardware can't communicate with the CH34x
> chip. Particularly obvious wrong baud rates are 256000 and 921600 which
> deviate by 2.3% and 7.4% respectively. This proposed patch will bring the
> errors for these baud rates to below 0.5%. This patch will significantly
> improve the error of some other unusual baud rates too (such as 133
> from 10% error to 0% error). Currently ch341_set_baudrate_lcr() will
> accept any baud rate and can produce a practically arbitrary large error
> (for example a 40% error for 500) this patch does not address this
> issue.
It doesn't hurt to expand the commit message with further details from
your other mail.
> Signed-off-by: jontio
You need to sign off with your full name. It should also match the From
line (author).
> ---
> drivers/usb/serial/ch341.c | 45 ++
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 3bb1fff02bed..7cd1d6f70b56 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -54,6 +54,9 @@
> #define CH341_BAUDBASE_FACTOR 1532620800
> #define CH341_BAUDBASE_DIVMAX 3
>
> +/* Chip frequency is 12Mhz. not quite the same as (CH341_BAUDBASE_FACTOR>>7)
> */
> +#define CH341_OSC_FREQUENCY 1200
> +
> /* Break support - the information used to implement this was gleaned from
> * the Net/FreeBSD uchcom.c driver by Takanori Watanabe. Domo arigato.
> */
> @@ -168,6 +171,48 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
> factor = 0x1 - factor;
> a = (factor & 0xff00) | divisor;
>
> + /*
> + * Calculate baud error using the 0,1,2,3 LSB and
> + * also the error without the divisor (LSB==7).
> + * Decide whether the divisor should be used.
Wrap also comments at 72 cols or so.
> + */
> + uint32_t msB = (a>>8) & 0xFF;
> + uint32_t lsB = a & 0xFF;
> + int32_t baud_wanted = priv->baud_rate;
> + uint32_t denom = ((1<<(10-3*lsB))*(256-msB));
It's not obvious from just looking at the above chunk that 3*lsB < 10.
And some style issues:
- declare variables at the start of the function (or possibly start of
block), and defer non-trivial initialisation
- use the kernel types u32, s32 or plain (unsigned) int instead of the
c99 types.
- no camel case, msb, lsb is fine
- add a space on both sides of operators (also in your comments)
- drop the denom outmost parenthesis (also in some expressions below)
- please use lowercase hex notation for consistency with the rest of
the driver (function)
> + /*
> + * baud_wanted==(CH341_OSC_FREQUENCY/256) implies MSB==0 for no divisor
> + * the 100 is for rounding.
> + */
> + if (denom && ((baud_wanted+100) >=
> (((uint32_t)CH341_OSC_FREQUENCY)>>8))) {
> +
> + /* Calculate error for divisor */
> + int32_t baud_expected = ((uint32_t)CH341_OSC_FREQUENCY) / denom;
> + uint32_t baud_error_difference = abs(baud_expected-baud_wanted);
> +
> + /* Calculate a for no divisor */
> + uint32_t a_no_divisor =
> ((0x1-(((uint32_t)CH341_OSC_FREQUENCY)<<8) /
> + baud_wanted+128) & 0xFF00) | 0x07;
> +
> + /* a_no_divisor is only valid for MSB<248 */
> + if ((a_no_divisor>>8) < 248) {
> +
> + /* Calculate error for no divisor */
> + int32_t baud_expected_no_divisor =
> ((uint32_t)CH341_OSC_FREQUENCY) /
> + (256-(a_no_divisor>>8));
> + uint32_t baud_error_difference_no_divisor =
> + abs(baud_expected_no_divisor-baud_wanted);
> +
> + /*
> + * If error using no divisor is less than using
> + * a divisor then use it instead for the "a" word.
> + */
> + if (baud_error_difference_no_divisor <
> baud_error_difference)
> + a = a_no_divisor;
> + }
> +
Stray newline.
> + }
> +
> /*
>* CH341A buffers data until a full endpoint-size packet (32 bytes)
>* has been received unless bit 7 is set.
Ok, I'm gonna have to look at this again, but perhaps you can consider
the style input meanwhile.
Johan
Re: linux/drivers/usb/serial/ch341.c calculates some baud rates wrong
On Mon, Jun 10, 2019 at 02:32:16PM +1200, Jonathan Olds wrote:
> Hi Johan,
>
> Thanks for the info. I followed
> https://nickdesaulniers.github.io/blog/2017/05/16/submitting-your-first-patc
> h-to-the-linux-kernel-and-responding-to-feedback/ and made a proposal patch
> ("[PATCH] USB: serial: ch341: fix wrong baud rate setting calculation"
> https://lore.kernel.org/linux-usb/20190608051309.4689-1-jon...@i4free.co.nz/
> ).
Good job, looks like you got most things right from the start, but I'll
comment on the patch directly.
> I've measured the actual baud rates for a lot of given baud rates and I
> think I have deduced the formulas for calculating the "a" value. "a" is a
> uint16 and split up in two, a LSB and a MSB. The current driver only uses
> LSB from the set {0,1,2,3}. There is another valid LSB of 7 that the current
> driver doesn't use. The formula for LSB from the set {0,1,2,3} is...
>
> Actual baud rate == 2^(3*LSB-10)*1200/(256-MSB), if LSB is in {0,1,2,3}
> and 0
> When LSB == 7 then things are a bit different. LSB==7 seems to switch off
> the clock divider that the LSB usually does but only if MSB<248; when
> MSB>=248 the clock divider is turned back on and LSB is effectively 3 again.
> So we can also use the following formula...
>
> Actual baud rate == 1200/(256-MSB), if LSB == 7 and 0
> So the trick is to use these formulas to find a MSB and a LSB that produce
> and actual baud rate that are as close as possible to the desired baud rate.
> With errors greater than say 2 to 3% hardware will start to fail to
> communicate.
>
> Looking at some common baud rates only the higher rates are affected by not
> using a LSB of 7. Of the typical rates only 256000 and 921600 are affected.
> However other unusual frequencies are affected too such as 133 and I
> think you could calculate a lot more unusual affected frequencies. That
> being the case I think calculating the MSB when LSB == 7 for a given wanted
> baud rate might be a better solution than special cases for each affected
> baud rate.
Agreed.
> I've tested the patch with my hardware and it seems to work fine with every
> setting I could throw at it. I am aware that I've only tried it on my
> hardware with a CH340G chip. So trying with different chips and computers
> would be a good idea (I've tested it on the CH340G chip with two computers).
>
> My measurements/workings as a libre/open office calc file can be downloaded
> from https://jontio.github.io/linux_kernel_work/ch43x_tests.ods .
>
> I measured the following with the current driver (without my patch) using my
> scope...
>
> Baud wanted Baud measured Error as % of wanted
> 5050 0.0%
> 7575.20.3%
> 110 109.5 0.5%
> 135 134.6 0.3%
> 150 150.4 0.3%
> 300 300.8 0.3%
> 600 601.3 0.2%
> 1200 1201.9 0.2%
> 1800 1801.8 0.1%
> 2400 2403.8 0.2%
> 4800 4807.7 0.2%
> 7200 72150.2%
> 9600 9615.4 0.2%
> 14400 14430 0.2%
> 19200 19231 0.2%
> 38400 38462 0.2%
> 56000 56054 0.1%
> 57600 57837 0.4%
> 115200115207 0.0%
> 128000127551 0.4%
> 230400230415 0.0%
> 25600025 2.3%
> 460800460617 0.0%
> 921600853242 7.4%
> 100 999001 0.1%
> 133 1204819 9.6%
> 1843200 1496334 18.8%
> 200 1984127 0.8%
> 500 2985075 40.3%
>
> The patch will fix 256000, 133 and 921600 but not 1843200 and 500.
Nice job indeed, I think some of the above belongs in the commit as well.
I don't have time to dig into the details myself at the moment, but I'll
point out some minor issues with you patch meanwhile.
Thanks,
Johan
Re: [PATCH] usb: xhci: dbc: get rid of global pointer
On Wed, Jun 19, 2019 at 09:33:40AM +0300, Felipe Balbi wrote: > Johan Hovold writes: > > Are you sure you actually did register two xhci debug ttys? > > hmm, let me check: ... > Hmm, so it only really registers after writing to sysfs file. Man, this > is an odd driver :-) I hear you. :) > @Mathias, can you drop the previous fix? I'll try to come up with a > better version of this. > > @Johan, thanks for the review. You're welcome. Johan signature.asc Description: PGP signature
Re: [PATCH] USB: serial: option: add support for GosunCn ME3630 RNDIS mode
On Wed, Jun 19, 2019 at 12:30:19AM +0200, Jörgen Storvist wrote: > Added USB IDs for GosunCn ME3630 cellular module in RNDIS mode. > > T: Bus=03 Lev=01 Prnt=01 Port=01 Cnt=03 Dev#= 18 Spd=480 MxCh= 0 > D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1 > P: Vendor=19d2 ProdID=0601 Rev=03.18 > S: Manufacturer=Android > S: Product=Android > S: SerialNumber=b950269c > C: #Ifs= 5 Cfg#= 1 Atr=a0 MxPwr=500mA > I: If#=0x0 Alt= 0 #EPs= 1 Cls=e0(wlcon) Sub=01 Prot=03 Driver=rndis_host > I: If#=0x1 Alt= 0 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=rndis_host > I: If#=0x2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option > I: If#=0x3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option > I: If#=0x4 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option > > Signed-off-by: Jörgen Storvist Applied, thanks. Johan
Re: [PATCH] usb: xhci: dbc: get rid of global pointer
On Mon, Jun 17, 2019 at 09:43:19AM +0300, Felipe Balbi wrote:
>
> Hi,
>
> Johan Hovold writes:
> > On Tue, Jun 11, 2019 at 08:24:16PM +0300, Felipe Balbi wrote:
> >> If we happen to have two XHCI controllers with DbC capability, then
> >> there's no hope this will ever work as the global pointer will be
> >> overwritten by the controller that probes last.
> >>
> >> Avoid this problem by keeping the tty_driver struct pointer inside
> >> struct xhci_dbc.
> >
> > How did you test this patch?
>
> by running it on a machine that actually has two DbCs
>
> >> @@ -279,52 +279,52 @@ static const struct tty_operations dbc_tty_ops = {
> >>.unthrottle = dbc_tty_unthrottle,
> >> };
> >>
> >> -static struct tty_driver *dbc_tty_driver;
> >> -
> >> int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
> >> {
> >>int status;
> >>struct xhci_dbc *dbc = xhci->dbc;
> >>
> >> - dbc_tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
> >> + dbc->tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
> >> TTY_DRIVER_DYNAMIC_DEV);
> >> - if (IS_ERR(dbc_tty_driver)) {
> >> - status = PTR_ERR(dbc_tty_driver);
> >> - dbc_tty_driver = NULL;
> >> + if (IS_ERR(dbc->tty_driver)) {
> >> + status = PTR_ERR(dbc->tty_driver);
> >> + dbc->tty_driver = NULL;
> >>return status;
> >>}
> >>
> >> - dbc_tty_driver->driver_name = "dbc_serial";
> >> - dbc_tty_driver->name = "ttyDBC";
> >> + dbc->tty_driver->driver_name = "dbc_serial";
> >> + dbc->tty_driver->name = "ttyDBC";
> >
> > You're now registering multiple drivers for the same thing (and wasting
> > a major number for each) and specifically using the same name, which
> > should lead to name clashes when registering the second port.
>
> No warnings were printed while running this, actually. Odd
Odd indeed. I get the expected warning from sysfs when trying to
register a second tty using an already registered name:
[ 643.360555] sysfs: cannot create duplicate filename '/class/tty/ttyS0'
[ 643.360637] CPU: 1 PID: 2383 Comm: modprobe Not tainted 5.2.0-rc1 #2
[ 643.360702] Hardware name: /D34010WYK, BIOS
WYLPT10H.86A.0051.2019.0322.1320 03/22/2019
[ 643.360784] Call Trace:
[ 643.360823] dump_stack+0x46/0x60
[ 643.360865] sysfs_warn_dup.cold.3+0x17/0x2f
[ 643.360914] sysfs_do_create_link_sd.isra.2+0xa6/0xc0
[ 643.360961] device_add+0x30d/0x660
[ 643.360987] tty_register_device_attr+0xdd/0x1d0
[ 643.361018] ? sysfs_create_file_ns+0x5d/0x90
[ 643.361049] usb_serial_device_probe+0x72/0xf0 [usbserial]
...
Are you sure you actually did register two xhci debug ttys?
Johan
signature.asc
Description: PGP signature
Re: [PATCH] usb: xhci: dbc: get rid of global pointer
On Tue, Jun 11, 2019 at 08:24:16PM +0300, Felipe Balbi wrote:
> If we happen to have two XHCI controllers with DbC capability, then
> there's no hope this will ever work as the global pointer will be
> overwritten by the controller that probes last.
>
> Avoid this problem by keeping the tty_driver struct pointer inside
> struct xhci_dbc.
How did you test this patch?
> Fixes: dfba2174dc42 usb: xhci: Add DbC support in xHCI driver
> Cc: # v4.16+
> Signed-off-by: Felipe Balbi
> ---
> drivers/usb/host/xhci-dbgcap.c | 4 +--
> drivers/usb/host/xhci-dbgcap.h | 3 +-
> drivers/usb/host/xhci-dbgtty.c | 54 +-
> 3 files changed, 31 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
> index 52e32644a4b2..5f56b650c0ea 100644
> --- a/drivers/usb/host/xhci-dbgcap.c
> +++ b/drivers/usb/host/xhci-dbgcap.c
> @@ -948,7 +948,7 @@ int xhci_dbc_init(struct xhci_hcd *xhci)
> return 0;
>
> init_err1:
> - xhci_dbc_tty_unregister_driver();
> + xhci_dbc_tty_unregister_driver(xhci);
> init_err2:
> xhci_do_dbc_exit(xhci);
> init_err3:
> @@ -963,7 +963,7 @@ void xhci_dbc_exit(struct xhci_hcd *xhci)
> return;
>
> device_remove_file(dev, &dev_attr_dbc);
> - xhci_dbc_tty_unregister_driver();
> + xhci_dbc_tty_unregister_driver(xhci);
> xhci_dbc_stop(xhci);
> xhci_do_dbc_exit(xhci);
> }
> diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h
> index ce0c6072bd48..30dedf36c566 100644
> --- a/drivers/usb/host/xhci-dbgcap.h
> +++ b/drivers/usb/host/xhci-dbgcap.h
> @@ -151,6 +151,7 @@ struct xhci_dbc {
> struct dbc_ep eps[2];
>
> struct dbc_port port;
> + struct tty_driver *tty_driver;
> };
>
> #define dbc_bulkout_ctx(d) \
> @@ -196,7 +197,7 @@ static inline struct dbc_ep *get_out_ep(struct xhci_hcd
> *xhci)
> int xhci_dbc_init(struct xhci_hcd *xhci);
> void xhci_dbc_exit(struct xhci_hcd *xhci);
> int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci);
> -void xhci_dbc_tty_unregister_driver(void);
> +void xhci_dbc_tty_unregister_driver(struct xhci_hcd *xhci);
> int xhci_dbc_tty_register_device(struct xhci_hcd *xhci);
> void xhci_dbc_tty_unregister_device(struct xhci_hcd *xhci);
> struct dbc_request *dbc_alloc_request(struct dbc_ep *dep, gfp_t gfp_flags);
> diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
> index aff79ff5aba4..300fc770a0d5 100644
> --- a/drivers/usb/host/xhci-dbgtty.c
> +++ b/drivers/usb/host/xhci-dbgtty.c
> @@ -279,52 +279,52 @@ static const struct tty_operations dbc_tty_ops = {
> .unthrottle = dbc_tty_unthrottle,
> };
>
> -static struct tty_driver *dbc_tty_driver;
> -
> int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
> {
> int status;
> struct xhci_dbc *dbc = xhci->dbc;
>
> - dbc_tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
> + dbc->tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
> TTY_DRIVER_DYNAMIC_DEV);
> - if (IS_ERR(dbc_tty_driver)) {
> - status = PTR_ERR(dbc_tty_driver);
> - dbc_tty_driver = NULL;
> + if (IS_ERR(dbc->tty_driver)) {
> + status = PTR_ERR(dbc->tty_driver);
> + dbc->tty_driver = NULL;
> return status;
> }
>
> - dbc_tty_driver->driver_name = "dbc_serial";
> - dbc_tty_driver->name = "ttyDBC";
> + dbc->tty_driver->driver_name = "dbc_serial";
> + dbc->tty_driver->name = "ttyDBC";
You're now registering multiple drivers for the same thing (and wasting
a major number for each) and specifically using the same name, which
should lead to name clashes when registering the second port.
Possibly better than the current situation, but why not fix this
properly instead? Register the driver once, and just pick a new minor
number for each controller.
> - dbc_tty_driver->type = TTY_DRIVER_TYPE_SERIAL;
> - dbc_tty_driver->subtype = SERIAL_TYPE_NORMAL;
> - dbc_tty_driver->init_termios = tty_std_termios;
> - dbc_tty_driver->init_termios.c_cflag =
> + dbc->tty_driver->type = TTY_DRIVER_TYPE_SERIAL;
> + dbc->tty_driver->subtype = SERIAL_TYPE_NORMAL;
> + dbc->tty_driver->init_termios = tty_std_termios;
> + dbc->tty_driver->init_termios.c_cflag =
> B9600 | CS8 | CREAD | HUPCL | CLOCAL;
> - dbc_tty_driver->init_termios.c_ispeed = 9600;
> - dbc_tty_driver->init_termios.c_ospeed = 9600;
> - dbc_tty_driver->driver_state = &dbc->port;
> + dbc->tty_driver->init_termios.c_ispeed = 9600;
> + dbc->tty_driver->init_termios.c_ospeed = 9600;
> + dbc->tty_driver->driver_state = &dbc->port;
>
> - tty_set_operations(dbc_tty_driver, &dbc_tty_ops);
> + tty_set_operations(dbc->tty_driver, &dbc_tty_ops);
>
> - status = tty
Re: [PATCH] USB:serial:pl2303:Add new PID to support PL2303HXN (TYPE_HXN)
On Wed, Jun 12, 2019 at 09:18:41PM +0800, Charles Yeh wrote: > Prolific has developed a new USB to UART chip: PL2303HXN > PL2303HXN : PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE/PL2303GB > The Vendor request used by the PL2303HXN (TYPE_HXN) is different from > the existing PL2303 series (TYPE_HX & TYPE_01). > Therefore, different Vendor requests are used to issue related commands. > Signed-off-by: Charles Yeh > --- As I've asked you several times already, make sure to - add a patch version to your subject (e.g. [PATCH v5], or which version number you're currently at) - include a changelog here, below the --- line, so we know what changed since previous versions - fix up your subject line (add a space after each colon) Please fix up and resend. Thanks, Johan
[GIT PULL] USB-serial fixes for 5.2-rc5
The following changes since commit a188339ca5a396acc588e5851ed7e19f66b0ebd9: Linux 5.2-rc1 (2019-05-19 15:47:09 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-5.2-rc5 for you to fetch changes up to f3dfd4072c3ee6e287f501a18b5718b185d6a940: USB: serial: option: add Telit 0x1260 and 0x1261 compositions (2019-05-21 11:27:24 +0200) USB-serial fixes for 5.2-rc5 Here are some new device ids for option and pl2303. All have been in linux-next with no reported issues. Signed-off-by: Johan Hovold Chris Packham (1): USB: serial: pl2303: add Allied Telesis VT-Kit3 Daniele Palmas (1): USB: serial: option: add Telit 0x1260 and 0x1261 compositions Jörgen Storvist (1): USB: serial: option: add support for Simcom SIM7500/SIM7600 RNDIS mode drivers/usb/serial/option.c | 6 ++ drivers/usb/serial/pl2303.c | 1 + drivers/usb/serial/pl2303.h | 3 +++ 3 files changed, 10 insertions(+)
Re: [PATCH] usb: chipidea: Use dev_err() instead of pr_err()
On Mon, Jun 03, 2019 at 11:09:01PM -0300, Fabio Estevam wrote:
> dev_err() is more appropriate for printing error messages inside
> drivers, so switch to dev_err().
>
> Signed-off-by: Fabio Estevam
> ---
> drivers/usb/chipidea/core.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 27749ace2d93..1b6495829265 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -523,8 +523,9 @@ int hw_device_reset(struct ci_hdrc *ci)
> hw_write(ci, OP_USBMODE, USBMODE_SLOM, USBMODE_SLOM);
>
> if (hw_read(ci, OP_USBMODE, USBMODE_CM) != USBMODE_CM_DC) {
> - pr_err("cannot enter in %s device mode", ci_role(ci)->name);
> - pr_err("lpm = %i", ci->hw_bank.lpm);
> + dev_err(ci->dev, "cannot enter in %s device mode",
> + ci_role(ci)->name);
> + dev_err(ci->dev, "lpm = %i", ci->hw_bank.lpm);
Please also add the missing newlines '\n' (and check if there are more
instances of that mistake in this driver).
> return -ENODEV;
> }
Johan
