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@xxxxxxxxxxxxxx> 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
