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.
Signed-off-by: Virupax Sadashivpetimath <[email protected]>
---
drivers/usb/musb/musb_host.c | 98 +++++++++++++++++++++++++++++++++++++++--
include/linux/usb.h | 2 +
2 files changed, 95 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 4bb717d..ff1af0a 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -813,9 +813,28 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
if (load_count) {
/* PIO to load FIFO */
qh->segsize = load_count;
- musb_write_fifo(hw_ep, load_count, buf);
+ if (!buf) {
+ sg_miter_start(&urb->sg_miter, urb->sg, 1,
+ SG_MITER_ATOMIC
+ | SG_MITER_FROM_SG);
+ if (!sg_miter_next(&urb->sg_miter)) {
+ dev_err(musb->controller,
+ "error: sg"
+ "list empty\n");
+ sg_miter_stop(&urb->sg_miter);
+ goto finish;
+ }
+ buf = urb->sg_miter.addr + urb->sg->offset +
+ urb->actual_length;
+ load_count = min_t(u32, load_count,
+ urb->sg_miter.length);
+ musb_write_fifo(hw_ep, load_count, buf);
+ urb->sg_miter.consumed = load_count;
+ sg_miter_stop(&urb->sg_miter);
+ } else
+ musb_write_fifo(hw_ep, load_count, buf);
}
-
+finish:
/* re-enable interrupt */
musb_writew(mbase, MUSB_INTRTXE, int_txe);
@@ -1116,6 +1135,7 @@ void musb_host_tx(struct musb *musb, u8 epnum)
void __iomem *mbase = musb->mregs;
struct dma_channel *dma;
bool transfer_pending = false;
+ static bool use_sg;
musb_ep_select(mbase, epnum);
tx_csr = musb_readw(epio, MUSB_TXCSR);
@@ -1163,6 +1183,7 @@ void musb_host_tx(struct musb *musb, u8 epnum)
return;
}
+done:
if (status) {
if (dma_channel_status(dma) == MUSB_DMA_STATUS_BUSY) {
dma->status = MUSB_DMA_STATUS_CORE_ABORT;
@@ -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;
+
+ if (use_sg) {
+ /* sg_miter_start is already done in musb_ep_program */
+ if (!sg_miter_next(&urb->sg_miter)) {
+ dev_err(musb->controller, "error: sg list empty\n");
+ sg_miter_stop(&urb->sg_miter);
+ status = -EINVAL;
+ goto done;
+ }
+ urb->transfer_buffer = urb->sg_miter.addr;
+ length = min_t(u32, length, urb->sg_miter.length);
+ musb_write_fifo(hw_ep, length, urb->transfer_buffer);
+ urb->sg_miter.consumed = length;
+ sg_miter_stop(&urb->sg_miter);
+ } else {
+ musb_write_fifo(hw_ep, length, urb->transfer_buffer + offset);
+ }
+
qh->segsize = length;
+ if (use_sg) {
+ if (offset + length >= urb->transfer_buffer_length)
+ use_sg = false;
+ }
+
musb_ep_select(mbase, epnum);
musb_writew(epio, MUSB_TXCSR,
MUSB_TXCSR_H_WZC_BITS | MUSB_TXCSR_TXPKTRDY);
@@ -1442,6 +1492,8 @@ void musb_host_rx(struct musb *musb, u8 epnum)
bool done = false;
u32 status;
struct dma_channel *dma;
+ static bool use_sg;
+ unsigned int sg_flags = SG_MITER_ATOMIC | SG_MITER_TO_SG;
musb_ep_select(mbase, epnum);
@@ -1756,10 +1808,43 @@ void musb_host_rx(struct musb *musb, u8 epnum)
#endif /* Mentor DMA */
if (!dma) {
+ unsigned int received_len;
+
/* Unmap the buffer so that CPU can use it */
usb_hcd_unmap_urb_for_dma(musb_to_hcd(musb), urb);
- done = musb_host_packet_rx(musb, urb,
- epnum, iso_err);
+
+ /*
+ * We need to map sg if the transfer_buffer is
+ * NULL.
+ */
+ if (!urb->transfer_buffer) {
+ use_sg = true;
+ sg_miter_start(&urb->sg_miter, urb->sg, 1,
+ sg_flags);
+ }
+
+ if (use_sg) {
+ if (!sg_miter_next(&urb->sg_miter)) {
+ dev_err(musb->controller, "error: sg list empty\n");
+ sg_miter_stop(&urb->sg_miter);
+ status = -EINVAL;
+ done = true;
+ goto finish;
+ }
+ urb->transfer_buffer = urb->sg_miter.addr;
+ received_len = urb->actual_length;
+ qh->offset = 0x0;
+ done = musb_host_packet_rx(musb, urb, epnum,
+ iso_err);
+ /* Calculate the number of bytes received */
+ received_len = urb->actual_length -
+ received_len;
+ urb->sg_miter.consumed = received_len;
+ sg_miter_stop(&urb->sg_miter);
+ } else {
+ done = musb_host_packet_rx(musb, urb,
+ epnum, iso_err);
+ }
dev_dbg(musb->controller, "read %spacket\n", done ? "last " : "");
}
}
@@ -1768,6 +1853,9 @@ finish:
urb->actual_length += xfer_len;
qh->offset += xfer_len;
if (done) {
+ if (use_sg)
+ use_sg = false;
+
if (urb->status == -EINPROGRESS)
urb->status = status;
musb_advance_schedule(musb, urb, hw_ep, USB_DIR_IN);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index dea39dc..348648a 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -21,6 +21,7 @@
#include <linux/sched.h> /* for current && schedule_timeout */
#include <linux/mutex.h> /* for struct mutex */
#include <linux/pm_runtime.h> /* for runtime PM */
+#include <linux/scatterlist.h>
struct usb_device;
struct usb_driver;
@@ -1309,6 +1310,7 @@ struct urb {
usb_complete_t complete; /* (in) completion routine */
struct usb_iso_packet_descriptor iso_frame_desc[0];
/* (in) ISO ONLY */
+ struct sg_mapping_iter sg_miter; /* handling highmem data in PIO mode */
};
/* ----------------------------------------------------------------------- */
--
1.7.4.3
On Thu, Aug 02, 2012 at 12:06:42PM +0530, 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.
>
> Signed-off-by: Virupax Sadashivpetimath <[email protected]>
Why is this not a problem in any other host controller? Are you sure
this fix is correct? Why do you need to modify the struct urb for this?
greg k-h
> -----Original Message-----
> From: Greg KH [mailto:[email protected]]
> Sent: Thursday, August 02, 2012 4:30 PM
> To: Virupax SADASHIVPETIMATH
> Cc: [email protected]; [email protected]; [email protected]; Praveena
> NADAHALLY
> Subject: Re: [PATCH] usb:musb:musb_host: Handle highmem in PIO mode
>
> On Thu, Aug 02, 2012 at 12:06:42PM +0530, 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.
> >
> > Signed-off-by: Virupax Sadashivpetimath <[email protected]>
>
> Why is this not a problem in any other host controller?
Problem is seen only when the RAM on the board is 1GB or more. When the urb sg is in highmem.
Below crash is seen without the patch
[ 50.467529] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[ 50.475616] pgd = c0004000
[ 50.478302] [00000000] *pgd=00000000
[ 50.481872] Internal error: Oops: 817 [#1] PREEMPT SMP ARM
[ 50.546630] CPU: 0 Tainted: G O (3.4.0+ #1)
[ 50.552062] PC is at __raw_readsl+0x30/0x100
[ 50.556304] LR is at 0x0
[ 50.558837] pc : [<c028b500>] lr : [<00000000>] psr: 20000193
[ 50.558837] sp : c09b5c80 ip : 00000000 fp : c09b5cb4
[ 50.570312] r10: db9a46c0 r9 : c0a45538 r8 : 00000000
[ 50.575531] r7 : 00000002 r6 : df860028 r5 : 00000200 r4 : 00010101
[ 50.582031] r3 : 464c457f r2 : 00000078 r1 : 00000000 r0 : df860028
[ 50.588562] Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel
[ 50.595947] Control: 10c5787d Table: 1bf0c04a DAC: 00000015
> Are you sure this fix is correct?
I have tested the patch on the board with the issue and it seems to work.
> Why do you need to modify the struct urb for this?
The URB transfer may take more than 1 interrupt for the complete transfer
to store the state of sg_miter specific to urb, struct urb is used.
Thanks
Virupax S
> -----Original Message-----
> From: Virupax SADASHIVPETIMATH
> Sent: Thursday, August 02, 2012 5:35 PM
> To: 'Greg KH'
> Cc: [email protected]; [email protected]; [email protected]; Praveena
> NADAHALLY
> Subject: RE: [PATCH] usb:musb:musb_host: Handle highmem in PIO mode
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:[email protected]]
> > Sent: Thursday, August 02, 2012 4:30 PM
> > To: Virupax SADASHIVPETIMATH
> > Cc: [email protected]; [email protected]; [email protected]; Praveena
> > NADAHALLY
> > Subject: Re: [PATCH] usb:musb:musb_host: Handle highmem in PIO mode
> >
> > On Thu, Aug 02, 2012 at 12:06:42PM +0530, 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.
> > >
> > > Signed-off-by: Virupax Sadashivpetimath <[email protected]>
> >
> > Why is this not a problem in any other host controller?
>
> Problem is seen only when the RAM on the board is 1GB or more. When the urb sg is in
> highmem.
And also many of the host controllers are using the DMA mode for all sizes
of urb transfer, because of which the problem is not seen in those controllers.
Thanks
Virupax S
On Thu, 2 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.
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -21,6 +21,7 @@
> #include <linux/sched.h> /* for current && schedule_timeout */
> #include <linux/mutex.h> /* for struct mutex */
> #include <linux/pm_runtime.h> /* for runtime PM */
> +#include <linux/scatterlist.h>
>
> struct usb_device;
> struct usb_driver;
> @@ -1309,6 +1310,7 @@ struct urb {
> usb_complete_t complete; /* (in) completion routine */
> struct usb_iso_packet_descriptor iso_frame_desc[0];
> /* (in) ISO ONLY */
> + struct sg_mapping_iter sg_miter; /* handling highmem data in PIO mode */
> };
This is unacceptable. Fields like this should be stored in the
URB's hcpriv structure, not in the URB itself.
Alan Stern
> -----Original Message-----
> From: Alan Stern [mailto:[email protected]]
> Sent: Thursday, August 02, 2012 8:18 PM
> To: Virupax SADASHIVPETIMATH
> Cc: [email protected]; [email protected]; [email protected]; linux-
> [email protected]; Praveena NADAHALLY
> Subject: Re: [PATCH] usb:musb:musb_host: Handle highmem in PIO mode
>
> > --- a/include/linux/usb.h
> > +++ b/include/linux/usb.h
> > @@ -21,6 +21,7 @@
> > #include <linux/sched.h> /* for current && schedule_timeout */
> > #include <linux/mutex.h> /* for struct mutex */
> > #include <linux/pm_runtime.h> /* for runtime PM */
> > +#include <linux/scatterlist.h>
> >
> > struct usb_device;
> > struct usb_driver;
> > @@ -1309,6 +1310,7 @@ struct urb {
> > usb_complete_t complete; /* (in) completion routine */
> > struct usb_iso_packet_descriptor iso_frame_desc[0];
> > /* (in) ISO ONLY */
> > + struct sg_mapping_iter sg_miter; /* handling highmem data in PIO mode */
> > };
>
> This is unacceptable. Fields like this should be stored in the
> URB's hcpriv structure, not in the URB itself.
Ok I will add it in the hcpriv structure. Can you please comment on other
part of the code also, so that all the changes can be done together.
Thanks
Virupax S
On Fri, 3 Aug 2012, Virupax SADASHIVPETIMATH wrote:
> > This is unacceptable. Fields like this should be stored in the
> > URB's hcpriv structure, not in the URB itself.
>
> Ok I will add it in the hcpriv structure. Can you please comment on other
> part of the code also, so that all the changes can be done together.
I have no other comments.
Alan Stern