[PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[Sean Nyekjaer]

Add an atomic_xfer method to the driver so that it behaves correctly
when controlling a PMIC that is responsible for device shutdown.

The atomic_xfer method added is similar to the one from the i2c-mv64xxx
driver. When running an atomic_xfer a bool flag in the driver data is
set, the interrupt is not unmasked on transfer start, and the IRQ
handler is manually invoked while waiting for pending transfers to
complete.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Changes since v1:
 - Removed dma in atomic

Changes since v2:
 - Changed stm32f7_i2c_xfer_msg() so we have less of a diff
 - Changed try_wait_for_completion -> completion_done

 drivers/i2c/busses/i2c-stm32f7.c | 51 +++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index 579b30581725..16e2cb0b82bf 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
 	u32 dnf_dt;
 	u32 dnf;
 	struct stm32f7_i2c_alert *alert;
+	bool atomic;
 };
 
 /*
@@ -915,7 +916,8 @@ static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
 
 	/* Configure DMA or enable RX/TX interrupt */
 	i2c_dev->use_dma = false;
-	if (i2c_dev->dma && f7_msg->count >= STM32F7_I2C_DMA_LEN_MIN) {
+	if (i2c_dev->dma && f7_msg->count >= STM32F7_I2C_DMA_LEN_MIN
+	    && !i2c_dev->atomic) {
 		ret = stm32_i2c_prep_dma_xfer(i2c_dev->dev, i2c_dev->dma,
 					      msg->flags & I2C_M_RD,
 					      f7_msg->count, f7_msg->buf,
@@ -939,6 +941,9 @@ static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
 			cr1 |= STM32F7_I2C_CR1_TXDMAEN;
 	}
 
+	if (i2c_dev->atomic)
+		cr1 &= ~STM32F7_I2C_ALL_IRQ_MASK; /* Disable all interrupts */
+
 	/* Configure Start/Repeated Start */
 	cr2 |= STM32F7_I2C_CR2_START;
 
@@ -1670,7 +1675,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 (completion_done(&i2c_dev->complete))
+			return 1;
+	}
+
+	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 +1714,12 @@ 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);
+
 	ret = f7_msg->result;
 	if (ret) {
 		if (i2c_dev->use_dma)
@@ -1727,6 +1751,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 = 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 = true;
+	return stm32f7_i2c_xfer_core(i2c_adap, msgs, num);
+}
+
 static int stm32f7_i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 				  unsigned short flags, char read_write,
 				  u8 command, int size,
@@ -2095,6 +2137,7 @@ static u32 stm32f7_i2c_func(struct i2c_adapter *adap)
 
 static const struct i2c_algorithm stm32f7_i2c_algo = {
 	.master_xfer = stm32f7_i2c_xfer,
+	.master_xfer_atomic = stm32f7_i2c_xfer_atomic,
 	.smbus_xfer = stm32f7_i2c_smbus_xfer,
 	.functionality = stm32f7_i2c_func,
 	.reg_slave = stm32f7_i2c_reg_slave,
-- 
2.41.0

[PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[Sean Nyekjaer]

Add an atomic_xfer method to the driver so that it behaves correctly
when controlling a PMIC that is responsible for device shutdown.

The atomic_xfer method added is similar to the one from the i2c-mv64xxx
driver. When running an atomic_xfer a bool flag in the driver data is
set, the interrupt is not unmasked on transfer start, and the IRQ
handler is manually invoked while waiting for pending transfers to
complete.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Changes since v1:
 - Removed dma in atomic

Changes since v2:
 - Changed stm32f7_i2c_xfer_msg() so we have less of a diff
 - Changed try_wait_for_completion -> completion_done

 drivers/i2c/busses/i2c-stm32f7.c | 51 +++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index 579b30581725..16e2cb0b82bf 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
 	u32 dnf_dt;
 	u32 dnf;
 	struct stm32f7_i2c_alert *alert;
+	bool atomic;
 };
 
 /*
@@ -915,7 +916,8 @@ static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
 
 	/* Configure DMA or enable RX/TX interrupt */
 	i2c_dev->use_dma = false;
-	if (i2c_dev->dma && f7_msg->count >= STM32F7_I2C_DMA_LEN_MIN) {
+	if (i2c_dev->dma && f7_msg->count >= STM32F7_I2C_DMA_LEN_MIN
+	    && !i2c_dev->atomic) {
 		ret = stm32_i2c_prep_dma_xfer(i2c_dev->dev, i2c_dev->dma,
 					      msg->flags & I2C_M_RD,
 					      f7_msg->count, f7_msg->buf,
@@ -939,6 +941,9 @@ static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
 			cr1 |= STM32F7_I2C_CR1_TXDMAEN;
 	}
 
+	if (i2c_dev->atomic)
+		cr1 &= ~STM32F7_I2C_ALL_IRQ_MASK; /* Disable all interrupts */
+
 	/* Configure Start/Repeated Start */
 	cr2 |= STM32F7_I2C_CR2_START;
 
@@ -1670,7 +1675,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 (completion_done(&i2c_dev->complete))
+			return 1;
+	}
+
+	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 +1714,12 @@ 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);
+
 	ret = f7_msg->result;
 	if (ret) {
 		if (i2c_dev->use_dma)
@@ -1727,6 +1751,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 = 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 = true;
+	return stm32f7_i2c_xfer_core(i2c_adap, msgs, num);
+}
+
 static int stm32f7_i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 				  unsigned short flags, char read_write,
 				  u8 command, int size,
@@ -2095,6 +2137,7 @@ static u32 stm32f7_i2c_func(struct i2c_adapter *adap)
 
 static const struct i2c_algorithm stm32f7_i2c_algo = {
 	.master_xfer = stm32f7_i2c_xfer,
+	.master_xfer_atomic = stm32f7_i2c_xfer_atomic,
 	.smbus_xfer = stm32f7_i2c_smbus_xfer,
 	.functionality = stm32f7_i2c_func,
 	.reg_slave = stm32f7_i2c_reg_slave,
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[Andi Shyti]

Hi Pierre-Yves, Alain,

mind taking a look here?

[...]

> @@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
>  	u32 dnf_dt;
>  	u32 dnf;
>  	struct stm32f7_i2c_alert *alert;
> +	bool atomic;

this smells a bit racy here, this works only if the xfer's are
always sequential.

What happens when we receive at the same time two xfer's, one
atomic and one non atomic?

Andi

Re: [PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[Andi Shyti]

Hi Pierre-Yves, Alain,

mind taking a look here?

[...]

> @@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
>  	u32 dnf_dt;
>  	u32 dnf;
>  	struct stm32f7_i2c_alert *alert;
> +	bool atomic;

this smells a bit racy here, this works only if the xfer's are
always sequential.

What happens when we receive at the same time two xfer's, one
atomic and one non atomic?

Andi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[Sean Nyekjaer]

Hi Andy,

> On 3 Sep 2023, at 14.46, Andi Shyti <andi.shyti@kernel.org> wrote:
> 
> Hi Pierre-Yves, Alain,
> 
> mind taking a look here?
> 
> [...]
> 
>> @@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
>> u32 dnf_dt;
>> u32 dnf;
>> struct stm32f7_i2c_alert *alert;
>> + bool atomic;
> 
> this smells a bit racy here, this works only if the xfer's are
> always sequential.
> 
> What happens when we receive at the same time two xfer's, one
> atomic and one non atomic?

From the include/i2c.h:
 * @master_xfer_atomic: same as @master_xfer. Yet, only using atomic context
 *   so e.g. PMICs can be accessed very late before shutdown. Optional.

So it’s only used very late in the shutdown.

It’s implemented the same way as in:
drivers/i2c/busses/i2c-imx.c
drivers/i2c/busses/i2c-meson.c
drivers/i2c/busses/i2c-mv64xxx.c
drivers/i2c/busses/i2c-tegra.c
… etc…


In drivers/i2c/i2c-core.h it’s determined whether it’s atomic transfer or not:

/*
 * We only allow atomic transfers for very late communication, e.g. to access a
 * PMIC when powering down. Atomic transfers are a corner case and not for
 * generic use!
 */
static inline bool i2c_in_atomic_xfer_mode(void)
{
        return system_state > SYSTEM_RUNNING && irqs_disabled();
}

So you would not have an atomic transfer and later an non atomic.

/Sean

Re: [PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[Sean Nyekjaer]

Hi Andy,

> On 3 Sep 2023, at 14.46, Andi Shyti <andi.shyti@kernel.org> wrote:
> 
> Hi Pierre-Yves, Alain,
> 
> mind taking a look here?
> 
> [...]
> 
>> @@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
>> u32 dnf_dt;
>> u32 dnf;
>> struct stm32f7_i2c_alert *alert;
>> + bool atomic;
> 
> this smells a bit racy here, this works only if the xfer's are
> always sequential.
> 
> What happens when we receive at the same time two xfer's, one
> atomic and one non atomic?

From the include/i2c.h:
 * @master_xfer_atomic: same as @master_xfer. Yet, only using atomic context
 *   so e.g. PMICs can be accessed very late before shutdown. Optional.

So it’s only used very late in the shutdown.

It’s implemented the same way as in:
drivers/i2c/busses/i2c-imx.c
drivers/i2c/busses/i2c-meson.c
drivers/i2c/busses/i2c-mv64xxx.c
drivers/i2c/busses/i2c-tegra.c
… etc…


In drivers/i2c/i2c-core.h it’s determined whether it’s atomic transfer or not:

/*
 * We only allow atomic transfers for very late communication, e.g. to access a
 * PMIC when powering down. Atomic transfers are a corner case and not for
 * generic use!
 */
static inline bool i2c_in_atomic_xfer_mode(void)
{
        return system_state > SYSTEM_RUNNING && irqs_disabled();
}

So you would not have an atomic transfer and later an non atomic.

/Sean
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[Andi Shyti]

Hi Sean,

On Mon, Sep 04, 2023 at 07:29:59AM +0200, Sean Nyekjaer wrote:
> Hi Andy,
> 
> > On 3 Sep 2023, at 14.46, Andi Shyti <andi.shyti@kernel.org> wrote:
> > 
> > Hi Pierre-Yves, Alain,
> > 
> > mind taking a look here?
> > 
> > [...]
> > 
> >> @@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
> >> u32 dnf_dt;
> >> u32 dnf;
> >> struct stm32f7_i2c_alert *alert;
> >> + bool atomic;
> > 
> > this smells a bit racy here, this works only if the xfer's are
> > always sequential.
> > 
> > What happens when we receive at the same time two xfer's, one
> > atomic and one non atomic?
> 
> From the include/i2c.h:
>  * @master_xfer_atomic: same as @master_xfer. Yet, only using atomic context
>  *   so e.g. PMICs can be accessed very late before shutdown. Optional.
> 
> So it’s only used very late in the shutdown.
> 
> It’s implemented the same way as in:
> drivers/i2c/busses/i2c-imx.c
> drivers/i2c/busses/i2c-meson.c
> drivers/i2c/busses/i2c-mv64xxx.c
> drivers/i2c/busses/i2c-tegra.c
> … etc…
> 
> 
> In drivers/i2c/i2c-core.h it’s determined whether it’s atomic transfer or not:
> 
> /*
>  * We only allow atomic transfers for very late communication, e.g. to access a
>  * PMIC when powering down. Atomic transfers are a corner case and not for
>  * generic use!
>  */
> static inline bool i2c_in_atomic_xfer_mode(void)
> {
>         return system_state > SYSTEM_RUNNING && irqs_disabled();
> }
> 
> So you would not have an atomic transfer and later an non atomic.

What about the opposite? I.e. a non atomic and later an atomic,
for very late tardive communications :)

Andi

Re: [PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[Andi Shyti]

Hi Sean,

On Mon, Sep 04, 2023 at 07:29:59AM +0200, Sean Nyekjaer wrote:
> Hi Andy,
> 
> > On 3 Sep 2023, at 14.46, Andi Shyti <andi.shyti@kernel.org> wrote:
> > 
> > Hi Pierre-Yves, Alain,
> > 
> > mind taking a look here?
> > 
> > [...]
> > 
> >> @@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
> >> u32 dnf_dt;
> >> u32 dnf;
> >> struct stm32f7_i2c_alert *alert;
> >> + bool atomic;
> > 
> > this smells a bit racy here, this works only if the xfer's are
> > always sequential.
> > 
> > What happens when we receive at the same time two xfer's, one
> > atomic and one non atomic?
> 
> From the include/i2c.h:
>  * @master_xfer_atomic: same as @master_xfer. Yet, only using atomic context
>  *   so e.g. PMICs can be accessed very late before shutdown. Optional.
> 
> So it’s only used very late in the shutdown.
> 
> It’s implemented the same way as in:
> drivers/i2c/busses/i2c-imx.c
> drivers/i2c/busses/i2c-meson.c
> drivers/i2c/busses/i2c-mv64xxx.c
> drivers/i2c/busses/i2c-tegra.c
> … etc…
> 
> 
> In drivers/i2c/i2c-core.h it’s determined whether it’s atomic transfer or not:
> 
> /*
>  * We only allow atomic transfers for very late communication, e.g. to access a
>  * PMIC when powering down. Atomic transfers are a corner case and not for
>  * generic use!
>  */
> static inline bool i2c_in_atomic_xfer_mode(void)
> {
>         return system_state > SYSTEM_RUNNING && irqs_disabled();
> }
> 
> So you would not have an atomic transfer and later an non atomic.

What about the opposite? I.e. a non atomic and later an atomic,
for very late tardive communications :)

Andi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[Sean Nyekjaer]

Hi Andi,

> On 6 Sep 2023, at 01.08, Andi Shyti <andi.shyti@kernel.org> wrote:
> 
> Hi Sean,
> 
> On Mon, Sep 04, 2023 at 07:29:59AM +0200, Sean Nyekjaer wrote:
>> Hi Andy,
>> 
>>> On 3 Sep 2023, at 14.46, Andi Shyti <andi.shyti@kernel.org> wrote:
>>> 
>>> Hi Pierre-Yves, Alain,
>>> 
>>> mind taking a look here?
>>> 
>>> [...]
>>> 
>>>> @@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
>>>> u32 dnf_dt;
>>>> u32 dnf;
>>>> struct stm32f7_i2c_alert *alert;
>>>> + bool atomic;
>>> 
>>> this smells a bit racy here, this works only if the xfer's are
>>> always sequential.
>>> 
>>> What happens when we receive at the same time two xfer's, one
>>> atomic and one non atomic?
>> 
>> From the include/i2c.h:
>> * @master_xfer_atomic: same as @master_xfer. Yet, only using atomic context
>> *   so e.g. PMICs can be accessed very late before shutdown. Optional.
>> 
>> So it’s only used very late in the shutdown.
>> 
>> It’s implemented the same way as in:
>> drivers/i2c/busses/i2c-imx.c
>> drivers/i2c/busses/i2c-meson.c
>> drivers/i2c/busses/i2c-mv64xxx.c
>> drivers/i2c/busses/i2c-tegra.c
>> … etc…
>> 
>> 
>> In drivers/i2c/i2c-core.h it’s determined whether it’s atomic transfer or not:
>> 
>> /*
>> * We only allow atomic transfers for very late communication, e.g. to access a
>> * PMIC when powering down. Atomic transfers are a corner case and not for
>> * generic use!
>> */
>> static inline bool i2c_in_atomic_xfer_mode(void)
>> {
>>        return system_state > SYSTEM_RUNNING && irqs_disabled();
>> }
>> 
>> So you would not have an atomic transfer and later an non atomic.
> 
> What about the opposite? I.e. a non atomic and later an atomic,
> for very late tardive communications :)

Sure it’s the opposite? Normal scenario is “non atomic” transfers going on and under shutdown it switches to “atomic”.
From i2c_in_atomic_xfer_mode() it can’t go from “atomic” -> “non atomic”.

extern enum system_states {
SYSTEM_BOOTING,
SYSTEM_SCHEDULING,
SYSTEM_FREEING_INITMEM,
SYSTEM_RUNNING,
SYSTEM_HALT,
SYSTEM_POWER_OFF,
SYSTEM_RESTART,
SYSTEM_SUSPEND,
} system_state;

If you are asking what happens if a “non atomic” transfer is ongoing and irq’s is disabled, IDK.

Let’s get Wolfram in the loop (Sorry I forgot to add you) :)

/Sean

Re: [PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[Sean Nyekjaer]

Hi Andi,

> On 6 Sep 2023, at 01.08, Andi Shyti <andi.shyti@kernel.org> wrote:
> 
> Hi Sean,
> 
> On Mon, Sep 04, 2023 at 07:29:59AM +0200, Sean Nyekjaer wrote:
>> Hi Andy,
>> 
>>> On 3 Sep 2023, at 14.46, Andi Shyti <andi.shyti@kernel.org> wrote:
>>> 
>>> Hi Pierre-Yves, Alain,
>>> 
>>> mind taking a look here?
>>> 
>>> [...]
>>> 
>>>> @@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
>>>> u32 dnf_dt;
>>>> u32 dnf;
>>>> struct stm32f7_i2c_alert *alert;
>>>> + bool atomic;
>>> 
>>> this smells a bit racy here, this works only if the xfer's are
>>> always sequential.
>>> 
>>> What happens when we receive at the same time two xfer's, one
>>> atomic and one non atomic?
>> 
>> From the include/i2c.h:
>> * @master_xfer_atomic: same as @master_xfer. Yet, only using atomic context
>> *   so e.g. PMICs can be accessed very late before shutdown. Optional.
>> 
>> So it’s only used very late in the shutdown.
>> 
>> It’s implemented the same way as in:
>> drivers/i2c/busses/i2c-imx.c
>> drivers/i2c/busses/i2c-meson.c
>> drivers/i2c/busses/i2c-mv64xxx.c
>> drivers/i2c/busses/i2c-tegra.c
>> … etc…
>> 
>> 
>> In drivers/i2c/i2c-core.h it’s determined whether it’s atomic transfer or not:
>> 
>> /*
>> * We only allow atomic transfers for very late communication, e.g. to access a
>> * PMIC when powering down. Atomic transfers are a corner case and not for
>> * generic use!
>> */
>> static inline bool i2c_in_atomic_xfer_mode(void)
>> {
>>        return system_state > SYSTEM_RUNNING && irqs_disabled();
>> }
>> 
>> So you would not have an atomic transfer and later an non atomic.
> 
> What about the opposite? I.e. a non atomic and later an atomic,
> for very late tardive communications :)

Sure it’s the opposite? Normal scenario is “non atomic” transfers going on and under shutdown it switches to “atomic”.
From i2c_in_atomic_xfer_mode() it can’t go from “atomic” -> “non atomic”.

extern enum system_states {
SYSTEM_BOOTING,
SYSTEM_SCHEDULING,
SYSTEM_FREEING_INITMEM,
SYSTEM_RUNNING,
SYSTEM_HALT,
SYSTEM_POWER_OFF,
SYSTEM_RESTART,
SYSTEM_SUSPEND,
} system_state;

If you are asking what happens if a “non atomic” transfer is ongoing and irq’s is disabled, IDK.

Let’s get Wolfram in the loop (Sorry I forgot to add you) :)

/Sean


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[Andi Shyti]

Hi Sean,

> >>>> @@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
> >>>> u32 dnf_dt;
> >>>> u32 dnf;
> >>>> struct stm32f7_i2c_alert *alert;
> >>>> + bool atomic;
> >>> 
> >>> this smells a bit racy here, this works only if the xfer's are
> >>> always sequential.
> >>> 
> >>> What happens when we receive at the same time two xfer's, one
> >>> atomic and one non atomic?
> >> 
> >> From the include/i2c.h:
> >> * @master_xfer_atomic: same as @master_xfer. Yet, only using atomic context
> >> *   so e.g. PMICs can be accessed very late before shutdown. Optional.
> >> 
> >> So it’s only used very late in the shutdown.
> >> 
> >> It’s implemented the same way as in:
> >> drivers/i2c/busses/i2c-imx.c
> >> drivers/i2c/busses/i2c-meson.c
> >> drivers/i2c/busses/i2c-mv64xxx.c
> >> drivers/i2c/busses/i2c-tegra.c
> >> … etc…
> >> 
> >> 
> >> In drivers/i2c/i2c-core.h it’s determined whether it’s atomic transfer or not:
> >> 
> >> /*
> >> * We only allow atomic transfers for very late communication, e.g. to access a
> >> * PMIC when powering down. Atomic transfers are a corner case and not for
> >> * generic use!
> >> */
> >> static inline bool i2c_in_atomic_xfer_mode(void)
> >> {
> >>        return system_state > SYSTEM_RUNNING && irqs_disabled();
> >> }
> >> 
> >> So you would not have an atomic transfer and later an non atomic.
> > 
> > What about the opposite? I.e. a non atomic and later an atomic,
> > for very late tardive communications :)
> 
> Sure it’s the opposite? Normal scenario is “non atomic” transfers going on and under shutdown it switches to “atomic”.
> From i2c_in_atomic_xfer_mode() it can’t go from “atomic” -> “non atomic”.

well at some point we move from non atomic to atomic and we
preempt whatever is non atomic in order to go atomic, including
non atomic transfers.

A "global" variable thrown there without protection is a bit weak
and we need to be sure to be covering all possible scenarios when
this variable is used.

> extern enum system_states {
> SYSTEM_BOOTING,
> SYSTEM_SCHEDULING,
> SYSTEM_FREEING_INITMEM,
> SYSTEM_RUNNING,
> SYSTEM_HALT,
> SYSTEM_POWER_OFF,
> SYSTEM_RESTART,
> SYSTEM_SUSPEND,
> } system_state;
> 
> If you are asking what happens if a “non atomic” transfer is ongoing and irq’s is disabled, IDK.
> 
> Let’s get Wolfram in the loop (Sorry I forgot to add you) :)

Nah, it's OK... I am thinking aloud here and trying to cover
possible scenarios. I also think that setting up a spinlock might
be too much paranoiac and not necessary.

I'm going to ack it... but I will keep a few thoughts on thinking
what can happen wrong here.

Acked-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

Re: [PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[Andi Shyti]

Hi Sean,

> >>>> @@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
> >>>> u32 dnf_dt;
> >>>> u32 dnf;
> >>>> struct stm32f7_i2c_alert *alert;
> >>>> + bool atomic;
> >>> 
> >>> this smells a bit racy here, this works only if the xfer's are
> >>> always sequential.
> >>> 
> >>> What happens when we receive at the same time two xfer's, one
> >>> atomic and one non atomic?
> >> 
> >> From the include/i2c.h:
> >> * @master_xfer_atomic: same as @master_xfer. Yet, only using atomic context
> >> *   so e.g. PMICs can be accessed very late before shutdown. Optional.
> >> 
> >> So it’s only used very late in the shutdown.
> >> 
> >> It’s implemented the same way as in:
> >> drivers/i2c/busses/i2c-imx.c
> >> drivers/i2c/busses/i2c-meson.c
> >> drivers/i2c/busses/i2c-mv64xxx.c
> >> drivers/i2c/busses/i2c-tegra.c
> >> … etc…
> >> 
> >> 
> >> In drivers/i2c/i2c-core.h it’s determined whether it’s atomic transfer or not:
> >> 
> >> /*
> >> * We only allow atomic transfers for very late communication, e.g. to access a
> >> * PMIC when powering down. Atomic transfers are a corner case and not for
> >> * generic use!
> >> */
> >> static inline bool i2c_in_atomic_xfer_mode(void)
> >> {
> >>        return system_state > SYSTEM_RUNNING && irqs_disabled();
> >> }
> >> 
> >> So you would not have an atomic transfer and later an non atomic.
> > 
> > What about the opposite? I.e. a non atomic and later an atomic,
> > for very late tardive communications :)
> 
> Sure it’s the opposite? Normal scenario is “non atomic” transfers going on and under shutdown it switches to “atomic”.
> From i2c_in_atomic_xfer_mode() it can’t go from “atomic” -> “non atomic”.

well at some point we move from non atomic to atomic and we
preempt whatever is non atomic in order to go atomic, including
non atomic transfers.

A "global" variable thrown there without protection is a bit weak
and we need to be sure to be covering all possible scenarios when
this variable is used.

> extern enum system_states {
> SYSTEM_BOOTING,
> SYSTEM_SCHEDULING,
> SYSTEM_FREEING_INITMEM,
> SYSTEM_RUNNING,
> SYSTEM_HALT,
> SYSTEM_POWER_OFF,
> SYSTEM_RESTART,
> SYSTEM_SUSPEND,
> } system_state;
> 
> If you are asking what happens if a “non atomic” transfer is ongoing and irq’s is disabled, IDK.
> 
> Let’s get Wolfram in the loop (Sorry I forgot to add you) :)

Nah, it's OK... I am thinking aloud here and trying to cover
possible scenarios. I also think that setting up a spinlock might
be too much paranoiac and not necessary.

I'm going to ack it... but I will keep a few thoughts on thinking
what can happen wrong here.

Acked-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

On Wed, Aug 16, 2023 at 10:05:52AM +0200, Sean Nyekjaer wrote:
> Add an atomic_xfer method to the driver so that it behaves correctly
> when controlling a PMIC that is responsible for device shutdown.
> 
> The atomic_xfer method added is similar to the one from the i2c-mv64xxx
> driver. When running an atomic_xfer a bool flag in the driver data is
> set, the interrupt is not unmasked on transfer start, and the IRQ
> handler is manually invoked while waiting for pending transfers to
> complete.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 596 bytes --]

On Wed, Aug 16, 2023 at 10:05:52AM +0200, Sean Nyekjaer wrote:
> Add an atomic_xfer method to the driver so that it behaves correctly
> when controlling a PMIC that is responsible for device shutdown.
> 
> The atomic_xfer method added is similar to the one from the i2c-mv64xxx
> driver. When running an atomic_xfer a bool flag in the driver data is
> set, the interrupt is not unmasked on transfer start, and the IRQ
> handler is manually invoked while waiting for pending transfers to
> complete.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>

Applied to for-next, thanks!


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel