Re: [PATCH v5 8/8] PCI/AER: Add sysfs attributes for log ratelimits

["Krzysztof Wilczyński" <kw@linux.com>]

Hello,

A few small comments.

> +/*
> + * Ratelimit enable toggle
> + * 0: disabled with ratelimit.interval = 0
> + * 1: enabled with ratelimit.interval = nonzero
> + */

Move the above comment so it hugs ratelimit_log_enable_store().  Also,
perhaps "Ratelimit enable toggle:", while at it, and feel free to indent
both choices for readability.

That sad, I am not sure if we really  need to explain here how this
particular store() function works.

> +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.

> +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.

> +	if (kstrtobool(buf, &enable) < 0)
> +		return -EINVAL;
> +
> +	if (enable)
> +		interval = DEFAULT_RATELIMIT_INTERVAL;
> +	else
> +		interval = 0;
> +
> +	pdev->aer_report->cor_log_ratelimit.interval = interval;
> +	pdev->aer_report->uncor_log_ratelimit.interval = interval;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(ratelimit_log_enable);

[...]
> +	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;

> +	if (kstrtoint(buf, 0, &burst) < 0)				\
> +		return -EINVAL;						\
> +									\
> +	pdev->aer_report->ratelimit.burst = burst;			\
> +									\
> +	return count;							\
> +}									\
> +static DEVICE_ATTR_RW(name)

Thank you!

	Krzysztof