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
