[V2,3/8] drm/msm: Unconditionally call dev_pm_opp_of_remove_table()

dev_pm_opp_of_remove_table() doesn't report any errors when it fails to
find the OPP table with error -ENODEV (i.e. OPP table not present for
the device). And we can call dev_pm_opp_of_remove_table()
unconditionally here.

While at it, also create a label to put clkname.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>


---
V2:
- Compare with -ENODEV only for failures.
- Create new label to put clkname.
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 +++++---------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |  1 -
 drivers/gpu/drm/msm/dsi/dsi_host.c      |  8 ++------
 3 files changed, 7 insertions(+), 16 deletions(-)

-- 
2.25.0.rc1.19.g042ed3e048af

On 8/28/2020 11:37 AM, Viresh Kumar wrote:
> dev_pm_opp_of_remove_table() doesn't report any errors when it fails to
> find the OPP table with error -ENODEV (i.e. OPP table not present for
> the device). And we can call dev_pm_opp_of_remove_table()
> unconditionally here.

Its a little tricky to call things unconditionally for this driver, more below.

> 
> While at it, also create a label to put clkname.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> ---
> V2:
> - Compare with -ENODEV only for failures.
> - Create new label to put clkname.
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 +++++---------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |  1 -
>   drivers/gpu/drm/msm/dsi/dsi_host.c      |  8 ++------
>   3 files changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index c0a4d4e16d82..c8287191951f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1010,12 +1010,9 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
>   		return PTR_ERR(dpu_kms->opp_table);
>   	/* OPP table is optional */
>   	ret = dev_pm_opp_of_add_table(dev);
> -	if (!ret) {
> -		dpu_kms->has_opp_table = true;
> -	} else if (ret != -ENODEV) {
> +	if (ret && ret != -ENODEV) {
>   		dev_err(dev, "invalid OPP table in device tree\n");
> -		dev_pm_opp_put_clkname(dpu_kms->opp_table);
> -		return ret;
> +		goto put_clkname;

So FWIU, dpu_unbind() gets called even when dpu_bind() fails for some reason.
I tried to address that earlier [1] which I realized did not land. But with these changes
it will be even more broken unless we identify if we failed dpu_bind() before
adding the OPP table, while adding it, or all went well with opps and handle things
accordingly in dpu_unbind.

[1] https://lore.kernel.org/patchwork/patch/1275632/

>   	}
>   
>   	mp = &dpu_kms->mp;
> @@ -1037,8 +1034,8 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
>   	priv->kms = &dpu_kms->base;
>   	return ret;
>   err:
> -	if (dpu_kms->has_opp_table)
> -		dev_pm_opp_of_remove_table(dev);
> +	dev_pm_opp_of_remove_table(dev);
> +put_clkname:
>   	dev_pm_opp_put_clkname(dpu_kms->opp_table);
>   	return ret;
>   }
> @@ -1056,8 +1053,7 @@ static void dpu_unbind(struct device *dev, struct device *master, void *data)
>   	if (dpu_kms->rpm_enabled)
>   		pm_runtime_disable(&pdev->dev);
>   
> -	if (dpu_kms->has_opp_table)
> -		dev_pm_opp_of_remove_table(dev);
> +	dev_pm_opp_of_remove_table(dev);
>   	dev_pm_opp_put_clkname(dpu_kms->opp_table);
>   }
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index e140cd633071..8295979a7165 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -129,7 +129,6 @@ struct dpu_kms {
>   	bool rpm_enabled;
>   
>   	struct opp_table *opp_table;
> -	bool has_opp_table;
>   
>   	struct dss_module_power mp;
>   
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index b17ac6c27554..4335fe33250c 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -113,7 +113,6 @@ struct msm_dsi_host {
>   	struct clk *byte_intf_clk;
>   
>   	struct opp_table *opp_table;
> -	bool has_opp_table;
>   
>   	u32 byte_clk_rate;
>   	u32 pixel_clk_rate;
> @@ -1891,9 +1890,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
>   		return PTR_ERR(msm_host->opp_table);
>   	/* OPP table is optional */
>   	ret = dev_pm_opp_of_add_table(&pdev->dev);
> -	if (!ret) {
> -		msm_host->has_opp_table = true;
> -	} else if (ret != -ENODEV) {
> +	if (ret && ret != -ENODEV) {
>   		dev_err(&pdev->dev, "invalid OPP table in device tree\n");
>   		dev_pm_opp_put_clkname(msm_host->opp_table);
>   		return ret;
> @@ -1934,8 +1931,7 @@ void msm_dsi_host_destroy(struct mipi_dsi_host *host)
>   	mutex_destroy(&msm_host->cmd_mutex);
>   	mutex_destroy(&msm_host->dev_mutex);
>   
> -	if (msm_host->has_opp_table)
> -		dev_pm_opp_of_remove_table(&msm_host->pdev->dev);
> +	dev_pm_opp_of_remove_table(&msm_host->pdev->dev);
>   	dev_pm_opp_put_clkname(msm_host->opp_table);
>   	pm_runtime_disable(&msm_host->pdev->dev);
>   }
>

On 9/1/2020 2:08 PM, Viresh Kumar wrote:
> On 01-09-20, 13:01, Rajendra Nayak wrote:
>> So FWIU, dpu_unbind() gets called even when dpu_bind() fails for some reason.
> 
> Ahh, I see.
> 
>> I tried to address that earlier [1] which I realized did not land.
> 
> I don't think that patch was required, as you can call
> dev_pm_opp_put_clkname() multiple times and it will return without any
> errors/crash.

We did see a crash (Sai had reported it), perhaps with dsi [1] and not this
driver. But it was the same scenario that was possible here as well, which is
dev_pm_opp_put_clkname() getting called without dev_pm_opp_set_clkname()
being done. I think we ended up passing a NULL as opp_table in that case
and the function tries de-referencing it.

> 
>> But with these changes
>> it will be even more broken unless we identify if we failed dpu_bind() before
>> adding the OPP table, while adding it, or all went well with opps and handle things
>> accordingly in dpu_unbind.
> 
> Maybe not as dev_pm_opp_of_remove_table() can be called multiple times
> as well without any errors or crash.

Can it be called without the driver ever doing a dev_pm_opp_of_add_table()?

[1] https://lore.kernel.org/patchwork/patch/1275628/

On 01-09-20, 15:15, Rajendra Nayak wrote:
> 

> On 9/1/2020 2:08 PM, Viresh Kumar wrote:

> > On 01-09-20, 13:01, Rajendra Nayak wrote:

> > > So FWIU, dpu_unbind() gets called even when dpu_bind() fails for some reason.

> > 

> > Ahh, I see.

> > 

> > > I tried to address that earlier [1] which I realized did not land.

> > 

> > I don't think that patch was required, as you can call

> > dev_pm_opp_put_clkname() multiple times and it will return without any

> > errors/crash.

> 

> We did see a crash (Sai had reported it), perhaps with dsi [1] and not this

> driver. But it was the same scenario that was possible here as well, which is

> dev_pm_opp_put_clkname() getting called without dev_pm_opp_set_clkname()

> being done. I think we ended up passing a NULL as opp_table in that case

> and the function tries de-referencing it.


Heh, yeah I did miss that stupid thing :(

> > 

> > > But with these changes

> > > it will be even more broken unless we identify if we failed dpu_bind() before

> > > adding the OPP table, while adding it, or all went well with opps and handle things

> > > accordingly in dpu_unbind.

> > 

> > Maybe not as dev_pm_opp_of_remove_table() can be called multiple times

> > as well without any errors or crash.

> 

> Can it be called without the driver ever doing a dev_pm_opp_of_add_table()?


Yes, as we will fail to find the OPP device in that case with -ENODEV
and so won't even print a warning.

Also if the OPP table was previously added as a response to
dev_pm_opp_set_clkname(), then we won't free it as well. So yes, it
should work just fine.

-- 
viresh

On 28-08-20, 11:37, Viresh Kumar wrote:
> dev_pm_opp_of_remove_table() doesn't report any errors when it fails to
> find the OPP table with error -ENODEV (i.e. OPP table not present for
> the device). And we can call dev_pm_opp_of_remove_table()
> unconditionally here.
> 
> While at it, also create a label to put clkname.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Can someone please apply this and the other drm patch (2/8) ?

On 05-10-20, 11:56, Viresh Kumar wrote:
> On 28-08-20, 11:37, Viresh Kumar wrote:

> > dev_pm_opp_of_remove_table() doesn't report any errors when it fails to

> > find the OPP table with error -ENODEV (i.e. OPP table not present for

> > the device). And we can call dev_pm_opp_of_remove_table()

> > unconditionally here.

> > 

> > While at it, also create a label to put clkname.

> > 

> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> 

> Can someone please apply this and the other drm patch (2/8) ?


Rob/Rajendra, can someone please have a look at these patches ?

-- 
viresh

On 21-10-20, 09:58, Rob Clark wrote:
> On Wed, Oct 21, 2020 at 12:24 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> >

> > On 05-10-20, 11:56, Viresh Kumar wrote:

> > > On 28-08-20, 11:37, Viresh Kumar wrote:

> > > > dev_pm_opp_of_remove_table() doesn't report any errors when it fails to

> > > > find the OPP table with error -ENODEV (i.e. OPP table not present for

> > > > the device). And we can call dev_pm_opp_of_remove_table()

> > > > unconditionally here.

> > > >

> > > > While at it, also create a label to put clkname.

> > > >

> > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> > >

> > > Can someone please apply this and the other drm patch (2/8) ?

> >

> > Rob/Rajendra, can someone please have a look at these patches ?

> 

> Oh, sorry I missed that, could someone re-send it and CC

> freedreno@lists.freedesktop.org so it shows up in patchworks.. that is

> more reliable counting on me to not overlook something in my mail


I have just bounced it back to you and freedreno was already cc'd,
though I have bounced it there again.

-- 
viresh