linux-kernel - Re: [PATCH v2] i2c: stm32f7: Add atomic

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: <20230726212132.vylu6qulpyu5fndh@intel.intel>
Date:   Wed, 26 Jul 2023 23:21:32 +0200
From:   Andi Shyti <andi.shyti@...nel.org>
To:     Sean Nyekjaer <sean@...nix.com>
Cc:     Pierre-Yves MORDRET <pierre-yves.mordret@...s.st.com>,
        Alain Volmat <alain.volmat@...s.st.com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        linux-i2c@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] i2c: stm32f7: Add atomic_xfer method to driver

Hi Sean,

[...]

> @@ -905,38 +906,43 @@ static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
>  		cr2 |= STM32F7_I2C_CR2_NBYTES(f7_msg->count);
>  	}
>  
> -	/* Enable NACK, STOP, error and transfer complete interrupts */
> -	cr1 |= STM32F7_I2C_CR1_ERRIE | STM32F7_I2C_CR1_TCIE |
> -		STM32F7_I2C_CR1_STOPIE | STM32F7_I2C_CR1_NACKIE;
> -
> -	/* Clear DMA req and TX/RX interrupt */
> -	cr1 &= ~(STM32F7_I2C_CR1_RXIE | STM32F7_I2C_CR1_TXIE |
> -			STM32F7_I2C_CR1_RXDMAEN | STM32F7_I2C_CR1_TXDMAEN);
> -
> -	/* Configure DMA or enable RX/TX interrupt */
> -	i2c_dev->use_dma = false;
> -	if (i2c_dev->dma && f7_msg->count >= STM32F7_I2C_DMA_LEN_MIN) {
> -		ret = stm32_i2c_prep_dma_xfer(i2c_dev->dev, i2c_dev->dma,
> -					      msg->flags & I2C_M_RD,
> -					      f7_msg->count, f7_msg->buf,
> -					      stm32f7_i2c_dma_callback,
> -					      i2c_dev);
> -		if (!ret)
> -			i2c_dev->use_dma = true;
> -		else
> -			dev_warn(i2c_dev->dev, "can't use DMA\n");
> -	}
> +	if (!i2c_dev->atomic) {
> +		/* Enable NACK, STOP, error and transfer complete interrupts */
> +		cr1 |= STM32F7_I2C_CR1_ERRIE | STM32F7_I2C_CR1_TCIE |
> +			STM32F7_I2C_CR1_STOPIE | STM32F7_I2C_CR1_NACKIE;
> +
> +		/* Clear DMA req and TX/RX interrupt */
> +		cr1 &= ~(STM32F7_I2C_CR1_RXIE | STM32F7_I2C_CR1_TXIE |
> +				STM32F7_I2C_CR1_RXDMAEN | STM32F7_I2C_CR1_TXDMAEN);
> +
> +		/* Configure DMA or enable RX/TX interrupt */
> +		i2c_dev->use_dma = false;
> +		if (i2c_dev->dma && f7_msg->count >= STM32F7_I2C_DMA_LEN_MIN) {
> +			ret = stm32_i2c_prep_dma_xfer(i2c_dev->dev, i2c_dev->dma,
> +					msg->flags & I2C_M_RD,
> +					f7_msg->count, f7_msg->buf,
> +					stm32f7_i2c_dma_callback,
> +					i2c_dev);
> +			if (!ret)
> +				i2c_dev->use_dma = true;
> +			else
> +				dev_warn(i2c_dev->dev, "can't use DMA\n");
> +		}
>  
> -	if (!i2c_dev->use_dma) {
> -		if (msg->flags & I2C_M_RD)
> -			cr1 |= STM32F7_I2C_CR1_RXIE;
> -		else
> -			cr1 |= STM32F7_I2C_CR1_TXIE;
> +		if (!i2c_dev->use_dma) {
> +			if (msg->flags & I2C_M_RD)
> +				cr1 |= STM32F7_I2C_CR1_RXIE;
> +			else
> +				cr1 |= STM32F7_I2C_CR1_TXIE;
> +		} else {
> +			if (msg->flags & I2C_M_RD)
> +				cr1 |= STM32F7_I2C_CR1_RXDMAEN;
> +			else
> +				cr1 |= STM32F7_I2C_CR1_TXDMAEN;
> +		}
>  	} else {
> -		if (msg->flags & I2C_M_RD)
> -			cr1 |= STM32F7_I2C_CR1_RXDMAEN;
> -		else
> -			cr1 |= STM32F7_I2C_CR1_TXDMAEN;
> +		/* Disable all interrupts */
> +		cr1 &= ~STM32F7_I2C_ALL_IRQ_MASK;

if you do

	if (i2c_dev->atomic) {
		/* Disable all interrupts */
		cr1 &= ~STM32F7_I2C_ALL_IRQ_MASK;
		return;
	}

you save all the above from a leveel of indentation.

>  	}
>  
>  	/* Configure Start/Repeated Start */
> @@ -1670,7 +1676,22 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
> +static int stm32f7_i2c_wait_polling(struct stm32f7_i2c_dev *i2c_dev)
> +{
> +	ktime_t timeout = ktime_add_ms(ktime_get(), i2c_dev->adap.timeout);
> +
> +	while (ktime_compare(ktime_get(), timeout) < 0) {
> +		udelay(5);
> +		stm32f7_i2c_isr_event(0, i2c_dev);
> +
> +		if (try_wait_for_completion(&i2c_dev->complete))
> +			return 1;

I'm wondering if it makes really sense to have a complete() and
wait_for_completion() scheme here.

> +	}
> +
> +	return 0;
> +}
> +
> +static int stm32f7_i2c_xfer_core(struct i2c_adapter *i2c_adap,
>  			    struct i2c_msg msgs[], int num)
>  {
>  	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
> @@ -1694,8 +1715,13 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
>  
>  	stm32f7_i2c_xfer_msg(i2c_dev, msgs);
>  
> -	time_left = wait_for_completion_timeout(&i2c_dev->complete,
> -						i2c_dev->adap.timeout);
> +	if (!i2c_dev->atomic) {
> +		time_left = wait_for_completion_timeout(&i2c_dev->complete,
> +							i2c_dev->adap.timeout);
> +	} else {
> +		time_left = stm32f7_i2c_wait_polling(i2c_dev);
> +	}

please, drop the brackets here... and time_left here serves only
not to get the -ETIMEDOUT... looks a bit ugly to me, but can't
think of a better way.

> +
>  	ret = f7_msg->result;
>  	if (ret) {
>  		if (i2c_dev->use_dma)
> @@ -1727,6 +1753,24 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
>  	return (ret < 0) ? ret : num;
>  }
>  
> +static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
> +			    struct i2c_msg msgs[], int num)
> +{
> +	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
> +
> +	i2c_dev->atomic = 0;

false

> +	return stm32f7_i2c_xfer_core(i2c_adap, msgs, num);
> +}
> +
> +static int stm32f7_i2c_xfer_atomic(struct i2c_adapter *i2c_adap,
> +			    struct i2c_msg msgs[], int num)
> +{
> +	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
> +
> +	i2c_dev->atomic = 1;

true

Andi

> +	return stm32f7_i2c_xfer_core(i2c_adap, msgs, num);
> +}
> +