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

[Hans de Goede <hdegoede@redhat.com>]

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