Re: [PATCH v5 1/3] uacce: supports device isolation feature

[yekai(A)]

On 2022/7/8 15:28, Greg KH wrote:
> On Fri, Jul 08, 2022 at 03:08:18PM +0800, Kai Ye wrote:
> > UACCE adds the hardware error isolation API. Users can configure
> > the isolation frequency by this sysfs node. UACCE reports the device
> > isolate state to the user space. If the AER error frequency exceeds
> > the value of setting for a certain period of time, the device will be
> > isolated.
> >
> > Signed-off-by: Kai Ye <yekai13@xxxxxxxxxx>
> > ---
> >  drivers/misc/uacce/uacce.c | 55 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/uacce.h      | 11 ++++++++
> >  2 files changed, 66 insertions(+)
> >
> > diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
> > index 281c54003edc..d07b5f1f0596 100644
> > --- a/drivers/misc/uacce/uacce.c
> > +++ b/drivers/misc/uacce/uacce.c
> > @@ -7,6 +7,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/uacce.h>
> >
> > +#define MAX_ERR_ISOLATE_COUNT		65535
> > +
> >  static struct class *uacce_class;
> >  static dev_t uacce_devt;
> >  static DEFINE_MUTEX(uacce_mutex);
> > @@ -339,12 +341,63 @@ static ssize_t region_dus_size_show(struct device *dev,
> >  		       uacce->qf_pg_num[UACCE_QFRT_DUS] << PAGE_SHIFT);
> >  }
> >
> > +static ssize_t isolate_show(struct device *dev,
> > +			    struct device_attribute *attr, char *buf)
> > +{
> > +	struct uacce_device *uacce = to_uacce_device(dev);
> > +
> > +	if (!uacce->ops->get_isolate_state)
> > +		return -ENODEV;
>
> If there is no callback, why is this sysfs even created at all?  Please
> do not create it if it can not be accessed.
>
> Use the is_visable() callback for the group to do this.

If is_visable() is used as the judgment, all uacce device nodes cannot 
be registered if there is no callback by test.

> > +
> > +	return sysfs_emit(buf, "%d\n", uacce->ops->get_isolate_state(uacce));
> > +}
> > +
> > +static ssize_t isolate_strategy_show(struct device *dev,
> > +				     struct device_attribute *attr, char *buf)
> > +{
> > +	struct uacce_device *uacce = to_uacce_device(dev);
> > +	u32 val;
> > +
> > +	if (!uacce->ops->isolate_strategy_read)
> > +		return -ENODEV;
>
> Same here, don't have a sysfs file that does nothing.
>
> > +
> > +	val = uacce->ops->isolate_strategy_read(uacce);
> > +	if (val > MAX_ERR_ISOLATE_COUNT)
> > +		return -EINVAL;
> > +
> > +	return sysfs_emit(buf, "%u\n", val);
> > +}
> > +
> > +static ssize_t isolate_strategy_store(struct device *dev,
> > +				      struct device_attribute *attr,
> > +				      const char *buf, size_t count)
> > +{
> > +	struct uacce_device *uacce = to_uacce_device(dev);
> > +	unsigned long val;
> > +	int ret;
> > +
> > +	if (!uacce->ops->isolate_strategy_write)
> > +		return -ENODEV;
>
> Same here.
>
> thanks,
>
> greg k-h
> .