Message ID | 20220107041744.2388470-1-f.fainelli@gmail.com (mailing list archive) |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | bcma: get SoC device struct & assign dma_mask | expand |
On Thu, Jan 06, 2022 at 08:17:44PM -0800, Florian Fainelli wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > For bus devices to be fully usable it's required to set their DMA > parameters. > > For years it has been missing and remained unnoticed because of > mips_dma_alloc_coherent() silently handling the empty coherent_dma_mask. > Kernel 4.19 came with a lot of DMA changes and caused a regression on > the bcm47xx. Starting with the commit f8c55dc6e828 ("MIPS: use generic > dma noncoherent ops for simple noncoherent platforms") DMA coherent > allocations just fail. Example: > [ 1.114914] bgmac_bcma bcma0:2: Allocation of TX ring 0x200 failed > [ 1.121215] bgmac_bcma bcma0:2: Unable to alloc memory for DMA > [ 1.127626] bgmac_bcma: probe of bcma0:2 failed with error -12 > [ 1.133838] bgmac_bcma: Broadcom 47xx GBit MAC driver loaded > > This change fixes above regression in addition to the MIPS bcm47xx > commit 321c46b91550 ("MIPS: BCM47XX: Setup struct device for the SoC"). How did it take so long to notice this? > I don't know if this is what you had in mind. I am also not sure if we > should have the bcma_bus_type implement .dma_configure and set it to > platform_dma_configure? Unless you have an OF tree that declares DMA limitations there is no need to call platform_dma_configure. > --- a/drivers/bcma/host_soc.c > +++ b/drivers/bcma/host_soc.c > @@ -191,6 +191,8 @@ int __init bcma_host_soc_init(struct bcma_soc *soc) > struct bcma_bus *bus = &soc->bus; > int err; > > + bus->dev = soc->dev; > + > /* Scan bus and initialize it */ > err = bcma_bus_early_register(bus); > if (err) > diff --git a/drivers/bcma/main.c b/drivers/bcma/main.c > index 8e7ca3e4c8c4..6793c2ff60fd 100644 > --- a/drivers/bcma/main.c > +++ b/drivers/bcma/main.c > @@ -253,6 +253,8 @@ void bcma_prepare_core(struct bcma_bus *bus, struct bcma_device *core) > if (IS_ENABLED(CONFIG_OF) && bus->dev) { > core->dma_dev = bus->dev; > } else { > + if (!core->dev.coherent_dma_mask) > + core->dev.coherent_dma_mask = DMA_BIT_MASK(32); Is there any way coherent_dma_mask could already be set here? > core->dev.dma_mask = &core->dev.coherent_dma_mask; > core->dma_dev = &core->dev; Not really needed for the quick fix, but pointing the dma_mask to the coherent_dma_mask is a bad idea as it removes the often used feature to use a different mask for the cohernent allocations vs the streaming mappings.
On 10.01.2022 10:09, Christoph Hellwig wrote: > On Thu, Jan 06, 2022 at 08:17:44PM -0800, Florian Fainelli wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> For bus devices to be fully usable it's required to set their DMA >> parameters. >> >> For years it has been missing and remained unnoticed because of >> mips_dma_alloc_coherent() silently handling the empty coherent_dma_mask. >> Kernel 4.19 came with a lot of DMA changes and caused a regression on >> the bcm47xx. Starting with the commit f8c55dc6e828 ("MIPS: use generic >> dma noncoherent ops for simple noncoherent platforms") DMA coherent >> allocations just fail. Example: >> [ 1.114914] bgmac_bcma bcma0:2: Allocation of TX ring 0x200 failed >> [ 1.121215] bgmac_bcma bcma0:2: Unable to alloc memory for DMA >> [ 1.127626] bgmac_bcma: probe of bcma0:2 failed with error -12 >> [ 1.133838] bgmac_bcma: Broadcom 47xx GBit MAC driver loaded >> >> This change fixes above regression in addition to the MIPS bcm47xx >> commit 321c46b91550 ("MIPS: BCM47XX: Setup struct device for the SoC"). > > How did it take so long to notice this? I noticed it 3 years ago and sent semi-fix that wasn't accepted (for probably a good reason) back then: [PATCH wireless-drivers-next] bcma: get SoC device struct & copy its DMA params to the subdevices https://patchwork.kernel.org/project/linux-wireless/patch/20190121101121.24555-1-zajec5@gmail.com/ Since then OpenWrt carried on above fix as a downstream patch. I guess OpenWrt is the only active user of that code. Thanks a lot Florian for bringing that upstream.
On 1/10/22 1:09 AM, Christoph Hellwig wrote: > On Thu, Jan 06, 2022 at 08:17:44PM -0800, Florian Fainelli wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> For bus devices to be fully usable it's required to set their DMA >> parameters. >> >> For years it has been missing and remained unnoticed because of >> mips_dma_alloc_coherent() silently handling the empty coherent_dma_mask. >> Kernel 4.19 came with a lot of DMA changes and caused a regression on >> the bcm47xx. Starting with the commit f8c55dc6e828 ("MIPS: use generic >> dma noncoherent ops for simple noncoherent platforms") DMA coherent >> allocations just fail. Example: >> [ 1.114914] bgmac_bcma bcma0:2: Allocation of TX ring 0x200 failed >> [ 1.121215] bgmac_bcma bcma0:2: Unable to alloc memory for DMA >> [ 1.127626] bgmac_bcma: probe of bcma0:2 failed with error -12 >> [ 1.133838] bgmac_bcma: Broadcom 47xx GBit MAC driver loaded >> >> This change fixes above regression in addition to the MIPS bcm47xx >> commit 321c46b91550 ("MIPS: BCM47XX: Setup struct device for the SoC"). > > How did it take so long to notice this? > >> I don't know if this is what you had in mind. I am also not sure if we >> should have the bcma_bus_type implement .dma_configure and set it to >> platform_dma_configure? > > Unless you have an OF tree that declares DMA limitations there is no > need to call platform_dma_configure. OK, so we may have to implement that callback then because on ARM-based Devices we have BCMA plus Device Tree being used. Although none of those ARM platforms define any DMA apertures or restrictions, I suppose > >> --- a/drivers/bcma/host_soc.c >> +++ b/drivers/bcma/host_soc.c >> @@ -191,6 +191,8 @@ int __init bcma_host_soc_init(struct bcma_soc *soc) >> struct bcma_bus *bus = &soc->bus; >> int err; >> >> + bus->dev = soc->dev; >> + >> /* Scan bus and initialize it */ >> err = bcma_bus_early_register(bus); >> if (err) >> diff --git a/drivers/bcma/main.c b/drivers/bcma/main.c >> index 8e7ca3e4c8c4..6793c2ff60fd 100644 >> --- a/drivers/bcma/main.c >> +++ b/drivers/bcma/main.c >> @@ -253,6 +253,8 @@ void bcma_prepare_core(struct bcma_bus *bus, struct bcma_device *core) >> if (IS_ENABLED(CONFIG_OF) && bus->dev) { >> core->dma_dev = bus->dev; >> } else { >> + if (!core->dev.coherent_dma_mask) >> + core->dev.coherent_dma_mask = DMA_BIT_MASK(32); > > Is there any way coherent_dma_mask could already be set here? On MIPS-based devices (not using Device Tree), I don't see a way this could be set, however on ARM based devices, since they use Device Tree, maybe this value could be different than 0. I will run some tests on both types of devices and post an updated patch if relevant. Thanks! > >> core->dev.dma_mask = &core->dev.coherent_dma_mask; >> core->dma_dev = &core->dev; > > Not really needed for the quick fix, but pointing the dma_mask to the > coherent_dma_mask is a bad idea as it removes the often used feature > to use a different mask for the cohernent allocations vs the streaming > mappings. OK.
diff --git a/drivers/bcma/host_soc.c b/drivers/bcma/host_soc.c index 90d5bdc12e03..fd2e8ff17c76 100644 --- a/drivers/bcma/host_soc.c +++ b/drivers/bcma/host_soc.c @@ -191,6 +191,8 @@ int __init bcma_host_soc_init(struct bcma_soc *soc) struct bcma_bus *bus = &soc->bus; int err; + bus->dev = soc->dev; + /* Scan bus and initialize it */ err = bcma_bus_early_register(bus); if (err) diff --git a/drivers/bcma/main.c b/drivers/bcma/main.c index 8e7ca3e4c8c4..6793c2ff60fd 100644 --- a/drivers/bcma/main.c +++ b/drivers/bcma/main.c @@ -253,6 +253,8 @@ void bcma_prepare_core(struct bcma_bus *bus, struct bcma_device *core) if (IS_ENABLED(CONFIG_OF) && bus->dev) { core->dma_dev = bus->dev; } else { + if (!core->dev.coherent_dma_mask) + core->dev.coherent_dma_mask = DMA_BIT_MASK(32); core->dev.dma_mask = &core->dev.coherent_dma_mask; core->dma_dev = &core->dev; }