Re: [RFC 08/10] platform/x86/intel/ifs: Add IFS sysfs interface

["Joseph, Jithu" <jithu.joseph@intel.com>]

On 3/3/2022 4:31 PM, Williams, Dan J wrote:
> On Tue, 2022-03-01 at 11:54 -0800, Jithu Joseph wrote:
>> Implement sysfs interface to trigger ifs test for a targeted core or
>> all cores. For all core testing, the kernel will start testing from core 0
>> and proceed to the next core one after another. After the ifs test on the
>> last core, the test stops until the administrator starts another round of

>> +
>> +/*
>> + * The sysfs interface to check the test status:
>> + * To check the result, for example, cpu0
>> + * cat /sys/devices/system/cpu/cpu0/ifs/details
>> + */
>> +static ssize_t details_show(struct device *dev,
>> +                           struct device_attribute *attr,
>> +                           char *buf)
>> +{
>> +       unsigned int cpu = dev->id;
>> +       int ret;
>> +
>> +       if (down_trylock(&ifs_sem))
>> +               return -EBUSY;
> 
> What is the ifs_sem protecting? This result is immediately invalid
> after the lock is dropped anyway, so why hold it over reading the
> value? You can't prevent 2 threads racing each other here.

percpu thread running scan_test_worker() will update per_cpu(ifs_state, cpu).scan_details. (before signalling this thread to run, this lock would be acquired)
This is to protect against the scenario where if the percpu thread is running a test and if at the same time a user is querying its status, they would see busy.

> 
>> +
>> +       ret = sprintf(buf, "%llx\n", per_cpu(ifs_state, cpu).scan_details);
> 
> Should be sysfs_emit() which includes the page buffer safety.

grep KH also pointed this out ... will replace this throughout

> 
> Also, you likely want that format string to be %#llx so that userspace
> knows explicitly that this is a hexadecimal value.

Agreed will do this


>> +
>> +/*
>> + * The sysfs interface for single core testing
>> + * To start test, for example, cpu0
>> + * echo 1 > /sys/devices/system/cpu/cpu0/ifs/run_test
>> + * To check the result:
>> + * cat /sys/devices/system/cpu/cpu0/ifs/result
(there is a typo in the comment result -> status)
> 
> Just have a CPU mask as an input parameter and avoid needing to hang
> ifs sysfs attributes underneath /sys/device/system/cpu/ifs.

The percpu sysfs has the additional function of providing percpu status and  details. 
The global interface is unable to provide the status and details for all the cores in the system. It does give a summary, which
guides the user to the appropriate percpu status/details

 
>> + */
>> +static ssize_t allcpu_run_test_store(struct device *dev,
>> +                                    struct device_attribute *attr,
>> +                                    const char *buf, size_t count)
>> +{
>> +       bool var;
>> +       int rc;
>> +
>> +       if (ifs_disabled)
>> +               return -ENXIO;
>> +
>> +       rc = kstrtobool(buf, &var);
>> +       if (rc < 0 || var != 1)
>> +               return -EINVAL;
> 
> You could just cut to the chase and do: sysfs_eq(buf, "1")

Thanks will use this

 

>> +
>> +/*
>> + * Percpu and allcpu ifs have attributes named "status".
>> + * Since the former is defined in this same file using DEVICE_ATTR_RO()
>> + * the latter is defined directly.
>> + */
>> +static struct device_attribute dev_attr_allcpu_status = {
>> +       .attr   = { .name = "status", .mode = 0444 },
>> +       .show   = allcpu_status_show,
>> +};
> 
> Can still do the one line declartion like this and skip the comment.
> 
> DEVICE_ATTR(status, 0444, allcpu_status_show, NULL);

Will change as per your suggestion above

> 
>> +
>> +/* global scan sysfs attributes */
>> +static struct attribute *cpu_ifs_attrs[] = {
>> +       &dev_attr_reload.attr,
>> +       &dev_attr_allcpu_run_test.attr,
>> +       &dev_attr_image_version.attr,
>> +       &dev_attr_cpu_fail_list.attr,
>> +       &dev_attr_cpu_untested_list.attr,
>> +       &dev_attr_cpu_pass_list.attr,
>> +       &dev_attr_allcpu_status.attr,
>> +       NULL
>> +};
>> +
>> +const struct attribute_group cpu_ifs_attr_group = {
> 
> static?

will do

Jithu