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