diff mbox series

[SRU,Eoan/raspi2,1/1] UBUNTU: SAUCE: dwc_otg: checking the urb->transfer_buffer too early (#3332)

Message ID 20191120141856.4890-2-hui.wang@canonical.com
State New
Headers show
Series IO errors when writing large amounts of data to USB storage in eoan on RPI2/3 (armhf kernel) | expand

Commit Message

Hui Wang Nov. 20, 2019, 2:18 p.m. UTC
BugLink: https://bugs.launchpad.net/bugs/1852510

After enable the HIGHMEM and VMSPLIT_3G, the dwc_otg driver doesn't
work well on Pi2/3 boards with 1G physical ram. Users experience
the failure when copying a file of 600M size to the USB stick. And
at the same time, the dmesg shows:
usb 1-1.1.2: reset high-speed USB device number 8 using dwc_otg
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_ERROR driverbyte=DRIVER_OK
blk_update_request: I/O error, dev sda, sector 3024048 op 0x1:(WRITE) flags 0x4000 phys_seg 15 prio class 0

When this happens, the sg_buf sent to the driver is located in the
highmem region, the usb_sg_init() in the core/message.c will leave
transfer_buffer to NULL if the sg_buf is in highmem, but in the
dwc_otg driver, it returns -EINVAL unconditionally if transfer_buffer
is NULL.

The driver can handle the situation of buffer to be NULL, if it is in
DMA mode, it will convert an address from transfer_dma.

But if the conversion fails or it is in the PIO mode, we should check
buffer and return -EINVAL if it is NULL.

https://github.com/raspberrypi/linux/pull/3341
BugLink: https://bugs.launchpad.net/bugs/1852510
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Connor Kuehl Nov. 20, 2019, 4:24 p.m. UTC | #1
On 11/20/19 6:18 AM, Hui Wang wrote:
> BugLink: https://bugs.launchpad.net/bugs/1852510
> 
> After enable the HIGHMEM and VMSPLIT_3G, the dwc_otg driver doesn't
> work well on Pi2/3 boards with 1G physical ram. Users experience
> the failure when copying a file of 600M size to the USB stick. And
> at the same time, the dmesg shows:
> usb 1-1.1.2: reset high-speed USB device number 8 using dwc_otg
> sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_ERROR driverbyte=DRIVER_OK
> blk_update_request: I/O error, dev sda, sector 3024048 op 0x1:(WRITE) flags 0x4000 phys_seg 15 prio class 0
> 
> When this happens, the sg_buf sent to the driver is located in the
> highmem region, the usb_sg_init() in the core/message.c will leave
> transfer_buffer to NULL if the sg_buf is in highmem, but in the
> dwc_otg driver, it returns -EINVAL unconditionally if transfer_buffer
> is NULL.
> 
> The driver can handle the situation of buffer to be NULL, if it is in
> DMA mode, it will convert an address from transfer_dma.
> 
> But if the conversion fails or it is in the PIO mode, we should check
> buffer and return -EINVAL if it is NULL.
> 
> https://github.com/raspberrypi/linux/pull/3341
> BugLink: https://bugs.launchpad.net/bugs/1852510
> Signed-off-by: Hui Wang <hui.wang@canonical.com>

Positive test results on multiple boards with the 1 image. This can be 
fixed when applying, but the BugLink is listed twice in the commit message.

Acked-by: Connor Kuehl <connor.kuehl@canonical.com>

> ---
>   drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c b/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
> index 08a3e41038a3..ffa8cb2ad49a 100644
> --- a/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
> +++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
> @@ -821,10 +821,6 @@ static int dwc_otg_urb_enqueue(struct usb_hcd *hcd,
>   		dump_urb_info(urb, "dwc_otg_urb_enqueue");
>   	}
>   #endif
> -
> -	if (!urb->transfer_buffer && urb->transfer_buffer_length)
> -		return -EINVAL;
> -
>   	if ((usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS)
>   	    || (usb_pipetype(urb->pipe) == PIPE_INTERRUPT)) {
>   		if (!dwc_otg_hcd_is_bandwidth_allocated
> @@ -881,6 +877,13 @@ static int dwc_otg_urb_enqueue(struct usb_hcd *hcd,
>   			      &urb->transfer_dma, buf);
>   	}
>   
> +	if (!buf && urb->transfer_buffer_length) {
> +		DWC_FREE(dwc_otg_urb);
> +		DWC_ERROR("transfer_buffer is NULL in PIO mode or both "
> +			   "transfer_buffer and transfer_dma are NULL in DMA mode\n");
> +		return -EINVAL;
> +	}
> +
>   	if (!(urb->transfer_flags & URB_NO_INTERRUPT))
>   		flags |= URB_GIVEBACK_ASAP;
>   	if (urb->transfer_flags & URB_ZERO_PACKET)
>
Stefan Bader Nov. 20, 2019, 4:30 p.m. UTC | #2
On 20.11.19 15:18, Hui Wang wrote:
> BugLink: https://bugs.launchpad.net/bugs/1852510
> 
> After enable the HIGHMEM and VMSPLIT_3G, the dwc_otg driver doesn't
> work well on Pi2/3 boards with 1G physical ram. Users experience
> the failure when copying a file of 600M size to the USB stick. And
> at the same time, the dmesg shows:
> usb 1-1.1.2: reset high-speed USB device number 8 using dwc_otg
> sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_ERROR driverbyte=DRIVER_OK
> blk_update_request: I/O error, dev sda, sector 3024048 op 0x1:(WRITE) flags 0x4000 phys_seg 15 prio class 0
> 
> When this happens, the sg_buf sent to the driver is located in the
> highmem region, the usb_sg_init() in the core/message.c will leave
> transfer_buffer to NULL if the sg_buf is in highmem, but in the
> dwc_otg driver, it returns -EINVAL unconditionally if transfer_buffer
> is NULL.
> 
> The driver can handle the situation of buffer to be NULL, if it is in
> DMA mode, it will convert an address from transfer_dma.
> 
> But if the conversion fails or it is in the PIO mode, we should check
> buffer and return -EINVAL if it is NULL.
> 
> https://github.com/raspberrypi/linux/pull/3341
> BugLink: https://bugs.launchpad.net/bugs/1852510
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---

Testing looks good. We can drop the additional buglink when committing.

>  drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c b/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
> index 08a3e41038a3..ffa8cb2ad49a 100644
> --- a/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
> +++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
> @@ -821,10 +821,6 @@ static int dwc_otg_urb_enqueue(struct usb_hcd *hcd,
>  		dump_urb_info(urb, "dwc_otg_urb_enqueue");
>  	}
>  #endif
> -
> -	if (!urb->transfer_buffer && urb->transfer_buffer_length)
> -		return -EINVAL;
> -
>  	if ((usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS)
>  	    || (usb_pipetype(urb->pipe) == PIPE_INTERRUPT)) {
>  		if (!dwc_otg_hcd_is_bandwidth_allocated
> @@ -881,6 +877,13 @@ static int dwc_otg_urb_enqueue(struct usb_hcd *hcd,
>  			      &urb->transfer_dma, buf);
>  	}
>  
> +	if (!buf && urb->transfer_buffer_length) {
> +		DWC_FREE(dwc_otg_urb);
> +		DWC_ERROR("transfer_buffer is NULL in PIO mode or both "
> +			   "transfer_buffer and transfer_dma are NULL in DMA mode\n");
> +		return -EINVAL;
> +	}
> +
>  	if (!(urb->transfer_flags & URB_NO_INTERRUPT))
>  		flags |= URB_GIVEBACK_ASAP;
>  	if (urb->transfer_flags & URB_ZERO_PACKET)
>
diff mbox series

Patch

diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c b/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
index 08a3e41038a3..ffa8cb2ad49a 100644
--- a/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
+++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
@@ -821,10 +821,6 @@  static int dwc_otg_urb_enqueue(struct usb_hcd *hcd,
 		dump_urb_info(urb, "dwc_otg_urb_enqueue");
 	}
 #endif
-
-	if (!urb->transfer_buffer && urb->transfer_buffer_length)
-		return -EINVAL;
-
 	if ((usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS)
 	    || (usb_pipetype(urb->pipe) == PIPE_INTERRUPT)) {
 		if (!dwc_otg_hcd_is_bandwidth_allocated
@@ -881,6 +877,13 @@  static int dwc_otg_urb_enqueue(struct usb_hcd *hcd,
 			      &urb->transfer_dma, buf);
 	}
 
+	if (!buf && urb->transfer_buffer_length) {
+		DWC_FREE(dwc_otg_urb);
+		DWC_ERROR("transfer_buffer is NULL in PIO mode or both "
+			   "transfer_buffer and transfer_dma are NULL in DMA mode\n");
+		return -EINVAL;
+	}
+
 	if (!(urb->transfer_flags & URB_NO_INTERRUPT))
 		flags |= URB_GIVEBACK_ASAP;
 	if (urb->transfer_flags & URB_ZERO_PACKET)