Re: [PATCH v3 1/1] dmaengine: dw: Select only supported masters for ACPI devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 




[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux PCI]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux