| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d0e9f90b-61f7-43b7-b54c-b9548692911f@ti.com>
Date: Mon, 29 Dec 2025 11:18:40 +0530
From: Swamil Jain <s-jain1@...com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
CC: <dri-devel@...ts.freedesktop.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <devarsht@...com>, <praneeth@...com>,
<h-shenoy@...com>, <u-kumar1@...com>, <jyri.sarha@....fi>,
<airlied@...il.com>, <simona@...ll.ch>, <maarten.lankhorst@...ux.intel.com>,
<mripard@...nel.org>, <tzimmermann@...e.de>, <robh@...nel.org>,
<krzk+dt@...nel.org>, <conor+dt@...nel.org>, <aradhya.bhatia@...ux.dev>
Subject: Re: [PATCH v2 2/3] drm/tidss: Power up attached PM domains on probe
Hi Tomi,
Thanks for the feedback.
On 12/1/25 18:10, Tomi Valkeinen wrote:
> Hi,
>
> On 25/11/2025 18:59, Swamil Jain wrote:
>> From: Devarsh Thakkar <devarsht@...com>
>>
>> Some SoC's such as AM62P have dedicated power domains
>> for OLDI which need to be powered on separately along
>> with display controller.
>>
>> So during driver probe, power up all attached PM domains
>> enumerated in devicetree node for DSS.
>>
>> This also prepares base to add display support for AM62P.
>>
>> Signed-off-by: Devarsh Thakkar <devarsht@...com>
>> [j-choudhary@...com: fix PM call sequence causing kernel crash in OLDI]
>> Signed-off-by: Jayesh Choudhary <j-choudhary@...com>
>> Signed-off-by: Swamil Jain <s-jain1@...com>
>> ---
>> drivers/gpu/drm/tidss/tidss_drv.c | 88 +++++++++++++++++++++++++++++--
>> drivers/gpu/drm/tidss/tidss_drv.h | 4 ++
>> 2 files changed, 89 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
>> index 1c8cc18bc53c..50158281715f 100644
>> --- a/drivers/gpu/drm/tidss/tidss_drv.c
>> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
>> @@ -8,6 +8,7 @@
>> #include <linux/of.h>
>> #include <linux/module.h>
>> #include <linux/pm_runtime.h>
>> +#include <linux/pm_domain.h>
>> #include <linux/aperture.h>
>>
>> #include <drm/clients/drm_client_setup.h>
>> @@ -107,6 +108,72 @@ static const struct drm_driver tidss_driver = {
>> .minor = 0,
>> };
>>
>> +static int tidss_detach_pm_domains(struct tidss_device *tidss)
>> +{
>> + int i;
>> +
>> + if (tidss->num_domains <= 1)
>> + return 0;
>> +
>> + for (i = 0; i < tidss->num_domains; i++) {
>> + if (tidss->pd_link[i] && !IS_ERR(tidss->pd_link[i]))
>> + device_link_del(tidss->pd_link[i]);
>> + if (tidss->pd_dev[i] && !IS_ERR(tidss->pd_dev[i]))
>> + dev_pm_domain_detach(tidss->pd_dev[i], true);
>
> There's IS_ERR_OR_NULL()
Ack, will replace with IS_ERR_OR_NULL.
>
>> + tidss->pd_dev[i] = NULL;
>> + tidss->pd_link[i] = NULL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int tidss_attach_pm_domains(struct tidss_device *tidss)
>> +{
>> + struct device *dev = tidss->dev;
>> + int i;
>> + int ret;
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct device_node *np = pdev->dev.of_node;
>> +
>> + tidss->num_domains = of_count_phandle_with_args(np, "power-domains",
>> + "#power-domain-cells");
>> + if (tidss->num_domains <= 1) {
>> + dev_dbg(dev, "One or less power domains, no need to do attach domains\n");
>
> I don't think this print is needed. It would be printed on almost all
> platforms with DSS.
Yeah, true, will remove this in the next version.
>
>> + return 0;
>> + }
>> +
>> + tidss->pd_dev = devm_kmalloc_array(dev, tidss->num_domains,
>> + sizeof(*tidss->pd_dev), GFP_KERNEL);
>> + if (!tidss->pd_dev)
>> + return -ENOMEM;
>> +
>> + tidss->pd_link = devm_kmalloc_array(dev, tidss->num_domains,
>> + sizeof(*tidss->pd_link), GFP_KERNEL);
>> + if (!tidss->pd_link)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < tidss->num_domains; i++) {
>> + tidss->pd_dev[i] = dev_pm_domain_attach_by_id(dev, i);
>> + if (IS_ERR(tidss->pd_dev[i])) {
>> + ret = PTR_ERR(tidss->pd_dev[i]);
>> + goto fail;
>> + }
>> +
>> + tidss->pd_link[i] = device_link_add(dev, tidss->pd_dev[i],
>> + DL_FLAG_STATELESS |
>> + DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
>> + if (!tidss->pd_link[i]) {
>> + ret = -EINVAL;
>> + goto fail;
>> + }
>> + }
>> +
>> + return 0;
>> +fail:
>> + tidss_detach_pm_domains(tidss);
>> + return ret;
>> +}
>> +
>> static int tidss_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> @@ -129,15 +196,24 @@ static int tidss_probe(struct platform_device *pdev)
>>
>> spin_lock_init(&tidss->irq_lock);
>>
>> + /* powering up associated OLDI domains */
>
> I think the function name is self-explanatory, no comment needed.
Will remove the comment.
>
>> + ret = tidss_attach_pm_domains(tidss);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to attach power domains %d\n", ret);
>> + goto err_detach_pm_domains;
>
> This is not correct. If tidss_attach_pm_domains() fails, it should do
> its own cleanup, thus there's no need to goto err_detach_pm_domains.
>
Ok, will make tidss_attach_pm_domains do it's own cleanup.
>> + }
>> +
>> ret = dispc_init(tidss);
>> if (ret) {
>> dev_err(dev, "failed to initialize dispc: %d\n", ret);
>> - return ret;
>> + goto err_detach_pm_domains;
>> }
>>
>> ret = tidss_oldi_init(tidss);
>> - if (ret)
>> - return dev_err_probe(dev, ret, "failed to init OLDI\n");
>> + if (ret) {
>> + dev_dbg(dev, "failed to init OLDI: %d\n", ret);
>> + goto err_oldi_deinit;
>
> Same here, this is just not correct. Please go through the error
> handling paths with your patch. This also changes dev_err_probe() to
> dev_dbg().
Sure.
>
>> + }
>>
>> pm_runtime_enable(dev);
>>
>> @@ -203,8 +279,12 @@ static int tidss_probe(struct platform_device *pdev)
>> pm_runtime_dont_use_autosuspend(dev);
>> pm_runtime_disable(dev);
>>
>> +err_oldi_deinit:
>> tidss_oldi_deinit(tidss);
>>
>> +err_detach_pm_domains:
>> + tidss_detach_pm_domains(tidss);
>> +
>> return ret;
>> }
>>
>> @@ -232,6 +312,8 @@ static void tidss_remove(struct platform_device *pdev)
>> /* devm allocated dispc goes away with the dev so mark it NULL */
>> dispc_remove(tidss);
>>
>> + tidss_detach_pm_domains(tidss);
>> +
>> dev_dbg(dev, "%s done\n", __func__);
>> }
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
>> index e1c1f41d8b4b..6eb17cb32043 100644
>> --- a/drivers/gpu/drm/tidss/tidss_drv.h
>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
>> @@ -41,6 +41,10 @@ struct tidss_device {
>> /* protects the irq masks field and irqenable/irqstatus registers */
>> spinlock_t irq_lock;
>> dispc_irq_t irq_mask; /* enabled irqs */
>> +
>> + int num_domains; /* Handle attached PM domains */
>
> What does the comment mean?
The number of power domains that needs to be handled, will rephrase it
to make it more understandable.
>
> Tomi
>
>> + struct device **pd_dev;
>> + struct device_link **pd_link;
>> };
>>
>> #define to_tidss(__dev) container_of(__dev, struct tidss_device, ddev)
>
Powered by blists - more mailing lists