| 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: <8afe0502-2e2a-a89b-dbe0-bf74aef98290@linux.intel.com>
Date: Thu, 9 Feb 2023 19:00:57 +0530
From: Rajat Khandelwal <rajat.khandelwal@...ux.intel.com>
To: "Box, David E" <david.e.box@...el.com>,
"hdegoede@...hat.com" <hdegoede@...hat.com>,
"markgross@...nel.org" <markgross@...nel.org>,
"irenic.rajneesh@...il.com" <irenic.rajneesh@...il.com>
Cc: "Khandelwal, Rajat" <rajat.khandelwal@...el.com>,
"platform-driver-x86@...r.kernel.org"
<platform-driver-x86@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4] platform/x86/intel/pmc: core: Add support to show
LTR-ignored components
Hi David,
Please find the comments inline.
On 2/5/2023 12:49 AM, Box, David E wrote:
> Hi Rajat,
>
> On Sun, 2023-02-05 at 23:14 +0530, Rajat Khandelwal wrote:
>> Currently, 'ltr_ignore' sysfs attribute, when read, returns nothing, even
>> if there are components whose LTR values have been ignored.
>>
>> This patch adds the feature to print out such components, if they exist.
>>
>> Signed-off-by: Rajat Khandelwal <rajat.khandelwal@...ux.intel.com>
>> ---
>>
>> v4: Mutex unlock during error conditions
>>
>> v3: Incorporated a mutex lock for accessing 'ltr_ignore_list'
>>
>> v2: kmalloc -> devm_kmalloc
>>
>> drivers/platform/x86/intel/pmc/core.c | 59 ++++++++++++++++++++++-----
>> drivers/platform/x86/intel/pmc/core.h | 2 +-
>> 2 files changed, 49 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmc/core.c
>> b/drivers/platform/x86/intel/pmc/core.c
>> index 3a15d32d7644..f9d4b2865b03 100644
>> --- a/drivers/platform/x86/intel/pmc/core.c
>> +++ b/drivers/platform/x86/intel/pmc/core.c
>> @@ -53,6 +53,17 @@ const struct pmc_bit_map msr_map[] = {
>> {}
>> };
>>
>> +/* Mutual exclusion to access the list of LTR-ignored components */
>> +static DEFINE_MUTEX(ltr_entry_mutex);
>> +
>> +struct ltr_entry {
>> + u32 comp_index;
>> + const char *comp_name;
>> + struct list_head node;
>> +};
>> +
>> +static LIST_HEAD(ltr_ignore_list);
>> +
>> static inline u32 pmc_core_reg_read(struct pmc_dev *pmcdev, int reg_offset)
>> {
>> return readl(pmcdev->regbase + reg_offset);
>> @@ -435,27 +446,18 @@ static int pmc_core_pll_show(struct seq_file *s, void
>> *unused)
>> }
>> DEFINE_SHOW_ATTRIBUTE(pmc_core_pll);
>>
>> -int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value)
>> +void pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value)
>> {
>> const struct pmc_reg_map *map = pmcdev->map;
>> u32 reg;
>> - int err = 0;
>>
>> mutex_lock(&pmcdev->lock);
>>
>> - if (value > map->ltr_ignore_max) {
>> - err = -EINVAL;
>> - goto out_unlock;
>> - }
>> -
>> reg = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset);
>> reg |= BIT(value);
>> pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, reg);
>>
>> -out_unlock:
>> mutex_unlock(&pmcdev->lock);
>> -
>> - return err;
>> }
>>
>> static ssize_t pmc_core_ltr_ignore_write(struct file *file,
>> @@ -464,6 +466,8 @@ static ssize_t pmc_core_ltr_ignore_write(struct file
>> *file,
>> {
>> struct seq_file *s = file->private_data;
>> struct pmc_dev *pmcdev = s->private;
>> + const struct pmc_reg_map *map = pmcdev->map;
>> + struct ltr_entry *entry;
>> u32 buf_size, value;
>> int err;
>>
>> @@ -473,13 +477,46 @@ static ssize_t pmc_core_ltr_ignore_write(struct file
>> *file,
>> if (err)
>> return err;
>>
>> - err = pmc_core_send_ltr_ignore(pmcdev, value);
>> + if (value > map->ltr_ignore_max)
>> + return -EINVAL;
>> +
>> + mutex_lock(<r_entry_mutex);
>> +
>> + list_for_each_entry(entry, <r_ignore_list, node) {
>> + if (entry->comp_index == value) {
>> + err = -EEXIST;
> Do we need to return an error here? We don't offer a way to undo the ignore and
> rewriting it doesn't hurt anything. I'm okay with ignoring this.
Surely, it won't hurt to just write the value again. It does provide a sense of notion
to the user that "this component was already set" (something like that).
Not that big a deal, but I would like to keep it that way, if that's okay? :)
>
>> + goto out_unlock;
>> + }
>> + }
>> +
>> + entry = devm_kmalloc(&pmcdev->pdev->dev, sizeof(*entry), GFP_KERNEL);
>> + if (!entry) {
>> + err = -ENOMEM;
>> + goto out_unlock;
>> + }
>> +
>> + entry->comp_name = map->ltr_show_sts[value].name;
>> + entry->comp_index = value;
>> + list_add_tail(&entry->node, <r_ignore_list);
>> +
>> + pmc_core_send_ltr_ignore(pmcdev, value);
>> +
>> +out_unlock:
>> + mutex_unlock(<r_entry_mutex);
> You can allocate your entry and do the assignment before you take the list lock.
> If the allocation fails, return immediately without a goto.
>
> You can also move pmc_core_send_ltr_ignore() after the unlock.
Ok, so I allocate it only after I see that the list doesn't already has the value.
That is why I take the lock and proceed.
pmc_core_send_ltr_ignore() can be moved after the unlock.
Please let me know your comments for v5.
Thanks
Rajat
>
> David
>
>>
>> return err == 0 ? count : err;
>> }
>>
>> static int pmc_core_ltr_ignore_show(struct seq_file *s, void *unused)
>> {
>> + struct ltr_entry *entry;
>> +
>> + mutex_lock(<r_entry_mutex);
>> + list_for_each_entry(entry, <r_ignore_list, node) {
>> + seq_printf(s, "%s\n", entry->comp_name);
>> + }
>> + mutex_unlock(<r_entry_mutex);
>> +
>> return 0;
>> }
>>
>> diff --git a/drivers/platform/x86/intel/pmc/core.h
>> b/drivers/platform/x86/intel/pmc/core.h
>> index 810204d758ab..da35b0fcbe6e 100644
>> --- a/drivers/platform/x86/intel/pmc/core.h
>> +++ b/drivers/platform/x86/intel/pmc/core.h
>> @@ -396,7 +396,7 @@ extern const struct pmc_reg_map adl_reg_map;
>> extern const struct pmc_reg_map mtl_reg_map;
>>
>> extern void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev);
>> -extern int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value);
>> +extern void pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value);
>>
>> void spt_core_init(struct pmc_dev *pmcdev);
>> void cnp_core_init(struct pmc_dev *pmcdev);
>> --
>> 2.34.1
>>
Powered by blists - more mailing lists