linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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 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).