diff mbox series

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

Message ID 6e4110032f8711e8bb0acbeccfe66dec3b09d5c1.1598594714.git.viresh.kumar@linaro.org
State Accepted
Commit 6400a8e886c6a3ee0cfcbcdc0867a3e91186683c
Headers show
Series [V2,1/8] cpufreq: imx6q: Unconditionally call dev_pm_opp_of_remove_table() | expand

Commit Message

Viresh Kumar Aug. 28, 2020, 6:07 a.m. UTC
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

Comments

Rajendra Nayak Sept. 1, 2020, 7:31 a.m. UTC | #1
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);
>   }
>
Rajendra Nayak Sept. 1, 2020, 9:45 a.m. UTC | #2
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/
Viresh Kumar Sept. 1, 2020, 9:50 a.m. UTC | #3
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
Viresh Kumar Oct. 5, 2020, 6:26 a.m. UTC | #4
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) ?
Viresh Kumar Oct. 21, 2020, 7:24 a.m. UTC | #5
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
Viresh Kumar Oct. 22, 2020, 5:12 a.m. UTC | #6
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
diff mbox series

Patch

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;
 	}
 
 	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);
 }