Received-SPF: pass (google.com: domain of linux-kernel+bounces-147343-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 16 Apr 2024 21:00:50 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Serge Semin <fancer.lancer@gmail.com>
Cc: Viresh Kumar <vireshk@kernel.org>, Vinod Koul <vkoul@kernel.org>,
	Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>, dmaengine@vger.kernel.org,
	linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/4] dmaengine: dw: Add peripheral bus width verification
Message-ID: <Zh680h4h6hURIb82@smile.fi.intel.com>
References: <20240416162908.24180-1-fancer.lancer@gmail.com>
 <20240416162908.24180-2-fancer.lancer@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240416162908.24180-2-fancer.lancer@gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

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?

-- 
With Best Regards,
Andy Shevchenko