linux-kernel - Re: [PATCH v2 6/9] coresight: Avoid enable programming clock duplicately

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: <eee538cf-d1ce-4444-97f4-450e4604f1ff@arm.com>
Date: Wed, 7 May 2025 11:25:42 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Leo Yan <leo.yan@....com>
Cc: Suzuki K Poulose <suzuki.poulose@....com>,
 Mike Leach <mike.leach@...aro.org>, James Clark <james.clark@...aro.org>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>, coresight@...ts.linaro.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH v2 6/9] coresight: Avoid enable programming clock
 duplicately

On 5/6/25 14:48, Leo Yan wrote:
> Hi Anshuman,
> 
> On Fri, May 02, 2025 at 12:08:55PM +0530, Anshuman Khandual wrote:
>> On 4/23/25 20:47, Leo Yan wrote:
>>> The programming clock is enabled by AMBA bus driver before a dynamic
>>> probe.  As a result, a CoreSight driver may redundantly enable the same
>>> clock.
>>
>> Are you sure AMBA bus driver always enables such clocks in all scenarios ?
> 
> Yes.  I confirmed that AMBA bus driver enables the programming clock
> prior to calling CoreSight device's probes (see amba_probe()).
> 
> I checked other AMBA device drivers (e.g., drivers/dma/amba-pl08x.c)
> never touch APB programming clock and the clock by default is covered
> by AMAB bus driver.

Alright.

> 
>> Even if that is true - why cannot coresight_get_enable_apb_pclk() ensured
>> to be called only for the platform drivers cases via code re-organization,
>> rather than changing the coresight_get_enable_apb_pclk() helper itself.
> 
> The purpose is to unify the clock enabling for both static probe and
> dynamic (AMBA) probe.
> 
> Let us take funnel driver as an example.  With the change in this patch,
> the clock operations will be consolidated in a central place
> (e.g., funnel_probe()).  Therefore, we can avoid to spread the drvdata
> allocation and clock operations into dynamic probe and static (platform)
> probe separately.
> 
>   funnel_probe()
>   {
>       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> 
>       drvdata->pclk = coresight_get_enable_apb_pclk();
>   }
> 
>   dynamic_funnel_probe()
>   {
>       funnel_probe();
>   }
> 
>   funnel_platform_probe()
>   {
>       funnel_probe();
>   }

Makes sense.

> 
> Thanks,
> Leo
> 
>>> To avoid this, add a check for device type and skip enabling the
>>> programming clock for AMBA devices.  The returned NULL pointer will be
>>> tolerated by the drivers.
>>>
>>> Fixes: 73d779a03a76 ("coresight: etm4x: Change etm4_platform_driver driver for MMIO devices")
>>> Signed-off-by: Leo Yan <leo.yan@....com>
>>> ---
>>>  include/linux/coresight.h | 11 +++++++----
>>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>>> index b888f6ed59b2..26eb4a61b992 100644
>>> --- a/include/linux/coresight.h
>>> +++ b/include/linux/coresight.h
>>> @@ -476,15 +476,18 @@ static inline bool is_coresight_device(void __iomem *base)
>>>   * Returns:
>>>   *
>>>   * clk   - Clock is found and enabled
>>> + * NULL  - Clock is not needed as it is managed by the AMBA bus driver
>>>   * ERROR - Clock is found but failed to enable
>>>   */
>>>  static inline struct clk *coresight_get_enable_apb_pclk(struct device *dev)
>>>  {
>>> -	struct clk *pclk;
>>> +	struct clk *pclk = NULL;
>>>  
>>> -	pclk = devm_clk_get_enabled(dev, "apb_pclk");
>>> -	if (IS_ERR(pclk))
>>> -		pclk = devm_clk_get_enabled(dev, "apb");
>>> +	if (!dev_is_amba(dev)) {
>>> +		pclk = devm_clk_get_enabled(dev, "apb_pclk");
>>> +		if (IS_ERR(pclk))
>>> +			pclk = devm_clk_get_enabled(dev, "apb");
>>> +	}
>>>  
>>>  	return pclk;
>>>  }