| Message ID | 20240416162908.24180-2-fancer.lancer@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dmaengine: dw: Fix src/dst addr width misconfig | expand |
On Tue, Apr 16, 2024 at 07:28:55PM +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 which would 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. Please, add a word that you apply the check in the dwc_config() which is supposed to be called before preparing any transfer? ... > +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 */ Actually not only this direction, but TBH I do not see value in these comments. > + return 0; > + > + max_width = dw->pdata->data_width[dwc->dws.p_master]; > + > + /* Fall-back to 1byte transfer width if undefined */ 1-byte (as you even used in the commit message correctly) > + 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; So, can't you simply call clamp() for both fields in dwc_config()? > + return 0; > +} ... > + int err; Hmm... we have two functions one of which is using different name for this. Can we have a patch to convert to err the other one?
On Tue, Apr 16, 2024 at 09:00:50PM +0300, Andy Shevchenko wrote: > On Tue, Apr 16, 2024 at 07:28:55PM +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 which would 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. > > Please, add a word that you apply the check in the dwc_config() which is > supposed to be called before preparing any transfer? Ok. > > ... > > > +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 */ > > Actually not only this direction, DW DMAC driver supports only three directions: dw->dma.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV) | BIT(DMA_MEM_TO_MEM); so in this case the else-clause is intended to be taken for the DMA_MEM_TO_MEM transfers only. > but TBH I do not see value in these comments. Value is in signifying that DMA_MEM_TO_MEM is implied by the else clause. If in some future point we have DMA_DEV_TO_DEV support added to the driver, then the if-else statement must be aligned respectively. > > > + return 0; > > + > > + max_width = dw->pdata->data_width[dwc->dws.p_master]; > > + > > + /* Fall-back to 1byte transfer width if undefined */ > > 1-byte > (as you even used in the commit message correctly) Ok. > > > + 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; > > So, can't you simply call clamp() for both fields in dwc_config()? Alas I can't. Because the addr-width is the non-memory peripheral setting. We can't change it since the client drivers calculate it on the application-specific basis (CSR widths, transfer length, etc). So we must make sure that the specified value is supported. > > > + return 0; > > +} > > ... > > > + int err; > > Hmm... we have two functions one of which is using different name for this. Right, the driver uses both variants (see of.c, platform.c, pci.c too). > Can we have a patch to convert to err the other one? To be honest I'd prefer to use the "ret" name instead. It better describes the variable usage context (Although the statements like "if (err) ..." look a bit more readable). So I'd rather convert the "err" vars to "ret". What do you think? -Serge(y) > > -- > With Best Regards, > Andy Shevchenko > >
On Wed, Apr 17, 2024 at 10:54:42PM +0300, Serge Semin wrote: > On Tue, Apr 16, 2024 at 09:00:50PM +0300, Andy Shevchenko wrote: > > On Tue, Apr 16, 2024 at 07:28:55PM +0300, Serge Semin wrote: ... > > > + 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; > > > > > So, can't you simply call clamp() for both fields in dwc_config()? > > Alas I can't. Because the addr-width is the non-memory peripheral > setting. We can't change it since the client drivers calculate it on > the application-specific basis (CSR widths, transfer length, etc). So > we must make sure that the specified value is supported. What I meant is to convert this "update" part to the clamping, so we will have the check as the above like _verify_() { if (reg_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) return -E...; if (!is_power_of_2(reg_width) || reg_width > max_width) return -EINVAL; /* bus width is valid */ return 0; } dwc_config() { err = ... if (err = ...) clamp? else if (err) return err; } But it's up to you to choose the better variant. I just share the idea. > > > + return 0; > > > +} ... > > > + int err; > > > Hmm... we have two functions one of which is using different name for this. > > Right, the driver uses both variants (see of.c, platform.c, pci.c too). > > > Can we have a patch to convert to err the other one? > > To be honest I'd prefer to use the "ret" name instead. It better > describes the variable usage context (Although the statements like "if > (err) ..." look a bit more readable). So I'd rather convert the "err" > vars to "ret". What do you think? I'm fine with any choice, just my point is to get it consistent across the driver.
On Thu, Apr 18, 2024 at 12:43:25PM +0300, Andy Shevchenko wrote: > On Wed, Apr 17, 2024 at 10:54:42PM +0300, Serge Semin wrote: > > On Tue, Apr 16, 2024 at 09:00:50PM +0300, Andy Shevchenko wrote: > > > On Tue, Apr 16, 2024 at 07:28:55PM +0300, Serge Semin wrote: > > ... > > > > > + 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; > > > > > > > > So, can't you simply call clamp() for both fields in dwc_config()? > > > > Alas I can't. Because the addr-width is the non-memory peripheral > > setting. We can't change it since the client drivers calculate it on > > the application-specific basis (CSR widths, transfer length, etc). So > > we must make sure that the specified value is supported. > > What I meant is to convert this "update" part to the clamping, so > we will have the check as the above like > > _verify_() > { > if (reg_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > return -E...; > if (!is_power_of_2(reg_width) || reg_width > max_width) > return -EINVAL; > > /* bus width is valid */ > return 0; > } > > dwc_config() > { > err = ... > if (err = ...) > clamp? > else if (err) > return err; > } > > But it's up to you to choose the better variant. I just share the idea. Ok. Thanks for the suggestion. But I'll stick to my solution then. The specified *_addr_width values can't/shouldn't be clamped anyway and having a single verification function will comply to what will be implemented for the rest of the dwc_onfig() parts in this patchset. > > > > > + return 0; > > > > +} > > ... > > > > > + int err; > > > > > Hmm... we have two functions one of which is using different name for this. > > > > Right, the driver uses both variants (see of.c, platform.c, pci.c too). > > > > > Can we have a patch to convert to err the other one? > > > > To be honest I'd prefer to use the "ret" name instead. It better > > describes the variable usage context (Although the statements like "if > > (err) ..." look a bit more readable). So I'd rather convert the "err" > > vars to "ret". What do you think? > > I'm fine with any choice, just my point is to get it consistent across > the driver. Ok. "ret" it is then. -Serge(y) > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c index 5f7d690e3dba..c297ca9d5706 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 1byte 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 err; 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); + err = dwc_verify_p_buswidth(chan); + if (err) + return err; + 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 which would 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> --- drivers/dma/dw/core.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)