linux-kernel - Re: [PATCH 3/4] dmaengine: dw: Simplify prepare CTL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zh7LyszPd2sNfWRm@smile.fi.intel.com>
Date: Tue, 16 Apr 2024 22:04:42 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Serge Semin <fancer.lancer@...il.com>
Cc: Viresh Kumar <vireshk@...nel.org>, Vinod Koul <vkoul@...nel.org>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>, dmaengine@...r.kernel.org,
	linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] dmaengine: dw: Simplify prepare CTL_LO methods

On Tue, Apr 16, 2024 at 07:28:57PM +0300, Serge Semin wrote:
> Currently the CTL LO fields are calculated on the platform-specific basis.
> It's implemented by means of the prepare_ctllo() callbacks using the
> ternary operator within the local variables init block at the beginning of
> the block scope. The functions code currently is relatively hard to
> comprehend and isn't that optimal since implies four conditional
> statements executed and two additional local variables defined. Let's
> simplify the DW AHB DMA prepare_ctllo() method by unrolling the ternary
> operators into the normal if-else statement, dropping redundant
> master-interface ID variables and initializing the local variables based
> on the singly evaluated DMA-transfer direction check. Thus the method will
> look much more readable since now the fields content can be easily
> inferred right from the if-else branch. Provide the same update in the
> Intel DMA32 core driver for sake of the driver code unification.
> 
> Note besides of the effects described above this update is basically a
> preparation before dropping the max burst encoding callback. It will
> require calling the burst fields calculation methods right in the
> prepare_ctllo() callbacks, which would have made the later function code
> even more complex.

Yeah, this is inherited from the original driver where it used to be a macro.

..

> +	if (dwc->direction == DMA_MEM_TO_DEV) {
> +		sms = dwc->dws.m_master;
> +		smsize = 0;
> +		dms = dwc->dws.p_master;
> +		dmsize = sconfig->dst_maxburst;

I would group it differently, i.e.

		sms = dwc->dws.m_master;
		dms = dwc->dws.p_master;
		smsize = 0;
		dmsize = sconfig->dst_maxburst;

> +	} else if (dwc->direction == DMA_DEV_TO_MEM) {
> +		sms = dwc->dws.p_master;
> +		smsize = sconfig->src_maxburst;
> +		dms = dwc->dws.m_master;
> +		dmsize = 0;
> +	} else /* DMA_MEM_TO_MEM */ {
> +		sms = dwc->dws.m_master;
> +		smsize = 0;
> +		dms = dwc->dws.m_master;
> +		dmsize = 0;
> +	}

Ditto for two above cases.

>  static u32 idma32_prepare_ctllo(struct dw_dma_chan *dwc)
>  {
>  	struct dma_slave_config	*sconfig = &dwc->dma_sconfig;
> -	u8 smsize = (dwc->direction == DMA_DEV_TO_MEM) ? sconfig->src_maxburst : 0;
> -	u8 dmsize = (dwc->direction == DMA_MEM_TO_DEV) ? sconfig->dst_maxburst : 0;
> +	u8 smsize, dmsize;
> +
> +	if (dwc->direction == DMA_MEM_TO_DEV) {
> +		smsize = 0;
> +		dmsize = sconfig->dst_maxburst;
> +	} else if (dwc->direction == DMA_DEV_TO_MEM) {
> +		smsize = sconfig->src_maxburst;
> +		dmsize = 0;
> +	} else /* DMA_MEM_TO_MEM */ {
> +		smsize = 0;
> +		dmsize = 0;
> +	}

	u8 smsize = 0, dmsize = 0;

	if (dwc->direction == DMA_MEM_TO_DEV)
		dmsize = sconfig->dst_maxburst;
	else if (dwc->direction == DMA_DEV_TO_MEM)
		smsize = sconfig->src_maxburst;

?

Something similar also can be done in the Synopsys case above, no?

-- 
With Best Regards,
Andy Shevchenko