RE: [PATCH v2] usb:musb:musb_host: Handle highmem in PIO mode
> -Original Message- > From: Alan Stern [mailto:st...@rowland.harvard.edu] > Sent: Tuesday, August 07, 2012 11:17 PM > To: Virupax SADASHIVPETIMATH > Cc: ba...@ti.com; gre...@linuxfoundation.org; linux-...@vger.kernel.org; > linux- > ker...@vger.kernel.org; linus.wall...@linaro.org; Praveena NADAHALLY; Rajaram > REGUPATHY; Vikrant BAPAT > Subject: Re: [PATCH v2] usb:musb:musb_host: Handle highmem in PIO mode > > > > +*/ > > + if (!urb->transfer_buffer) > > + use_sg = true; > > Here you test urb->transfer_buffer. I will make the test as if (!use_sg && !urb->transfer_buffer) use_sg = true; > > + if (use_sg) { > > + /* sg_miter_start is already done in musb_ep_program */ > > + if (!sg_miter_next(>sg_miter)) { > > + dev_err(musb->controller, "error: sg list empty\n"); > > + sg_miter_stop(>sg_miter); > > + status = -EINVAL; > > + goto done; > > + } > > + urb->transfer_buffer = qh->sg_miter.addr; > > And here you set it. As a result, on the next iteration of this > routine the test above won't work right. (This function gets invoked > once for each entry in the sg list, right?) > > Is there any reason to set urb->transfer_buffer here? You could just > use qh->sg_miter.addr directly in the musb_write_fifo() call two lines > below. I will change it. Thanks Virupax S -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] usb:musb:musb_host: Handle highmem in PIO mode
-Original Message- From: Alan Stern [mailto:st...@rowland.harvard.edu] Sent: Tuesday, August 07, 2012 11:17 PM To: Virupax SADASHIVPETIMATH Cc: ba...@ti.com; gre...@linuxfoundation.org; linux-...@vger.kernel.org; linux- ker...@vger.kernel.org; linus.wall...@linaro.org; Praveena NADAHALLY; Rajaram REGUPATHY; Vikrant BAPAT Subject: Re: [PATCH v2] usb:musb:musb_host: Handle highmem in PIO mode +*/ + if (!urb-transfer_buffer) + use_sg = true; Here you test urb-transfer_buffer. I will make the test as if (!use_sg !urb-transfer_buffer) use_sg = true; + if (use_sg) { + /* sg_miter_start is already done in musb_ep_program */ + if (!sg_miter_next(qh-sg_miter)) { + dev_err(musb-controller, error: sg list empty\n); + sg_miter_stop(qh-sg_miter); + status = -EINVAL; + goto done; + } + urb-transfer_buffer = qh-sg_miter.addr; And here you set it. As a result, on the next iteration of this routine the test above won't work right. (This function gets invoked once for each entry in the sg list, right?) Is there any reason to set urb-transfer_buffer here? You could just use qh-sg_miter.addr directly in the musb_write_fifo() call two lines below. I will change it. Thanks Virupax S -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] usb:musb:musb_host: Handle highmem in PIO mode
On Tue, 7 Aug 2012, Virupax Sadashivpetimath wrote: > In case of USB bulk transfer, when himem page > is received, the usb_sg_init function sets the > urb transfer buffer to NULL. When such URB > transfer is handled, kernel crashes in PIO mode. > Handle this by mapping the highmem buffer in PIO mode. ... > @@ -1332,9 +1353,38 @@ void musb_host_tx(struct musb *musb, u8 epnum) > length = qh->maxpacket; > /* Unmap the buffer so that CPU can use it */ > usb_hcd_unmap_urb_for_dma(musb_to_hcd(musb), urb); > - musb_write_fifo(hw_ep, length, urb->transfer_buffer + offset); > + > + /* > + * We need to map sg if the transfer_buffer is > + * NULL. > + */ > + if (!urb->transfer_buffer) > + use_sg = true; Here you test urb->transfer_buffer. > + if (use_sg) { > + /* sg_miter_start is already done in musb_ep_program */ > + if (!sg_miter_next(>sg_miter)) { > + dev_err(musb->controller, "error: sg list empty\n"); > + sg_miter_stop(>sg_miter); > + status = -EINVAL; > + goto done; > + } > + urb->transfer_buffer = qh->sg_miter.addr; And here you set it. As a result, on the next iteration of this routine the test above won't work right. (This function gets invoked once for each entry in the sg list, right?) Is there any reason to set urb->transfer_buffer here? You could just use qh->sg_miter.addr directly in the musb_write_fifo() call two lines below. > + length = min_t(u32, length, qh->sg_miter.length); > + musb_write_fifo(hw_ep, length, urb->transfer_buffer); > + qh->sg_miter.consumed = length; > + sg_miter_stop(>sg_miter); > + } else { > + musb_write_fifo(hw_ep, length, urb->transfer_buffer + offset); > + } Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] usb:musb:musb_host: Handle highmem in PIO mode
On Tue, 7 Aug 2012, Virupax Sadashivpetimath wrote: In case of USB bulk transfer, when himem page is received, the usb_sg_init function sets the urb transfer buffer to NULL. When such URB transfer is handled, kernel crashes in PIO mode. Handle this by mapping the highmem buffer in PIO mode. ... @@ -1332,9 +1353,38 @@ void musb_host_tx(struct musb *musb, u8 epnum) length = qh-maxpacket; /* Unmap the buffer so that CPU can use it */ usb_hcd_unmap_urb_for_dma(musb_to_hcd(musb), urb); - musb_write_fifo(hw_ep, length, urb-transfer_buffer + offset); + + /* + * We need to map sg if the transfer_buffer is + * NULL. + */ + if (!urb-transfer_buffer) + use_sg = true; Here you test urb-transfer_buffer. + if (use_sg) { + /* sg_miter_start is already done in musb_ep_program */ + if (!sg_miter_next(qh-sg_miter)) { + dev_err(musb-controller, error: sg list empty\n); + sg_miter_stop(qh-sg_miter); + status = -EINVAL; + goto done; + } + urb-transfer_buffer = qh-sg_miter.addr; And here you set it. As a result, on the next iteration of this routine the test above won't work right. (This function gets invoked once for each entry in the sg list, right?) Is there any reason to set urb-transfer_buffer here? You could just use qh-sg_miter.addr directly in the musb_write_fifo() call two lines below. + length = min_t(u32, length, qh-sg_miter.length); + musb_write_fifo(hw_ep, length, urb-transfer_buffer); + qh-sg_miter.consumed = length; + sg_miter_stop(qh-sg_miter); + } else { + musb_write_fifo(hw_ep, length, urb-transfer_buffer + offset); + } Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/