Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Thu, 5 May 2022 07:51:42 -0700
From:   Nathan Chancellor <nathan@kernel.org>
To:     Michael Walle <michael@walle.cc>
Cc:     Codrin Ciubotariu <codrin.ciubotariu@microchip.com>,
        Nicolas Ferre <nicolas.ferre@microchip.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Claudiu Beznea <claudiu.beznea@microchip.com>,
        linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, llvm@lists.linux.dev
Subject: Re: [PATCH v2] i2c: at91: use dma safe buffers
Message-ID: <YnPkfrI4Udl9lMR8@dev-arch.thelio-3990X>
References: <20220407150828.202513-1-michael@walle.cc>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220407150828.202513-1-michael@walle.cc>
Precedence: bulk

On Thu, Apr 07, 2022 at 05:08:28PM +0200, Michael Walle wrote:
> The supplied buffer might be on the stack and we get the following error
> message:
> [    3.312058] at91_i2c e0070600.i2c: rejecting DMA map of vmalloc memory
> 
> Use i2c_{get,put}_dma_safe_msg_buf() to get a DMA-able memory region if
> necessary.
> 
> Fixes: 60937b2cdbf9 ("i2c: at91: add dma support")
> Signed-off-by: Michael Walle <michael@walle.cc>
> Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
> ---
> changes since v1:
>  - remove extra empty line
>  - add fixes tag as suggested by Codrin
> 
>  drivers/i2c/busses/i2c-at91-master.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
> index b0eae94909f4..5eca3b3bb609 100644
> --- a/drivers/i2c/busses/i2c-at91-master.c
> +++ b/drivers/i2c/busses/i2c-at91-master.c
> @@ -656,6 +656,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
>  	unsigned int_addr_flag = 0;
>  	struct i2c_msg *m_start = msg;
>  	bool is_read;
> +	u8 *dma_buf;
>  
>  	dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
>  
> @@ -703,7 +704,17 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
>  	dev->msg = m_start;
>  	dev->recv_len_abort = false;
>  
> +	if (dev->use_dma) {
> +		dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1);
> +		if (!dma_buf) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +		dev->buf = dma_buf;
> +	}
> +
>  	ret = at91_do_twi_transfer(dev);
> +	i2c_put_dma_safe_msg_buf(dma_buf, m_start, !ret);
>  
>  	ret = (ret < 0) ? ret : num;
>  out:
> -- 
> 2.30.2
> 
> 

This change as commit 03fbb903c8bf ("i2c: at91: use dma safe buffers")
causes the following clang warning:

drivers/i2c/busses/i2c-at91-master.c:707:6: error: variable 'dma_buf' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
        if (dev->use_dma) {
            ^~~~~~~~~~~~
drivers/i2c/busses/i2c-at91-master.c:717:27: note: uninitialized use occurs here
        i2c_put_dma_safe_msg_buf(dma_buf, m_start, !ret);
                                 ^~~~~~~
drivers/i2c/busses/i2c-at91-master.c:707:2: note: remove the 'if' if its condition is always true
        if (dev->use_dma) {
        ^~~~~~~~~~~~~~~~~~
drivers/i2c/busses/i2c-at91-master.c:659:13: note: initialize the variable 'dma_buf' to silence this warning
        u8 *dma_buf;
                   ^
                    = NULL
1 error generated.

Should this variable be initialized or should the call to
i2c_put_dma_safe_msg_buf() be moved into the if statement?

Cheers,
Nathan