| Message ID | a4cf8400-3b11-4d09-9b46-6d6382423ce7@broadcom.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Kalle Valo |
| Headers | show |
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
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
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
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 >
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 --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)); }