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

[huangchenghai <huangchenghai2@huawei.com>]

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