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    [W:0.031 / U:150.708 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site