lkml.org 
[lkml]   [2011]   [Dec]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] b43: fix regression in PIO case
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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2011-12-28 01:13    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site