| Message ID | 20240816150931.142208-2-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [1/4] cpuidle: psci: Simplify with scoped for each OF child loop | expand |
On Fri, 16 Aug 2024 17:09:29 +0200 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > Obtain the device node reference with scoped/cleanup.h to reduce error > handling and make the code a bit simpler. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> The original code looks suspect. See below. > --- > drivers/cpuidle/cpuidle-riscv-sbi.c | 21 +++++++-------------- > 1 file changed, 7 insertions(+), 14 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c > index a6e123dfe394..5bb3401220d2 100644 > --- a/drivers/cpuidle/cpuidle-riscv-sbi.c > +++ b/drivers/cpuidle/cpuidle-riscv-sbi.c > @@ -8,6 +8,7 @@ > > #define pr_fmt(fmt) "cpuidle-riscv-sbi: " fmt > > +#include <linux/cleanup.h> > #include <linux/cpuhotplug.h> > #include <linux/cpuidle.h> > #include <linux/cpumask.h> > @@ -236,19 +237,16 @@ static int sbi_cpuidle_dt_init_states(struct device *dev, > { > struct sbi_cpuidle_data *data = per_cpu_ptr(&sbi_cpuidle_data, cpu); > struct device_node *state_node; > - struct device_node *cpu_node; > u32 *states; > int i, ret; > > - cpu_node = of_cpu_device_node_get(cpu); > + struct device_node *cpu_node __free(device_node) = of_cpu_device_node_get(cpu); > if (!cpu_node) > return -ENODEV; > > states = devm_kcalloc(dev, state_count, sizeof(*states), GFP_KERNEL); > - if (!states) { > - ret = -ENOMEM; > - goto fail; > - } > + if (!states) > + return -ENOMEM; > > /* Parse SBI specific details from state DT nodes */ > for (i = 1; i < state_count; i++) { > @@ -264,10 +262,8 @@ static int sbi_cpuidle_dt_init_states(struct device *dev, > > pr_debug("sbi-state %#x index %d\n", states[i], i); > } > - if (i != state_count) { > - ret = -ENODEV; > - goto fail; > - } > + if (i != state_count) > + return -ENODEV; > > /* Initialize optional data, used for the hierarchical topology. */ > ret = sbi_dt_cpu_init_topology(drv, data, state_count, cpu); The handling of error ret from here doesn't free the node. Bug or something subtle I'm missing? If it's a bug, then fixes tag. > @@ -277,10 +273,7 @@ static int sbi_cpuidle_dt_init_states(struct device *dev, > /* Store states in the per-cpu struct. */ > data->states = states; > > -fail: > - of_node_put(cpu_node); > - > - return ret; > + return 0; > } > > static void sbi_cpuidle_deinit_cpu(int cpu)
On Mon, 19 Aug 2024 17:13:13 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Fri, 16 Aug 2024 17:09:29 +0200 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > Obtain the device node reference with scoped/cleanup.h to reduce error > > handling and make the code a bit simpler. > > > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > The original code looks suspect. See below. Whilst here... Why not do similar for state_node to avoid the delayed return check. Existing code { state_node = of_get_cpu_state_node(cpu_node, i - 1); if (!state_node) break; ret = sbi_dt_parse_state_node(state_node, &states[i]); of_node_put(state_node); if (ret) //another bug here on holding cpu_node btw. return ret; pr_debug("sbi-state %#x index %d\n", states[i], i); } //I think only path to this is is early break above. if (i != state_count) { ret = -ENODEV; goto fail; } Can be something like { struct device_node *state_node __free(device_node) = = of_get-cpu_State_nod(cpu_node, i - 1); if (!state_node) return -ENODEV; ret = sbi_dt_parse_state_node(state_node, &states[i]); if (ret) return ret; pr_debug("sbi-state %#x index %d\n", states[i], i); } > > > --- > > drivers/cpuidle/cpuidle-riscv-sbi.c | 21 +++++++-------------- > > 1 file changed, 7 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c > > index a6e123dfe394..5bb3401220d2 100644 > > --- a/drivers/cpuidle/cpuidle-riscv-sbi.c > > +++ b/drivers/cpuidle/cpuidle-riscv-sbi.c > > @@ -8,6 +8,7 @@ > > > > #define pr_fmt(fmt) "cpuidle-riscv-sbi: " fmt > > > > +#include <linux/cleanup.h> > > #include <linux/cpuhotplug.h> > > #include <linux/cpuidle.h> > > #include <linux/cpumask.h> > > @@ -236,19 +237,16 @@ static int sbi_cpuidle_dt_init_states(struct device *dev, > > { > > struct sbi_cpuidle_data *data = per_cpu_ptr(&sbi_cpuidle_data, cpu); > > struct device_node *state_node; > > - struct device_node *cpu_node; > > u32 *states; > > int i, ret; > > > > - cpu_node = of_cpu_device_node_get(cpu); > > + struct device_node *cpu_node __free(device_node) = of_cpu_device_node_get(cpu); > > if (!cpu_node) > > return -ENODEV; > > > > states = devm_kcalloc(dev, state_count, sizeof(*states), GFP_KERNEL); > > - if (!states) { > > - ret = -ENOMEM; > > - goto fail; > > - } > > + if (!states) > > + return -ENOMEM; > > > > /* Parse SBI specific details from state DT nodes */ > > for (i = 1; i < state_count; i++) { > > @@ -264,10 +262,8 @@ static int sbi_cpuidle_dt_init_states(struct device *dev, > > > > pr_debug("sbi-state %#x index %d\n", states[i], i); > > } > > - if (i != state_count) { > > - ret = -ENODEV; > > - goto fail; > > - } > > + if (i != state_count) > > + return -ENODEV; > > > > /* Initialize optional data, used for the hierarchical topology. */ > > ret = sbi_dt_cpu_init_topology(drv, data, state_count, cpu); > The handling of error ret from here doesn't free the node. > > Bug or something subtle I'm missing? > > If it's a bug, then fixes tag. > > > > @@ -277,10 +273,7 @@ static int sbi_cpuidle_dt_init_states(struct device *dev, > > /* Store states in the per-cpu struct. */ > > data->states = states; > > > > -fail: > > - of_node_put(cpu_node); > > - > > - return ret; > > + return 0; > > } > > > > static void sbi_cpuidle_deinit_cpu(int cpu) >
On 19/08/2024 18:13, Jonathan Cameron wrote: >> >> /* Parse SBI specific details from state DT nodes */ >> for (i = 1; i < state_count; i++) { >> @@ -264,10 +262,8 @@ static int sbi_cpuidle_dt_init_states(struct device *dev, >> >> pr_debug("sbi-state %#x index %d\n", states[i], i); >> } >> - if (i != state_count) { >> - ret = -ENODEV; >> - goto fail; >> - } >> + if (i != state_count) >> + return -ENODEV; >> >> /* Initialize optional data, used for the hierarchical topology. */ >> ret = sbi_dt_cpu_init_topology(drv, data, state_count, cpu); > The handling of error ret from here doesn't free the node. > > Bug or something subtle I'm missing? > > If it's a bug, then fixes tag. Yeah, indeed it is a fix. Best regards, Krzysztof
On 19/08/2024 18:19, Jonathan Cameron wrote: > On Mon, 19 Aug 2024 17:13:13 +0100 > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > >> On Fri, 16 Aug 2024 17:09:29 +0200 >> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >> >>> Obtain the device node reference with scoped/cleanup.h to reduce error >>> handling and make the code a bit simpler. >>> >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> The original code looks suspect. See below. > > Whilst here... Why not do similar for state_node to avoid > the delayed return check. > Existing code > { > state_node = of_get_cpu_state_node(cpu_node, i - 1); > if (!state_node) > break; I don't see how __free() helps here. You can return regardless of __free(). > > ret = sbi_dt_parse_state_node(state_node, &states[i]); > of_node_put(state_node); ... and this code is quite easy to read: you get reference and immediately release it. > > if (ret) > //another bug here on holding cpu_node btw. > return ret; > pr_debug("sbi-state %#x index %d\n", states[i], i); > } > //I think only path to this is is early break above. > if (i != state_count) { > ret = -ENODEV; > goto fail; > } > Can be something like > > { > struct device_node *state_node __free(device_node) = > = of_get-cpu_State_nod(cpu_node, i - 1); > > if (!state_node) > return -ENODEV; > > ret = sbi_dt_parse_state_node(state_node, &states[i]); > if (ret) > return ret; > > pr_debug("sbi-state %#x index %d\n", states[i], i); > } > Maybe I miss something, but I do not see how the __free() simplifies here anything. Best regards, Krzysztof
On Tue, 20 Aug 2024 11:36:32 +0200 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 19/08/2024 18:19, Jonathan Cameron wrote: > > On Mon, 19 Aug 2024 17:13:13 +0100 > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > > > >> On Fri, 16 Aug 2024 17:09:29 +0200 > >> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> > >>> Obtain the device node reference with scoped/cleanup.h to reduce error > >>> handling and make the code a bit simpler. > >>> > >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> The original code looks suspect. See below. > > > > Whilst here... Why not do similar for state_node to avoid > > the delayed return check. > > Existing code > > { > > state_node = of_get_cpu_state_node(cpu_node, i - 1); > > if (!state_node) > > break; > > I don't see how __free() helps here. You can return regardless of __free(). > > > > > ret = sbi_dt_parse_state_node(state_node, &states[i]); > > of_node_put(state_node); > > ... and this code is quite easy to read: you get reference and > immediately release it. > > > > > if (ret) > > //another bug here on holding cpu_node btw. > > return ret; > > pr_debug("sbi-state %#x index %d\n", states[i], i); > > } > > //I think only path to this is is early break above. > > if (i != state_count) { > > ret = -ENODEV; > > goto fail; > > } > > Can be something like > > > > { > > struct device_node *state_node __free(device_node) = > > = of_get-cpu_State_nod(cpu_node, i - 1); > > > > if (!state_node) > > return -ENODEV; > > > > ret = sbi_dt_parse_state_node(state_node, &states[i]); > > if (ret) > > return ret; > > > > pr_debug("sbi-state %#x index %d\n", states[i], i); > > } > > > > Maybe I miss something, but I do not see how the __free() simplifies > here anything. Personal preference. To my eyes, it does, but indeed not a huge advantage. Jonathan > > Best regards, > Krzysztof >
diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c index a6e123dfe394..5bb3401220d2 100644 --- a/drivers/cpuidle/cpuidle-riscv-sbi.c +++ b/drivers/cpuidle/cpuidle-riscv-sbi.c @@ -8,6 +8,7 @@ #define pr_fmt(fmt) "cpuidle-riscv-sbi: " fmt +#include <linux/cleanup.h> #include <linux/cpuhotplug.h> #include <linux/cpuidle.h> #include <linux/cpumask.h> @@ -236,19 +237,16 @@ static int sbi_cpuidle_dt_init_states(struct device *dev, { struct sbi_cpuidle_data *data = per_cpu_ptr(&sbi_cpuidle_data, cpu); struct device_node *state_node; - struct device_node *cpu_node; u32 *states; int i, ret; - cpu_node = of_cpu_device_node_get(cpu); + struct device_node *cpu_node __free(device_node) = of_cpu_device_node_get(cpu); if (!cpu_node) return -ENODEV; states = devm_kcalloc(dev, state_count, sizeof(*states), GFP_KERNEL); - if (!states) { - ret = -ENOMEM; - goto fail; - } + if (!states) + return -ENOMEM; /* Parse SBI specific details from state DT nodes */ for (i = 1; i < state_count; i++) { @@ -264,10 +262,8 @@ static int sbi_cpuidle_dt_init_states(struct device *dev, pr_debug("sbi-state %#x index %d\n", states[i], i); } - if (i != state_count) { - ret = -ENODEV; - goto fail; - } + if (i != state_count) + return -ENODEV; /* Initialize optional data, used for the hierarchical topology. */ ret = sbi_dt_cpu_init_topology(drv, data, state_count, cpu); @@ -277,10 +273,7 @@ static int sbi_cpuidle_dt_init_states(struct device *dev, /* Store states in the per-cpu struct. */ data->states = states; -fail: - of_node_put(cpu_node); - - return ret; + return 0; } static void sbi_cpuidle_deinit_cpu(int cpu)
Obtain the device node reference with scoped/cleanup.h to reduce error handling and make the code a bit simpler. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- drivers/cpuidle/cpuidle-riscv-sbi.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)