[PATCH] platform/x86: int3472/discrete: Drop a forward declaration

[PATCH] platform/x86: int3472/discrete: Drop a forward declaration

[Uwe Kleine-König]

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(-)

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
-- 
2.37.2

Re: [PATCH] platform/x86: int3472/discrete: Drop a forward declaration

[Daniel Scally]

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

Re: [PATCH] platform/x86: int3472/discrete: Drop a forward declaration

[Hans de Goede]

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