| Message ID | 20230205174418.1927876-1-rajat.khandelwal@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4] platform/x86/intel/pmc: core: Add support to show LTR-ignored components | expand |
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@linux.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. > + 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. 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 >
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@linux.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 >>
Hi Rajat, On Thu, 2023-02-09 at 19:00 +0530, Rajat Khandelwal wrote: > 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@linux.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? :) Okay. It is a new error being returned for something that used to be allowed. Please add it to the commit message. > > > > > > + 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. Ah, I missed that. > pmc_core_send_ltr_ignore() can be moved after the unlock. Yes. David > > 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 > > >
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; + 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); 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);
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@linux.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(-)