[PATCH v2 0/2] fbdev/simplefb: Add missing simple-framebuffer features

[PATCH v2 0/2] fbdev/simplefb: Add missing simple-framebuffer features

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi,

This contains two patches that bring simplefb up to feature parity with
simpledrm. The patches add support for the "memory-region" property that
provides an alternative to the "reg" property to describe the memory
used for the framebuffer and allow attaching the simple-framebuffer
device to one or more generic power domains to make sure they aren't
turned off during the boot process and take down the display
configuration.

Changes in v2:
- remove unnecessary call to simplefb_detach_genpds() since that's
  already done automatically by devres
- fix crash if power-domains property is missing in DT

Thanks,
Thierry

Thierry Reding (2):
  fbdev/simplefb: Support memory-region property
  fbdev/simplefb: Add support for generic power-domains

 drivers/video/fbdev/simplefb.c | 128 +++++++++++++++++++++++++++++++--
 1 file changed, 123 insertions(+), 5 deletions(-)

-- 
2.42.0

[PATCH v2 1/2] fbdev/simplefb: Support memory-region property

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The simple-framebuffer bindings specify that the "memory-region"
property can be used as an alternative to the "reg" property to define
the framebuffer memory used by the display hardware. Implement support
for this in the simplefb driver.

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/video/fbdev/simplefb.c | 35 +++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 62f99f6fccd3..18025f34fde7 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -21,6 +21,7 @@
 #include <linux/platform_device.h>
 #include <linux/clk.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_clk.h>
 #include <linux/of_platform.h>
 #include <linux/parser.h>
@@ -121,12 +122,13 @@ struct simplefb_params {
 	u32 height;
 	u32 stride;
 	struct simplefb_format *format;
+	struct resource memory;
 };
 
 static int simplefb_parse_dt(struct platform_device *pdev,
 			   struct simplefb_params *params)
 {
-	struct device_node *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.of_node, *mem;
 	int ret;
 	const char *format;
 	int i;
@@ -166,6 +168,23 @@ static int simplefb_parse_dt(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
+	mem = of_parse_phandle(np, "memory-region", 0);
+	if (mem) {
+		ret = of_address_to_resource(mem, 0, &params->memory);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to parse memory-region\n");
+			of_node_put(mem);
+			return ret;
+		}
+
+		if (of_property_present(np, "reg"))
+			dev_warn(&pdev->dev, "preferring \"memory-region\" over \"reg\" property\n");
+
+		of_node_put(mem);
+	} else {
+		memset(&params->memory, 0, sizeof(params->memory));
+	}
+
 	return 0;
 }
 
@@ -193,6 +212,8 @@ static int simplefb_parse_pd(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
+	memset(&params->memory, 0, sizeof(params->memory));
+
 	return 0;
 }
 
@@ -431,10 +452,14 @@ static int simplefb_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(&pdev->dev, "No memory resource\n");
-		return -EINVAL;
+	if (params.memory.start == 0 && params.memory.end == 0) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!res) {
+			dev_err(&pdev->dev, "No memory resource\n");
+			return -EINVAL;
+		}
+	} else {
+		res = &params.memory;
 	}
 
 	mem = request_mem_region(res->start, resource_size(res), "simplefb");
-- 
2.42.0

[PATCH v2 2/2] fbdev/simplefb: Add support for generic power-domains

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The simple-framebuffer device tree bindings document the power-domains
property, so make sure that simplefb supports it. This ensures that the
power domains remain enabled as long as simplefb is active.

v2: - remove unnecessary call to simplefb_detach_genpds() since that's
      already done automatically by devres
    - fix crash if power-domains property is missing in DT

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/video/fbdev/simplefb.c | 93 ++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 18025f34fde7..fe682af63827 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -25,6 +25,7 @@
 #include <linux/of_clk.h>
 #include <linux/of_platform.h>
 #include <linux/parser.h>
+#include <linux/pm_domain.h>
 #include <linux/regulator/consumer.h>
 
 static const struct fb_fix_screeninfo simplefb_fix = {
@@ -78,6 +79,11 @@ struct simplefb_par {
 	unsigned int clk_count;
 	struct clk **clks;
 #endif
+#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
+	unsigned int num_genpds;
+	struct device **genpds;
+	struct device_link **genpd_links;
+#endif
 #if defined CONFIG_OF && defined CONFIG_REGULATOR
 	bool regulators_enabled;
 	u32 regulator_count;
@@ -432,6 +438,89 @@ static void simplefb_regulators_enable(struct simplefb_par *par,
 static void simplefb_regulators_destroy(struct simplefb_par *par) { }
 #endif
 
+#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
+static void simplefb_detach_genpds(void *res)
+{
+	struct simplefb_par *par = res;
+	unsigned int i = par->num_genpds;
+
+	if (par->num_genpds <= 1)
+		return;
+
+	while (i--) {
+		if (par->genpd_links[i])
+			device_link_del(par->genpd_links[i]);
+
+		if (!IS_ERR_OR_NULL(par->genpds[i]))
+			dev_pm_domain_detach(par->genpds[i], true);
+	}
+}
+
+static int simplefb_attach_genpds(struct simplefb_par *par,
+				  struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	unsigned int i;
+	int err;
+
+	err = of_count_phandle_with_args(dev->of_node, "power-domains",
+					 "#power-domain-cells");
+	if (err < 0) {
+		dev_info(dev, "failed to parse power-domains: %d\n", err);
+		return err;
+	}
+
+	par->num_genpds = err;
+
+	/*
+	 * Single power-domain devices are handled by the driver core, so
+	 * nothing to do here.
+	 */
+	if (par->num_genpds <= 1)
+		return 0;
+
+	par->genpds = devm_kcalloc(dev, par->num_genpds, sizeof(*par->genpds),
+				   GFP_KERNEL);
+	if (!par->genpds)
+		return -ENOMEM;
+
+	par->genpd_links = devm_kcalloc(dev, par->num_genpds,
+					sizeof(*par->genpd_links),
+					GFP_KERNEL);
+	if (!par->genpd_links)
+		return -ENOMEM;
+
+	for (i = 0; i < par->num_genpds; i++) {
+		par->genpds[i] = dev_pm_domain_attach_by_id(dev, i);
+		if (IS_ERR(par->genpds[i])) {
+			err = PTR_ERR(par->genpds[i]);
+			if (err == -EPROBE_DEFER) {
+				simplefb_detach_genpds(par);
+				return err;
+			}
+
+			dev_warn(dev, "failed to attach domain %u: %d\n", i, err);
+			continue;
+		}
+
+		par->genpd_links[i] = device_link_add(dev, par->genpds[i],
+						      DL_FLAG_STATELESS |
+						      DL_FLAG_PM_RUNTIME |
+						      DL_FLAG_RPM_ACTIVE);
+		if (!par->genpd_links[i])
+			dev_warn(dev, "failed to link power-domain %u\n", i);
+	}
+
+	return devm_add_action_or_reset(dev, simplefb_detach_genpds, par);
+}
+#else
+static int simplefb_attach_genpds(struct simplefb_par *par,
+				  struct platform_device *pdev)
+{
+	return 0;
+}
+#endif
+
 static int simplefb_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -518,6 +607,10 @@ static int simplefb_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error_clocks;
 
+	ret = simplefb_attach_genpds(par, pdev);
+	if (ret < 0)
+		goto error_regulators;
+
 	simplefb_clocks_enable(par, pdev);
 	simplefb_regulators_enable(par, pdev);
 
-- 
2.42.0

Re: [PATCH v2 0/2] fbdev/simplefb: Add missing simple-framebuffer features

[Hans de Goede]

Hi,

On 11/1/23 18:20, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Hi,
> 
> This contains two patches that bring simplefb up to feature parity with
> simpledrm. The patches add support for the "memory-region" property that
> provides an alternative to the "reg" property to describe the memory
> used for the framebuffer and allow attaching the simple-framebuffer
> device to one or more generic power domains to make sure they aren't
> turned off during the boot process and take down the display
> configuration.
> 
> Changes in v2:
> - remove unnecessary call to simplefb_detach_genpds() since that's
>   already done automatically by devres
> - fix crash if power-domains property is missing in DT

Thanks, the new version looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

for the series.

Helge, will you pick these 2 up, or shall I push them to drm-misc-fixes?

Regards,

Hans





> Thierry Reding (2):
>   fbdev/simplefb: Support memory-region property
>   fbdev/simplefb: Add support for generic power-domains
> 
>  drivers/video/fbdev/simplefb.c | 128 +++++++++++++++++++++++++++++++--
>  1 file changed, 123 insertions(+), 5 deletions(-)
> 

Re: [PATCH v2 0/2] fbdev/simplefb: Add missing simple-framebuffer features

[Hans de Goede]

Hi,

On 11/1/23 18:54, Hans de Goede wrote:
> Hi,
> 
> On 11/1/23 18:20, Thierry Reding wrote:
>> From: Thierry Reding <treding@nvidia.com>
>>
>> Hi,
>>
>> This contains two patches that bring simplefb up to feature parity with
>> simpledrm. The patches add support for the "memory-region" property that
>> provides an alternative to the "reg" property to describe the memory
>> used for the framebuffer and allow attaching the simple-framebuffer
>> device to one or more generic power domains to make sure they aren't
>> turned off during the boot process and take down the display
>> configuration.
>>
>> Changes in v2:
>> - remove unnecessary call to simplefb_detach_genpds() since that's
>>   already done automatically by devres
>> - fix crash if power-domains property is missing in DT
> 
> Thanks, the new version looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> for the series.
> 
> Helge, will you pick these 2 up, or shall I push them to drm-misc-fixes?

I have pushed this to drm-misc-next now.

I now I said drm-misc-fixes at first, but on a second look
these really are not fixes, so getting them in mainline
will have to wait to the next merge-window.

Regards,

Hans

Re: [PATCH v2 2/2] fbdev/simplefb: Add support for generic power-domains

[Richard Acayan]

Hello,

On Wed, Nov 01, 2023 at 06:20:17PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> The simple-framebuffer device tree bindings document the power-domains
> property, so make sure that simplefb supports it. This ensures that the
> power domains remain enabled as long as simplefb is active.
>
> v2: - remove unnecessary call to simplefb_detach_genpds() since that's
>       already done automatically by devres
>     - fix crash if power-domains property is missing in DT
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/video/fbdev/simplefb.c | 93 ++++++++++++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 18025f34fde7..fe682af63827 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -25,6 +25,7 @@
>  #include <linux/of_clk.h>
>  #include <linux/of_platform.h>
>  #include <linux/parser.h>
> +#include <linux/pm_domain.h>
>  #include <linux/regulator/consumer.h>
>  
>  static const struct fb_fix_screeninfo simplefb_fix = {
> @@ -78,6 +79,11 @@ struct simplefb_par {
>  	unsigned int clk_count;
>  	struct clk **clks;
>  #endif
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> +	unsigned int num_genpds;

This is the cause of the crash that occurred on the older patch series.
The field is unsigned, a deviation from v6.6:drivers/remoteproc/imx_rproc.c.

Instead of making it signed, this version emits an error whenever the
count is negative.

> +	struct device **genpds;
> +	struct device_link **genpd_links;
> +#endif
>  #if defined CONFIG_OF && defined CONFIG_REGULATOR
>  	bool regulators_enabled;
>  	u32 regulator_count;
> @@ -432,6 +438,89 @@ static void simplefb_regulators_enable(struct simplefb_par *par,
>  static void simplefb_regulators_destroy(struct simplefb_par *par) { }
>  #endif
>  
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> +static void simplefb_detach_genpds(void *res)
> +{
> +	struct simplefb_par *par = res;
> +	unsigned int i = par->num_genpds;
> +
> +	if (par->num_genpds <= 1)
> +		return;
> +
> +	while (i--) {
> +		if (par->genpd_links[i])
> +			device_link_del(par->genpd_links[i]);
> +
> +		if (!IS_ERR_OR_NULL(par->genpds[i]))
> +			dev_pm_domain_detach(par->genpds[i], true);
> +	}
> +}
> +
> +static int simplefb_attach_genpds(struct simplefb_par *par,
> +				  struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	unsigned int i;
> +	int err;
> +
> +	err = of_count_phandle_with_args(dev->of_node, "power-domains",
> +					 "#power-domain-cells");
> +	if (err < 0) {
> +		dev_info(dev, "failed to parse power-domains: %d\n", err);
> +		return err;

This error path is taken when there is no power-domains property in the
device tree with err = -ENOENT.

Strangely, this does not suppress the error like the next if statement,
even though it is possible that nothing is wrong.

> +	}
> +
> +	par->num_genpds = err;
> +
> +	/*
> +	 * Single power-domain devices are handled by the driver core, so
> +	 * nothing to do here.
> +	 */
> +	if (par->num_genpds <= 1)
> +		return 0;
> +
> +	par->genpds = devm_kcalloc(dev, par->num_genpds, sizeof(*par->genpds),
> +				   GFP_KERNEL);
<snip>
> @@ -518,6 +607,10 @@ static int simplefb_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto error_clocks;
>  
> +	ret = simplefb_attach_genpds(par, pdev);
> +	if (ret < 0)
> +		goto error_regulators;

With the error case specified above, not specifying power-domains (which
is valid according to dtschema) causes the entire driver to fail
whenever there are no power domains in the device tree.

On google-sargo, this causes a bug where the framebuffer fails to probe:

    [    0.409290] simple-framebuffer 9c000000.framebuffer: failed to parse power-domains: -2
    [    0.409340] simple-framebuffer: probe of 9c000000.framebuffer failed with error -2

Re: [PATCH v2 2/2] fbdev/simplefb: Add support for generic power-domains

[Hans de Goede]

Hi,

On 11/21/23 02:17, Richard Acayan wrote:
> Hello,
> 
> On Wed, Nov 01, 2023 at 06:20:17PM +0100, Thierry Reding wrote:
>> From: Thierry Reding <treding@nvidia.com>
>>
>> The simple-framebuffer device tree bindings document the power-domains
>> property, so make sure that simplefb supports it. This ensures that the
>> power domains remain enabled as long as simplefb is active.
>>
>> v2: - remove unnecessary call to simplefb_detach_genpds() since that's
>>       already done automatically by devres
>>     - fix crash if power-domains property is missing in DT
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>> ---
>>  drivers/video/fbdev/simplefb.c | 93 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 93 insertions(+)
>>
>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>> index 18025f34fde7..fe682af63827 100644
>> --- a/drivers/video/fbdev/simplefb.c
>> +++ b/drivers/video/fbdev/simplefb.c
>> @@ -25,6 +25,7 @@
>>  #include <linux/of_clk.h>
>>  #include <linux/of_platform.h>
>>  #include <linux/parser.h>
>> +#include <linux/pm_domain.h>
>>  #include <linux/regulator/consumer.h>
>>  
>>  static const struct fb_fix_screeninfo simplefb_fix = {
>> @@ -78,6 +79,11 @@ struct simplefb_par {
>>  	unsigned int clk_count;
>>  	struct clk **clks;
>>  #endif
>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>> +	unsigned int num_genpds;
> 
> This is the cause of the crash that occurred on the older patch series.
> The field is unsigned, a deviation from v6.6:drivers/remoteproc/imx_rproc.c.
> 
> Instead of making it signed, this version emits an error whenever the
> count is negative.

I'm not sure what you are trying to say here ?

>> +	struct device **genpds;
>> +	struct device_link **genpd_links;
>> +#endif
>>  #if defined CONFIG_OF && defined CONFIG_REGULATOR
>>  	bool regulators_enabled;
>>  	u32 regulator_count;
>> @@ -432,6 +438,89 @@ static void simplefb_regulators_enable(struct simplefb_par *par,
>>  static void simplefb_regulators_destroy(struct simplefb_par *par) { }
>>  #endif
>>  
>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>> +static void simplefb_detach_genpds(void *res)
>> +{
>> +	struct simplefb_par *par = res;
>> +	unsigned int i = par->num_genpds;
>> +
>> +	if (par->num_genpds <= 1)
>> +		return;
>> +
>> +	while (i--) {
>> +		if (par->genpd_links[i])
>> +			device_link_del(par->genpd_links[i]);
>> +
>> +		if (!IS_ERR_OR_NULL(par->genpds[i]))
>> +			dev_pm_domain_detach(par->genpds[i], true);
>> +	}
>> +}
>> +
>> +static int simplefb_attach_genpds(struct simplefb_par *par,
>> +				  struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	unsigned int i;
>> +	int err;
>> +
>> +	err = of_count_phandle_with_args(dev->of_node, "power-domains",
>> +					 "#power-domain-cells");
>> +	if (err < 0) {
>> +		dev_info(dev, "failed to parse power-domains: %d\n", err);
>> +		return err;
> 
> This error path is taken when there is no power-domains property in the
> device tree with err = -ENOENT.
> 
> Strangely, this does not suppress the error like the next if statement,
> even though it is possible that nothing is wrong.
> 
>> +	}
>> +
>> +	par->num_genpds = err;
>> +
>> +	/*
>> +	 * Single power-domain devices are handled by the driver core, so
>> +	 * nothing to do here.
>> +	 */
>> +	if (par->num_genpds <= 1)
>> +		return 0;
>> +
>> +	par->genpds = devm_kcalloc(dev, par->num_genpds, sizeof(*par->genpds),
>> +				   GFP_KERNEL);
> <snip>
>> @@ -518,6 +607,10 @@ static int simplefb_probe(struct platform_device *pdev)
>>  	if (ret < 0)
>>  		goto error_clocks;
>>  
>> +	ret = simplefb_attach_genpds(par, pdev);
>> +	if (ret < 0)
>> +		goto error_regulators;
> 
> With the error case specified above, not specifying power-domains (which
> is valid according to dtschema) causes the entire driver to fail
> whenever there are no power domains in the device tree.
> 
> On google-sargo, this causes a bug where the framebuffer fails to probe:
> 
>     [    0.409290] simple-framebuffer 9c000000.framebuffer: failed to parse power-domains: -2
>     [    0.409340] simple-framebuffer: probe of 9c000000.framebuffer failed with error -2

Ok so this is a problem, sorry for not catching this during review.

I believe that this should be fixed by changing the code to:

	err = of_count_phandle_with_args(dev->of_node, "power-domains",
					 "#power-domain-cells");
	if (err < 0) {
		if (err == -ENOENT)
			return 0;

		dev_info(dev, "failed to parse power-domains: %d\n", err);
		return err;
	}

Can you submit a (tested) patch fixing this? Then I'll push it
to drm-misc-next right away.

Regards,

Hans

Re: [PATCH v2 2/2] fbdev/simplefb: Add support for generic power-domains

[Richard Acayan]

On Tue, Nov 21, 2023 at 10:01:18AM +0100, Hans de Goede wrote:
> Hi,
>
> On 11/21/23 02:17, Richard Acayan wrote:
>> Hello,
>> 
>> On Wed, Nov 01, 2023 at 06:20:17PM +0100, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> The simple-framebuffer device tree bindings document the power-domains
>>> property, so make sure that simplefb supports it. This ensures that the
>>> power domains remain enabled as long as simplefb is active.
>>>
>>> v2: - remove unnecessary call to simplefb_detach_genpds() since that's
>>>       already done automatically by devres
>>>     - fix crash if power-domains property is missing in DT
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>  drivers/video/fbdev/simplefb.c | 93 ++++++++++++++++++++++++++++++++++
>>>  1 file changed, 93 insertions(+)
>>>
>>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>>> index 18025f34fde7..fe682af63827 100644
>>> --- a/drivers/video/fbdev/simplefb.c
>>> +++ b/drivers/video/fbdev/simplefb.c
>>> @@ -25,6 +25,7 @@
>>>  #include <linux/of_clk.h>
>>>  #include <linux/of_platform.h>
>>>  #include <linux/parser.h>
>>> +#include <linux/pm_domain.h>
>>>  #include <linux/regulator/consumer.h>
>>>  
>>>  static const struct fb_fix_screeninfo simplefb_fix = {
>>> @@ -78,6 +79,11 @@ struct simplefb_par {
>>>  	unsigned int clk_count;
>>>  	struct clk **clks;
>>>  #endif
>>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>>> +	unsigned int num_genpds;
>> 
>> This is the cause of the crash that occurred on the older patch series.
>> The field is unsigned, a deviation from v6.6:drivers/remoteproc/imx_rproc.c.
>> 
>> Instead of making it signed, this version emits an error whenever the
>> count is negative.
>
> I'm not sure what you are trying to say here ?

In v1 of the patch, there was no error propagation from of_count_phandle_with_args
and this field was directly assigned to the return value. This was a
problem (the "crash" as mentioned in this patch's changelog) when the
return value is negative, since unsigned integers cannot hold negative
values. On mainstream architectures, the driver would believe that there
is an absurd amount of power domains.

I compared the versions of this patch and figured that the fix to the
crash was more error handling.

Basically, if "unsigned" was removed, the error handling for the call to
of_count_phandle_with_args could be dropped with few consequences.

>>> +	struct device **genpds;
>>> +	struct device_link **genpd_links;
>>> +#endif
>>>  #if defined CONFIG_OF && defined CONFIG_REGULATOR
>>>  	bool regulators_enabled;
>>>  	u32 regulator_count;
>>> @@ -432,6 +438,89 @@ static void simplefb_regulators_enable(struct simplefb_par *par,
>>>  static void simplefb_regulators_destroy(struct simplefb_par *par) { }
>>>  #endif
>>>  
>>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>>> +static void simplefb_detach_genpds(void *res)
>>> +{
>>> +	struct simplefb_par *par = res;
>>> +	unsigned int i = par->num_genpds;
>>> +
>>> +	if (par->num_genpds <= 1)
>>> +		return;
>>> +
>>> +	while (i--) {
>>> +		if (par->genpd_links[i])
>>> +			device_link_del(par->genpd_links[i]);
>>> +
>>> +		if (!IS_ERR_OR_NULL(par->genpds[i]))
>>> +			dev_pm_domain_detach(par->genpds[i], true);
>>> +	}
>>> +}
>>> +
>>> +static int simplefb_attach_genpds(struct simplefb_par *par,
>>> +				  struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	unsigned int i;
>>> +	int err;
>>> +
>>> +	err = of_count_phandle_with_args(dev->of_node, "power-domains",
>>> +					 "#power-domain-cells");
>>> +	if (err < 0) {
>>> +		dev_info(dev, "failed to parse power-domains: %d\n", err);
>>> +		return err;
>> 
>> This error path is taken when there is no power-domains property in the
>> device tree with err = -ENOENT.
>> 
>> Strangely, this does not suppress the error like the next if statement,
>> even though it is possible that nothing is wrong.
>> 
>>> +	}
>>> +
>>> +	par->num_genpds = err;
>>> +
>>> +	/*
>>> +	 * Single power-domain devices are handled by the driver core, so
>>> +	 * nothing to do here.
>>> +	 */
>>> +	if (par->num_genpds <= 1)
>>> +		return 0;
>>> +
>>> +	par->genpds = devm_kcalloc(dev, par->num_genpds, sizeof(*par->genpds),
>>> +				   GFP_KERNEL);
>> <snip>
>>> @@ -518,6 +607,10 @@ static int simplefb_probe(struct platform_device *pdev)
>>>  	if (ret < 0)
>>>  		goto error_clocks;
>>>  
>>> +	ret = simplefb_attach_genpds(par, pdev);
>>> +	if (ret < 0)
>>> +		goto error_regulators;
>> 
>> With the error case specified above, not specifying power-domains (which
>> is valid according to dtschema) causes the entire driver to fail
>> whenever there are no power domains in the device tree.
>> 
>> On google-sargo, this causes a bug where the framebuffer fails to probe:
>> 
>>     [    0.409290] simple-framebuffer 9c000000.framebuffer: failed to parse power-domains: -2
>>     [    0.409340] simple-framebuffer: probe of 9c000000.framebuffer failed with error -2
>
> Ok so this is a problem, sorry for not catching this during review.
>
> I believe that this should be fixed by changing the code to:
>
> 	err = of_count_phandle_with_args(dev->of_node, "power-domains",
> 					 "#power-domain-cells");
> 	if (err < 0) {
> 		if (err == -ENOENT)
> 			return 0;
>
> 		dev_info(dev, "failed to parse power-domains: %d\n", err);
> 		return err;
> 	}
>
> Can you submit a (tested) patch fixing this? Then I'll push it
> to drm-misc-next right away.

Okay, will do. If my above response changes the preferred fix, let me
know.

Thank you for committing to having this fixed.

> Regards,
>
> Hans

Re: [PATCH v2 2/2] fbdev/simplefb: Add support for generic power-domains

[Hans de Goede]

Hi,

On 11/22/23 01:01, Richard Acayan wrote:
> On Tue, Nov 21, 2023 at 10:01:18AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/21/23 02:17, Richard Acayan wrote:
>>> Hello,
>>>
>>> On Wed, Nov 01, 2023 at 06:20:17PM +0100, Thierry Reding wrote:
>>>> From: Thierry Reding <treding@nvidia.com>
>>>>
>>>> The simple-framebuffer device tree bindings document the power-domains
>>>> property, so make sure that simplefb supports it. This ensures that the
>>>> power domains remain enabled as long as simplefb is active.
>>>>
>>>> v2: - remove unnecessary call to simplefb_detach_genpds() since that's
>>>>       already done automatically by devres
>>>>     - fix crash if power-domains property is missing in DT
>>>>
>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>> ---
>>>>  drivers/video/fbdev/simplefb.c | 93 ++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 93 insertions(+)
>>>>
>>>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>>>> index 18025f34fde7..fe682af63827 100644
>>>> --- a/drivers/video/fbdev/simplefb.c
>>>> +++ b/drivers/video/fbdev/simplefb.c
>>>> @@ -25,6 +25,7 @@
>>>>  #include <linux/of_clk.h>
>>>>  #include <linux/of_platform.h>
>>>>  #include <linux/parser.h>
>>>> +#include <linux/pm_domain.h>
>>>>  #include <linux/regulator/consumer.h>
>>>>  
>>>>  static const struct fb_fix_screeninfo simplefb_fix = {
>>>> @@ -78,6 +79,11 @@ struct simplefb_par {
>>>>  	unsigned int clk_count;
>>>>  	struct clk **clks;
>>>>  #endif
>>>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>>>> +	unsigned int num_genpds;
>>>
>>> This is the cause of the crash that occurred on the older patch series.
>>> The field is unsigned, a deviation from v6.6:drivers/remoteproc/imx_rproc.c.
>>>
>>> Instead of making it signed, this version emits an error whenever the
>>> count is negative.
>>
>> I'm not sure what you are trying to say here ?
> 
> In v1 of the patch, there was no error propagation from of_count_phandle_with_args
> and this field was directly assigned to the return value. This was a
> problem (the "crash" as mentioned in this patch's changelog) when the
> return value is negative, since unsigned integers cannot hold negative
> values. On mainstream architectures, the driver would believe that there
> is an absurd amount of power domains.
> 
> I compared the versions of this patch and figured that the fix to the
> crash was more error handling.
> 
> Basically, if "unsigned" was removed, the error handling for the call to
> of_count_phandle_with_args could be dropped with few consequences.

I see, thank you for explaining.

I believe that actually handling the error is better then storing
a negative value, so I believe that the fix in v2 is correct.

>>>> +	struct device **genpds;
>>>> +	struct device_link **genpd_links;
>>>> +#endif
>>>>  #if defined CONFIG_OF && defined CONFIG_REGULATOR
>>>>  	bool regulators_enabled;
>>>>  	u32 regulator_count;
>>>> @@ -432,6 +438,89 @@ static void simplefb_regulators_enable(struct simplefb_par *par,
>>>>  static void simplefb_regulators_destroy(struct simplefb_par *par) { }
>>>>  #endif
>>>>  
>>>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>>>> +static void simplefb_detach_genpds(void *res)
>>>> +{
>>>> +	struct simplefb_par *par = res;
>>>> +	unsigned int i = par->num_genpds;
>>>> +
>>>> +	if (par->num_genpds <= 1)
>>>> +		return;
>>>> +
>>>> +	while (i--) {
>>>> +		if (par->genpd_links[i])
>>>> +			device_link_del(par->genpd_links[i]);
>>>> +
>>>> +		if (!IS_ERR_OR_NULL(par->genpds[i]))
>>>> +			dev_pm_domain_detach(par->genpds[i], true);
>>>> +	}
>>>> +}
>>>> +
>>>> +static int simplefb_attach_genpds(struct simplefb_par *par,
>>>> +				  struct platform_device *pdev)
>>>> +{
>>>> +	struct device *dev = &pdev->dev;
>>>> +	unsigned int i;
>>>> +	int err;
>>>> +
>>>> +	err = of_count_phandle_with_args(dev->of_node, "power-domains",
>>>> +					 "#power-domain-cells");
>>>> +	if (err < 0) {
>>>> +		dev_info(dev, "failed to parse power-domains: %d\n", err);
>>>> +		return err;
>>>
>>> This error path is taken when there is no power-domains property in the
>>> device tree with err = -ENOENT.
>>>
>>> Strangely, this does not suppress the error like the next if statement,
>>> even though it is possible that nothing is wrong.
>>>
>>>> +	}
>>>> +
>>>> +	par->num_genpds = err;
>>>> +
>>>> +	/*
>>>> +	 * Single power-domain devices are handled by the driver core, so
>>>> +	 * nothing to do here.
>>>> +	 */
>>>> +	if (par->num_genpds <= 1)
>>>> +		return 0;
>>>> +
>>>> +	par->genpds = devm_kcalloc(dev, par->num_genpds, sizeof(*par->genpds),
>>>> +				   GFP_KERNEL);
>>> <snip>
>>>> @@ -518,6 +607,10 @@ static int simplefb_probe(struct platform_device *pdev)
>>>>  	if (ret < 0)
>>>>  		goto error_clocks;
>>>>  
>>>> +	ret = simplefb_attach_genpds(par, pdev);
>>>> +	if (ret < 0)
>>>> +		goto error_regulators;
>>>
>>> With the error case specified above, not specifying power-domains (which
>>> is valid according to dtschema) causes the entire driver to fail
>>> whenever there are no power domains in the device tree.
>>>
>>> On google-sargo, this causes a bug where the framebuffer fails to probe:
>>>
>>>     [    0.409290] simple-framebuffer 9c000000.framebuffer: failed to parse power-domains: -2
>>>     [    0.409340] simple-framebuffer: probe of 9c000000.framebuffer failed with error -2
>>
>> Ok so this is a problem, sorry for not catching this during review.
>>
>> I believe that this should be fixed by changing the code to:
>>
>> 	err = of_count_phandle_with_args(dev->of_node, "power-domains",
>> 					 "#power-domain-cells");
>> 	if (err < 0) {
>> 		if (err == -ENOENT)
>> 			return 0;
>>
>> 		dev_info(dev, "failed to parse power-domains: %d\n", err);
>> 		return err;
>> 	}
>>
>> Can you submit a (tested) patch fixing this? Then I'll push it
>> to drm-misc-next right away.
> 
> Okay, will do. If my above response changes the preferred fix, let me
> know.

Please move ahead with the proposed fixed, thank you.

> Thank you for committing to having this fixed.

You're welcome.

Regards,

Hans