linux-kernel - Re: [PATCH v2] i2c: tegra: Share same DMA channel for Rx and Tx

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: <Y/cr1EKI7R6rtg+O@orome>
Date:   Thu, 23 Feb 2023 10:03:16 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Akhil R <akhilrajeev@...dia.com>
Cc:     christian.koenig@....com, digetx@...il.com, jonathanh@...dia.com,
        ldewangan@...dia.com, linux-i2c@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
        sumit.semwal@...aro.org, wsa@...nel.org, robh+dt@...nel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v2] i2c: tegra: Share same DMA channel for Rx and Tx

On Wed, Feb 22, 2023 at 03:57:59PM +0530, Akhil R wrote:
> Allocate only one DMA channel for I2C and share it for both Tx and Rx.
> Since I2C supports only half duplex, there is no impact on perf with
> this.
> 
> Signed-off-by: Akhil R <akhilrajeev@...dia.com>
> ---
> v1->v2: Remove WARN_ON for DMA channel mismatch. There is only one
> channel in use with this change.
> 
>  drivers/i2c/busses/i2c-tegra.c | 54 ++++++++++------------------------
>  1 file changed, 15 insertions(+), 39 deletions(-)

I'm a little confused by this. All device trees already list the very
same reference for both TX and RX DMA channels in the I2C nodes, so
these channels are already effectively shared, aren't they?

So all this does is to get rid of the duplicated pointer? In practice,
is the DMA channel pointer going to point to the exact same memory or
are these separate objects that happen to point to the same hardware
resource?

In either case, I think the commit message should clarify that. Also, a
few minor nits below...

> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 6aab84c8d22b..f52b835f1700 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -248,8 +248,7 @@ struct tegra_i2c_hw_feature {
>   * @msg_read: indicates that the transfer is a read access
>   * @timings: i2c timings information like bus frequency
>   * @multimaster_mode: indicates that I2C controller is in multi-master mode
> - * @tx_dma_chan: DMA transmit channel
> - * @rx_dma_chan: DMA receive channel
> + * @dma_chan: DMA channel
>   * @dma_phys: handle to DMA resources
>   * @dma_buf: pointer to allocated DMA buffer
>   * @dma_buf_size: DMA buffer size
> @@ -281,8 +280,7 @@ struct tegra_i2c_dev {
>  	u8 *msg_buf;
>  
>  	struct completion dma_complete;
> -	struct dma_chan *tx_dma_chan;
> -	struct dma_chan *rx_dma_chan;
> +	struct dma_chan *dma_chan;
>  	unsigned int dma_buf_size;
>  	struct device *dma_dev;
>  	dma_addr_t dma_phys;
> @@ -398,7 +396,7 @@ static int tegra_i2c_dma_submit(struct tegra_i2c_dev *i2c_dev, size_t len)
>  	reinit_completion(&i2c_dev->dma_complete);
>  
>  	dir = i2c_dev->msg_read ? DMA_DEV_TO_MEM : DMA_MEM_TO_DEV;
> -	chan = i2c_dev->msg_read ? i2c_dev->rx_dma_chan : i2c_dev->tx_dma_chan;
> +	chan = i2c_dev->dma_chan;

Is there any point in keeping around the local variable? It doesn't
serve any purpose anymore.

>  
>  	dma_desc = dmaengine_prep_slave_single(chan, i2c_dev->dma_phys,
>  					       len, dir, DMA_PREP_INTERRUPT |
> @@ -426,14 +424,9 @@ static void tegra_i2c_release_dma(struct tegra_i2c_dev *i2c_dev)
>  		i2c_dev->dma_buf = NULL;
>  	}
>  
> -	if (i2c_dev->tx_dma_chan) {
> -		dma_release_channel(i2c_dev->tx_dma_chan);
> -		i2c_dev->tx_dma_chan = NULL;
> -	}
> -
> -	if (i2c_dev->rx_dma_chan) {
> -		dma_release_channel(i2c_dev->rx_dma_chan);
> -		i2c_dev->rx_dma_chan = NULL;
> +	if (i2c_dev->dma_chan) {
> +		dma_release_channel(i2c_dev->dma_chan);
> +		i2c_dev->dma_chan = NULL;
>  	}
>  }
>  
> @@ -457,25 +450,18 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
>  		return 0;
>  	}
>  
> -	chan = dma_request_chan(i2c_dev->dev, "rx");
> -	if (IS_ERR(chan)) {
> -		err = PTR_ERR(chan);
> -		goto err_out;
> -	}
> -
> -	i2c_dev->rx_dma_chan = chan;
> -
> +	/* The same channel will be used for both Rx and Tx.
> +	 * Keeping the name as tx for backward compatibility with
> +	 * existing devicetrees.
> +	 */

Block comments should have the starting /* on a line of its own. Also,
s/Rx/RX/ and s/Tx/TX/ for consistency. Looks like you also used "Rx" and
"Tx" in the commit message. The driver uses "RX" and "TX" elsewhere, so
better switch to that in both this comment and the commit message.

Also, perhaps quote the "tx" reference above to highlight that it's the
literal string in DT that you're referring to.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)