[MODERATED] Re: [PATCH v4 04/10] TAAv4 4

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Sep 24, 2019 at 04:25:03PM -0700, speck for Pawan Gupta wrote:
> On Tue, Sep 24, 2019 at 07:04:59AM +0200, speck for Greg KH wrote:
> > Either way, my original comment of "do not use sysfs_create_group() when
> > you have a 'struct device'" still stands, that needs to be fixed up no
> > matter what here.
> 
> Thanks for your inputs. As we have the 'struct device' are you
> suggesting to use device_create_file()?

You should never use sysfs_create_file() if you have a 'struct device'
so yes, that's at least one thing that needs to be fixed.

> Approach 1 - sysfs_create_file() => device_create_file()
> 
> -----------------
> diff --git a/arch/x86/kernel/cpu/tsx.c b/arch/x86/kernel/cpu/tsx.c
> index 40fec8ab82b1..4799c081198c 100644
> --- a/arch/x86/kernel/cpu/tsx.c
> +++ b/arch/x86/kernel/cpu/tsx.c
> @@ -162,7 +162,7 @@ static void tsx_update_on_each_cpu(bool val)
>  	put_online_cpus();
>  }
>  
> -static ssize_t tsx_show(struct device *cdev,
> +static ssize_t htm_show(struct device *cdev,
>  			struct device_attribute *attr,
>  			char *buf)
>  {
> @@ -171,7 +171,7 @@ static ssize_t tsx_show(struct device *cdev,
>  
>  static DEFINE_MUTEX(tsx_mutex);
>  
> -static ssize_t tsx_store(struct device *cdev,
> +static ssize_t htm_store(struct device *cdev,
>  			 struct device_attribute *attr,
>  			 const char *buf, size_t count)
>  {
> @@ -197,15 +197,14 @@ static ssize_t tsx_store(struct device *cdev,
>  	return count;
>  }
>  
> -static DEVICE_ATTR_RW(tsx);
> +static const DEVICE_ATTR_RW(htm);
>  
>  static int __init tsx_sysfs_init(void)
>  {
>  	if (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED)
>  		return -ENODEV;
>  
> -	return sysfs_create_file(&cpu_subsys.dev_root->kobj,
> -				 &dev_attr_tsx.attr);
> +	return device_create_file(cpu_subsys.dev_root, &dev_attr_htm);
>  }
>  
> -late_initcall(tsx_sysfs_init);
> +device_initcall(tsx_sysfs_init);

You are renaming the file here too, but that's fine, it's not my file :)

> -------------------------------------------------------------------
> 
> If the first approach is not what you are expecting, is the below
> patch to add the attribute to the cpu_root_attrs list okay?
> 
> Approach 2 - Add tsx attribute to cpu_root_attrs in drivers/base/cpu.c
> 
> --------------------
> diff --git a/arch/x86/kernel/cpu/tsx.c b/arch/x86/kernel/cpu/tsx.c
> index da75021daa1d..631996e129d5 100644
> --- a/arch/x86/kernel/cpu/tsx.c
> +++ b/arch/x86/kernel/cpu/tsx.c
> @@ -158,17 +158,15 @@ static void tsx_update_on_each_cpu(bool val)
>  	put_online_cpus();
>  }
>  
> -static ssize_t tsx_show(struct device *cdev,
> -			struct device_attribute *attr,
> -			char *buf)
> +ssize_t htm_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	return sprintf(buf, "%d\n", tsx_ctrl_state == TSX_CTRL_ENABLE ? 1 : 0);
>  }
>  
>  static DEFINE_MUTEX(tsx_mutex);
>  
> -static ssize_t tsx_store(struct device *cdev, struct device_attribute *attr,
> -			 const char *buf, size_t count)
> +ssize_t htm_store(struct device *dev, struct device_attribute *attr,
> +		  const char *buf, size_t count)
>  {
>  	enum tsx_ctrl_states requested_state;
>  	ssize_t ret;
> @@ -229,32 +227,10 @@ static ssize_t tsx_store(struct device *cdev, struct device_attribute *attr,
>  	return count;
>  }
>  
> -static DEVICE_ATTR_RW(tsx);
> -
> -static struct attribute *tsx_ctrl_attrs[] = {
> -	&dev_attr_tsx.attr,
> -	NULL
> -};
> -
> -static umode_t tsx_ctrl_attr_is_visible(struct kobject *kobj,
> -					struct attribute *attr, int index)
> +umode_t htm_is_visible(void)
>  {
> -	if ((attr == &dev_attr_tsx.attr) &&
> -	    (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED))
> +	if (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED)
>  		return 0;
>  
> -	return attr->mode;
> +	return 0644;
>  }
> -
> -static struct attribute_group tsx_ctrl_attr_group = {
> -	.attrs		= tsx_ctrl_attrs,
> -	.is_visible	= tsx_ctrl_attr_is_visible,
> -};
> -
> -static int __init tsx_sysfs_init(void)
> -{
> -	return sysfs_create_group(&cpu_subsys.dev_root->kobj,
> -				  &tsx_ctrl_attr_group);
> -}
> -
> -late_initcall(tsx_sysfs_init);
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 80d43ff9a7ec..2b1579c7f22d 100644
> --- a/drivers/base/cpu.c
> +++ b/drivers/base/cpu.c
> @@ -460,6 +460,34 @@ struct device *cpu_device_create(struct device *parent, void *drvdata,
>  }
>  EXPORT_SYMBOL_GPL(cpu_device_create);
>  
> +ssize_t __weak htm_show(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	return -ENODEV;
> +}
> +
> +ssize_t __weak htm_store(struct device *dev, struct device_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	return -ENODEV;
> +}
> +
> +DEVICE_ATTR_RW(htm);
> +
> +umode_t __weak htm_is_visible(void)
> +{
> +	return 0;
> +}
> +
> +static umode_t cpu_root_attrs_is_visible(struct kobject *kobj,
> +					 struct attribute *attr, int index)
> +{
> +	if (attr == &dev_attr_htm.attr)
> +		return htm_is_visible();
> +
> +	return attr->mode;
> +}
> +
>  #ifdef CONFIG_GENERIC_CPU_AUTOPROBE
>  static DEVICE_ATTR(modalias, 0444, print_cpu_modalias, NULL);
>  #endif
> @@ -481,11 +509,13 @@ static struct attribute *cpu_root_attrs[] = {
>  #ifdef CONFIG_GENERIC_CPU_AUTOPROBE
>  	&dev_attr_modalias.attr,
>  #endif
> +	&dev_attr_htm.attr,
>  	NULL
>  };
>  
>  static struct attribute_group cpu_root_attr_group = {
> -	.attrs = cpu_root_attrs,
> +	.attrs		= cpu_root_attrs,
> +	.is_visible	= cpu_root_attrs_is_visible,
>  };
>  
>  static const struct attribute_group *cpu_root_attr_groups[] = {


I like this second one a _lot_ better.  You are creating the file at the
correct time, in the correct location, and handling if you should expose
the file or not to userspace correctly.  Nice work!

thanks,

greg k-h