Re: [PATCH RESEND v4 1/6] dmaengine: dw: Add peripheral bus width verification

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Sep 16, 2024 at 02:43:48PM +0300, Andy Shevchenko wrote:
> On Sat, Sep 14, 2024 at 10:22:22PM +0300, Serge Semin wrote:
> > 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?
> 
> Please, provide one we can test!
> 
> > Could you please provide more details about what exactly happening?
> 
> Sure. AFAICT the only problematic line is this:
> 
>         else if (!is_power_of_2(reg_width) || reg_width > max_width)
> 
> in your patch, and it may trigger, for example, when max_width == 0.
> This, in accordance with my brief investigation, happens due to the following.
> 
> The DMA slave configuration is being copied twice in DW DMA code:
> 1) when respective filter function triggers (see acpi/of glue code);
> 2) when the channel is about to be allocated.
> 
> The iDMA32 has only a single master, and hence m_master == p_master,
> BUT the filter function in the acpi code is universal and it copies
> the wrong (from the iDMA32 perspective) value to p_master.
> As the result, when you retrieve the max_width, it takes the value from
> p_master, which is defined to 1 (sic!), and hence assigns it to 0.
> 
> I don't know how to quickfix this as the proper fix seems to provide
> the correct data in the first place.
> 
> Any ideas, patches we may test?

P.S. for your advocacy it seems that your change actually revealed an
inconsistency in the existing code. But still, it made a regression.

-- 
With Best Regards,
Andy Shevchenko