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@xxxxxxxxxxxxxxx>
> > > ---
> > >
> > > 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
> > >
