| Message ID | 20240920155820.3340081-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/1] dmaengine: dw: Select only supported masters for ACPI devices | expand |
Hi Andy On Fri, Sep 20, 2024 at 06:56:17PM +0300, Andy Shevchenko wrote: > From: Serge Semin <fancer.lancer@gmail.com> > > The recently submitted fix-commit revealed a problem in the iDMA 32-bit > platform code. Even though the controller supported only a single master > the dw_dma_acpi_filter() method hard-coded two master interfaces with IDs > 0 and 1. As a result the sanity check implemented in the commit > b336268dde75 ("dmaengine: dw: Add peripheral bus width verification") > got incorrect interface data width and thus prevented the client drivers > from configuring the DMA-channel with the EINVAL error returned. E.g., > the next error was printed for the PXA2xx SPI controller driver trying > to configure the requested channels: > > > [ 164.525604] pxa2xx_spi_pci 0000:00:07.1: DMA slave config failed > > [ 164.536105] pxa2xx_spi_pci 0000:00:07.1: failed to get DMA TX descriptor > > [ 164.543213] spidev spi-SPT0001:00: SPI transfer failed: -16 > > The problem would have been spotted much earlier if the iDMA 32-bit > controller supported more than one master interfaces. But since it > supports just a single master and the iDMA 32-bit specific code just > ignores the master IDs in the CTLLO preparation method, the issue has > been gone unnoticed so far. > > Fix the problem by specifying the default master ID for both memory > and peripheral devices in the driver data. Thus the issue noticed for > the iDMA 32-bit controllers will be eliminated and the ACPI-probed > DW DMA controllers will be configured with the correct master ID by > default. > > Cc: stable@vger.kernel.org > Fixes: b336268dde75 ("dmaengine: dw: Add peripheral bus width verification") > Fixes: 199244d69458 ("dmaengine: dw: add support of iDMA 32-bit hardware") > Reported-by: Ferry Toth <fntoth@gmail.com> > Closes: https://lore.kernel.org/dmaengine/ZuXbCKUs1iOqFu51@black.fi.intel.com/ > Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Closes: https://lore.kernel.org/dmaengine/ZuXgI-VcHpMgbZ91@black.fi.intel.com/ > Co-developed-by: Serge Semin <fancer.lancer@gmail.com> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > v3: rewrote to use driver_data > v2: https://lore.kernel.org/r/20240919185151.7331-1-fancer.lancer@gmail.com IMO v2 looked better for me. I am sure you know, but Master IDs is a platform-specific thing specific for each slave/peripheral device connected to the DMA controller. Depending on the chip design one peripheral device can be accessed over the one master IDs, another device/memory may have another master connected (can be up to four master IDs in general). That's why the master IDs have been declared in the dw_dma_slave structure. So adding them to struct dw_dma_chip_pdata doesn't seem like a good idea seeing it contains the generic DW DMA controller info. On the contrary my implementation seems a bit more coherent since it just changes the default slave IDs defined in the dw_dma_acpi_filter() method and initialized in the dw_dma_slave instance without adding slave-specific fields to the generic controller data. What seems like a much better alternative to the both approaches, is to use the dw_dma_slave instance defined in the mrfld_spi_setup() method for the Intel Merrifield SPI PXA2xx DMA-interface in drivers/spi/spi-pxa2xx-pci.c. But AFAICT that data is left unused since the DMA-engine handle and connection parameters are determined by the channel name. Right? Is it possible to make use of the filter-function specified to the dma_request_slave_channel_compat() method? -Serge(y) > > drivers/dma/dw/acpi.c | 6 ++++-- > drivers/dma/dw/internal.h | 8 ++++++++ > drivers/dma/dw/pci.c | 4 ++-- > 3 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma/dw/acpi.c b/drivers/dma/dw/acpi.c > index c510c109d2c3..b6452fffa657 100644 > --- a/drivers/dma/dw/acpi.c > +++ b/drivers/dma/dw/acpi.c > @@ -8,13 +8,15 @@ > > static bool dw_dma_acpi_filter(struct dma_chan *chan, void *param) > { > + struct dw_dma *dw = to_dw_dma(chan->device); > + struct dw_dma_chip_pdata *data = dev_get_drvdata(dw->dma.dev); > struct acpi_dma_spec *dma_spec = param; > struct dw_dma_slave slave = { > .dma_dev = dma_spec->dev, > .src_id = dma_spec->slave_id, > .dst_id = dma_spec->slave_id, > - .m_master = 0, > - .p_master = 1, > + .m_master = data->m_master, > + .p_master = data->p_master, > }; > > return dw_dma_filter(chan, &slave); > diff --git a/drivers/dma/dw/internal.h b/drivers/dma/dw/internal.h > index 779b3cbcf30d..99d9f61b2254 100644 > --- a/drivers/dma/dw/internal.h > +++ b/drivers/dma/dw/internal.h > @@ -51,11 +51,15 @@ struct dw_dma_chip_pdata { > int (*probe)(struct dw_dma_chip *chip); > int (*remove)(struct dw_dma_chip *chip); > struct dw_dma_chip *chip; > + u8 m_master; > + u8 p_master; > }; > > static __maybe_unused const struct dw_dma_chip_pdata dw_dma_chip_pdata = { > .probe = dw_dma_probe, > .remove = dw_dma_remove, > + .m_master = 0, > + .p_master = 1, > }; > > static const struct dw_dma_platform_data idma32_pdata = { > @@ -72,6 +76,8 @@ static __maybe_unused const struct dw_dma_chip_pdata idma32_chip_pdata = { > .pdata = &idma32_pdata, > .probe = idma32_dma_probe, > .remove = idma32_dma_remove, > + .m_master = 0, > + .p_master = 0, > }; > > static const struct dw_dma_platform_data xbar_pdata = { > @@ -88,6 +94,8 @@ static __maybe_unused const struct dw_dma_chip_pdata xbar_chip_pdata = { > .pdata = &xbar_pdata, > .probe = idma32_dma_probe, > .remove = idma32_dma_remove, > + .m_master = 0, > + .p_master = 0, > }; > > int dw_dma_fill_pdata(struct device *dev, struct dw_dma_platform_data *pdata); > diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c > index adf2d69834b8..a3aae3d1c093 100644 > --- a/drivers/dma/dw/pci.c > +++ b/drivers/dma/dw/pci.c > @@ -56,10 +56,10 @@ static int dw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *pid) > if (ret) > return ret; > > - dw_dma_acpi_controller_register(chip->dw); > - > pci_set_drvdata(pdev, data); > > + dw_dma_acpi_controller_register(chip->dw); > + > return 0; > } > > -- > 2.43.0.rc1.1336.g36b5255a03ac >
On Mon, Sep 23, 2024 at 01:01:08AM +0300, Serge Semin wrote: > On Fri, Sep 20, 2024 at 06:56:17PM +0300, Andy Shevchenko wrote: ... > > Fix the problem by specifying the default master ID for both memory > > and peripheral devices in the driver data. Thus the issue noticed for > > the iDMA 32-bit controllers will be eliminated and the ACPI-probed > > DW DMA controllers will be configured with the correct master ID by > > default. > > --- > > v3: rewrote to use driver_data > > v2: https://lore.kernel.org/r/20240919185151.7331-1-fancer.lancer@gmail.com > > IMO v2 looked better for me. I disagree, obviously, since I sent a v3. (I can drop your authorship and tags in v4) > I am sure you know, but Master IDs is a > platform-specific thing specific for each slave/peripheral device > connected to the DMA controller. Depending on the chip design one > peripheral device can be accessed over the one master IDs, another > device/memory may have another master connected (can be up to four > master IDs in general). That's why the master IDs have been declared > in the dw_dma_slave structure. Correct. > So adding them to struct > dw_dma_chip_pdata doesn't seem like a good idea seeing it contains the > generic DW DMA controller info. So far there is no evidence that the channels are integrated differently on the same DMA controller over all hardware that uses this IP. > On the contrary my implementation > seems a bit more coherent since it just changes the default slave IDs > defined in the dw_dma_acpi_filter() method and initialized in the > dw_dma_slave instance without adding slave-specific fields to the > generic controller data. The default enumeration for PCI + ACPI or pure ACPI devices is not changed with my patch, but actually makes it better (increases granularity). The defaults are platform specific and that's what driver_data is for. While you like your solution, the problem with it that it doesn't cover different orders, so it's half-baked solution, I think. Mine doesn't change the status quo about integration (see above) and has better approach regarding different ordering. Both implementations have a flaw regarding per-channel master configuration. > What seems like a much better alternative to the both approaches, is > to use the dw_dma_slave instance defined in the mrfld_spi_setup() > method for the Intel Merrifield SPI PXA2xx DMA-interface in > drivers/spi/spi-pxa2xx-pci.c. But AFAICT that data is left unused > since the DMA-engine handle and connection parameters are determined > by the channel name. Right? Is it possible to make use of the > filter-function specified to the dma_request_slave_channel_compat() > method? Unfortunately no, in ACPI case the only data we use is the name (index) of the channel in the respective resources. Everything else is done automatically.
On Mon, Sep 23, 2024 at 11:21:37AM +0300, Andy Shevchenko wrote: > On Mon, Sep 23, 2024 at 01:01:08AM +0300, Serge Semin wrote: > > On Fri, Sep 20, 2024 at 06:56:17PM +0300, Andy Shevchenko wrote: > > ... > > > > Fix the problem by specifying the default master ID for both memory > > > and peripheral devices in the driver data. Thus the issue noticed for > > > the iDMA 32-bit controllers will be eliminated and the ACPI-probed > > > DW DMA controllers will be configured with the correct master ID by > > > default. > > > > --- > > > v3: rewrote to use driver_data > > > v2: https://lore.kernel.org/r/20240919185151.7331-1-fancer.lancer@gmail.com > > > > IMO v2 looked better for me. > > I disagree, obviously, since I sent a v3. > (I can drop your authorship and tags in v4) No need in that. See my further comments. > > > I am sure you know, but Master IDs is a > > platform-specific thing specific for each slave/peripheral device > > connected to the DMA controller. Depending on the chip design one > > peripheral device can be accessed over the one master IDs, another > > device/memory may have another master connected (can be up to four > > master IDs in general). That's why the master IDs have been declared > > in the dw_dma_slave structure. > > Correct. > > > So adding them to struct > > dw_dma_chip_pdata doesn't seem like a good idea seeing it contains the > > generic DW DMA controller info. > > So far there is no evidence that the channels are integrated differently on > the same DMA controller over all hardware that uses this IP. I've got one device (which currently isn't supported by the vanilla kernel) which DW DMA-controller have a weird feature of being able to communicate with one of the peripherals via both available masters.) So the master IDs order can differ from what is currently set by default (for ACPI). Anyway regarding the amount of the master I/F-es, yes, I failed to find any platform with more than two masters synthesized in. But based on the DW DMAC IP-core databook there can be up to four of them (see the DMAH_NUM_MASTER_INT parameter). > > > On the contrary my implementation > > seems a bit more coherent since it just changes the default slave IDs > > defined in the dw_dma_acpi_filter() method and initialized in the > > dw_dma_slave instance without adding slave-specific fields to the > > generic controller data. > > The default enumeration for PCI + ACPI or pure ACPI devices is not > changed with my patch, but actually makes it better (increases granularity). > The defaults are platform specific and that's what driver_data is for. Since it's a default setting for the particular controller then it seems it would be better to signify that semantic in the fields name. Moreover seeing it's a per-platform setup I would recomment to move these fields to the dw_dma_platform_data structure instead. (Please see my last comment regarding this.) > > While you like your solution, the problem with it that it doesn't cover > different orders, so it's half-baked solution, I think. Mine doesn't > change the status quo about integration (see above) and has better approach > regarding different ordering. Well, your solution doesn't cover the different order of the master IDs either, because the IDs order is still fixed but on the per-controller basis. Yes, in that regard your approach is bit more comprehensive, but it still remains half-baked since, as you said yourself further, it doesn't cover the cases of the non-default master IDs combination. My solution doesn't change the status quo either, but merely fixes the case which is currently incorrectly handled in the dw_dma_acpi_filter() method. The rest remains the same. (See further before responding to this comment.) > Both implementations have a flaw regarding per-channel master configuration. Right, none of our approaches provide a complete solution of the problem with a per-peripheral device master IDs setup. Based on this and what was said in the previous comments chunk, I would normally prefer to choose a simpler, more localised, less invasive fix, which is provided in my version of the change. That's why I started the discussion in the first place. (Please see my last comment before answering to this one.) > > > What seems like a much better alternative to the both approaches, is > > to use the dw_dma_slave instance defined in the mrfld_spi_setup() > > method for the Intel Merrifield SPI PXA2xx DMA-interface in > > drivers/spi/spi-pxa2xx-pci.c. But AFAICT that data is left unused > > since the DMA-engine handle and connection parameters are determined > > by the channel name. Right? Is it possible to make use of the > > filter-function specified to the dma_request_slave_channel_compat() > > method? > > Unfortunately no, in ACPI case the only data we use is the name (index) of > the channel in the respective resources. Everything else is done automatically. Right, but AFAICS ACPI doesn't provide the complete settings in this case. I thought about some combined solution: retrieve the DMAC channel via the standard name/index-based approach and then pass the dw_dma_slave settings via the custom filter method specified by the client driver, thus making use of what is implemented in the Merrifield SPI PXA2xx driver. Alas implementing this approach would require to alter the generic DMA-engine core somehow.( In anyway if you prefer your version of the fix, fine by me. I've provided my reasoning above. If it wasn't enough to persuade you I won't be insisting on my change anymore especially seeing its your and Varesh duty to maintain the driver. But, again IMO, it seems to be better to add the default_{m,p}_master/d{p,m}_master/etc fields to the dw_dma_platform_data structure since the platform-specific controller settings have been consolidated in there. The dw_dma_chip_pdata structure looks as more like generic driver data storage. -Serge(y) > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Sep 23, 2024 at 02:57:27PM +0300, Serge Semin wrote: > On Mon, Sep 23, 2024 at 11:21:37AM +0300, Andy Shevchenko wrote: > > On Mon, Sep 23, 2024 at 01:01:08AM +0300, Serge Semin wrote: > > > On Fri, Sep 20, 2024 at 06:56:17PM +0300, Andy Shevchenko wrote: ... > > > So adding them to struct > > > dw_dma_chip_pdata doesn't seem like a good idea seeing it contains the > > > generic DW DMA controller info. > > > > So far there is no evidence that the channels are integrated differently on > > the same DMA controller over all hardware that uses this IP. > > I've got one device (which currently isn't supported by the vanilla > kernel) which DW DMA-controller have a weird feature of being able to > communicate with one of the peripherals via both available masters.) > So the master IDs order can differ from what is currently set by > default (for ACPI). > > Anyway regarding the amount of the master I/F-es, yes, I failed to > find any platform with more than two masters synthesized in. But based > on the DW DMAC IP-core databook there can be up to four of them (see > the DMAH_NUM_MASTER_INT parameter). Not sure about weirdness, but IIRC those Intel ones should support that configuration as well, as the peripheral and memory busses are the same at the end. > > > On the contrary my implementation > > > seems a bit more coherent since it just changes the default slave IDs > > > defined in the dw_dma_acpi_filter() method and initialized in the > > > dw_dma_slave instance without adding slave-specific fields to the > > > generic controller data. > > > The default enumeration for PCI + ACPI or pure ACPI devices is not > > changed with my patch, but actually makes it better (increases granularity). > > The defaults are platform specific and that's what driver_data is for. > > Since it's a default setting for the particular controller then it > seems it would be better to signify that semantic in the fields name. > Moreover seeing it's a per-platform setup I would recomment to move > these fields to the dw_dma_platform_data structure instead. > (Please see my last comment regarding this.) > > > While you like your solution, the problem with it that it doesn't cover > > different orders, so it's half-baked solution, I think. Mine doesn't > > change the status quo about integration (see above) and has better approach > > regarding different ordering. > > Well, your solution doesn't cover the different order of the master > IDs either, Correct, I explicitly mentioned that that neither of proposed solutions cover this right now. > because the IDs order is still fixed but on the per-controller > basis. Yes, in that regard your approach is bit more comprehensive, > but it still remains half-baked since, as you said yourself further, > it doesn't cover the cases of the non-default master IDs combination. Right. And I propose to follow this way as long as we have no other devices in the wild. I.o.w. let's solve the problem when it appears. > My solution doesn't change the status quo either, but merely fixes the > case which is currently incorrectly handled in the > dw_dma_acpi_filter() method. The rest remains the same. > (See further before responding to this comment.) > > > Both implementations have a flaw regarding per-channel master configuration. > > Right, none of our approaches provide a complete solution of the > problem with a per-peripheral device master IDs setup. Based on this and > what was said in the previous comments chunk, I would normally prefer to > choose a simpler, more localised, less invasive fix, which is provided > in my version of the change. That's why I started the discussion > in the first place. > (Please see my last comment before answering to this one.) > > > What seems like a much better alternative to the both approaches, is > > > to use the dw_dma_slave instance defined in the mrfld_spi_setup() > > > method for the Intel Merrifield SPI PXA2xx DMA-interface in > > > drivers/spi/spi-pxa2xx-pci.c. But AFAICT that data is left unused > > > since the DMA-engine handle and connection parameters are determined > > > by the channel name. Right? Is it possible to make use of the > > > filter-function specified to the dma_request_slave_channel_compat() > > > method? > > > Unfortunately no, in ACPI case the only data we use is the name (index) of > > the channel in the respective resources. Everything else is done automatically. > > Right, but AFAICS ACPI doesn't provide the complete settings in this > case. I would even say in many sophisticated DMA controller cases... :-( > I thought about some combined solution: retrieve the DMAC > channel via the standard name/index-based approach and then pass the > dw_dma_slave settings via the custom filter method specified by the > client driver, thus making use of what is implemented in the > Merrifield SPI PXA2xx driver. Alas implementing this approach would > require to alter the generic DMA-engine core somehow.( This sounds way too far from KISS. > In anyway if you prefer your version of the fix, fine by me. I've > provided my reasoning above. If it wasn't enough to persuade you I > won't be insisting on my change anymore especially seeing its your and > Varesh duty to maintain the driver. Yes, I still prefer mine. > But, again IMO, it seems to be > better to add the default_{m,p}_master/d{p,m}_master/etc fields to the > dw_dma_platform_data structure since the platform-specific controller > settings have been consolidated in there. The dw_dma_chip_pdata > structure looks as more like generic driver data storage. I don't think that is correct place for it. The platform data is specific to the DMA controller as a whole and having there the master configuration will mean to have the arrays of them. This OTOH will break the OF setup where this comes from the slave descriptions and may not be provided with DMA controller, making it imbalanced. Yes, I may agree with you that chip data is not a good place either, but at least it isolates the case to PCI + ACPI / pure ACPI devices (and in particular we won't need to alter Intel Quark case). Ideally, we should parse the additional properties from ACPI for this kind of DMA controllers to get this from the _slave_ resources. Currently this is not done, but anyone may propose a such (would you like to volunteer?). ... TL;DR: If you are okay with your authorship in v3, I prefer it over other versions with the explanations given in this email thread.
On Mon, Sep 23, 2024 at 03:26:24PM +0300, Andy Shevchenko wrote: > On Mon, Sep 23, 2024 at 02:57:27PM +0300, Serge Semin wrote: > > On Mon, Sep 23, 2024 at 11:21:37AM +0300, Andy Shevchenko wrote: > > > On Mon, Sep 23, 2024 at 01:01:08AM +0300, Serge Semin wrote: > > > > On Fri, Sep 20, 2024 at 06:56:17PM +0300, Andy Shevchenko wrote: > > ... > > > Yes, I still prefer mine. > > > But, again IMO, it seems to be > > better to add the default_{m,p}_master/d{p,m}_master/etc fields to the > > dw_dma_platform_data structure since the platform-specific controller > > settings have been consolidated in there. The dw_dma_chip_pdata > > structure looks as more like generic driver data storage. > > I don't think that is correct place for it. The platform data is specific > to the DMA controller as a whole and having there the master configuration > will mean to have the arrays of them. This OTOH will break the OF setup > where this comes from the slave descriptions and may not be provided with > DMA controller, making it imbalanced. Yes, I may agree with you that chip data > is not a good place either, but at least it isolates the case to PCI + ACPI / > pure ACPI devices (and in particular we won't need to alter Intel Quark case). > > Ideally, we should parse the additional properties from ACPI for this kind > of DMA controllers to get this from the _slave_ resources. Currently this is > not done, but anyone may propose a such I guess it would also mean to fix all the firmware as well, wouldn't it? Do the Intel/AMD/etc ACPI firmware currently provide such a data? In anyway it would be inapplicable for the legacy hardware anyway. > (would you like to volunteer?). not really.) Maybe in some long-distance future when I get to meet a device on the ACPI-based platform with the DW DMAC + some peripheral experiencing the denoted problem, I'll think about implementing what we've discussed here. > > ... > > TL;DR: If you are okay with your authorship in v3, I prefer it over other > versions with the explanations given in this email thread. Ok. Let's leave it as of your preference. -Serge(y) > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Sep 23, 2024 at 03:46:04PM +0300, Serge Semin wrote: > On Mon, Sep 23, 2024 at 03:26:24PM +0300, Andy Shevchenko wrote: > > On Mon, Sep 23, 2024 at 02:57:27PM +0300, Serge Semin wrote: > > > On Mon, Sep 23, 2024 at 11:21:37AM +0300, Andy Shevchenko wrote: > > > > On Mon, Sep 23, 2024 at 01:01:08AM +0300, Serge Semin wrote: > > > > > On Fri, Sep 20, 2024 at 06:56:17PM +0300, Andy Shevchenko wrote: ... > > Yes, I still prefer mine. > > > > > But, again IMO, it seems to be > > > better to add the default_{m,p}_master/d{p,m}_master/etc fields to the > > > dw_dma_platform_data structure since the platform-specific controller > > > settings have been consolidated in there. The dw_dma_chip_pdata > > > structure looks as more like generic driver data storage. > > > > I don't think that is correct place for it. The platform data is specific > > to the DMA controller as a whole and having there the master configuration > > will mean to have the arrays of them. This OTOH will break the OF setup > > where this comes from the slave descriptions and may not be provided with > > DMA controller, making it imbalanced. Yes, I may agree with you that chip data > > is not a good place either, but at least it isolates the case to PCI + ACPI / > > pure ACPI devices (and in particular we won't need to alter Intel Quark case). > > > Ideally, we should parse the additional properties from ACPI for this kind > > of DMA controllers to get this from the _slave_ resources. Currently this is > > not done, but anyone may propose a such > > I guess it would also mean to fix all the firmware as well, wouldn't it? Nope, legacy will use current defaults. Only new will be more flexible. > Do the Intel/AMD/etc ACPI firmware currently provide such a data? I can't tell for AMD for sure, neither for Intel as a whole (not a product related engineer). I can tell only for my experience and I haven't known any of Intel devices with such IP that has it different. > In anyway it would be inapplicable for the legacy hardware anyway. Exactly! > > (would you like to volunteer?). > > not really.) Maybe in some long-distance future when I get to meet a > device on the ACPI-based platform with the DW DMAC + some peripheral > experiencing the denoted problem, I'll think about implementing what > we've discussed here. Something is telling me that this will never be needed IRL. ... > > TL;DR: If you are okay with your authorship in v3, I prefer it over other > > versions with the explanations given in this email thread. > > Ok. Let's leave it as of your preference. Thanks!
Hi, Op 20-09-2024 om 17:56 schreef Andy Shevchenko: > From: Serge Semin <fancer.lancer@gmail.com> > > The recently submitted fix-commit revealed a problem in the iDMA 32-bit > platform code. Even though the controller supported only a single master > the dw_dma_acpi_filter() method hard-coded two master interfaces with IDs > 0 and 1. As a result the sanity check implemented in the commit > b336268dde75 ("dmaengine: dw: Add peripheral bus width verification") > got incorrect interface data width and thus prevented the client drivers > from configuring the DMA-channel with the EINVAL error returned. E.g., > the next error was printed for the PXA2xx SPI controller driver trying > to configure the requested channels: > >> [ 164.525604] pxa2xx_spi_pci 0000:00:07.1: DMA slave config failed >> [ 164.536105] pxa2xx_spi_pci 0000:00:07.1: failed to get DMA TX descriptor >> [ 164.543213] spidev spi-SPT0001:00: SPI transfer failed: -16 > > The problem would have been spotted much earlier if the iDMA 32-bit > controller supported more than one master interfaces. But since it > supports just a single master and the iDMA 32-bit specific code just > ignores the master IDs in the CTLLO preparation method, the issue has > been gone unnoticed so far. > > Fix the problem by specifying the default master ID for both memory > and peripheral devices in the driver data. Thus the issue noticed for > the iDMA 32-bit controllers will be eliminated and the ACPI-probed > DW DMA controllers will be configured with the correct master ID by > default. > > Cc: stable@vger.kernel.org > Fixes: b336268dde75 ("dmaengine: dw: Add peripheral bus width verification") > Fixes: 199244d69458 ("dmaengine: dw: add support of iDMA 32-bit hardware") > Reported-by: Ferry Toth <fntoth@gmail.com> > Closes: https://lore.kernel.org/dmaengine/ZuXbCKUs1iOqFu51@black.fi.intel.com/ > Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Closes: https://lore.kernel.org/dmaengine/ZuXgI-VcHpMgbZ91@black.fi.intel.com/ > Co-developed-by: Serge Semin <fancer.lancer@gmail.com> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > v3: rewrote to use driver_data > v2: https://lore.kernel.org/r/20240919185151.7331-1-fancer.lancer@gmail.com > > drivers/dma/dw/acpi.c | 6 ++++-- > drivers/dma/dw/internal.h | 8 ++++++++ > drivers/dma/dw/pci.c | 4 ++-- > 3 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma/dw/acpi.c b/drivers/dma/dw/acpi.c > index c510c109d2c3..b6452fffa657 100644 > --- a/drivers/dma/dw/acpi.c > +++ b/drivers/dma/dw/acpi.c > @@ -8,13 +8,15 @@ > > static bool dw_dma_acpi_filter(struct dma_chan *chan, void *param) > { > + struct dw_dma *dw = to_dw_dma(chan->device); > + struct dw_dma_chip_pdata *data = dev_get_drvdata(dw->dma.dev); > struct acpi_dma_spec *dma_spec = param; > struct dw_dma_slave slave = { > .dma_dev = dma_spec->dev, > .src_id = dma_spec->slave_id, > .dst_id = dma_spec->slave_id, > - .m_master = 0, > - .p_master = 1, > + .m_master = data->m_master, > + .p_master = data->p_master, > }; > > return dw_dma_filter(chan, &slave); > diff --git a/drivers/dma/dw/internal.h b/drivers/dma/dw/internal.h > index 779b3cbcf30d..99d9f61b2254 100644 > --- a/drivers/dma/dw/internal.h > +++ b/drivers/dma/dw/internal.h > @@ -51,11 +51,15 @@ struct dw_dma_chip_pdata { > int (*probe)(struct dw_dma_chip *chip); > int (*remove)(struct dw_dma_chip *chip); > struct dw_dma_chip *chip; > + u8 m_master; > + u8 p_master; > }; > > static __maybe_unused const struct dw_dma_chip_pdata dw_dma_chip_pdata = { > .probe = dw_dma_probe, > .remove = dw_dma_remove, > + .m_master = 0, > + .p_master = 1, > }; > > static const struct dw_dma_platform_data idma32_pdata = { > @@ -72,6 +76,8 @@ static __maybe_unused const struct dw_dma_chip_pdata idma32_chip_pdata = { > .pdata = &idma32_pdata, > .probe = idma32_dma_probe, > .remove = idma32_dma_remove, > + .m_master = 0, > + .p_master = 0, > }; > > static const struct dw_dma_platform_data xbar_pdata = { > @@ -88,6 +94,8 @@ static __maybe_unused const struct dw_dma_chip_pdata xbar_chip_pdata = { > .pdata = &xbar_pdata, > .probe = idma32_dma_probe, > .remove = idma32_dma_remove, > + .m_master = 0, > + .p_master = 0, > }; > > int dw_dma_fill_pdata(struct device *dev, struct dw_dma_platform_data *pdata); > diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c > index adf2d69834b8..a3aae3d1c093 100644 > --- a/drivers/dma/dw/pci.c > +++ b/drivers/dma/dw/pci.c > @@ -56,10 +56,10 @@ static int dw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *pid) > if (ret) > return ret; > > - dw_dma_acpi_controller_register(chip->dw); > - > pci_set_drvdata(pdev, data); > > + dw_dma_acpi_controller_register(chip->dw); > + > return 0; > } > Tested-by: Ferry Toth <fntoth@gmail.com> (Intel Edison-Arduino)
On Tue, Sep 24, 2024 at 09:07:19PM +0200, Ferry Toth wrote: > Op 20-09-2024 om 17:56 schreef Andy Shevchenko: > > From: Serge Semin <fancer.lancer@gmail.com> > > > > The recently submitted fix-commit revealed a problem in the iDMA 32-bit > > platform code. Even though the controller supported only a single master > > the dw_dma_acpi_filter() method hard-coded two master interfaces with IDs > > 0 and 1. As a result the sanity check implemented in the commit > > b336268dde75 ("dmaengine: dw: Add peripheral bus width verification") > > got incorrect interface data width and thus prevented the client drivers > > from configuring the DMA-channel with the EINVAL error returned. E.g., > > the next error was printed for the PXA2xx SPI controller driver trying > > to configure the requested channels: > > > > > [ 164.525604] pxa2xx_spi_pci 0000:00:07.1: DMA slave config failed > > > [ 164.536105] pxa2xx_spi_pci 0000:00:07.1: failed to get DMA TX descriptor > > > [ 164.543213] spidev spi-SPT0001:00: SPI transfer failed: -16 > > > > The problem would have been spotted much earlier if the iDMA 32-bit > > controller supported more than one master interfaces. But since it > > supports just a single master and the iDMA 32-bit specific code just > > ignores the master IDs in the CTLLO preparation method, the issue has > > been gone unnoticed so far. > > > > Fix the problem by specifying the default master ID for both memory > > and peripheral devices in the driver data. Thus the issue noticed for > > the iDMA 32-bit controllers will be eliminated and the ACPI-probed > > DW DMA controllers will be configured with the correct master ID by > > default. ... > Tested-by: Ferry Toth <fntoth@gmail.com> (Intel Edison-Arduino) Thanks for testing! Vinod, can this be queued for v6.12-rc3?
diff --git a/drivers/dma/dw/acpi.c b/drivers/dma/dw/acpi.c index c510c109d2c3..b6452fffa657 100644 --- a/drivers/dma/dw/acpi.c +++ b/drivers/dma/dw/acpi.c @@ -8,13 +8,15 @@ static bool dw_dma_acpi_filter(struct dma_chan *chan, void *param) { + struct dw_dma *dw = to_dw_dma(chan->device); + struct dw_dma_chip_pdata *data = dev_get_drvdata(dw->dma.dev); struct acpi_dma_spec *dma_spec = param; struct dw_dma_slave slave = { .dma_dev = dma_spec->dev, .src_id = dma_spec->slave_id, .dst_id = dma_spec->slave_id, - .m_master = 0, - .p_master = 1, + .m_master = data->m_master, + .p_master = data->p_master, }; return dw_dma_filter(chan, &slave); diff --git a/drivers/dma/dw/internal.h b/drivers/dma/dw/internal.h index 779b3cbcf30d..99d9f61b2254 100644 --- a/drivers/dma/dw/internal.h +++ b/drivers/dma/dw/internal.h @@ -51,11 +51,15 @@ struct dw_dma_chip_pdata { int (*probe)(struct dw_dma_chip *chip); int (*remove)(struct dw_dma_chip *chip); struct dw_dma_chip *chip; + u8 m_master; + u8 p_master; }; static __maybe_unused const struct dw_dma_chip_pdata dw_dma_chip_pdata = { .probe = dw_dma_probe, .remove = dw_dma_remove, + .m_master = 0, + .p_master = 1, }; static const struct dw_dma_platform_data idma32_pdata = { @@ -72,6 +76,8 @@ static __maybe_unused const struct dw_dma_chip_pdata idma32_chip_pdata = { .pdata = &idma32_pdata, .probe = idma32_dma_probe, .remove = idma32_dma_remove, + .m_master = 0, + .p_master = 0, }; static const struct dw_dma_platform_data xbar_pdata = { @@ -88,6 +94,8 @@ static __maybe_unused const struct dw_dma_chip_pdata xbar_chip_pdata = { .pdata = &xbar_pdata, .probe = idma32_dma_probe, .remove = idma32_dma_remove, + .m_master = 0, + .p_master = 0, }; int dw_dma_fill_pdata(struct device *dev, struct dw_dma_platform_data *pdata); diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c index adf2d69834b8..a3aae3d1c093 100644 --- a/drivers/dma/dw/pci.c +++ b/drivers/dma/dw/pci.c @@ -56,10 +56,10 @@ static int dw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *pid) if (ret) return ret; - dw_dma_acpi_controller_register(chip->dw); - pci_set_drvdata(pdev, data); + dw_dma_acpi_controller_register(chip->dw); + return 0; }