Re: [RFC PATCH v2 1/3] Add support for seven more indicators in enclosure driver

[Bjorn Helgaas <helgaas@kernel.org>]

Follow subject line convention (also applies to other patches):

  $ git log --oneline drivers/misc/enclosure.c
  714f1af14bb0 misc: enclosure: replace snprintf in show functions with sysfs_emit
  6a57251c70a4 misc: enclosure: Update enclosure_remove_device() documentation to match reality
  82f5b473d91a misc: enclosure: Fix some kerneldoc anomalies
  529244bd1afc scsi: enclosure: Fix stale device oops with hot replug
  e3575c1201f0 misc: enclosure: Use struct_size() in kzalloc()
  750b54deb569 misc: enclosure: Remove unnecessary error check

I don't think "seven" is really relevant for the subject or even the
commit log since you list them all anyway.

On Wed, Feb 02, 2022 at 11:59:11AM -0600, Stuart Hayes wrote:
> This patch adds support for seven additional indicators (ok, rebuild,
> prdfail, hotspare, ica, ifa, disabled) to the enclosure driver, which
> currently only supports three (fault, active, locate). It also reduces
> duplicated code for the set and show functions for the sysfs attributes
> for all of the indicators, and allows users of the driver to provide
> common get_led and set_led callbacks to control all indicators (though
> it continues to support the existing callbacks for the three currently
> supported indicators, so it does not require any changes to code that
> already uses the enclosure driver).

This restructures things (replacing get_component_fault(),
set_component_fault(), etc with attributes) and adds things (the new
indicators and also maybe the .get_led() and .set_led() hooks).  It
would be nice to split all those into separate patches.

Also consider using imperative mood:
  https://chris.beams.io/posts/git-commit/
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-tip.rst?id=v5.16#n134

A few more minor comments below.

> @@ -0,0 +1,14 @@
> +What:		/sys/class/enclosure/<enclosure>/<component>/rebuild
> +What:		/sys/class/enclosure/<enclosure>/<component>/disabled
> +What:		/sys/class/enclosure/<enclosure>/<component>/hotspare
> +What:		/sys/class/enclosure/<enclosure>/<component>/ica
> +What:		/sys/class/enclosure/<enclosure>/<component>/ifa
> +What:		/sys/class/enclosure/<enclosure>/<component>/ok
> +What:		/sys/class/enclosure/<enclosure>/<component>/prdfail
> +Date:		February 2022
> +Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
> +Description:
> +		(RW) Get or set the indicator (1 is on, 0 is off) that
> +		corresponds to the attribute name, for a component in an
> +		enclosure. The "ica" and "ifa" states are "in critical
> +		array" and "in failed array".

What's "prdfail"?

> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
> index 1b010d9267c9..485d6a3c655b 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -473,30 +473,6 @@ static const char *const enclosure_type[] = {
>  	[ENCLOSURE_COMPONENT_ARRAY_DEVICE] = "array device",
>  };
>  
> -static ssize_t get_component_fault(struct device *cdev,
> -				   struct device_attribute *attr, char *buf)
> -{
> -	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> -	struct enclosure_component *ecomp = to_enclosure_component(cdev);
> -
> -	if (edev->cb->get_fault)
> -		edev->cb->get_fault(edev, ecomp);
> -	return sysfs_emit(buf, "%d\n", ecomp->fault);
> -}
> -
> -static ssize_t set_component_fault(struct device *cdev,
> -				   struct device_attribute *attr,
> -				   const char *buf, size_t count)
> -{
> -	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> -	struct enclosure_component *ecomp = to_enclosure_component(cdev);
> -	int val = simple_strtoul(buf, NULL, 0);
> -
> -	if (edev->cb->set_fault)
> -		edev->cb->set_fault(edev, ecomp, val);
> -	return count;
> -}
> -
>  static ssize_t get_component_status(struct device *cdev,
>  				    struct device_attribute *attr,char *buf)
>  {
> @@ -531,54 +507,6 @@ static ssize_t set_component_status(struct device *cdev,
>  		return -EINVAL;
>  }
>  
> -static ssize_t get_component_active(struct device *cdev,
> -				    struct device_attribute *attr, char *buf)
> -{
> -	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> -	struct enclosure_component *ecomp = to_enclosure_component(cdev);
> -
> -	if (edev->cb->get_active)
> -		edev->cb->get_active(edev, ecomp);
> -	return sysfs_emit(buf, "%d\n", ecomp->active);
> -}
> -
> -static ssize_t set_component_active(struct device *cdev,
> -				    struct device_attribute *attr,
> -				    const char *buf, size_t count)
> -{
> -	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> -	struct enclosure_component *ecomp = to_enclosure_component(cdev);
> -	int val = simple_strtoul(buf, NULL, 0);
> -
> -	if (edev->cb->set_active)
> -		edev->cb->set_active(edev, ecomp, val);
> -	return count;
> -}
> -
> -static ssize_t get_component_locate(struct device *cdev,
> -				    struct device_attribute *attr, char *buf)
> -{
> -	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> -	struct enclosure_component *ecomp = to_enclosure_component(cdev);
> -
> -	if (edev->cb->get_locate)
> -		edev->cb->get_locate(edev, ecomp);
> -	return sysfs_emit(buf, "%d\n", ecomp->locate);
> -}
> -
> -static ssize_t set_component_locate(struct device *cdev,
> -				    struct device_attribute *attr,
> -				    const char *buf, size_t count)
> -{
> -	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> -	struct enclosure_component *ecomp = to_enclosure_component(cdev);
> -	int val = simple_strtoul(buf, NULL, 0);
> -
> -	if (edev->cb->set_locate)
> -		edev->cb->set_locate(edev, ecomp, val);
> -	return count;
> -}
> -
>  static ssize_t get_component_power_status(struct device *cdev,
>  					  struct device_attribute *attr,
>  					  char *buf)
> @@ -641,29 +569,136 @@ static ssize_t get_component_slot(struct device *cdev,
>  	return sysfs_emit(buf, "%d\n", slot);
>  }
>  
> -static DEVICE_ATTR(fault, S_IRUGO | S_IWUSR, get_component_fault,
> -		    set_component_fault);
> +/*
> + * callbacks for attrs using enum enclosure_component_setting (LEDs)
> + */
> +static ssize_t led_show(struct device *cdev,
> +			enum enclosure_component_led led,
> +			char *buf)
> +{
> +	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> +	struct enclosure_component *ecomp = to_enclosure_component(cdev);
> +
> +	if (edev->cb->get_led)
> +		edev->cb->get_led(edev, ecomp, led);
> +	else

The switch is technically one statement, but it's so long that I think
it really needs braces around it.

> +		/*
> +		 * support old callbacks for fault/active/locate
> +		 */
> +		switch (led) {
> +		case ENCLOSURE_LED_FAULT:
> +			if (edev->cb->get_fault) {
> +				edev->cb->get_fault(edev, ecomp);
> +				ecomp->led[led] = ecomp->fault;
> +			}
> +			break;
> +		case ENCLOSURE_LED_ACTIVE:
> +			if (edev->cb->get_active) {
> +				edev->cb->get_active(edev, ecomp);
> +				ecomp->led[led] = ecomp->active;
> +			}
> +			break;
> +		case ENCLOSURE_LED_LOCATE:
> +			if (edev->cb->get_locate) {
> +				edev->cb->get_locate(edev, ecomp);
> +				ecomp->led[led] = ecomp->locate;
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +
> +	return sysfs_emit(buf, "%d\n", ecomp->led[led]);
> +}
> +
> +static ssize_t led_set(struct device *cdev,
> +		       enum enclosure_component_led led,
> +		       const char *buf, size_t count)
> +{
> +	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> +	struct enclosure_component *ecomp = to_enclosure_component(cdev);
> +	int err, val;
> +
> +	err = kstrtoint(buf, 0, &val);
> +	if (err)
> +		return err;
> +
> +	if (edev->cb->set_led)
> +		edev->cb->set_led(edev, ecomp, led, val);
> +	else

Same here.  Or you could just return early for the cb->set_led case
and unindent the below.

> +		/*
> +		 * support old callbacks for fault/active/locate
> +		 */
> +		switch (led) {
> +		case ENCLOSURE_LED_FAULT:
> +			if (edev->cb->set_fault)
> +				edev->cb->set_fault(edev, ecomp, val);
> +			break;
> +		case ENCLOSURE_LED_ACTIVE:
> +			if (edev->cb->set_active)
> +				edev->cb->set_active(edev, ecomp, val);
> +			break;
> +		case ENCLOSURE_LED_LOCATE:
> +			if (edev->cb->set_locate)
> +				edev->cb->set_locate(edev, ecomp, val);
> +			break;
> +		default:
> +			break;
> +		}

I guess you rely on the callbacks to set ecomp->led[led] (or
ecomp->fault, etc)?

Maybe do the "ecomp->led[led] = ecomp->fault" bits here instead of in
led_show() so that crashdumps will show the same info regardless of
whether led_show() has been run?  ...

Oh, never mind, I see that you only ever update led[led] in the
.get_led() callbacks.  In that case, why do you even *store* the
result in led[led]?  Couldn't you just return it from .get_led()?

> +	return count;
> +}
> +
> +#define LED_ATTR_RW(led_attr, led)					\
> +static ssize_t led_attr##_show(struct device *cdev,			\
> +			       struct device_attribute *attr,		\
> +			       char *buf)				\
> +{									\
> +	return led_show(cdev, led, buf);				\
> +}									\
> +static ssize_t led_attr##_store(struct device *cdev,			\
> +				struct device_attribute *attr,		\
> +				const char *buf, size_t count)		\
> +{									\
> +	return led_set(cdev, led, buf, count);				\
> +}									\
> +static DEVICE_ATTR_RW(led_attr)
> +
>  static DEVICE_ATTR(status, S_IRUGO | S_IWUSR, get_component_status,
>  		   set_component_status);
> -static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, get_component_active,
> -		   set_component_active);
> -static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
> -		   set_component_locate);
>  static DEVICE_ATTR(power_status, S_IRUGO | S_IWUSR, get_component_power_status,
>  		   set_component_power_status);
>  static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
>  static DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
> +LED_ATTR_RW(fault, ENCLOSURE_LED_FAULT);
> +LED_ATTR_RW(active, ENCLOSURE_LED_ACTIVE);
> +LED_ATTR_RW(locate, ENCLOSURE_LED_LOCATE);
> +LED_ATTR_RW(ok, ENCLOSURE_LED_OK);
> +LED_ATTR_RW(rebuild, ENCLOSURE_LED_REBUILD);
> +LED_ATTR_RW(prdfail, ENCLOSURE_LED_PRDFAIL);
> +LED_ATTR_RW(hotspare, ENCLOSURE_LED_HOTSPARE);
> +LED_ATTR_RW(ica, ENCLOSURE_LED_ICA);
> +LED_ATTR_RW(ifa, ENCLOSURE_LED_IFA);
> +LED_ATTR_RW(disabled, ENCLOSURE_LED_DISABLED);
>  
>  static struct attribute *enclosure_component_attrs[] = {
>  	&dev_attr_fault.attr,
>  	&dev_attr_status.attr,
>  	&dev_attr_active.attr,
>  	&dev_attr_locate.attr,
> +	&dev_attr_ok.attr,
> +	&dev_attr_rebuild.attr,
> +	&dev_attr_prdfail.attr,
> +	&dev_attr_hotspare.attr,
> +	&dev_attr_ica.attr,
> +	&dev_attr_ifa.attr,
> +	&dev_attr_disabled.attr,
>  	&dev_attr_power_status.attr,
>  	&dev_attr_type.attr,
>  	&dev_attr_slot.attr,
>  	NULL
>  };
> +

Spurious diff?  I see both with and without this blank line, but seems
more common without.

>  ATTRIBUTE_GROUPS(enclosure_component);

>  
>  static int __init enclosure_init(void)
> diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
> index 1c630e2c2756..98dd0f05efb7 100644
> --- a/include/linux/enclosure.h
> +++ b/include/linux/enclosure.h
> @@ -49,6 +49,20 @@ enum enclosure_component_setting {
>  	ENCLOSURE_SETTING_BLINK_B_OFF_ON = 7,
>  };
>  
> +enum enclosure_component_led {
> +	ENCLOSURE_LED_FAULT,
> +	ENCLOSURE_LED_ACTIVE,
> +	ENCLOSURE_LED_LOCATE,
> +	ENCLOSURE_LED_OK,
> +	ENCLOSURE_LED_REBUILD,
> +	ENCLOSURE_LED_PRDFAIL,
> +	ENCLOSURE_LED_HOTSPARE,
> +	ENCLOSURE_LED_ICA,
> +	ENCLOSURE_LED_IFA,
> +	ENCLOSURE_LED_DISABLED,
> +	ENCLOSURE_LED_MAX,
> +};
> +
>  struct enclosure_device;
>  struct enclosure_component;
>  struct enclosure_component_callbacks {
> @@ -72,6 +86,13 @@ struct enclosure_component_callbacks {
>  	int (*set_locate)(struct enclosure_device *,
>  			  struct enclosure_component *,
>  			  enum enclosure_component_setting);
> +	void (*get_led)(struct enclosure_device *edev,
> +			struct enclosure_component *ecomp,
> +			enum enclosure_component_led);
> +	int (*set_led)(struct enclosure_device *edev,
> +		       struct enclosure_component *ecomp,
> +		       enum enclosure_component_led,
> +		       enum enclosure_component_setting);
>  	void (*get_power_status)(struct enclosure_device *,
>  				 struct enclosure_component *);
>  	int (*set_power_status)(struct enclosure_device *,
> @@ -90,6 +111,7 @@ struct enclosure_component {
>  	int fault;
>  	int active;
>  	int locate;
> +	int led[ENCLOSURE_LED_MAX];
>  	int slot;
>  	enum enclosure_status status;
>  	int power_status;
> -- 
> 2.31.1
>