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

From: Hans de Goede <hdegoede@redhat.com>
To: Richard Acayan <mailingradian@gmail.com>
Cc: linux-fbdev@vger.kernel.org, Robert Foss <rfoss@kernel.org>,
	Helge Deller <deller@gmx.de>,
	dri-devel@lists.freedesktop.org,
	Jon Hunter <jonathanh@nvidia.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	linux-tegra@vger.kernel.org
Subject: Re: [PATCH v2 2/2] fbdev/simplefb: Add support for generic power-domains
Date: Wed, 22 Nov 2023 09:42:34 +0100	[thread overview]
Message-ID: <3e7a7b77-9104-4141-b8a9-3caca05416dd@redhat.com> (raw)
In-Reply-To: <ZV1E1aigUFcLceWK@radian>

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