linux-kernel - Re: [PATCH] drivers/perf: hisi: Add identifier sysfs file

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: <ae89e842-8bec-91f6-e758-a269c596ed5e@hisilicon.com>
Date:   Tue, 23 Jun 2020 20:35:11 +0800
From:   Shaokun Zhang <zhangshaokun@...ilicon.com>
To:     John Garry <john.garry@...wei.com>, <will@...nel.org>,
        <mark.rutland@....com>
CC:     <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <qiangqing.zhang@....com>,
        <jolsa@...hat.com>, <linuxarm@...wei.com>
Subject: Re: [PATCH] drivers/perf: hisi: Add identifier sysfs file

Hi John,

Thanks your further explaination and I'm ok on it, so for this patch:

Acked-by: Shaokun Zhang <zhangshaokun@...ilicon.com>

Thanks,
Shaokun

在 2020/6/18 17:18, John Garry 写道:
> On 18/06/2020 02:40, Shaokun Zhang wrote:
>>> }
>>>   +    hha_pmu->identifier = readl(hha_pmu->base + HHA_VERSION);
>> Since we are now refactoring the PMU framework, the PMU version offset
>> is always the same except DDRC PMU and other uncore PMU modules will
>> also use this, how about we do it as the common code:
>>
>> #define HISI_PMU_VERSION_REG   0x1CF0
>> int hisi_uncore_pmu_version(struct hisi_pmu *hisi_pmu)
>> {
>>         return readl(hisi_pmu->base + HISI_PMU_VERSION_REG);
>> }
>> EXPORT_SYMBOL_GPL(hisi_uncore_pmu_version);
> 
> Hi Shaokun,
> 
> Some points to make:
> 
> - It's hardly worth having a separate function to do this 1-line readl()
> call, especially since it not even generic (DDRC is different)
> 
> - We would have to export it (or put in a common header file with static
> inline keywords) - less exports are good
> 
> - with factoring out common code, it's good to reduce total code - this
> change would increase it, AFAICS
> 
> - This is HW specific. The driver is currently layered such that all HW
> specific stuff is in the HW driver (like hisi_uncore_ddrc_pmu.c), and
> not library code (hisi_uncore_pmu.c). I don't see why you want to mix
> that, like you're proposing in the framework revision you proposed
> internally.
> 
> Thanks,
> John
> 
>>
>> hha_pmu->identifier = hisi_uncore_pmu_version(hha_pmu);
>> we can remove the duplicated PMU version register definition in each
>> module.
>>
>> Thanks,
>> Shaokun
>>
>>> +
>>>       return 0;
>>>   }
>>>   @@ -320,10 +323,23 @@ static const struct attribute_group
>>> hisi_hha_pmu_cpumask_attr_group = {
>>>       .attrs = hisi_hha_pmu_cpumask_attrs,
>>>   };
> 
> 
> .