* [PATCH] libertas if_usb: Fix crash on 64-bit machines @ 2009-10-30 17:45 David Woodhouse 2009-10-30 18:17 ` David Miller 2009-10-30 18:23 ` Larry Finger 0 siblings, 2 replies; 14+ messages in thread From: David Woodhouse @ 2009-10-30 17:45 UTC (permalink / raw) To: linville; +Cc: libertas-dev, linux-wireless, dcbw, stern, davem On a 64-bit kernel, skb->tail is an offset, not a pointer. The libertas usb driver passes it to usb_fill_bulk_urb() anyway, causing interesting crashes. Fix that by using skb->data instead. This highlights a problem with usb_fill_bulk_urb(). It doesn't notice when dma_map_single() fails and return the error to its caller as it should. In fact it _can't_ currently return the error, since it returns void. So this problem was showing up only at unmap time, after we'd already suffered memory corruption by doing DMA to a bogus address. Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> Cc: stable@kernel.org --- diff --git a/drivers/net/wireless/libertas/if_usb.c b/drivers/net/wireless/libertas/if_usb.c index 92bc8c5..3fac4ef 100644 --- a/drivers/net/wireless/libertas/if_usb.c +++ b/drivers/net/wireless/libertas/if_usb.c @@ -508,7 +508,7 @@ static int __if_usb_submit_rx_urb(struct if_usb_card *cardp, /* Fill the receive configuration URB and initialise the Rx call back */ usb_fill_bulk_urb(cardp->rx_urb, cardp->udev, usb_rcvbulkpipe(cardp->udev, cardp->ep_in), - (void *) (skb->tail + (size_t) IPFIELD_ALIGN_OFFSET), + skb->data + IPFIELD_ALIGN_OFFSET, MRVDRV_ETH_RX_PACKET_BUFFER_SIZE, callbackfn, cardp); -- David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] libertas if_usb: Fix crash on 64-bit machines 2009-10-30 17:45 [PATCH] libertas if_usb: Fix crash on 64-bit machines David Woodhouse @ 2009-10-30 18:17 ` David Miller 2009-10-30 18:23 ` Larry Finger 1 sibling, 0 replies; 14+ messages in thread From: David Miller @ 2009-10-30 18:17 UTC (permalink / raw) To: dwmw2; +Cc: linville, libertas-dev, linux-wireless, dcbw, stern From: David Woodhouse <dwmw2@infradead.org> Date: Fri, 30 Oct 2009 17:45:14 +0000 > On a 64-bit kernel, skb->tail is an offset, not a pointer. The libertas > usb driver passes it to usb_fill_bulk_urb() anyway, causing interesting > crashes. Fix that by using skb->data instead. > > This highlights a problem with usb_fill_bulk_urb(). It doesn't notice > when dma_map_single() fails and return the error to its caller as it > should. In fact it _can't_ currently return the error, since it returns > void. > > So this problem was showing up only at unmap time, after we'd already > suffered memory corruption by doing DMA to a bogus address. > > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> > Cc: stable@kernel.org Acked-by: David S. Miller <davem@davemloft.net> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libertas if_usb: Fix crash on 64-bit machines 2009-10-30 17:45 [PATCH] libertas if_usb: Fix crash on 64-bit machines David Woodhouse 2009-10-30 18:17 ` David Miller @ 2009-10-30 18:23 ` Larry Finger 2009-10-30 18:44 ` Christian Lamparter 1 sibling, 1 reply; 14+ messages in thread From: Larry Finger @ 2009-10-30 18:23 UTC (permalink / raw) To: David Woodhouse Cc: linville, libertas-dev, linux-wireless, dcbw, stern, davem David Woodhouse wrote: > On a 64-bit kernel, skb->tail is an offset, not a pointer. The libertas > usb driver passes it to usb_fill_bulk_urb() anyway, causing interesting > crashes. Fix that by using skb->data instead. > > This highlights a problem with usb_fill_bulk_urb(). It doesn't notice > when dma_map_single() fails and return the error to its caller as it > should. In fact it _can't_ currently return the error, since it returns > void. This should be fixed. If changing the code to return the error would be too invasive (It is used in 30+ drivers), perhaps the routine should be modified to log a warning when dma mapping fails. I will submit an RFC to do that. Larry ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libertas if_usb: Fix crash on 64-bit machines 2009-10-30 18:23 ` Larry Finger @ 2009-10-30 18:44 ` Christian Lamparter 2009-10-30 18:51 ` David Woodhouse 0 siblings, 1 reply; 14+ messages in thread From: Christian Lamparter @ 2009-10-30 18:44 UTC (permalink / raw) To: Larry Finger Cc: David Woodhouse, linville, libertas-dev, linux-wireless, dcbw, stern, davem On Friday 30 October 2009 19:23:15 Larry Finger wrote: > David Woodhouse wrote: > > On a 64-bit kernel, skb->tail is an offset, not a pointer. The libertas > > usb driver passes it to usb_fill_bulk_urb() anyway, causing interesting > > crashes. Fix that by using skb->data instead. > > > > This highlights a problem with usb_fill_bulk_urb(). It doesn't notice > > when dma_map_single() fails and return the error to its caller as it > > should. In fact it _can't_ currently return the error, since it returns > > void. > > This should be fixed. If changing the code to return the error would > be too invasive (It is used in 30+ drivers), perhaps the routine > should be modified to log a warning when dma mapping fails. I will > submit an RFC to do that. err, hold on a sec: - include/linux/usb.h - static inline void usb_fill_bulk_urb(struct urb *urb, struct usb_device *dev, unsigned int pipe, void *transfer_buffer, int buffer_length, usb_complete_t complete_fn, void *context) { urb->dev = dev; urb->pipe = pipe; urb->transfer_buffer = transfer_buffer; urb->transfer_buffer_length = buffer_length; urb->complete = complete_fn; urb->context = context; } that's just a fill-in macro. AFAICT usb_submit_urb does the tricky dma mapping. Regards, Chr ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libertas if_usb: Fix crash on 64-bit machines 2009-10-30 18:44 ` Christian Lamparter @ 2009-10-30 18:51 ` David Woodhouse 2009-10-30 19:08 ` Larry Finger ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: David Woodhouse @ 2009-10-30 18:51 UTC (permalink / raw) To: Christian Lamparter Cc: Larry Finger, linville, libertas-dev, linux-wireless, dcbw, stern, davem On Fri, 2009-10-30 at 19:44 +0100, Christian Lamparter wrote: > > that's just a fill-in macro. > AFAICT usb_submit_urb does the tricky dma mapping. Ah, that makes sense. In that case, all we need to do is make map_urb_for_dma() do the right thing. -- David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libertas if_usb: Fix crash on 64-bit machines 2009-10-30 18:51 ` David Woodhouse @ 2009-10-30 19:08 ` Larry Finger 2009-10-30 19:26 ` Christian Lamparter 2009-10-31 1:41 ` [PATCH] libertas if_usb: Fix crash on 64-bit machines Alan Stern 2 siblings, 0 replies; 14+ messages in thread From: Larry Finger @ 2009-10-30 19:08 UTC (permalink / raw) To: David Woodhouse Cc: Christian Lamparter, linville, libertas-dev, linux-wireless, dcbw, stern, davem David Woodhouse wrote: > On Fri, 2009-10-30 at 19:44 +0100, Christian Lamparter wrote: >> that's just a fill-in macro. >> AFAICT usb_submit_urb does the tricky dma mapping. > > Ah, that makes sense. In that case, all we need to do is make > map_urb_for_dma() do the right thing. After I sent my message, I realized that usb_submit_urb() and friends do no checking and that the test had to be further down the line. Larry ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libertas if_usb: Fix crash on 64-bit machines 2009-10-30 18:51 ` David Woodhouse 2009-10-30 19:08 ` Larry Finger @ 2009-10-30 19:26 ` Christian Lamparter 2009-11-04 19:16 ` John W. Linville 2009-10-31 1:41 ` [PATCH] libertas if_usb: Fix crash on 64-bit machines Alan Stern 2 siblings, 1 reply; 14+ messages in thread From: Christian Lamparter @ 2009-10-30 19:26 UTC (permalink / raw) To: David Woodhouse Cc: Larry Finger, linville, libertas-dev, linux-wireless, dcbw, stern, davem On Friday 30 October 2009 19:51:21 David Woodhouse wrote: > On Fri, 2009-10-30 at 19:44 +0100, Christian Lamparter wrote: > > > > that's just a fill-in macro. > > AFAICT usb_submit_urb does the tricky dma mapping. > > Ah, that makes sense. In that case, all we need to do is make > map_urb_for_dma() do the right thing. well, but while we're on the subject of libertas. I took a quick look around and wrote down some hopefully _helpful_ comments. That said, I don't have any libertas hw, so I have no idea if the attached code will actually do what its supposed to do... I'll leave it up to the professionals to test & write a real fix(es) with a proper commit message. Notes: - most + IPFIELD_ALIGN_OFFSET can be replaced by a single skb_reserve, right after allocation. - skb_tail_pointer(skb) should be used to get the right rx_buf pointer. - setting URB_ZERO_PACKET is pointless for urbs which are submitted to an IN endpoint. --- diff --git a/drivers/net/wireless/libertas/if_usb.c b/drivers/net/wireless/libertas/if_usb.c index a8262de..f220db9 100644 --- a/drivers/net/wireless/libertas/if_usb.c +++ b/drivers/net/wireless/libertas/if_usb.c @@ -506,17 +506,16 @@ static int __if_usb_submit_rx_urb(struct if_usb_card *cardp, goto rx_ret; } + skb_reserve(skb, IPFIELD_ALIGN_OFFSET); cardp->rx_skb = skb; /* Fill the receive configuration URB and initialise the Rx call back */ usb_fill_bulk_urb(cardp->rx_urb, cardp->udev, usb_rcvbulkpipe(cardp->udev, cardp->ep_in), - (void *) (skb->tail + (size_t) IPFIELD_ALIGN_OFFSET), + skb_tail_pointer(skb), MRVDRV_ETH_RX_PACKET_BUFFER_SIZE, callbackfn, cardp); - cardp->rx_urb->transfer_flags |= URB_ZERO_PACKET; - lbs_deb_usb2(&cardp->udev->dev, "Pointer for rx_urb %p\n", cardp->rx_urb); if ((ret = usb_submit_urb(cardp->rx_urb, GFP_ATOMIC))) { lbs_deb_usbd(&cardp->udev->dev, "Submit Rx URB failed: %d\n", ret); @@ -557,7 +556,7 @@ static void if_usb_receive_fwload(struct urb *urb) } if (cardp->fwdnldover) { - __le32 *tmp = (__le32 *)(skb->data + IPFIELD_ALIGN_OFFSET); + __le32 *tmp = (__le32 *)skb->data; if (tmp[0] == cpu_to_le32(CMD_TYPE_INDICATION) && tmp[1] == cpu_to_le32(MACREG_INT_CODE_FIRMWARE_READY)) { @@ -572,8 +571,7 @@ static void if_usb_receive_fwload(struct urb *urb) return; } if (cardp->bootcmdresp <= 0) { - memcpy (&bootcmdresp, skb->data + IPFIELD_ALIGN_OFFSET, - sizeof(bootcmdresp)); + memcpy(&bootcmdresp, skb->data, sizeof(bootcmdresp)); if (le16_to_cpu(cardp->udev->descriptor.bcdDevice) < 0x3106) { kfree_skb(skb); @@ -619,8 +617,7 @@ static void if_usb_receive_fwload(struct urb *urb) return; } - memcpy(syncfwheader, skb->data + IPFIELD_ALIGN_OFFSET, - sizeof(struct fwsyncheader)); + memcpy(syncfwheader, skb->data, sizeof(struct fwsyncheader)); if (!syncfwheader->cmd) { lbs_deb_usb2(&cardp->udev->dev, "FW received Blk with correct CRC\n"); @@ -665,7 +662,6 @@ static inline void process_cmdtypedata(int recvlength, struct sk_buff *skb, return; } - skb_reserve(skb, IPFIELD_ALIGN_OFFSET); skb_put(skb, recvlength); skb_pull(skb, MESSAGE_HEADER_LEN); @@ -719,7 +715,7 @@ static void if_usb_receive(struct urb *urb) int recvlength = urb->actual_length; uint8_t *recvbuff = NULL; uint32_t recvtype = 0; - __le32 *pkt = (__le32 *)(skb->data + IPFIELD_ALIGN_OFFSET); + __le32 *pkt = (__le32 *)skb->data; uint32_t event; lbs_deb_enter(LBS_DEB_USB); @@ -732,7 +728,7 @@ static void if_usb_receive(struct urb *urb) goto setup_for_next; } - recvbuff = skb->data + IPFIELD_ALIGN_OFFSET; + recvbuff = skb->data; recvtype = le32_to_cpu(pkt[0]); lbs_deb_usbd(&cardp->udev->dev, "Recv length = 0x%x, Recv type = 0x%X\n", ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] libertas if_usb: Fix crash on 64-bit machines 2009-10-30 19:26 ` Christian Lamparter @ 2009-11-04 19:16 ` John W. Linville 2009-11-04 19:36 ` David Woodhouse 0 siblings, 1 reply; 14+ messages in thread From: John W. Linville @ 2009-11-04 19:16 UTC (permalink / raw) To: Christian Lamparter Cc: David Woodhouse, Larry Finger, libertas-dev, linux-wireless, dcbw, stern, davem Is anyone evaluating these suggestions? Should I be expecting properly formatted patch emails? John On Fri, Oct 30, 2009 at 08:26:46PM +0100, Christian Lamparter wrote: > On Friday 30 October 2009 19:51:21 David Woodhouse wrote: > > On Fri, 2009-10-30 at 19:44 +0100, Christian Lamparter wrote: > > > > > > that's just a fill-in macro. > > > AFAICT usb_submit_urb does the tricky dma mapping. > > > > Ah, that makes sense. In that case, all we need to do is make > > map_urb_for_dma() do the right thing. > > well, but while we're on the subject of libertas. > > I took a quick look around and wrote down some hopefully _helpful_ comments. > That said, I don't have any libertas hw, so I have no idea if the attached > code will actually do what its supposed to do... I'll leave it up to the > professionals to test & write a real fix(es) with a proper commit message. > > Notes: > - most + IPFIELD_ALIGN_OFFSET can be replaced by a > single skb_reserve, right after allocation. > > - skb_tail_pointer(skb) should be used to get > the right rx_buf pointer. > > - setting URB_ZERO_PACKET is pointless for urbs > which are submitted to an IN endpoint. > --- > diff --git a/drivers/net/wireless/libertas/if_usb.c b/drivers/net/wireless/libertas/if_usb.c > index a8262de..f220db9 100644 > --- a/drivers/net/wireless/libertas/if_usb.c > +++ b/drivers/net/wireless/libertas/if_usb.c > @@ -506,17 +506,16 @@ static int __if_usb_submit_rx_urb(struct if_usb_card *cardp, > goto rx_ret; > } > > + skb_reserve(skb, IPFIELD_ALIGN_OFFSET); > cardp->rx_skb = skb; > > /* Fill the receive configuration URB and initialise the Rx call back */ > usb_fill_bulk_urb(cardp->rx_urb, cardp->udev, > usb_rcvbulkpipe(cardp->udev, cardp->ep_in), > - (void *) (skb->tail + (size_t) IPFIELD_ALIGN_OFFSET), > + skb_tail_pointer(skb), > MRVDRV_ETH_RX_PACKET_BUFFER_SIZE, callbackfn, > cardp); > > - cardp->rx_urb->transfer_flags |= URB_ZERO_PACKET; > - > lbs_deb_usb2(&cardp->udev->dev, "Pointer for rx_urb %p\n", cardp->rx_urb); > if ((ret = usb_submit_urb(cardp->rx_urb, GFP_ATOMIC))) { > lbs_deb_usbd(&cardp->udev->dev, "Submit Rx URB failed: %d\n", ret); > @@ -557,7 +556,7 @@ static void if_usb_receive_fwload(struct urb *urb) > } > > if (cardp->fwdnldover) { > - __le32 *tmp = (__le32 *)(skb->data + IPFIELD_ALIGN_OFFSET); > + __le32 *tmp = (__le32 *)skb->data; > > if (tmp[0] == cpu_to_le32(CMD_TYPE_INDICATION) && > tmp[1] == cpu_to_le32(MACREG_INT_CODE_FIRMWARE_READY)) { > @@ -572,8 +571,7 @@ static void if_usb_receive_fwload(struct urb *urb) > return; > } > if (cardp->bootcmdresp <= 0) { > - memcpy (&bootcmdresp, skb->data + IPFIELD_ALIGN_OFFSET, > - sizeof(bootcmdresp)); > + memcpy(&bootcmdresp, skb->data, sizeof(bootcmdresp)); > > if (le16_to_cpu(cardp->udev->descriptor.bcdDevice) < 0x3106) { > kfree_skb(skb); > @@ -619,8 +617,7 @@ static void if_usb_receive_fwload(struct urb *urb) > return; > } > > - memcpy(syncfwheader, skb->data + IPFIELD_ALIGN_OFFSET, > - sizeof(struct fwsyncheader)); > + memcpy(syncfwheader, skb->data, sizeof(struct fwsyncheader)); > > if (!syncfwheader->cmd) { > lbs_deb_usb2(&cardp->udev->dev, "FW received Blk with correct CRC\n"); > @@ -665,7 +662,6 @@ static inline void process_cmdtypedata(int recvlength, struct sk_buff *skb, > return; > } > > - skb_reserve(skb, IPFIELD_ALIGN_OFFSET); > skb_put(skb, recvlength); > skb_pull(skb, MESSAGE_HEADER_LEN); > > @@ -719,7 +715,7 @@ static void if_usb_receive(struct urb *urb) > int recvlength = urb->actual_length; > uint8_t *recvbuff = NULL; > uint32_t recvtype = 0; > - __le32 *pkt = (__le32 *)(skb->data + IPFIELD_ALIGN_OFFSET); > + __le32 *pkt = (__le32 *)skb->data; > uint32_t event; > > lbs_deb_enter(LBS_DEB_USB); > @@ -732,7 +728,7 @@ static void if_usb_receive(struct urb *urb) > goto setup_for_next; > } > > - recvbuff = skb->data + IPFIELD_ALIGN_OFFSET; > + recvbuff = skb->data; > recvtype = le32_to_cpu(pkt[0]); > lbs_deb_usbd(&cardp->udev->dev, > "Recv length = 0x%x, Recv type = 0x%X\n", > > > > -- John W. Linville Someday the world will need a hero, and you linville@tuxdriver.com might be all we have. Be ready. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libertas if_usb: Fix crash on 64-bit machines 2009-11-04 19:16 ` John W. Linville @ 2009-11-04 19:36 ` David Woodhouse 2009-11-04 20:01 ` Dan Williams 0 siblings, 1 reply; 14+ messages in thread From: David Woodhouse @ 2009-11-04 19:36 UTC (permalink / raw) To: John W. Linville Cc: Christian Lamparter, Larry Finger, libertas-dev, linux-wireless, dcbw, stern, davem On Wed, 2009-11-04 at 14:16 -0500, John W. Linville wrote: > Is anyone evaluating these suggestions? Should I be expecting > properly formatted patch emails? The suggestions make sense, but not for stable. Let's fix the crasher bug first, and worry about the rest later. -- David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libertas if_usb: Fix crash on 64-bit machines 2009-11-04 19:36 ` David Woodhouse @ 2009-11-04 20:01 ` Dan Williams 2009-11-04 21:16 ` John W. Linville 0 siblings, 1 reply; 14+ messages in thread From: Dan Williams @ 2009-11-04 20:01 UTC (permalink / raw) To: David Woodhouse Cc: John W. Linville, Christian Lamparter, Larry Finger, libertas-dev, linux-wireless, stern, davem On Wed, 2009-11-04 at 19:36 +0000, David Woodhouse wrote: > On Wed, 2009-11-04 at 14:16 -0500, John W. Linville wrote: > > Is anyone evaluating these suggestions? Should I be expecting > > properly formatted patch emails? > > The suggestions make sense, but not for stable. Let's fix the crasher > bug first, and worry about the rest later. Sounds good to me... Dan ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libertas if_usb: Fix crash on 64-bit machines 2009-11-04 20:01 ` Dan Williams @ 2009-11-04 21:16 ` John W. Linville 2009-11-04 22:12 ` [PATCH] libertas if_usb: tiny usb-rx overhaul Christian Lamparter 0 siblings, 1 reply; 14+ messages in thread From: John W. Linville @ 2009-11-04 21:16 UTC (permalink / raw) To: Dan Williams Cc: David Woodhouse, Christian Lamparter, Larry Finger, libertas-dev, linux-wireless, stern, davem On Wed, Nov 04, 2009 at 12:01:43PM -0800, Dan Williams wrote: > On Wed, 2009-11-04 at 19:36 +0000, David Woodhouse wrote: > > On Wed, 2009-11-04 at 14:16 -0500, John W. Linville wrote: > > > Is anyone evaluating these suggestions? Should I be expecting > > > properly formatted patch emails? > > > > The suggestions make sense, but not for stable. Let's fix the crasher > > bug first, and worry about the rest later. > > Sounds good to me... Yes, of course. So, Christian, will you be following-up? Thanks! John -- John W. Linville Someday the world will need a hero, and you linville@tuxdriver.com might be all we have. Be ready. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] libertas if_usb: tiny usb-rx overhaul 2009-11-04 21:16 ` John W. Linville @ 2009-11-04 22:12 ` Christian Lamparter 2009-11-10 7:02 ` Dan Williams 0 siblings, 1 reply; 14+ messages in thread From: Christian Lamparter @ 2009-11-04 22:12 UTC (permalink / raw) To: linux-wireless; +Cc: John W. Linville, Dan Williams, libertas-dev This patch reorganizes libertas' if_usb rx routines. - URB_ZERO_PACKET flag has no use for urbs for incoming endpoints. - skb_tail_pointer(skb) should be used to get the right rx_buf pointer. - most + IPFIELD_ALIGN_OFFSET can be prevented by moving skb_reserve up right after allocation. Signed-off-by: Christian Lamparter <chunkeey@googlemail.com> --- any Tested-by: ? --- diff --git a/drivers/net/wireless/libertas/if_usb.c b/drivers/net/wireless/libertas/if_usb.c index a8262de..f220db9 100644 --- a/drivers/net/wireless/libertas/if_usb.c +++ b/drivers/net/wireless/libertas/if_usb.c @@ -506,17 +506,16 @@ static int __if_usb_submit_rx_urb(struct if_usb_card *cardp, goto rx_ret; } + skb_reserve(skb, IPFIELD_ALIGN_OFFSET); cardp->rx_skb = skb; /* Fill the receive configuration URB and initialise the Rx call back */ usb_fill_bulk_urb(cardp->rx_urb, cardp->udev, usb_rcvbulkpipe(cardp->udev, cardp->ep_in), - skb->data + IPFIELD_ALIGN_OFFSET, + skb_tail_pointer(skb), MRVDRV_ETH_RX_PACKET_BUFFER_SIZE, callbackfn, cardp); - cardp->rx_urb->transfer_flags |= URB_ZERO_PACKET; - lbs_deb_usb2(&cardp->udev->dev, "Pointer for rx_urb %p\n", cardp->rx_urb); if ((ret = usb_submit_urb(cardp->rx_urb, GFP_ATOMIC))) { lbs_deb_usbd(&cardp->udev->dev, "Submit Rx URB failed: %d\n", ret); @@ -557,7 +556,7 @@ static void if_usb_receive_fwload(struct urb *urb) } if (cardp->fwdnldover) { - __le32 *tmp = (__le32 *)(skb->data + IPFIELD_ALIGN_OFFSET); + __le32 *tmp = (__le32 *)skb->data; if (tmp[0] == cpu_to_le32(CMD_TYPE_INDICATION) && tmp[1] == cpu_to_le32(MACREG_INT_CODE_FIRMWARE_READY)) { @@ -572,8 +571,7 @@ static void if_usb_receive_fwload(struct urb *urb) return; } if (cardp->bootcmdresp <= 0) { - memcpy (&bootcmdresp, skb->data + IPFIELD_ALIGN_OFFSET, - sizeof(bootcmdresp)); + memcpy(&bootcmdresp, skb->data, sizeof(bootcmdresp)); if (le16_to_cpu(cardp->udev->descriptor.bcdDevice) < 0x3106) { kfree_skb(skb); @@ -619,8 +617,7 @@ static void if_usb_receive_fwload(struct urb *urb) return; } - memcpy(syncfwheader, skb->data + IPFIELD_ALIGN_OFFSET, - sizeof(struct fwsyncheader)); + memcpy(syncfwheader, skb->data, sizeof(struct fwsyncheader)); if (!syncfwheader->cmd) { lbs_deb_usb2(&cardp->udev->dev, "FW received Blk with correct CRC\n"); @@ -665,7 +662,6 @@ static inline void process_cmdtypedata(int recvlength, struct sk_buff *skb, return; } - skb_reserve(skb, IPFIELD_ALIGN_OFFSET); skb_put(skb, recvlength); skb_pull(skb, MESSAGE_HEADER_LEN); @@ -719,7 +715,7 @@ static void if_usb_receive(struct urb *urb) int recvlength = urb->actual_length; uint8_t *recvbuff = NULL; uint32_t recvtype = 0; - __le32 *pkt = (__le32 *)(skb->data + IPFIELD_ALIGN_OFFSET); + __le32 *pkt = (__le32 *)skb->data; uint32_t event; lbs_deb_enter(LBS_DEB_USB); @@ -732,7 +728,7 @@ static void if_usb_receive(struct urb *urb) goto setup_for_next; } - recvbuff = skb->data + IPFIELD_ALIGN_OFFSET; + recvbuff = skb->data; recvtype = le32_to_cpu(pkt[0]); lbs_deb_usbd(&cardp->udev->dev, "Recv length = 0x%x, Recv type = 0x%X\n", ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] libertas if_usb: tiny usb-rx overhaul 2009-11-04 22:12 ` [PATCH] libertas if_usb: tiny usb-rx overhaul Christian Lamparter @ 2009-11-10 7:02 ` Dan Williams 0 siblings, 0 replies; 14+ messages in thread From: Dan Williams @ 2009-11-10 7:02 UTC (permalink / raw) To: Christian Lamparter; +Cc: linux-wireless, John W. Linville, libertas-dev On Wed, 2009-11-04 at 23:12 +0100, Christian Lamparter wrote: > This patch reorganizes libertas' if_usb rx routines. > > - URB_ZERO_PACKET flag has no use for urbs for > incoming endpoints. > > - skb_tail_pointer(skb) should be used to get > the right rx_buf pointer. > > - most + IPFIELD_ALIGN_OFFSET can be prevented by > moving skb_reserve up right after allocation. > > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com> With 5.110.22p23 firmware if anyone cares... Thanks! Acked-by: Dan Williams <dcbw@redhat.com> Tested-by: Dan Williams <dcbw@redhat.com> > --- > any Tested-by: ? > --- > diff --git a/drivers/net/wireless/libertas/if_usb.c b/drivers/net/wireless/libertas/if_usb.c > index a8262de..f220db9 100644 > --- a/drivers/net/wireless/libertas/if_usb.c > +++ b/drivers/net/wireless/libertas/if_usb.c > @@ -506,17 +506,16 @@ static int __if_usb_submit_rx_urb(struct if_usb_card *cardp, > goto rx_ret; > } > > + skb_reserve(skb, IPFIELD_ALIGN_OFFSET); > cardp->rx_skb = skb; > > /* Fill the receive configuration URB and initialise the Rx call back */ > usb_fill_bulk_urb(cardp->rx_urb, cardp->udev, > usb_rcvbulkpipe(cardp->udev, cardp->ep_in), > - skb->data + IPFIELD_ALIGN_OFFSET, > + skb_tail_pointer(skb), > MRVDRV_ETH_RX_PACKET_BUFFER_SIZE, callbackfn, > cardp); > > - cardp->rx_urb->transfer_flags |= URB_ZERO_PACKET; > - > lbs_deb_usb2(&cardp->udev->dev, "Pointer for rx_urb %p\n", cardp->rx_urb); > if ((ret = usb_submit_urb(cardp->rx_urb, GFP_ATOMIC))) { > lbs_deb_usbd(&cardp->udev->dev, "Submit Rx URB failed: %d\n", ret); > @@ -557,7 +556,7 @@ static void if_usb_receive_fwload(struct urb *urb) > } > > if (cardp->fwdnldover) { > - __le32 *tmp = (__le32 *)(skb->data + IPFIELD_ALIGN_OFFSET); > + __le32 *tmp = (__le32 *)skb->data; > > if (tmp[0] == cpu_to_le32(CMD_TYPE_INDICATION) && > tmp[1] == cpu_to_le32(MACREG_INT_CODE_FIRMWARE_READY)) { > @@ -572,8 +571,7 @@ static void if_usb_receive_fwload(struct urb *urb) > return; > } > if (cardp->bootcmdresp <= 0) { > - memcpy (&bootcmdresp, skb->data + IPFIELD_ALIGN_OFFSET, > - sizeof(bootcmdresp)); > + memcpy(&bootcmdresp, skb->data, sizeof(bootcmdresp)); > > if (le16_to_cpu(cardp->udev->descriptor.bcdDevice) < 0x3106) { > kfree_skb(skb); > @@ -619,8 +617,7 @@ static void if_usb_receive_fwload(struct urb *urb) > return; > } > > - memcpy(syncfwheader, skb->data + IPFIELD_ALIGN_OFFSET, > - sizeof(struct fwsyncheader)); > + memcpy(syncfwheader, skb->data, sizeof(struct fwsyncheader)); > > if (!syncfwheader->cmd) { > lbs_deb_usb2(&cardp->udev->dev, "FW received Blk with correct CRC\n"); > @@ -665,7 +662,6 @@ static inline void process_cmdtypedata(int recvlength, struct sk_buff *skb, > return; > } > > - skb_reserve(skb, IPFIELD_ALIGN_OFFSET); > skb_put(skb, recvlength); > skb_pull(skb, MESSAGE_HEADER_LEN); > > @@ -719,7 +715,7 @@ static void if_usb_receive(struct urb *urb) > int recvlength = urb->actual_length; > uint8_t *recvbuff = NULL; > uint32_t recvtype = 0; > - __le32 *pkt = (__le32 *)(skb->data + IPFIELD_ALIGN_OFFSET); > + __le32 *pkt = (__le32 *)skb->data; > uint32_t event; > > lbs_deb_enter(LBS_DEB_USB); > @@ -732,7 +728,7 @@ static void if_usb_receive(struct urb *urb) > goto setup_for_next; > } > > - recvbuff = skb->data + IPFIELD_ALIGN_OFFSET; > + recvbuff = skb->data; > recvtype = le32_to_cpu(pkt[0]); > lbs_deb_usbd(&cardp->udev->dev, > "Recv length = 0x%x, Recv type = 0x%X\n", > > _______________________________________________ > libertas-dev mailing list > libertas-dev@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/libertas-dev ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libertas if_usb: Fix crash on 64-bit machines 2009-10-30 18:51 ` David Woodhouse 2009-10-30 19:08 ` Larry Finger 2009-10-30 19:26 ` Christian Lamparter @ 2009-10-31 1:41 ` Alan Stern 2 siblings, 0 replies; 14+ messages in thread From: Alan Stern @ 2009-10-31 1:41 UTC (permalink / raw) To: David Woodhouse Cc: Christian Lamparter, Larry Finger, linville, libertas-dev, linux-wireless, dcbw, davem On Fri, 30 Oct 2009, David Woodhouse wrote: > On Fri, 2009-10-30 at 19:44 +0100, Christian Lamparter wrote: > > > > that's just a fill-in macro. > > AFAICT usb_submit_urb does the tricky dma mapping. > > Ah, that makes sense. In that case, all we need to do is make > map_urb_for_dma() do the right thing. map_urb_for_dma() does little more than call dma_map_single(). That's what you'll have to fix. Alan Stern ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2009-11-10 7:02 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-10-30 17:45 [PATCH] libertas if_usb: Fix crash on 64-bit machines David Woodhouse 2009-10-30 18:17 ` David Miller 2009-10-30 18:23 ` Larry Finger 2009-10-30 18:44 ` Christian Lamparter 2009-10-30 18:51 ` David Woodhouse 2009-10-30 19:08 ` Larry Finger 2009-10-30 19:26 ` Christian Lamparter 2009-11-04 19:16 ` John W. Linville 2009-11-04 19:36 ` David Woodhouse 2009-11-04 20:01 ` Dan Williams 2009-11-04 21:16 ` John W. Linville 2009-11-04 22:12 ` [PATCH] libertas if_usb: tiny usb-rx overhaul Christian Lamparter 2009-11-10 7:02 ` Dan Williams 2009-10-31 1:41 ` [PATCH] libertas if_usb: Fix crash on 64-bit machines Alan Stern
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.