| Message ID | 20240501084110.4165-2-shresthprasad7@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,next] tty: sunsu: Simplify device_node cleanup by using __free | expand |
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,
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
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
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) > >
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...
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
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
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
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)
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(-)