Re: [PATCH] platform/x86/intel/pmc: core: Add support to show LTR-ignored components

["Box, David E" <david.e.box@xxxxxxxxx>]

Hi,

On Fri, 2023-01-13 at 19:32 +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>
> ---
>  drivers/platform/x86/intel/pmc/core.c | 47 ++++++++++++++++++++-------
>  1 file changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/core.c
> b/drivers/platform/x86/intel/pmc/core.c
> index a1fe1e0dcf4a..30fff4461807 100644
> --- a/drivers/platform/x86/intel/pmc/core.c
> +++ b/drivers/platform/x86/intel/pmc/core.c
> @@ -129,6 +129,14 @@ static const struct pmc_bit_map *ext_spt_pfear_map[] = {
>         NULL
>  };
>  
> +struct ltr_entry {
> +       u32 comp_index;
> +       const char *comp_name;
> +       struct list_head node;
> +};
> +
> +static LIST_HEAD(ltr_ignore_list);
> +
>  static const struct pmc_bit_map spt_ltr_show_map[] = {
>         {"SOUTHPORT_A",         SPT_PMC_LTR_SPA},
>         {"SOUTHPORT_B",         SPT_PMC_LTR_SPB},
> @@ -1327,27 +1335,18 @@ static int pmc_core_pll_show(struct seq_file *s, void
> *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_pll);
>  
> -static int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value)
> +static 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,
> @@ -1356,6 +1355,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;
>  
> @@ -1365,13 +1366,35 @@ 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;
>  
> -       return err == 0 ? count : err;
> +       list_for_each_entry(entry, &ltr_ignore_list, node) {
> +               if (entry->comp_index == value)
> +                       return -EEXIST;
> +       }
> +
> +       entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> +       if (!entry)
> +               return -ENOMEM;

Currently will leak memory when the driver is removed. struct pmc_dev has a
pointer to the platform device so you can use devm_kmalloc.

> +
> +       entry->comp_name = map->ltr_show_sts[value].name;
> +       entry->comp_index = value;
> +       list_add_tail(&entry->node, &ltr_ignore_list);
> +
> +       pmc_core_send_ltr_ignore(pmcdev, value);
> +
> +       return count;
>  }
>  
>  static int pmc_core_ltr_ignore_show(struct seq_file *s, void *unused)
>  {
> +       struct ltr_entry *entry;
> +
> +       list_for_each_entry(entry, &ltr_ignore_list, node) {
> +               seq_printf(s, "%s\n", entry->comp_name);
> +       }

It would be helpful to add a first column that shows the index. But I guess we
don't show this in ltr_show either.

David

> +
>         return 0;
>  }
>