linux-kernel - Re: [PATCH v5 04/10] coresight: Appropriately disable programming clocks

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cd250adb-61e7-414f-bf17-6cc960e44f7b@arm.com>
Date: Wed, 30 Jul 2025 12:01:25 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Leo Yan <leo.yan@....com>
Cc: Mark Brown <broonie@...nel.org>, Mike Leach <mike.leach@...aro.org>,
 James Clark <james.clark@...aro.org>,
 Anshuman Khandual <anshuman.khandual@....com>,
 Yeoreum Yun <yeoreum.yun@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>, coresight@...ts.linaro.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 04/10] coresight: Appropriately disable programming
 clocks

On 30/07/2025 11:54, Leo Yan wrote:
> On Wed, Jul 30, 2025 at 10:27:48AM +0100, Suzuki Kuruppassery Poulose wrote:
>> On 30/07/2025 09:56, Leo Yan wrote:
>>> On Tue, Jul 29, 2025 at 01:30:28PM +0100, Suzuki Kuruppassery Poulose wrote:
>>>> On 29/07/2025 12:31, Mark Brown wrote:
>>>>> On Mon, Jul 28, 2025 at 05:45:04PM +0100, Mark Brown wrote:
>>>>>> On Thu, Jul 24, 2025 at 04:22:34PM +0100, Leo Yan wrote:
>>>>>>
>>>>>> Previously we would return NULL for any error (which isn't super great
>>>>>> for deferred probe but never mind).
>>>>>>
>>>>>>> +	pclk = devm_clk_get_enabled(dev, "apb_pclk");
>>>>>>> +	if (IS_ERR(pclk))
>>>>>>> +		pclk = devm_clk_get_enabled(dev, "apb");
>>>>>>
>>>>>> ...
>>>>>>
>>>>>>>     	return pclk;
>>>>>>>     }
>>>>>>
>>>>>> Now we pass errors back to the caller, making missing clocks fatal.
>>>>>
>>>>> Thinking about this some more I think for compatiblity these clocks need
>>>>> to be treated as optional - that's what the original code was
>>>>> effectively doing, and I can imagine this isn't the only SoC which has
>>>>> (hopefully) always on clocks and didn't wire things up in DT.
>>>>
>>>> You're right. The static components (funnels, replicators) don't have
>>>> APB programming interface and hence no clocks. That said, may be the
>>>> "is amba device" check could be used to enforce the presence of a clock.
>>>
>>> I was wondering how this issue slipped through when I tested it on the
>>> Hikey960 board. The Hikey960 also has one static funnel, but it binds
>>> pclk with the static funnel node. That's why I didn't detect the issue.
>>>
>>> I don't think using optional clock API is right thing, as DT binding
>>> schema claims the pclk is mandatory for dynamic components. My proposal
>>> is to enable the clocks only when IORESOURCE_MEM is available, something
>>> like:
>>>
>>>     if (res) {
>>>         ret = coresight_get_enable_clocks(dev, &drvdata->pclk,
>>>                                           &drvdata->atclk);
>>
>> That may not work, as they may need the ATCLK enabled to
>> push the trace over ATB. They may skip the APB, as there
>> is no programming interface.
> 
> If so, I will use an extra patch to skip pclk enabling for static funnel
> and replicator, as a result, patch 04 will be:
> 
>    if (res) {
>        drvdata->pclk = coresight_get_enable_apb_pclk(dev);
>        if (IS_ERR(drvdata->pclk))
>            return PTR_ERR(drvdata->pclk);
>    }
> 
> Then, when consolidation in patch 07, it will have a code:
> 
>    /* Only enable pclk for a device with I/O resource */
>    ret = coresight_get_enable_clocks(dev, res ? &drvdata->pclk : NULL,
>                                      &drvdata->atclk);

Can we not differentiate the error code in devm_get..() for

  ENOENT (not found) vs Some other failure ?

I would recommend using that and don't force the use of apb_clk/apb
for AMBA devices. If the firmware doesn't specify a clock, but does
specify the CoreSight components, it knows it better.

Suzuki



> 
> This turns out to be the case for both static funnel and replicator
> devices — regardless of whether the DT binding includes "apb_pclk" or
> not, the driver will always skip enabling it. Any concerns?
> 
> Thanks,
> Leo