[PATCH v1 0/1] PCI: ibmphp: Simplify ibmphp_init_devno() parameter

[PATCH v1 0/1] PCI: ibmphp: Simplify ibmphp_init_devno() parameter

[Pramod Maurya]

ibmphp_init_devno() in the IBM PCI Hot Plug controller driver takes a
struct slot ** (double pointer) parameter, but the function never
re-assigns the pointer itself -- it only dereferences it to read and
write the slot's fields.  The double indirection is therefore
unnecessary and makes the code harder to read.

Root cause:
  The function was originally written with a struct slot ** parameter,
  perhaps anticipating that the pointer might need to be re-assigned,
  but this never happens in practice.  All accesses inside the function
  are of the form (*cur_slot)->field, which is equivalent to
  cur_slot->field when the parameter is a struct slot *.

Fix:
  - Change the parameter type from struct slot ** to struct slot *.
  - Replace all (*cur_slot)->field accesses with cur_slot->field.
  - Add the parameter name to the declaration in ibmphp.h and move
    the comment to its own line to stay within 100 columns.
  - Update the single call site in ibmphp_ebda.c to pass tmp_slot
    directly instead of &tmp_slot.

Note: a few pre-existing CHECK-level style issues (cast spacing,
debug() argument alignment) appear in the diff because the changed
lines are touched by this patch.  They are not introduced by this
change and are left for a separate cleanup.

This patch has been validated with scripts/checkpatch.pl --strict and
produces 0 errors, 0 warnings, and 0 checks introduced by this patch.

Pramod Maurya (1):
  PCI: ibmphp: Simplify ibmphp_init_devno() to take struct slot *

 drivers/pci/hotplug/ibmphp.h      |  3 ++-
 drivers/pci/hotplug/ibmphp_core.c | 30 +++++++++++++++---------------
 drivers/pci/hotplug/ibmphp_ebda.c |  2 +-
 3 files changed, 18 insertions(+), 17 deletions(-)

-- 
2.52.0

[PATCH v1 1/1] PCI: ibmphp: Simplify ibmphp_init_devno() to take struct slot *

[Pramod Maurya]

ibmphp_init_devno() takes a struct slot ** parameter but only ever
dereferences it to access the slot's fields.  No re-assignment of the
pointer itself is performed, so the double indirection is unnecessary.

Simplify the function to take a struct slot * instead, update all
internal accesses from (*cur_slot)-> to cur_slot->, and update the
single call site in ibmphp_ebda.c to pass tmp_slot directly instead
of &tmp_slot.

Signed-off-by: Pramod Maurya <pramod.nexgen@gmail.com>
---
 drivers/pci/hotplug/ibmphp.h      |  3 ++-
 drivers/pci/hotplug/ibmphp_core.c | 30 +++++++++++++++---------------
 drivers/pci/hotplug/ibmphp_ebda.c |  2 +-
 3 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/hotplug/ibmphp.h b/drivers/pci/hotplug/ibmphp.h
index c248a09be7b5..e80c45eeaa73 100644
--- a/drivers/pci/hotplug/ibmphp.h
+++ b/drivers/pci/hotplug/ibmphp.h
@@ -733,7 +733,8 @@ struct controller {
 
 /* Functions */
 
-int ibmphp_init_devno(struct slot **);	/* This function is called from EBDA, so we need it not be static */
+/* ibmphp_ebda.c calls this, so it cannot be static */
+int ibmphp_init_devno(struct slot *cur_slot);
 int ibmphp_do_disable_slot(struct slot *slot_cur);
 int ibmphp_update_slot_info(struct slot *);	/* This function is called from HPC, so we need it to not be static */
 int ibmphp_configure_card(struct pci_func *, u8);
diff --git a/drivers/pci/hotplug/ibmphp_core.c b/drivers/pci/hotplug/ibmphp_core.c
index aca86c092d4a..b122769baee1 100644
--- a/drivers/pci/hotplug/ibmphp_core.c
+++ b/drivers/pci/hotplug/ibmphp_core.c
@@ -109,7 +109,7 @@ static int __init get_max_slots(void)
  * Parameters: struct slot
  * Returns 0 or errors
  */
-int ibmphp_init_devno(struct slot **cur_slot)
+int ibmphp_init_devno(struct slot *cur_slot)
 {
 	struct irq_routing_table *rtable;
 	int len;
@@ -130,21 +130,21 @@ int ibmphp_init_devno(struct slot **cur_slot)
 		return -1;
 	}
 	for (loop = 0; loop < len; loop++) {
-		if ((*cur_slot)->number == rtable->slots[loop].slot &&
-		    (*cur_slot)->bus == rtable->slots[loop].bus) {
-			(*cur_slot)->device = PCI_SLOT(rtable->slots[loop].devfn);
+		if (cur_slot->number == rtable->slots[loop].slot &&
+		    cur_slot->bus == rtable->slots[loop].bus) {
+			cur_slot->device = PCI_SLOT(rtable->slots[loop].devfn);
 			for (i = 0; i < 4; i++)
-				(*cur_slot)->irq[i] = IO_APIC_get_PCI_irq_vector((int) (*cur_slot)->bus,
-						(int) (*cur_slot)->device, i);
-
-			debug("(*cur_slot)->irq[0] = %x\n",
-					(*cur_slot)->irq[0]);
-			debug("(*cur_slot)->irq[1] = %x\n",
-					(*cur_slot)->irq[1]);
-			debug("(*cur_slot)->irq[2] = %x\n",
-					(*cur_slot)->irq[2]);
-			debug("(*cur_slot)->irq[3] = %x\n",
-					(*cur_slot)->irq[3]);
+				cur_slot->irq[i] = IO_APIC_get_PCI_irq_vector((int) cur_slot->bus,
+						(int) cur_slot->device, i);
+
+			debug("cur_slot->irq[0] = %x\n",
+					cur_slot->irq[0]);
+			debug("cur_slot->irq[1] = %x\n",
+					cur_slot->irq[1]);
+			debug("cur_slot->irq[2] = %x\n",
+					cur_slot->irq[2]);
+			debug("cur_slot->irq[3] = %x\n",
+					cur_slot->irq[3]);
 
 			debug("rtable->exclusive_irqs = %x\n",
 					rtable->exclusive_irqs);
diff --git a/drivers/pci/hotplug/ibmphp_ebda.c b/drivers/pci/hotplug/ibmphp_ebda.c
index c1939b88e75b..0b018ae3b348 100644
--- a/drivers/pci/hotplug/ibmphp_ebda.c
+++ b/drivers/pci/hotplug/ibmphp_ebda.c
@@ -880,7 +880,7 @@ static int __init ebda_rsrc_controller(void)
 			if (rc)
 				goto error;
 
-			rc = ibmphp_init_devno(&tmp_slot);
+			rc = ibmphp_init_devno(tmp_slot);
 			if (rc)
 				goto error;
 			tmp_slot->hotplug_slot.ops = &ibmphp_hotplug_slot_ops;
-- 
2.52.0

Re: [PATCH v1 1/1] PCI: ibmphp: Simplify ibmphp_init_devno() to take struct slot *

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unbalanced Lifecycle Operations / NULL Pointer Dereference on module initialization failure.
- [Low] The patch resolves a documented TODO item by simplifying the signature of ibmphp_init_devno(), but fails to remove the corresponding entry from the drivers/pci/hotplug/TODO file.
--

Note: The standard inline review format has been altered to a free-form
summary due to recitation restrictions. No direct quotes from the patch are
included.

commit 10dfdd5d4dd69d8f479f459382630a4bba2b9d73
Author: Pramod Maurya <pramod.nexgen@gmail.com>

PCI: ibmphp: Simplify ibmphp_init_devno() to take struct slot *

This commit simplifies the ibmphp_init_devno() function by changing its
parameter from a double pointer to a single pointer. It updates all
internal accesses and the call site in ebda_rsrc_controller() to pass the
pointer directly.

This isn't a bug, but since this patch successfully simplifies the function
signature, should the corresponding task also be removed from the
drivers/pci/hotplug/TODO file? Leaving the resolved item in the list might
cause some confusion for future cleanups.

Additionally, while reviewing the call site in ebda_rsrc_controller(), I
noticed an existing issue in the error handling path. If
ibmphp_init_devno() succeeds for one slot but fails for a subsequent slot,
the code takes the goto error path. The successfully processed slot remains
in the global ibmphp_slot_head list, but the pci_hp_register() loop is
bypassed.

Does this cause a panic during module initialization failure? When the
error cascades up to ibmphp_init(), it appears to invoke ibmphp_unload()
and free_slots(). Since pci_hp_register() was never called for the first
slot, its hotplug_slot.pci_slot remains null and its slot_list remains
uninitialized. Calling pci_hp_del() and pci_hp_destroy() on this slot
looks like it would dereference these invalid pointers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515110944.216993-2-pramod.nexgen@gmail.com?part=1