[PATCH v3 0/2] tty: serial: dz: convert atomic_* to refcount_*

[PATCH v3 0/2] tty: serial: dz: convert atomic_* to refcount_*

[Deepak R Varma]

The patch series proposes to transition the driver from using atomic_t APIs to
refcount_t APIs for reference count management.

Note: patch 2/2 depends on patch 1/2. Hence please apply path 1/2 first.

Changes in v3:
   1. Patch series introduced rather than individual patches.
   2. Update patch subject line to indicate the atomic_t variable being changed

Changes in v2:
   1. Separate the change to patch per variable rather than combining multiple
      atomic variable changes into a single patch.

Please note:
   The patches are compile tested using dec_station.defconfig for MIPS architecture.

Deepak R Varma (2):
  tty: serial: dz: convert atomic_* to refcount_* APIs for map_guard
  tty: serial: dz: convert atomic_* to refcount_* APIs for irq_guard

 drivers/tty/serial/dz.c | 40 +++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

--
2.34.1

[PATCH v3 1/2] tty: serial: dz: convert atomic_* to refcount_* APIs for map_guard

[Deepak R Varma]

The refcount_* APIs are designed to address known issues with the
atomic_t APIs for reference counting. They provide following distinct
advantages
   - protect the reference counters from overflow/underflow
   - avoid use-after-free errors
   - provide improved memory ordering guarantee schemes
   - neater and safer.
Hence, replace the atomic_* APIs by their equivalent refcount_t
API functions.

This patch proposal address the following warnings generated by
the atomic_as_refcounter.cocci coccinelle script
atomic_add_return(-1, ...)

Signed-off-by: Deepak R Varma <drv@mailo.com>
---
Please Note:
   1. The patch is compile tested using dec_station.defconfig for MIPS architecture.
   2. This patch should be applied before patch 2/2 of this series due to
      dependency.

Changes in v3:
   1. Include the individual patches in a series and highlight dependency.
      Feedback provided by gregkh@linuxfoundation.org

Changes in v2:
   1. Separate the combined change into one variable per patch as
      suggested by gregkh@linuxfoundation.org
   2. There was additional feedback on validating the change as it appeared to
      modify the existing logic. However, I found that the logic does not
      change with the proposed refcount_* APIs used in this change. Hence that
      feedback is not applied in this version.



 drivers/tty/serial/dz.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/dz.c b/drivers/tty/serial/dz.c
index 6b7ed7f2f3ca..b70edc248f8b 100644
--- a/drivers/tty/serial/dz.c
+++ b/drivers/tty/serial/dz.c
@@ -47,6 +47,7 @@
 #include <linux/tty_flip.h>

 #include <linux/atomic.h>
+#include <linux/refcount.h>
 #include <linux/io.h>
 #include <asm/bootinfo.h>

@@ -75,7 +76,7 @@ struct dz_port {

 struct dz_mux {
 	struct dz_port		dport[DZ_NB_PORT];
-	atomic_t		map_guard;
+	refcount_t		map_guard;
 	atomic_t		irq_guard;
 	int			initialised;
 };
@@ -662,13 +663,11 @@ static const char *dz_type(struct uart_port *uport)
 static void dz_release_port(struct uart_port *uport)
 {
 	struct dz_mux *mux = to_dport(uport)->mux;
-	int map_guard;

 	iounmap(uport->membase);
 	uport->membase = NULL;

-	map_guard = atomic_add_return(-1, &mux->map_guard);
-	if (!map_guard)
+	if (refcount_dec_and_test(&mux->map_guard))
 		release_mem_region(uport->mapbase, dec_kn_slot_size);
 }

@@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
 static int dz_request_port(struct uart_port *uport)
 {
 	struct dz_mux *mux = to_dport(uport)->mux;
-	int map_guard;
 	int ret;

-	map_guard = atomic_add_return(1, &mux->map_guard);
-	if (map_guard == 1) {
-		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
-					"dz")) {
-			atomic_add(-1, &mux->map_guard);
-			printk(KERN_ERR
-			       "dz: Unable to reserve MMIO resource\n");
+	refcount_inc(&mux->map_guard);
+	if (refcount_read(&mux->map_guard) == 1) {
+		if (!request_mem_region(uport->mapbase, dec_kn_slot_size, "dz")) {
+			refcount_dec(&mux->map_guard);
+			printk(KERN_ERR "dz: Unable to reserve MMIO resource\n");
 			return -EBUSY;
 		}
 	}
 	ret = dz_map_port(uport);
 	if (ret) {
-		map_guard = atomic_add_return(-1, &mux->map_guard);
-		if (!map_guard)
+		if (refcount_dec_and_test(&mux->map_guard))
 			release_mem_region(uport->mapbase, dec_kn_slot_size);
 		return ret;
 	}
--
2.34.1

[PATCH 2/2] tty: serial: dz: convert atomic_* to refcount_* APIs for irq_guard

[Deepak R Varma]

The refcount_* APIs are designed to address known issues with the
atomic_t APIs for reference counting. They provide following distinct
advantages:
   - protect the reference counters from overflow/underflow
   - avoid use-after-free errors
   - provide improved memory ordering guarantee schemes
   - neater and safer.
Hence, replace the atomic_* APIs by their equivalent refcount_t
API functions.

This patch proposal address the following warnings generated by
the atomic_as_refcounter.cocci coccinelle script
atomic_add_return(-1, ...)

Signed-off-by: Deepak R Varma <drv@mailo.com>
---
Please Note:
   1. The patch is compile tested using dec_station.defconfig for MIPS architecture.
   2. This patch should be applied after patch 1/2 of this series due to
      dependency.

Changes in v3:
   1. Include the individual patches in a series and highlight dependency.
      Feedback provided by gregkh@linuxfoundation.org

Changes in v2:
   1. Separate the combined change into one variable per patch as
      suggested by gregkh@linuxfoundation.org


 drivers/tty/serial/dz.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/dz.c b/drivers/tty/serial/dz.c
index b70edc248f8b..0aa59a9beeb7 100644
--- a/drivers/tty/serial/dz.c
+++ b/drivers/tty/serial/dz.c
@@ -46,7 +46,6 @@
 #include <linux/tty.h>
 #include <linux/tty_flip.h>

-#include <linux/atomic.h>
 #include <linux/refcount.h>
 #include <linux/io.h>
 #include <asm/bootinfo.h>
@@ -77,7 +76,7 @@ struct dz_port {
 struct dz_mux {
 	struct dz_port		dport[DZ_NB_PORT];
 	refcount_t		map_guard;
-	atomic_t		irq_guard;
+	refcount_t		irq_guard;
 	int			initialised;
 };

@@ -400,18 +399,16 @@ static int dz_startup(struct uart_port *uport)
 	struct dz_port *dport = to_dport(uport);
 	struct dz_mux *mux = dport->mux;
 	unsigned long flags;
-	int irq_guard;
 	int ret;
 	u16 tmp;

-	irq_guard = atomic_add_return(1, &mux->irq_guard);
-	if (irq_guard != 1)
+	refcount_inc(&mux->irq_guard);
+	if (refcount_read(&mux->irq_guard) != 1)
 		return 0;

-	ret = request_irq(dport->port.irq, dz_interrupt,
-			  IRQF_SHARED, "dz", mux);
+	ret = request_irq(dport->port.irq, dz_interrupt, IRQF_SHARED, "dz", mux);
 	if (ret) {
-		atomic_add(-1, &mux->irq_guard);
+		refcount_dec(&mux->irq_guard);
 		printk(KERN_ERR "dz: Cannot get IRQ %d!\n", dport->port.irq);
 		return ret;
 	}
@@ -441,15 +438,13 @@ static void dz_shutdown(struct uart_port *uport)
 	struct dz_port *dport = to_dport(uport);
 	struct dz_mux *mux = dport->mux;
 	unsigned long flags;
-	int irq_guard;
 	u16 tmp;

 	spin_lock_irqsave(&dport->port.lock, flags);
 	dz_stop_tx(&dport->port);
 	spin_unlock_irqrestore(&dport->port.lock, flags);

-	irq_guard = atomic_add_return(-1, &mux->irq_guard);
-	if (!irq_guard) {
+	if (refcount_dec_and_test(&mux->irq_guard)) {
 		/* Disable interrupts.  */
 		tmp = dz_in(dport, DZ_CSR);
 		tmp &= ~(DZ_RIE | DZ_TIE);
--
2.34.1

[PATCH v4 0/2] tty: serial: dz: convert atomic_* to refcount_*

[Deepak R Varma]

The patch series proposes to transition the driver from using atomic_t APIs to
refcount_t APIs for reference count management.

Dependency note: Please apply path 1/2 first since patch 2/2 depends on it.

Changes in v4:
   1. Correct the dependency note text above.
   2. Version label added for patch 2/2. No functional change.

Changes in v3:
   1. Patch series introduced rather than individual patches.
   2. Update patch subject line to indicate the atomic_t variable being changed

Changes in v2:
   1. Separate the change to patch per variable rather than combining multiple
      atomic variable changes into a single patch.

Please note:
   The patches are compile tested using dec_station.defconfig for MIPS architecture.

Deepak R Varma (2):
  tty: serial: dz: convert atomic_* to refcount_* APIs for map_guard
  tty: serial: dz: convert atomic_* to refcount_* APIs for irq_guard

 drivers/tty/serial/dz.c | 40 +++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

--
2.34.1

[PATCH v4 1/2] tty: serial: dz: convert atomic_* to refcount_* APIs for map_guard

[Deepak R Varma]

The refcount_* APIs are designed to address known issues with the
atomic_t APIs for reference counting. They provide following distinct
advantages
   - protect the reference counters from overflow/underflow
   - avoid use-after-free errors
   - provide improved memory ordering guarantee schemes
   - neater and safer.
Hence, replace the atomic_* APIs by their equivalent refcount_t
API functions.

This patch proposal address the following warnings generated by
the atomic_as_refcounter.cocci coccinelle script
atomic_add_return(-1, ...)

Signed-off-by: Deepak R Varma <drv@mailo.com>
---
Please Note:
   1. The patch is compile tested using dec_station.defconfig for MIPS architecture.
   2. This patch should be applied before patch 2/2 of this series due to
      dependency.

Changes in v4:
   1. None.

Changes in v3:
   1. Include the individual patches in a series and highlight dependency.
      Feedback provided by gregkh@linuxfoundation.org

Changes in v2:
   1. Separate the combined change into one variable per patch as
      suggested by gregkh@linuxfoundation.org
   2. There was additional feedback on validating the change as it appeared to
      modify the existing logic. However, I found that the logic does not
      change with the proposed refcount_* APIs used in this change. Hence that
      feedback is not applied in this version.



 drivers/tty/serial/dz.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/dz.c b/drivers/tty/serial/dz.c
index 6b7ed7f2f3ca..b70edc248f8b 100644
--- a/drivers/tty/serial/dz.c
+++ b/drivers/tty/serial/dz.c
@@ -47,6 +47,7 @@
 #include <linux/tty_flip.h>

 #include <linux/atomic.h>
+#include <linux/refcount.h>
 #include <linux/io.h>
 #include <asm/bootinfo.h>

@@ -75,7 +76,7 @@ struct dz_port {

 struct dz_mux {
 	struct dz_port		dport[DZ_NB_PORT];
-	atomic_t		map_guard;
+	refcount_t		map_guard;
 	atomic_t		irq_guard;
 	int			initialised;
 };
@@ -662,13 +663,11 @@ static const char *dz_type(struct uart_port *uport)
 static void dz_release_port(struct uart_port *uport)
 {
 	struct dz_mux *mux = to_dport(uport)->mux;
-	int map_guard;

 	iounmap(uport->membase);
 	uport->membase = NULL;

-	map_guard = atomic_add_return(-1, &mux->map_guard);
-	if (!map_guard)
+	if (refcount_dec_and_test(&mux->map_guard))
 		release_mem_region(uport->mapbase, dec_kn_slot_size);
 }

@@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
 static int dz_request_port(struct uart_port *uport)
 {
 	struct dz_mux *mux = to_dport(uport)->mux;
-	int map_guard;
 	int ret;

-	map_guard = atomic_add_return(1, &mux->map_guard);
-	if (map_guard == 1) {
-		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
-					"dz")) {
-			atomic_add(-1, &mux->map_guard);
-			printk(KERN_ERR
-			       "dz: Unable to reserve MMIO resource\n");
+	refcount_inc(&mux->map_guard);
+	if (refcount_read(&mux->map_guard) == 1) {
+		if (!request_mem_region(uport->mapbase, dec_kn_slot_size, "dz")) {
+			refcount_dec(&mux->map_guard);
+			printk(KERN_ERR "dz: Unable to reserve MMIO resource\n");
 			return -EBUSY;
 		}
 	}
 	ret = dz_map_port(uport);
 	if (ret) {
-		map_guard = atomic_add_return(-1, &mux->map_guard);
-		if (!map_guard)
+		if (refcount_dec_and_test(&mux->map_guard))
 			release_mem_region(uport->mapbase, dec_kn_slot_size);
 		return ret;
 	}
--
2.34.1

[PATCH v4 2/2] tty: serial: dz: convert atomic_* to refcount_* APIs for irq_guard

[Deepak R Varma]

The refcount_* APIs are designed to address known issues with the
atomic_t APIs for reference counting. They provide following distinct
advantages:
   - protect the reference counters from overflow/underflow
   - avoid use-after-free errors
   - provide improved memory ordering guarantee schemes
   - neater and safer.
Hence, replace the atomic_* APIs by their equivalent refcount_t
API functions.

This patch proposal address the following warnings generated by
the atomic_as_refcounter.cocci coccinelle script
atomic_add_return(-1, ...)

Signed-off-by: Deepak R Varma <drv@mailo.com>
---
Please Note:
   1. The patch is compile tested using dec_station.defconfig for MIPS architecture.
   2. This patch should be applied after patch 1/2 of this series due to
      dependency.

Changes in v4:
   1. Add the patch version label.

Changes in v3:
   1. Include the individual patches in a series and highlight dependency.
      Feedback provided by gregkh@linuxfoundation.org

Changes in v2:
   1. Separate the combined change into one variable per patch as
      suggested by gregkh@linuxfoundation.org


 drivers/tty/serial/dz.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/dz.c b/drivers/tty/serial/dz.c
index b70edc248f8b..0aa59a9beeb7 100644
--- a/drivers/tty/serial/dz.c
+++ b/drivers/tty/serial/dz.c
@@ -46,7 +46,6 @@
 #include <linux/tty.h>
 #include <linux/tty_flip.h>

-#include <linux/atomic.h>
 #include <linux/refcount.h>
 #include <linux/io.h>
 #include <asm/bootinfo.h>
@@ -77,7 +76,7 @@ struct dz_port {
 struct dz_mux {
 	struct dz_port		dport[DZ_NB_PORT];
 	refcount_t		map_guard;
-	atomic_t		irq_guard;
+	refcount_t		irq_guard;
 	int			initialised;
 };

@@ -400,18 +399,16 @@ static int dz_startup(struct uart_port *uport)
 	struct dz_port *dport = to_dport(uport);
 	struct dz_mux *mux = dport->mux;
 	unsigned long flags;
-	int irq_guard;
 	int ret;
 	u16 tmp;

-	irq_guard = atomic_add_return(1, &mux->irq_guard);
-	if (irq_guard != 1)
+	refcount_inc(&mux->irq_guard);
+	if (refcount_read(&mux->irq_guard) != 1)
 		return 0;

-	ret = request_irq(dport->port.irq, dz_interrupt,
-			  IRQF_SHARED, "dz", mux);
+	ret = request_irq(dport->port.irq, dz_interrupt, IRQF_SHARED, "dz", mux);
 	if (ret) {
-		atomic_add(-1, &mux->irq_guard);
+		refcount_dec(&mux->irq_guard);
 		printk(KERN_ERR "dz: Cannot get IRQ %d!\n", dport->port.irq);
 		return ret;
 	}
@@ -441,15 +438,13 @@ static void dz_shutdown(struct uart_port *uport)
 	struct dz_port *dport = to_dport(uport);
 	struct dz_mux *mux = dport->mux;
 	unsigned long flags;
-	int irq_guard;
 	u16 tmp;

 	spin_lock_irqsave(&dport->port.lock, flags);
 	dz_stop_tx(&dport->port);
 	spin_unlock_irqrestore(&dport->port.lock, flags);

-	irq_guard = atomic_add_return(-1, &mux->irq_guard);
-	if (!irq_guard) {
+	if (refcount_dec_and_test(&mux->irq_guard)) {
 		/* Disable interrupts.  */
 		tmp = dz_in(dport, DZ_CSR);
 		tmp &= ~(DZ_RIE | DZ_TIE);
--
2.34.1

Re: [PATCH v4 1/2] tty: serial: dz: convert atomic_* to refcount_* APIs for map_guard

[Jiri Slaby]

On 26. 12. 22, 7:21, Deepak R Varma wrote:
> The refcount_* APIs are designed to address known issues with the
> atomic_t APIs for reference counting. They provide following distinct
> advantages
>     - protect the reference counters from overflow/underflow
>     - avoid use-after-free errors
>     - provide improved memory ordering guarantee schemes
>     - neater and safer.

Really? (see below)

> --- a/drivers/tty/serial/dz.c
> +++ b/drivers/tty/serial/dz.c
...
> @@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
>   static int dz_request_port(struct uart_port *uport)
>   {
>   	struct dz_mux *mux = to_dport(uport)->mux;
> -	int map_guard;
>   	int ret;
> 
> -	map_guard = atomic_add_return(1, &mux->map_guard);
> -	if (map_guard == 1) {
> -		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> -					"dz")) {
> -			atomic_add(-1, &mux->map_guard);
> -			printk(KERN_ERR
> -			       "dz: Unable to reserve MMIO resource\n");
> +	refcount_inc(&mux->map_guard);
> +	if (refcount_read(&mux->map_guard) == 1) {

This is now racy, right?

thanks,
-- 
js
suse labs

Re: [PATCH v4 2/2] tty: serial: dz: convert atomic_* to refcount_* APIs for irq_guard

[Jiri Slaby]

On 26. 12. 22, 7:21, Deepak R Varma wrote:
> The refcount_* APIs are designed to address known issues with the
> atomic_t APIs for reference counting. They provide following distinct
> advantages:
>     - protect the reference counters from overflow/underflow
>     - avoid use-after-free errors
>     - provide improved memory ordering guarantee schemes
>     - neater and safer.
> Hence, replace the atomic_* APIs by their equivalent refcount_t
> API functions.
> 
> This patch proposal address the following warnings generated by
> the atomic_as_refcounter.cocci coccinelle script
> atomic_add_return(-1, ...)
...
> --- a/drivers/tty/serial/dz.c
> +++ b/drivers/tty/serial/dz.c
...
> @@ -400,18 +399,16 @@ static int dz_startup(struct uart_port *uport)
>   	struct dz_port *dport = to_dport(uport);
>   	struct dz_mux *mux = dport->mux;
>   	unsigned long flags;
> -	int irq_guard;
>   	int ret;
>   	u16 tmp;
> 
> -	irq_guard = atomic_add_return(1, &mux->irq_guard);
> -	if (irq_guard != 1)
> +	refcount_inc(&mux->irq_guard);
> +	if (refcount_read(&mux->irq_guard) != 1)
>   		return 0;
> 
> -	ret = request_irq(dport->port.irq, dz_interrupt,
> -			  IRQF_SHARED, "dz", mux);
> +	ret = request_irq(dport->port.irq, dz_interrupt, IRQF_SHARED, "dz", mux);

How is this related to the above described change?

-- 
js
suse labs

Re: [PATCH v4 1/2] tty: serial: dz: convert atomic_* to refcount_* APIs for map_guard

[Deepak R Varma]

On Tue, Jan 03, 2023 at 09:59:52AM +0100, Jiri Slaby wrote:
> On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > The refcount_* APIs are designed to address known issues with the
> > atomic_t APIs for reference counting. They provide following distinct
> > advantages
> >     - protect the reference counters from overflow/underflow
> >     - avoid use-after-free errors
> >     - provide improved memory ordering guarantee schemes
> >     - neater and safer.
>
> Really? (see below)
>
> > --- a/drivers/tty/serial/dz.c
> > +++ b/drivers/tty/serial/dz.c
> ...
> > @@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
> >   static int dz_request_port(struct uart_port *uport)
> >   {
> >   	struct dz_mux *mux = to_dport(uport)->mux;
> > -	int map_guard;
> >   	int ret;
> >
> > -	map_guard = atomic_add_return(1, &mux->map_guard);
> > -	if (map_guard == 1) {
> > -		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> > -					"dz")) {
> > -			atomic_add(-1, &mux->map_guard);
> > -			printk(KERN_ERR
> > -			       "dz: Unable to reserve MMIO resource\n");
> > +	refcount_inc(&mux->map_guard);
> > +	if (refcount_read(&mux->map_guard) == 1) {
>
> This is now racy, right?

Hello Jiri,
Thank you for the feedback. You are correct. I have split a single instruction
in two (or more?) instructions potentially resulting in race conditions. I
looked through the refcount_* APIs but did not find a direct match.


Can you please comment if the the following variation will avoid race condition?

	if (!refcount_add_not_zero(1, &mux->map_guard)) {
		refcount_inc(&mux->map_guard);
		...
	}

Here, refcount_add_not_zero would return false if &mux->map_guard is already 0.
Which means, incrementing it by 1 would have met the earlier if evaluation.
Whereas, if &mux->map_guard is something other than 0, refcount_add_not_zero
will increment it by 1 and return true, in which case the if condition will
fail, similar to the previous if evaluation.

Hope that helps clarify my revised thought. Can you please let me know if this
revision looks safe?

Thank you,
./drv



>
> thanks,
> --
> js
> suse labs
>

Re: [PATCH v4 2/2] tty: serial: dz: convert atomic_* to refcount_* APIs for irq_guard

[Deepak R Varma]

On Tue, Jan 03, 2023 at 10:00:48AM +0100, Jiri Slaby wrote:
> On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > The refcount_* APIs are designed to address known issues with the
> > atomic_t APIs for reference counting. They provide following distinct
> > advantages:
> >     - protect the reference counters from overflow/underflow
> >     - avoid use-after-free errors
> >     - provide improved memory ordering guarantee schemes
> >     - neater and safer.
> > Hence, replace the atomic_* APIs by their equivalent refcount_t
> > API functions.
> >
> > This patch proposal address the following warnings generated by
> > the atomic_as_refcounter.cocci coccinelle script
> > atomic_add_return(-1, ...)
> ...
> > --- a/drivers/tty/serial/dz.c
> > +++ b/drivers/tty/serial/dz.c
> ...
> > @@ -400,18 +399,16 @@ static int dz_startup(struct uart_port *uport)
> >   	struct dz_port *dport = to_dport(uport);
> >   	struct dz_mux *mux = dport->mux;
> >   	unsigned long flags;
> > -	int irq_guard;
> >   	int ret;
> >   	u16 tmp;
> >
> > -	irq_guard = atomic_add_return(1, &mux->irq_guard);
> > -	if (irq_guard != 1)
> > +	refcount_inc(&mux->irq_guard);
> > +	if (refcount_read(&mux->irq_guard) != 1)
> >   		return 0;
> >
> > -	ret = request_irq(dport->port.irq, dz_interrupt,
> > -			  IRQF_SHARED, "dz", mux);
> > +	ret = request_irq(dport->port.irq, dz_interrupt, IRQF_SHARED, "dz", mux);
>
> How is this related to the above described change?

No, it is not. My apologies. I must have joined the lines for improved readability
and forgot to revert. I will restore this in next revision based on the feedback
on the other patch of this series. OR I can include this change in the current
change log as a "while at it..." statement. Would you advise me?

Thank you,
./drv

>
> --
> js
> suse labs
>

Re: [PATCH v4 1/2] tty: serial: dz: convert atomic_* to refcount_* APIs for map_guard

[Greg Kroah-Hartman]

On Tue, Jan 03, 2023 at 03:35:15PM +0530, Deepak R Varma wrote:
> On Tue, Jan 03, 2023 at 09:59:52AM +0100, Jiri Slaby wrote:
> > On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > > The refcount_* APIs are designed to address known issues with the
> > > atomic_t APIs for reference counting. They provide following distinct
> > > advantages
> > >     - protect the reference counters from overflow/underflow
> > >     - avoid use-after-free errors
> > >     - provide improved memory ordering guarantee schemes
> > >     - neater and safer.
> >
> > Really? (see below)
> >
> > > --- a/drivers/tty/serial/dz.c
> > > +++ b/drivers/tty/serial/dz.c
> > ...
> > > @@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
> > >   static int dz_request_port(struct uart_port *uport)
> > >   {
> > >   	struct dz_mux *mux = to_dport(uport)->mux;
> > > -	int map_guard;
> > >   	int ret;
> > >
> > > -	map_guard = atomic_add_return(1, &mux->map_guard);
> > > -	if (map_guard == 1) {
> > > -		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> > > -					"dz")) {
> > > -			atomic_add(-1, &mux->map_guard);
> > > -			printk(KERN_ERR
> > > -			       "dz: Unable to reserve MMIO resource\n");
> > > +	refcount_inc(&mux->map_guard);
> > > +	if (refcount_read(&mux->map_guard) == 1) {
> >
> > This is now racy, right?
> 
> Hello Jiri,
> Thank you for the feedback. You are correct. I have split a single instruction
> in two (or more?) instructions potentially resulting in race conditions. I
> looked through the refcount_* APIs but did not find a direct match.
> 
> 
> Can you please comment if the the following variation will avoid race condition?
> 
> 	if (!refcount_add_not_zero(1, &mux->map_guard)) {
> 		refcount_inc(&mux->map_guard);
> 		...
> 	}

What do you think?  The onus is on you to prove the conversion is
correct, otherwise, why do the conversion at all?

Actually, why do this at all, what is the goal here?  And how was this
tested?

thanks,

greg k-h

Re: [PATCH v4 2/2] tty: serial: dz: convert atomic_* to refcount_* APIs for irq_guard

[Greg Kroah-Hartman]

On Tue, Jan 03, 2023 at 03:39:17PM +0530, Deepak R Varma wrote:
> On Tue, Jan 03, 2023 at 10:00:48AM +0100, Jiri Slaby wrote:
> > On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > > The refcount_* APIs are designed to address known issues with the
> > > atomic_t APIs for reference counting. They provide following distinct
> > > advantages:
> > >     - protect the reference counters from overflow/underflow
> > >     - avoid use-after-free errors
> > >     - provide improved memory ordering guarantee schemes
> > >     - neater and safer.
> > > Hence, replace the atomic_* APIs by their equivalent refcount_t
> > > API functions.
> > >
> > > This patch proposal address the following warnings generated by
> > > the atomic_as_refcounter.cocci coccinelle script
> > > atomic_add_return(-1, ...)
> > ...
> > > --- a/drivers/tty/serial/dz.c
> > > +++ b/drivers/tty/serial/dz.c
> > ...
> > > @@ -400,18 +399,16 @@ static int dz_startup(struct uart_port *uport)
> > >   	struct dz_port *dport = to_dport(uport);
> > >   	struct dz_mux *mux = dport->mux;
> > >   	unsigned long flags;
> > > -	int irq_guard;
> > >   	int ret;
> > >   	u16 tmp;
> > >
> > > -	irq_guard = atomic_add_return(1, &mux->irq_guard);
> > > -	if (irq_guard != 1)
> > > +	refcount_inc(&mux->irq_guard);
> > > +	if (refcount_read(&mux->irq_guard) != 1)
> > >   		return 0;
> > >
> > > -	ret = request_irq(dport->port.irq, dz_interrupt,
> > > -			  IRQF_SHARED, "dz", mux);
> > > +	ret = request_irq(dport->port.irq, dz_interrupt, IRQF_SHARED, "dz", mux);
> >
> > How is this related to the above described change?
> 
> No, it is not. My apologies. I must have joined the lines for improved readability
> and forgot to revert. I will restore this in next revision based on the feedback
> on the other patch of this series. OR I can include this change in the current
> change log as a "while at it..." statement. Would you advise me?

NEVER have a "while at it..." change as part of a commit unless it is
relevant to the main change being made.  You know better...

thanks,

greg k-h

Re: [PATCH v4 1/2] tty: serial: dz: convert atomic_* to refcount_* APIs for map_guard

[Deepak R Varma]

On Wed, Jan 04, 2023 at 09:28:13AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 03, 2023 at 03:35:15PM +0530, Deepak R Varma wrote:
> > > > -			printk(KERN_ERR
> > > > -			       "dz: Unable to reserve MMIO resource\n");
> > > > +	refcount_inc(&mux->map_guard);
> > > > +	if (refcount_read(&mux->map_guard) == 1) {
> > >
> > > This is now racy, right?
> >
> > Hello Jiri,
> > Thank you for the feedback. You are correct. I have split a single instruction
> > in two (or more?) instructions potentially resulting in race conditions. I
> > looked through the refcount_* APIs but did not find a direct match.
> >
> >
> > Can you please comment if the the following variation will avoid race condition?
> >
> > 	if (!refcount_add_not_zero(1, &mux->map_guard)) {
> > 		refcount_inc(&mux->map_guard);
> > 		...
> > 	}
>
> What do you think?  The onus is on you to prove the conversion is
> correct, otherwise, why do the conversion at all?

Hello Greg,
Okay. Sounds good. I think the revised approach should be safer. I will work on
finding a means to prove that.

>
> Actually, why do this at all, what is the goal here?  And how was this
> tested?

The objective here is to migrate to specific and improved APIs that are already
proved to be better for different reasons as mentioned in the patch log
messages. This is as per the Linux Kernel documentation.

In terms of testing, First, I did a compile and build test of the changes.
I also wrote separate small dummy modules and tested the API transformation.
However, these modules were standalone and limited in complexity and intensity.
I will try to make these more intense, multithreaded and run the test again.

Thank you as always :)
./drv

>
> thanks,
>
> greg k-h

Re: [PATCH v4 2/2] tty: serial: dz: convert atomic_* to refcount_* APIs for irq_guard

[Deepak R Varma]

On Wed, Jan 04, 2023 at 09:28:50AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 03, 2023 at 03:39:17PM +0530, Deepak R Varma wrote:
> > On Tue, Jan 03, 2023 at 10:00:48AM +0100, Jiri Slaby wrote:
> > > On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > > > +	ret = request_irq(dport->port.irq, dz_interrupt, IRQF_SHARED, "dz", mux);
> > >
> > > How is this related to the above described change?
> >
> > No, it is not. My apologies. I must have joined the lines for improved readability
> > and forgot to revert. I will restore this in next revision based on the feedback
> > on the other patch of this series. OR I can include this change in the current
> > change log as a "while at it..." statement. Would you advise me?
>
> NEVER have a "while at it..." change as part of a commit unless it is
> relevant to the main change being made.  You know better...

Sounds very good. Thank you for the advise. I will revert the change in the next
revision.

Thank you,
./drv

>
> thanks,
>
> greg k-h

Re: [PATCH v4 1/2] tty: serial: dz: convert atomic_* to refcount_* APIs for map_guard

[Deepak R Varma]

On Tue, Jan 03, 2023 at 09:59:52AM +0100, Jiri Slaby wrote:
> On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > The refcount_* APIs are designed to address known issues with the
> > atomic_t APIs for reference counting. They provide following distinct
> > advantages
> >     - protect the reference counters from overflow/underflow
> >     - avoid use-after-free errors
> >     - provide improved memory ordering guarantee schemes
> >     - neater and safer.
> 
> Really? (see below)
> 
> > --- a/drivers/tty/serial/dz.c
> > +++ b/drivers/tty/serial/dz.c
> ...
> > @@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
> >   static int dz_request_port(struct uart_port *uport)
> >   {
> >   	struct dz_mux *mux = to_dport(uport)->mux;
> > -	int map_guard;
> >   	int ret;
> > 
> > -	map_guard = atomic_add_return(1, &mux->map_guard);
> > -	if (map_guard == 1) {
> > -		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> > -					"dz")) {
> > -			atomic_add(-1, &mux->map_guard);
> > -			printk(KERN_ERR
> > -			       "dz: Unable to reserve MMIO resource\n");
> > +	refcount_inc(&mux->map_guard);
> > +	if (refcount_read(&mux->map_guard) == 1) {
> 
> This is now racy, right?

Hello Jiri,
I found this [1] commit which introduced similar transformation in a
neighbouring driver. Can you please comment how is this different from the
current patch proposal?

[1] commit ID: 22a33651a56f ("convert sbd_duart.map_guard from atomic_t to refcount_t")

On a side note, I have not been able to find an exact 1:1 map to the
atomic_add_result API. I am wondering should we have one?

Thank you,
./drv


Thank you,
./drv

> 
> thanks,
> -- 
> js
> suse labs
> 

RE: [PATCH v4 1/2] tty: serial: dz: convert atomic_* to refcount_* APIs for map_guard

[Reshetova, Elena]

 
> On Tue, Jan 03, 2023 at 09:59:52AM +0100, Jiri Slaby wrote:
> > On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > > The refcount_* APIs are designed to address known issues with the
> > > atomic_t APIs for reference counting. They provide following distinct
> > > advantages
> > >     - protect the reference counters from overflow/underflow
> > >     - avoid use-after-free errors
> > >     - provide improved memory ordering guarantee schemes
> > >     - neater and safer.
> >
> > Really? (see below)
> >
> > > --- a/drivers/tty/serial/dz.c
> > > +++ b/drivers/tty/serial/dz.c
> > ...
> > > @@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
> > >   static int dz_request_port(struct uart_port *uport)
> > >   {
> > >   	struct dz_mux *mux = to_dport(uport)->mux;
> > > -	int map_guard;
> > >   	int ret;
> > >
> > > -	map_guard = atomic_add_return(1, &mux->map_guard);
> > > -	if (map_guard == 1) {
> > > -		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> > > -					"dz")) {
> > > -			atomic_add(-1, &mux->map_guard);
> > > -			printk(KERN_ERR
> > > -			       "dz: Unable to reserve MMIO resource\n");
> > > +	refcount_inc(&mux->map_guard);
> > > +	if (refcount_read(&mux->map_guard) == 1) {
> >
> > This is now racy, right?
> 
> Hello Jiri,
> I found this [1] commit which introduced similar transformation in a
> neighbouring driver. Can you please comment how is this different from the
> current patch proposal?
> 
> [1] commit ID: 22a33651a56f ("convert sbd_duart.map_guard from atomic_t to
> refcount_t")
> 
> On a side note, I have not been able to find an exact 1:1 map to the
> atomic_add_result API. I am wondering should we have one?

In past we have decided not to provide this API for refcount_t
because for truly correctly behaving reference counters it should not be needed
(vs atomics that cover a broader range of use cases). 
Can you use !refcount_inc_not_zero in the above case?

Best Regards,
Elena.

Re: [PATCH v4 1/2] tty: serial: dz: convert atomic_* to refcount_* APIs for map_guard

[Deepak R Varma]

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

On Tue, Jan 10, 2023 at 07:27:44AM +0000, Reshetova, Elena wrote:
>  
> > On Tue, Jan 03, 2023 at 09:59:52AM +0100, Jiri Slaby wrote:
> > > On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > > > The refcount_* APIs are designed to address known issues with the
> > > > atomic_t APIs for reference counting. They provide following distinct
> > > > advantages
> > > >     - protect the reference counters from overflow/underflow
> > > >     - avoid use-after-free errors
> > > >     - provide improved memory ordering guarantee schemes
> > > >     - neater and safer.
> > >
> > > Really? (see below)
> > >
> > > > --- a/drivers/tty/serial/dz.c
> > > > +++ b/drivers/tty/serial/dz.c
> > > ...
> > > > @@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
> > > >   static int dz_request_port(struct uart_port *uport)
> > > >   {
> > > >   	struct dz_mux *mux = to_dport(uport)->mux;
> > > > -	int map_guard;
> > > >   	int ret;
> > > >
> > > > -	map_guard = atomic_add_return(1, &mux->map_guard);
> > > > -	if (map_guard == 1) {
> > > > -		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> > > > -					"dz")) {
> > > > -			atomic_add(-1, &mux->map_guard);
> > > > -			printk(KERN_ERR
> > > > -			       "dz: Unable to reserve MMIO resource\n");
> > > > +	refcount_inc(&mux->map_guard);
> > > > +	if (refcount_read(&mux->map_guard) == 1) {
> > >
> > > This is now racy, right?
> > 
> > Hello Jiri,
> > I found this [1] commit which introduced similar transformation in a
> > neighbouring driver. Can you please comment how is this different from the
> > current patch proposal?
> > 
> > [1] commit ID: 22a33651a56f ("convert sbd_duart.map_guard from atomic_t to
> > refcount_t")
> > 
> > On a side note, I have not been able to find an exact 1:1 map to the
> > atomic_add_result API. I am wondering should we have one?
> 

Hello Elena,

> In past we have decided not to provide this API for refcount_t
> because for truly correctly behaving reference counters it should not be needed
> (vs atomics that cover a broader range of use cases). 

So, there is no FAA refcount wrapper? I think this is a pretty common need.
Please correct me if I am wrong.

> Can you use !refcount_inc_not_zero in the above case?

I actually did try that but was not sure if truly addresses the objection.
Please attached and let me know if you have a feedback on the alternate
approach.

Thank you,
./drv


> 
> Best Regards,
> Elena.

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

############## ORIGINAL CODE ##################################
-       map_guard = atomic_add_return(1, &mux->map_guard);
-       if (map_guard == 1) {
-               if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
-                                       "dz")) {
-                       atomic_add(-1, &mux->map_guard);
-                       printk(KERN_ERR
-                              "dz: Unable to reserve MMIO resource\n");
                        return -EBUSY;
                }
        }

############## INITIAL APPROACH ##################################
+       refcount_inc(&mux->map_guard);
+       if (refcount_read(&mux->map_guard) == 1) {
+               if (!request_mem_region(uport->mapbase, dec_kn_slot_size, "dz")) {
+                       refcount_dec(&mux->map_guard);
+                       printk(KERN_ERR "dz: Unable to reserve MMIO resource\n");
                        return -EBUSY;
                }
        }

############## ALTERNATE APPROACH ##################################

+       if (!refcount_inc_not_zero(&mux->map_guard)) {
+               refcount_inc(&mux->map_guard);
+               if (!request_mem_region(uport->mapbase, dec_kn_slot_size, "dz")) {
+                       refcount_dec(&mux->map_guard);
+                       printk(KERN_ERR "dz: Unable to reserve MMIO resource\n");
                        return -EBUSY;
                }
        }

Re: [PATCH v4 1/2] tty: serial: dz: convert atomic_* to refcount_* APIs for map_guard

[Greg Kroah-Hartman]

On Tue, Jan 10, 2023 at 01:17:54PM +0530, Deepak R Varma wrote:
> On Tue, Jan 10, 2023 at 07:27:44AM +0000, Reshetova, Elena wrote:
> >  
> > > On Tue, Jan 03, 2023 at 09:59:52AM +0100, Jiri Slaby wrote:
> > > > On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > > > > The refcount_* APIs are designed to address known issues with the
> > > > > atomic_t APIs for reference counting. They provide following distinct
> > > > > advantages
> > > > >     - protect the reference counters from overflow/underflow
> > > > >     - avoid use-after-free errors
> > > > >     - provide improved memory ordering guarantee schemes
> > > > >     - neater and safer.
> > > >
> > > > Really? (see below)
> > > >
> > > > > --- a/drivers/tty/serial/dz.c
> > > > > +++ b/drivers/tty/serial/dz.c
> > > > ...
> > > > > @@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
> > > > >   static int dz_request_port(struct uart_port *uport)
> > > > >   {
> > > > >   	struct dz_mux *mux = to_dport(uport)->mux;
> > > > > -	int map_guard;
> > > > >   	int ret;
> > > > >
> > > > > -	map_guard = atomic_add_return(1, &mux->map_guard);
> > > > > -	if (map_guard == 1) {
> > > > > -		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> > > > > -					"dz")) {
> > > > > -			atomic_add(-1, &mux->map_guard);
> > > > > -			printk(KERN_ERR
> > > > > -			       "dz: Unable to reserve MMIO resource\n");
> > > > > +	refcount_inc(&mux->map_guard);
> > > > > +	if (refcount_read(&mux->map_guard) == 1) {
> > > >
> > > > This is now racy, right?
> > > 
> > > Hello Jiri,
> > > I found this [1] commit which introduced similar transformation in a
> > > neighbouring driver. Can you please comment how is this different from the
> > > current patch proposal?
> > > 
> > > [1] commit ID: 22a33651a56f ("convert sbd_duart.map_guard from atomic_t to
> > > refcount_t")
> > > 
> > > On a side note, I have not been able to find an exact 1:1 map to the
> > > atomic_add_result API. I am wondering should we have one?
> > 
> 
> Hello Elena,
> 
> > In past we have decided not to provide this API for refcount_t
> > because for truly correctly behaving reference counters it should not be needed
> > (vs atomics that cover a broader range of use cases). 
> 
> So, there is no FAA refcount wrapper? I think this is a pretty common need.
> Please correct me if I am wrong.
> 
> > Can you use !refcount_inc_not_zero in the above case?
> 
> I actually did try that but was not sure if truly addresses the objection.
> Please attached and let me know if you have a feedback on the alternate
> approach.
> 
> Thank you,
> ./drv
> 
> 
> > 
> > Best Regards,
> > Elena.

> ############## ORIGINAL CODE ##################################
> -       map_guard = atomic_add_return(1, &mux->map_guard);
> -       if (map_guard == 1) {
> -               if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> -                                       "dz")) {
> -                       atomic_add(-1, &mux->map_guard);
> -                       printk(KERN_ERR
> -                              "dz: Unable to reserve MMIO resource\n");
>                         return -EBUSY;
>                 }
>         }
> 
> ############## INITIAL APPROACH ##################################
> +       refcount_inc(&mux->map_guard);
> +       if (refcount_read(&mux->map_guard) == 1) {
> +               if (!request_mem_region(uport->mapbase, dec_kn_slot_size, "dz")) {
> +                       refcount_dec(&mux->map_guard);
> +                       printk(KERN_ERR "dz: Unable to reserve MMIO resource\n");
>                         return -EBUSY;
>                 }
>         }
> 
> ############## ALTERNATE APPROACH ##################################
> 
> +       if (!refcount_inc_not_zero(&mux->map_guard)) {
> +               refcount_inc(&mux->map_guard);
> +               if (!request_mem_region(uport->mapbase, dec_kn_slot_size, "dz")) {
> +                       refcount_dec(&mux->map_guard);
> +                       printk(KERN_ERR "dz: Unable to reserve MMIO resource\n");
>                         return -EBUSY;
>                 }
>         }
> 

This feels odd to me, why not just use a normal lock instead?

thanks,

greg k-h