| Message ID | 20240802075100.6475-2-fancer.lancer@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b336268dde75cb09bd795cb24893d52152a9191f |
| Headers | show |
| Series | dmaengine: dw: Fix src/dst addr width misconfig | expand |
On Fri, Aug 02, 2024 at 10:50:46AM +0300, Serge Semin wrote: > Currently the src_addr_width and dst_addr_width fields of the > dma_slave_config structure are mapped to the CTLx.SRC_TR_WIDTH and > CTLx.DST_TR_WIDTH fields of the peripheral bus side in order to have the > properly aligned data passed to the target device. It's done just by > converting the passed peripheral bus width to the encoded value using the > __ffs() function. This implementation has several problematic sides: > > 1. __ffs() is undefined if no bit exist in the passed value. Thus if the > specified addr-width is DMA_SLAVE_BUSWIDTH_UNDEFINED, __ffs() may return > unexpected value depending on the platform-specific implementation. > > 2. DW AHB DMA-engine permits having the power-of-2 transfer width limited > by the DMAH_Mk_HDATA_WIDTH IP-core synthesize parameter. Specifying > bus-width out of that constraints scope will definitely cause unexpected > result since the destination reg will be only partly touched than the > client driver implied. > > Let's fix all of that by adding the peripheral bus width verification > method and calling it in dwc_config() which is supposed to be executed > before preparing any transfer. The new method will make sure that the > passed source or destination address width is valid and if undefined then > the driver will just fallback to the 1-byte width transfer. This patch broke Intel Merrifield iDMA32 + SPI PXA2xx configuration to me. Since it's first in the series and most likely the rest is dependent and we are almost at the release date I propose to roll back and start again after v6.12-rc1 will be out. Vinod, can we revert the entire series, please?
On Sat, Sep 14, 2024 at 10:12:35PM +0300, Andy Shevchenko wrote: > On Fri, Aug 02, 2024 at 10:50:46AM +0300, Serge Semin wrote: > > Currently the src_addr_width and dst_addr_width fields of the > > dma_slave_config structure are mapped to the CTLx.SRC_TR_WIDTH and > > CTLx.DST_TR_WIDTH fields of the peripheral bus side in order to have the > > properly aligned data passed to the target device. It's done just by > > converting the passed peripheral bus width to the encoded value using the > > __ffs() function. This implementation has several problematic sides: > > > > 1. __ffs() is undefined if no bit exist in the passed value. Thus if the > > specified addr-width is DMA_SLAVE_BUSWIDTH_UNDEFINED, __ffs() may return > > unexpected value depending on the platform-specific implementation. > > > > 2. DW AHB DMA-engine permits having the power-of-2 transfer width limited > > by the DMAH_Mk_HDATA_WIDTH IP-core synthesize parameter. Specifying > > bus-width out of that constraints scope will definitely cause unexpected > > result since the destination reg will be only partly touched than the > > client driver implied. > > > > Let's fix all of that by adding the peripheral bus width verification > > method and calling it in dwc_config() which is supposed to be executed > > before preparing any transfer. The new method will make sure that the > > passed source or destination address width is valid and if undefined then > > the driver will just fallback to the 1-byte width transfer. > > This patch broke Intel Merrifield iDMA32 + SPI PXA2xx configuration to > me. Since it's first in the series and most likely the rest is > dependent and we are almost at the release date I propose to roll back > and start again after v6.12-rc1 will be out. Vinod, can we revert the > entire series, please? I guess it's not the best option, since the patch has already been backported to the stable kernels anyway. Rolling back it from all of them seems tiresome. Let's at least try to fix the just discovered problem? Could you please provide more details about what exactly happening? -Serge(y) > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c index 4b3402156eae..d4c694b0f55a 100644 --- a/drivers/dma/dw/core.c +++ b/drivers/dma/dw/core.c @@ -16,6 +16,7 @@ #include <linux/init.h> #include <linux/interrupt.h> #include <linux/io.h> +#include <linux/log2.h> #include <linux/mm.h> #include <linux/module.h> #include <linux/slab.h> @@ -780,10 +781,43 @@ bool dw_dma_filter(struct dma_chan *chan, void *param) } EXPORT_SYMBOL_GPL(dw_dma_filter); +static int dwc_verify_p_buswidth(struct dma_chan *chan) +{ + struct dw_dma_chan *dwc = to_dw_dma_chan(chan); + struct dw_dma *dw = to_dw_dma(chan->device); + u32 reg_width, max_width; + + if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV) + reg_width = dwc->dma_sconfig.dst_addr_width; + else if (dwc->dma_sconfig.direction == DMA_DEV_TO_MEM) + reg_width = dwc->dma_sconfig.src_addr_width; + else /* DMA_MEM_TO_MEM */ + return 0; + + max_width = dw->pdata->data_width[dwc->dws.p_master]; + + /* Fall-back to 1-byte transfer width if undefined */ + if (reg_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) + reg_width = DMA_SLAVE_BUSWIDTH_1_BYTE; + else if (!is_power_of_2(reg_width) || reg_width > max_width) + return -EINVAL; + else /* bus width is valid */ + return 0; + + /* Update undefined addr width value */ + if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV) + dwc->dma_sconfig.dst_addr_width = reg_width; + else /* DMA_DEV_TO_MEM */ + dwc->dma_sconfig.src_addr_width = reg_width; + + return 0; +} + static int dwc_config(struct dma_chan *chan, struct dma_slave_config *sconfig) { struct dw_dma_chan *dwc = to_dw_dma_chan(chan); struct dw_dma *dw = to_dw_dma(chan->device); + int ret; memcpy(&dwc->dma_sconfig, sconfig, sizeof(*sconfig)); @@ -792,6 +826,10 @@ static int dwc_config(struct dma_chan *chan, struct dma_slave_config *sconfig) dwc->dma_sconfig.dst_maxburst = clamp(dwc->dma_sconfig.dst_maxburst, 0U, dwc->max_burst); + ret = dwc_verify_p_buswidth(chan); + if (ret) + return ret; + dw->encode_maxburst(dwc, &dwc->dma_sconfig.src_maxburst); dw->encode_maxburst(dwc, &dwc->dma_sconfig.dst_maxburst);
Currently the src_addr_width and dst_addr_width fields of the dma_slave_config structure are mapped to the CTLx.SRC_TR_WIDTH and CTLx.DST_TR_WIDTH fields of the peripheral bus side in order to have the properly aligned data passed to the target device. It's done just by converting the passed peripheral bus width to the encoded value using the __ffs() function. This implementation has several problematic sides: 1. __ffs() is undefined if no bit exist in the passed value. Thus if the specified addr-width is DMA_SLAVE_BUSWIDTH_UNDEFINED, __ffs() may return unexpected value depending on the platform-specific implementation. 2. DW AHB DMA-engine permits having the power-of-2 transfer width limited by the DMAH_Mk_HDATA_WIDTH IP-core synthesize parameter. Specifying bus-width out of that constraints scope will definitely cause unexpected result since the destination reg will be only partly touched than the client driver implied. Let's fix all of that by adding the peripheral bus width verification method and calling it in dwc_config() which is supposed to be executed before preparing any transfer. The new method will make sure that the passed source or destination address width is valid and if undefined then the driver will just fallback to the 1-byte width transfer. Fixes: 029a40e97d0d ("dmaengine: dw: provide DMA capabilities") Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- Changelog v2: - Add a note to the commit message about having the verification method called in the dwc_config() function. (Andy) - Add hyphen to "1byte" in the in-situ comment. (Andy) - Convert "err" to "ret" variable. (Andy) --- drivers/dma/dw/core.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)