login
Search
Hi Jesse,
This is v2 of the series that implements a series of changes
that allows the PCI core to manage slot names, rather than
individual hotplug drivers.
It addresses comments raised during the first round of review,
most notably the bug that Kenji-san found of possible false name
collisions.
This series applies against linux-next.
I'm including the previous explanation/justification to remind
folks of the raison d'etre for this patchset.
Thanks.
/ac
There are several benefits to this approach:
1) The core can prevent duplicate slot names on systems
with broken firmware.
2) Since the kobject core keeps a copy of the slot name,
there is no need for each driver to manage a separate
copy, especially since the core can rename slots from
underneath drivers. We save runtime memory by only
referencing the kobject name.
2.a) The PCI hotplug core doesn't need its own reference
for slot name either.
3) Individual hotplug drivers become just a little bit
simpler by pushing as many kzalloc() calls for 'name'
down into the PCI core as possible.
---
Alex Chiang (13):
PCI: Hotplug core: remove 'name'
PCI: shcphp: remove 'name' parameter
PCI: SGI Hotplug: stop managing bss_hotplug_slot->name
PCI: rpaphp: stop managing hotplug_slot->name
PCI: pciehp: remove 'name' parameter
PCI: ibmphp: stop managing hotplug_slot->name
PCI: fakephp: remove 'name' parameter
PCI: cpqphp: stop managing hotplug_slot->name
PCI: cpci_hotplug: stop managing hotplug_slot->name
PCI: acpiphp: remove 'name' parameter
PCI, PCI Hotplug: introduce slot_name helpers
PCI: prevent duplicate slot names
PCI Hotplug core: add 'name' param pci_hp_register interface
drivers/pci/hotplug/acpiphp.h | 9 +-
drivers/pci/hotplug/acpiphp_core.c | 32 +++++---
...Update pci_hp_register() to take a const char *name parameter. The motivation for this is to clean up the individual hotplug drivers so that each one does not have to manage its own name. The PCI core should be the place where we manage the name. We update the interface and all callsites first, in a "no functional change" manner, and clean up the drivers later. Cc: jbarnes@virtuousgeek.org Cc: kristen.c.accardi@intel.com Cc: matthew@wil.cx Cc: kaneshige.kenji@jp.fujitsu.com Signed-off-by: Alex Chiang <achiang@hp.com> --- drivers/pci/hotplug/acpiphp_core.c | 3 ++- drivers/pci/hotplug/cpci_hotplug_core.c | 3 ++- drivers/pci/hotplug/cpqphp_core.c | 3 ++- drivers/pci/hotplug/fakephp.c | 3 ++- drivers/pci/hotplug/ibmphp_ebda.c | 3 ++- drivers/pci/hotplug/pci_hotplug_core.c | 15 ++++++++------- drivers/pci/hotplug/pciehp_core.c | 3 ++- drivers/pci/hotplug/rpaphp_slot.c | 2 +- drivers/pci/hotplug/sgi_hotplug.c | 3 ++- drivers/pci/hotplug/shpchp_core.c | 3 ++- include/linux/pci_hotplug.h | 5 ++++- 11 files changed, 29 insertions(+), 17 deletions(-) diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c index 0e496e8..e984176 100644 --- a/drivers/pci/hotplug/acpiphp_core.c +++ b/drivers/pci/hotplug/acpiphp_core.c @@ -340,7 +340,8 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot) retval = pci_hp_register(slot->hotplug_slot, acpiphp_slot->bridge->pci_bus, - acpiphp_slot->device); + acpiphp_slot->device, + slot->name); if (retval == -EBUSY) goto error_hpslot; if (retval) { diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c index 9359479..5e5dee8 100644 --- a/drivers/pci/hotplug/cpci_hotplug_core.c +++ b/drivers/pci/hotplug/cpci_hotplug_core.c @@ -285,7 +285,8 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last) ...
Not a huge fan of this style ... the usual style would be: extern int pci_hp_register(struct hotplug_slot *, struct pci_bus *, int nr, const char *name); Otherwise Acked-by: Matthew Wilcox <willy@linux.intel.com> -- Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." --
Prevent callers of pci_create_slot() from registering slots with duplicate names. This condition occurs most often when PCI hotplug drivers are loaded on platforms with broken firmware that assigns identical names to multiple slots. We now rename these duplicate slots on behalf of the user. If firmware assigns the name N to multiple slots, then: The first registered slot is assigned N The second registered slot is assigned N-1 The third registered slot is assigned N-2 The Mth registered slot becomes N-M A side effect of this patch is that the error condition for when multiple drivers attempt to claim the same slot becomes much more prominent. In other words, the previous error condition returned for duplicate slot names (-EEXIST) masked the case when multiple drivers attempted to claim the same slot. Now, the -EBUSY return makes the true error more obvious. This is the permanent fix mentioned in earlier commits: shpchp: Rename duplicate slot name N as N-1, N-2, N-M... d6a9e9b40be7da84f82eb414c2ad98c5bb69986b pciehp: Rename duplicate slot name N as N-1, N-2, N-M... 167e782e301188c7c7e31e486bbeea5f918324c1 Cc: jbarnes@virtuousgeek.org Cc: kristen.c.accardi@intel.com Cc: matthew@wil.cx Cc: kaneshige.kenji@jp.fujitsu.com Signed-off-by: Alex Chiang <achiang@hp.com> --- drivers/pci/hotplug/pci_hotplug_core.c | 23 ++++-- drivers/pci/hotplug/pciehp_core.c | 14 ---- drivers/pci/hotplug/shpchp_core.c | 15 ---- drivers/pci/slot.c | 117 +++++++++++++++++++++++++++----- include/linux/pci.h | 3 + 5 files changed, 117 insertions(+), 55 deletions(-) diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c index 3e37d63..2232608 100644 --- a/drivers/pci/hotplug/pci_hotplug_core.c +++ b/drivers/pci/hotplug/pci_hotplug_core.c @@ -558,7 +558,8 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr, const char *name) { int ...
Not quite true ... the Mth registered slot becomes N-(M-1). With the
I don't like the goto-loop style (yes, I know we do it elsewhere), and I
think we only need to krealloc inside the 'if' condition. Something
like this, perhaps:
static char *make_slot_name(const char *name)
{
char *new_name;
int len, max, dup;
new_name = kstrdup(name, GFP_KERNEL);
if (!new_name)
return NULL;
/*
* Make sure we hit the realloc case the first time through the
* loop. 'len' will be strlen(name) + 3 at that point which is
* enough space for "name-X" and the trailing NUL.
*/
len = strlen(name) + 2;
max = 1;
dup = 1;
for (;;) {
struct kobject *dup_slot;
dup_slot = kset_find_obj(pci_slots_kset, new_name);
if (!dup_slot)
break;
kobject_put(dup_slot);
if (dup == max) {
len++;
max *= 10;
new_name = krealloc(new_name, len, GFP_KERNEL);
if (!new_name)
break;
}
sprintf(new_name, "%s-%d", name, dup++);
}
return new_name;
I think you need to kfree() slot before you assign an ERR_PTR to it.
Actually, since the 'err' label does that, just make that:
if (!slot_name) {
err = -ENOMEM;
goto err;
Are you going to get enough information to debug problems with just this
WARN_ON? And do we want to decline to renumber a slot to the same
number as an existing one?
Anyway, looks good, and I really like the name-change for this function.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
Hi Willy, Thanks for the review. I've pretty much incorporated all your As Rolf Eike Beer pointed out, a failed krealloc() will leak the old version of new_name, so I did this instead: kfree(new_name); new_name = kmalloc(len, GFP_KERNEL); This is better than krealloc() in several ways: 1. we avoid the unneeded memcpy that krealloc() does for us. we don't need it because we're going to sprintf over it anyway. 2. the explicit kfree(new_name) means we won't leak I think this should be sufficient for the following reasons: 1. This API was added for ppc; I can't imagine any other arch actually needing to renumber a slot after create. 2. I added this check at BenH's request as a "belt and suspenders" sort of thing; neither of us expects to really get a collision here ever, and if we do, it's an OFW error (iirc). 3. I think I will return early though, because otherwise, Thanks. /ac --
Agreed. -- Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." --
If krealloc() fails you will leak the old new_name here. Eike
Thanks for catching this. I already responded to Willy that I'm changing it to: kfree(new_name); new_name = kmalloc(len, GFP_KERNEL); That will prevent any leaks. Thanks for the review. /ac --
I have two comments here. (1) I think the reference counter of the device will be leaked if (dev == NULL) && (dev->slot != NULL). We need pci_dev_put() whenever dev is not NULL. (2) When the slot is empty, the 'dev' will be always NULL. Therefore, 'tmp_slot' will be always NULL on the empty slot here. Because of this, the following code to rename the slot will not work on the Thanks, Kenji Kaneshige --
You're right, thank you. Ok, I was trying to avoid creating a new interface called "pci_find_phys_slot()" or something similar, but I think we need it, and it will fix this issue. Thanks again for the review. /ac --
Hi Kenji-san, You are right. We still only have to worry about the case where a _detection_ driver was loaded before a _hotplug_ driver, and we need to worry abou the case of an empty slot. I created a new interface called pci_get_physical_slot() that will tell us if we've already created a slot or not. This will work even if the slot is empty. Using this inteface should simplify the tortured logic in my last Hm, ok. I might think about adding some code to the fakephp driver to use it as a debug tool as well... The new pci_get_physical_slot() interface will solve both issues. Thanks. /ac --
In preparation for cleaning up the various hotplug drivers such that they don't have to manage their own 'name' parameters anymore, we provide the following convenience functions: pci_slot_name() hotplug_slot_name() These helpers will be used by individual hotplug drivers. Cc: jbarnes@virtuousgeek.org Cc: kristen.c.accardi@intel.com Cc: matthew@wil.cx Cc: kaneshige.kenji@jp.fujitsu.com Signed-off-by: Alex Chiang <achiang@hp.com> --- include/linux/pci.h | 5 +++++ include/linux/pci_hotplug.h | 5 +++++ 2 files changed, 10 insertions(+), 0 deletions(-) diff --git a/include/linux/pci.h b/include/linux/pci.h index fd6efbc..bd259c3 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -64,6 +64,11 @@ struct pci_slot { struct kobject kobj; }; +static inline const char *pci_slot_name(const struct pci_slot *slot) +{ + return kobject_name(&slot->kobj); +} + /* File state for mmap()s on /proc/bus/pci/X/Y */ enum pci_mmap_state { pci_mmap_io, diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h index 303834b..7184bee 100644 --- a/include/linux/pci_hotplug.h +++ b/include/linux/pci_hotplug.h @@ -165,6 +165,11 @@ struct hotplug_slot { }; #define to_hotplug_slot(n) container_of(n, struct hotplug_slot, kobj) +static inline const char *hotplug_slot_name(const struct hotplug_slot *slot) +{ + return pci_slot_name(slot->pci_slot); +} + extern int pci_hp_register(struct hotplug_slot *, struct pci_bus *, int nr, --
Acked-by: Matthew Wilcox <willy@linux.intel.com> -- Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." --
We do not need to manage our own name parameter, especially since the PCI core can change it on our behalf, in the case of duplicate slot names. Remove 'name' from acpiphp's version of struct slot. Cc: jbarnes@virtuousgeek.org Cc: kristen.c.accardi@intel.com Cc: kaneshige.kenji@jp.fujitsu.com Signed-off-by: Alex Chiang <achiang@hp.com> --- drivers/pci/hotplug/acpiphp.h | 9 +++++---- drivers/pci/hotplug/acpiphp_core.c | 31 ++++++++++++++++--------------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h index 5a58b07..f9e244d 100644 --- a/drivers/pci/hotplug/acpiphp.h +++ b/drivers/pci/hotplug/acpiphp.h @@ -50,9 +50,6 @@ #define info(format, arg...) printk(KERN_INFO "%s: " format, MY_NAME , ## arg) #define warn(format, arg...) printk(KERN_WARNING "%s: " format, MY_NAME , ## arg) -/* name size which is used for entries in pcihpfs */ -#define SLOT_NAME_SIZE 20 /* {_SUN} */ - struct acpiphp_bridge; struct acpiphp_slot; @@ -63,9 +60,13 @@ struct slot { struct hotplug_slot *hotplug_slot; struct acpiphp_slot *acpi_slot; struct hotplug_slot_info info; - char name[SLOT_NAME_SIZE]; }; +static inline const char *slot_name(struct slot *slot) +{ + return hotplug_slot_name(slot->hotplug_slot); +} + /* * struct acpiphp_bridge - PCI bridge information * diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c index e984176..55b2b77 100644 --- a/drivers/pci/hotplug/acpiphp_core.c +++ b/drivers/pci/hotplug/acpiphp_core.c @@ -44,6 +44,9 @@ #define MY_NAME "acpiphp" +/* name size which is used for entries in pcihpfs */ +#define SLOT_NAME_SIZE 20 /* {_SUN} */ + static int debug; int acpiphp_debug; @@ -84,7 +87,6 @@ static struct hotplug_slot_ops acpi_hotplug_slot_ops = { .get_adapter_status = get_adapter_status, }; - /** * acpiphp_register_attention - set attention LED callback * @info: must ...
What's the difference between snprintf and scnprintf? And why were we bothering to use snprintf anyway? For when we fall into a parallel universe where a u32 can have more than twenty digits? -- Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." --
I think this may have already been answered somewhere else, but scnprintf tells you number of characters that actually fits into the buffer whereas snprintf tells you the number of characters Well, I think there are some in-flight patches that want to change sun to a 64 bit value, which makes me think we want to change SLOT_NAME_SIZE to 21... /ac --
We no longer need to manage our version of hotplug_slot->name since the PCI and hotplug core manage it on our behalf. Now, we simply advise the PCI core of the name that we would like, and let the core take care of the rest. Cc: jbarnes@virtuousgeek.org Cc: kristen.c.accardi@intel.com Cc: scottm@somanetworks.com Signed-off-by: Alex Chiang <achiang@hp.com> --- drivers/pci/hotplug/cpci_hotplug.h | 6 ++ drivers/pci/hotplug/cpci_hotplug_core.c | 75 ++++++++++++------------------- drivers/pci/hotplug/cpci_hotplug_pci.c | 4 +- 3 files changed, 36 insertions(+), 49 deletions(-) diff --git a/drivers/pci/hotplug/cpci_hotplug.h b/drivers/pci/hotplug/cpci_hotplug.h index d9769b3..9fff878 100644 --- a/drivers/pci/hotplug/cpci_hotplug.h +++ b/drivers/pci/hotplug/cpci_hotplug.h @@ -30,6 +30,7 @@ #include <linux/types.h> #include <linux/pci.h> +#include <linux/pci_hotplug.h> /* PICMG 2.1 R2.0 HS CSR bits: */ #define HS_CSR_INS 0x0080 @@ -69,6 +70,11 @@ struct cpci_hp_controller { struct cpci_hp_controller_ops *ops; }; +static inline const char *slot_name(struct slot *slot) +{ + return hotplug_slot_name(slot->hotplug_slot); +} + extern int cpci_hp_register_controller(struct cpci_hp_controller *controller); extern int cpci_hp_unregister_controller(struct cpci_hp_controller *controller); extern int cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last); diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c index 5e5dee8..84cf489 100644 --- a/drivers/pci/hotplug/cpci_hotplug_core.c +++ b/drivers/pci/hotplug/cpci_hotplug_core.c @@ -108,7 +108,7 @@ enable_slot(struct hotplug_slot *hotplug_slot) struct slot *slot = hotplug_slot->private; int retval = 0; - dbg("%s - physical_slot = %s", __func__, hotplug_slot->name); + dbg("%s - physical_slot = %s", __func__, slot_name(slot)); if (controller->ops->set_power) retval = controller->ops->set_power(slot, 1); @@ -121,25 +121,23 @@ ...
Unfortunately, our name is kind of crappy. Can we not give the user anything better than this? I can't seem to find the CompactPCI specification to see if we have a better name available. I wonder if we can use DMI data on this class of machine. Scott? -- Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." --
This is about all that can be done, unfortunately. I've never seen anything useful with regards to this in DMI on the boards I've looked at, and we would have to rework the board driver -> CPCI hotplug core interface to pass that information along even if we could get it for some boards. In general, it is a difficult problem to solve since the same set of system master and peripheral boards in a different chassis could very well be at different geographic addresses depending on where the chassis vendor has placed the system master slot(s). Scott -- Scott Murray SOMA Networks, Inc. Toronto, Ontario e-mail: scottm@somanetworks.com --
We no longer need to manage our version of hotplug_slot->name since the PCI and hotplug core manage it on our behalf. Now, we simply advise the PCI core of the name that we would like, and let the core take care of the rest. Cc: jbarnes@virtuousgeek.org Cc: kristen.c.accardi@intel.com Signed-off-by: Alex Chiang <achiang@hp.com> --- drivers/pci/hotplug/cpqphp.h | 13 +++++------- drivers/pci/hotplug/cpqphp_core.c | 39 +++++++++++++++++-------------------- 2 files changed, 23 insertions(+), 29 deletions(-) diff --git a/drivers/pci/hotplug/cpqphp.h b/drivers/pci/hotplug/cpqphp.h index b1decfa..afaf8f6 100644 --- a/drivers/pci/hotplug/cpqphp.h +++ b/drivers/pci/hotplug/cpqphp.h @@ -449,6 +449,11 @@ extern u8 cpqhp_disk_irq; /* inline functions */ +static inline char *slot_name(struct slot *slot) +{ + return hotplug_slot_name(slot->hotplug_slot); +} + /* * return_resource * @@ -696,14 +701,6 @@ static inline int get_presence_status(struct controller *ctrl, struct slot *slot return presence_save; } -#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); -} - - static inline int wait_for_ctrl_irq(struct controller *ctrl) { DECLARE_WAITQUEUE(wait, current); diff --git a/drivers/pci/hotplug/cpqphp_core.c b/drivers/pci/hotplug/cpqphp_core.c index a7fe458..008ae5d 100644 --- a/drivers/pci/hotplug/cpqphp_core.c +++ b/drivers/pci/hotplug/cpqphp_core.c @@ -315,14 +315,15 @@ static void release_slot(struct hotplug_slot *hotplug_slot) { struct slot *slot = hotplug_slot->private; - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name); + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot)); kfree(slot->hotplug_slot->info); - kfree(slot->hotplug_slot->name); kfree(slot->hotplug_slot); kfree(slot); } +#define SLOT_NAME_SIZE 10 + static int ctrl_slot_setup(struct controller ...
So ... we're using %d here and %u in acpiphp. Obviously we don't expect to get a number above 2 billion, but I think if we do have some utterly bogus firmware that gives us a number above 2 billion, printing a positive number is a better user experience than a negative number. We clearly have a common pattern here where hotplug drivers have a number insteaqd of a name (I would venture this is the most common). Maybe we need a common helper? I think this is a subject for the long-term todo list, not something that needs to be addressed in the context of this patch series. -- Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." --
Well, on some HP machines, a slot number >2 billion actually does make sense, if you convert it to hex, and then consider that to be some sort of encoding for topology. Yeah, ok -- long term todo. ;) /ac --
Remove 'name' from fakephp's struct dummy_slot, as the PCI core will now manage our slot name for us. Cc: jbarnes@virtuousgeek.org Cc: kristen.c.accardi@intel.com Signed-off-by: Alex Chiang <achiang@hp.com> --- drivers/pci/hotplug/fakephp.c | 19 ++++++++++--------- 1 files changed, 10 insertions(+), 9 deletions(-) diff --git a/drivers/pci/hotplug/fakephp.c b/drivers/pci/hotplug/fakephp.c index f1c1817..7447faf 100644 --- a/drivers/pci/hotplug/fakephp.c +++ b/drivers/pci/hotplug/fakephp.c @@ -66,7 +66,6 @@ struct dummy_slot { struct pci_dev *dev; struct work_struct remove_work; unsigned long removed; - char name[8]; }; static int debug; @@ -96,10 +95,13 @@ static void dummy_release(struct hotplug_slot *slot) kfree(dslot); } +#define SLOT_NAME_SIZE 8 + static int add_slot(struct pci_dev *dev) { struct dummy_slot *dslot; struct hotplug_slot *slot; + char name[SLOT_NAME_SIZE]; int retval = -ENOMEM; static int count = 1; @@ -119,15 +121,13 @@ static int add_slot(struct pci_dev *dev) if (!dslot) goto error_info; - slot->name = dslot->name; - snprintf(slot->name, sizeof(dslot->name), "fake%d", count++); - dbg("slot->name = %s\n", slot->name); + scnprintf(name, SLOT_NAME_SIZE, "fake%d", count++); + dbg("slot->name = %s\n", name); slot->ops = &dummy_hotplug_slot_ops; slot->release = &dummy_release; slot->private = dslot; - retval = pci_hp_register(slot, dev->bus, PCI_SLOT(dev->devfn), - slot->name); + retval = pci_hp_register(slot, dev->bus, PCI_SLOT(dev->devfn), name); if (retval) { err("pci_hp_register failed with error %d\n", retval); goto error_dslot; @@ -168,10 +168,11 @@ static void remove_slot(struct dummy_slot *dslot) { int retval; - dbg("removing slot %s\n", dslot->slot->name); + dbg("removing slot %s\n", hotplug_slot_name(dslot->slot)); retval = pci_hp_deregister(dslot->slot); if (retval) - err("Problem unregistering a slot %s\n", dslot->slot->name); + err("Problem ...
We no longer need to manage our version of hotplug_slot->name since the PCI and hotplug core manage it on our behalf. Now, we simply advise the PCI core of the name that we would like, and let the core take care of the rest. Additionally, slightly rearrange the members of struct slot so they are naturally aligned to eliminate holes. Cc: jbarnes@virtuousgeek.org Cc: kristen.c.accardi@intel.com Signed-off-by: Alex Chiang <achiang@hp.com> --- drivers/pci/hotplug/ibmphp.h | 5 ++--- drivers/pci/hotplug/ibmphp_ebda.c | 20 +++++++------------- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/drivers/pci/hotplug/ibmphp.h b/drivers/pci/hotplug/ibmphp.h index 612d963..a8d391a 100644 --- a/drivers/pci/hotplug/ibmphp.h +++ b/drivers/pci/hotplug/ibmphp.h @@ -707,17 +707,16 @@ struct slot { u8 device; u8 number; u8 real_physical_slot_num; - char name[100]; u32 capabilities; u8 supported_speed; u8 supported_bus_mode; + u8 flag; /* this is for disable slot and polling */ + u8 ctlr_index; struct hotplug_slot *hotplug_slot; struct controller *ctrl; struct pci_func *func; u8 irq[4]; - u8 flag; /* this is for disable slot and polling */ int bit_mode; /* 0 = 32, 1 = 64 */ - u8 ctlr_index; struct bus_info *bus_on; struct list_head ibm_slot_list; u8 status; diff --git a/drivers/pci/hotplug/ibmphp_ebda.c b/drivers/pci/hotplug/ibmphp_ebda.c index 85528d6..402dc10 100644 --- a/drivers/pci/hotplug/ibmphp_ebda.c +++ b/drivers/pci/hotplug/ibmphp_ebda.c @@ -587,11 +587,14 @@ static u8 calculate_first_slot (u8 slot_num) return first_slot + 1; } + +#define SLOT_NAME_SIZE 30 + static char *create_file_name (struct slot * slot_cur) { struct opt_rio *opt_vg_ptr = NULL; struct opt_rio_lo *opt_lo_ptr = NULL; - static char str[30]; + static char str[SLOT_NAME_SIZE]; int which = 0; /* rxe = 1, chassis = 0 */ u8 number = 1; /* either chassis or rxe # */ u8 first_slot = 1; @@ -703,7 +706,6 @@ static void ...
We do not need to manage our own name parameter, especially since the PCI core can change it on our behalf, in the case of duplicate slot names. Remove 'name' from pciehp's version of struct slot, and remove unused 'task_list' as well. Cc: jbarnes@virtuousgeek.org Cc: kristen.c.accardi@intel.com Cc: kaneshige.kenji@jp.fujitsu.com Signed-off-by: Alex Chiang <achiang@hp.com> --- drivers/pci/hotplug/pciehp.h | 9 +++++-- drivers/pci/hotplug/pciehp_core.c | 25 ++++++++++--------- drivers/pci/hotplug/pciehp_ctrl.c | 48 +++++++++++++++++++------------------ 3 files changed, 43 insertions(+), 39 deletions(-) diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h index 217427a..3be5dde 100644 --- a/drivers/pci/hotplug/pciehp.h +++ b/drivers/pci/hotplug/pciehp.h @@ -61,15 +61,13 @@ extern struct workqueue_struct *pciehp_wq; struct slot { u8 bus; u8 device; - u32 number; u8 state; - struct timer_list task_event; u8 hp_slot; + u32 number; struct controller *ctrl; struct hpc_ops *hpc_ops; struct hotplug_slot *hotplug_slot; struct list_head slot_list; - char name[SLOT_NAME_SIZE]; unsigned long last_emi_toggle; struct delayed_work work; /* work for button event */ struct mutex lock; @@ -162,6 +160,11 @@ int pciehp_enable_slot(struct slot *p_slot); int pciehp_disable_slot(struct slot *p_slot); int pcie_enable_notification(struct controller *ctrl); +static inline const char *slot_name(struct slot *slot) +{ + return hotplug_slot_name(slot->hotplug_slot); +} + static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device) { struct slot *slot; diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c index bed77af..0466e64 100644 --- a/drivers/pci/hotplug/pciehp_core.c +++ b/drivers/pci/hotplug/pciehp_core.c @@ -180,7 +180,7 @@ static struct hotplug_slot_attribute hotplug_slot_attr_lock = { */ static void release_slot(struct hotplug_slot *hotplug_slot) ...
We no longer need to manage our version of hotplug_slot->name since the PCI and hotplug core manage it on our behalf. This means that alloc_slot_struct() no longer needs to take a 'name' param. On the other hand, we give rpaphp_register_slot() a 'name' param now to simplify the hotplug registration process. rpaphp_register_slot() can directly pass the drc_name to the PCI hotplug core. Cc: jbarnes@virtuousgeek.org Cc: kristen.c.accardi@intel.com Cc: benh@kernel.crashing.org Signed-off-by: Alex Chiang <achiang@hp.com> --- drivers/pci/hotplug/rpaphp.h | 10 +++++++--- drivers/pci/hotplug/rpaphp_core.c | 6 +++--- drivers/pci/hotplug/rpaphp_pci.c | 4 ++-- drivers/pci/hotplug/rpaphp_slot.c | 26 +++++++++----------------- 4 files changed, 21 insertions(+), 25 deletions(-) diff --git a/drivers/pci/hotplug/rpaphp.h b/drivers/pci/hotplug/rpaphp.h index 7d5921b..12f8aa7 100644 --- a/drivers/pci/hotplug/rpaphp.h +++ b/drivers/pci/hotplug/rpaphp.h @@ -73,13 +73,17 @@ struct slot { u32 index; u32 type; u32 power_domain; - char *name; struct device_node *dn; struct pci_bus *bus; struct list_head *pci_devs; struct hotplug_slot *hotplug_slot; }; +static inline const char *slot_name(struct slot *slot) +{ + return hotplug_slot_name(slot->hotplug_slot); +} + extern struct hotplug_slot_ops rpaphp_hotplug_slot_ops; extern struct list_head rpaphp_slot_head; @@ -96,8 +100,8 @@ extern int rpaphp_get_drc_props(struct device_node *dn, int *drc_index, /* rpaphp_slot.c */ extern void dealloc_slot_struct(struct slot *slot); -extern struct slot *alloc_slot_struct(struct device_node *dn, int drc_index, char *drc_name, int power_domain); -extern int rpaphp_register_slot(struct slot *slot); +extern struct slot *alloc_slot_struct(struct device_node *dn, int drc_index, int power_domain); +extern int rpaphp_register_slot(struct slot *slot, const char *name); extern int rpaphp_deregister_slot(struct slot *slot); #endif /* _PPC64PHP_H ...
We no longer need to manage our version of hotplug_slot->name since the PCI and hotplug core manage it on our behalf. Update the sn_hp_slot_private_alloc() interface to fill in the correct name for us, as that function already has all the parameters needed to determine the name. Cc: jbarnes@virtuousgeek.org Cc: kristen.c.accardi@intel.com Cc: jpk@sgi.com Signed-off-by: Alex Chiang <achiang@hp.com> --- drivers/pci/hotplug/sgi_hotplug.c | 19 ++++++------------- 1 files changed, 6 insertions(+), 13 deletions(-) diff --git a/drivers/pci/hotplug/sgi_hotplug.c b/drivers/pci/hotplug/sgi_hotplug.c index 6d20bbd..d748698 100644 --- a/drivers/pci/hotplug/sgi_hotplug.c +++ b/drivers/pci/hotplug/sgi_hotplug.c @@ -161,7 +161,8 @@ static int sn_pci_bus_valid(struct pci_bus *pci_bus) } static int sn_hp_slot_private_alloc(struct hotplug_slot *bss_hotplug_slot, - struct pci_bus *pci_bus, int device) + struct pci_bus *pci_bus, int device, + char *name) { struct pcibus_info *pcibus_info; struct slot *slot; @@ -173,15 +174,9 @@ static int sn_hp_slot_private_alloc(struct hotplug_slot *bss_hotplug_slot, return -ENOMEM; bss_hotplug_slot->private = slot; - bss_hotplug_slot->name = kmalloc(SN_SLOT_NAME_SIZE, GFP_KERNEL); - if (!bss_hotplug_slot->name) { - kfree(bss_hotplug_slot->private); - return -ENOMEM; - } - slot->device_num = device; slot->pci_bus = pci_bus; - sprintf(bss_hotplug_slot->name, "%04x:%02x:%02x", + sprintf(name, "%04x:%02x:%02x", pci_domain_nr(pci_bus), ((u16)pcibus_info->pbi_buscommon.bs_persist_busnum), device + 1); @@ -608,7 +603,6 @@ static inline int get_power_status(struct hotplug_slot *bss_hotplug_slot, static void sn_release_slot(struct hotplug_slot *bss_hotplug_slot) { kfree(bss_hotplug_slot->info); - kfree(bss_hotplug_slot->name); kfree(bss_hotplug_slot->private); kfree(bss_hotplug_slot); } @@ -618,6 +612,7 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus) int ...
We do not need to manage our own name parameter, especially since the PCI core can change it on our behalf, in the case of duplicate slot names. Remove 'name' from shpchp's version of struct slot. This change also removes the unused struct task_event from the slot structure. Cc: jbarnes@virtuousgeek.org Cc: kristen.c.accardi@intel.com Cc: kaneshige.kenji@jp.fujitsu.com Signed-off-by: Alex Chiang <achiang@hp.com> --- drivers/pci/hotplug/pciehp_hpc.c | 1 - drivers/pci/hotplug/shpchp.h | 9 +++++-- drivers/pci/hotplug/shpchp_core.c | 28 ++++++++++------------ drivers/pci/hotplug/shpchp_ctrl.c | 48 +++++++++++++++++++------------------ 4 files changed, 43 insertions(+), 43 deletions(-) diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 1cfe172..be5fc12 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -1044,7 +1044,6 @@ static int pcie_init_slot(struct controller *ctrl) slot->device = ctrl->slot_device_offset + slot->hp_slot; slot->hpc_ops = ctrl->hpc_ops; slot->number = ctrl->first_slot; - snprintf(slot->name, SLOT_NAME_SIZE, "%d", slot->number); mutex_init(&slot->lock); INIT_DELAYED_WORK(&slot->work, pciehp_queue_pushbutton_work); list_add(&slot->slot_list, &ctrl->slot_list); diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h index 8a026f7..4d9fed0 100644 --- a/drivers/pci/hotplug/shpchp.h +++ b/drivers/pci/hotplug/shpchp.h @@ -69,15 +69,13 @@ struct slot { u8 state; u8 presence_save; u8 pwr_save; - struct timer_list task_event; - u8 hp_slot; struct controller *ctrl; struct hpc_ops *hpc_ops; struct hotplug_slot *hotplug_slot; struct list_head slot_list; - char name[SLOT_NAME_SIZE]; struct delayed_work work; /* work for button event */ struct mutex lock; + u8 hp_slot; }; struct event_info { @@ -169,6 +167,11 @@ extern void cleanup_slots(struct controller *ctrl); extern void ...
Now that the PCI core manages the 'name' for each individual hotplug driver, and all drivers have been converted to use hotplug_slot_name(), there is no need for the PCI hotplug core to drag around its own copy of name either. Cc: jbarnes@virtuousgeek.org Cc: kristen.c.accardi@intel.com Cc: matthew@wil.cx Cc: kaneshige.kenji@jp.fujitsu.com Signed-off-by: Alex Chiang <achiang@hp.com> --- drivers/pci/hotplug/pci_hotplug_core.c | 6 +++--- include/linux/pci_hotplug.h | 3 --- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c index 2232608..c2ca365 100644 --- a/drivers/pci/hotplug/pci_hotplug_core.c +++ b/drivers/pci/hotplug/pci_hotplug_core.c @@ -533,7 +533,7 @@ static struct hotplug_slot *get_slot_from_name (const char *name) spin_lock(&pci_hotplug_slot_list_lock); list_for_each (tmp, &pci_hotplug_slot_list) { slot = list_entry (tmp, struct hotplug_slot, slot_list); - if (strcmp(slot->name, name) == 0) + if (strcmp(hotplug_slot_name(slot), name) == 0) goto out; } slot = NULL; @@ -640,7 +640,7 @@ int pci_hp_deregister(struct hotplug_slot *hotplug) if (!hotplug) return -ENODEV; - temp = get_slot_from_name(hotplug->name); + temp = get_slot_from_name(hotplug_slot_name(hotplug)); if (temp != hotplug) return -ENODEV; @@ -650,7 +650,7 @@ int pci_hp_deregister(struct hotplug_slot *hotplug) slot = hotplug->pci_slot; fs_remove_slot(slot); - dbg("Removed slot %s from the list\n", hotplug->name); + dbg("Removed slot %s from the list\n", hotplug_slot_name(hotplug)); hotplug->release(hotplug); slot->hotplug = NULL; diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h index 7184bee..9899ea3 100644 --- a/include/linux/pci_hotplug.h +++ b/include/linux/pci_hotplug.h @@ -142,8 +142,6 @@ struct hotplug_slot_info { /** * struct hotplug_slot - used to register a physical slot with the hotplug pci ...
