From 9c3dee5915d2f90e1fa518f0abc28a380e762cfb Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Sun, 24 Nov 2024 01:47:07 +0100 Subject: [PATCH] Most changes suggested in the code review including one to avoid leaking pci_dev structs in count_pci_device_instances() --- README.md | 24 ++++++++++++++---------- asustor.c | 37 ++++++++++++++++++++++++------------- 2 files changed, 38 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 334e7fe..0bfef4e 100644 --- a/README.md +++ b/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._ diff --git a/asustor.c b/asustor.c index 6860fd3..7580dd3 100644 --- a/asustor.c +++ b/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)