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