Re: [PATCH] b43: support new RX header, noticed to be used in 598.314+ fw

Re: [PATCH] b43: support new RX header, noticed to be used in 598.314+ fw

[Guennadi Liakhovetski]

Hi

Sorry for "simulating" a reply - I'm not subscribed to linux-wireless, so,
cannot (easily) reply.
   
The commit in subject line

commit 17030f48e31adde5b043741c91ba143f5f7db0fd
From: Rafał Miłecki <zajec5@gmail.com>
Date: Thu, 11 Aug 2011 17:16:27 +0200
Subject: [PATCH] b43: support new RX header, noticed to be used in 598.314+ fw

breaks my b43 SDIO card in 3.2-ish kernels. Given the approaching 
3.2-final release, would be nice to have it fixed before that. The card 
I've got is marked with some "SD-Link11g" from some "C-guys, Inc." from 
Taiwan:-) Using firmware version 410.31754. Do I really _have_ to update 
or is the driver supposed to handle "all" firmware versions, or at least 
not drop ones, it used to support before?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] b43: support new RX header, noticed to be used in 598.314+ fw

[Larry Finger]

On 12/26/2011 09:29 AM, Guennadi Liakhovetski wrote:
> Hi
>
> Sorry for "simulating" a reply - I'm not subscribed to linux-wireless, so,
> cannot (easily) reply.
>
> The commit in subject line
>
> commit 17030f48e31adde5b043741c91ba143f5f7db0fd
> From: Rafał Miłecki<zajec5@gmail.com>
> Date: Thu, 11 Aug 2011 17:16:27 +0200
> Subject: [PATCH] b43: support new RX header, noticed to be used in 598.314+ fw
>
> breaks my b43 SDIO card in 3.2-ish kernels. Given the approaching
> 3.2-final release, would be nice to have it fixed before that. The card
> I've got is marked with some "SD-Link11g" from some "C-guys, Inc." from
> Taiwan:-) Using firmware version 410.31754. Do I really _have_ to update
> or is the driver supposed to handle "all" firmware versions, or at least
> not drop ones, it used to support before?

The only cards that *need* to use the new-style firmware are those for which 
firmware does not exist in the older versions. As this is not the case for your 
device, something else is wrong.

Please post the details for your card. If you had a PCI system, I would ask for 
'lspci -n'. Please do the same for the SDIO card.

 From your message, it appears that 3.1 works OK. If that is true, would it be 
possible for you to bisect this problem?

Larry

[PATCH] b43: fix regression in PIO case

[Guennadi Liakhovetski]

This patch fixes the regression, introduced by

commit 17030f48e31adde5b043741c91ba143f5f7db0fd
From: Rafał Miłecki <zajec5@gmail.com>
Date: Thu, 11 Aug 2011 17:16:27 +0200
Subject: [PATCH] b43: support new RX header, noticed to be used in 598.314+ fw

in PIO case.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
diff --git a/drivers/net/wireless/b43/pio.c b/drivers/net/wireless/b43/pio.c
index ce8a4bd..b64b64c 100644
--- a/drivers/net/wireless/b43/pio.c
+++ b/drivers/net/wireless/b43/pio.c
@@ -617,9 +617,19 @@ static bool pio_rx_frame(struct b43_pio_rxqueue *q)
 	const char *err_msg = NULL;
 	struct b43_rxhdr_fw4 *rxhdr =
 		(struct b43_rxhdr_fw4 *)wl->pio_scratchspace;
+	size_t rxhdr_size = sizeof(*rxhdr);
 
 	BUILD_BUG_ON(sizeof(wl->pio_scratchspace) < sizeof(*rxhdr));
-	memset(rxhdr, 0, sizeof(*rxhdr));
+	switch (dev->fw.hdr_format) {
+	case B43_FW_HDR_410:
+	case B43_FW_HDR_351:
+		rxhdr_size -= sizeof(rxhdr->format_598) -
+			sizeof(rxhdr->format_351);
+		break;
+	case B43_FW_HDR_598:
+		break;
+	}
+	memset(rxhdr, 0, rxhdr_size);
 
 	/* Check if we have data and wait for it to get ready. */
 	if (q->rev >= 8) {
@@ -657,11 +667,11 @@ data_ready:
 
 	/* Get the preamble (RX header) */
 	if (q->rev >= 8) {
-		b43_block_read(dev, rxhdr, sizeof(*rxhdr),
+		b43_block_read(dev, rxhdr, rxhdr_size,
 			       q->mmio_base + B43_PIO8_RXDATA,
 			       sizeof(u32));
 	} else {
-		b43_block_read(dev, rxhdr, sizeof(*rxhdr),
+		b43_block_read(dev, rxhdr, rxhdr_size,
 			       q->mmio_base + B43_PIO_RXDATA,
 			       sizeof(u16));
 	}

Re: [PATCH] b43: fix regression in PIO case

[Larry Finger]

On 12/26/2011 11:28 AM, Guennadi Liakhovetski wrote:
> This patch fixes the regression, introduced by
>
> commit 17030f48e31adde5b043741c91ba143f5f7db0fd
> From: Rafał Miłecki<zajec5@gmail.com>
> Date: Thu, 11 Aug 2011 17:16:27 +0200
> Subject: [PATCH] b43: support new RX header, noticed to be used in 598.314+ fw
>
> in PIO case.
>
> Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
> ---
> diff --git a/drivers/net/wireless/b43/pio.c b/drivers/net/wireless/b43/pio.c
> index ce8a4bd..b64b64c 100644
> --- a/drivers/net/wireless/b43/pio.c
> +++ b/drivers/net/wireless/b43/pio.c
> @@ -617,9 +617,19 @@ static bool pio_rx_frame(struct b43_pio_rxqueue *q)
>   	const char *err_msg = NULL;
>   	struct b43_rxhdr_fw4 *rxhdr =
>   		(struct b43_rxhdr_fw4 *)wl->pio_scratchspace;
> +	size_t rxhdr_size = sizeof(*rxhdr);
>
>   	BUILD_BUG_ON(sizeof(wl->pio_scratchspace)<  sizeof(*rxhdr));
> -	memset(rxhdr, 0, sizeof(*rxhdr));
> +	switch (dev->fw.hdr_format) {
> +	case B43_FW_HDR_410:
> +	case B43_FW_HDR_351:
> +		rxhdr_size -= sizeof(rxhdr->format_598) -
> +			sizeof(rxhdr->format_351);
> +		break;
> +	case B43_FW_HDR_598:
> +		break;
> +	}

I do not think the above will work for format_410. By my count, the format_410 
struct is 4 bytes longer than the format_351 struct.

Even if it does work, I suggest using the following:

	size_t rxhdr_size;

    	BUILD_BUG_ON(sizeof(wl->pio_scratchspace)<  sizeof(*rxhdr));
	switch (dev->fw.hdr_format) {
	case B43_FW_HDR_351:
		rxhdr_size = sizeof(rxhdr->format_351);
		break;
	case B43_FW_HDR_410:
etc.


Larry

[PATCH] b43: fix regression in PIO case

[Larry Finger]

On 12/26/2011 11:28 AM, Guennadi Liakhovetski wrote:
> This patch fixes the regression, introduced by
>
> commit 17030f48e31adde5b043741c91ba143f5f7db0fd
> From: Rafa? Mi?ecki<zajec5@gmail.com>
> Date: Thu, 11 Aug 2011 17:16:27 +0200
> Subject: [PATCH] b43: support new RX header, noticed to be used in 598.314+ fw
>
> in PIO case.
>
> Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
> ---
> diff --git a/drivers/net/wireless/b43/pio.c b/drivers/net/wireless/b43/pio.c
> index ce8a4bd..b64b64c 100644
> --- a/drivers/net/wireless/b43/pio.c
> +++ b/drivers/net/wireless/b43/pio.c
> @@ -617,9 +617,19 @@ static bool pio_rx_frame(struct b43_pio_rxqueue *q)
>   	const char *err_msg = NULL;
>   	struct b43_rxhdr_fw4 *rxhdr =
>   		(struct b43_rxhdr_fw4 *)wl->pio_scratchspace;
> +	size_t rxhdr_size = sizeof(*rxhdr);
>
>   	BUILD_BUG_ON(sizeof(wl->pio_scratchspace)<  sizeof(*rxhdr));
> -	memset(rxhdr, 0, sizeof(*rxhdr));
> +	switch (dev->fw.hdr_format) {
> +	case B43_FW_HDR_410:
> +	case B43_FW_HDR_351:
> +		rxhdr_size -= sizeof(rxhdr->format_598) -
> +			sizeof(rxhdr->format_351);
> +		break;
> +	case B43_FW_HDR_598:
> +		break;
> +	}

I do not think the above will work for format_410. By my count, the format_410 
struct is 4 bytes longer than the format_351 struct.

Even if it does work, I suggest using the following:

	size_t rxhdr_size;

    	BUILD_BUG_ON(sizeof(wl->pio_scratchspace)<  sizeof(*rxhdr));
	switch (dev->fw.hdr_format) {
	case B43_FW_HDR_351:
		rxhdr_size = sizeof(rxhdr->format_351);
		break;
	case B43_FW_HDR_410:
etc.


Larry

Re: [PATCH] b43: fix regression in PIO case

[Guennadi Liakhovetski]

On Mon, 26 Dec 2011, Larry Finger wrote:

> On 12/26/2011 11:28 AM, Guennadi Liakhovetski wrote:
> > This patch fixes the regression, introduced by
> > 
> > commit 17030f48e31adde5b043741c91ba143f5f7db0fd
> > From: Rafał Miłecki<zajec5@gmail.com>
> > Date: Thu, 11 Aug 2011 17:16:27 +0200
> > Subject: [PATCH] b43: support new RX header, noticed to be used in 598.314+
> > fw
> > 
> > in PIO case.
> > 
> > Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
> > ---
> > diff --git a/drivers/net/wireless/b43/pio.c b/drivers/net/wireless/b43/pio.c
> > index ce8a4bd..b64b64c 100644
> > --- a/drivers/net/wireless/b43/pio.c
> > +++ b/drivers/net/wireless/b43/pio.c
> > @@ -617,9 +617,19 @@ static bool pio_rx_frame(struct b43_pio_rxqueue *q)
> >   	const char *err_msg = NULL;
> >   	struct b43_rxhdr_fw4 *rxhdr =
> >   		(struct b43_rxhdr_fw4 *)wl->pio_scratchspace;
> > +	size_t rxhdr_size = sizeof(*rxhdr);
> > 
> >   	BUILD_BUG_ON(sizeof(wl->pio_scratchspace)<  sizeof(*rxhdr));
> > -	memset(rxhdr, 0, sizeof(*rxhdr));
> > +	switch (dev->fw.hdr_format) {
> > +	case B43_FW_HDR_410:
> > +	case B43_FW_HDR_351:
> > +		rxhdr_size -= sizeof(rxhdr->format_598) -
> > +			sizeof(rxhdr->format_351);
> > +		break;
> > +	case B43_FW_HDR_598:
> > +		break;
> > +	}
> 
> I do not think the above will work for format_410. By my count, the format_410
> struct is 4 bytes longer than the format_351 struct.

I think you're looking at struct b43_txhdr, whereas the problem is in 
struct b43_rxhdr_fw4.

Thanks
Guennadi

> 
> Even if it does work, I suggest using the following:
> 
> 	size_t rxhdr_size;
> 
>    	BUILD_BUG_ON(sizeof(wl->pio_scratchspace)<  sizeof(*rxhdr));
> 	switch (dev->fw.hdr_format) {
> 	case B43_FW_HDR_351:
> 		rxhdr_size = sizeof(rxhdr->format_351);
> 		break;
> 	case B43_FW_HDR_410:
> etc.
> 
> 
> Larry
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] b43: fix regression in PIO case

[Larry Finger]

On 12/26/2011 12:17 PM, Guennadi Liakhovetski wrote:
> On Mon, 26 Dec 2011, Larry Finger wrote:
>
>> On 12/26/2011 11:28 AM, Guennadi Liakhovetski wrote:
>>> This patch fixes the regression, introduced by
>>>
>>> commit 17030f48e31adde5b043741c91ba143f5f7db0fd
>>> From: Rafał Miłecki<zajec5@gmail.com>
>>> Date: Thu, 11 Aug 2011 17:16:27 +0200
>>> Subject: [PATCH] b43: support new RX header, noticed to be used in 598.314+
>>> fw
>>>
>>> in PIO case.
>>>
>>> Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
>>> ---
>>> diff --git a/drivers/net/wireless/b43/pio.c b/drivers/net/wireless/b43/pio.c
>>> index ce8a4bd..b64b64c 100644
>>> --- a/drivers/net/wireless/b43/pio.c
>>> +++ b/drivers/net/wireless/b43/pio.c
>>> @@ -617,9 +617,19 @@ static bool pio_rx_frame(struct b43_pio_rxqueue *q)
>>>    	const char *err_msg = NULL;
>>>    	struct b43_rxhdr_fw4 *rxhdr =
>>>    		(struct b43_rxhdr_fw4 *)wl->pio_scratchspace;
>>> +	size_t rxhdr_size = sizeof(*rxhdr);
>>>
>>>    	BUILD_BUG_ON(sizeof(wl->pio_scratchspace)<   sizeof(*rxhdr));
>>> -	memset(rxhdr, 0, sizeof(*rxhdr));
>>> +	switch (dev->fw.hdr_format) {
>>> +	case B43_FW_HDR_410:
>>> +	case B43_FW_HDR_351:
>>> +		rxhdr_size -= sizeof(rxhdr->format_598) -
>>> +			sizeof(rxhdr->format_351);
>>> +		break;
>>> +	case B43_FW_HDR_598:
>>> +		break;
>>> +	}
>>
>> I do not think the above will work for format_410. By my count, the format_410
>> struct is 4 bytes longer than the format_351 struct.
>
> I think you're looking at struct b43_txhdr, whereas the problem is in
> struct b43_rxhdr_fw4.
>
> Thanks
> Guennadi
>
>>
>> Even if it does work, I suggest using the following:
>>
>> 	size_t rxhdr_size;
>>
>>     	BUILD_BUG_ON(sizeof(wl->pio_scratchspace)<   sizeof(*rxhdr));
>> 	switch (dev->fw.hdr_format) {
>> 	case B43_FW_HDR_351:
>> 		rxhdr_size = sizeof(rxhdr->format_351);
>> 		break;
>> 	case B43_FW_HDR_410:

You are correct - I was looking in the wrong place. Even so, I prefer setting 
rxhdr_size in the case branches, and not adjust one preset earlier.

Larry

[PATCH] b43: fix regression in PIO case

[Larry Finger]

On 12/26/2011 12:17 PM, Guennadi Liakhovetski wrote:
> On Mon, 26 Dec 2011, Larry Finger wrote:
>
>> On 12/26/2011 11:28 AM, Guennadi Liakhovetski wrote:
>>> This patch fixes the regression, introduced by
>>>
>>> commit 17030f48e31adde5b043741c91ba143f5f7db0fd
>>> From: Rafa? Mi?ecki<zajec5@gmail.com>
>>> Date: Thu, 11 Aug 2011 17:16:27 +0200
>>> Subject: [PATCH] b43: support new RX header, noticed to be used in 598.314+
>>> fw
>>>
>>> in PIO case.
>>>
>>> Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
>>> ---
>>> diff --git a/drivers/net/wireless/b43/pio.c b/drivers/net/wireless/b43/pio.c
>>> index ce8a4bd..b64b64c 100644
>>> --- a/drivers/net/wireless/b43/pio.c
>>> +++ b/drivers/net/wireless/b43/pio.c
>>> @@ -617,9 +617,19 @@ static bool pio_rx_frame(struct b43_pio_rxqueue *q)
>>>    	const char *err_msg = NULL;
>>>    	struct b43_rxhdr_fw4 *rxhdr =
>>>    		(struct b43_rxhdr_fw4 *)wl->pio_scratchspace;
>>> +	size_t rxhdr_size = sizeof(*rxhdr);
>>>
>>>    	BUILD_BUG_ON(sizeof(wl->pio_scratchspace)<   sizeof(*rxhdr));
>>> -	memset(rxhdr, 0, sizeof(*rxhdr));
>>> +	switch (dev->fw.hdr_format) {
>>> +	case B43_FW_HDR_410:
>>> +	case B43_FW_HDR_351:
>>> +		rxhdr_size -= sizeof(rxhdr->format_598) -
>>> +			sizeof(rxhdr->format_351);
>>> +		break;
>>> +	case B43_FW_HDR_598:
>>> +		break;
>>> +	}
>>
>> I do not think the above will work for format_410. By my count, the format_410
>> struct is 4 bytes longer than the format_351 struct.
>
> I think you're looking at struct b43_txhdr, whereas the problem is in
> struct b43_rxhdr_fw4.
>
> Thanks
> Guennadi
>
>>
>> Even if it does work, I suggest using the following:
>>
>> 	size_t rxhdr_size;
>>
>>     	BUILD_BUG_ON(sizeof(wl->pio_scratchspace)<   sizeof(*rxhdr));
>> 	switch (dev->fw.hdr_format) {
>> 	case B43_FW_HDR_351:
>> 		rxhdr_size = sizeof(rxhdr->format_351);
>> 		break;
>> 	case B43_FW_HDR_410:

You are correct - I was looking in the wrong place. Even so, I prefer setting 
rxhdr_size in the case branches, and not adjust one preset earlier.

Larry

Re: [PATCH] b43: fix regression in PIO case

[Rafał Miłecki]

W dniu 26 grudnia 2011 18:28 użytkownik Guennadi Liakhovetski
<g.liakhovetski@gmx.de> napisał:
> This patch fixes the regression, introduced by
>
> commit 17030f48e31adde5b043741c91ba143f5f7db0fd
> From: Rafał Miłecki <zajec5@gmail.com>
> Date: Thu, 11 Aug 2011 17:16:27 +0200
> Subject: [PATCH] b43: support new RX header, noticed to be used in 598.314+ fw
>
> in PIO case.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> diff --git a/drivers/net/wireless/b43/pio.c b/drivers/net/wireless/b43/pio.c
> index ce8a4bd..b64b64c 100644
> --- a/drivers/net/wireless/b43/pio.c
> +++ b/drivers/net/wireless/b43/pio.c
> @@ -617,9 +617,19 @@ static bool pio_rx_frame(struct b43_pio_rxqueue *q)
>        const char *err_msg = NULL;
>        struct b43_rxhdr_fw4 *rxhdr =
>                (struct b43_rxhdr_fw4 *)wl->pio_scratchspace;
> +       size_t rxhdr_size = sizeof(*rxhdr);
>
>        BUILD_BUG_ON(sizeof(wl->pio_scratchspace) < sizeof(*rxhdr));
> -       memset(rxhdr, 0, sizeof(*rxhdr));
> +       switch (dev->fw.hdr_format) {
> +       case B43_FW_HDR_410:
> +       case B43_FW_HDR_351:
> +               rxhdr_size -= sizeof(rxhdr->format_598) -
> +                       sizeof(rxhdr->format_351);
> +               break;
> +       case B43_FW_HDR_598:
> +               break;
> +       }
> +       memset(rxhdr, 0, rxhdr_size);

Huuh, that's really tricky. Can you just do "normal" conditions as
Larry suggested, please?

-- 
Rafał

Re: [PATCH] b43: fix regression in PIO case

[Guennadi Liakhovetski]

On Tue, 27 Dec 2011, Rafał Miłecki wrote:

> W dniu 26 grudnia 2011 18:28 użytkownik Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> napisał:
> > This patch fixes the regression, introduced by
> >
> > commit 17030f48e31adde5b043741c91ba143f5f7db0fd
> > From: Rafał Miłecki <zajec5@gmail.com>
> > Date: Thu, 11 Aug 2011 17:16:27 +0200
> > Subject: [PATCH] b43: support new RX header, noticed to be used in 598.314+ fw
> >
> > in PIO case.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > diff --git a/drivers/net/wireless/b43/pio.c b/drivers/net/wireless/b43/pio.c
> > index ce8a4bd..b64b64c 100644
> > --- a/drivers/net/wireless/b43/pio.c
> > +++ b/drivers/net/wireless/b43/pio.c
> > @@ -617,9 +617,19 @@ static bool pio_rx_frame(struct b43_pio_rxqueue *q)
> >        const char *err_msg = NULL;
> >        struct b43_rxhdr_fw4 *rxhdr =
> >                (struct b43_rxhdr_fw4 *)wl->pio_scratchspace;
> > +       size_t rxhdr_size = sizeof(*rxhdr);
> >
> >        BUILD_BUG_ON(sizeof(wl->pio_scratchspace) < sizeof(*rxhdr));
> > -       memset(rxhdr, 0, sizeof(*rxhdr));
> > +       switch (dev->fw.hdr_format) {
> > +       case B43_FW_HDR_410:
> > +       case B43_FW_HDR_351:
> > +               rxhdr_size -= sizeof(rxhdr->format_598) -
> > +                       sizeof(rxhdr->format_351);
> > +               break;
> > +       case B43_FW_HDR_598:
> > +               break;
> > +       }
> > +       memset(rxhdr, 0, rxhdr_size);
> 
> Huuh, that's really tricky. Can you just do "normal" conditions as
> Larry suggested, please?

Sorry? I absolutely see nothing tricky there. Do you think this would look 
"less tricky" to you:

	switch (dev->fw.hdr_format) {
	case B43_FW_HDR_410:
	case B43_FW_HDR_351:
		rxhdr_size = offsetof(struct b43_rxhdr_fw4,
					format_351) +
			sizeof(rxhdr_size->format_351);
		break;
	case B43_FW_HDR_598:
		rxhdr_size = sizeof(*rxhdr);
		break;
	}

?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] b43: fix regression in PIO case

[Larry Finger]

On 12/27/2011 05:05 PM, Guennadi Liakhovetski wrote:
> On Tue, 27 Dec 2011, Rafał Miłecki wrote:
>
>> W dniu 26 grudnia 2011 18:28 użytkownik Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de>  napisał:
>>> This patch fixes the regression, introduced by
>>>
>>> commit 17030f48e31adde5b043741c91ba143f5f7db0fd
>>> From: Rafał Miłecki<zajec5@gmail.com>
>>> Date: Thu, 11 Aug 2011 17:16:27 +0200
>>> Subject: [PATCH] b43: support new RX header, noticed to be used in 598.314+ fw
>>>
>>> in PIO case.
>>>
>>> Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
>>> ---
>>> diff --git a/drivers/net/wireless/b43/pio.c b/drivers/net/wireless/b43/pio.c
>>> index ce8a4bd..b64b64c 100644
>>> --- a/drivers/net/wireless/b43/pio.c
>>> +++ b/drivers/net/wireless/b43/pio.c
>>> @@ -617,9 +617,19 @@ static bool pio_rx_frame(struct b43_pio_rxqueue *q)
>>>         const char *err_msg = NULL;
>>>         struct b43_rxhdr_fw4 *rxhdr =
>>>                 (struct b43_rxhdr_fw4 *)wl->pio_scratchspace;
>>> +       size_t rxhdr_size = sizeof(*rxhdr);
>>>
>>>         BUILD_BUG_ON(sizeof(wl->pio_scratchspace)<  sizeof(*rxhdr));
>>> -       memset(rxhdr, 0, sizeof(*rxhdr));
>>> +       switch (dev->fw.hdr_format) {
>>> +       case B43_FW_HDR_410:
>>> +       case B43_FW_HDR_351:
>>> +               rxhdr_size -= sizeof(rxhdr->format_598) -
>>> +                       sizeof(rxhdr->format_351);
>>> +               break;
>>> +       case B43_FW_HDR_598:
>>> +               break;
>>> +       }
>>> +       memset(rxhdr, 0, rxhdr_size);
>>
>> Huuh, that's really tricky. Can you just do "normal" conditions as
>> Larry suggested, please?
>
> Sorry? I absolutely see nothing tricky there. Do you think this would look
> "less tricky" to you:
>
> 	switch (dev->fw.hdr_format) {
> 	case B43_FW_HDR_410:
> 	case B43_FW_HDR_351:
> 		rxhdr_size = offsetof(struct b43_rxhdr_fw4,
> 					format_351) +
> 			sizeof(rxhdr_size->format_351);
> 		break;
> 	case B43_FW_HDR_598:
> 		rxhdr_size = sizeof(*rxhdr);
> 		break;
> 	}
>

How about this?

Index: wireless-testing-new/drivers/net/wireless/b43/pio.c
===================================================================
--- wireless-testing-new.orig/drivers/net/wireless/b43/pio.c
+++ wireless-testing-new/drivers/net/wireless/b43/pio.c
@@ -617,9 +617,20 @@ static bool pio_rx_frame(struct b43_pio_
         const char *err_msg = NULL;
         struct b43_rxhdr_fw4 *rxhdr =
                 (struct b43_rxhdr_fw4 *)wl->pio_scratchspace;
+       size_t rxhdr_size;

         BUILD_BUG_ON(sizeof(wl->pio_scratchspace) < sizeof(*rxhdr));
-       memset(rxhdr, 0, sizeof(*rxhdr));
+       switch (dev->fw.hdr_format) {
+       case B43_FW_HDR_410:
+       case B43_FW_HDR_351:
+               rxhdr_size = sizeof(rxhdr->format_351);
+               break;
+       case B43_FW_HDR_598:
+       default:
+               rxhdr_size = sizeof(rxhdr->format_598);
+               break;
+       }
+       memset(rxhdr, 0, rxhdr_size);

         /* Check if we have data and wait for it to get ready. */
         if (q->rev >= 8) {

Larry

Re: [PATCH] b43: fix regression in PIO case

[Guennadi Liakhovetski]

On Tue, 27 Dec 2011, Larry Finger wrote:

> On 12/27/2011 05:05 PM, Guennadi Liakhovetski wrote:
> > On Tue, 27 Dec 2011, Rafał Miłecki wrote:
> > 
> > > W dniu 26 grudnia 2011 18:28 użytkownik Guennadi Liakhovetski
> > > <g.liakhovetski@gmx.de>  napisał:
> > > > This patch fixes the regression, introduced by
> > > > 
> > > > commit 17030f48e31adde5b043741c91ba143f5f7db0fd
> > > > From: Rafał Miłecki<zajec5@gmail.com>
> > > > Date: Thu, 11 Aug 2011 17:16:27 +0200
> > > > Subject: [PATCH] b43: support new RX header, noticed to be used in
> > > > 598.314+ fw
> > > > 
> > > > in PIO case.
> > > > 
> > > > Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
> > > > ---
> > > > diff --git a/drivers/net/wireless/b43/pio.c
> > > > b/drivers/net/wireless/b43/pio.c
> > > > index ce8a4bd..b64b64c 100644
> > > > --- a/drivers/net/wireless/b43/pio.c
> > > > +++ b/drivers/net/wireless/b43/pio.c
> > > > @@ -617,9 +617,19 @@ static bool pio_rx_frame(struct b43_pio_rxqueue *q)
> > > >         const char *err_msg = NULL;
> > > >         struct b43_rxhdr_fw4 *rxhdr =
> > > >                 (struct b43_rxhdr_fw4 *)wl->pio_scratchspace;
> > > > +       size_t rxhdr_size = sizeof(*rxhdr);
> > > > 
> > > >         BUILD_BUG_ON(sizeof(wl->pio_scratchspace)<  sizeof(*rxhdr));
> > > > -       memset(rxhdr, 0, sizeof(*rxhdr));
> > > > +       switch (dev->fw.hdr_format) {
> > > > +       case B43_FW_HDR_410:
> > > > +       case B43_FW_HDR_351:
> > > > +               rxhdr_size -= sizeof(rxhdr->format_598) -
> > > > +                       sizeof(rxhdr->format_351);
> > > > +               break;
> > > > +       case B43_FW_HDR_598:
> > > > +               break;
> > > > +       }
> > > > +       memset(rxhdr, 0, rxhdr_size);
> > > 
> > > Huuh, that's really tricky. Can you just do "normal" conditions as
> > > Larry suggested, please?
> > 
> > Sorry? I absolutely see nothing tricky there. Do you think this would look
> > "less tricky" to you:
> > 
> > 	switch (dev->fw.hdr_format) {
> > 	case B43_FW_HDR_410:
> > 	case B43_FW_HDR_351:
> > 		rxhdr_size = offsetof(struct b43_rxhdr_fw4,
> > 					format_351) +
> > 			sizeof(rxhdr_size->format_351);
> > 		break;
> > 	case B43_FW_HDR_598:
> > 		rxhdr_size = sizeof(*rxhdr);
> > 		break;
> > 	}
> > 
> 
> How about this?
> 
> Index: wireless-testing-new/drivers/net/wireless/b43/pio.c
> ===================================================================
> --- wireless-testing-new.orig/drivers/net/wireless/b43/pio.c
> +++ wireless-testing-new/drivers/net/wireless/b43/pio.c
> @@ -617,9 +617,20 @@ static bool pio_rx_frame(struct b43_pio_
>         const char *err_msg = NULL;
>         struct b43_rxhdr_fw4 *rxhdr =
>                 (struct b43_rxhdr_fw4 *)wl->pio_scratchspace;
> +       size_t rxhdr_size;
> 
>         BUILD_BUG_ON(sizeof(wl->pio_scratchspace) < sizeof(*rxhdr));
> -       memset(rxhdr, 0, sizeof(*rxhdr));
> +       switch (dev->fw.hdr_format) {
> +       case B43_FW_HDR_410:
> +       case B43_FW_HDR_351:
> +               rxhdr_size = sizeof(rxhdr->format_351);
> +               break;
> +       case B43_FW_HDR_598:
> +       default:
> +               rxhdr_size = sizeof(rxhdr->format_598);
> +               break;
> +       }
> +       memset(rxhdr, 0, rxhdr_size);
> 
>         /* Check if we have data and wait for it to get ready. */
>         if (q->rev >= 8) {

I am sorry, I'm either being blind and stupid or you're trying to do 
something quite wrong there. struct b43_rxhdr_fw4 has a bunch of fields 
first, then at the end it has a union of two fields: format_598 and 
format_351, right? rxhdr is pointing at the struct itself. Before the 
offending patch memset() used to clean the whole struct. Now in your above 
version you calculate the size of one of those union fields and nullify 
that many bytes from the _beginning_ of the whole struct.

I've seen myself being wrong before, but here... I'll let you judge 
though.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] b43: fix regression in PIO case

[Larry Finger]

On 12/27/2011 06:00 PM, Guennadi Liakhovetski wrote:
> On Tue, 27 Dec 2011, Larry Finger wrote:
>
>> On 12/27/2011 05:05 PM, Guennadi Liakhovetski wrote:
>>> On Tue, 27 Dec 2011, Rafał Miłecki wrote:
>>>
>>>> W dniu 26 grudnia 2011 18:28 użytkownik Guennadi Liakhovetski
>>>> <g.liakhovetski@gmx.de>   napisał:
>>>>> This patch fixes the regression, introduced by
>>>>>
>>>>> commit 17030f48e31adde5b043741c91ba143f5f7db0fd
>>>>> From: Rafał Miłecki<zajec5@gmail.com>
>>>>> Date: Thu, 11 Aug 2011 17:16:27 +0200
>>>>> Subject: [PATCH] b43: support new RX header, noticed to be used in
>>>>> 598.314+ fw
>>>>>
>>>>> in PIO case.
>>>>>
>>>>> Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
>>>>> ---
>>>>> diff --git a/drivers/net/wireless/b43/pio.c
>>>>> b/drivers/net/wireless/b43/pio.c
>>>>> index ce8a4bd..b64b64c 100644
>>>>> --- a/drivers/net/wireless/b43/pio.c
>>>>> +++ b/drivers/net/wireless/b43/pio.c
>>>>> @@ -617,9 +617,19 @@ static bool pio_rx_frame(struct b43_pio_rxqueue *q)
>>>>>          const char *err_msg = NULL;
>>>>>          struct b43_rxhdr_fw4 *rxhdr =
>>>>>                  (struct b43_rxhdr_fw4 *)wl->pio_scratchspace;
>>>>> +       size_t rxhdr_size = sizeof(*rxhdr);
>>>>>
>>>>>          BUILD_BUG_ON(sizeof(wl->pio_scratchspace)<   sizeof(*rxhdr));
>>>>> -       memset(rxhdr, 0, sizeof(*rxhdr));
>>>>> +       switch (dev->fw.hdr_format) {
>>>>> +       case B43_FW_HDR_410:
>>>>> +       case B43_FW_HDR_351:
>>>>> +               rxhdr_size -= sizeof(rxhdr->format_598) -
>>>>> +                       sizeof(rxhdr->format_351);
>>>>> +               break;
>>>>> +       case B43_FW_HDR_598:
>>>>> +               break;
>>>>> +       }
>>>>> +       memset(rxhdr, 0, rxhdr_size);
>>>>
>>>> Huuh, that's really tricky. Can you just do "normal" conditions as
>>>> Larry suggested, please?
>>>
>>> Sorry? I absolutely see nothing tricky there. Do you think this would look
>>> "less tricky" to you:
>>>
>>> 	switch (dev->fw.hdr_format) {
>>> 	case B43_FW_HDR_410:
>>> 	case B43_FW_HDR_351:
>>> 		rxhdr_size = offsetof(struct b43_rxhdr_fw4,
>>> 					format_351) +
>>> 			sizeof(rxhdr_size->format_351);
>>> 		break;
>>> 	case B43_FW_HDR_598:
>>> 		rxhdr_size = sizeof(*rxhdr);
>>> 		break;
>>> 	}
>>>
>>
>> How about this?
>>
>> Index: wireless-testing-new/drivers/net/wireless/b43/pio.c
>> ===================================================================
>> --- wireless-testing-new.orig/drivers/net/wireless/b43/pio.c
>> +++ wireless-testing-new/drivers/net/wireless/b43/pio.c
>> @@ -617,9 +617,20 @@ static bool pio_rx_frame(struct b43_pio_
>>          const char *err_msg = NULL;
>>          struct b43_rxhdr_fw4 *rxhdr =
>>                  (struct b43_rxhdr_fw4 *)wl->pio_scratchspace;
>> +       size_t rxhdr_size;
>>
>>          BUILD_BUG_ON(sizeof(wl->pio_scratchspace)<  sizeof(*rxhdr));
>> -       memset(rxhdr, 0, sizeof(*rxhdr));
>> +       switch (dev->fw.hdr_format) {
>> +       case B43_FW_HDR_410:
>> +       case B43_FW_HDR_351:
>> +               rxhdr_size = sizeof(rxhdr->format_351);
>> +               break;
>> +       case B43_FW_HDR_598:
>> +       default:
>> +               rxhdr_size = sizeof(rxhdr->format_598);
>> +               break;
>> +       }
>> +       memset(rxhdr, 0, rxhdr_size);
>>
>>          /* Check if we have data and wait for it to get ready. */
>>          if (q->rev>= 8) {
>
> I am sorry, I'm either being blind and stupid or you're trying to do
> something quite wrong there. struct b43_rxhdr_fw4 has a bunch of fields
> first, then at the end it has a union of two fields: format_598 and
> format_351, right? rxhdr is pointing at the struct itself. Before the
> offending patch memset() used to clean the whole struct. Now in your above
> version you calculate the size of one of those union fields and nullify
> that many bytes from the _beginning_ of the whole struct.
>
> I've seen myself being wrong before, but here... I'll let you judge
> though.

No, you are right. I misread the code. Your patch above would work and is 
probably as clean as one can expect.

Larry

Re: [PATCH] b43: fix regression in PIO case

[Guennadi Liakhovetski]

On Tue, 27 Dec 2011, Larry Finger wrote:

[snip]

> No, you are right. I misread the code. Your patch above would work and is
> probably as clean as one can expect.

John, Linus, please ack / pull the original patch from this thread into 
3.2.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/