Hi Andy On Fri, Sep 20, 2024 at 06:56:17PM +0300, Andy Shevchenko wrote: > From: Serge Semin <fancer.lancer@xxxxxxxxx> > > 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@xxxxxxxxxxxxxxx > 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@xxxxxxxxx> > Closes: https://lore.kernel.org/dmaengine/ZuXbCKUs1iOqFu51@xxxxxxxxxxxxxxxxxx/ > Reported-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> > Closes: https://lore.kernel.org/dmaengine/ZuXgI-VcHpMgbZ91@xxxxxxxxxxxxxxxxxx/ > Co-developed-by: Serge Semin <fancer.lancer@xxxxxxxxx> > Signed-off-by: Serge Semin <fancer.lancer@xxxxxxxxx> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> > --- > v3: rewrote to use driver_data > v2: https://lore.kernel.org/r/20240919185151.7331-1-fancer.lancer@xxxxxxxxx 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 >