Re: [PATCH v2 2/4] uacce: fix isolate sysfs check condition

[huangchenghai]

On Mon, Sep 16, 2025 at 11:15 PM +0800, Greg KH wrote:
> On Tue, Sep 16, 2025 at 10:48:09PM +0800, Chenghai Huang wrote:
> > The uacce supports device isolation feature. If the driver
> > implements the isolate_err_threshold_read and
> > isolate_err_threshold_write callbacks, the uacce will create sysfs
> > files. Users can read and configure isolation policies through
> > sysfs. Currently, if either isolate_err_threshold_read or
> > isolate_err_threshold_write callback exists, sysfs files are
> > created.
> >
> > However, accessing a non-existent callback may cause a system panic.
> Where is the callback happening that fails?  Shouldn't that be checked
> instead of doing this change?
>
> > Therefore, sysfs files are only created when both
> > isolate_err_threshold_read and isolate_err_threshold_write are
> > present.
> What if a device only has 1?  That should still work properly?
>
> And why not just create the file if it is going to be used, that is the
> real solution here.
>
> thanks,
>
> greg k-h
Thank you for your feedback.I agree that the check should be done in the 
corresponding `isolate_strategy_show()` and `isolate_strategy_store()` 
functions.

How about the updated:

@@ -402,6 +402,9 @@ static ssize_t isolate_strategy_show(struct device 
*dev, struct device_attribute
        struct uacce_device *uacce = to_uacce_device(dev);
        u32 val;

+       if (!uacce->ops->isolate_err_threshold_read)
+               return -ENOENT;
+
        val = uacce->ops->isolate_err_threshold_read(uacce);

        return sysfs_emit(buf, "%u\n", val);
@@ -414,6 +417,9 @@ static ssize_t isolate_strategy_store(struct device 
*dev, struct device_attribut
        unsigned long val;
        int ret;

+       if (!uacce->ops->isolate_err_threshold_write)
+               return -ENOENT;
+
        if (kstrtoul(buf, 0, &val) < 0)
                return -EINVAL;

@@ -460,9 +466,7 @@ static umode_t uacce_dev_is_visible(struct kobject 
*kobj,
            (!uacce->qf_pg_num[UACCE_QFRT_DUS])))
                return 0;

-       if (attr == &dev_attr_isolate_strategy.attr &&
-           (!uacce->ops->isolate_err_threshold_read ||
-            !uacce->ops->isolate_err_threshold_write))
+       if (attr == &dev_attr_isolate_strategy.attr)
                return 0;

This way, the sysfs files will only be created if they are going to be 
used, and the checks are done at the appropriate places.

Thanks,
Chenghai