* [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 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
* 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
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.