From 7d5f1e615e69c234f9a9ec9c944a10f21aeeb2f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Mon, 16 Dec 2024 18:10:09 +0200 Subject: [PATCH 01/13] PCI: shpchp: Remove logging from module init/exit functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The logging in shpchp module init/exit functions is not very useful. Remove it. Link: https://lore.kernel.org/r/20241216161012.1774-2-ilpo.jarvinen@linux.intel.com Signed-off-by: Ilpo Järvinen Signed-off-by: Bjorn Helgaas --- drivers/pci/hotplug/shpchp_core.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c index a92e28b72908..a10ce7be7f51 100644 --- a/drivers/pci/hotplug/shpchp_core.c +++ b/drivers/pci/hotplug/shpchp_core.c @@ -324,20 +324,12 @@ static struct pci_driver shpc_driver = { static int __init shpcd_init(void) { - int retval; - - retval = pci_register_driver(&shpc_driver); - dbg("%s: pci_register_driver = %d\n", __func__, retval); - info(DRIVER_DESC " version: " DRIVER_VERSION "\n"); - - return retval; + return pci_register_driver(&shpc_driver); } static void __exit shpcd_cleanup(void) { - dbg("unload_shpchpd()\n"); pci_unregister_driver(&shpc_driver); - info(DRIVER_DESC " version: " DRIVER_VERSION " unloaded\n"); } module_init(shpcd_init); From 49998220089285efd8dad91d7b249df6f5cfe1b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Mon, 16 Dec 2024 18:10:10 +0200 Subject: [PATCH 02/13] PCI: shpchp: Change dbg() -> ctrl_dbg() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Convert the last user of dbg() to use ctrl_dbg(). Link: https://lore.kernel.org/r/20241216161012.1774-3-ilpo.jarvinen@linux.intel.com Signed-off-by: Ilpo Järvinen Signed-off-by: Bjorn Helgaas --- drivers/pci/hotplug/shpchp_hpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c index 012b9e3fe5b0..bfbec7c1a6b1 100644 --- a/drivers/pci/hotplug/shpchp_hpc.c +++ b/drivers/pci/hotplug/shpchp_hpc.c @@ -675,7 +675,7 @@ static int shpc_get_cur_bus_speed(struct controller *ctrl) out: bus->cur_bus_speed = bus_speed; - dbg("Current bus speed = %d\n", bus_speed); + ctrl_dbg(ctrl, "Current bus speed = %d\n", bus_speed); return retval; } From b52ce0b6d54aa6b53ab0e3872cd35f51ba036368 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Mon, 17 Feb 2025 11:55:49 +0200 Subject: [PATCH 03/13] PCI: shpchp: Remove unused logging wrappers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shpchp hotplug driver defines logging wrapper with generic names which are just duplicates of existing generic printk() wrappers. They are also unused so remove them. Link: https://lore.kernel.org/r/20250217095550.2789-2-ilpo.jarvinen@linux.intel.com Signed-off-by: Ilpo Järvinen Signed-off-by: Bjorn Helgaas --- drivers/pci/hotplug/shpchp.h | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h index f0e2d2d54d71..10ba0bfac419 100644 --- a/drivers/pci/hotplug/shpchp.h +++ b/drivers/pci/hotplug/shpchp.h @@ -33,18 +33,6 @@ extern bool shpchp_poll_mode; extern int shpchp_poll_time; extern bool shpchp_debug; -#define dbg(format, arg...) \ -do { \ - if (shpchp_debug) \ - printk(KERN_DEBUG "%s: " format, MY_NAME, ## arg); \ -} while (0) -#define err(format, arg...) \ - printk(KERN_ERR "%s: " format, MY_NAME, ## arg) -#define info(format, arg...) \ - printk(KERN_INFO "%s: " format, MY_NAME, ## arg) -#define warn(format, arg...) \ - printk(KERN_WARNING "%s: " format, MY_NAME, ## arg) - #define ctrl_dbg(ctrl, format, arg...) \ do { \ if (shpchp_debug) \ From 588021b28642a1509bdae65ae22329240b39d543 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Mon, 17 Feb 2025 11:55:50 +0200 Subject: [PATCH 04/13] PCI: shpchp: Remove 'shpchp_debug' module parameter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "shpchp_debug" module parameter is used to enable debug logging. The generic ability to turn on/off debug prints dynamically covers this use case already so there is no need for module specific debug handling. The ctrl_dbg() wrapper also uses a low-level pci_printk() despite always using KERN_DEBUG level. Remove "shpchp_debug" parameter and convert ctrl_dbg() to use pci_dbg(). From now on, shpchp can be debugged using the normal dynamic debugger by setting CONFIG_DYNAMIC_DEBUG=y and then either adding to kernel cmdline: dyndbg="file drivers/pci/hotplug/shpchp* +p" or using this command on a running kernel: echo 'file drivers/pci/hotplug/shpchp* +p' > /sys/kernel/debug/dynamic_debug/control Link: https://lore.kernel.org/r/20250217095550.2789-3-ilpo.jarvinen@linux.intel.com Signed-off-by: Ilpo Järvinen Signed-off-by: Bjorn Helgaas --- drivers/pci/hotplug/shpchp.h | 6 +----- drivers/pci/hotplug/shpchp_core.c | 3 --- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h index 10ba0bfac419..a425530e0939 100644 --- a/drivers/pci/hotplug/shpchp.h +++ b/drivers/pci/hotplug/shpchp.h @@ -34,11 +34,7 @@ extern int shpchp_poll_time; extern bool shpchp_debug; #define ctrl_dbg(ctrl, format, arg...) \ - do { \ - if (shpchp_debug) \ - pci_printk(KERN_DEBUG, ctrl->pci_dev, \ - format, ## arg); \ - } while (0) + pci_dbg(ctrl->pci_dev, format, ## arg) #define ctrl_err(ctrl, format, arg...) \ pci_err(ctrl->pci_dev, format, ## arg) #define ctrl_info(ctrl, format, arg...) \ diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c index a10ce7be7f51..0c341453afc6 100644 --- a/drivers/pci/hotplug/shpchp_core.c +++ b/drivers/pci/hotplug/shpchp_core.c @@ -22,7 +22,6 @@ #include "shpchp.h" /* Global variables */ -bool shpchp_debug; bool shpchp_poll_mode; int shpchp_poll_time; @@ -33,10 +32,8 @@ int shpchp_poll_time; MODULE_AUTHOR(DRIVER_AUTHOR); MODULE_DESCRIPTION(DRIVER_DESC); -module_param(shpchp_debug, bool, 0644); module_param(shpchp_poll_mode, bool, 0644); module_param(shpchp_poll_time, int, 0644); -MODULE_PARM_DESC(shpchp_debug, "Debugging mode enabled or not"); MODULE_PARM_DESC(shpchp_poll_mode, "Using polling mechanism for hot-plug events or not"); MODULE_PARM_DESC(shpchp_poll_time, "Polling mechanism frequency, in seconds"); From 8ff4574cf73dba061d4a07e3b6094c5ecb2d2efe Mon Sep 17 00:00:00 2001 From: Guilherme Giacomo Simoes Date: Mon, 17 Feb 2025 15:56:38 -0300 Subject: [PATCH 05/13] PCI: cpcihp: Remove unused .get_power() and .set_power() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The .get_power() and .set_power() function pointers in struct cpci_hp_controller_ops were declared but never implemented by any driver. Thus, to improve code readability and reduce resource usage, remove these pointers and the code that has never been used. Link: https://lore.kernel.org/r/20250217185638.398925-1-trintaeoitogc@gmail.com Signed-off-by: Guilherme Giacomo Simoes Signed-off-by: Bjorn Helgaas [kwilczynski: commit log] Signed-off-by: Krzysztof Wilczyński --- drivers/pci/hotplug/cpci_hotplug.h | 2 -- drivers/pci/hotplug/cpci_hotplug_core.c | 17 ++--------------- 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/pci/hotplug/cpci_hotplug.h b/drivers/pci/hotplug/cpci_hotplug.h index 03fa39ab0c88..a31d6b62f523 100644 --- a/drivers/pci/hotplug/cpci_hotplug.h +++ b/drivers/pci/hotplug/cpci_hotplug.h @@ -44,8 +44,6 @@ struct cpci_hp_controller_ops { int (*enable_irq)(void); int (*disable_irq)(void); int (*check_irq)(void *dev_id); - u8 (*get_power)(struct slot *slot); - int (*set_power)(struct slot *slot, int value); }; struct cpci_hp_controller { diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c index d0559d2faf50..dd93e53ea7c2 100644 --- a/drivers/pci/hotplug/cpci_hotplug_core.c +++ b/drivers/pci/hotplug/cpci_hotplug_core.c @@ -71,13 +71,10 @@ static int enable_slot(struct hotplug_slot *hotplug_slot) { struct slot *slot = to_slot(hotplug_slot); - int retval = 0; dbg("%s - physical_slot = %s", __func__, slot_name(slot)); - if (controller->ops->set_power) - retval = controller->ops->set_power(slot, 1); - return retval; + return 0; } static int @@ -109,12 +106,6 @@ disable_slot(struct hotplug_slot *hotplug_slot) } cpci_led_on(slot); - if (controller->ops->set_power) { - retval = controller->ops->set_power(slot, 0); - if (retval) - goto disable_error; - } - slot->adapter_status = 0; if (slot->extracting) { @@ -129,11 +120,7 @@ disable_slot(struct hotplug_slot *hotplug_slot) static u8 cpci_get_power_status(struct slot *slot) { - u8 power = 1; - - if (controller->ops->get_power) - power = controller->ops->get_power(slot); - return power; + return 1; } static int From 5c8265fa63e4c60cd80f28bb0a682cf97b8a60e9 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Tue, 25 Feb 2025 18:06:01 +0100 Subject: [PATCH 06/13] PCI: hotplug: Drop superfluous pci_hotplug_slot_list The PCI hotplug core keeps a list of all registered slots. Its sole purpose is to WARN() on slot removal if another slot is using the same name. But this can never happen because already on slot creation, an error is returned and multiple messages are emitted if a slot's name is duplicated: pci_hp_register() __pci_hp_register() __pci_hp_initialize() pci_create_slot() kobject_init_and_add() kobject_add_varg() kobject_add_internal() create_dir() sysfs_create_dir_ns() kernfs_create_dir_ns() sysfs_warn_dup() pr_warn("cannot create duplicate filename ...") pr_err("%s failed for %s with -EEXIST, ..."); Drop the superfluous list. Link: https://lore.kernel.org/r/603735bc50eb370bc7f1c358441ac671360bab25.1740501868.git.lukas@wunner.de Signed-off-by: Lukas Wunner Signed-off-by: Bjorn Helgaas --- drivers/pci/hotplug/pci_hotplug_core.c | 32 -------------------------- include/linux/pci_hotplug.h | 2 -- 2 files changed, 34 deletions(-) diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c index 36236ac88fd5..9e3cde91c167 100644 --- a/drivers/pci/hotplug/pci_hotplug_core.c +++ b/drivers/pci/hotplug/pci_hotplug_core.c @@ -18,14 +18,12 @@ #include #include #include -#include #include #include #include #include #include #include -#include #include #include #include @@ -42,9 +40,6 @@ /* local variables */ static bool debug; -static LIST_HEAD(pci_hotplug_slot_list); -static DEFINE_MUTEX(pci_hp_mutex); - /* Weee, fun with macros... */ #define GET_STATUS(name, type) \ static int get_##name(struct hotplug_slot *slot, type *value) \ @@ -375,17 +370,6 @@ static void fs_remove_slot(struct pci_slot *pci_slot) pci_hp_remove_module_link(pci_slot); } -static struct hotplug_slot *get_slot_from_name(const char *name) -{ - struct hotplug_slot *slot; - - list_for_each_entry(slot, &pci_hotplug_slot_list, slot_list) { - if (strcmp(hotplug_slot_name(slot), name) == 0) - return slot; - } - return NULL; -} - /** * __pci_hp_register - register a hotplug_slot with the PCI hotplug subsystem * @slot: pointer to the &struct hotplug_slot to register @@ -484,10 +468,6 @@ int pci_hp_add(struct hotplug_slot *slot) return result; kobject_uevent(&pci_slot->kobj, KOBJ_ADD); - mutex_lock(&pci_hp_mutex); - list_add(&slot->slot_list, &pci_hotplug_slot_list); - mutex_unlock(&pci_hp_mutex); - dbg("Added slot %s to the list\n", hotplug_slot_name(slot)); return 0; } EXPORT_SYMBOL_GPL(pci_hp_add); @@ -514,21 +494,9 @@ EXPORT_SYMBOL_GPL(pci_hp_deregister); */ void pci_hp_del(struct hotplug_slot *slot) { - struct hotplug_slot *temp; - if (WARN_ON(!slot)) return; - mutex_lock(&pci_hp_mutex); - temp = get_slot_from_name(hotplug_slot_name(slot)); - if (WARN_ON(temp != slot)) { - mutex_unlock(&pci_hp_mutex); - return; - } - - list_del(&slot->slot_list); - mutex_unlock(&pci_hp_mutex); - dbg("Removed slot %s from the list\n", hotplug_slot_name(slot)); fs_remove_slot(slot->pci_slot); } EXPORT_SYMBOL_GPL(pci_hp_del); diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h index 3a10d6ec3ee7..ec77ccf1fc4d 100644 --- a/include/linux/pci_hotplug.h +++ b/include/linux/pci_hotplug.h @@ -50,7 +50,6 @@ struct hotplug_slot_ops { /** * struct hotplug_slot - used to register a physical slot with the hotplug pci core * @ops: pointer to the &struct hotplug_slot_ops to be used for this slot - * @slot_list: internal list used to track hotplug PCI slots * @pci_slot: represents a physical slot * @owner: The module owner of this structure * @mod_name: The module name (KBUILD_MODNAME) of this structure @@ -59,7 +58,6 @@ struct hotplug_slot { const struct hotplug_slot_ops *ops; /* Variables below this are for use only by the hotplug pci core. */ - struct list_head slot_list; struct pci_slot *pci_slot; struct module *owner; const char *mod_name; From 666550a8066a5333ddc96ca2fd28cb49318e789e Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Tue, 25 Feb 2025 18:06:02 +0100 Subject: [PATCH 07/13] PCI: hotplug: Drop superfluous try_module_get() calls In December 2002, historic commit https://git.kernel.org/tglx/history/c/bec7aa00ffe5 ("[PATCH] more module warning fixes") amended the PCI hotplug core to acquire a reference on the hotplug driver module when a sysfs attribute is accessed. That was necessary because back in the day, sysfs code did not take any precautions to prevent module unloading when an attribute was accessed. Soon after in July 2003, historic commit https://git.kernel.org/tglx/history/c/1cf6d20f6078 ("[PATCH] SYSFS: add module referencing to sysfs attribute files.") addressed that deficiency. But the commit neglected to remove the now unnecessary reference acquisition from the PCI hotplug core. The commit acquired a module reference for the entire duration between open() and close() of a sysfs attribute. This made it impossible to unload a module while attributes were kept open by user space. That's possible today: When a hotplug driver module is unloaded, it removes sysfs attributes of all its hotplug slots by calling pci_hp_del(). This will wait for any concurrent user space operation to finish: pci_hp_del() fs_remove_slot() sysfs_remove_file() sysfs_remove_file_ns() kernfs_remove_by_name_ns() __kernfs_remove() kernfs_drain() A user space operation such as read() briefly acquires a reference on the attribute with kernfs_get_active(). kernfs_drain() waits until all such references are released before allowing attribute removal. Once the attribute is removed, any subsequent user space operation on a still open attribute file will return -ENODEV. Thus, reference acquisition by the PCI hotplug core is still unnecessary today. So drop it at long last. Link: https://lore.kernel.org/r/ed950fa2722967be4491146c7b867c1e7be11d37.1740501868.git.lukas@wunner.de Signed-off-by: Lukas Wunner Signed-off-by: Bjorn Helgaas --- drivers/pci/hotplug/pci_hotplug_core.c | 23 +---------------------- 1 file changed, 1 insertion(+), 22 deletions(-) diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c index 9e3cde91c167..de5b501b40be 100644 --- a/drivers/pci/hotplug/pci_hotplug_core.c +++ b/drivers/pci/hotplug/pci_hotplug_core.c @@ -14,7 +14,7 @@ * Scott Murray */ -#include /* try_module_get & module_put */ +#include #include #include #include @@ -46,11 +46,8 @@ static int get_##name(struct hotplug_slot *slot, type *value) \ { \ const struct hotplug_slot_ops *ops = slot->ops; \ int retval = 0; \ - if (!try_module_get(slot->owner)) \ - return -ENODEV; \ if (ops->get_##name) \ retval = ops->get_##name(slot, value); \ - module_put(slot->owner); \ return retval; \ } @@ -83,10 +80,6 @@ static ssize_t power_write_file(struct pci_slot *pci_slot, const char *buf, power = (u8)(lpower & 0xff); dbg("power = %d\n", power); - if (!try_module_get(slot->owner)) { - retval = -ENODEV; - goto exit; - } switch (power) { case 0: if (slot->ops->disable_slot) @@ -102,9 +95,7 @@ static ssize_t power_write_file(struct pci_slot *pci_slot, const char *buf, err("Illegal value specified for power\n"); retval = -EINVAL; } - module_put(slot->owner); -exit: if (retval) return retval; return count; @@ -141,15 +132,9 @@ static ssize_t attention_write_file(struct pci_slot *pci_slot, const char *buf, attention = (u8)(lattention & 0xff); dbg(" - attention = %d\n", attention); - if (!try_module_get(slot->owner)) { - retval = -ENODEV; - goto exit; - } if (ops->set_attention_status) retval = ops->set_attention_status(slot, attention); - module_put(slot->owner); -exit: if (retval) return retval; return count; @@ -207,15 +192,9 @@ static ssize_t test_write_file(struct pci_slot *pci_slot, const char *buf, test = (u32)(ltest & 0xffffffff); dbg("test = %d\n", test); - if (!try_module_get(slot->owner)) { - retval = -ENODEV; - goto exit; - } if (slot->ops->hardware_test) retval = slot->ops->hardware_test(slot, test); - module_put(slot->owner); -exit: if (retval) return retval; return count; From 62460bcb5a2ac37bb416aada0167db3fe78ac385 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Tue, 25 Feb 2025 18:06:03 +0100 Subject: [PATCH 08/13] PCI: hotplug: Drop superfluous NULL pointer checks in has_*_file() The PCI hotplug core contains five has_*_file() functions to determine whether a certain sysfs file shall be added (or removed) for a given hotplug slot. The functions perform NULL pointer checks for the hotplug_slot and its hotplug_slot_ops. However the callers already perform these checks: pci_hp_register() __pci_hp_register() __pci_hp_initialize() pci_hp_deregister() pci_hp_del() The only way to actually trigger these checks is to call pci_hp_add() without having called pci_hp_initialize(). Amend pci_hp_add() to catch that and drop the now superfluous NULL pointer checks in has_*_file(). Drop the same superfluous checks from pci_hp_create_module_link(), which is (only) called from pci_hp_add(). Link: https://lore.kernel.org/r/37d1928edf8c3201a8b10794f1db3142e16e02b9.1740501868.git.lukas@wunner.de Signed-off-by: Lukas Wunner Signed-off-by: Bjorn Helgaas --- drivers/pci/hotplug/pci_hotplug_core.c | 17 ++++++----------- drivers/pci/slot.c | 2 -- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c index de5b501b40be..d4c12451570b 100644 --- a/drivers/pci/hotplug/pci_hotplug_core.c +++ b/drivers/pci/hotplug/pci_hotplug_core.c @@ -209,8 +209,6 @@ static bool has_power_file(struct pci_slot *pci_slot) { struct hotplug_slot *slot = pci_slot->hotplug; - if ((!slot) || (!slot->ops)) - return false; if ((slot->ops->enable_slot) || (slot->ops->disable_slot) || (slot->ops->get_power_status)) @@ -222,8 +220,6 @@ static bool has_attention_file(struct pci_slot *pci_slot) { struct hotplug_slot *slot = pci_slot->hotplug; - if ((!slot) || (!slot->ops)) - return false; if ((slot->ops->set_attention_status) || (slot->ops->get_attention_status)) return true; @@ -234,8 +230,6 @@ static bool has_latch_file(struct pci_slot *pci_slot) { struct hotplug_slot *slot = pci_slot->hotplug; - if ((!slot) || (!slot->ops)) - return false; if (slot->ops->get_latch_status) return true; return false; @@ -245,8 +239,6 @@ static bool has_adapter_file(struct pci_slot *pci_slot) { struct hotplug_slot *slot = pci_slot->hotplug; - if ((!slot) || (!slot->ops)) - return false; if (slot->ops->get_adapter_status) return true; return false; @@ -256,8 +248,6 @@ static bool has_test_file(struct pci_slot *pci_slot) { struct hotplug_slot *slot = pci_slot->hotplug; - if ((!slot) || (!slot->ops)) - return false; if (slot->ops->hardware_test) return true; return false; @@ -439,9 +429,14 @@ EXPORT_SYMBOL_GPL(__pci_hp_initialize); */ int pci_hp_add(struct hotplug_slot *slot) { - struct pci_slot *pci_slot = slot->pci_slot; + struct pci_slot *pci_slot; int result; + if (WARN_ON(!slot)) + return -EINVAL; + + pci_slot = slot->pci_slot; + result = fs_add_slot(pci_slot); if (result) return result; diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c index 36b44be0489d..dd6e80b7db09 100644 --- a/drivers/pci/slot.c +++ b/drivers/pci/slot.c @@ -340,8 +340,6 @@ void pci_hp_create_module_link(struct pci_slot *pci_slot) struct kobject *kobj = NULL; int ret; - if (!slot || !slot->ops) - return; kobj = kset_find_obj(module_kset, slot->mod_name); if (!kobj) return; From 34bd6141a62d21853b61e759f5c617059b2f3655 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Tue, 25 Feb 2025 18:06:04 +0100 Subject: [PATCH 09/13] PCI: hotplug: Avoid backpointer dereferencing in has_*_file() The PCI hotplug core contains five has_*_file() functions to determine whether a certain sysfs file shall be added (or removed) for a given hotplug slot. The functions receive a struct pci_slot pointer which they have to dereference back to a struct hotplug_slot. Avoid by passing them a struct hotplug_slot pointer directly. Link: https://lore.kernel.org/r/5b2f5b4ac45285953d00fd7637732a93fd40d26e.1740501868.git.lukas@wunner.de Signed-off-by: Lukas Wunner Signed-off-by: Bjorn Helgaas --- drivers/pci/hotplug/pci_hotplug_core.c | 56 +++++++++++--------------- 1 file changed, 23 insertions(+), 33 deletions(-) diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c index d4c12451570b..a992bf51af98 100644 --- a/drivers/pci/hotplug/pci_hotplug_core.c +++ b/drivers/pci/hotplug/pci_hotplug_core.c @@ -205,10 +205,8 @@ static struct pci_slot_attribute hotplug_slot_attr_test = { .store = test_write_file }; -static bool has_power_file(struct pci_slot *pci_slot) +static bool has_power_file(struct hotplug_slot *slot) { - struct hotplug_slot *slot = pci_slot->hotplug; - if ((slot->ops->enable_slot) || (slot->ops->disable_slot) || (slot->ops->get_power_status)) @@ -216,79 +214,71 @@ static bool has_power_file(struct pci_slot *pci_slot) return false; } -static bool has_attention_file(struct pci_slot *pci_slot) +static bool has_attention_file(struct hotplug_slot *slot) { - struct hotplug_slot *slot = pci_slot->hotplug; - if ((slot->ops->set_attention_status) || (slot->ops->get_attention_status)) return true; return false; } -static bool has_latch_file(struct pci_slot *pci_slot) +static bool has_latch_file(struct hotplug_slot *slot) { - struct hotplug_slot *slot = pci_slot->hotplug; - if (slot->ops->get_latch_status) return true; return false; } -static bool has_adapter_file(struct pci_slot *pci_slot) +static bool has_adapter_file(struct hotplug_slot *slot) { - struct hotplug_slot *slot = pci_slot->hotplug; - if (slot->ops->get_adapter_status) return true; return false; } -static bool has_test_file(struct pci_slot *pci_slot) +static bool has_test_file(struct hotplug_slot *slot) { - struct hotplug_slot *slot = pci_slot->hotplug; - if (slot->ops->hardware_test) return true; return false; } -static int fs_add_slot(struct pci_slot *pci_slot) +static int fs_add_slot(struct hotplug_slot *slot, struct pci_slot *pci_slot) { int retval = 0; /* Create symbolic link to the hotplug driver module */ pci_hp_create_module_link(pci_slot); - if (has_power_file(pci_slot)) { + if (has_power_file(slot)) { retval = sysfs_create_file(&pci_slot->kobj, &hotplug_slot_attr_power.attr); if (retval) goto exit_power; } - if (has_attention_file(pci_slot)) { + if (has_attention_file(slot)) { retval = sysfs_create_file(&pci_slot->kobj, &hotplug_slot_attr_attention.attr); if (retval) goto exit_attention; } - if (has_latch_file(pci_slot)) { + if (has_latch_file(slot)) { retval = sysfs_create_file(&pci_slot->kobj, &hotplug_slot_attr_latch.attr); if (retval) goto exit_latch; } - if (has_adapter_file(pci_slot)) { + if (has_adapter_file(slot)) { retval = sysfs_create_file(&pci_slot->kobj, &hotplug_slot_attr_presence.attr); if (retval) goto exit_adapter; } - if (has_test_file(pci_slot)) { + if (has_test_file(slot)) { retval = sysfs_create_file(&pci_slot->kobj, &hotplug_slot_attr_test.attr); if (retval) @@ -298,18 +288,18 @@ static int fs_add_slot(struct pci_slot *pci_slot) goto exit; exit_test: - if (has_adapter_file(pci_slot)) + if (has_adapter_file(slot)) sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_presence.attr); exit_adapter: - if (has_latch_file(pci_slot)) + if (has_latch_file(slot)) sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_latch.attr); exit_latch: - if (has_attention_file(pci_slot)) + if (has_attention_file(slot)) sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_attention.attr); exit_attention: - if (has_power_file(pci_slot)) + if (has_power_file(slot)) sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_power.attr); exit_power: pci_hp_remove_module_link(pci_slot); @@ -317,23 +307,23 @@ static int fs_add_slot(struct pci_slot *pci_slot) return retval; } -static void fs_remove_slot(struct pci_slot *pci_slot) +static void fs_remove_slot(struct hotplug_slot *slot, struct pci_slot *pci_slot) { - if (has_power_file(pci_slot)) + if (has_power_file(slot)) sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_power.attr); - if (has_attention_file(pci_slot)) + if (has_attention_file(slot)) sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_attention.attr); - if (has_latch_file(pci_slot)) + if (has_latch_file(slot)) sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_latch.attr); - if (has_adapter_file(pci_slot)) + if (has_adapter_file(slot)) sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_presence.attr); - if (has_test_file(pci_slot)) + if (has_test_file(slot)) sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_test.attr); pci_hp_remove_module_link(pci_slot); @@ -437,7 +427,7 @@ int pci_hp_add(struct hotplug_slot *slot) pci_slot = slot->pci_slot; - result = fs_add_slot(pci_slot); + result = fs_add_slot(slot, pci_slot); if (result) return result; @@ -471,7 +461,7 @@ void pci_hp_del(struct hotplug_slot *slot) if (WARN_ON(!slot)) return; - fs_remove_slot(slot->pci_slot); + fs_remove_slot(slot, slot->pci_slot); } EXPORT_SYMBOL_GPL(pci_hp_del); From cc973ef13f8e31608d34c349c6ed85d44d716523 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Tue, 25 Feb 2025 18:06:05 +0100 Subject: [PATCH 10/13] PCI: hotplug: Inline pci_hp_{create,remove}_module_link() For no apparent reason, the pci_hp_{create,remove}_module_link() helpers live in slot.c, even though they're only called from two functions in pci_hotplug_core.c. Inline the helpers to reduce code size and number of exported symbols. Link: https://lore.kernel.org/r/c207f03cfe32ae9002d9b453001a1dd63d9ab3fb.1740501868.git.lukas@wunner.de Signed-off-by: Lukas Wunner Signed-off-by: Bjorn Helgaas --- drivers/pci/hotplug/pci_hotplug_core.c | 14 +++++++-- drivers/pci/slot.c | 42 -------------------------- include/linux/pci.h | 5 --- 3 files changed, 11 insertions(+), 50 deletions(-) diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c index a992bf51af98..d30f1316c98e 100644 --- a/drivers/pci/hotplug/pci_hotplug_core.c +++ b/drivers/pci/hotplug/pci_hotplug_core.c @@ -245,10 +245,18 @@ static bool has_test_file(struct hotplug_slot *slot) static int fs_add_slot(struct hotplug_slot *slot, struct pci_slot *pci_slot) { + struct kobject *kobj; int retval = 0; /* Create symbolic link to the hotplug driver module */ - pci_hp_create_module_link(pci_slot); + kobj = kset_find_obj(module_kset, slot->mod_name); + if (kobj) { + retval = sysfs_create_link(&pci_slot->kobj, kobj, "module"); + if (retval) + dev_err(&pci_slot->bus->dev, + "Error creating sysfs link (%d)\n", retval); + kobject_put(kobj); + } if (has_power_file(slot)) { retval = sysfs_create_file(&pci_slot->kobj, @@ -302,7 +310,7 @@ static int fs_add_slot(struct hotplug_slot *slot, struct pci_slot *pci_slot) if (has_power_file(slot)) sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_power.attr); exit_power: - pci_hp_remove_module_link(pci_slot); + sysfs_remove_link(&pci_slot->kobj, "module"); exit: return retval; } @@ -326,7 +334,7 @@ static void fs_remove_slot(struct hotplug_slot *slot, struct pci_slot *pci_slot) if (has_test_file(slot)) sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_test.attr); - pci_hp_remove_module_link(pci_slot); + sysfs_remove_link(&pci_slot->kobj, "module"); } /** diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c index dd6e80b7db09..50fb3eb595fe 100644 --- a/drivers/pci/slot.c +++ b/drivers/pci/slot.c @@ -7,7 +7,6 @@ #include #include -#include #include #include #include "pci.h" @@ -325,47 +324,6 @@ void pci_destroy_slot(struct pci_slot *slot) } EXPORT_SYMBOL_GPL(pci_destroy_slot); -#if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE) -#include -/** - * pci_hp_create_module_link - create symbolic link to hotplug driver module - * @pci_slot: struct pci_slot - * - * Helper function for pci_hotplug_core.c to create symbolic link to - * the hotplug driver module. - */ -void pci_hp_create_module_link(struct pci_slot *pci_slot) -{ - struct hotplug_slot *slot = pci_slot->hotplug; - struct kobject *kobj = NULL; - int ret; - - kobj = kset_find_obj(module_kset, slot->mod_name); - if (!kobj) - return; - ret = sysfs_create_link(&pci_slot->kobj, kobj, "module"); - if (ret) - dev_err(&pci_slot->bus->dev, "Error creating sysfs link (%d)\n", - ret); - kobject_put(kobj); -} -EXPORT_SYMBOL_GPL(pci_hp_create_module_link); - -/** - * pci_hp_remove_module_link - remove symbolic link to the hotplug driver - * module. - * @pci_slot: struct pci_slot - * - * Helper function for pci_hotplug_core.c to remove symbolic link to - * the hotplug driver module. - */ -void pci_hp_remove_module_link(struct pci_slot *pci_slot) -{ - sysfs_remove_link(&pci_slot->kobj, "module"); -} -EXPORT_SYMBOL_GPL(pci_hp_remove_module_link); -#endif - static int pci_slot_init(void) { struct kset *pci_bus_kset; diff --git a/include/linux/pci.h b/include/linux/pci.h index 47b31ad724fa..a0f5c8fcd9c7 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -2447,11 +2447,6 @@ static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int res static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { } #endif -#if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE) -void pci_hp_create_module_link(struct pci_slot *pci_slot); -void pci_hp_remove_module_link(struct pci_slot *pci_slot); -#endif - /** * pci_pcie_cap - get the saved PCIe capability offset * @dev: PCI device From 9d7db4db19827380e225914618c0c1bf435ed2f5 Mon Sep 17 00:00:00 2001 From: Feng Tang Date: Mon, 3 Mar 2025 10:36:30 +0800 Subject: [PATCH 11/13] PCI/portdrv: Only disable pciehp interrupts early when needed Firmware developers reported that Linux issues two PCIe hotplug commands in very short intervals on an ARM server, which doesn't comply with the PCIe spec. According to PCIe r6.1, sec 6.7.3.2, if the Command Completed event is supported, software must wait for a command to complete or wait at least 1 second before sending a new command. In the failure case, the first PCIe hotplug command is from get_port_device_capability(), which sends a command to disable PCIe hotplug interrupts without waiting for its completion, and the second command comes from pcie_enable_notification() of pciehp driver, which enables hotplug interrupts again. Fix this by only disabling the hotplug interrupts when the pciehp driver is not enabled. Link: https://lore.kernel.org/r/20250303023630.78397-1-feng.tang@linux.alibaba.com Fixes: 2bd50dd800b5 ("PCI: PCIe: Disable PCIe port services during port initialization") Suggested-by: Lukas Wunner Signed-off-by: Feng Tang [bhelgaas: commit log] Signed-off-by: Bjorn Helgaas Reviewed-by: Lukas Wunner Reviewed-by: Kuppuswamy Sathyanarayanan --- drivers/pci/pcie/portdrv.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c index 02e73099bad0..e8318fd5f6ed 100644 --- a/drivers/pci/pcie/portdrv.c +++ b/drivers/pci/pcie/portdrv.c @@ -228,10 +228,12 @@ static int get_port_device_capability(struct pci_dev *dev) /* * Disable hot-plug interrupts in case they have been enabled - * by the BIOS and the hot-plug service driver is not loaded. + * by the BIOS and the hot-plug service driver won't be loaded + * to handle them. */ - pcie_capability_clear_word(dev, PCI_EXP_SLTCTL, - PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE); + if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE)) + pcie_capability_clear_word(dev, PCI_EXP_SLTCTL, + PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE); } #ifdef CONFIG_PCIEAER From e3260237aaadc9799107ccb940c6688195c4518d Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Tue, 11 Mar 2025 07:27:32 +0100 Subject: [PATCH 12/13] PCI: pciehp: Avoid unnecessary device replacement check Hot-removal of nested PCI hotplug ports suffers from a long-standing race condition which can lead to a deadlock: A parent hotplug port acquires pci_lock_rescan_remove(), then waits for pciehp to unbind from a child hotplug port. Meanwhile that child hotplug port tries to acquire pci_lock_rescan_remove() as well in order to remove its own children. The deadlock only occurs if the parent acquires pci_lock_rescan_remove() first, not if the child happens to acquire it first. Several workarounds to avoid the issue have been proposed and discarded over the years, e.g.: https://lore.kernel.org/r/4c882e25194ba8282b78fe963fec8faae7cf23eb.1529173804.git.lukas@wunner.de/ A proper fix is being worked on, but needs more time as it is nontrivial and necessarily intrusive. Recent commit 9d573d19547b ("PCI: pciehp: Detect device replacement during system sleep") provokes more frequent occurrence of the deadlock when removing more than one Thunderbolt device during system sleep. The commit sought to detect device replacement, but also triggered on device removal. Differentiating reliably between replacement and removal is impossible because pci_get_dsn() returns 0 both if the device was removed, as well as if it was replaced with one lacking a Device Serial Number. Avoid the more frequent occurrence of the deadlock by checking whether the hotplug port itself was hot-removed. If so, there's no sense in checking whether its child device was replaced. This works because the ->resume_noirq() callback is invoked in top-down order for the entire hierarchy: A parent hotplug port detecting device replacement (or removal) marks all children as removed using pci_dev_set_disconnected() and a child hotplug port can then reliably detect being removed. Link: https://lore.kernel.org/r/02f166e24c87d6cde4085865cce9adfdfd969688.1741674172.git.lukas@wunner.de Fixes: 9d573d19547b ("PCI: pciehp: Detect device replacement during system sleep") Reported-by: Kenneth Crudup Closes: https://lore.kernel.org/r/83d9302a-f743-43e4-9de2-2dd66d91ab5b@panix.com/ Reported-by: Chia-Lin Kao (AceLan) Closes: https://lore.kernel.org/r/20240926125909.2362244-1-acelan.kao@canonical.com/ Tested-by: Kenneth Crudup Tested-by: Mika Westerberg Signed-off-by: Lukas Wunner Signed-off-by: Bjorn Helgaas Reviewed-by: Mika Westerberg Reviewed-by: Kuppuswamy Sathyanarayanan Cc: stable@vger.kernel.org # v6.11+ --- drivers/pci/hotplug/pciehp_core.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c index ff458e692fed..997841c69893 100644 --- a/drivers/pci/hotplug/pciehp_core.c +++ b/drivers/pci/hotplug/pciehp_core.c @@ -286,9 +286,12 @@ static int pciehp_suspend(struct pcie_device *dev) static bool pciehp_device_replaced(struct controller *ctrl) { - struct pci_dev *pdev __free(pci_dev_put); + struct pci_dev *pdev __free(pci_dev_put) = NULL; u32 reg; + if (pci_dev_is_disconnected(ctrl->pcie->port)) + return false; + pdev = pci_get_slot(ctrl->pcie->port->subordinate, PCI_DEVFN(0, 0)); if (!pdev) return true; From 527664f738afb6f2c58022cd35e63801e5dc7aec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Fri, 21 Mar 2025 18:21:14 +0200 Subject: [PATCH 13/13] PCI: pciehp: Don't enable HPIE when resuming in poll mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PCIe hotplug can operate in poll mode without interrupt handlers using a polling kthread only. eb34da60edee ("PCI: pciehp: Disable hotplug interrupt during suspend") failed to consider that and enables HPIE (Hot-Plug Interrupt Enable) unconditionally when resuming the Port. Only set HPIE if non-poll mode is in use. This makes pcie_enable_interrupt() match how pcie_enable_notification() already handles HPIE. Link: https://lore.kernel.org/r/20250321162114.3939-1-ilpo.jarvinen@linux.intel.com Fixes: eb34da60edee ("PCI: pciehp: Disable hotplug interrupt during suspend") Signed-off-by: Ilpo Järvinen Signed-off-by: Bjorn Helgaas Reviewed-by: Lukas Wunner --- drivers/pci/hotplug/pciehp_hpc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index bb5a8d9f03ad..28ab393af1c0 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -842,7 +842,9 @@ void pcie_enable_interrupt(struct controller *ctrl) { u16 mask; - mask = PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE; + mask = PCI_EXP_SLTCTL_DLLSCE; + if (!pciehp_poll_mode) + mask |= PCI_EXP_SLTCTL_HPIE; pcie_write_cmd(ctrl, mask, mask); }