[PATCH] use bus_slot number for name

[PATCH] use bus_slot number for name

[Kristen Accardi]

For systems with multiple hotplug controllers, you need to use more than
just the slot number to uniquely name the slot.  Without a unique slot
name, the pci_hp_register() will fail.  This patch adds the bus number
to the name.

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/hotplug/pciehp.h linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pciehp.h
--- linux-2.6.13-rc4/drivers/pci/hotplug/pciehp.h	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pciehp.h	2005-08-04 17:57:18.000000000 -0700
@@ -302,7 +302,7 @@ static inline void return_resource(struc
 
 static inline void make_slot_name(char *buffer, int buffer_size, struct slot *slot)
 {
-	snprintf(buffer, buffer_size, "%d", slot->number);
+	snprintf(buffer, buffer_size, "%04d_%04d", slot->bus, slot->number);
 }
 
 enum php_ctlr_type {
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/hotplug/shpchp.h linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/shpchp.h
--- linux-2.6.13-rc4/drivers/pci/hotplug/shpchp.h	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/shpchp.h	2005-08-04 17:57:18.000000000 -0700
@@ -46,7 +46,7 @@ extern int shpchp_poll_mode;
 extern int shpchp_poll_time;
 extern int shpchp_debug;
 
-/*#define dbg(format, arg...) do { if (shpchp_debug) printk(KERN_DEBUG "%s: " format, MY_NAME , ## arg); } while (0)*/
+/* #define dbg(format, arg...) do { if (shpchp_debug) printk(KERN_DEBUG "%s: " format, MY_NAME , ## arg); } while (0) */
 #define dbg(format, arg...) do { if (shpchp_debug) printk("%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)
@@ -411,7 +411,7 @@ static inline void return_resource(struc
 
 static inline void make_slot_name(char *buffer, int buffer_size, struct slot *slot)
 {
-	snprintf(buffer, buffer_size, "%d", slot->number);
+	snprintf(buffer, buffer_size, "%04d_%04d", slot->bus, slot->number);
 }
 
 enum php_ctlr_type {

Re: [PATCH] use bus_slot number for name

[Dave Jones]

On Fri, Aug 05, 2005 at 12:16:06PM -0700, Kristen Accardi wrote:
 > For systems with multiple hotplug controllers, you need to use more than
 > just the slot number to uniquely name the slot.  Without a unique slot
 > name, the pci_hp_register() will fail.  This patch adds the bus number
 > to the name.
 > 
 > Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
 > 
 > diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/hotplug/pciehp.h linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pciehp.h
 > --- linux-2.6.13-rc4/drivers/pci/hotplug/pciehp.h	2005-07-28 15:44:44.000000000 -0700
 > +++ linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pciehp.h	2005-08-04 17:57:18.000000000 -0700
 > @@ -302,7 +302,7 @@ static inline void return_resource(struc
 >  
 >  static inline void make_slot_name(char *buffer, int buffer_size, struct slot *slot)
 >  {
 > -	snprintf(buffer, buffer_size, "%d", slot->number);
 > +	snprintf(buffer, buffer_size, "%04d_%04d", slot->bus, slot->number);
 >  }

Won't using..

	snprintf(buffer, buffer_size, "%s", pci_name(slot));

work equally as well, and also future-proof this ?
 
		Dave

Re: [PATCH] use bus_slot number for name

[Kristen Accardi]

On Fri, 2005-08-05 at 15:51 -0400, Dave Jones wrote:
> On Fri, Aug 05, 2005 at 12:16:06PM -0700, Kristen Accardi wrote:
>  > For systems with multiple hotplug controllers, you need to use more than
>  > just the slot number to uniquely name the slot.  Without a unique slot
>  > name, the pci_hp_register() will fail.  This patch adds the bus number
>  > to the name.
>  > 
>  > Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
>  > 
>  > diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/hotplug/pciehp.h linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pciehp.h
>  > --- linux-2.6.13-rc4/drivers/pci/hotplug/pciehp.h	2005-07-28 15:44:44.000000000 -0700
>  > +++ linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pciehp.h	2005-08-04 17:57:18.000000000 -0700
>  > @@ -302,7 +302,7 @@ static inline void return_resource(struc
>  >  
>  >  static inline void make_slot_name(char *buffer, int buffer_size, struct slot *slot)
>  >  {
>  > -	snprintf(buffer, buffer_size, "%d", slot->number);
>  > +	snprintf(buffer, buffer_size, "%04d_%04d", slot->bus, slot->number);
>  >  }
> 
> Won't using..
> 
> 	snprintf(buffer, buffer_size, "%s", pci_name(slot));
> 
> work equally as well, and also future-proof this ?
>  
> 		Dave

Well, it isn't as convenient since pci_dev is not available from the
slot structure. But I will do it if this is the general consensus.

Re: [PATCH] use bus_slot number for name

[Kristen Accardi]

On Fri, 2005-08-05 at 15:51 -0400, Dave Jones wrote:
> On Fri, Aug 05, 2005 at 12:16:06PM -0700, Kristen Accardi wrote:
>  > For systems with multiple hotplug controllers, you need to use more than
>  > just the slot number to uniquely name the slot.  Without a unique slot
>  > name, the pci_hp_register() will fail.  This patch adds the bus number
>  > to the name.
>  > 
>  > Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
>  > 
>  > diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/hotplug/pciehp.h linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pciehp.h
>  > --- linux-2.6.13-rc4/drivers/pci/hotplug/pciehp.h	2005-07-28 15:44:44.000000000 -0700
>  > +++ linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pciehp.h	2005-08-04 17:57:18.000000000 -0700
>  > @@ -302,7 +302,7 @@ static inline void return_resource(struc
>  >  
>  >  static inline void make_slot_name(char *buffer, int buffer_size, struct slot *slot)
>  >  {
>  > -	snprintf(buffer, buffer_size, "%d", slot->number);
>  > +	snprintf(buffer, buffer_size, "%04d_%04d", slot->bus, slot->number);
>  >  }
> 
> Won't using..
> 
> 	snprintf(buffer, buffer_size, "%s", pci_name(slot));
> 
> work equally as well, and also future-proof this ?
>  
> 		Dave

What do you think of this patch - it is basically what you asked for
except that I moved the functionality up to pci_hotplug.h so that all
drivers can use the same nameing convention if they feel like it.

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/hotplug/pciehp_core.c linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pciehp_core.c
--- linux-2.6.13-rc4/drivers/pci/hotplug/pciehp_core.c	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pciehp_core.c	2005-08-05 14:15:09.000000000 -0700
@@ -140,7 +140,7 @@ static int init_slots(struct controller 
 			goto error_hpslot;
 		memset(new_slot->hotplug_slot->info, 0,
 					sizeof(struct hotplug_slot_info));
-		new_slot->hotplug_slot->name = kmalloc(SLOT_NAME_SIZE,
+		new_slot->hotplug_slot->name = kmalloc(HPSLOT_NAME_SIZE,
 						GFP_KERNEL);
 		if (!new_slot->hotplug_slot->name)
 			goto error_info;
@@ -156,7 +156,7 @@ static int init_slots(struct controller 
 		/* register this slot with the hotplug pci core */
 		new_slot->hotplug_slot->private = new_slot;
 		new_slot->hotplug_slot->release = &release_slot;
-		make_slot_name(new_slot->hotplug_slot->name, SLOT_NAME_SIZE, new_slot);
+		pci_hp_make_slot_name(new_slot->hotplug_slot, ctrl->pci_dev); 
 		new_slot->hotplug_slot->ops = &pciehp_hotplug_slot_ops;
 
 		new_slot->hpc_ops->get_power_status(new_slot, &(new_slot->hotplug_slot->info->power_status));
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/hotplug/pciehp.h linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pciehp.h
--- linux-2.6.13-rc4/drivers/pci/hotplug/pciehp.h	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pciehp.h	2005-08-05 14:14:54.000000000 -0700
@@ -298,12 +298,6 @@ static inline void return_resource(struc
 	*head = node;
 }
 
-#define SLOT_NAME_SIZE 10
-
-static inline void make_slot_name(char *buffer, int buffer_size, struct slot *slot)
-{
-	snprintf(buffer, buffer_size, "%d", slot->number);
-}
 
 enum php_ctlr_type {
 	PCI,
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/hotplug/pci_hotplug.h linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pci_hotplug.h
--- linux-2.6.13-rc4/drivers/pci/hotplug/pci_hotplug.h	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pci_hotplug.h	2005-08-05 14:21:45.000000000 -0700
@@ -170,6 +170,12 @@ struct hotplug_slot {
 };
 #define to_hotplug_slot(n) container_of(n, struct hotplug_slot, kobj)
 
+#define HPSLOT_NAME_SIZE BUS_ID_SIZE 
+static inline void pci_hp_make_slot_name(struct hotplug_slot *hpslot, struct pci_dev *pdev)
+{
+	snprintf(hpslot->name, HPSLOT_NAME_SIZE, "%s", pci_name(pdev));
+}
+
 extern int pci_hp_register		(struct hotplug_slot *slot);
 extern int pci_hp_deregister		(struct hotplug_slot *slot);
 extern int pci_hp_change_slot_info	(struct hotplug_slot *slot,
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/hotplug/shpchp_core.c linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/shpchp_core.c
--- linux-2.6.13-rc4/drivers/pci/hotplug/shpchp_core.c	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/shpchp_core.c	2005-08-05 14:15:18.000000000 -0700
@@ -134,7 +134,7 @@ static int init_slots(struct controller 
 		if (!new_slot->hotplug_slot->info)
 			goto error_hpslot;
 		memset(new_slot->hotplug_slot->info, 0, sizeof (struct hotplug_slot_info));
-		new_slot->hotplug_slot->name = kmalloc (SLOT_NAME_SIZE, GFP_KERNEL);
+		new_slot->hotplug_slot->name = kmalloc (HPSLOT_NAME_SIZE, GFP_KERNEL);
 		if (!new_slot->hotplug_slot->name)
 			goto error_info;
 
@@ -154,7 +154,7 @@ static int init_slots(struct controller 
 		/* register this slot with the hotplug pci core */
 		new_slot->hotplug_slot->private = new_slot;
 		new_slot->hotplug_slot->release = &release_slot;
-		make_slot_name(new_slot->hotplug_slot->name, SLOT_NAME_SIZE, new_slot);
+		pci_hp_make_slot_name(new_slot->hotplug_slot, ctrl->pci_dev);
 		new_slot->hotplug_slot->ops = &shpchp_hotplug_slot_ops;
 
 		new_slot->hpc_ops->get_power_status(new_slot, &(new_slot->hotplug_slot->info->power_status));
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/hotplug/shpchp.h linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/shpchp.h
--- linux-2.6.13-rc4/drivers/pci/hotplug/shpchp.h	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/shpchp.h	2005-08-05 14:14:43.000000000 -0700
@@ -46,7 +46,7 @@ extern int shpchp_poll_mode;
 extern int shpchp_poll_time;
 extern int shpchp_debug;
 
-/*#define dbg(format, arg...) do { if (shpchp_debug) printk(KERN_DEBUG "%s: " format, MY_NAME , ## arg); } while (0)*/
+/* #define dbg(format, arg...) do { if (shpchp_debug) printk(KERN_DEBUG "%s: " format, MY_NAME , ## arg); } while (0) */
 #define dbg(format, arg...) do { if (shpchp_debug) printk("%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)
@@ -407,12 +407,6 @@ static inline void return_resource(struc
 	*head = node;
 }
 
-#define SLOT_NAME_SIZE 10
-
-static inline void make_slot_name(char *buffer, int buffer_size, struct slot *slot)
-{
-	snprintf(buffer, buffer_size, "%d", slot->number);
-}
 
 enum php_ctlr_type {
 	PCI,

Re: [PATCH] use bus_slot number for name

[Greg KH]

On Fri, Aug 05, 2005 at 03:51:23PM -0400, Dave Jones wrote:
> On Fri, Aug 05, 2005 at 12:16:06PM -0700, Kristen Accardi wrote:
>  > For systems with multiple hotplug controllers, you need to use more than
>  > just the slot number to uniquely name the slot.  Without a unique slot
>  > name, the pci_hp_register() will fail.  This patch adds the bus number
>  > to the name.
>  > 
>  > Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
>  > 
>  > diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/hotplug/pciehp.h linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pciehp.h
>  > --- linux-2.6.13-rc4/drivers/pci/hotplug/pciehp.h	2005-07-28 15:44:44.000000000 -0700
>  > +++ linux-2.6.13-rc4-shpchp-slot-name-fix/drivers/pci/hotplug/pciehp.h	2005-08-04 17:57:18.000000000 -0700
>  > @@ -302,7 +302,7 @@ static inline void return_resource(struc
>  >  
>  >  static inline void make_slot_name(char *buffer, int buffer_size, struct slot *slot)
>  >  {
>  > -	snprintf(buffer, buffer_size, "%d", slot->number);
>  > +	snprintf(buffer, buffer_size, "%04d_%04d", slot->bus, slot->number);
>  >  }
> 
> Won't using..
> 
> 	snprintf(buffer, buffer_size, "%s", pci_name(slot));

As Kristen already said, a slot could refer to multiple pci_dev devices,
so I don't know how well this would really work out.

Although you might want to put the domain in that name, if known, as
large boxes might need that, right?

thanks,

greg k-h

Re: [Pcihpd-discuss] Re: [PATCH] use bus_slot number for name

[Greg KH]

On Fri, Aug 05, 2005 at 02:31:07PM -0700, Kristen Accardi wrote:
> +#define HPSLOT_NAME_SIZE BUS_ID_SIZE 
> +static inline void pci_hp_make_slot_name(struct hotplug_slot *hpslot, struct pci_dev *pdev)
> +{
> +	snprintf(hpslot->name, HPSLOT_NAME_SIZE, "%s", pci_name(pdev));
> +}

I don't think that others can use this, as you can have multiple struct
pci_dev for the same "slot".

thanks,

greg k-h

Re: [Pcihpd-discuss] [PATCH] use bus_slot number for name

[Matthew Wilcox]

On Fri, Aug 05, 2005 at 12:16:06PM -0700, Kristen Accardi wrote:
> For systems with multiple hotplug controllers, you need to use more than
> just the slot number to uniquely name the slot.  Without a unique slot
> name, the pci_hp_register() will fail.  This patch adds the bus number
> to the name.

That doesn't make much sense.  The slot number should at least be unique
to the chassis, if not to the whole machine.  HP's large machines with
multiple cabinets encode the cabinet number in the return from _SUN.
It ends up as something like 80103 for a large machine while still being
merely slot 3 for the smaller machines.

IOW, I think this is a firmware bug which needs to be fixed there.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [Pcihpd-discuss] [PATCH] use bus_slot number for name

[Kristen Accardi]

On Wed, 2005-08-10 at 16:01 +0100, Matthew Wilcox wrote:
> On Fri, Aug 05, 2005 at 12:16:06PM -0700, Kristen Accardi wrote:
> > For systems with multiple hotplug controllers, you need to use more than
> > just the slot number to uniquely name the slot.  Without a unique slot
> > name, the pci_hp_register() will fail.  This patch adds the bus number
> > to the name.
> 
> That doesn't make much sense.  The slot number should at least be unique
> to the chassis, if not to the whole machine.  HP's large machines with
> multiple cabinets encode the cabinet number in the return from _SUN.
> It ends up as something like 80103 for a large machine while still being
> merely slot 3 for the smaller machines.
> 
> IOW, I think this is a firmware bug which needs to be fixed there.
> 

Just wanted to let you know that I'm not ignoring your comment :).  I'm
checking now to see if the firmware is required to make the slot number
unique across all controllers.  I also am expecting a hardware/firmware
update for the machine that exhibited this behavior, and so will retest
when I get it and let you know.  although, I'm not sure if it's a good
idea to trust the BIOS to do this properly even if it's required.