Message-ID: <20240903221820.GA26364@bhelgaas>
Date: Tue, 3 Sep 2024 17:18:20 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Tony Hutter <hutter2@...l.gov>
Cc: bhelgaas@...gle.com, minyard@....org, linux-pci@...r.kernel.org,
	openipmi-developer@...ts.sourceforge.net,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] PCI: Introduce Cray ClusterStor E1000 NVMe slot LED
 driver

On Tue, Aug 27, 2024 at 02:03:48PM -0700, Tony Hutter wrote:
> Add driver to control the NVMe slot LEDs on the Cray ClusterStor E1000.
> The driver provides hotplug attention status callbacks for the 24 NVMe
> slots on the E1000.  This allows users to access the E1000's locate and
> fault LEDs via the normal /sys/bus/pci/slots/<slot>/attention sysfs
> entries.  This driver uses IPMI to communicate with the E1000 controller to
> toggle the LEDs.

I hope/assume the interface is the same as one of the others, i.e.,
the existing one added for NVMe behind VMD by
https://git.kernel.org/linus/576243b3f9ea ("PCI: pciehp: Allow
exclusive userspace control of indicators") or the new one for NPEM
and the _DSM at
https://lore.kernel.org/linux-pci/20240814122900.13525-3-mariusz.tkaczyk@linux.intel.com/

I suppose we intend that the ledmon utility will be able to drive
these LEDs?  Whatever the user, we should try to minimize the number
of different interfaces for this functionality.

A few minor random comments from a quick look below.

> +config HOTPLUG_PCI_PCIE_CRAY_E1000
> +	tristate "PCIE Hotplug extensions for Cray ClusterStor E1000"

s/PCIE/PCIe/

> +static ssize_t craye1k_show(struct kobject *kobj, struct kobj_attribute *kattr,
> +			    char *buf);
> +static ssize_t craye1k_store(struct kobject *kobj, struct kobj_attribute *kattr,
> +			     const char *buf,
> +			     size_t count);
> +static void craye1k_new_smi(int iface, struct device *dev);
> +static void craye1k_smi_gone(int iface);
> +static void craye1k_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data);

Is it possible to reorder the function implementations such that these
forward declarations are not needed?  That's the typical Linux style, so
that ordering will be more familiar to readers.

> +static atomic64_t *craye1k_lookup_stat(struct kobject *kobj, const char *name)
> +{
> +	struct craye1k *craye1k;
> +	struct device *dev;
> +	int i;
> +
> +	/* Lookup table for name -> atomic64_t offset */
> +	const struct {
> +		const char *name;
> +		size_t offset;
> +	} table[] = {
> +		CRAYE1K_TABLE(check_primary),
> +		CRAYE1K_TABLE(check_primary_failed),
> +		CRAYE1K_TABLE(was_already_primary),
> +		CRAYE1K_TABLE(was_not_already_primary),
> +		CRAYE1K_TABLE(set_primary),
> +		CRAYE1K_TABLE(set_initial_primary_failed),
> +		CRAYE1K_TABLE(set_primary_failed),
> +		CRAYE1K_TABLE(set_led_locate_failed),
> +		CRAYE1K_TABLE(set_led_fault_failed),
> +		CRAYE1K_TABLE(set_led_readback_failed),
> +		CRAYE1K_TABLE(set_led_failed),
> +		CRAYE1K_TABLE(get_led_failed),
> +		CRAYE1K_TABLE(completion_timeout),
> +		CRAYE1K_TABLE(wrong_msgid),
> +		CRAYE1K_TABLE(request_failed)
> +	};

Looks like possibly this table could be static instead of being on the
stack?

> + * __craye1k_set_primary() - Tell the BMC we want to be the primary server
> + *
> + * An E1000 board has two physical servers on it.  In order to set a slot
> + * NVMe LED, this server needs to first tell the BMC that it's the primary
> + * server.
> + *
> + * Returns: 0 on success, 1 otherwise.
> + */
> +

Spurious blank line.

> +static int __craye1k_set_primary(struct craye1k *craye1k)

> + * craye1k_is_primary() - Are we the primary server?
> + *
> + * Returns: 1 if we are the primary server, 0 otherwise.
> + */
> +static int craye1k_is_primary(struct craye1k *craye1k)
> +{
> +	u8 byte = 0;
> +	int rc;
> +
> +	/* Response byte is 0x1 on success */
> +	rc = craye1k_do_command(craye1k, CRAYE1K_CMD_PRIMARY, &byte, 1);
> +	atomic64_inc(&craye1k->check_primary);
> +	if (rc == 0x1)
> +		return 1;   /* success */
> +
> +	atomic64_inc(&craye1k->check_primary_failed);
> +	return 0;   /* We are not the primary server node */
> +}
> +
> +/*
> + * craye1k_set_primary() - Attempt to set ourselves as the primary server
> + *
> + * Returns: 0 on success, 1 otherwise.

Maybe return a negative error value like -EIO for failure?  Then the
caller can simply pass that return value up.  Same for
__craye1k_set_primary().

> +	 * We know that our attention status callback functions have been swapped
> +	 * into the PCI device's hotplug_slot->ops values.  We can use that
> +	 * knowledge to lookup our craye1k.
> +	 *
> +	 * To do that, we use the current hotplug_slot->ops value, which is going
> +	 * to be one of the entries in craye1k->ops[], and offset our slot number
> +	 * to get the address of craye1k->ops[0].  We then use that with
> +	 * container_of() to get craye1k.  Slots start at 1, so account for that.

99% of this file fits in 80 columns.  This and one or two other
comments use 81, which seems like a random width.  Can you reflow
these to fit in 80?

> +static int __craye1k_get_attention_status(struct hotplug_slot *hotplug_slot,
> +					  u8 *status, bool set_primary)
> +{
> +	unsigned char slot;
> +	int locate, fault;
> +	int rc = 0;
> +	struct craye1k *craye1k;
> +
> +	slot = PSN(to_ctrl(hotplug_slot));
> +	if (!(slot >= 1 && slot <= 24)) {
> +		rc = -EINVAL;
> +		goto out;

There's no cleanup at "out", so drop the "rc" and the label, use
"return -EINVAL/-EIO/etc " directly here, and then "return 0" at the
end.

> +	}
> +
> +	craye1k = craye1k_from_hotplug_slot(hotplug_slot);
> +
> +	if (set_primary) {
> +		if (craye1k_set_primary(craye1k) != 0) {
> +			rc = -EIO;
> +			goto out;
> +		}
> +	}
> +
> +	locate = craye1k_get_slot_led(craye1k, slot, true);
> +	if (locate == -1) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	fault = craye1k_get_slot_led(craye1k, slot, false);
> +	if (fault == -1) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (rc != 0)
> +		atomic64_inc(&craye1k->get_led_failed);
> +
> +	*status = locate << 1 | fault;
> +
> +out:
> +	return rc;
> +}

Powered by blists - more mailing lists