From: Bjorn Helgaas <helgaas@kernel.org>
To: Stuart Hayes <stuart.w.hayes@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
linux-pci@vger.kernel.org,
Dan Williams <dan.j.williams@intel.com>,
Keith Busch <kbusch@kernel.org>,
kw@linux.com, mariusz.tkaczyk@linux.intel.com, lukas@wunner.de,
pavel@ucw.cz, linux-cxl@vger.kernel.org,
martin.petersen@oracle.com,
James.Bottomley@hansenpartnership.com,
Arnd Bergmann <arnd@arndb.de>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [RFC PATCH v2 1/3] Add support for seven more indicators in enclosure driver
Date: Wed, 2 Feb 2022 17:55:58 -0600 [thread overview]
Message-ID: <20220202235558.GA54005@bhelgaas> (raw)
In-Reply-To: <d71f2483ed6cb6ea74a267301982c14e6bfa79a7.1643822289.git.stuart.w.hayes@gmail.com>
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
>
next prev parent reply other threads:[~2022-02-02 23:56 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-02 17:59 [RFC PATCH v2 0/3] Add PCIe enclosure management support Stuart Hayes
2022-02-02 17:59 ` [RFC PATCH v2 1/3] Add support for seven more indicators in enclosure driver Stuart Hayes
2022-02-02 23:55 ` Bjorn Helgaas [this message]
2022-02-02 17:59 ` [RFC PATCH v2 2/3] Add PCIe enclosure management auxiliary driver Stuart Hayes
2022-02-03 0:58 ` Bjorn Helgaas
2022-02-02 17:59 ` [RFC PATCH v2 3/3] Register auxiliary device for PCIe enclosure management Stuart Hayes
2022-02-10 11:52 ` [RFC PATCH v2 0/3] Add PCIe enclosure management support Mariusz Tkaczyk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220202235558.GA54005@bhelgaas \
--to=helgaas@kernel.org \
--cc=James.Bottomley@hansenpartnership.com \
--cc=arnd@arndb.de \
--cc=bhelgaas@google.com \
--cc=dan.j.williams@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=kbusch@kernel.org \
--cc=kw@linux.com \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=mariusz.tkaczyk@linux.intel.com \
--cc=martin.petersen@oracle.com \
--cc=pavel@ucw.cz \
--cc=stuart.w.hayes@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).