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
[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)
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 Charles.
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
Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
On Tue, 24 Sep 2019, David Heinzelmann wrote: > > I really don't understand this. > > > > Your patch involves the case where there was a connect-change event but > > the port is still enabled. Now maybe I've forgotten about one of the > > pathways, but it seems like that combination should never occur. > > > > Certainly it shouldn't occur in your case. The device disconnects and > > then reconnects with a new set of descriptors. The disconnect should > > cause the port to be disabled, and the port should remain disabled > > after the reconnect occurs. So how can your new code run in the first > > place? > > > > Alan Stern > > > > Hi, > > I have a log with two devices which are connected to a hub and the hub is > plugged in. > > The device which is not working in this log: > > Sep 24 08:32:21 kernel: usb 2-6-port1: status 0203 change 0011 > Sep 24 08:32:21 kernel: usb 2-6.1: new SuperSpeed Gen 1 USB device number 65 > using xhci_hcd Ah, SuperSpeed. You're using a USB-3 device. That does make a difference. > Sep 24 08:32:21 kernel: usb 2-6.1: udev 65, busnum 2, minor = 192 > Sep 24 08:32:21 kernel: usb 2-6.1: New USB device found, idVendor=1409, > idProduct=3240, bcdDevice= 0.00 > Sep 24 08:32:21 kernel: usb 2-6.1: New USB device strings: Mfr=1, Product=2, > SerialNumber=0 > Sep 24 08:32:21 kernel: usb 2-6.1: Product: USB 3.0 Camera > Sep 24 08:32:21 kernel: usb 2-6.1: Manufacturer: Camera Manufacturer > > Now the firmware download happens and the device is re-enumerating and a > disconnect/connect should occur. > But the only change which is seen is the following output: > > Sep 24 08:32:23 kernel: usb 2-6-port1: link state change > Sep 24 08:32:23 kernel: usb 2-6-port1: status 0203, change 0041, 5.0 Gb/s > > Now the resuscitation is happening but from my understanding this is not > correct as in the reality there was a > reconnect from the device. So I tried to initiate a device reconnect if the > device descriptor changed. > > It also seems to me that the enumeration from the second device (usb > 2-6-port1) is blocking > the port change event and so the actual disconnect is missed. Now it all makes sense. Yes, I agree that your patch is the appropriate thing to do -- except that it contains at least one logic error: It doesn't handle the return code from usb_get_device_descriptor() properly. Also, I think you should expand the immediately preceding comment. Explain that it is indeed possible for the port to be enabled at this point, because USB-3 connections are initialized automatically by the host controller hardware when the connection is detected. Alan Stern
Re: BUG report: usb: dwc3: Link TRB triggered an intterupt without IOC being setted
On 24.9.2019 17.45, alex zheng wrote: Hi Mathias, I try to ignore the DMA errors, then the transfer continues but it complete with data lost, it seems like these ERROR Transfer event should be right and must not be ignore. test app show: "did not get enough data, received size:14410176/1500" kernel log show: (you can see more detail info in the attached log files) Logs show your transfer ring has four segments, but hardware fails to jump from the last segment back to first) Last TRB (LINK TRB) of each segment points to the next segment, last segments link trb points back to first segment. In your case: 0x1d117000 -> 0x1eb09000 -> 0x1eb0a000 -> 0x1dbda000 -> (back to 0x1d117000) For some reason your hardware doesn't treat the last TRB at the last segment as a LINK TRB, instead it just issues a transfer event for it, and continues to the next address instead of jumping back to first segment: Transfer event for last TRB at last segment: 0x1dbda000 (TRB: 0x1dbdaff0): This is a link TRB and should not generate transfer event: xhci-hcd.0.auto: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 16 comp_code 1 xhci-hcd xhci-hcd.0.auto: Looking for event-dma 1dbdaff0 trb-start 1d117000 trb-end 1d117000 seg-start 1d117000 seg-end 1d10 xhci-hcd xhci-hcd.0.auto: Ignoring error Next transfer event should be for TRB at fisrt segment (0x1d117000) but event shows its trying to handle a event from TRB at 1dbdb000, which isn't even part of the ring. xhci-hcd xhci-hcd.0.auto: process trans event : ep_index = 16, event_dma = 1dbdb000 xhci-hcd xhci-hcd.0.auto: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 16 comp_code 1 xhci-hcd xhci-hcd.0.auto: Looking for event-dma 1dbdb000 trb-start 1d117000 trb-end 1d117000 seg-start 1d117000 seg-end 1d10 xhci-hcd xhci-hcd.0.auto: Ignoring error -Mathias
RE: BUG report: usb: dwc3: Link TRB triggered an intterupt without IOC being setted
From: Mathias Nyman > Sent: 25 September 2019 15:48 > > On 24.9.2019 17.45, alex zheng wrote: > > Hi Mathias, ... > Logs show your transfer ring has four segments, but hardware fails to > jump from the last segment back to first) > > Last TRB (LINK TRB) of each segment points to the next segment, > last segments link trb points back to first segment. > > In your case: > 0x1d117000 -> 0x1eb09000 -> 0x1eb0a000 -> 0x1dbda000 -> (back to 0x1d117000) > > For some reason your hardware doesn't treat the last TRB at the last segment > as a LINK TRB, instead it just issues a transfer event for it, and continues > to > the next address instead of jumping back to first segment: That could be a cache coherency (or flushing (etc)) issue. >> This is our self-design platform (ARM v7 cpu core with synopsys DWC USB3.0 >> controller). Or maybe your hardware is just getting some of the memory accesses wrong? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Pheckee23
带开发瓢 +Ⅴ :ⓀⓕⓅ⒎⒎⒍⒏
RE: BUG report: usb: dwc3: Link TRB triggered an intterupt without IOC being setted
Hi, David Laight writes: > From: Mathias Nyman >> Sent: 25 September 2019 15:48 >> >> On 24.9.2019 17.45, alex zheng wrote: >> > Hi Mathias, > ... >> Logs show your transfer ring has four segments, but hardware fails to >> jump from the last segment back to first) >> >> Last TRB (LINK TRB) of each segment points to the next segment, >> last segments link trb points back to first segment. >> >> In your case: >> 0x1d117000 -> 0x1eb09000 -> 0x1eb0a000 -> 0x1dbda000 -> (back to 0x1d117000) >> >> For some reason your hardware doesn't treat the last TRB at the last segment >> as a LINK TRB, instead it just issues a transfer event for it, and continues >> to >> the next address instead of jumping back to first segment: > > That could be a cache coherency (or flushing (etc)) issue. XHCI has a HW-configurable maximum number of segments in a ring. AFAICT, xhci driver doesn't take that into consideration today. Perhaps the HW in question doesn't like more than 3 segments. Mathias, what was the register to check this? Do you remember? -- balbi signature.asc Description: PGP signature
[PATCH -next] usb: cdns3: Fix sheduling with locks held.
Patch fix issue in cdns3_ep0_feature_handle_device function. The function usleep_range can't be used there because this function is called with locks held and IRQs disabled in cdns3_device_thread_irq_handler(). To resolve this issue patch replaces usleep_range with mdelay. Signed-off-by: Pawel Laszczak --- drivers/usb/cdns3/ep0.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c index 44f652e8b5a2..0445da0a5a0c 100644 --- a/drivers/usb/cdns3/ep0.c +++ b/drivers/usb/cdns3/ep0.c @@ -332,7 +332,7 @@ static int cdns3_ep0_feature_handle_device(struct cdns3_device *priv_dev, * for sending status stage. * This time should be less then 3ms. */ - usleep_range(1000, 2000); + mdelay(1); cdns3_set_register_bit(&priv_dev->regs->usb_cmd, USB_CMD_STMODE | USB_STS_TMODE_SEL(tmode - 1)); -- 2.17.1
Re: [PATCH -next] usb: cdns3: Fix sheduling with locks held.
On Thu, Sep 26, 2019 at 07:19:27AM +0100, Pawel Laszczak wrote: > Patch fix issue in cdns3_ep0_feature_handle_device function. > > The function usleep_range can't be used there because this function is > called with locks held and IRQs disabled in > cdns3_device_thread_irq_handler(). > > To resolve this issue patch replaces usleep_range with mdelay. > > Signed-off-by: Pawel Laszczak What commit does this fix? Did someone report it? If so, please properly credit them here. thanks, greg k-h
