On Tue, Sep 09, 2008 at 04:00:12AM -0600, Alex Chiang wrote:
> 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
Not quite true ... the Mth registered slot becomes N-(M-1). With the
first '-' being a literal and the second being a minus sign ;-)
> - * Allow pcihp drivers to override the ACPI_PCI_SLOT name.
> + * Allow pcihp drivers to override existing slot name.
I would leave "the" in here.
> +static char *make_slot_name(const char *name)
> +{
> + char *new_name;
> + int len, width, dup = 1;
> + struct kobject *dup_slot;
> +
> + new_name = kstrdup(name, GFP_KERNEL);
> + if (!new_name)
> + goto out;
> +
> + /*
> + * Start off allocating enough room for "name-X"
> + */
> + len = strlen(name) + 2;
> + width = 1;
> +
> +try_again:
> + dup_slot = kset_find_obj(pci_slots_kset, new_name);
> + if (!dup_slot)
> + goto out;
> + kobject_put(dup_slot);
> +
> + /*
> + * We hit this the first time through, which gives us
> + * space for terminating NULL, and then every power of 10
> + * afterwards, which gives us space to add another digit
> + * to "name-XX..."
> + */
> + if (dup % width == 0) {
> + len++;
> + width *= 10;
> + }
> +
> + new_name = krealloc(new_name, len, GFP_KERNEL);
> + if (!new_name)
> + goto out;
> +
> + memset(new_name, 0, len);
> + scnprintf(new_name, len, "%s-%d", name, dup++);
> + goto try_again;
> +
> +out:
> + return new_name;
> +}
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;
}
> @@ -89,7 +134,19 @@ static struct kobj_type pci_slot_ktype = {
> * either return a new &struct pci_slot to the caller, or if the pci_slot
> * already exists, its refcount will be incremented.
> *
> - * Slots are uniquely identified by a @pci_bus, @slot_nr, @name tuple.
> + * Slots are uniquely identified by a @pci_bus, @slot_nr tuple.
> + *
> + * The kobject API imposes a restriction on us, and does not allow sysfs
> + * entries with duplicate names. There are known platforms with broken
> + * firmware that assign the same name to multiple slots.
> + *
> + * We workaround these broken platforms by renaming the slots on behalf
> + * of the caller. If firmware assigns name N to multiple slots:
> + *
> + * The first slot is assigned N
> + * The second slot is assigned N-1
> + * The third slot is assigned N-2
> + * The Mth slot is assigned N-M
How about simply replacing this last line with:
> + * etc.
> struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
> const char *name)
> {
> struct pci_dev *dev;
> struct pci_slot *slot;
> - int err;
> + int err = 0;
> + char *slot_name = NULL;
>
> down_write(&pci_bus_sem);
>
> @@ -144,12 +201,18 @@ placeholder:
>
> slot->bus = parent;
> slot->number = slot_nr;
> -
> slot->kobj.kset = pci_slots_kset;
> +
> + slot_name = make_slot_name(name);
> + if (!slot_name) {
> + slot = ERR_PTR(-ENOMEM);
> + goto err;
> + }
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;
}
> -void pci_update_slot_number(struct pci_slot *slot, int slot_nr)
> +void pci_renumber_slot(struct pci_slot *slot, int slot_nr)
> {
> - int name_count = 0;
> struct pci_slot *tmp;
>
> down_write(&pci_bus_sem);
>
> - list_for_each_entry(tmp, &slot->bus->slots, list) {
> + list_for_each_entry(tmp, &slot->bus->slots, list)
> WARN_ON(tmp->number == slot_nr);
> - if (!strcmp(kobject_name(&tmp->kobj), kobject_name(&slot->kobj)))
> - name_count++;
> - }
> -
> - if (name_count > 1)
> - printk(KERN_WARNING "pci_update_slot_number found %d slots with the same name: %s\n", name_count, kobject_name(&slot->kobj));
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."
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
