[PATCH] counter: interrupt-cnt: Protect enable/disable OPs with mutex

[PATCH] counter: interrupt-cnt: Protect enable/disable OPs with mutex

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Enable/disable seems to be racy on SMP, consider the following scenario:

CPU0					CPU1

interrupt_cnt_enable_write(true)
{
	if (priv->enabled == enable)
		return 0;

	if (enable) {
		priv->enabled = true;
					interrupt_cnt_enable_write(false)
					{
						if (priv->enabled == enable)
							return 0;

						if (enable) {
							priv->enabled = true;
							enable_irq(priv->irq);
						} else {
							disable_irq(priv->irq)
							priv->enabled = false;
						}
		enable_irq(priv->irq);
	} else {
		disable_irq(priv->irq);
		priv->enabled = false;
	}

The above would result in priv->enabled == false, but IRQ left enabled.
Protect both write (above race) and read (to propagate the value on SMP)
callbacks with a mutex.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/counter/interrupt-cnt.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
index 949598d51575a..d83848d0fe2af 100644
--- a/drivers/counter/interrupt-cnt.c
+++ b/drivers/counter/interrupt-cnt.c
@@ -3,12 +3,14 @@
  * Copyright (c) 2021 Pengutronix, Oleksij Rempel <kernel@pengutronix.de>
  */
 
+#include <linux/cleanup.h>
 #include <linux/counter.h>
 #include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
 
@@ -19,6 +21,7 @@ struct interrupt_cnt_priv {
 	struct gpio_desc *gpio;
 	int irq;
 	bool enabled;
+	struct mutex lock;
 	struct counter_signal signals;
 	struct counter_synapse synapses;
 	struct counter_count cnts;
@@ -41,6 +44,8 @@ static int interrupt_cnt_enable_read(struct counter_device *counter,
 {
 	struct interrupt_cnt_priv *priv = counter_priv(counter);
 
+	guard(mutex)(&priv->lock);
+
 	*enable = priv->enabled;
 
 	return 0;
@@ -51,6 +56,8 @@ static int interrupt_cnt_enable_write(struct counter_device *counter,
 {
 	struct interrupt_cnt_priv *priv = counter_priv(counter);
 
+	guard(mutex)(&priv->lock);
+
 	if (priv->enabled == enable)
 		return 0;
 
@@ -227,6 +234,8 @@ static int interrupt_cnt_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	mutex_init(&priv->lock);
+
 	ret = devm_counter_add(dev, counter);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to add counter\n");
-- 
2.49.0

Re: [PATCH] counter: interrupt-cnt: Protect enable/disable OPs with mutex

[Ahmad Fatoum]

Hello Alexander,

On 31.03.25 18:36, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> Enable/disable seems to be racy on SMP, consider the following scenario:
> 
> CPU0					CPU1
> 
> interrupt_cnt_enable_write(true)
> {
> 	if (priv->enabled == enable)
> 		return 0;
> 
> 	if (enable) {
> 		priv->enabled = true;
> 					interrupt_cnt_enable_write(false)
> 					{
> 						if (priv->enabled == enable)
> 							return 0;
> 
> 						if (enable) {
> 							priv->enabled = true;
> 							enable_irq(priv->irq);
> 						} else {
> 							disable_irq(priv->irq)
> 							priv->enabled = false;
> 						}
> 		enable_irq(priv->irq);
> 	} else {
> 		disable_irq(priv->irq);
> 		priv->enabled = false;
> 	}
> 
> The above would result in priv->enabled == false, but IRQ left enabled.
> Protect both write (above race) and read (to propagate the value on SMP)
> callbacks with a mutex.

Doesn't sysfs/kernfs already ensure that the ops may not be called concurrently
on the same open file?

Thanks,
Ahmad

> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> ---
>  drivers/counter/interrupt-cnt.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> index 949598d51575a..d83848d0fe2af 100644
> --- a/drivers/counter/interrupt-cnt.c
> +++ b/drivers/counter/interrupt-cnt.c
> @@ -3,12 +3,14 @@
>   * Copyright (c) 2021 Pengutronix, Oleksij Rempel <kernel@pengutronix.de>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/counter.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/platform_device.h>
>  #include <linux/types.h>
>  
> @@ -19,6 +21,7 @@ struct interrupt_cnt_priv {
>  	struct gpio_desc *gpio;
>  	int irq;
>  	bool enabled;
> +	struct mutex lock;
>  	struct counter_signal signals;
>  	struct counter_synapse synapses;
>  	struct counter_count cnts;
> @@ -41,6 +44,8 @@ static int interrupt_cnt_enable_read(struct counter_device *counter,
>  {
>  	struct interrupt_cnt_priv *priv = counter_priv(counter);
>  
> +	guard(mutex)(&priv->lock);
> +
>  	*enable = priv->enabled;
>  
>  	return 0;
> @@ -51,6 +56,8 @@ static int interrupt_cnt_enable_write(struct counter_device *counter,
>  {
>  	struct interrupt_cnt_priv *priv = counter_priv(counter);
>  
> +	guard(mutex)(&priv->lock);
> +
>  	if (priv->enabled == enable)
>  		return 0;
>  
> @@ -227,6 +234,8 @@ static int interrupt_cnt_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	mutex_init(&priv->lock);
> +
>  	ret = devm_counter_add(dev, counter);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to add counter\n");


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] counter: interrupt-cnt: Protect enable/disable OPs with mutex

[Sverdlin, Alexander]

Hi Ahmad,

On Tue, 2025-04-01 at 06:50 +0200, Ahmad Fatoum wrote:
> > Enable/disable seems to be racy on SMP, consider the following scenario:
> > 
> > CPU0					CPU1
> > 
> > interrupt_cnt_enable_write(true)
> > {
> >  	if (priv->enabled == enable)
> >  		return 0;
> > 
> >  	if (enable) {
> >  		priv->enabled = true;
> >  					interrupt_cnt_enable_write(false)
> >  					{
> >  						if (priv->enabled == enable)
> >  							return 0;
> > 
> >  						if (enable) {
> >  							priv->enabled = true;
> >  							enable_irq(priv->irq);
> >  						} else {
> >  							disable_irq(priv->irq)
> >  							priv->enabled = false;
> >  						}
> >  		enable_irq(priv->irq);
> >  	} else {
> >  		disable_irq(priv->irq);
> >  		priv->enabled = false;
> >  	}
> > 
> > The above would result in priv->enabled == false, but IRQ left enabled.
> > Protect both write (above race) and read (to propagate the value on SMP)
> > callbacks with a mutex.
> 
> Doesn't sysfs/kernfs already ensure that the ops may not be called concurrently
> on the same open file?

as I understand the code, the operations on the same FD will be serialized, i.e.
if you open an entry and access it concurrently within the same process.

If you apply the following patch on top of the proposed patch:

--- a/drivers/counter/interrupt-cnt.c
+++ b/drivers/counter/interrupt-cnt.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2021 Pengutronix, Oleksij Rempel <kernel@pengutronix.de>
  */
 
+#include <linux/delay.h>
 #include <linux/cleanup.h>
 #include <linux/counter.h>
 #include <linux/gpio/consumer.h>
@@ -56,6 +57,9 @@ static int interrupt_cnt_enable_write(struct counter_device *counter,
 {
 	struct interrupt_cnt_priv *priv = counter_priv(counter);
 
+	WARN_ON(!mutex_trylock(&priv->lock));
+	mutex_unlock(&priv->lock);
+
 	guard(mutex)(&priv->lock);
 
 	if (priv->enabled == enable)
@@ -69,6 +73,8 @@ static int interrupt_cnt_enable_write(struct counter_device *counter,
 		priv->enabled = false;
 	}
 
+	msleep(1000);
+
 	return 0;
 }
 

And would run `while true; do echo 0 > /sys/.../enable; echo 1 > /sys/.../enable; done &`
twice, you'd see the following quickly:

WARNING: CPU: 1 PID: 754 at /drivers/counter/interrupt-cnt.c:60 interrupt_cnt_enable_write+0xa0/0xb0 [interrupt_cnt]
CPU: 1 UID: 0 PID: 754 Comm: sh
pc : interrupt_cnt_enable_write+0xa0/0xb0 [interrupt_cnt]
lr : interrupt_cnt_enable_write+0x34/0xb0 [interrupt_cnt]
Call trace:
 interrupt_cnt_enable_write+0xa0/0xb0 [interrupt_cnt] (P)
 counter_comp_u8_store+0xcc/0x118 [counter]
 dev_attr_store+0x20/0x40
 sysfs_kf_write+0x84/0xa8
 kernfs_fop_write_iter+0x128/0x1e0
 vfs_write+0x248/0x388
 ksys_write+0x78/0x118
 __arm64_sys_write+0x24/0x38
 invoke_syscall+0x50/0x120

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH] counter: interrupt-cnt: Protect enable/disable OPs with mutex

[Ahmad Fatoum]

Hello Alexander,

On 4/1/25 10:38, Sverdlin, Alexander wrote:
> On Tue, 2025-04-01 at 06:50 +0200, Ahmad Fatoum wrote:
>>> Enable/disable seems to be racy on SMP, consider the following scenario:
>>>
>>> CPU0					CPU1
>>>
>>> interrupt_cnt_enable_write(true)
>>> {
>>>  	if (priv->enabled == enable)
>>>  		return 0;
>>>
>>>  	if (enable) {
>>>  		priv->enabled = true;
>>>  					interrupt_cnt_enable_write(false)
>>>  					{
>>>  						if (priv->enabled == enable)
>>>  							return 0;
>>>
>>>  						if (enable) {
>>>  							priv->enabled = true;
>>>  							enable_irq(priv->irq);
>>>  						} else {
>>>  							disable_irq(priv->irq)
>>>  							priv->enabled = false;
>>>  						}
>>>  		enable_irq(priv->irq);
>>>  	} else {
>>>  		disable_irq(priv->irq);
>>>  		priv->enabled = false;
>>>  	}
>>>
>>> The above would result in priv->enabled == false, but IRQ left enabled.
>>> Protect both write (above race) and read (to propagate the value on SMP)
>>> callbacks with a mutex.
>>
>> Doesn't sysfs/kernfs already ensure that the ops may not be called concurrently
>> on the same open file?
> 
> as I understand the code, the operations on the same FD will be serialized, i.e.
> if you open an entry and access it concurrently within the same process.

Thanks for checking! I agree now that we indeed need synchronization here.

Cheers,
Ahmad

> 
> If you apply the following patch on top of the proposed patch:
> 
> --- a/drivers/counter/interrupt-cnt.c
> +++ b/drivers/counter/interrupt-cnt.c
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2021 Pengutronix, Oleksij Rempel <kernel@pengutronix.de>
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/cleanup.h>
>  #include <linux/counter.h>
>  #include <linux/gpio/consumer.h>
> @@ -56,6 +57,9 @@ static int interrupt_cnt_enable_write(struct counter_device *counter,
>  {
>  	struct interrupt_cnt_priv *priv = counter_priv(counter);
>  
> +	WARN_ON(!mutex_trylock(&priv->lock));
> +	mutex_unlock(&priv->lock);
> +
>  	guard(mutex)(&priv->lock);
>  
>  	if (priv->enabled == enable)
> @@ -69,6 +73,8 @@ static int interrupt_cnt_enable_write(struct counter_device *counter,
>  		priv->enabled = false;
>  	}
>  
> +	msleep(1000);
> +
>  	return 0;
>  }
>  
> 
> And would run `while true; do echo 0 > /sys/.../enable; echo 1 > /sys/.../enable; done &`
> twice, you'd see the following quickly:
> 
> WARNING: CPU: 1 PID: 754 at /drivers/counter/interrupt-cnt.c:60 interrupt_cnt_enable_write+0xa0/0xb0 [interrupt_cnt]
> CPU: 1 UID: 0 PID: 754 Comm: sh
> pc : interrupt_cnt_enable_write+0xa0/0xb0 [interrupt_cnt]
> lr : interrupt_cnt_enable_write+0x34/0xb0 [interrupt_cnt]
> Call trace:
>  interrupt_cnt_enable_write+0xa0/0xb0 [interrupt_cnt] (P)
>  counter_comp_u8_store+0xcc/0x118 [counter]
>  dev_attr_store+0x20/0x40
>  sysfs_kf_write+0x84/0xa8
>  kernfs_fop_write_iter+0x128/0x1e0
>  vfs_write+0x248/0x388
>  ksys_write+0x78/0x118
>  __arm64_sys_write+0x24/0x38
>  invoke_syscall+0x50/0x120
> 

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |

Re: [PATCH] counter: interrupt-cnt: Protect enable/disable OPs with mutex

[Sverdlin, Alexander]

Dear maintainers,

On Mon, 2025-03-31 at 18:36 +0200, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> Enable/disable seems to be racy on SMP, consider the following scenario:
> 
> CPU0					CPU1
> 
> interrupt_cnt_enable_write(true)
> {
> 	if (priv->enabled == enable)
> 		return 0;
> 
> 	if (enable) {
> 		priv->enabled = true;
> 					interrupt_cnt_enable_write(false)
> 					{
> 						if (priv->enabled == enable)
> 							return 0;
> 
> 						if (enable) {
> 							priv->enabled = true;
> 							enable_irq(priv->irq);
> 						} else {
> 							disable_irq(priv->irq)
> 							priv->enabled = false;
> 						}
> 		enable_irq(priv->irq);
> 	} else {
> 		disable_irq(priv->irq);
> 		priv->enabled = false;
> 	}
> 
> The above would result in priv->enabled == false, but IRQ left enabled.
> Protect both write (above race) and read (to propagate the value on SMP)
> callbacks with a mutex.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

I've noticed that the patch has been marked as "Changes Requested" in
the patchwork, could it be a mistake? Because I never received any
change request.

> ---
>  drivers/counter/interrupt-cnt.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> index 949598d51575a..d83848d0fe2af 100644
> --- a/drivers/counter/interrupt-cnt.c
> +++ b/drivers/counter/interrupt-cnt.c
> @@ -3,12 +3,14 @@
>   * Copyright (c) 2021 Pengutronix, Oleksij Rempel <kernel@pengutronix.de>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/counter.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/platform_device.h>
>  #include <linux/types.h>
>  
> @@ -19,6 +21,7 @@ struct interrupt_cnt_priv {
>  	struct gpio_desc *gpio;
>  	int irq;
>  	bool enabled;
> +	struct mutex lock;
>  	struct counter_signal signals;
>  	struct counter_synapse synapses;
>  	struct counter_count cnts;
> @@ -41,6 +44,8 @@ static int interrupt_cnt_enable_read(struct counter_device *counter,
>  {
>  	struct interrupt_cnt_priv *priv = counter_priv(counter);
>  
> +	guard(mutex)(&priv->lock);
> +
>  	*enable = priv->enabled;
>  
>  	return 0;
> @@ -51,6 +56,8 @@ static int interrupt_cnt_enable_write(struct counter_device *counter,
>  {
>  	struct interrupt_cnt_priv *priv = counter_priv(counter);
>  
> +	guard(mutex)(&priv->lock);
> +
>  	if (priv->enabled == enable)
>  		return 0;
>  
> @@ -227,6 +234,8 @@ static int interrupt_cnt_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	mutex_init(&priv->lock);
> +
>  	ret = devm_counter_add(dev, counter);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to add counter\n");

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH] counter: interrupt-cnt: Protect enable/disable OPs with mutex

[William Breathitt Gray]

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

On Fri, May 02, 2025 at 09:24:20AM +0000, Sverdlin, Alexander wrote:
> Dear maintainers,
> 
> On Mon, 2025-03-31 at 18:36 +0200, A. Sverdlin wrote:
> > From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> >
> > Enable/disable seems to be racy on SMP, consider the following scenario:
> >
> > CPU0					CPU1
> >
> > interrupt_cnt_enable_write(true)
> > {
> > 	if (priv->enabled == enable)
> > 		return 0;
> >
> > 	if (enable) {
> > 		priv->enabled = true;
> > 					interrupt_cnt_enable_write(false)
> > 					{
> > 						if (priv->enabled == enable)
> > 							return 0;
> >
> > 						if (enable) {
> > 							priv->enabled = true;
> > 							enable_irq(priv->irq);
> > 						} else {
> > 							disable_irq(priv->irq)
> > 							priv->enabled = false;
> > 						}
> > 		enable_irq(priv->irq);
> > 	} else {
> > 		disable_irq(priv->irq);
> > 		priv->enabled = false;
> > 	}
> >
> > The above would result in priv->enabled == false, but IRQ left enabled.
> > Protect both write (above race) and read (to propagate the value on SMP)
> > callbacks with a mutex.
> >
> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> I've noticed that the patch has been marked as "Changes Requested" in
> the patchwork, could it be a mistake? Because I never received any
> change request.

Hi Alexander,

I can't comment on the patchwork status because I don't use that
service, but I apologize nonetheless for the delay in responding to your
patch submission. I'm hoping for an Ack from Oleksij, but this is a
pretty straight-forward fix that I'll be happy to pick it up regardless.

Would you provide a Fixes line so the stable trees can pick this up for
the necessary kernel versions?

Thanks,

William Breathitt Gray

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

Re: [PATCH] counter: interrupt-cnt: Protect enable/disable OPs with mutex

[Oleksij Rempel]

On Fri, May 02, 2025 at 08:43:18PM +0900, William Breathitt Gray wrote:
> On Fri, May 02, 2025 at 09:24:20AM +0000, Sverdlin, Alexander wrote:
> > Dear maintainers,
> > 
> > On Mon, 2025-03-31 at 18:36 +0200, A. Sverdlin wrote:
> > > From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > >
> > > Enable/disable seems to be racy on SMP, consider the following scenario:
> > >
> > > CPU0					CPU1
> > >
> > > interrupt_cnt_enable_write(true)
> > > {
> > > 	if (priv->enabled == enable)
> > > 		return 0;
> > >
> > > 	if (enable) {
> > > 		priv->enabled = true;
> > > 					interrupt_cnt_enable_write(false)
> > > 					{
> > > 						if (priv->enabled == enable)
> > > 							return 0;
> > >
> > > 						if (enable) {
> > > 							priv->enabled = true;
> > > 							enable_irq(priv->irq);
> > > 						} else {
> > > 							disable_irq(priv->irq)
> > > 							priv->enabled = false;
> > > 						}
> > > 		enable_irq(priv->irq);
> > > 	} else {
> > > 		disable_irq(priv->irq);
> > > 		priv->enabled = false;
> > > 	}
> > >
> > > The above would result in priv->enabled == false, but IRQ left enabled.
> > > Protect both write (above race) and read (to propagate the value on SMP)
> > > callbacks with a mutex.
> > >
> > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > 
> > I've noticed that the patch has been marked as "Changes Requested" in
> > the patchwork, could it be a mistake? Because I never received any
> > change request.
> 
> Hi Alexander,
> 
> I can't comment on the patchwork status because I don't use that
> service, but I apologize nonetheless for the delay in responding to your
> patch submission. I'm hoping for an Ack from Oleksij, but this is a
> pretty straight-forward fix that I'll be happy to pick it up regardless.
> 
> Would you provide a Fixes line so the stable trees can pick this up for
> the necessary kernel versions?

Sorry for delay, you can add my
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] counter: interrupt-cnt: Protect enable/disable OPs with mutex

[William Breathitt Gray]

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

On Fri, May 02, 2025 at 01:50:24PM +0200, Oleksij Rempel wrote:
> On Fri, May 02, 2025 at 08:43:18PM +0900, William Breathitt Gray wrote:
> > On Fri, May 02, 2025 at 09:24:20AM +0000, Sverdlin, Alexander wrote:
> > > I've noticed that the patch has been marked as "Changes Requested" in
> > > the patchwork, could it be a mistake? Because I never received any
> > > change request.
> >
> > Hi Alexander,
> >
> > I can't comment on the patchwork status because I don't use that
> > service, but I apologize nonetheless for the delay in responding to your
> > patch submission. I'm hoping for an Ack from Oleksij, but this is a
> > pretty straight-forward fix that I'll be happy to pick it up regardless.
> >
> > Would you provide a Fixes line so the stable trees can pick this up for
> > the necessary kernel versions?
> 
> Sorry for delay, you can add my
> Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thanks Oleksij!

Would you provide an Ack as well for Alexander's other interrupt-cnt
patch involving a change from atomic_t to atomic_long_t? [^1]

William Breathitt Gray

[^1] https://lore.kernel.org/linux-iio/20250331152222.2263776-1-alexander.sverdlin@siemens.com/

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

Re: [PATCH] counter: interrupt-cnt: Protect enable/disable OPs with mutex

[Sverdlin, Alexander]

Thanks for the quick reply William,

On Fri, 2025-05-02 at 20:43 +0900, William Breathitt Gray wrote:
> > > From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > > 
> > > Enable/disable seems to be racy on SMP, consider the following scenario:
> > > 
> > > CPU0 CPU1
> > > 
> > > interrupt_cnt_enable_write(true)
> > > {
> > >   if (priv->enabled == enable)
> > >   return 0;
> > > 
> > >   if (enable) {
> > >   priv->enabled = true;
> > >   interrupt_cnt_enable_write(false)
> > >   {
> > >   if (priv->enabled == enable)
> > >   return 0;
> > > 
> > >   if (enable) {
> > >   priv->enabled = true;
> > >   enable_irq(priv->irq);
> > >   } else {
> > >   disable_irq(priv->irq)
> > >   priv->enabled = false;
> > >   }
> > >   enable_irq(priv->irq);
> > >   } else {
> > >   disable_irq(priv->irq);
> > >   priv->enabled = false;
> > >   }
> > > 
> > > The above would result in priv->enabled == false, but IRQ left enabled.
> > > Protect both write (above race) and read (to propagate the value on SMP)
> > > callbacks with a mutex.
> > > 
> > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > 
> > I've noticed that the patch has been marked as "Changes Requested" in
> > the patchwork, could it be a mistake? Because I never received any
> > change request.
> 
> Hi Alexander,
> 
> I can't comment on the patchwork status because I don't use that
> service, but I apologize nonetheless for the delay in responding to your
> patch submission. I'm hoping for an Ack from Oleksij, but this is a
> pretty straight-forward fix that I'll be happy to pick it up regardless.
> 
> Would you provide a Fixes line so the stable trees can pick this up for
> the necessary kernel versions?

shall I re-spin or would this separate tag suffice?

Fixes: a55ebd47f21f ("counter: add IRQ or GPIO based counter")

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH] counter: interrupt-cnt: Protect enable/disable OPs with mutex

[William Breathitt Gray]

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

On Fri, May 02, 2025 at 04:32:02PM +0000, Sverdlin, Alexander wrote:
> On Fri, 2025-05-02 at 20:43 +0900, William Breathitt Gray wrote:
> > Would you provide a Fixes line so the stable trees can pick this up for
> > the necessary kernel versions?
> 
> shall I re-spin or would this separate tag suffice?
> 
> Fixes: a55ebd47f21f ("counter: add IRQ or GPIO based counter")

No need to resend, I'll add in this Fixes line as I pick it up.

Thanks!

William Breathitt Gray

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

Re: [PATCH] counter: interrupt-cnt: Protect enable/disable OPs with mutex

[William Breathitt Gray]

On Mon, 31 Mar 2025 18:36:40 +0200, A. Sverdlin wrote:
> Enable/disable seems to be racy on SMP, consider the following scenario:
> 
> CPU0					CPU1
> 
> interrupt_cnt_enable_write(true)
> {
> 	if (priv->enabled == enable)
> 		return 0;
> 
> [...]

Applied, thanks!

[1/1] counter: interrupt-cnt: Protect enable/disable OPs with mutex
      commit: 7351312632e831e51383f48957d47712fae791ef

Best regards,
-- 
William Breathitt Gray <wbg@kernel.org>