| Message ID | 20220920070101.907596-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/x86: int3472/discrete: Drop a forward declaration | expand |
Hi Uwe On 20/09/2022 08:01, Uwe Kleine-König wrote: > By swapping the definition of skl_int3472_discrete_remove() and > skl_int3472_discrete_probe() the forward declaration of the former can > be dropped. This is a good thing as it removes code duplication. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Ah thanks - not sure why I ever did it that way now that you point it out :) Reviewed-by: Daniel Scally <djrscally@gmail.com> > --- > Hello, > > I didn't check in detail, but in my experience calling the remove > function in the error path of the probe function is prone to cleanup > errors. I didn't spot anything after a quick glance, but let me point > out this is unstable. E.g. in an error path of > skl_int3472_register_clock() the function is left with > int3472->clock.clk pointing to an unregistered clk and int3472->clock.cl > == NULL. Someone modifying the return function must be well aware of > that. > > Best regards > Uwe I take your point - I have to revisit this driver shortly anyway, so I'll try to remember to revise that away. Thanks > > drivers/platform/x86/intel/int3472/discrete.c | 34 +++++++++---------- > 1 file changed, 16 insertions(+), 18 deletions(-) > > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c > index ed4c9d760757..974a132db651 100644 > --- a/drivers/platform/x86/intel/int3472/discrete.c > +++ b/drivers/platform/x86/intel/int3472/discrete.c > @@ -331,7 +331,22 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472) > return 0; > } > > -static int skl_int3472_discrete_remove(struct platform_device *pdev); > +static int skl_int3472_discrete_remove(struct platform_device *pdev) > +{ > + struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev); > + > + gpiod_remove_lookup_table(&int3472->gpios); > + > + if (int3472->clock.cl) > + skl_int3472_unregister_clock(int3472); > + > + gpiod_put(int3472->clock.ena_gpio); > + gpiod_put(int3472->clock.led_gpio); > + > + skl_int3472_unregister_regulator(int3472); > + > + return 0; > +} > > static int skl_int3472_discrete_probe(struct platform_device *pdev) > { > @@ -383,23 +398,6 @@ static int skl_int3472_discrete_probe(struct platform_device *pdev) > return 0; > } > > -static int skl_int3472_discrete_remove(struct platform_device *pdev) > -{ > - struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev); > - > - gpiod_remove_lookup_table(&int3472->gpios); > - > - if (int3472->clock.cl) > - skl_int3472_unregister_clock(int3472); > - > - gpiod_put(int3472->clock.ena_gpio); > - gpiod_put(int3472->clock.led_gpio); > - > - skl_int3472_unregister_regulator(int3472); > - > - return 0; > -} > - > static const struct acpi_device_id int3472_device_id[] = { > { "INT3472", 0 }, > { } > > base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
Hi, On 9/20/22 09:01, Uwe Kleine-König wrote: > By swapping the definition of skl_int3472_discrete_remove() and > skl_int3472_discrete_probe() the forward declaration of the former can > be dropped. This is a good thing as it removes code duplication. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > Hello, > > I didn't check in detail, but in my experience calling the remove > function in the error path of the probe function is prone to cleanup > errors. I didn't spot anything after a quick glance, but let me point > out this is unstable. E.g. in an error path of > skl_int3472_register_clock() the function is left with > int3472->clock.clk pointing to an unregistered clk and int3472->clock.cl > == NULL. Someone modifying the return function must be well aware of > that. > > Best regards > Uwe > > drivers/platform/x86/intel/int3472/discrete.c | 34 +++++++++---------- > 1 file changed, 16 insertions(+), 18 deletions(-) > > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c > index ed4c9d760757..974a132db651 100644 > --- a/drivers/platform/x86/intel/int3472/discrete.c > +++ b/drivers/platform/x86/intel/int3472/discrete.c > @@ -331,7 +331,22 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472) > return 0; > } > > -static int skl_int3472_discrete_remove(struct platform_device *pdev); > +static int skl_int3472_discrete_remove(struct platform_device *pdev) > +{ > + struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev); > + > + gpiod_remove_lookup_table(&int3472->gpios); > + > + if (int3472->clock.cl) > + skl_int3472_unregister_clock(int3472); > + > + gpiod_put(int3472->clock.ena_gpio); > + gpiod_put(int3472->clock.led_gpio); > + > + skl_int3472_unregister_regulator(int3472); > + > + return 0; > +} > > static int skl_int3472_discrete_probe(struct platform_device *pdev) > { > @@ -383,23 +398,6 @@ static int skl_int3472_discrete_probe(struct platform_device *pdev) > return 0; > } > > -static int skl_int3472_discrete_remove(struct platform_device *pdev) > -{ > - struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev); > - > - gpiod_remove_lookup_table(&int3472->gpios); > - > - if (int3472->clock.cl) > - skl_int3472_unregister_clock(int3472); > - > - gpiod_put(int3472->clock.ena_gpio); > - gpiod_put(int3472->clock.led_gpio); > - > - skl_int3472_unregister_regulator(int3472); > - > - return 0; > -} > - > static const struct acpi_device_id int3472_device_id[] = { > { "INT3472", 0 }, > { } > > base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c index ed4c9d760757..974a132db651 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -331,7 +331,22 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472) return 0; } -static int skl_int3472_discrete_remove(struct platform_device *pdev); +static int skl_int3472_discrete_remove(struct platform_device *pdev) +{ + struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev); + + gpiod_remove_lookup_table(&int3472->gpios); + + if (int3472->clock.cl) + skl_int3472_unregister_clock(int3472); + + gpiod_put(int3472->clock.ena_gpio); + gpiod_put(int3472->clock.led_gpio); + + skl_int3472_unregister_regulator(int3472); + + return 0; +} static int skl_int3472_discrete_probe(struct platform_device *pdev) { @@ -383,23 +398,6 @@ static int skl_int3472_discrete_probe(struct platform_device *pdev) return 0; } -static int skl_int3472_discrete_remove(struct platform_device *pdev) -{ - struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev); - - gpiod_remove_lookup_table(&int3472->gpios); - - if (int3472->clock.cl) - skl_int3472_unregister_clock(int3472); - - gpiod_put(int3472->clock.ena_gpio); - gpiod_put(int3472->clock.led_gpio); - - skl_int3472_unregister_regulator(int3472); - - return 0; -} - static const struct acpi_device_id int3472_device_id[] = { { "INT3472", 0 }, { }
By swapping the definition of skl_int3472_discrete_remove() and skl_int3472_discrete_probe() the forward declaration of the former can be dropped. This is a good thing as it removes code duplication. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, I didn't check in detail, but in my experience calling the remove function in the error path of the probe function is prone to cleanup errors. I didn't spot anything after a quick glance, but let me point out this is unstable. E.g. in an error path of skl_int3472_register_clock() the function is left with int3472->clock.clk pointing to an unregistered clk and int3472->clock.cl == NULL. Someone modifying the return function must be well aware of that. Best regards Uwe drivers/platform/x86/intel/int3472/discrete.c | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868