On Sun, Mar 23, 2025 at 5:21 AM Krzysztof Wilczyński <kw@xxxxxxxxx> wrote:
> That sad, I am not sure if we really need to explain here how this
> particular store() function works.
Will remove in v6 since it made sense to others w/o comment.
> > +static ssize_t ratelimit_log_enable_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct pci_dev *pdev = to_pci_dev(dev);
> > + bool enable = pdev->aer_report->cor_log_ratelimit.interval != 0;
> > +
> > + return sysfs_emit(buf, "%d\n", enable);
> > +}
>
> Perhaps "status" or "enabled" for the variable name above.
s/enable/enabled in v6.
> > +static ssize_t ratelimit_log_enable_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct pci_dev *pdev = to_pci_dev(dev);
> > + bool enable;
> > + int interval;
>
> I would love if we could add the following here:
>
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
>
> To ensure that only privileged user also sporting a proper set capabilities
> can modify this attribute.
> [...]
> > + static ssize_t \
> > + name##_store(struct device *dev, struct device_attribute *attr, \
> > + const char *buf, size_t count) \
> > +{ \
> > + struct pci_dev *pdev = to_pci_dev(dev); \
> > + int burst; \
>
> Same as earlier comment. Add the following:
>
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
Will add in v6.
Thanks,
Jon
