diff mbox

New brcmfmac errors in 4.12: brcmf_sdio_rxglom: sublen ... not multiple of 8

Message ID a4cf8400-3b11-4d09-9b46-6d6382423ce7@broadcom.com (mailing list archive)
State RFC
Delegated to: Kalle Valo
Headers show

Commit Message

Arend van Spriel May 14, 2017, 8:21 a.m. UTC
On 13-5-2017 16:55, Hans de Goede wrote:
> Hi,
> 
> On 13-05-17 15:39, Heiner Kallweit wrote:
>> Am 13.05.2017 um 14:35 schrieb Hans de Goede:
>>> <resend with the author of the commit causing this added>
>>>
>>> Hi,
>>>
>>> On 13-05-17 14:19, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> I've just rebased my personal kernel tree to what will soon be 4.12-rc1
>>>> and I'm getting my dmesg log filled with the following errors:
>>>>
>>>> [   32.528271] brcmfmac: brcmf_sdio_rxglom: sublen 524 not multiple
>>>> of 8
>>>> [   32.528296] brcmfmac: brcmf_sdio_rxglom: sublen 84 not multiple of 8
>>>> [   33.063241] brcmfmac: brcmf_sdio_rxglom: sublen 1492 not multiple
>>>> of 8
>>>>
>> After a brief look at the code I'm not sure that the check actually
>> checks
>> for an error condition. Apart from the error messages:
>> Do you face issues with the functionality of the driver?
> 
> Yes after a while I get -ETIMEOUT errors for any sdio transfers
> to the device. But I'm not sure if this is caused by this commit,
> I think I've seen this once with 4.11 too.
> 
> I've reverted the commit for now, but I'm fine with instead of
> doing the revert dropping the error check if the brcmfmac developers
> think that is ok. Currently the ETIMEOUT seems to be gone, so
> if dropping the revert causes it to re-appear then we know more.

Instead of reverting please give this patch a try and let me know if it
works for you.

Regards,
Arend
---

Comments

Hans de Goede May 14, 2017, 11:45 a.m. UTC | #1
Hi,

On 14-05-17 10:21, Arend Van Spriel wrote:
> On 13-5-2017 16:55, Hans de Goede wrote:
>> Hi,
>>
>> On 13-05-17 15:39, Heiner Kallweit wrote:
>>> Am 13.05.2017 um 14:35 schrieb Hans de Goede:
>>>> <resend with the author of the commit causing this added>
>>>>
>>>> Hi,
>>>>
>>>> On 13-05-17 14:19, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> I've just rebased my personal kernel tree to what will soon be 4.12-rc1
>>>>> and I'm getting my dmesg log filled with the following errors:
>>>>>
>>>>> [   32.528271] brcmfmac: brcmf_sdio_rxglom: sublen 524 not multiple
>>>>> of 8
>>>>> [   32.528296] brcmfmac: brcmf_sdio_rxglom: sublen 84 not multiple of 8
>>>>> [   33.063241] brcmfmac: brcmf_sdio_rxglom: sublen 1492 not multiple
>>>>> of 8
>>>>>
>>> After a brief look at the code I'm not sure that the check actually
>>> checks
>>> for an error condition. Apart from the error messages:
>>> Do you face issues with the functionality of the driver?
>>
>> Yes after a while I get -ETIMEOUT errors for any sdio transfers
>> to the device. But I'm not sure if this is caused by this commit,
>> I think I've seen this once with 4.11 too.
>>
>> I've reverted the commit for now, but I'm fine with instead of
>> doing the revert dropping the error check if the brcmfmac developers
>> think that is ok. Currently the ETIMEOUT seems to be gone, so
>> if dropping the revert causes it to re-appear then we know more.
> 
> Instead of reverting please give this patch a try and let me know if it
> works for you.
> 
> Regards,
> Arend
> ---
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> b/drivers/n
> index fc64b89..e034500 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -3422,7 +3422,7 @@ static int brcmf_sdio_bus_preinit(struct device *dev)
>                  /* otherwise, set txglomalign */
>                  value = sdiodev->settings->bus.sdio.sd_sgentry_align;
>                  /* SDIO ADMA requires at least 32 bit alignment */
> -               value = max_t(u32, value, 4);
> +               value = max_t(u32, value, ALIGNMENT);
>                  err = brcmf_iovar_data_set(dev, "bus:txglomalign", &value,
>                                             sizeof(u32));
>          }
> 

I can confirm that the above fix fixes the messages.

Regards,

Hans
Arend van Spriel May 14, 2017, 12:56 p.m. UTC | #2
On Sun, May 14, 2017 at 1:45 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 14-05-17 10:21, Arend Van Spriel wrote:
>>
>> On 13-5-2017 16:55, Hans de Goede wrote:
>>>
>>> Hi,
>>>
>>> On 13-05-17 15:39, Heiner Kallweit wrote:
>>>>
>>>> Am 13.05.2017 um 14:35 schrieb Hans de Goede:
>>>>>
>>>>> <resend with the author of the commit causing this added>
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 13-05-17 14:19, Hans de Goede wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I've just rebased my personal kernel tree to what will soon be
>>>>>> 4.12-rc1
>>>>>> and I'm getting my dmesg log filled with the following errors:
>>>>>>
>>>>>> [   32.528271] brcmfmac: brcmf_sdio_rxglom: sublen 524 not multiple
>>>>>> of 8
>>>>>> [   32.528296] brcmfmac: brcmf_sdio_rxglom: sublen 84 not multiple of
>>>>>> 8
>>>>>> [   33.063241] brcmfmac: brcmf_sdio_rxglom: sublen 1492 not multiple
>>>>>> of 8
>>>>>>
>>>> After a brief look at the code I'm not sure that the check actually
>>>> checks
>>>> for an error condition. Apart from the error messages:
>>>> Do you face issues with the functionality of the driver?
>>>
>>>
>>> Yes after a while I get -ETIMEOUT errors for any sdio transfers
>>> to the device. But I'm not sure if this is caused by this commit,
>>> I think I've seen this once with 4.11 too.
>>>
>>> I've reverted the commit for now, but I'm fine with instead of
>>> doing the revert dropping the error check if the brcmfmac developers
>>> think that is ok. Currently the ETIMEOUT seems to be gone, so
>>> if dropping the revert causes it to re-appear then we know more.
>>
>>
>> Instead of reverting please give this patch a try and let me know if it
>> works for you.
>>
>> Regards,
>> Arend
>> ---
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> b/drivers/n
>> index fc64b89..e034500 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> @@ -3422,7 +3422,7 @@ static int brcmf_sdio_bus_preinit(struct device
>> *dev)
>>                  /* otherwise, set txglomalign */
>>                  value = sdiodev->settings->bus.sdio.sd_sgentry_align;
>>                  /* SDIO ADMA requires at least 32 bit alignment */
>> -               value = max_t(u32, value, 4);
>> +               value = max_t(u32, value, ALIGNMENT);
>>                  err = brcmf_iovar_data_set(dev, "bus:txglomalign",
>> &value,
>>                                             sizeof(u32));
>>          }
>>
>
> I can confirm that the above fix fixes the messages.

Thanks, Hans

I will send out a patch with your Tested-by:

Regards,
Arend
Arend van Spriel May 19, 2017, 6:03 p.m. UTC | #3
On 14-5-2017 13:45, Hans de Goede wrote:
> Hi,
> 
> On 14-05-17 10:21, Arend Van Spriel wrote:
>> On 13-5-2017 16:55, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 13-05-17 15:39, Heiner Kallweit wrote:
>>>> Am 13.05.2017 um 14:35 schrieb Hans de Goede:
>>>>> <resend with the author of the commit causing this added>
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 13-05-17 14:19, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I've just rebased my personal kernel tree to what will soon be
>>>>>> 4.12-rc1
>>>>>> and I'm getting my dmesg log filled with the following errors:
>>>>>>
>>>>>> [   32.528271] brcmfmac: brcmf_sdio_rxglom: sublen 524 not multiple
>>>>>> of 8
>>>>>> [   32.528296] brcmfmac: brcmf_sdio_rxglom: sublen 84 not multiple
>>>>>> of 8
>>>>>> [   33.063241] brcmfmac: brcmf_sdio_rxglom: sublen 1492 not multiple
>>>>>> of 8
>>>>>>
>>>> After a brief look at the code I'm not sure that the check actually
>>>> checks
>>>> for an error condition. Apart from the error messages:
>>>> Do you face issues with the functionality of the driver?
>>>
>>> Yes after a while I get -ETIMEOUT errors for any sdio transfers
>>> to the device. But I'm not sure if this is caused by this commit,
>>> I think I've seen this once with 4.11 too.
>>>
>>> I've reverted the commit for now, but I'm fine with instead of
>>> doing the revert dropping the error check if the brcmfmac developers
>>> think that is ok. Currently the ETIMEOUT seems to be gone, so
>>> if dropping the revert causes it to re-appear then we know more.
>>
>> Instead of reverting please give this patch a try and let me know if it
>> works for you.
>>
>> Regards,
>> Arend
>> ---
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> b/drivers/n
>> index fc64b89..e034500 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> @@ -3422,7 +3422,7 @@ static int brcmf_sdio_bus_preinit(struct device
>> *dev)
>>                  /* otherwise, set txglomalign */
>>                  value = sdiodev->settings->bus.sdio.sd_sgentry_align;
>>                  /* SDIO ADMA requires at least 32 bit alignment */
>> -               value = max_t(u32, value, 4);
>> +               value = max_t(u32, value, ALIGNMENT);
>>                  err = brcmf_iovar_data_set(dev, "bus:txglomalign",
>> &value,
>>                                             sizeof(u32));
>>          }
>>
> 
> I can confirm that the above fix fixes the messages.

Heiner,

Could I get your ack or even better your test verdict as well?

Regards,
Arend
Heiner Kallweit May 19, 2017, 7:07 p.m. UTC | #4
Am 19.05.2017 um 20:03 schrieb Arend Van Spriel:
> On 14-5-2017 13:45, Hans de Goede wrote:
>> Hi,
>>
>> On 14-05-17 10:21, Arend Van Spriel wrote:
>>> On 13-5-2017 16:55, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 13-05-17 15:39, Heiner Kallweit wrote:
>>>>> Am 13.05.2017 um 14:35 schrieb Hans de Goede:
>>>>>> <resend with the author of the commit causing this added>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 13-05-17 14:19, Hans de Goede wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I've just rebased my personal kernel tree to what will soon be
>>>>>>> 4.12-rc1
>>>>>>> and I'm getting my dmesg log filled with the following errors:
>>>>>>>
>>>>>>> [   32.528271] brcmfmac: brcmf_sdio_rxglom: sublen 524 not multiple
>>>>>>> of 8
>>>>>>> [   32.528296] brcmfmac: brcmf_sdio_rxglom: sublen 84 not multiple
>>>>>>> of 8
>>>>>>> [   33.063241] brcmfmac: brcmf_sdio_rxglom: sublen 1492 not multiple
>>>>>>> of 8
>>>>>>>
>>>>> After a brief look at the code I'm not sure that the check actually
>>>>> checks
>>>>> for an error condition. Apart from the error messages:
>>>>> Do you face issues with the functionality of the driver?
>>>>
>>>> Yes after a while I get -ETIMEOUT errors for any sdio transfers
>>>> to the device. But I'm not sure if this is caused by this commit,
>>>> I think I've seen this once with 4.11 too.
>>>>
>>>> I've reverted the commit for now, but I'm fine with instead of
>>>> doing the revert dropping the error check if the brcmfmac developers
>>>> think that is ok. Currently the ETIMEOUT seems to be gone, so
>>>> if dropping the revert causes it to re-appear then we know more.
>>>
>>> Instead of reverting please give this patch a try and let me know if it
>>> works for you.
>>>
>>> Regards,
>>> Arend
>>> ---
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> b/drivers/n
>>> index fc64b89..e034500 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> @@ -3422,7 +3422,7 @@ static int brcmf_sdio_bus_preinit(struct device
>>> *dev)
>>>                  /* otherwise, set txglomalign */
>>>                  value = sdiodev->settings->bus.sdio.sd_sgentry_align;
>>>                  /* SDIO ADMA requires at least 32 bit alignment */
>>> -               value = max_t(u32, value, 4);
>>> +               value = max_t(u32, value, ALIGNMENT);
>>>                  err = brcmf_iovar_data_set(dev, "bus:txglomalign",
>>> &value,
>>>                                             sizeof(u32));
>>>          }
>>>
>>
>> I can confirm that the above fix fixes the messages.
> 
> Heiner,
> 
> Could I get your ack or even better your test verdict as well?
> 
Unfortunately I don't have hardware to test this (as sdio isn't available
on Odroid C2). However the patch looks logical and good to me.

I add Helmut as he gave his Tested-by for commit 5ef1ecf060f28e
"mmc: sdio: fix alignment issue in struct sdio_func".
Maybe he can test your patch as well.

Rgds, Heiner

> Regards,
> Arend
>
Arend van Spriel May 19, 2017, 7:24 p.m. UTC | #5
On 19-5-2017 21:07, Heiner Kallweit wrote:
> Am 19.05.2017 um 20:03 schrieb Arend Van Spriel:
>> On 14-5-2017 13:45, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 14-05-17 10:21, Arend Van Spriel wrote:
>>>> On 13-5-2017 16:55, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 13-05-17 15:39, Heiner Kallweit wrote:
>>>>>> Am 13.05.2017 um 14:35 schrieb Hans de Goede:
>>>>>>> <resend with the author of the commit causing this added>
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 13-05-17 14:19, Hans de Goede wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I've just rebased my personal kernel tree to what will soon be
>>>>>>>> 4.12-rc1
>>>>>>>> and I'm getting my dmesg log filled with the following errors:
>>>>>>>>
>>>>>>>> [   32.528271] brcmfmac: brcmf_sdio_rxglom: sublen 524 not multiple
>>>>>>>> of 8
>>>>>>>> [   32.528296] brcmfmac: brcmf_sdio_rxglom: sublen 84 not multiple
>>>>>>>> of 8
>>>>>>>> [   33.063241] brcmfmac: brcmf_sdio_rxglom: sublen 1492 not multiple
>>>>>>>> of 8
>>>>>>>>
>>>>>> After a brief look at the code I'm not sure that the check actually
>>>>>> checks
>>>>>> for an error condition. Apart from the error messages:
>>>>>> Do you face issues with the functionality of the driver?
>>>>>
>>>>> Yes after a while I get -ETIMEOUT errors for any sdio transfers
>>>>> to the device. But I'm not sure if this is caused by this commit,
>>>>> I think I've seen this once with 4.11 too.
>>>>>
>>>>> I've reverted the commit for now, but I'm fine with instead of
>>>>> doing the revert dropping the error check if the brcmfmac developers
>>>>> think that is ok. Currently the ETIMEOUT seems to be gone, so
>>>>> if dropping the revert causes it to re-appear then we know more.
>>>>
>>>> Instead of reverting please give this patch a try and let me know if it
>>>> works for you.
>>>>
>>>> Regards,
>>>> Arend
>>>> ---
>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>> b/drivers/n
>>>> index fc64b89..e034500 100644
>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>> @@ -3422,7 +3422,7 @@ static int brcmf_sdio_bus_preinit(struct device
>>>> *dev)
>>>>                  /* otherwise, set txglomalign */
>>>>                  value = sdiodev->settings->bus.sdio.sd_sgentry_align;
>>>>                  /* SDIO ADMA requires at least 32 bit alignment */
>>>> -               value = max_t(u32, value, 4);
>>>> +               value = max_t(u32, value, ALIGNMENT);
>>>>                  err = brcmf_iovar_data_set(dev, "bus:txglomalign",
>>>> &value,
>>>>                                             sizeof(u32));
>>>>          }
>>>>
>>>
>>> I can confirm that the above fix fixes the messages.
>>
>> Heiner,
>>
>> Could I get your ack or even better your test verdict as well?
>>
> Unfortunately I don't have hardware to test this (as sdio isn't available
> on Odroid C2). However the patch looks logical and good to me.
> 
> I add Helmut as he gave his Tested-by for commit 5ef1ecf060f28e
> "mmc: sdio: fix alignment issue in struct sdio_func".
> Maybe he can test your patch as well.

Ok. Thanks,
Arend

> Rgds, Heiner
> 
>> Regards,
>> Arend
>>
>
diff mbox

Patch

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
b/drivers/n
index fc64b89..e034500 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -3422,7 +3422,7 @@  static int brcmf_sdio_bus_preinit(struct device *dev)
                /* otherwise, set txglomalign */
                value = sdiodev->settings->bus.sdio.sd_sgentry_align;
                /* SDIO ADMA requires at least 32 bit alignment */
-               value = max_t(u32, value, 4);
+               value = max_t(u32, value, ALIGNMENT);
                err = brcmf_iovar_data_set(dev, "bus:txglomalign", &value,
                                           sizeof(u32));
        }