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: <07544E8E-6C08-40F0-984B-738A69DC686F@geanix.com>
Date:   Wed, 16 Aug 2023 08:59:02 +0200
From:   Sean Nyekjaer <sean@...nix.com>
To:     Andi Shyti <andi.shyti@...nel.org>
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 Andi,

Thanks for the review

> On 26 Jul 2023, at 23.21, Andi Shyti <andi.shyti@...nel.org> wrote:
> 
> 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.

Agree, it would be best not to indent this.
But the last step in the function is to write the cr1 value :) Goto doesn’t seem very pretty, but up to you...


> 
>> }
>> 
>> /* Configure Start/Repeated Start */
>> @@ -1670,7 +1676,22 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data)
>> return IRQ_HANDLED;
>> }
>> 

[ … ]

>> - 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.

Done.

> 
>> +
>> 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

/Sean