1. Patchew
  2. linux
  3. [PATCH v2 00/10] soc: sunxi: sram: Fixes and D1 support
  4. View patch

[PATCH v2 05/10] soc: sunxi: sram: Fix probe function ordering issues

Samuel Holland posted 10 patches 3 years, 7 months ago
[PATCH v2 05/10] soc: sunxi: sram: Fix probe function ordering issues
Posted by Samuel Holland 3 years, 7 months ago 
Errors from debugfs are intended to be non-fatal, and should not prevent
the driver from probing.

Since debugfs file creation is treated as infallible, move it below the
parts of the probe function that can fail. This prevents an error
elsewhere in the probe function from causing the file to leak. Do the
same for the call to of_platform_populate().

Finally, checkpatch suggests an octal literal for the file permissions.

Fixes: 4af34b572a85 ("drivers: soc: sunxi: Introduce SoC driver to map SRAMs")
Fixes: 5828729bebbb ("soc: sunxi: export a regmap for EMAC clock reg on A64")
Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

(no changes since v1)

 drivers/soc/sunxi/sunxi_sram.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/soc/sunxi/sunxi_sram.c b/drivers/soc/sunxi/sunxi_sram.c
index a858a37fcdd4..52d07bed7664 100644
--- a/drivers/soc/sunxi/sunxi_sram.c
+++ b/drivers/soc/sunxi/sunxi_sram.c
@@ -332,9 +332,9 @@ static struct regmap_config sunxi_sram_emac_clock_regmap = {
 
 static int __init sunxi_sram_probe(struct platform_device *pdev)
 {
-	struct dentry *d;
 	struct regmap *emac_clock;
 	const struct sunxi_sramc_variant *variant;
+	struct device *dev = &pdev->dev;
 
 	sram_dev = &pdev->dev;
 
@@ -346,13 +346,6 @@ static int __init sunxi_sram_probe(struct platform_device *pdev)
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
-	of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
-
-	d = debugfs_create_file("sram", S_IRUGO, NULL, NULL,
-				&sunxi_sram_fops);
-	if (!d)
-		return -ENOMEM;
-
 	if (variant->num_emac_clocks > 0) {
 		emac_clock = devm_regmap_init_mmio(&pdev->dev, base,
 						   &sunxi_sram_emac_clock_regmap);
@@ -361,6 +354,10 @@ static int __init sunxi_sram_probe(struct platform_device *pdev)
 			return PTR_ERR(emac_clock);
 	}
 
+	of_platform_populate(dev->of_node, NULL, NULL, dev);
+
+	debugfs_create_file("sram", 0444, NULL, NULL, &sunxi_sram_fops);
+
 	return 0;
 }
 
-- 
2.35.1
Re: [PATCH v2 05/10] soc: sunxi: sram: Fix probe function ordering issues
Posted by Heiko Stübner 3 years, 7 months ago 
Am Montag, 15. August 2022, 06:12:42 CEST schrieb Samuel Holland:
> Errors from debugfs are intended to be non-fatal, and should not prevent
> the driver from probing.
> 
> Since debugfs file creation is treated as infallible, move it below the
> parts of the probe function that can fail. This prevents an error
> elsewhere in the probe function from causing the file to leak. Do the
> same for the call to of_platform_populate().
> 
> Finally, checkpatch suggests an octal literal for the file permissions.
> 
> Fixes: 4af34b572a85 ("drivers: soc: sunxi: Introduce SoC driver to map SRAMs")
> Fixes: 5828729bebbb ("soc: sunxi: export a regmap for EMAC clock reg on A64")
> Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Tested-by: Heiko Stuebner <heiko@sntech.de>

but one thing below

> ---
> 
> (no changes since v1)
> 
>  drivers/soc/sunxi/sunxi_sram.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soc/sunxi/sunxi_sram.c b/drivers/soc/sunxi/sunxi_sram.c
> index a858a37fcdd4..52d07bed7664 100644
> --- a/drivers/soc/sunxi/sunxi_sram.c
> +++ b/drivers/soc/sunxi/sunxi_sram.c
> @@ -332,9 +332,9 @@ static struct regmap_config sunxi_sram_emac_clock_regmap = {
>  
>  static int __init sunxi_sram_probe(struct platform_device *pdev)
>  {
> -	struct dentry *d;
>  	struct regmap *emac_clock;
>  	const struct sunxi_sramc_variant *variant;
> +	struct device *dev = &pdev->dev;
>  
>  	sram_dev = &pdev->dev;
>  
> @@ -346,13 +346,6 @@ static int __init sunxi_sram_probe(struct platform_device *pdev)
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
>  
> -	of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> -
> -	d = debugfs_create_file("sram", S_IRUGO, NULL, NULL,
> -				&sunxi_sram_fops);
> -	if (!d)
> -		return -ENOMEM;
> -
>  	if (variant->num_emac_clocks > 0) {
>  		emac_clock = devm_regmap_init_mmio(&pdev->dev, base,
>  						   &sunxi_sram_emac_clock_regmap);
> @@ -361,6 +354,10 @@ static int __init sunxi_sram_probe(struct platform_device *pdev)
>  			return PTR_ERR(emac_clock);
>  	}
>  
> +	of_platform_populate(dev->of_node, NULL, NULL, dev);

hmm, of_platform_populate() can actually fail [0] it just looks a bit like
sunxi driver seem to ignore that by {chance, design?} [1] .

So I guess this might want to have handling for probably unlikely
possible errors instead?


Heiko

[0] https://elixir.bootlin.com/linux/latest/source/drivers/of/platform.c#L463
[1] https://elixir.bootlin.com/linux/latest/source/drivers/bus/sun50i-de2.c#L22
> +
> +	debugfs_create_file("sram", 0444, NULL, NULL, &sunxi_sram_fops);
> +
>  	return 0;
>  }
>  
>
Re: [PATCH v2 05/10] soc: sunxi: sram: Fix probe function ordering issues
Posted by Samuel Holland 3 years, 7 months ago 
On 8/15/22 9:06 AM, Heiko Stübner wrote:
> Am Montag, 15. August 2022, 06:12:42 CEST schrieb Samuel Holland:
>> Errors from debugfs are intended to be non-fatal, and should not prevent
>> the driver from probing.
>>
>> Since debugfs file creation is treated as infallible, move it below the
>> parts of the probe function that can fail. This prevents an error
>> elsewhere in the probe function from causing the file to leak. Do the
>> same for the call to of_platform_populate().
>>
>> Finally, checkpatch suggests an octal literal for the file permissions.
>>
>> Fixes: 4af34b572a85 ("drivers: soc: sunxi: Introduce SoC driver to map SRAMs")
>> Fixes: 5828729bebbb ("soc: sunxi: export a regmap for EMAC clock reg on A64")
>> Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
> 
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> 
> but one thing below
> 
>> ---
>>
>> (no changes since v1)
>>
>>  drivers/soc/sunxi/sunxi_sram.c | 13 +++++--------
>>  1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/soc/sunxi/sunxi_sram.c b/drivers/soc/sunxi/sunxi_sram.c
>> index a858a37fcdd4..52d07bed7664 100644
>> --- a/drivers/soc/sunxi/sunxi_sram.c
>> +++ b/drivers/soc/sunxi/sunxi_sram.c
>> @@ -332,9 +332,9 @@ static struct regmap_config sunxi_sram_emac_clock_regmap = {
>>  
>>  static int __init sunxi_sram_probe(struct platform_device *pdev)
>>  {
>> -	struct dentry *d;
>>  	struct regmap *emac_clock;
>>  	const struct sunxi_sramc_variant *variant;
>> +	struct device *dev = &pdev->dev;
>>  
>>  	sram_dev = &pdev->dev;
>>  
>> @@ -346,13 +346,6 @@ static int __init sunxi_sram_probe(struct platform_device *pdev)
>>  	if (IS_ERR(base))
>>  		return PTR_ERR(base);
>>  
>> -	of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
>> -
>> -	d = debugfs_create_file("sram", S_IRUGO, NULL, NULL,
>> -				&sunxi_sram_fops);
>> -	if (!d)
>> -		return -ENOMEM;
>> -
>>  	if (variant->num_emac_clocks > 0) {
>>  		emac_clock = devm_regmap_init_mmio(&pdev->dev, base,
>>  						   &sunxi_sram_emac_clock_regmap);
>> @@ -361,6 +354,10 @@ static int __init sunxi_sram_probe(struct platform_device *pdev)
>>  			return PTR_ERR(emac_clock);
>>  	}
>>  
>> +	of_platform_populate(dev->of_node, NULL, NULL, dev);
> 
> hmm, of_platform_populate() can actually fail [0] it just looks a bit like
> sunxi driver seem to ignore that by {chance, design?} [1] .
> 
> So I guess this might want to have handling for probably unlikely
> possible errors instead?

Strictly speaking, neither this driver nor the DE2 bus driver depend on any of
the child nodes having a platform device present or a driver attached. So
failing to populate the child devices should not necessarily prevent this driver
from probing. Possibly it deserves a dev_warn(), but...

I don't think of_platform_populate() can actually fail when passed a valid node.
of_platform_bus_create() calls itself recursively, but otherwise always returns 0.

Regards,
Samuel

> Heiko
> 
> [0] https://elixir.bootlin.com/linux/latest/source/drivers/of/platform.c#L463
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/bus/sun50i-de2.c#L22
>> +
>> +	debugfs_create_file("sram", 0444, NULL, NULL, &sunxi_sram_fops);
>> +
>>  	return 0;
>>  }

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.