linux-kernel - Re: [PATCH v7 01/34] i2c: tegra: Make tegra_i2c_flush

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]

Date:   Thu, 17 Sep 2020 13:10:57 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Jonathan Hunter <jonathanh@...dia.com>,
        Laxman Dewangan <ldewangan@...dia.com>,
        Wolfram Sang <wsa@...-dreams.de>,
        Michał Mirosław <mirq-linux@...e.qmqm.pl>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        linux-i2c@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 01/34] i2c: tegra: Make tegra_i2c_flush_fifos() usable
 in atomic transfer

On Wed, Sep 09, 2020 at 01:39:33AM +0300, Dmitry Osipenko wrote:
> The tegra_i2c_flush_fifos() shouldn't sleep in atomic transfer and jiffies
> are not updating if interrupts are disabled. Let's switch to use iopoll
> API helpers for register-polling. The iopoll API provides helpers for both
> atomic and non-atomic cases.
> 
> Note that this patch doesn't fix any known problem because normally FIFO
> is flushed at the time of starting a new transfer.
> 
> Reviewed-by: Michał Mirosław <mirq-linux@...e.qmqm.pl>
> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 00d3e4d7a01e..ab88cdd70376 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -470,9 +470,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
>  
>  static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>  {
> -	unsigned long timeout = jiffies + HZ;
> -	unsigned int offset;
> -	u32 mask, val;
> +	u32 mask, val, offset, reg_offset;

Is there are reason why we need reg_offset? Seems to me like we could
simplify this, see below.

> +	void __iomem *addr;
> +	int err;
>  
>  	if (i2c_dev->hw->has_mst_fifo) {
>  		mask = I2C_MST_FIFO_CONTROL_TX_FLUSH |
> @@ -488,12 +488,19 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>  	val |= mask;
>  	i2c_writel(i2c_dev, val, offset);
>  
> -	while (i2c_readl(i2c_dev, offset) & mask) {
> -		if (time_after(jiffies, timeout)) {
> -			dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n");
> -			return -ETIMEDOUT;
> -		}
> -		usleep_range(1000, 2000);
> +	reg_offset = tegra_i2c_reg_addr(i2c_dev, offset);
> +	addr = i2c_dev->base + reg_offset;

Why not just:

	offset = tegra_i2c_reg_offset(i2c_dev, offset);
	addr = i2c_dev->base + offset;

or even just:

	addr = i2c_dev->base + tegra_i2c_reg_offset(i2c_dev, offset);

? That makes the patch much smaller because you don't have to rewrite
the whole variable declaration block and just add the "addr" and "err"
variables while removing "timeout".

Thierry

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