| Message ID | 20241030-soc-atmel-soc-cleanup-v1-2-32b9e0773b14@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drivers: soc: atmel: fix device_node release in atmel_soc_device_init() | expand |
On 30/10/2024 18:10, Javier Carrasco wrote: > Switch to a more robust approach to automatically release the node when > it goes out of scope, dropping the need for explicit calls to > of_node_put(). Please use subject prefixes matching the subsystem. You can get them for example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is touching. For bindings, the preferred subjects are explained here: https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters There is never a "drivers" prefix. Especially not first (because as middle appears for FEW subsystems, not for SoC though). > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > --- > drivers/soc/atmel/soc.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c > index 64b1ad063073..298b542dd1c0 100644 > --- a/drivers/soc/atmel/soc.c > +++ b/drivers/soc/atmel/soc.c > @@ -399,15 +399,12 @@ static const struct of_device_id at91_soc_allowed_list[] __initconst = { > > static int __init atmel_soc_device_init(void) > { > - struct device_node *np = of_find_node_by_path("/"); > + struct device_node *np __free(device_node) = of_find_node_by_path("/"); > > - if (!of_match_node(at91_soc_allowed_list, np)) { > - of_node_put(np); You just added this code. Don't add code which immediately you remove. Squash two patches. Best regards, Krzysztof
On 31/10/2024 12:07, Krzysztof Kozlowski wrote: > On 30/10/2024 18:10, Javier Carrasco wrote: >> Switch to a more robust approach to automatically release the node when >> it goes out of scope, dropping the need for explicit calls to >> of_node_put(). > > Please use subject prefixes matching the subsystem. You can get them for > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory > your patch is touching. For bindings, the preferred subjects are > explained here: > https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters > > There is never a "drivers" prefix. Especially not first (because as > middle appears for FEW subsystems, not for SoC though). > Thanks, I added that by mistake. I will fix that for v2. >> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> >> --- >> drivers/soc/atmel/soc.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c >> index 64b1ad063073..298b542dd1c0 100644 >> --- a/drivers/soc/atmel/soc.c >> +++ b/drivers/soc/atmel/soc.c >> @@ -399,15 +399,12 @@ static const struct of_device_id at91_soc_allowed_list[] __initconst = { >> >> static int __init atmel_soc_device_init(void) >> { >> - struct device_node *np = of_find_node_by_path("/"); >> + struct device_node *np __free(device_node) = of_find_node_by_path("/"); >> >> - if (!of_match_node(at91_soc_allowed_list, np)) { >> - of_node_put(np); > > You just added this code. Don't add code which immediately you remove. > Squash two patches. > > Best regards, > Krzysztof > As I said in another thread, I split the solution into a first one to be applied to stable kernels, and a second one that uses a more robust approach that is not supported by all stable kernels. Best regards, Javier Carrasco
On 31/10/2024 12:14, Javier Carrasco wrote: > On 31/10/2024 12:07, Krzysztof Kozlowski wrote: >> On 30/10/2024 18:10, Javier Carrasco wrote: >>> Switch to a more robust approach to automatically release the node when >>> it goes out of scope, dropping the need for explicit calls to >>> of_node_put(). >> >> Please use subject prefixes matching the subsystem. You can get them for >> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory >> your patch is touching. For bindings, the preferred subjects are >> explained here: >> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters >> >> There is never a "drivers" prefix. Especially not first (because as >> middle appears for FEW subsystems, not for SoC though). >> > > Thanks, I added that by mistake. I will fix that for v2. > >>> >>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> >>> --- >>> drivers/soc/atmel/soc.c | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c >>> index 64b1ad063073..298b542dd1c0 100644 >>> --- a/drivers/soc/atmel/soc.c >>> +++ b/drivers/soc/atmel/soc.c >>> @@ -399,15 +399,12 @@ static const struct of_device_id at91_soc_allowed_list[] __initconst = { >>> >>> static int __init atmel_soc_device_init(void) >>> { >>> - struct device_node *np = of_find_node_by_path("/"); >>> + struct device_node *np __free(device_node) = of_find_node_by_path("/"); >>> >>> - if (!of_match_node(at91_soc_allowed_list, np)) { >>> - of_node_put(np); >> >> You just added this code. Don't add code which immediately you remove. >> Squash two patches. >> >> Best regards, >> Krzysztof >> > > > As I said in another thread, I split the solution into a first one to be > applied to stable kernels, and a second one that uses a more robust > approach that is not supported by all stable kernels. > Same response as in other thread: 1. Then send backport for stable, if you care about stable. You inflate history with irrational commits just instead of doing proper stable backport. 2. Creating some commits purely for stable in the mainline kernel is not a correct approach. We work here on mainline and in mainline this is one logical change: fixing issue. Whether you fix issue with of_node_put or cleanup or by removing of_find_node_by_path() call, it does not matter. All of these are fixing the same, one issue. If you think about stable kernels, then work on backports, not inflate mainline kernel with multiple commits doing the same, creating artificial history. Best regards, Krzysztof
On 31/10/2024 12:07, Krzysztof Kozlowski wrote: > On 30/10/2024 18:10, Javier Carrasco wrote: >> Switch to a more robust approach to automatically release the node when >> it goes out of scope, dropping the need for explicit calls to >> of_node_put(). > > Please use subject prefixes matching the subsystem. You can get them for > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory > your patch is touching. For bindings, the preferred subjects are > explained here: > https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters > > There is never a "drivers" prefix. Especially not first (because as > middle appears for FEW subsystems, not for SoC though). > Interestingly, 10 out of 20 patches for this file use the "drivers" prefix first, so I guess it was an error that propagated for some time. I will stick to soc: atmel: for v2. > > Best regards, > Krzysztof >
diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c index 64b1ad063073..298b542dd1c0 100644 --- a/drivers/soc/atmel/soc.c +++ b/drivers/soc/atmel/soc.c @@ -399,15 +399,12 @@ static const struct of_device_id at91_soc_allowed_list[] __initconst = { static int __init atmel_soc_device_init(void) { - struct device_node *np = of_find_node_by_path("/"); + struct device_node *np __free(device_node) = of_find_node_by_path("/"); - if (!of_match_node(at91_soc_allowed_list, np)) { - of_node_put(np); + if (!of_match_node(at91_soc_allowed_list, np)) return 0; - } at91_soc_init(socs); - of_node_put(np); return 0; }
Switch to a more robust approach to automatically release the node when it goes out of scope, dropping the need for explicit calls to of_node_put(). Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- drivers/soc/atmel/soc.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)