Most changes suggested in the code review

including one to avoid leaking pci_dev structs in
count_pci_device_instances()

Author: DanielGibson

README.md

@@ -59,9 +59,9 @@ The following DMI system-manufacturer / system-product-name combinations are cur
 
 - LEDs (front panel, disk)
   - Represented as subdirectories in `/sys/class/leds/`
-    - In the subdirectories, you can set `brightness` to 0 or 1 to switch the LED off or on, for example  
-      `$ echo 1 | sudo tee blue:power/brightness`  
-      similarly, the `trigger` can be configured, see [below](#set-triggers-for-leds)
+    - In the subdirectories, you can set `brightness` to 0 or 1 to switch the LED off or on
+      (for example `echo 1 | sudo tee blue:power/brightness`). Similarly, the `trigger` can
+      also be configured, see [below](#set-triggers-for-leds).
     - Sometimes the name of an LED doesn't exactly represent its color, for example, on the
       *Flashstor FS6712X*, the `blue:lan` LED is actually purple when connected with 10GBit
       (but blue when connected with 1GBit). Also, sometimes two LEDs physically appear as one, so
@@ -154,6 +154,8 @@ echo r8169-0-200:00:link > /sys/class/leds/blue\:lan
 one being marked with square brackes (e.g. `[none]  kbd-scrolllock kbd-numlock kbd-capslock ...`).
 
 Note that currently the disk-related triggers (like `disk-activity`) do **not** work with NVME drives.
+That's a general limitation of the Linux kernel that is independent of this project.
+If this feature is ever implemented in the kernel, it will automatically work with this driver.
 
 ### `it87` and PWM polarity
 
@@ -168,19 +170,21 @@ Note that `it87` conflicts with `asustor-it87`, you may wish to add `it87` to th
 If the `asustor` kernel module doesn't detect your device correctly, you can force it to treat your
 ASUSTOR device as one of the supported devices by setting the `force_device` module parameter.
 
-can be used like:  
-`$ sudo modprobe asustor force_device=AS66xx`  
-or by creating an `/etc/modprobe.d/asustor.conf` text file with the following content:
+This can be done manually with `sudo modprobe asustor force_device=AS66xx`, or by creating
+`/etc/modprobe.d/asustor.conf` with the following content:
 
 ```
 # override device detection of the asustor kernel module
 options asustor force_device=FS67xx
 ```
 
-Of course you should replace "FS67xx" with the device you want to try, see the [Compatiblity](#compatibility)
-section above for how the `asustor` kernel module identifies devices, or  
-`$ sudo modinfo -p asustor`  
-which will print a short usage info including the currently supported device names for `force_device`.
+Please replace "FS67xx" with the device you want to try.  
+See the [Compatiblity](#compatibility)-section above for how the `asustor` kernel module identifies devices.
+Alternatively, can use the following command to print module parameters, including the currently supported device names for `force_device`:
+
+```console
+$ sudo modinfo -p asustor
+```
 
 _**NOTE:** If you need to use the `force_device` parameter to make your device work, please open an issue
 so the detection logic in the `asustor` kernel module can be fixed to properly support it._

asustor.c

@@ -248,6 +248,10 @@ struct pci_device_match {
 	int16_t max_count; // how often that device should exist at most
 };
 
+enum {
+	DEVICE_COUNT_MAX = 0x7fff // INT16_MAX - used for "no upper limit"
+};
+
 // ASUSTOR Platform.
 struct asustor_driver_data {
 	const char *name; // used for force_device and for some log messages
@@ -268,8 +272,8 @@ static struct asustor_driver_data asustor_fs6700_driver_data = {
 	.pci_matches = {
 		// PCI bridge: ASMedia Technology Inc. ASM2806 4-Port PCIe x2 Gen3 Packet Switch (rev 01)
 		// FS6712X seems to have 15 of these, no idea about FS6706T (so I keep min_count at 1)
-		// currently the upper limit doesn't matter so I just use 10000
-		{ 0x1b21, 0x2806, 1, 10000 },
+		// currently the upper limit doesn't matter so I just use DEVICE_COUNT_MAX
+		{ 0x1b21, 0x2806, 1, DEVICE_COUNT_MAX },
 	},
 	.leds = &asustor_fs6700_gpio_leds_lookup,
 	.keys = &asustor_fs6700_gpio_keys_lookup,
@@ -280,7 +284,8 @@ static struct asustor_driver_data asustor_6700_driver_data = {
 	// AS67xx needs to match PCI devices because it has the same DMI data as *F*S67xx
 	.pci_matches = {
 		// PCI bridge: ASMedia Technology Inc. ASM2806 4-Port PCIe x2 Gen3 Packet Switch (rev 01)
-		// *F*S67xx seems to use these, I think *A*S67xx doesn't, so expect 0
+		// *F*S67xx seems to use these, *A*S67xx doesn't, so expect 0
+		// (BTW, AS6706T has 5x "ASM2812 6-Port PCIe x4 Gen3 Packet Switch" [1b21:2812], I hope *F*S6706T doesn't)
 		{ 0x1b21, 0x2806, 0, 0 },
 	},
 	.leds = &asustor_6700_gpio_leds_lookup,
@@ -319,7 +324,7 @@ static const struct dmi_system_id asustor_systems[] = {
 	// NOTE: each entry in this table must have its own unique asustor_driver_data
 	//       (having a unique .name) set as .driver_data
 	{
-		// Note: yes, this is the same DMI match as the next entry, because just by DMI,
+		// NOTE: yes, this is the same DMI match as the next entry, because just by DMI,
 		//       FS67xx and AS67xx can't be told apart. But our custom matching logic
 		//       in find_matching_asustor_system() also takes driver_data->pci_matches[]
 		//       into account, so that should be ok.
@@ -330,7 +335,7 @@ static const struct dmi_system_id asustor_systems[] = {
 		.driver_data = &asustor_fs6700_driver_data,
 	},
 	{
-		// Note: This not only matches (and works with) AS670xT (Lockerstor Gen2),
+		// NOTE: This not only matches (and works with) AS670xT (Lockerstor Gen2),
 		//       but also AS540xT (Nimbustor Gen2)
 		.matches = {
 			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Intel Corporation"),
@@ -442,22 +447,28 @@ static bool dmi_matches(const struct dmi_system_id *dmi)
 	return true;
 }
 
-// how often does the PCI(e) device with given vendor/device IDs exist in this system?
+// How many PCI(e) devices with given vendor/device IDs exist in this system?
 static int count_pci_device_instances(unsigned int vendor, unsigned int device)
 {
-	// start with -1 as the do-while loop runs at least once even if nothing is found
-	int ret            = -1;
-	struct pci_dev *pd = NULL;
-	do {
+	int ret = 0;
+	struct pci_dev *pd, *last_pd = NULL;
+
+	while ((pd = pci_get_device(vendor, device, last_pd)) != NULL) {
+		if (last_pd) {
+			pci_dev_put(last_pd);
+		}
+		last_pd = pd;
 		++ret;
-		pd = pci_get_device(vendor, device, pd);
-	} while (pd != NULL);
+	}
+	if (last_pd) {
+		pci_dev_put(last_pd);
+	}
 	return ret;
 }
 
 // check if sys->pci_matches[] match with the PCI devices in the system
 // NOTE: this only checks if the devices in sys->pci_matches[] exist in the system
-//       with the expected count (or do *not* exist if the counts are 0),
+//       with the expected count (or the device does *not* exist if the counts are 0),
 //       PCI devices existing that aren't listed in sys->pci_matches[] is expected
 //       and does not make this function fail.
 static bool pci_devices_match(const struct asustor_driver_data *sys)