| Message ID | cover.1598594714.git.viresh.kumar@linaro.org |
|---|---|
| Headers | show |
| Series | opp: Unconditionally call dev_pm_opp_of_remove_table() | expand |
Hi, On Fri, Aug 28, 2020 at 1:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Fri, 28 Aug 2020 at 08:08, Viresh Kumar <viresh.kumar@linaro.org> 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. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > Replaced v1 with v2 on my next branch, thanks! Actually, I don't see it on there yet, but at least the old broken v1 isn't there anymore. ;-) I picked v2 and tried it on my sc7180-based device (which does have OPP tables). It worked fine. Thus: Tested-by: Douglas Anderson <dianders@chromium.org> I looked at the code and it looks right to me. Thus: Reviewed-by: Douglas Anderson <dianders@chromium.org> > Just to be sure, this patch doesn't depend on any changes for the opp > core that are queued for v5.10? Running atop mmc-next, I see the check for -ENODEV, so I'm gonna assume that the required change is there. $ git grep -A10 'void _dev_pm_opp_find_and_remove_table' -- drivers/opp/core.c drivers/opp/core.c:void _dev_pm_opp_find_and_remove_table(struct device *dev) drivers/opp/core.c-{ drivers/opp/core.c- struct opp_table *opp_table; drivers/opp/core.c- drivers/opp/core.c- /* Check for existing table for 'dev' */ drivers/opp/core.c- opp_table = _find_opp_table(dev); drivers/opp/core.c- if (IS_ERR(opp_table)) { drivers/opp/core.c- int error = PTR_ERR(opp_table); drivers/opp/core.c- drivers/opp/core.c- if (error != -ENODEV) drivers/opp/core.c- WARN(1, "%s: opp_table: %d\n",
On 28-08-20, 10:43, Ulf Hansson wrote: > On Fri, 28 Aug 2020 at 08:08, Viresh Kumar <viresh.kumar@linaro.org> 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. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > Replaced v1 with v2 on my next branch, thanks! > > Just to be sure, this patch doesn't depend on any changes for the opp > core that are queued for v5.10? The recent crashes reported by Anders and Naresh were related to a OPP core bug, for which I have just sent the fix here: https://lore.kernel.org/lkml/922ff0759a16299e24cacfc981ac07914d8f1826.1598865786.git.viresh.kumar@linaro.org/ This is already tested by Naresh now and finally everything works as expected. I am going to get this fix merged in 5.9-rc4, but we do have a dependency now with that fix. What's the best way to handle this stuff now ? The easiest IMO would be for me to send these patches through the OPP tree, otherwise people need to carry this and the OPP fix (for which I can provide the branch/tag). -- viresh
On Mon, 31 Aug 2020 at 12:45, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 28-08-20, 10:43, Ulf Hansson wrote: > > On Fri, 28 Aug 2020 at 08:08, Viresh Kumar <viresh.kumar@linaro.org> 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. > > > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > Replaced v1 with v2 on my next branch, thanks! > > > > Just to be sure, this patch doesn't depend on any changes for the opp > > core that are queued for v5.10? > > The recent crashes reported by Anders and Naresh were related to a OPP > core bug, for which I have just sent the fix here: > > https://lore.kernel.org/lkml/922ff0759a16299e24cacfc981ac07914d8f1826.1598865786.git.viresh.kumar@linaro.org/ > > This is already tested by Naresh now and finally everything works as > expected. > > I am going to get this fix merged in 5.9-rc4, but we do have a > dependency now with that fix. > > What's the best way to handle this stuff now ? The easiest IMO would > be for me to send these patches through the OPP tree, otherwise people > need to carry this and the OPP fix (for which I can provide the > branch/tag). No need for a tag/branch to be shared. Instead I am simply going to defer to pick up any related changes for mmc, until I can rebase my tree on an rc[n] that contains your fix. When that is possible, please re-post the mmc patches. Kind regards Uffe
On 28-08-20, 11:37, Viresh Kumar wrote: > Hello, > > This cleans up some of the user code around calls to > dev_pm_opp_of_remove_table(). > > All the patches can be picked by respective maintainers directly except > for the last patch, which needs the previous two to get merged first. > > These are based for 5.9-rc1. > Viresh Kumar (8): > cpufreq: imx6q: Unconditionally call dev_pm_opp_of_remove_table() > drm/lima: Unconditionally call dev_pm_opp_of_remove_table() > drm/msm: Unconditionally call dev_pm_opp_of_remove_table() > mmc: sdhci-msm: Unconditionally call dev_pm_opp_of_remove_table() > spi: spi-geni-qcom: Unconditionally call dev_pm_opp_of_remove_table() > spi: spi-qcom-qspi: Unconditionally call dev_pm_opp_of_remove_table() > tty: serial: qcom_geni_serial: Unconditionally call > dev_pm_opp_of_remove_table() > qcom-geni-se: remove has_opp_table During testing by some of the Linaro folks on linux-next, we found out that there was a bug in the OPP core (which makes the kernel crash in some corner cases with these patches) for which I have sent a fix today which should be part of 5.9-rc4: https://lore.kernel.org/lkml/922ff0759a16299e24cacfc981ac07914d8f1826.1598865786.git.viresh.kumar@linaro.org/ Please apply the patches over rc4 only once it comes out (I will confirm by that time once the patch gets merged). Else you guys can provide your Ack and I can take the patches through OPP tree.
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); > } > -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
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. > 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. > [1] https://lore.kernel.org/patchwork/patch/1275632/ -- viresh
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/ -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
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.
On 31-08-20, 12:57, Ulf Hansson wrote: > On Mon, 31 Aug 2020 at 12:45, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 28-08-20, 10:43, Ulf Hansson wrote: > > > On Fri, 28 Aug 2020 at 08:08, Viresh Kumar <viresh.kumar@linaro.org> 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. > > > > > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > > > Replaced v1 with v2 on my next branch, thanks! > > > > > > Just to be sure, this patch doesn't depend on any changes for the opp > > > core that are queued for v5.10? > > > > The recent crashes reported by Anders and Naresh were related to a OPP > > core bug, for which I have just sent the fix here: > > > > https://lore.kernel.org/lkml/922ff0759a16299e24cacfc981ac07914d8f1826.1598865786.git.viresh.kumar@linaro.org/ > > > > This is already tested by Naresh now and finally everything works as > > expected. > > > > I am going to get this fix merged in 5.9-rc4, but we do have a > > dependency now with that fix. > > > > What's the best way to handle this stuff now ? The easiest IMO would > > be for me to send these patches through the OPP tree, otherwise people > > need to carry this and the OPP fix (for which I can provide the > > branch/tag). > > No need for a tag/branch to be shared. Instead I am simply going to > defer to pick up any related changes for mmc, until I can rebase my > tree on an rc[n] that contains your fix. > > When that is possible, please re-post the mmc patches. The dependency patch got merged in 5.9-rc4. Do you still want me to resend this patch or you can apply it directly from here ? -- viresh
On 31-08-20, 16:39, Viresh Kumar wrote: > On 28-08-20, 11:37, Viresh Kumar wrote: > > Hello, > > > > This cleans up some of the user code around calls to > > dev_pm_opp_of_remove_table(). > > > > All the patches can be picked by respective maintainers directly except > > for the last patch, which needs the previous two to get merged first. > > > > These are based for 5.9-rc1. > > > Viresh Kumar (8): > > cpufreq: imx6q: Unconditionally call dev_pm_opp_of_remove_table() > > drm/lima: Unconditionally call dev_pm_opp_of_remove_table() > > drm/msm: Unconditionally call dev_pm_opp_of_remove_table() > > mmc: sdhci-msm: Unconditionally call dev_pm_opp_of_remove_table() > > spi: spi-geni-qcom: Unconditionally call dev_pm_opp_of_remove_table() > > spi: spi-qcom-qspi: Unconditionally call dev_pm_opp_of_remove_table() > > tty: serial: qcom_geni_serial: Unconditionally call > > dev_pm_opp_of_remove_table() > > qcom-geni-se: remove has_opp_table > > During testing by some of the Linaro folks on linux-next, we found out > that there was a bug in the OPP core (which makes the kernel crash in > some corner cases with these patches) for which I have sent a fix > today which should be part of 5.9-rc4: > > https://lore.kernel.org/lkml/922ff0759a16299e24cacfc981ac07914d8f1826.1598865786.git.viresh.kumar@linaro.org/ > > Please apply the patches over rc4 only once it comes out (I will > confirm by that time once the patch gets merged). Else you guys can > provide your Ack and I can take the patches through OPP tree. The fix got merged in 5.9-rc4, please apply the patches from this series in your trees and base them on rc4. Thanks.
On Wed, 9 Sep 2020 at 13:07, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 31-08-20, 12:57, Ulf Hansson wrote: > > On Mon, 31 Aug 2020 at 12:45, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > On 28-08-20, 10:43, Ulf Hansson wrote: > > > > On Fri, 28 Aug 2020 at 08:08, Viresh Kumar <viresh.kumar@linaro.org> 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. > > > > > > > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > > > > > Replaced v1 with v2 on my next branch, thanks! > > > > > > > > Just to be sure, this patch doesn't depend on any changes for the opp > > > > core that are queued for v5.10? > > > > > > The recent crashes reported by Anders and Naresh were related to a OPP > > > core bug, for which I have just sent the fix here: > > > > > > https://lore.kernel.org/lkml/922ff0759a16299e24cacfc981ac07914d8f1826.1598865786.git.viresh.kumar@linaro.org/ > > > > > > This is already tested by Naresh now and finally everything works as > > > expected. > > > > > > I am going to get this fix merged in 5.9-rc4, but we do have a > > > dependency now with that fix. > > > > > > What's the best way to handle this stuff now ? The easiest IMO would > > > be for me to send these patches through the OPP tree, otherwise people > > > need to carry this and the OPP fix (for which I can provide the > > > branch/tag). > > > > No need for a tag/branch to be shared. Instead I am simply going to > > defer to pick up any related changes for mmc, until I can rebase my > > tree on an rc[n] that contains your fix. > > > > When that is possible, please re-post the mmc patches. > > The dependency patch got merged in 5.9-rc4. Do you still want me to > resend this patch or you can apply it directly from here ? Please re-submit, then I will pick it from the patchtracker. Kind regards Uffe
On Fri, 28 Aug 2020 11:37:45 +0530, Viresh Kumar wrote: > This cleans up some of the user code around calls to > dev_pm_opp_of_remove_table(). > > All the patches can be picked by respective maintainers directly except > for the last patch, which needs the previous two to get merged first. > > These are based for 5.9-rc1. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/2] spi: spi-geni-qcom: Unconditionally call dev_pm_opp_of_remove_table() commit: 7d568edff5cb7968cc5f29e85da15f941b8070b8 [2/2] spi: spi-qcom-qspi: Unconditionally call dev_pm_opp_of_remove_table() commit: 062cf7fc927d2546b58ed128383e5c52f26a00a5 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hello, This cleans up some of the user code around calls to dev_pm_opp_of_remove_table(). All the patches can be picked by respective maintainers directly except for the last patch, which needs the previous two to get merged first. These are based for 5.9-rc1. Rajendra, Since most of these changes are related to qcom stuff, it would be great if you can give them a try. I wasn't able to test them due to lack of hardware. Ulf, I had to revise the sdhci patch, sorry about that. Please pick this one. Diff between V1 and V2 is mentioned in each of the patches separately. Viresh Kumar (8): cpufreq: imx6q: Unconditionally call dev_pm_opp_of_remove_table() drm/lima: Unconditionally call dev_pm_opp_of_remove_table() drm/msm: Unconditionally call dev_pm_opp_of_remove_table() mmc: sdhci-msm: Unconditionally call dev_pm_opp_of_remove_table() spi: spi-geni-qcom: Unconditionally call dev_pm_opp_of_remove_table() spi: spi-qcom-qspi: Unconditionally call dev_pm_opp_of_remove_table() tty: serial: qcom_geni_serial: Unconditionally call dev_pm_opp_of_remove_table() qcom-geni-se: remove has_opp_table drivers/cpufreq/imx6q-cpufreq.c | 10 ++-------- drivers/gpu/drm/lima/lima_devfreq.c | 6 +----- drivers/gpu/drm/lima/lima_devfreq.h | 1 - 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 ++------ drivers/mmc/host/sdhci-msm.c | 14 +++++--------- drivers/spi/spi-geni-qcom.c | 13 +++++-------- drivers/spi/spi-qcom-qspi.c | 15 ++++++--------- drivers/tty/serial/qcom_geni_serial.c | 13 +++++-------- include/linux/qcom-geni-se.h | 2 -- 11 files changed, 31 insertions(+), 66 deletions(-) base-commit: 9123e3a74ec7b934a4a099e98af6a61c2f80bbf5 -- 2.25.0.rc1.19.g042ed3e048af