| Message ID | 569D61BA.8020701@broadcom.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
On 18/01/16 22:05, Arend van Spriel wrote: > On 18-1-2016 12:30, Marc Zyngier wrote: >> Hi Kalle, >> >> On 16/01/16 11:57, Kalle Valo wrote: >>> Marc Zyngier <marc.zyngier@arm.com> writes: >>> >>>> David, Hante, >>>> >>>> On 13/01/16 02:51, David Miller wrote: >>>> >>>> [...] >>>> >>>>> Hante Meuleman (33): >>>> [...] >>>>> brcmfmac: Move all module parameters to one place >>> >>> As a reminder to myself this is the commit id: >>> >>> 7d34b0560567 brcmfmac: Move all module parameters to one place >>> >>>> This particular patch breaks one of my boxes in a spectacular way: >>>> >>>> [ 3.602155] Unable to handle kernel paging request at virtual address 000027e4 >>>> [ 3.602160] pgd = c0003000 >>>> [ 3.602169] [000027e4] *pgd=80000040004003, *pmd=00000000 >>>> [ 3.602181] Internal error: Oops: 206 [#1] PREEMPT SMP ARM >>> >>> [...] >>> >>>> This is caused by this hunk: >>>> >>>> @@ -890,7 +887,8 @@ static void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev) >>>> if (!sdiodev->sg_support) >>>> return; >>>> >>>> - nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE, brcmf_sdiod_txglomsz); >>>> + nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE, >>>> + sdiodev->bus_if->drvr->settings->sdiod_txglomsz); >>>> nents += (nents >> 4) + 1; >>>> >>>> WARN_ON(nents > sdiodev->max_segment_count); >>>> >>>> were drvr->settings is NULL (as the settings allocation seems to be done >>>> much later). The fix is not completely obvious to me (probably requires >>>> pushing the call to brcmf_mp_device_attach() down into the various bus >>>> specific functions). An alternative would be to restore the txglomsz >>>> parameter as it was before and not rely on settings being allocated. >>> >>> Should we revert the patch or can you Hante fix this? The revert doesn't >>> seem to be trivial so I would appreciate if someone could send a patch. >> >> I've worked out a partial revert (see below) that allows my system to >> boot, but I'd rather see a proper fix from the maintainer of this code. > > Hi Marc, > > Thanks for the patch, but Hante has created a different patch basically > deferring the allocation of the sgtable. Feel free to give it a spin on > your box and share the results. Hi Arend, This patch fixes indeed the problem, thanks (I had to undo the mangling your mailer had done, though). So feel free to add my: Reported-by: Marc Zyngier <marc.zyngier@arm.com> Tested-by: Marc Zyngier <marc.zyngier@arm.com> It would be good if this could make it in -rc1. > I am looking for a arm64 platform to add to my test machines so if > you can recommend one (with PCIe). Note that this crash happened with 32bit ARM, without PCIe (the box is a "Cubietruck"). As for arm64, there is now quite a few machines out there (ARM sells a dev board called Juno, but there is also various offerings with AMD Opteron A1100, Cavium ThunderX, APM X-Gene, and maybe even some Broadcom based systems, who knows! ;-). Thanks, M.
On 19-1-2016 9:55, Marc Zyngier wrote: > On 18/01/16 22:05, Arend van Spriel wrote: >> On 18-1-2016 12:30, Marc Zyngier wrote: >>> Hi Kalle, >>> >>> On 16/01/16 11:57, Kalle Valo wrote: >>>> Marc Zyngier <marc.zyngier@arm.com> writes: >>>> >>>>> David, Hante, >>>>> >>>>> On 13/01/16 02:51, David Miller wrote: >>>>> >>>>> [...] >>>>> >>>>>> Hante Meuleman (33): >>>>> [...] >>>>>> brcmfmac: Move all module parameters to one place >>>> >>>> As a reminder to myself this is the commit id: >>>> >>>> 7d34b0560567 brcmfmac: Move all module parameters to one place >>>> >>>>> This particular patch breaks one of my boxes in a spectacular way: >>>>> >>>>> [ 3.602155] Unable to handle kernel paging request at virtual address 000027e4 >>>>> [ 3.602160] pgd = c0003000 >>>>> [ 3.602169] [000027e4] *pgd=80000040004003, *pmd=00000000 >>>>> [ 3.602181] Internal error: Oops: 206 [#1] PREEMPT SMP ARM >>>> >>>> [...] >>>> >>>>> This is caused by this hunk: >>>>> >>>>> @@ -890,7 +887,8 @@ static void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev) >>>>> if (!sdiodev->sg_support) >>>>> return; >>>>> >>>>> - nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE, brcmf_sdiod_txglomsz); >>>>> + nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE, >>>>> + sdiodev->bus_if->drvr->settings->sdiod_txglomsz); >>>>> nents += (nents >> 4) + 1; >>>>> >>>>> WARN_ON(nents > sdiodev->max_segment_count); >>>>> >>>>> were drvr->settings is NULL (as the settings allocation seems to be done >>>>> much later). The fix is not completely obvious to me (probably requires >>>>> pushing the call to brcmf_mp_device_attach() down into the various bus >>>>> specific functions). An alternative would be to restore the txglomsz >>>>> parameter as it was before and not rely on settings being allocated. >>>> >>>> Should we revert the patch or can you Hante fix this? The revert doesn't >>>> seem to be trivial so I would appreciate if someone could send a patch. >>> >>> I've worked out a partial revert (see below) that allows my system to >>> boot, but I'd rather see a proper fix from the maintainer of this code. >> >> Hi Marc, >> >> Thanks for the patch, but Hante has created a different patch basically >> deferring the allocation of the sgtable. Feel free to give it a spin on >> your box and share the results. > > Hi Arend, > > This patch fixes indeed the problem, thanks (I had to undo the mangling > your mailer had done, though). So feel free to add my: Thanks, Marc I started using thunderbird on windows 7. Probably better revert to the one on Citrix. > Reported-by: Marc Zyngier <marc.zyngier@arm.com> > Tested-by: Marc Zyngier <marc.zyngier@arm.com> > > It would be good if this could make it in -rc1. Will try to submit a clean patch to Kalle. >> I am looking for a arm64 platform to add to my test machines so if >> you can recommend one (with PCIe). > > Note that this crash happened with 32bit ARM, without PCIe (the box is a > "Cubietruck"). > > As for arm64, there is now quite a few machines out there (ARM sells a > dev board called Juno, but there is also various offerings with AMD > Opteron A1100, Cavium ThunderX, APM X-Gene, and maybe even some Broadcom > based systems, who knows! ;-). Thanks. Actually found something in our corporate spam: http://www.broadcom.com/press/release.php?id=s948493 Regards, Arend > Thanks, > > M. > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19/01/16 09:36, Arend van Spriel wrote: > On 19-1-2016 9:55, Marc Zyngier wrote: >> On 18/01/16 22:05, Arend van Spriel wrote: >>> On 18-1-2016 12:30, Marc Zyngier wrote: >>>> Hi Kalle, >>>> >>>> On 16/01/16 11:57, Kalle Valo wrote: >>>>> Marc Zyngier <marc.zyngier@arm.com> writes: >>>>> >>>>>> David, Hante, >>>>>> >>>>>> On 13/01/16 02:51, David Miller wrote: >>>>>> >>>>>> [...] >>>>>> >>>>>>> Hante Meuleman (33): >>>>>> [...] >>>>>>> brcmfmac: Move all module parameters to one place >>>>> >>>>> As a reminder to myself this is the commit id: >>>>> >>>>> 7d34b0560567 brcmfmac: Move all module parameters to one place >>>>> >>>>>> This particular patch breaks one of my boxes in a spectacular way: >>>>>> >>>>>> [ 3.602155] Unable to handle kernel paging request at virtual address 000027e4 >>>>>> [ 3.602160] pgd = c0003000 >>>>>> [ 3.602169] [000027e4] *pgd=80000040004003, *pmd=00000000 >>>>>> [ 3.602181] Internal error: Oops: 206 [#1] PREEMPT SMP ARM >>>>> >>>>> [...] >>>>> >>>>>> This is caused by this hunk: >>>>>> >>>>>> @@ -890,7 +887,8 @@ static void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev) >>>>>> if (!sdiodev->sg_support) >>>>>> return; >>>>>> >>>>>> - nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE, brcmf_sdiod_txglomsz); >>>>>> + nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE, >>>>>> + sdiodev->bus_if->drvr->settings->sdiod_txglomsz); >>>>>> nents += (nents >> 4) + 1; >>>>>> >>>>>> WARN_ON(nents > sdiodev->max_segment_count); >>>>>> >>>>>> were drvr->settings is NULL (as the settings allocation seems to be done >>>>>> much later). The fix is not completely obvious to me (probably requires >>>>>> pushing the call to brcmf_mp_device_attach() down into the various bus >>>>>> specific functions). An alternative would be to restore the txglomsz >>>>>> parameter as it was before and not rely on settings being allocated. >>>>> >>>>> Should we revert the patch or can you Hante fix this? The revert doesn't >>>>> seem to be trivial so I would appreciate if someone could send a patch. >>>> >>>> I've worked out a partial revert (see below) that allows my system to >>>> boot, but I'd rather see a proper fix from the maintainer of this code. >>> >>> Hi Marc, >>> >>> Thanks for the patch, but Hante has created a different patch basically >>> deferring the allocation of the sgtable. Feel free to give it a spin on >>> your box and share the results. >> >> Hi Arend, >> >> This patch fixes indeed the problem, thanks (I had to undo the mangling >> your mailer had done, though). So feel free to add my: > > Thanks, Marc > > I started using thunderbird on windows 7. Probably better revert to the > one on Citrix. > >> Reported-by: Marc Zyngier <marc.zyngier@arm.com> >> Tested-by: Marc Zyngier <marc.zyngier@arm.com> >> >> It would be good if this could make it in -rc1. > > Will try to submit a clean patch to Kalle. > >>> I am looking for a arm64 platform to add to my test machines so if >>> you can recommend one (with PCIe). >> >> Note that this crash happened with 32bit ARM, without PCIe (the box is a >> "Cubietruck"). >> >> As for arm64, there is now quite a few machines out there (ARM sells a >> dev board called Juno, but there is also various offerings with AMD >> Opteron A1100, Cavium ThunderX, APM X-Gene, and maybe even some Broadcom >> based systems, who knows! ;-). > > Thanks. Actually found something in our corporate spam: > > http://www.broadcom.com/press/release.php?id=s948493 Ah, quality marketroid blurb! :-) Yeah, this looks promising, assuming it is available in the not too distant future (I think I reviewed some of the PCIe code for that platform...). Thanks, M.
Marc Zyngier <marc.zyngier@arm.com> writes: >>>> Should we revert the patch or can you Hante fix this? The revert doesn't >>>> seem to be trivial so I would appreciate if someone could send a patch. >>> >>> I've worked out a partial revert (see below) that allows my system to >>> boot, but I'd rather see a proper fix from the maintainer of this code. >> >> Hi Marc, >> >> Thanks for the patch, but Hante has created a different patch basically >> deferring the allocation of the sgtable. Feel free to give it a spin on >> your box and share the results. > > Hi Arend, > > This patch fixes indeed the problem, thanks (I had to undo the mangling > your mailer had done, though). So feel free to add my: > > Reported-by: Marc Zyngier <marc.zyngier@arm.com> > Tested-by: Marc Zyngier <marc.zyngier@arm.com> > > It would be good if this could make it in -rc1. I doubt it makes it to -rc1, most likely to -rc2. But let's see.
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index 5363739..b98db8a 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -879,11 +879,24 @@ int brcmf_sdiod_abort(struct brcmf_sdio_dev *sdiodev, uint fn) return 0; } -static void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev) +void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev) { + struct sdio_func *func; + struct mmc_host *host; + uint max_blocks; uint nents; int err; + func = sdiodev->func[2]; + host = func->card->host; + sdiodev->sg_support = host->max_segs > 1; + max_blocks = min_t(uint, host->max_blk_count, 511u); + sdiodev->max_request_size = min_t(uint, host->max_req_size, + max_blocks * func->cur_blksize); + sdiodev->max_segment_count = min_t(uint, host->max_segs, + SG_MAX_SINGLE_ALLOC); + sdiodev->max_segment_size = host->max_seg_size; + if (!sdiodev->sg_support) return; @@ -1021,9 +1034,6 @@ static void brcmf_sdiod_host_fixup(struct mmc_host *host) static int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev) { - struct sdio_func *func; - struct mmc_host *host; - uint max_blocks; int ret = 0; sdiodev->num_funcs = 2; @@ -1054,26 +1064,6 @@ static int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev) goto out; } - /* - * determine host related variables after brcmf_sdiod_probe() - * as func->cur_blksize is properly set and F2 init has been - * completed successfully. - */ - func = sdiodev->func[2]; - host = func->card->host; - sdiodev->sg_support = host->max_segs > 1; - max_blocks = min_t(uint, host->max_blk_count, 511u); - sdiodev->max_request_size = min_t(uint, host->max_req_size, - max_blocks * func->cur_blksize); - sdiodev->max_segment_count = min_t(uint, host->max_segs, - SG_MAX_SINGLE_ALLOC); - sdiodev->max_segment_size = host->max_seg_size; - - /* allocate scatter-gather table. sg support - * will be disabled upon allocation failure. - */ - brcmf_sdiod_sgtable_alloc(sdiodev); - ret = brcmf_sdiod_freezer_attach(sdiodev); if (ret) goto out; @@ -1084,7 +1074,7 @@ static int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev) ret = -ENODEV; goto out; } - brcmf_sdiod_host_fixup(host); + brcmf_sdiod_host_fixup(sdiodev->func[2]->card->host); out: if (ret) brcmf_sdiod_remove(sdiodev); diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c index dd66143..a14d9d9d 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c @@ -4114,6 +4114,11 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev) goto fail; } + /* allocate scatter-gather table. sg support + * will be disabled upon allocation failure. + */ + brcmf_sdiod_sgtable_alloc(bus->sdiodev); + /* Query the F2 block size, set roundup accordingly */ bus->blocksize = bus->sdiodev->func[2]->cur_blksize; bus->roundup = min(max_roundup, bus->blocksize); diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h index 5ec7a6d..23f2231 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h @@ -342,6 +342,7 @@ int brcmf_sdiod_ramrw(struct brcmf_sdio_dev *sdiodev, bool write, u32 address, /* Issue an abort to the specified function */ int brcmf_sdiod_abort(struct brcmf_sdio_dev *sdiodev, uint fn); +void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev); void brcmf_sdiod_change_state(struct brcmf_sdio_dev *sdiodev, enum brcmf_sdiod_state state);