On 8/14/2021 1:23 AM, Lukas Wunner wrote:
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.
Thank you!
+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.
Because I hope this will also support NPEM as well, since it is so
similar except for using a PCIe extended capability instead of a _DSM
method. This will make it very easy to add the support... I just don't
have any NPEM hardware yet.
+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.
I missed that, thanks!
