[PATCH v2][next] tty: sunsu: Simplify device_node cleanup by using __free

[PATCH v2][next] tty: sunsu: Simplify device_node cleanup by using __free

[Shresth Prasad]

Add `__free` function attribute to `ap` and `match` pointer
initialisations which ensure that the pointers are freed as soon as they
go out of scope, thus removing the need to manually free them using
`of_node_put`.

This also removes the need for the `goto` statement and the `rc`
variable.

Tested using a qemu x86_64 virtual machine.

Suggested-by: Julia Lawall <julia.lawall@inria.fr>
Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
---
Changes in v2:
    - Specify how the patch was tested

 drivers/tty/serial/sunsu.c | 37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/serial/sunsu.c b/drivers/tty/serial/sunsu.c
index 67a5fc70bb4b..0f463da5e7ce 100644
--- a/drivers/tty/serial/sunsu.c
+++ b/drivers/tty/serial/sunsu.c
@@ -1382,44 +1382,29 @@ static inline struct console *SUNSU_CONSOLE(void)
 
 static enum su_type su_get_type(struct device_node *dp)
 {
-	struct device_node *ap = of_find_node_by_path("/aliases");
-	enum su_type rc = SU_PORT_PORT;
+	struct device_node *ap __free(device_node) =
+			    of_find_node_by_path("/aliases");
 
 	if (ap) {
 		const char *keyb = of_get_property(ap, "keyboard", NULL);
 		const char *ms = of_get_property(ap, "mouse", NULL);
-		struct device_node *match;
 
 		if (keyb) {
-			match = of_find_node_by_path(keyb);
+			struct device_node *match __free(device_node) =
+					    of_find_node_by_path(keyb);
 
-			/*
-			 * The pointer is used as an identifier not
-			 * as a pointer, we can drop the refcount on
-			 * the of__node immediately after getting it.
-			 */
-			of_node_put(match);
-
-			if (dp == match) {
-				rc = SU_PORT_KBD;
-				goto out;
-			}
+			if (dp == match)
+				return SU_PORT_KBD;
 		}
 		if (ms) {
-			match = of_find_node_by_path(ms);
+			struct device_node *match __free(device_node) =
+					    of_find_node_by_path(ms);
 
-			of_node_put(match);
-
-			if (dp == match) {
-				rc = SU_PORT_MS;
-				goto out;
-			}
+			if (dp == match)
+				return SU_PORT_MS;
 		}
 	}
-
-out:
-	of_node_put(ap);
-	return rc;
+	return SU_PORT_PORT;
 }
 
 static int su_probe(struct platform_device *op)
-- 
2.44.0

Re: [PATCH v2][next] tty: sunsu: Simplify device_node cleanup by using __free

[Jiri Slaby]

On 01. 05. 24, 10:41, Shresth Prasad wrote:
> Add `__free` function attribute to `ap` and `match` pointer
> initialisations which ensure that the pointers are freed as soon as they
> go out of scope, thus removing the need to manually free them using
> `of_node_put`.
> 
> This also removes the need for the `goto` statement and the `rc`
> variable.
> 
> Tested using a qemu x86_64 virtual machine.
> 
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
> ---
> Changes in v2:
>      - Specify how the patch was tested
> 
>   drivers/tty/serial/sunsu.c | 37 +++++++++++--------------------------
>   1 file changed, 11 insertions(+), 26 deletions(-)

Nice cleanup.

> --- a/drivers/tty/serial/sunsu.c
> +++ b/drivers/tty/serial/sunsu.c
> @@ -1382,44 +1382,29 @@ static inline struct console *SUNSU_CONSOLE(void)
>   
>   static enum su_type su_get_type(struct device_node *dp)
>   {
> -	struct device_node *ap = of_find_node_by_path("/aliases");
> -	enum su_type rc = SU_PORT_PORT;
> +	struct device_node *ap __free(device_node) =
> +			    of_find_node_by_path("/aliases");

If we used c++, that would be even nicer; like:
Destroyer ap(of_find_node_by_path("/aliases"));

But we don't :P. OTOH. can't we achieve that with macro-fu and typeof() 
magic? Perhaps not really exactly the above, but something like
   Destroyer(ap, of_find_node_by_path("/aliases"));
maybe?

Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

thanks,
-- 
js
suse labs

Re: [PATCH v2][next] tty: sunsu: Simplify device_node cleanup by using __free

[Shresth Prasad]

On Thu, May 2, 2024 at 1:26 PM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 01. 05. 24, 10:41, Shresth Prasad wrote:
> > Add `__free` function attribute to `ap` and `match` pointer
> > initialisations which ensure that the pointers are freed as soon as they
> > go out of scope, thus removing the need to manually free them using
> > `of_node_put`.
> >
> > This also removes the need for the `goto` statement and the `rc`
> > variable.
> >
> > Tested using a qemu x86_64 virtual machine.
> >
> > Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> > Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
> > ---
> > Changes in v2:
> >      - Specify how the patch was tested
> >
> >   drivers/tty/serial/sunsu.c | 37 +++++++++++--------------------------
> >   1 file changed, 11 insertions(+), 26 deletions(-)
>
> Nice cleanup.

Thanks :)

>
> > --- a/drivers/tty/serial/sunsu.c
> > +++ b/drivers/tty/serial/sunsu.c
> > @@ -1382,44 +1382,29 @@ static inline struct console *SUNSU_CONSOLE(void)
> >
> >   static enum su_type su_get_type(struct device_node *dp)
> >   {
> > -     struct device_node *ap = of_find_node_by_path("/aliases");
> > -     enum su_type rc = SU_PORT_PORT;
> > +     struct device_node *ap __free(device_node) =
> > +                         of_find_node_by_path("/aliases");
>
> If we used c++, that would be even nicer; like:
> Destroyer ap(of_find_node_by_path("/aliases"));
>
> But we don't :P. OTOH. can't we achieve that with macro-fu and typeof()
> magic? Perhaps not really exactly the above, but something like
>    Destroyer(ap, of_find_node_by_path("/aliases"));
> maybe?

Hmm, a macro like that could probably be written but it's more convenient
to use the GCC attribute like in the current implementation, IMO.

Jonathan Corbert, who introduced this, wrote about it here:
https://lwn.net/Articles/934679/

Regards,
Shresth

Re: [PATCH v2][next] tty: sunsu: Simplify device_node cleanup by using __free

[Ilpo Järvinen]

On Wed, 1 May 2024, Shresth Prasad wrote:

> Add `__free` function attribute to `ap` and `match` pointer
> initialisations which ensure that the pointers are freed as soon as they
> go out of scope, thus removing the need to manually free them using
> `of_node_put`.
> 
> This also removes the need for the `goto` statement and the `rc`
> variable.
> 
> Tested using a qemu x86_64 virtual machine.

Eh, how can you test this with an x86_64 VM ???

config SERIAL_SUNSU
        tristate "Sun SU serial support"
        depends on SPARC && PCI

-- 
 i.


> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
> ---
> Changes in v2:
>     - Specify how the patch was tested
> 
>  drivers/tty/serial/sunsu.c | 37 +++++++++++--------------------------
>  1 file changed, 11 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/tty/serial/sunsu.c b/drivers/tty/serial/sunsu.c
> index 67a5fc70bb4b..0f463da5e7ce 100644
> --- a/drivers/tty/serial/sunsu.c
> +++ b/drivers/tty/serial/sunsu.c
> @@ -1382,44 +1382,29 @@ static inline struct console *SUNSU_CONSOLE(void)
>  
>  static enum su_type su_get_type(struct device_node *dp)
>  {
> -	struct device_node *ap = of_find_node_by_path("/aliases");
> -	enum su_type rc = SU_PORT_PORT;
> +	struct device_node *ap __free(device_node) =
> +			    of_find_node_by_path("/aliases");
>  
>  	if (ap) {
>  		const char *keyb = of_get_property(ap, "keyboard", NULL);
>  		const char *ms = of_get_property(ap, "mouse", NULL);
> -		struct device_node *match;
>  
>  		if (keyb) {
> -			match = of_find_node_by_path(keyb);
> +			struct device_node *match __free(device_node) =
> +					    of_find_node_by_path(keyb);
>  
> -			/*
> -			 * The pointer is used as an identifier not
> -			 * as a pointer, we can drop the refcount on
> -			 * the of__node immediately after getting it.
> -			 */
> -			of_node_put(match);
> -
> -			if (dp == match) {
> -				rc = SU_PORT_KBD;
> -				goto out;
> -			}
> +			if (dp == match)
> +				return SU_PORT_KBD;
>  		}
>  		if (ms) {
> -			match = of_find_node_by_path(ms);
> +			struct device_node *match __free(device_node) =
> +					    of_find_node_by_path(ms);
>  
> -			of_node_put(match);
> -
> -			if (dp == match) {
> -				rc = SU_PORT_MS;
> -				goto out;
> -			}
> +			if (dp == match)
> +				return SU_PORT_MS;
>  		}
>  	}
> -
> -out:
> -	of_node_put(ap);
> -	return rc;
> +	return SU_PORT_PORT;
>  }
>  
>  static int su_probe(struct platform_device *op)
> 

Re: [PATCH v2][next] tty: sunsu: Simplify device_node cleanup by using __free

[Shresth Prasad]

On Thu, May 2, 2024 at 9:35 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Wed, 1 May 2024, Shresth Prasad wrote:
>
> > Add `__free` function attribute to `ap` and `match` pointer
> > initialisations which ensure that the pointers are freed as soon as they
> > go out of scope, thus removing the need to manually free them using
> > `of_node_put`.
> >
> > This also removes the need for the `goto` statement and the `rc`
> > variable.
> >
> > Tested using a qemu x86_64 virtual machine.
>
> Eh, how can you test this with an x86_64 VM ???
>
> config SERIAL_SUNSU
>         tristate "Sun SU serial support"
>         depends on SPARC && PCI
>

By that, I mean that I compiled the kernel and ran the produced bzImage
on a x86_64 qemu machine.
I unfortunately don't have the hardware to test it on, but I don't
think the change is complex enough to require testing on real hardware
(unless I'm assuming incorrectly).

Regards,
Shresth

> > Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> > Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
> > ---
> > Changes in v2:
> >     - Specify how the patch was tested
> >
> >  drivers/tty/serial/sunsu.c | 37 +++++++++++--------------------------
> >  1 file changed, 11 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/tty/serial/sunsu.c b/drivers/tty/serial/sunsu.c
> > index 67a5fc70bb4b..0f463da5e7ce 100644
> > --- a/drivers/tty/serial/sunsu.c
> > +++ b/drivers/tty/serial/sunsu.c
> > @@ -1382,44 +1382,29 @@ static inline struct console *SUNSU_CONSOLE(void)
> >
> >  static enum su_type su_get_type(struct device_node *dp)
> >  {
> > -     struct device_node *ap = of_find_node_by_path("/aliases");
> > -     enum su_type rc = SU_PORT_PORT;
> > +     struct device_node *ap __free(device_node) =
> > +                         of_find_node_by_path("/aliases");
> >
> >       if (ap) {
> >               const char *keyb = of_get_property(ap, "keyboard", NULL);
> >               const char *ms = of_get_property(ap, "mouse", NULL);
> > -             struct device_node *match;
> >
> >               if (keyb) {
> > -                     match = of_find_node_by_path(keyb);
> > +                     struct device_node *match __free(device_node) =
> > +                                         of_find_node_by_path(keyb);
> >
> > -                     /*
> > -                      * The pointer is used as an identifier not
> > -                      * as a pointer, we can drop the refcount on
> > -                      * the of__node immediately after getting it.
> > -                      */
> > -                     of_node_put(match);
> > -
> > -                     if (dp == match) {
> > -                             rc = SU_PORT_KBD;
> > -                             goto out;
> > -                     }
> > +                     if (dp == match)
> > +                             return SU_PORT_KBD;
> >               }
> >               if (ms) {
> > -                     match = of_find_node_by_path(ms);
> > +                     struct device_node *match __free(device_node) =
> > +                                         of_find_node_by_path(ms);
> >
> > -                     of_node_put(match);
> > -
> > -                     if (dp == match) {
> > -                             rc = SU_PORT_MS;
> > -                             goto out;
> > -                     }
> > +                     if (dp == match)
> > +                             return SU_PORT_MS;
> >               }
> >       }
> > -
> > -out:
> > -     of_node_put(ap);
> > -     return rc;
> > +     return SU_PORT_PORT;
> >  }
> >
> >  static int su_probe(struct platform_device *op)
> >

Re: [PATCH v2][next] tty: sunsu: Simplify device_node cleanup by using __free

[Greg KH]

On Thu, May 02, 2024 at 10:21:16PM +0530, Shresth Prasad wrote:
> On Thu, May 2, 2024 at 9:35 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Wed, 1 May 2024, Shresth Prasad wrote:
> >
> > > Add `__free` function attribute to `ap` and `match` pointer
> > > initialisations which ensure that the pointers are freed as soon as they
> > > go out of scope, thus removing the need to manually free them using
> > > `of_node_put`.
> > >
> > > This also removes the need for the `goto` statement and the `rc`
> > > variable.
> > >
> > > Tested using a qemu x86_64 virtual machine.
> >
> > Eh, how can you test this with an x86_64 VM ???
> >
> > config SERIAL_SUNSU
> >         tristate "Sun SU serial support"
> >         depends on SPARC && PCI
> >
> 
> By that, I mean that I compiled the kernel and ran the produced bzImage
> on a x86_64 qemu machine.

But you didn't include the driver you were testing :(

> I unfortunately don't have the hardware to test it on, but I don't
> think the change is complex enough to require testing on real hardware
> (unless I'm assuming incorrectly).

That's why I asked if you had tested this or not...

Re: [PATCH v2][next] tty: sunsu: Simplify device_node cleanup by using __free

[Shresth Prasad]

On Fri, May 3, 2024 at 11:04 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, May 02, 2024 at 10:21:16PM +0530, Shresth Prasad wrote:
> > On Thu, May 2, 2024 at 9:35 PM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > >
> > > On Wed, 1 May 2024, Shresth Prasad wrote:
> > >
> > > > Add `__free` function attribute to `ap` and `match` pointer
> > > > initialisations which ensure that the pointers are freed as soon as they
> > > > go out of scope, thus removing the need to manually free them using
> > > > `of_node_put`.
> > > >
> > > > This also removes the need for the `goto` statement and the `rc`
> > > > variable.
> > > >
> > > > Tested using a qemu x86_64 virtual machine.
> > >
> > > Eh, how can you test this with an x86_64 VM ???
> > >
> > > config SERIAL_SUNSU
> > >         tristate "Sun SU serial support"
> > >         depends on SPARC && PCI
> > >
> >
> > By that, I mean that I compiled the kernel and ran the produced bzImage
> > on a x86_64 qemu machine.
>
> But you didn't include the driver you were testing :(
>
> > I unfortunately don't have the hardware to test it on, but I don't
> > think the change is complex enough to require testing on real hardware
> > (unless I'm assuming incorrectly).
>
> That's why I asked if you had tested this or not...
>

Really sorry about that, I thought compiling and booting would qualify
as testing. What should I be doing then?

Regards,
Shresth

Re: [PATCH v2][next] tty: sunsu: Simplify device_node cleanup by using __free

[Greg KH]

On Fri, May 03, 2024 at 02:31:22PM +0530, Shresth Prasad wrote:
> On Fri, May 3, 2024 at 11:04 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, May 02, 2024 at 10:21:16PM +0530, Shresth Prasad wrote:
> > > On Thu, May 2, 2024 at 9:35 PM Ilpo Järvinen
> > > <ilpo.jarvinen@linux.intel.com> wrote:
> > > >
> > > > On Wed, 1 May 2024, Shresth Prasad wrote:
> > > >
> > > > > Add `__free` function attribute to `ap` and `match` pointer
> > > > > initialisations which ensure that the pointers are freed as soon as they
> > > > > go out of scope, thus removing the need to manually free them using
> > > > > `of_node_put`.
> > > > >
> > > > > This also removes the need for the `goto` statement and the `rc`
> > > > > variable.
> > > > >
> > > > > Tested using a qemu x86_64 virtual machine.
> > > >
> > > > Eh, how can you test this with an x86_64 VM ???
> > > >
> > > > config SERIAL_SUNSU
> > > >         tristate "Sun SU serial support"
> > > >         depends on SPARC && PCI
> > > >
> > >
> > > By that, I mean that I compiled the kernel and ran the produced bzImage
> > > on a x86_64 qemu machine.
> >
> > But you didn't include the driver you were testing :(
> >
> > > I unfortunately don't have the hardware to test it on, but I don't
> > > think the change is complex enough to require testing on real hardware
> > > (unless I'm assuming incorrectly).
> >
> > That's why I asked if you had tested this or not...
> >
> 
> Really sorry about that, I thought compiling and booting would qualify
> as testing. What should I be doing then?

Compiling and booting the code you change would be a good start :)

thanks,

greg k-h

Re: [PATCH v2][next] tty: sunsu: Simplify device_node cleanup by using __free

[Shresth Prasad]

On Sat, May 4, 2024 at 9:32 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, May 03, 2024 at 02:31:22PM +0530, Shresth Prasad wrote:
> > On Fri, May 3, 2024 at 11:04 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, May 02, 2024 at 10:21:16PM +0530, Shresth Prasad wrote:
> > > > On Thu, May 2, 2024 at 9:35 PM Ilpo Järvinen
> > > > <ilpo.jarvinen@linux.intel.com> wrote:
> > > > >
> > > > > On Wed, 1 May 2024, Shresth Prasad wrote:
> > > > >
> > > > > > Add `__free` function attribute to `ap` and `match` pointer
> > > > > > initialisations which ensure that the pointers are freed as soon as they
> > > > > > go out of scope, thus removing the need to manually free them using
> > > > > > `of_node_put`.
> > > > > >
> > > > > > This also removes the need for the `goto` statement and the `rc`
> > > > > > variable.
> > > > > >
> > > > > > Tested using a qemu x86_64 virtual machine.
> > > > >
> > > > > Eh, how can you test this with an x86_64 VM ???
> > > > >
> > > > > config SERIAL_SUNSU
> > > > >         tristate "Sun SU serial support"
> > > > >         depends on SPARC && PCI
> > > > >
> > > >
> > > > By that, I mean that I compiled the kernel and ran the produced bzImage
> > > > on a x86_64 qemu machine.
> > >
> > > But you didn't include the driver you were testing :(
> > >
> > > > I unfortunately don't have the hardware to test it on, but I don't
> > > > think the change is complex enough to require testing on real hardware
> > > > (unless I'm assuming incorrectly).
> > >
> > > That's why I asked if you had tested this or not...
> > >
> >
> > Really sorry about that, I thought compiling and booting would qualify
> > as testing. What should I be doing then?
>
> Compiling and booting the code you change would be a good start :)
>
> thanks,
>
> greg k-h

I've managed to successfully cross compile the kernel for sparc, along
with this module, but I couldn't figure out how to boot the generated
kernel image.

I looked around for quite a while but couldn't find any straightforward
guide suggesting how I would go about booting my own kernel on
qemu-system-sparc.

I would really appreciate it if you could point me in the right direction.

Regards,
Shresth