All of lore.kernel.org
 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 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.