Re: [PATCH v3] Add support for PCIe SSD status LED management

[Lukas Wunner <lukas@wunner.de>]

On Fri, Aug 13, 2021 at 05:36:53PM -0400, Stuart Hayes wrote:
> +struct mutex drive_status_dev_list_lock;
> +struct list_head drive_status_dev_list;

Should be declared static.

> +const guid_t pcie_ssd_leds_dsm_guid =
> +	GUID_INIT(0x5d524d9d, 0xfff9, 0x4d4b,
> +		  0x8c, 0xb7, 0x74, 0x7e, 0xd5, 0x1e, 0x19, 0x4d);

Same.

> +struct drive_status_led_ops dsm_drive_status_led_ops = {
> +	.get_supported_states = get_supported_states_dsm,
> +	.get_current_states = get_current_states_dsm,
> +	.set_current_states = set_current_states_dsm,
> +};

Same.

> +static void probe_pdev(struct pci_dev *pdev)
> +{
> +	/*
> +	 * This is only supported on PCIe storage devices and PCIe ports
> +	 */
> +	if (pdev->class != PCI_CLASS_STORAGE_EXPRESS &&
> +	    pdev->class != PCI_CLASS_BRIDGE_PCI)
> +		return;
> +	if (pdev_has_dsm(pdev))
> +		add_drive_status_dev(pdev, &dsm_drive_status_led_ops);
> +}

Why is &dsm_drive_status_led_ops passed to add_drive_status_dev()?
It's always the same argument.

> +static int __init ssd_leds_init(void)
> +{
> +	mutex_init(&drive_status_dev_list_lock);
> +	INIT_LIST_HEAD(&drive_status_dev_list);
> +
> +	bus_register_notifier(&pci_bus_type, &ssd_leds_pci_bus_nb);
> +	initial_scan_for_leds();
> +	return 0;
> +}

There's a concurrency issue here:  initial_scan_for_leds() uses
bus_for_each_dev(), which walks the bus's klist_devices.  When a
device is added (e.g. hotplugged), that device gets added to the
tail of klist_devices.  (See call to klist_add_tail() in
bus_add_device().)

It is thus possible that probe_pdev() is run concurrently for the
same device, once by the notifier and once by initial_scan_for_leds().

The problem is that add_drive_status_dev() only checks at the top
of the function whether the device has already been added to
drive_status_dev_list.  It goes on to instantiate the LED
and only then adds the device to drive_status_dev_list.

It's thus possible that the same LED is instantiated twice
which might confuse users.

Thanks,

Lukas