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 <[email protected]>
Signed-off-by: Shresth Prasad <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Shresth Prasad <[email protected]>
> ---
> 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 <[email protected]>
thanks,
--
js
suse labs
On Thu, May 2, 2024 at 1:26 PM Jiri Slaby <[email protected]> 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 <[email protected]>
> > Signed-off-by: Shresth Prasad <[email protected]>
> > ---
> > 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 Thu, May 2, 2024 at 9:35 PM Ilpo Järvinen
<[email protected]> 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 <[email protected]>
> > Signed-off-by: Shresth Prasad <[email protected]>
> > ---
> > 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 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 <[email protected]>
> Signed-off-by: Shresth Prasad <[email protected]>
> ---
> 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
> <[email protected]> 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 <[email protected]> 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
> > <[email protected]> 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 <[email protected]> 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
> > > <[email protected]> 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