* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).