tty: serial: dz: convert atomic_* to refcount_*

PATCH In review Deepak R Varma 4 participants 15 messages Dec 24, 2022 – Jan 10, 2023

Subsystems (2)

TTY layer and serial drivers Dz decstation DZ11 serial driver

lore

Share & tools

lore

Deepak R Varma

author

2022-12-24 16:34 UTC #1 lore raw

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

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 <[email protected]>

---
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 [email protected]

Changes in v2:
1. Separate the combined change into one variable per patch as suggested by [email protected]
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(-)

drivers/tty/serial/dz.c +9 ~~-14~~

index 6b7ed7f2f3ca..b70edc248f8b 100644
@@ -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

Jiri Slaby

maintainer

2023-01-03 09:00 UTC #2 lore raw

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

Quoted 7 lines · 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.

Really? (see below)

Quoted 2 lines

drivers/tty/serial/dz.c +0 -0

...

Quoted 6 lines

drivers/tty/serial/dz.c +0 -1

@@ -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;

drivers/tty/serial/dz.c static int dz_map_port(struct uart_port *uport) · line 692

Quoted 10 lines

drivers/tty/serial/dz.c +2 -7

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

Signature · 2 lines

js
suse labs

Deepak R Varma

author

2023-01-03 10:05 UTC #3 lore raw

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

Quoted 21 lines · Jiri Slaby

Quoted 7 lines · 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.
Really? (see below)
Quoted 2 lines
drivers/tty/serial/dz.c +0 -0
...
Quoted 7 lines
drivers/tty/serial/dz.c +0 -1
@@ -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;
 

Quoted 11 lines

drivers/tty/serial/dz.c static int dz_map_port(struct uart_port *uport) · line 692

Quoted 9 lines

drivers/tty/serial/dz.c +2 -7

-	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

Quoted 6 lines

thanks,
--
js
suse labs

Greg Kroah-Hartman

maintainer

2023-01-04 08:28 UTC #4 lore raw

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

Quoted 40 lines · Deepak R Varma

Quoted 21 lines · Jiri Slaby
Quoted 7 lines · 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.
Really? (see below)
Quoted 2 lines
drivers/tty/serial/dz.c +0 -0
...
Quoted 7 lines
drivers/tty/serial/dz.c +0 -1
@@ -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;
 
Quoted 11 lines
drivers/tty/serial/dz.c static int dz_map_port(struct uart_port *uport) · line 692
Quoted 9 lines
drivers/tty/serial/dz.c +2 -7
-	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.

Quoted 6 lines

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

Deepak R Varma

author

2023-01-04 09:00 UTC #5 lore raw

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

Quoted 16 lines · Greg Kroah-Hartman

Quoted 15 lines · Deepak R Varma
Quoted 6 lines
drivers/tty/serial/dz.c static int dz_map_port(struct uart_port *uport) · line 692
Quoted 4 lines
drivers/tty/serial/dz.c +2 -2
-			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?

Quoted 7 lines

Quoted 4 lines
	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.

Quoted 3 lines

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

Quoted 4 lines

thanks,
greg k-h

Deepak R Varma

author

2023-01-10 06:19 UTC #6 lore raw

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

In reply to Jiri Slaby

Quoted 21 lines · Jiri Slaby

Quoted 7 lines · 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.
Really? (see below)
Quoted 2 lines
drivers/tty/serial/dz.c +0 -0
...
Quoted 7 lines
drivers/tty/serial/dz.c +0 -1
@@ -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;
 

Quoted 11 lines

drivers/tty/serial/dz.c static int dz_map_port(struct uart_port *uport) · line 692

Quoted 9 lines

drivers/tty/serial/dz.c +2 -7

-	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

Quoted 6 lines

thanks,
Signature · 2 lines
js
suse labs

Reshetova, Elena

2023-01-10 07:30 UTC #7 lore raw

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

Quoted 40 lines · Jiri Slaby

Quoted 21 lines · Deepak R Varma
Quoted 7 lines
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)
Quoted 2 lines
drivers/tty/serial/dz.c +0 -0
...
Quoted 7 lines
drivers/tty/serial/dz.c +0 -1
@@ -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;
 
Quoted 11 lines
drivers/tty/serial/dz.c static int dz_map_port(struct uart_port *uport) · line 692
Quoted 9 lines
drivers/tty/serial/dz.c +2 -7
-	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?

Quoted 5 lines

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

Deepak R Varma

author

2023-01-10 07:48 UTC #8 lore raw

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

Quoted 40 lines · Elena

Quoted 40 lines · Jiri Slaby
Quoted 21 lines · Deepak R Varma
Quoted 7 lines
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)
Quoted 2 lines
drivers/tty/serial/dz.c +0 -0
...
Quoted 7 lines
drivers/tty/serial/dz.c +0 -1
@@ -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;
 
Quoted 11 lines
drivers/tty/serial/dz.c static int dz_map_port(struct uart_port *uport) · line 692
Quoted 9 lines
drivers/tty/serial/dz.c +2 -7
-	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?

Quoted 6 lines

Quoted 5 lines
[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,

Quoted 3 lines

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.

Quoted 1 line

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

Quoted 3 lines

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;
                }
        }

Greg Kroah-Hartman

maintainer

2023-01-10 07:59 UTC #9 lore raw

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

Quoted 96 lines · Deepak R Varma

Quoted 40 lines · Elena
Quoted 40 lines · Jiri Slaby
Quoted 21 lines · Deepak R Varma
Quoted 7 lines
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)
Quoted 2 lines
drivers/tty/serial/dz.c +0 -0
...
Quoted 7 lines
drivers/tty/serial/dz.c +0 -1
@@ -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;
 
Quoted 11 lines
drivers/tty/serial/dz.c static int dz_map_port(struct uart_port *uport) · line 692
Quoted 9 lines
drivers/tty/serial/dz.c +2 -7
-	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?
Quoted 6 lines
Quoted 5 lines
[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,
Quoted 3 lines
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.
Quoted 1 line
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
Quoted 3 lines
Best Regards,
Elena.
############## ORIGINAL CODE ##################################
drivers/tty/serial/dz.c +0 -7
-       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 ##################################
drivers/tty/serial/dz.c +5 -0
+       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 ##################################

drivers/tty/serial/dz.c static int dz_map_port(struct uart_port *uport) · line 698

Quoted 8 lines

drivers/tty/serial/dz.c +5 -0

+       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

Deepak R Varma

author

2022-12-24 16:34 UTC #10 lore raw

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

In reply to 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 <[email protected]>

---
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 [email protected]

Changes in v2:
1. Separate the combined change into one variable per patch as suggested by [email protected]

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

drivers/tty/serial/dz.c +6 ~~-11~~

index b70edc248f8b..0aa59a9beeb7 100644
@@ -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

Jiri Slaby

maintainer

2023-01-03 09:00 UTC #11 lore raw

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

Quoted 13 lines · 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, ...)

...

Quoted 2 lines

drivers/tty/serial/dz.c +0 -0

...

Quoted 7 lines

drivers/tty/serial/dz.c +0 -1

@@ -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;

drivers/tty/serial/dz.c static int dz_startup(struct uart_port *uport) · line 409

Quoted 10 lines

drivers/tty/serial/dz.c +3 -4

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

Signature · 2 lines

js
suse labs

Deepak R Varma

author

2023-01-03 10:09 UTC #12 lore raw

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

Quoted 32 lines · Jiri Slaby

Quoted 13 lines · 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, ...)
...
Quoted 2 lines
drivers/tty/serial/dz.c +0 -0
...
Quoted 14 lines
drivers/tty/serial/dz.c +2 -3
@@ -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;
 

Quoted 5 lines

drivers/tty/serial/dz.c static int dz_startup(struct uart_port *uport) · line 409
Quoted 3 lines
drivers/tty/serial/dz.c +1 -2
-	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

Quoted 5 lines

--
js
suse labs

Greg Kroah-Hartman

maintainer

2023-01-04 08:29 UTC #13 lore raw

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

Quoted 39 lines · Deepak R Varma

Quoted 32 lines · Jiri Slaby
Quoted 13 lines · 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, ...)
...
Quoted 2 lines
drivers/tty/serial/dz.c +0 -0
...
Quoted 14 lines
drivers/tty/serial/dz.c +2 -3
@@ -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;
 
Quoted 5 lines
drivers/tty/serial/dz.c static int dz_startup(struct uart_port *uport) · line 409
Quoted 3 lines
drivers/tty/serial/dz.c +1 -2
-	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?

Quoted 4 lines

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

Deepak R Varma

author

2023-01-04 09:03 UTC #14 lore raw

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

Quoted 7 lines · Greg Kroah-Hartman

Quoted 6 lines · Deepak R Varma
Quoted 4 lines · Jiri Slaby
drivers/tty/serial/dz.c static int dz_startup(struct uart_port *uport) · line 409
Quoted 1 line · Deepak R Varma
drivers/tty/serial/dz.c +1 -0
+	ret = request_irq(dport->port.irq, dz_interrupt, IRQF_SHARED, "dz", mux);
How is this related to the above described change?

Quoted 7 lines

Quoted 4 lines
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

Quoted 4 lines

thanks,
greg k-h