[PATCH v5 00/16] PCI: let the core manage slot names - Kernel

This is a discussion on [PATCH v5 00/16] PCI: let the core manage slot names - Kernel ; Hi all, This is v5 of the series that implements a series of changes that allows the PCI core to manage slot names, rather than individual hotplug drivers. Thanks again to Kenji-san for continuing to point out that my previous ...

+ Reply to Thread
Page 1 of 2 1 2 LastLast
Results 1 to 20 of 21

Thread: [PATCH v5 00/16] PCI: let the core manage slot names

  1. [PATCH v5 00/16] PCI: let the core manage slot names

    Hi all,

    This is v5 of the series that implements a series of changes
    that allows the PCI core to manage slot names, rather than
    individual hotplug drivers.

    Thanks again to Kenji-san for continuing to point out that my
    previous attempts were racy and had horrible, complicated code.
    And thanks to Willy for reviewing my poor attempt at introducing
    locking.

    I've decided to do them both a favor and take a new approach that
    does not change any locking semantics in any of the existing interfaces
    today (pci_hp_register, pci_hp_deregister, pci_create_slot).

    Keep in mind that the problems we're trying to solve are:

    - possible duplicate slot names from firmware
    - allow hotplug drivers to override detection driver slot names

    The race that Kenji-san points out occurs during slot creation, where
    both a detection driver and hotplug driver try to create a slot at
    the same time, with different names.

    The solution is to combine slot creation _and_ slot rename into one
    atomic operation via an updated pci_create_slot() API. pci_create_slot
    is already serialized, so that's a good start.

    We add a 'rename' parameter to pci_create_slot. The hotplug drivers set
    it to 1. The detection drivers set it to 0. Now, when trying to create
    a new slot:

    - check to see if it's already been created
    - if yes, and we want a rename, then go ahead and rename it
    - if yes, and we don't care about the name, then just return
    - if no, then create the slot normally

    This logic is fully described in the changelog in patch 04/16. I believe
    it is a much simpler approach than the earlier crap I was producing.

    I tested this series with the pci_slot driver and fakephp dup_slots=1
    with multiple loads and unloads in different order, and it all seemed
    to work.

    I _hope_ this is the last round.

    Thanks!

    /ac

    v4 -> v5:
    - add 'rename' param to pci_create_slot
    - make use of 'rename' param in pci_create_slot
    - remove v4 serialization
    - remove crap false name collsion code from v2 and v3
    - rpaphp uses kstrdup (Thanks to Pekka Enberg for suggestion)

    v3 -> v4:
    - Do not access hotplug_slot_name() before name initialization
    - Serialize pci_hp_register/deregister

    v2 -> v3:
    - incorporate Willy's code review comments
    - fix possible memory leak, pointed out by Rolf Eike Beer
    - make false name collision detection work for empty slots
    - add 'dup_slots' module_param to fakephp to help debug all this

    v1 -> v2:
    - fix possible false name collisions

    ---

    Alex Chiang (16):
    PCI Hotplug: fakephp: add duplicate slot name debugging
    PCI: Hotplug core: remove 'name'
    PCI: shcphp: remove 'name' parameter
    PCI: SGI Hotplug: stop managing bss_hotplug_slot->name
    PCI: rpaphp: kmalloc/kfree slot->name directly
    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: update pci_create_slot() to take a 'rename' param
    PCI: rename pci_update_slot_number to pci_renumber_slot
    PCI Hotplug core: add 'name' param pci_hp_register interface


    drivers/acpi/pci_slot.c | 2
    drivers/pci/hotplug/acpiphp.h | 9 +-
    drivers/pci/hotplug/acpiphp_core.c | 32 ++++---
    drivers/pci/hotplug/cpci_hotplug.h | 6 +
    drivers/pci/hotplug/cpci_hotplug_core.c | 75 ++++++----------
    drivers/pci/hotplug/cpci_hotplug_pci.c | 4 -
    drivers/pci/hotplug/cpqphp.h | 13 +--
    drivers/pci/hotplug/cpqphp_core.c | 43 ++++-----
    drivers/pci/hotplug/fakephp.c | 26 ++++-
    drivers/pci/hotplug/ibmphp.h | 5 -
    drivers/pci/hotplug/ibmphp_ebda.c | 19 +---
    drivers/pci/hotplug/pci_hotplug_core.c | 30 ++----
    drivers/pci/hotplug/pciehp.h | 9 +-
    drivers/pci/hotplug/pciehp_core.c | 49 ++++------
    drivers/pci/hotplug/pciehp_ctrl.c | 53 ++++++-----
    drivers/pci/hotplug/pciehp_hpc.c | 1
    drivers/pci/hotplug/rpaphp_slot.c | 10 +-
    drivers/pci/hotplug/sgi_hotplug.c | 18 +---
    drivers/pci/hotplug/shpchp.h | 9 +-
    drivers/pci/hotplug/shpchp_core.c | 52 ++++-------
    drivers/pci/hotplug/shpchp_ctrl.c | 48 +++++-----
    drivers/pci/slot.c | 150 ++++++++++++++++++++++++-------
    include/linux/pci.h | 9 +-
    include/linux/pci_hotplug.h | 11 +-
    24 files changed, 359 insertions(+), 324 deletions(-)

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. [PATCH v5 07/16] PCI: cpci_hotplug: stop managing hotplug_slot->name

    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
    ---

    drivers/pci/hotplug/cpci_hotplug.h | 6 ++
    drivers/pci/hotplug/cpci_hotplug_core.c | 76 ++++++++++++-------------------
    drivers/pci/hotplug/cpci_hotplug_pci.c | 4 +-
    3 files changed, 37 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
    #include
    +#include

    /* 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..de94f4f 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 @@ disable_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));

    down_write(&list_rwsem);

    /* Unconfigure device */
    - dbg("%s - unconfiguring slot %s",
    - __func__, slot->hotplug_slot->name);
    + dbg("%s - unconfiguring slot %s", __func__, slot_name(slot));
    if ((retval = cpci_unconfigure_slot(slot))) {
    err("%s - could not unconfigure slot %s",
    - __func__, slot->hotplug_slot->name);
    + __func__, slot_name(slot));
    goto disable_error;
    }
    - dbg("%s - finished unconfiguring slot %s",
    - __func__, slot->hotplug_slot->name);
    + dbg("%s - finished unconfiguring slot %s", __func__, slot_name(slot));

    /* Clear EXT (by setting it) */
    if (cpci_clear_ext(slot)) {
    err("%s - could not clear EXT for slot %s",
    - __func__, slot->hotplug_slot->name);
    + __func__, slot_name(slot));
    retval = -ENODEV;
    goto disable_error;
    }
    @@ -214,7 +212,6 @@ static void release_slot(struct hotplug_slot *hotplug_slot)
    struct slot *slot = hotplug_slot->private;

    kfree(slot->hotplug_slot->info);
    - kfree(slot->hotplug_slot->name);
    kfree(slot->hotplug_slot);
    if (slot->dev)
    pci_dev_put(slot->dev);
    @@ -222,12 +219,6 @@ static void release_slot(struct hotplug_slot *hotplug_slot)
    }

    #define SLOT_NAME_SIZE 6
    -static void
    -make_slot_name(struct slot *slot)
    -{
    - snprintf(slot->hotplug_slot->name,
    - SLOT_NAME_SIZE, "%02x:%02x", slot->bus->number, slot->number);
    -}

    int
    cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last)
    @@ -235,7 +226,7 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last)
    struct slot *slot;
    struct hotplug_slot *hotplug_slot;
    struct hotplug_slot_info *info;
    - char *name;
    + char name[SLOT_NAME_SIZE];
    int status = -ENOMEM;
    int i;

    @@ -262,35 +253,31 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last)
    goto error_hpslot;
    hotplug_slot->info = info;

    - name = kmalloc(SLOT_NAME_SIZE, GFP_KERNEL);
    - if (!name)
    - goto error_info;
    - hotplug_slot->name = name;
    -
    slot->bus = bus;
    slot->number = i;
    slot->devfn = PCI_DEVFN(i, 0);

    + snprintf(name, SLOT_NAME_SIZE, "%02x:%02x", bus->number, i);
    +
    hotplug_slot->private = slot;
    hotplug_slot->release = &release_slot;
    - make_slot_name(slot);
    hotplug_slot->ops = &cpci_hotplug_slot_ops;

    /*
    * Initialize the slot info structure with some known
    * good values.
    */
    - dbg("initializing slot %s", slot->hotplug_slot->name);
    + dbg("initializing slot %s", name);
    info->power_status = cpci_get_power_status(slot);
    info->attention_status = cpci_get_attention_status(slot);

    - dbg("registering slot %s", slot->hotplug_slot->name);
    - status = pci_hp_register(slot->hotplug_slot, bus, i,
    - slot->hotplug_slot->name);
    + dbg("registering slot %s", name);
    + status = pci_hp_register(slot->hotplug_slot, bus, i, name);
    if (status) {
    err("pci_hp_register failed with error %d", status);
    - goto error_name;
    + goto error_info;
    }
    + dbg("slot registered with name: %s", slot_name(slot));

    /* Add slot to our internal list */
    down_write(&list_rwsem);
    @@ -299,8 +286,6 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last)
    up_write(&list_rwsem);
    }
    return 0;
    -error_name:
    - kfree(name);
    error_info:
    kfree(info);
    error_hpslot:
    @@ -328,7 +313,7 @@ cpci_hp_unregister_bus(struct pci_bus *bus)
    list_del(&slot->slot_list);
    slots--;

    - dbg("deregistering slot %s", slot->hotplug_slot->name);
    + dbg("deregistering slot %s", slot_name(slot));
    status = pci_hp_deregister(slot->hotplug_slot);
    if (status) {
    err("pci_hp_deregister failed with error %d",
    @@ -380,11 +365,10 @@ init_slots(int clear_ins)
    return -1;
    }
    list_for_each_entry(slot, &slot_list, slot_list) {
    - dbg("%s - looking at slot %s",
    - __func__, slot->hotplug_slot->name);
    + dbg("%s - looking at slot %s", __func__, slot_name(slot));
    if (clear_ins && cpci_check_and_clear_ins(slot))
    dbg("%s - cleared INS for slot %s",
    - __func__, slot->hotplug_slot->name);
    + __func__, slot_name(slot));
    dev = pci_get_slot(slot->bus, PCI_DEVFN(slot->number, 0));
    if (dev) {
    if (update_adapter_status(slot->hotplug_slot, 1))
    @@ -415,8 +399,7 @@ check_slots(void)
    }
    extracted = inserted = 0;
    list_for_each_entry(slot, &slot_list, slot_list) {
    - dbg("%s - looking at slot %s",
    - __func__, slot->hotplug_slot->name);
    + dbg("%s - looking at slot %s", __func__, slot_name(slot));
    if (cpci_check_and_clear_ins(slot)) {
    /*
    * Some broken hardware (e.g. PLX 9054AB) asserts
    @@ -424,35 +407,34 @@ check_slots(void)
    */
    if (slot->dev) {
    warn("slot %s already inserted",
    - slot->hotplug_slot->name);
    + slot_name(slot));
    inserted++;
    continue;
    }

    /* Process insertion */
    - dbg("%s - slot %s inserted",
    - __func__, slot->hotplug_slot->name);
    + dbg("%s - slot %s inserted", __func__, slot_name(slot));

    /* GSM, debug */
    hs_csr = cpci_get_hs_csr(slot);
    dbg("%s - slot %s HS_CSR (1) = %04x",
    - __func__, slot->hotplug_slot->name, hs_csr);
    + __func__, slot_name(slot), hs_csr);

    /* Configure device */
    dbg("%s - configuring slot %s",
    - __func__, slot->hotplug_slot->name);
    + __func__, slot_name(slot));
    if (cpci_configure_slot(slot)) {
    err("%s - could not configure slot %s",
    - __func__, slot->hotplug_slot->name);
    + __func__, slot_name(slot));
    continue;
    }
    dbg("%s - finished configuring slot %s",
    - __func__, slot->hotplug_slot->name);
    + __func__, slot_name(slot));

    /* GSM, debug */
    hs_csr = cpci_get_hs_csr(slot);
    dbg("%s - slot %s HS_CSR (2) = %04x",
    - __func__, slot->hotplug_slot->name, hs_csr);
    + __func__, slot_name(slot), hs_csr);

    if (update_latch_status(slot->hotplug_slot, 1))
    warn("failure to update latch file");
    @@ -465,18 +447,18 @@ check_slots(void)
    /* GSM, debug */
    hs_csr = cpci_get_hs_csr(slot);
    dbg("%s - slot %s HS_CSR (3) = %04x",
    - __func__, slot->hotplug_slot->name, hs_csr);
    + __func__, slot_name(slot), hs_csr);

    inserted++;
    } else if (cpci_check_ext(slot)) {
    /* Process extraction request */
    dbg("%s - slot %s extracted",
    - __func__, slot->hotplug_slot->name);
    + __func__, slot_name(slot));

    /* GSM, debug */
    hs_csr = cpci_get_hs_csr(slot);
    dbg("%s - slot %s HS_CSR = %04x",
    - __func__, slot->hotplug_slot->name, hs_csr);
    + __func__, slot_name(slot), hs_csr);

    if (!slot->extracting) {
    if (update_latch_status(slot->hotplug_slot, 0)) {
    @@ -494,7 +476,7 @@ check_slots(void)
    * bother trying to tell the driver or not?
    */
    err("card in slot %s was improperly removed",
    - slot->hotplug_slot->name);
    + slot_name(slot));
    if (update_adapter_status(slot->hotplug_slot, 0))
    warn("failure to update adapter file");
    slot->extracting = 0;
    diff --git a/drivers/pci/hotplug/cpci_hotplug_pci.c b/drivers/pci/hotplug/cpci_hotplug_pci.c
    index df82b95..829c327 100644
    --- a/drivers/pci/hotplug/cpci_hotplug_pci.c
    +++ b/drivers/pci/hotplug/cpci_hotplug_pci.c
    @@ -209,7 +209,7 @@ int cpci_led_on(struct slot* slot)
    hs_cap + 2,
    hs_csr)) {
    err("Could not set LOO for slot %s",
    - slot->hotplug_slot->name);
    + hotplug_slot_name(slot->hotplug_slot));
    return -ENODEV;
    }
    }
    @@ -238,7 +238,7 @@ int cpci_led_off(struct slot* slot)
    hs_cap + 2,
    hs_csr)) {
    err("Could not clear LOO for slot %s",
    - slot->hotplug_slot->name);
    + hotplug_slot_name(slot->hotplug_slot));
    return -ENODEV;
    }
    }

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. [PATCH v5 08/16] PCI: cpqphp: stop managing hotplug_slot->name

    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
    ---

    drivers/pci/hotplug/cpqphp.h | 13 ++++-------
    drivers/pci/hotplug/cpqphp_core.c | 42 +++++++++++++++++--------------------
    2 files changed, 24 insertions(+), 31 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..724d42c 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 *ctrl,
    void __iomem *smbios_start,
    void __iomem *smbios_table)
    @@ -335,6 +336,7 @@ static int ctrl_slot_setup(struct controller *ctrl,
    u8 slot_number;
    u8 ctrl_slot;
    u32 tempdword;
    + char name[SLOT_NAME_SIZE];
    void __iomem *slot_entry= NULL;
    int result = -ENOMEM;

    @@ -363,16 +365,12 @@ static int ctrl_slot_setup(struct controller *ctrl,
    if (!hotplug_slot->info)
    goto error_hpslot;
    hotplug_slot_info = hotplug_slot->info;
    - hotplug_slot->name = kmalloc(SLOT_NAME_SIZE, GFP_KERNEL);
    -
    - if (!hotplug_slot->name)
    - goto error_info;

    slot->ctrl = ctrl;
    slot->bus = ctrl->bus;
    slot->device = slot_device;
    slot->number = slot_number;
    - dbg("slot->number = %d\n", slot->number);
    + dbg("slot->number = %u\n", slot->number);

    slot_entry = get_SMBIOS_entry(smbios_start, smbios_table, 9,
    slot_entry);
    @@ -418,9 +416,9 @@ static int ctrl_slot_setup(struct controller *ctrl,
    /* register this slot with the hotplug pci core */
    hotplug_slot->release = &release_slot;
    hotplug_slot->private = slot;
    - make_slot_name(hotplug_slot->name, SLOT_NAME_SIZE, slot);
    + snprintf(name, SLOT_NAME_SIZE, "%u", slot->number);
    hotplug_slot->ops = &cpqphp_hotplug_slot_ops;
    -
    +
    hotplug_slot_info->power_status = get_slot_enabled(ctrl, slot);
    hotplug_slot_info->attention_status =
    cpq_get_attention_status(ctrl, slot);
    @@ -437,10 +435,10 @@ static int ctrl_slot_setup(struct controller *ctrl,
    result = pci_hp_register(hotplug_slot,
    ctrl->pci_dev->subordinate,
    slot->device,
    - hotplug_slot->name);
    + name);
    if (result) {
    err("pci_hp_register failed with error %d\n", result);
    - goto error_name;
    + goto error_info;
    }

    slot->next = ctrl->slot;
    @@ -452,8 +450,6 @@ static int ctrl_slot_setup(struct controller *ctrl,
    }

    return 0;
    -error_name:
    - kfree(hotplug_slot->name);
    error_info:
    kfree(hotplug_slot_info);
    error_hpslot:
    @@ -639,7 +635,7 @@ static int set_attention_status (struct hotplug_slot *hotplug_slot, u8 status)
    u8 device;
    u8 function;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    if (cpqhp_get_bus_dev(ctrl, &bus, &devfn, slot->number) == -1)
    return -ENODEV;
    @@ -666,7 +662,7 @@ static int process_SI(struct hotplug_slot *hotplug_slot)
    u8 device;
    u8 function;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    if (cpqhp_get_bus_dev(ctrl, &bus, &devfn, slot->number) == -1)
    return -ENODEV;
    @@ -698,7 +694,7 @@ static int process_SS(struct hotplug_slot *hotplug_slot)
    u8 device;
    u8 function;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    if (cpqhp_get_bus_dev(ctrl, &bus, &devfn, slot->number) == -1)
    return -ENODEV;
    @@ -721,7 +717,7 @@ static int hardware_test(struct hotplug_slot *hotplug_slot, u32 value)
    struct slot *slot = hotplug_slot->private;
    struct controller *ctrl = slot->ctrl;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    return cpqhp_hardware_test(ctrl, value);
    }
    @@ -732,7 +728,7 @@ static int get_power_status(struct hotplug_slot *hotplug_slot, u8 *value)
    struct slot *slot = hotplug_slot->private;
    struct controller *ctrl = slot->ctrl;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    *value = get_slot_enabled(ctrl, slot);
    return 0;
    @@ -743,7 +739,7 @@ static int get_attention_status(struct hotplug_slot *hotplug_slot, u8 *value)
    struct slot *slot = hotplug_slot->private;
    struct controller *ctrl = slot->ctrl;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    *value = cpq_get_attention_status(ctrl, slot);
    return 0;
    @@ -754,7 +750,7 @@ static int get_latch_status(struct hotplug_slot *hotplug_slot, u8 *value)
    struct slot *slot = hotplug_slot->private;
    struct controller *ctrl = slot->ctrl;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    *value = cpq_get_latch_status(ctrl, slot);

    @@ -766,7 +762,7 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
    struct slot *slot = hotplug_slot->private;
    struct controller *ctrl = slot->ctrl;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    *value = get_presence_status(ctrl, slot);

    @@ -778,7 +774,7 @@ static int get_max_bus_speed (struct hotplug_slot *hotplug_slot, enum pci_bus_sp
    struct slot *slot = hotplug_slot->private;
    struct controller *ctrl = slot->ctrl;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    *value = ctrl->speed_capability;

    @@ -790,7 +786,7 @@ static int get_cur_bus_speed (struct hotplug_slot *hotplug_slot, enum pci_bus_sp
    struct slot *slot = hotplug_slot->private;
    struct controller *ctrl = slot->ctrl;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    *value = ctrl->speed;


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. [PATCH v5 11/16] PCI: pciehp: remove 'name' parameter

    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
    ---

    drivers/pci/hotplug/pciehp.h | 9 ++++--
    drivers/pci/hotplug/pciehp_core.c | 33 ++++++++++++-----------
    drivers/pci/hotplug/pciehp_ctrl.c | 53 ++++++++++++++++++++-----------------
    drivers/pci/hotplug/pciehp_hpc.c | 1 -
    4 files changed, 52 insertions(+), 44 deletions(-)

    diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
    index c367978..394f998 100644
    --- a/drivers/pci/hotplug/pciehp.h
    +++ b/drivers/pci/hotplug/pciehp.h
    @@ -74,15 +74,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;
    @@ -175,6 +173,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 af89d7b..62be1b5 100644
    --- a/drivers/pci/hotplug/pciehp_core.c
    +++ b/drivers/pci/hotplug/pciehp_core.c
    @@ -185,7 +185,7 @@ static void release_slot(struct hotplug_slot *hotplug_slot)
    struct slot *slot = hotplug_slot->private;

    ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
    - __func__, hotplug_slot->name);
    + __func__, hotplug_slot_name(hotplug_slot));

    kfree(hotplug_slot->info);
    kfree(hotplug_slot);
    @@ -196,6 +196,7 @@ static int init_slots(struct controller *ctrl)
    struct slot *slot;
    struct hotplug_slot *hotplug_slot;
    struct hotplug_slot_info *info;
    + char name[SLOT_NAME_SIZE];
    int retval = -ENOMEM;

    list_for_each_entry(slot, &ctrl->slot_list, slot_list) {
    @@ -209,15 +210,11 @@ static int init_slots(struct controller *ctrl)

    /* register this slot with the hotplug pci core */
    hotplug_slot->info = info;
    - hotplug_slot->name = slot->name;
    hotplug_slot->private = slot;
    hotplug_slot->release = &release_slot;
    hotplug_slot->ops = &pciehp_hotplug_slot_ops;
    - get_power_status(hotplug_slot, &info->power_status);
    - get_attention_status(hotplug_slot, &info->attention_status);
    - get_latch_status(hotplug_slot, &info->latch_status);
    - get_adapter_status(hotplug_slot, &info->adapter_status);
    slot->hotplug_slot = hotplug_slot;
    + snprintf(name, SLOT_NAME_SIZE, "%u", slot->number);

    ctrl_dbg(ctrl, "Registering bus=%x dev=%x hp_slot=%x sun=%x "
    "slot_device_offset=%x\n", slot->bus, slot->device,
    @@ -225,12 +222,16 @@ static int init_slots(struct controller *ctrl)
    retval = pci_hp_register(hotplug_slot,
    ctrl->pci_dev->subordinate,
    slot->device,
    - slot->name);
    + name);
    if (retval) {
    ctrl_err(ctrl, "pci_hp_register failed with error %d\n",
    retval);
    goto error_info;
    }
    + get_power_status(hotplug_slot, &info->power_status);
    + get_attention_status(hotplug_slot, &info->attention_status);
    + get_latch_status(hotplug_slot, &info->latch_status);
    + get_adapter_status(hotplug_slot, &info->adapter_status);
    /* create additional sysfs entries */
    if (EMI(ctrl)) {
    retval = sysfs_create_file(&hotplug_slot->pci_slot->kobj,
    @@ -273,7 +274,7 @@ static int set_attention_status(struct hotplug_slot *hotplug_slot, u8 status)
    struct slot *slot = hotplug_slot->private;

    ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
    - __func__, hotplug_slot->name);
    + __func__, slot_name(slot));

    hotplug_slot->info->attention_status = status;

    @@ -289,7 +290,7 @@ static int enable_slot(struct hotplug_slot *hotplug_slot)
    struct slot *slot = hotplug_slot->private;

    ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
    - __func__, hotplug_slot->name);
    + __func__, slot_name(slot));

    return pciehp_sysfs_enable_slot(slot);
    }
    @@ -300,7 +301,7 @@ static int disable_slot(struct hotplug_slot *hotplug_slot)
    struct slot *slot = hotplug_slot->private;

    ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
    - __func__, hotplug_slot->name);
    + __func__, slot_name(slot));

    return pciehp_sysfs_disable_slot(slot);
    }
    @@ -311,7 +312,7 @@ static int get_power_status(struct hotplug_slot *hotplug_slot, u8 *value)
    int retval;

    ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
    - __func__, hotplug_slot->name);
    + __func__, slot_name(slot));

    retval = slot->hpc_ops->get_power_status(slot, value);
    if (retval < 0)
    @@ -326,7 +327,7 @@ static int get_attention_status(struct hotplug_slot *hotplug_slot, u8 *value)
    int retval;

    ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
    - __func__, hotplug_slot->name);
    + __func__, slot_name(slot));

    retval = slot->hpc_ops->get_attention_status(slot, value);
    if (retval < 0)
    @@ -341,7 +342,7 @@ static int get_latch_status(struct hotplug_slot *hotplug_slot, u8 *value)
    int retval;

    ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
    - __func__, hotplug_slot->name);
    + __func__, slot_name(slot));

    retval = slot->hpc_ops->get_latch_status(slot, value);
    if (retval < 0)
    @@ -356,7 +357,7 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
    int retval;

    ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
    - __func__, hotplug_slot->name);
    + __func__, slot_name(slot));

    retval = slot->hpc_ops->get_adapter_status(slot, value);
    if (retval < 0)
    @@ -372,7 +373,7 @@ static int get_max_bus_speed(struct hotplug_slot *hotplug_slot,
    int retval;

    ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
    - __func__, hotplug_slot->name);
    + __func__, slot_name(slot));

    retval = slot->hpc_ops->get_max_bus_speed(slot, value);
    if (retval < 0)
    @@ -387,7 +388,7 @@ static int get_cur_bus_speed(struct hotplug_slot *hotplug_slot, enum pci_bus_spe
    int retval;

    ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
    - __func__, hotplug_slot->name);
    + __func__, slot_name(slot));

    retval = slot->hpc_ops->get_cur_bus_speed(slot, value);
    if (retval < 0)
    diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
    index acb7f9e..cce6e5f 100644
    --- a/drivers/pci/hotplug/pciehp_ctrl.c
    +++ b/drivers/pci/hotplug/pciehp_ctrl.c
    @@ -66,7 +66,7 @@ u8 pciehp_handle_attention_button(struct slot *p_slot)
    /*
    * Button pressed - See if need to TAKE ACTION!!!
    */
    - ctrl_info(ctrl, "Button pressed on Slot(%s)\n", p_slot->name);
    + ctrl_info(ctrl, "Button pressed on Slot(%s)\n", slot_name(p_slot));
    event_type = INT_BUTTON_PRESS;

    queue_interrupt_event(p_slot, event_type);
    @@ -88,13 +88,13 @@ u8 pciehp_handle_switch_change(struct slot *p_slot)
    /*
    * Switch opened
    */
    - ctrl_info(ctrl, "Latch open on Slot(%s)\n", p_slot->name);
    + ctrl_info(ctrl, "Latch open on Slot(%s)\n", slot_name(p_slot));
    event_type = INT_SWITCH_OPEN;
    } else {
    /*
    * Switch closed
    */
    - ctrl_info(ctrl, "Latch close on Slot(%s)\n", p_slot->name);
    + ctrl_info(ctrl, "Latch close on Slot(%s)\n", slot_name(p_slot));
    event_type = INT_SWITCH_CLOSE;
    }

    @@ -120,13 +120,14 @@ u8 pciehp_handle_presence_change(struct slot *p_slot)
    /*
    * Card Present
    */
    - ctrl_info(ctrl, "Card present on Slot(%s)\n", p_slot->name);
    + ctrl_info(ctrl, "Card present on Slot(%s)\n", slot_name(p_slot));
    event_type = INT_PRESENCE_ON;
    } else {
    /*
    * Not Present
    */
    - ctrl_info(ctrl, "Card not present on Slot(%s)\n", p_slot->name);
    + ctrl_info(ctrl, "Card not present on Slot(%s)\n",
    + slot_name(p_slot));
    event_type = INT_PRESENCE_OFF;
    }

    @@ -148,13 +149,13 @@ u8 pciehp_handle_power_fault(struct slot *p_slot)
    * power fault Cleared
    */
    ctrl_info(ctrl, "Power fault cleared on Slot(%s)\n",
    - p_slot->name);
    + slot_name(p_slot));
    event_type = INT_POWER_FAULT_CLEAR;
    } else {
    /*
    * power fault
    */
    - ctrl_info(ctrl, "Power fault on Slot(%s)\n", p_slot->name);
    + ctrl_info(ctrl, "Power fault on Slot(%s)\n", slot_name(p_slot));
    event_type = INT_POWER_FAULT;
    ctrl_info(ctrl, "power fault bit %x set\n", 0);
    }
    @@ -412,12 +413,12 @@ static void handle_button_press_event(struct slot *p_slot)
    p_slot->state = BLINKINGOFF_STATE;
    ctrl_info(ctrl,
    "PCI slot #%s - powering off due to button "
    - "press.\n", p_slot->name);
    + "press.\n", slot_name(p_slot));
    } else {
    p_slot->state = BLINKINGON_STATE;
    ctrl_info(ctrl,
    "PCI slot #%s - powering on due to button "
    - "press.\n", p_slot->name);
    + "press.\n", slot_name(p_slot));
    }
    /* blink green LED and turn off amber */
    if (PWR_LED(ctrl))
    @@ -434,7 +435,7 @@ static void handle_button_press_event(struct slot *p_slot)
    * press the attention again before the 5 sec. limit
    * expires to cancel hot-add or hot-remove
    */
    - ctrl_info(ctrl, "Button cancel on Slot(%s)\n", p_slot->name);
    + ctrl_info(ctrl, "Button cancel on Slot(%s)\n", slot_name(p_slot));
    ctrl_dbg(ctrl, "%s: button cancel\n", __func__);
    cancel_delayed_work(&p_slot->work);
    if (p_slot->state == BLINKINGOFF_STATE) {
    @@ -447,7 +448,7 @@ static void handle_button_press_event(struct slot *p_slot)
    if (ATTN_LED(ctrl))
    p_slot->hpc_ops->set_attention_status(p_slot, 0);
    ctrl_info(ctrl, "PCI slot #%s - action canceled "
    - "due to button press\n", p_slot->name);
    + "due to button press\n", slot_name(p_slot));
    p_slot->state = STATIC_STATE;
    break;
    case POWEROFF_STATE:
    @@ -457,7 +458,7 @@ static void handle_button_press_event(struct slot *p_slot)
    * this means that the previous attention button action
    * to hot-add or hot-remove is undergoing
    */
    - ctrl_info(ctrl, "Button ignore on Slot(%s)\n", p_slot->name);
    + ctrl_info(ctrl, "Button ignore on Slot(%s)\n", slot_name(p_slot));
    update_slot_info(p_slot);
    break;
    default:
    @@ -540,7 +541,7 @@ int pciehp_enable_slot(struct slot *p_slot)
    rc = p_slot->hpc_ops->get_adapter_status(p_slot, &getstatus);
    if (rc || !getstatus) {
    ctrl_info(ctrl, "%s: no adapter on slot(%s)\n",
    - __func__, p_slot->name);
    + __func__, slot_name(p_slot));
    mutex_unlock(&p_slot->ctrl->crit_sect);
    return -ENODEV;
    }
    @@ -548,7 +549,7 @@ int pciehp_enable_slot(struct slot *p_slot)
    rc = p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
    if (rc || getstatus) {
    ctrl_info(ctrl, "%s: latch open on slot(%s)\n",
    - __func__, p_slot->name);
    + __func__, slot_name(p_slot));
    mutex_unlock(&p_slot->ctrl->crit_sect);
    return -ENODEV;
    }
    @@ -558,7 +559,7 @@ int pciehp_enable_slot(struct slot *p_slot)
    rc = p_slot->hpc_ops->get_power_status(p_slot, &getstatus);
    if (rc || getstatus) {
    ctrl_info(ctrl, "%s: already enabled on slot(%s)\n",
    - __func__, p_slot->name);
    + __func__, slot_name(p_slot));
    mutex_unlock(&p_slot->ctrl->crit_sect);
    return -EINVAL;
    }
    @@ -594,7 +595,7 @@ int pciehp_disable_slot(struct slot *p_slot)
    ret = p_slot->hpc_ops->get_adapter_status(p_slot, &getstatus);
    if (ret || !getstatus) {
    ctrl_info(ctrl, "%s: no adapter on slot(%s)\n",
    - __func__, p_slot->name);
    + __func__, slot_name(p_slot));
    mutex_unlock(&p_slot->ctrl->crit_sect);
    return -ENODEV;
    }
    @@ -604,7 +605,7 @@ int pciehp_disable_slot(struct slot *p_slot)
    ret = p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
    if (ret || getstatus) {
    ctrl_info(ctrl, "%s: latch open on slot(%s)\n",
    - __func__, p_slot->name);
    + __func__, slot_name(p_slot));
    mutex_unlock(&p_slot->ctrl->crit_sect);
    return -ENODEV;
    }
    @@ -614,7 +615,7 @@ int pciehp_disable_slot(struct slot *p_slot)
    ret = p_slot->hpc_ops->get_power_status(p_slot, &getstatus);
    if (ret || !getstatus) {
    ctrl_info(ctrl, "%s: already disabled slot(%s)\n",
    - __func__, p_slot->name);
    + __func__, slot_name(p_slot));
    mutex_unlock(&p_slot->ctrl->crit_sect);
    return -EINVAL;
    }
    @@ -645,14 +646,16 @@ int pciehp_sysfs_enable_slot(struct slot *p_slot)
    break;
    case POWERON_STATE:
    ctrl_info(ctrl, "Slot %s is already in powering on state\n",
    - p_slot->name);
    + slot_name(p_slot));
    break;
    case BLINKINGOFF_STATE:
    case POWEROFF_STATE:
    - ctrl_info(ctrl, "Already enabled on slot %s\n", p_slot->name);
    + ctrl_info(ctrl, "Already enabled on slot %s\n",
    + slot_name(p_slot));
    break;
    default:
    - ctrl_err(ctrl, "Not a valid state on slot %s\n", p_slot->name);
    + ctrl_err(ctrl, "Not a valid state on slot %s\n",
    + slot_name(p_slot));
    break;
    }
    mutex_unlock(&p_slot->lock);
    @@ -678,14 +681,16 @@ int pciehp_sysfs_disable_slot(struct slot *p_slot)
    break;
    case POWEROFF_STATE:
    ctrl_info(ctrl, "Slot %s is already in powering off state\n",
    - p_slot->name);
    + slot_name(p_slot));
    break;
    case BLINKINGON_STATE:
    case POWERON_STATE:
    - ctrl_info(ctrl, "Already disabled on slot %s\n", p_slot->name);
    + ctrl_info(ctrl, "Already disabled on slot %s\n",
    + slot_name(p_slot));
    break;
    default:
    - ctrl_err(ctrl, "Not a valid state on slot %s\n", p_slot->name);
    + ctrl_err(ctrl, "Not a valid state on slot %s\n",
    + slot_name(p_slot));
    break;
    }
    mutex_unlock(&p_slot->lock);
    diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
    index 8e9530c..4c74d53 100644
    --- a/drivers/pci/hotplug/pciehp_hpc.c
    +++ b/drivers/pci/hotplug/pciehp_hpc.c
    @@ -1061,7 +1061,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);

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. [PATCH v5 12/16] PCI: rpaphp: kmalloc/kfree slot->name directly

    rpaphp tends to use slot->name directly everywhere, and doesn't
    ever need slot->hotplug_slot->name.

    struct hotplug_slot->name is going away, so convert rpaphp directly
    manipulate its own slot->name everywhere, and don't bother touching
    slot->hotplug_slot->name.

    Signed-off-by: Alex Chiang
    ---

    drivers/pci/hotplug/rpaphp_slot.c | 8 +++-----
    1 files changed, 3 insertions(+), 5 deletions(-)

    diff --git a/drivers/pci/hotplug/rpaphp_slot.c b/drivers/pci/hotplug/rpaphp_slot.c
    index 736d3b4..2ea9cf1 100644
    --- a/drivers/pci/hotplug/rpaphp_slot.c
    +++ b/drivers/pci/hotplug/rpaphp_slot.c
    @@ -43,7 +43,7 @@ static void rpaphp_release_slot(struct hotplug_slot *hotplug_slot)
    void dealloc_slot_struct(struct slot *slot)
    {
    kfree(slot->hotplug_slot->info);
    - kfree(slot->hotplug_slot->name);
    + kfree(slot->name);
    kfree(slot->hotplug_slot);
    kfree(slot);
    }
    @@ -63,11 +63,9 @@ struct slot *alloc_slot_struct(struct device_node *dn,
    GFP_KERNEL);
    if (!slot->hotplug_slot->info)
    goto error_hpslot;
    - slot->hotplug_slot->name = kmalloc(strlen(drc_name) + 1, GFP_KERNEL);
    - if (!slot->hotplug_slot->name)
    + slot->name = kstrdup(drc_name, GFP_KERNEL);
    + if (!slot->name)
    goto error_info;
    - slot->name = slot->hotplug_slot->name;
    - strcpy(slot->name, drc_name);
    slot->dn = dn;
    slot->index = drc_index;
    slot->power_domain = power_domain;

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. [PATCH v5 14/16] PCI: shcphp: remove 'name' parameter

    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
    ---

    drivers/pci/hotplug/shpchp.h | 9 +++++--
    drivers/pci/hotplug/shpchp_core.c | 38 ++++++++++++++---------------
    drivers/pci/hotplug/shpchp_ctrl.c | 48 +++++++++++++++++++------------------
    3 files changed, 48 insertions(+), 47 deletions(-)

    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 shpchp_queue_pushbutton_work(struct work_struct *work);
    extern int shpc_init( struct controller *ctrl, struct pci_dev *pdev);

    +static inline const char *slot_name(struct slot *slot)
    +{
    + return hotplug_slot_name(slot->hotplug_slot);
    +}
    +
    #ifdef CONFIG_ACPI
    #include
    static inline int get_hp_params_from_firmware(struct pci_dev *dev,
    diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
    index cfdd079..7af9191 100644
    --- a/drivers/pci/hotplug/shpchp_core.c
    +++ b/drivers/pci/hotplug/shpchp_core.c
    @@ -89,7 +89,7 @@ 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);
    @@ -101,6 +101,7 @@ static int init_slots(struct controller *ctrl)
    struct slot *slot;
    struct hotplug_slot *hotplug_slot;
    struct hotplug_slot_info *info;
    + char name[SLOT_NAME_SIZE];
    int retval = -ENOMEM;
    int i;

    @@ -119,8 +120,6 @@ static int init_slots(struct controller *ctrl)
    goto error_hpslot;
    hotplug_slot->info = info;

    - hotplug_slot->name = slot->name;
    -
    slot->hp_slot = i;
    slot->ctrl = ctrl;
    slot->bus = ctrl->pci_dev->subordinate->number;
    @@ -133,25 +132,24 @@ static int init_slots(struct controller *ctrl)
    /* register this slot with the hotplug pci core */
    hotplug_slot->private = slot;
    hotplug_slot->release = &release_slot;
    - snprintf(slot->name, SLOT_NAME_SIZE, "%d", slot->number);
    + snprintf(name, SLOT_NAME_SIZE, "%d", slot->number);
    hotplug_slot->ops = &shpchp_hotplug_slot_ops;

    - get_power_status(hotplug_slot, &info->power_status);
    - get_attention_status(hotplug_slot, &info->attention_status);
    - get_latch_status(hotplug_slot, &info->latch_status);
    - get_adapter_status(hotplug_slot, &info->adapter_status);
    -
    dbg("Registering bus=%x dev=%x hp_slot=%x sun=%x "
    "slot_device_offset=%x\n", slot->bus, slot->device,
    slot->hp_slot, slot->number, ctrl->slot_device_offset);
    retval = pci_hp_register(slot->hotplug_slot,
    - ctrl->pci_dev->subordinate, slot->device,
    - hotplug_slot->name);
    + ctrl->pci_dev->subordinate, slot->device, name);
    if (retval) {
    err("pci_hp_register failed with error %d\n", retval);
    goto error_info;
    }

    + get_power_status(hotplug_slot, &info->power_status);
    + get_attention_status(hotplug_slot, &info->attention_status);
    + get_latch_status(hotplug_slot, &info->latch_status);
    + get_adapter_status(hotplug_slot, &info->adapter_status);
    +
    list_add(&slot->slot_list, &ctrl->slot_list);
    }

    @@ -189,7 +187,7 @@ static int set_attention_status (struct hotplug_slot *hotplug_slot, u8 status)
    {
    struct slot *slot = get_slot(hotplug_slot);

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    hotplug_slot->info->attention_status = status;
    slot->hpc_ops->set_attention_status(slot, status);
    @@ -201,7 +199,7 @@ static int enable_slot (struct hotplug_slot *hotplug_slot)
    {
    struct slot *slot = get_slot(hotplug_slot);

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    return shpchp_sysfs_enable_slot(slot);
    }
    @@ -210,7 +208,7 @@ static int disable_slot (struct hotplug_slot *hotplug_slot)
    {
    struct slot *slot = get_slot(hotplug_slot);

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    return shpchp_sysfs_disable_slot(slot);
    }
    @@ -220,7 +218,7 @@ static int get_power_status (struct hotplug_slot *hotplug_slot, u8 *value)
    struct slot *slot = get_slot(hotplug_slot);
    int retval;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    retval = slot->hpc_ops->get_power_status(slot, value);
    if (retval < 0)
    @@ -234,7 +232,7 @@ static int get_attention_status (struct hotplug_slot *hotplug_slot, u8 *value)
    struct slot *slot = get_slot(hotplug_slot);
    int retval;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    retval = slot->hpc_ops->get_attention_status(slot, value);
    if (retval < 0)
    @@ -248,7 +246,7 @@ static int get_latch_status (struct hotplug_slot *hotplug_slot, u8 *value)
    struct slot *slot = get_slot(hotplug_slot);
    int retval;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    retval = slot->hpc_ops->get_latch_status(slot, value);
    if (retval < 0)
    @@ -262,7 +260,7 @@ static int get_adapter_status (struct hotplug_slot *hotplug_slot, u8 *value)
    struct slot *slot = get_slot(hotplug_slot);
    int retval;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    retval = slot->hpc_ops->get_adapter_status(slot, value);
    if (retval < 0)
    @@ -277,7 +275,7 @@ static int get_max_bus_speed(struct hotplug_slot *hotplug_slot,
    struct slot *slot = get_slot(hotplug_slot);
    int retval;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    retval = slot->hpc_ops->get_max_bus_speed(slot, value);
    if (retval < 0)
    @@ -291,7 +289,7 @@ static int get_cur_bus_speed (struct hotplug_slot *hotplug_slot, enum pci_bus_sp
    struct slot *slot = get_slot(hotplug_slot);
    int retval;

    - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));

    retval = slot->hpc_ops->get_cur_bus_speed(slot, value);
    if (retval < 0)
    diff --git a/drivers/pci/hotplug/shpchp_ctrl.c b/drivers/pci/hotplug/shpchp_ctrl.c
    index dfb5393..919b1ee 100644
    --- a/drivers/pci/hotplug/shpchp_ctrl.c
    +++ b/drivers/pci/hotplug/shpchp_ctrl.c
    @@ -70,7 +70,7 @@ u8 shpchp_handle_attention_button(u8 hp_slot, struct controller *ctrl)
    /*
    * Button pressed - See if need to TAKE ACTION!!!
    */
    - info("Button pressed on Slot(%s)\n", p_slot->name);
    + info("Button pressed on Slot(%s)\n", slot_name(p_slot));
    event_type = INT_BUTTON_PRESS;

    queue_interrupt_event(p_slot, event_type);
    @@ -98,7 +98,7 @@ u8 shpchp_handle_switch_change(u8 hp_slot, struct controller *ctrl)
    /*
    * Switch opened
    */
    - info("Latch open on Slot(%s)\n", p_slot->name);
    + info("Latch open on Slot(%s)\n", slot_name(p_slot));
    event_type = INT_SWITCH_OPEN;
    if (p_slot->pwr_save && p_slot->presence_save) {
    event_type = INT_POWER_FAULT;
    @@ -108,7 +108,7 @@ u8 shpchp_handle_switch_change(u8 hp_slot, struct controller *ctrl)
    /*
    * Switch closed
    */
    - info("Latch close on Slot(%s)\n", p_slot->name);
    + info("Latch close on Slot(%s)\n", slot_name(p_slot));
    event_type = INT_SWITCH_CLOSE;
    }

    @@ -135,13 +135,13 @@ u8 shpchp_handle_presence_change(u8 hp_slot, struct controller *ctrl)
    /*
    * Card Present
    */
    - info("Card present on Slot(%s)\n", p_slot->name);
    + info("Card present on Slot(%s)\n", slot_name(p_slot));
    event_type = INT_PRESENCE_ON;
    } else {
    /*
    * Not Present
    */
    - info("Card not present on Slot(%s)\n", p_slot->name);
    + info("Card not present on Slot(%s)\n", slot_name(p_slot));
    event_type = INT_PRESENCE_OFF;
    }

    @@ -164,14 +164,14 @@ u8 shpchp_handle_power_fault(u8 hp_slot, struct controller *ctrl)
    /*
    * Power fault Cleared
    */
    - info("Power fault cleared on Slot(%s)\n", p_slot->name);
    + info("Power fault cleared on Slot(%s)\n", slot_name(p_slot));
    p_slot->status = 0x00;
    event_type = INT_POWER_FAULT_CLEAR;
    } else {
    /*
    * Power fault
    */
    - info("Power fault on Slot(%s)\n", p_slot->name);
    + info("Power fault on Slot(%s)\n", slot_name(p_slot));
    event_type = INT_POWER_FAULT;
    /* set power fault status for this board */
    p_slot->status = 0xFF;
    @@ -493,11 +493,11 @@ static void handle_button_press_event(struct slot *p_slot)
    if (getstatus) {
    p_slot->state = BLINKINGOFF_STATE;
    info("PCI slot #%s - powering off due to button "
    - "press.\n", p_slot->name);
    + "press.\n", slot_name(p_slot));
    } else {
    p_slot->state = BLINKINGON_STATE;
    info("PCI slot #%s - powering on due to button "
    - "press.\n", p_slot->name);
    + "press.\n", slot_name(p_slot));
    }
    /* blink green LED and turn off amber */
    p_slot->hpc_ops->green_led_blink(p_slot);
    @@ -512,7 +512,7 @@ static void handle_button_press_event(struct slot *p_slot)
    * press the attention again before the 5 sec. limit
    * expires to cancel hot-add or hot-remove
    */
    - info("Button cancel on Slot(%s)\n", p_slot->name);
    + info("Button cancel on Slot(%s)\n", slot_name(p_slot));
    dbg("%s: button cancel\n", __func__);
    cancel_delayed_work(&p_slot->work);
    if (p_slot->state == BLINKINGOFF_STATE)
    @@ -521,7 +521,7 @@ static void handle_button_press_event(struct slot *p_slot)
    p_slot->hpc_ops->green_led_off(p_slot);
    p_slot->hpc_ops->set_attention_status(p_slot, 0);
    info("PCI slot #%s - action canceled due to button press\n",
    - p_slot->name);
    + slot_name(p_slot));
    p_slot->state = STATIC_STATE;
    break;
    case POWEROFF_STATE:
    @@ -531,7 +531,7 @@ static void handle_button_press_event(struct slot *p_slot)
    * this means that the previous attention button action
    * to hot-add or hot-remove is undergoing
    */
    - info("Button ignore on Slot(%s)\n", p_slot->name);
    + info("Button ignore on Slot(%s)\n", slot_name(p_slot));
    update_slot_info(p_slot);
    break;
    default:
    @@ -574,17 +574,17 @@ static int shpchp_enable_slot (struct slot *p_slot)
    mutex_lock(&p_slot->ctrl->crit_sect);
    rc = p_slot->hpc_ops->get_adapter_status(p_slot, &getstatus);
    if (rc || !getstatus) {
    - info("No adapter on slot(%s)\n", p_slot->name);
    + info("No adapter on slot(%s)\n", slot_name(p_slot));
    goto out;
    }
    rc = p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
    if (rc || getstatus) {
    - info("Latch open on slot(%s)\n", p_slot->name);
    + info("Latch open on slot(%s)\n", slot_name(p_slot));
    goto out;
    }
    rc = p_slot->hpc_ops->get_power_status(p_slot, &getstatus);
    if (rc || getstatus) {
    - info("Already enabled on slot(%s)\n", p_slot->name);
    + info("Already enabled on slot(%s)\n", slot_name(p_slot));
    goto out;
    }

    @@ -633,17 +633,17 @@ static int shpchp_disable_slot (struct slot *p_slot)

    rc = p_slot->hpc_ops->get_adapter_status(p_slot, &getstatus);
    if (rc || !getstatus) {
    - info("No adapter on slot(%s)\n", p_slot->name);
    + info("No adapter on slot(%s)\n", slot_name(p_slot));
    goto out;
    }
    rc = p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
    if (rc || getstatus) {
    - info("Latch open on slot(%s)\n", p_slot->name);
    + info("Latch open on slot(%s)\n", slot_name(p_slot));
    goto out;
    }
    rc = p_slot->hpc_ops->get_power_status(p_slot, &getstatus);
    if (rc || !getstatus) {
    - info("Already disabled slot(%s)\n", p_slot->name);
    + info("Already disabled slot(%s)\n", slot_name(p_slot));
    goto out;
    }

    @@ -671,14 +671,14 @@ int shpchp_sysfs_enable_slot(struct slot *p_slot)
    break;
    case POWERON_STATE:
    info("Slot %s is already in powering on state\n",
    - p_slot->name);
    + slot_name(p_slot));
    break;
    case BLINKINGOFF_STATE:
    case POWEROFF_STATE:
    - info("Already enabled on slot %s\n", p_slot->name);
    + info("Already enabled on slot %s\n", slot_name(p_slot));
    break;
    default:
    - err("Not a valid state on slot %s\n", p_slot->name);
    + err("Not a valid state on slot %s\n", slot_name(p_slot));
    break;
    }
    mutex_unlock(&p_slot->lock);
    @@ -703,14 +703,14 @@ int shpchp_sysfs_disable_slot(struct slot *p_slot)
    break;
    case POWEROFF_STATE:
    info("Slot %s is already in powering off state\n",
    - p_slot->name);
    + slot_name(p_slot));
    break;
    case BLINKINGON_STATE:
    case POWERON_STATE:
    - info("Already disabled on slot %s\n", p_slot->name);
    + info("Already disabled on slot %s\n", slot_name(p_slot));
    break;
    default:
    - err("Not a valid state on slot %s\n", p_slot->name);
    + err("Not a valid state on slot %s\n", slot_name(p_slot));
    break;
    }
    mutex_unlock(&p_slot->lock);

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. [PATCH v5 15/16] PCI: Hotplug core: remove 'name'

    Now that the PCI core manages the 'name' for each individual
    hotplug driver, and all drivers (except rpaphp) 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
    ---

    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 ed87585..4d8a8cd 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;
    @@ -616,7 +616,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;

    @@ -626,7 +626,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 a3a3245..a00bd1a 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 core
    - * @name: the name of the slot being registered. This string must
    - * be unique amoung slots registered on this system.
    * @ops: pointer to the &struct hotplug_slot_ops to be used for this slot
    * @info: pointer to the &struct hotplug_slot_info for the initial values for
    * this slot.
    @@ -153,7 +151,6 @@ struct hotplug_slot_info {
    * needs.
    */
    struct hotplug_slot {
    - char *name;
    struct hotplug_slot_ops *ops;
    struct hotplug_slot_info *info;
    void (*release) (struct hotplug_slot *slot);

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. [PATCH v5 10/16] PCI: ibmphp: stop managing hotplug_slot->name

    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
    ---

    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 release_slot(struct hotplug_slot *hotplug_slot)

    slot = hotplug_slot->private;
    kfree(slot->hotplug_slot->info);
    - kfree(slot->hotplug_slot->name);
    kfree(slot->hotplug_slot);
    slot->ctrl = NULL;
    slot->bus_on = NULL;
    @@ -734,6 +736,7 @@ static int __init ebda_rsrc_controller (void)
    struct bus_info *bus_info_ptr1, *bus_info_ptr2;
    int rc;
    struct slot *tmp_slot;
    + char name[SLOT_NAME_SIZE];

    addr = hpc_list_ptr->phys_addr;
    for (ctlr = 0; ctlr < hpc_list_ptr->num_ctlrs; ctlr++) {
    @@ -897,12 +900,6 @@ static int __init ebda_rsrc_controller (void)
    goto error_no_hp_info;
    }

    - hp_slot_ptr->name = kmalloc(30, GFP_KERNEL);
    - if (!hp_slot_ptr->name) {
    - rc = -ENOMEM;
    - goto error_no_hp_name;
    - }
    -
    tmp_slot = kzalloc(sizeof(*tmp_slot), GFP_KERNEL);
    if (!tmp_slot) {
    rc = -ENOMEM;
    @@ -964,10 +961,9 @@ static int __init ebda_rsrc_controller (void)
    } /* each hpc */

    list_for_each_entry(tmp_slot, &ibmphp_slot_head, ibm_slot_list) {
    - snprintf (tmp_slot->hotplug_slot->name, 30, "%s", create_file_name (tmp_slot));
    + snprintf(name, SLOT_NAME_SIZE, "%s", create_file_name(tmp_slot));
    pci_hp_register(tmp_slot->hotplug_slot,
    - pci_find_bus(0, tmp_slot->bus), tmp_slot->device,
    - tmp_slot->hotplug_slot->name);
    + pci_find_bus(0, tmp_slot->bus), tmp_slot->device, name);
    }

    print_ebda_hpc ();
    @@ -977,8 +973,6 @@ static int __init ebda_rsrc_controller (void)
    error:
    kfree (hp_slot_ptr->private);
    error_no_slot:
    - kfree (hp_slot_ptr->name);
    -error_no_hp_name:
    kfree (hp_slot_ptr->info);
    error_no_hp_info:
    kfree (hp_slot_ptr);

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. [PATCH v5 09/16] PCI: fakephp: remove 'name' parameter

    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
    ---

    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 3069f21..24dcbf1 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,20 +121,18 @@ 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);
    + snprintf(name, SLOT_NAME_SIZE, "fake%d", count++);
    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;
    }

    + dbg("slot->name = %s\n", hotplug_slot_name(slot));
    dslot->slot = slot;
    dslot->dev = pci_dev_get(dev);
    list_add (&dslot->node, &slot_list);
    @@ -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 unregistering a slot %s\n",
    + hotplug_slot_name(dslot->slot));
    }

    /* called from the single-threaded workqueue handler to remove a slot */
    @@ -309,7 +310,7 @@ static int disable_slot(struct hotplug_slot *slot)
    return -ENODEV;
    dslot = slot->private;

    - dbg("%s - physical_slot = %s\n", __func__, slot->name);
    + dbg("%s - physical_slot = %s\n", __func__, hotplug_slot_name(slot));

    for (func = 7; func >= 0; func--) {
    dev = pci_get_slot(dslot->dev->bus, dslot->dev->devfn + func);

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. [PATCH v5 13/16] PCI: SGI Hotplug: stop managing bss_hotplug_slot->name

    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
    ---

    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 device;
    struct pci_slot *pci_slot;
    struct hotplug_slot *bss_hotplug_slot;
    + char name[SN_SLOT_NAME_SIZE];
    int rc = 0;

    /*
    @@ -645,16 +640,14 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus)
    }

    if (sn_hp_slot_private_alloc(bss_hotplug_slot,
    - pci_bus, device)) {
    + pci_bus, device, name)) {
    rc = -ENOMEM;
    goto alloc_err;
    }
    -
    bss_hotplug_slot->ops = &sn_hotplug_slot_ops;
    bss_hotplug_slot->release = &sn_release_slot;

    - rc = pci_hp_register(bss_hotplug_slot, pci_bus, device,
    - bss_hotplug_slot->name);
    + rc = pci_hp_register(bss_hotplug_slot, pci_bus, device, name);
    if (rc)
    goto register_err;


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  11. [PATCH v5 16/16] PCI Hotplug: fakephp: add duplicate slot name debugging

    The PCI core now manages slot names on behalf of slot detection
    and slot hotplug drivers, including the handling of duplicate
    slot names.

    We can use the fakephp driver to help test the new functionality.
    Add a 'dup_slots' module param to force fakephp to create multiple
    slots with the same name. We can then verify that the PCI core
    correctly renamed the slots.

    sapphire:/sys/bus/pci/slots # modprobe fakephp dup_slots
    sapphire:/sys/bus/pci/slots # ls
    fake fake-10 fake-3 fake-5 fake-7 fake-9
    fake-1 fake-2 fake-4 fake-6 fake-8

    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
    ---

    drivers/pci/hotplug/fakephp.c | 10 ++++++++--
    1 files changed, 8 insertions(+), 2 deletions(-)

    diff --git a/drivers/pci/hotplug/fakephp.c b/drivers/pci/hotplug/fakephp.c
    index 24dcbf1..3a2637a 100644
    --- a/drivers/pci/hotplug/fakephp.c
    +++ b/drivers/pci/hotplug/fakephp.c
    @@ -69,6 +69,7 @@ struct dummy_slot {
    };

    static int debug;
    +static int dup_slots;
    static LIST_HEAD(slot_list);
    static struct workqueue_struct *dummyphp_wq;

    @@ -121,7 +122,11 @@ static int add_slot(struct pci_dev *dev)
    if (!dslot)
    goto error_info;

    - snprintf(name, SLOT_NAME_SIZE, "fake%d", count++);
    + if (dup_slots)
    + snprintf(name, SLOT_NAME_SIZE, "fake");
    + else
    + snprintf(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;
    @@ -375,4 +380,5 @@ MODULE_DESCRIPTION(DRIVER_DESC);
    MODULE_LICENSE("GPL");
    module_param(debug, bool, S_IRUGO | S_IWUSR);
    MODULE_PARM_DESC(debug, "Debugging mode enabled or not");
    -
    +module_param(dup_slots, bool, S_IRUGO | S_IWUSR);
    +MODULE_PARM_DESC(dup_slots, "Force duplicate slot names for debugging");

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  12. Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

    Alex Chiang wrote:
    > 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
    > etc.
    >
    > 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. The -EBUSY was getting masked previously by -EEXIST in
    > the event of duplicate slot names; this is no longer so.
    >
    > This is the permanent fix mentioned in earlier commits d6a9e9b4 and
    > 167e782e (shpchp/pciehp: Rename duplicate slot name...).
    >
    > Finally, we take advantage of the new 'rename' parameter in
    > pci_create_slot() to prevent a slot create/rename race between hotplug
    > drivers and detection drivers.
    >
    > Scenario A:
    > hotplug driver detection_driver
    > -------------- ----------------
    > pci_create_slot(rename=1)
    > pci_create_slot(rename=0)
    >
    > The hotplug driver creates the slot with its desired name, and then
    > releases the semaphore. Now, the detection driver tries to create
    > the same slot, but it already exists. We don't care about renaming,
    > so return the existing slot.
    >
    > Scenario B:
    > hotplug driver detection_driver
    > -------------- ----------------
    > pci_create_slot(rename=0)
    > pci_create_slot(rename=1)
    >
    > The detection driver creates the slot with name "X". Then the hotplug
    > driver tries to create the same slot, but wants the name "Y" instead.
    > We detect that we're trying to create the same slot and that we also
    > want a rename, so rename the slot to "Y" and return.
    >


    Thank your new patches. Very quick!!!

    Though I have not reviewed/tested your patches yet (of course), I have
    one concern as I said in the e-mail soon before. Does the new one
    consider the following senario?

    Scenario C:
    hotplug driver(A) hotplug_driver(B)
    -------------- ----------------
    pci_create_slot(name=A, rename=1)
    pci_create_slot(name=B, rename=1)

    The hotplug driver (A) creates the slot with name "A". The the hotplug
    driver (B) tries to create the same slot, but wants the name "B" instead.
    In this case, hotplug driver fails to create the slot and the slot name
    should not be changed to "B" from "A".

    Thanks,
    Kenji Kaneshige

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  13. Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

    * Kenji Kaneshige :
    >
    > Thank your new patches. Very quick!!!


    I'm trying to get into 2.6.28.

    > Though I have not reviewed/tested your patches yet (of course), I have
    > one concern as I said in the e-mail soon before. Does the new one
    > consider the following senario?
    >
    > Scenario C:
    > hotplug driver(A) hotplug_driver(B)
    > -------------- ----------------
    > pci_create_slot(name=A, rename=1)
    > pci_create_slot(name=B, rename=1)
    >
    > The hotplug driver (A) creates the slot with name "A". The the hotplug
    > driver (B) tries to create the same slot, but wants the name "B" instead.
    > In this case, hotplug driver fails to create the slot and the slot name
    > should not be changed to "B" from "A".


    Hm... I don't think this is a common scenario but...

    int pci_hp_register(...)
    {
    ...

    pci_slot = pci_create_slot(bus, slot_nr, name, 1);
    if (IS_ERR(pci_slot))
    return PTR_ERR(pci_slot);

    if (pci_slot->hotplug) {
    dbg("%s: already claimed\n", __func__);
    pci_destroy_slot(pci_slot);
    return -EBUSY;
    }
    ...
    }

    I could maybe move that check into pci_create_slot() instead.

    struct pci_slot *pci_create_slot(...)
    {
    ...

    /*
    * Get existing slot and rename if desired
    */
    slot = get_slot(parent, slot_nr);
    if (slot && rename) {
    if ((err = slot->hotplug ? -EBUSY : 0)
    || (err = rename_slot(slot, name))) {
    kobject_put(&slot->kobj);
    slot = NULL;
    goto err;
    } else
    goto out;
    } else if (slot)
    goto out;
    ...
    }

    Seems a little ugly to me, but maybe it's necessary?

    /ac

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  14. Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

    Alex Chiang wrote:
    > * Kenji Kaneshige :
    >> Thank your new patches. Very quick!!!

    >
    > I'm trying to get into 2.6.28.
    >
    >> Though I have not reviewed/tested your patches yet (of course), I have
    >> one concern as I said in the e-mail soon before. Does the new one
    >> consider the following senario?
    >>
    >> Scenario C:
    >> hotplug driver(A) hotplug_driver(B)
    >> -------------- ----------------
    >> pci_create_slot(name=A, rename=1)
    >> pci_create_slot(name=B, rename=1)
    >>
    >> The hotplug driver (A) creates the slot with name "A". The the hotplug
    >> driver (B) tries to create the same slot, but wants the name "B" instead.
    >> In this case, hotplug driver fails to create the slot and the slot name
    >> should not be changed to "B" from "A".

    >
    > Hm... I don't think this is a common scenario but...
    >


    It was a common scenario until recently because acpiphp and
    native hotplug drivers(pciehp, shpchp) had different naming
    rules. That is, acpiphp used _SUN number, while pciehp/shpchp
    used bus number and physical slot number pair. Although the
    pciehp/shpchp driver has been changed not to use bus_number
    for slot names and acpiphp and pciehp/shpchp has the same
    names on my system now, but I think the scenario is still
    possible because naming rule of each hotplug driver could be
    changed in the future again.

    By the way, acpiphp was changed to handle 64bit _SUN number
    recently for new ia64 HP servers, IIRC. Are hotplug slots
    on that server can also be handled through PCIe controller?
    If it is yes, I think _SUN doesn't match physical slot number
    because physical slot number (in Slot Capabilities Register)
    has only 13bit. In this case, the above scenario will happen.

    > int pci_hp_register(...)
    > {
    > ...
    >
    > pci_slot = pci_create_slot(bus, slot_nr, name, 1);
    > if (IS_ERR(pci_slot))
    > return PTR_ERR(pci_slot);
    >
    > if (pci_slot->hotplug) {
    > dbg("%s: already claimed\n", __func__);
    > pci_destroy_slot(pci_slot);
    > return -EBUSY;
    > }
    > ...
    > }
    >
    > I could maybe move that check into pci_create_slot() instead.
    >
    > struct pci_slot *pci_create_slot(...)
    > {
    > ...
    >
    > /*
    > * Get existing slot and rename if desired
    > */
    > slot = get_slot(parent, slot_nr);
    > if (slot && rename) {
    > if ((err = slot->hotplug ? -EBUSY : 0)
    > || (err = rename_slot(slot, name))) {
    > kobject_put(&slot->kobj);
    > slot = NULL;
    > goto err;
    > } else
    > goto out;
    > } else if (slot)
    > goto out;
    > ...
    > }
    >
    > Seems a little ugly to me, but maybe it's necessary?
    >


    I don't like this, and I think it's wrong because callers
    might get -EBUSY even though they are not related to hotplug.

    I thought of the following alternative ideas, when I was making
    sample patches. What do you think about those? My was concerned
    that both need to add hotplug related code into generic pci slot
    management code/API.

    - Add 'hotplug' arg to pci_create_slot(), instead of 'rename'
    flag. The pci_create_slot() would be as follows:

    struct pci_slot *pci_create_slot(..., struct hotplug_slot *hotplug)
    {
    ...
    /*
    * Get existing slot and rename if desired
    */
    slot = get_slot(parent, slot_nr);
    if (slot) {
    if (hotplug) {
    if ((err = slot->hotplug ? -EBUSY : 0)
    || err = rename_slot(slot, name))) {
    Some cleanups;
    return err;
    }
    }
    goto out;
    }
    ...
    out:
    if (hotplug)
    slot->hotplug = hotplug;
    ...
    }

    - Introduce new API to setup pci_slot->hotplug and rename. This would be
    as follows:

    int pci_slot_hp_register(struct pci_slot *pci_slot,
    struct hotplug_slot *hotplug_slot, const char *name)
    {
    ...

    if (pci_slot->hotplug) {
    Some cleanups;
    return -EBUSY;
    }
    if ((err = rename_slot(slot, name))
    Some cleanups;
    return err;
    }
    pci_slot->hotplug = hotplug;
    ...
    }

    It is intended to be used by pci_hp_register() as follows:

    int pci_hp_register(...)
    {
    ...
    pci_slot = pci_create_slot(bus, slot_nr, name);
    if ((result = IS_ERR(pci_slot)))
    goto out;
    if ((err = pci_slot_hp_register(pci_slot, hotplug, name)))
    goto out;
    ...
    }

    Thanks,
    Kenji Kaneshige


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  15. Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

    commit 2338f56b789bb030434be828c4788679d72901b5
    Author: Alex Chiang
    Date: Thu Oct 9 19:56:58 2008 -0600

    PCI: update pci_create_slot() to take a 'hotplug' param

    Slot detection drivers can co-exist with hotplug drivers. The names
    of the detected/claimed slots may be different depending on module
    load order.

    For legacy reasons, we need to allow hotplug drivers to override
    the slot name if a detection driver is loaded first (and they find
    the same slots).

    Creating and overriding slot names should be an atomic operation,
    otherwise you get a locking nightmare as various drivers race to
    call pci_create_slot().

    pci_create_slot() is already serialized by grabbing the pci_bus_sem.

    We update the API and add a 'hotplug' param, which is:

    set if the caller is a hotplug driver
    unset if the caller is a detection driver

    pci_create_slot() does not actually use the 'hotplug' parameter in this
    patch. A later patch will add the logic that uses it.

    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

    diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
    index d5b4ef8..8d4a568 100644
    --- a/drivers/acpi/pci_slot.c
    +++ b/drivers/acpi/pci_slot.c
    @@ -150,7 +150,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
    }

    snprintf(name, sizeof(name), "%u", (u32)sun);
    - pci_slot = pci_create_slot(pci_bus, device, name);
    + pci_slot = pci_create_slot(pci_bus, device, name, NULL);
    if (IS_ERR(pci_slot)) {
    err("pci_create_slot returned %ld\n", PTR_ERR(pci_slot));
    kfree(slot);
    diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
    index 0e7a511..191b58e 100644
    --- a/drivers/pci/hotplug/pci_hotplug_core.c
    +++ b/drivers/pci/hotplug/pci_hotplug_core.c
    @@ -579,7 +579,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
    * driver and call it here again. If we've already created the
    * pci_slot, the interface will simply bump the refcount.
    */
    - pci_slot = pci_create_slot(bus, slot_nr, name);
    + pci_slot = pci_create_slot(bus, slot_nr, name, slot);
    if (IS_ERR(pci_slot))
    return PTR_ERR(pci_slot);

    diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
    index b9b90ab..0e009c3 100644
    --- a/drivers/pci/slot.c
    +++ b/drivers/pci/slot.c
    @@ -83,6 +83,7 @@ static struct kobj_type pci_slot_ktype = {
    * @parent: struct pci_bus of parent bridge
    * @slot_nr: PCI_SLOT(pci_dev->devfn) or -1 for placeholder
    * @name: user visible string presented in /sys/bus/pci/slots/
    + * @hotplug: set if caller is hotplug driver, NULL otherwise
    *
    * PCI slots have first class attributes such as address, speed, width,
    * and a &struct pci_slot is used to manage them. This interface will
    @@ -111,7 +112,8 @@ static struct kobj_type pci_slot_ktype = {
    */

    struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
    - const char *name)
    + const char *name,
    + struct hotplug_slot *hotplug)
    {
    struct pci_dev *dev;
    struct pci_slot *slot;
    diff --git a/include/linux/pci.h b/include/linux/pci.h
    index 5d96b7a..8bbbbbc 100644
    --- a/include/linux/pci.h
    +++ b/include/linux/pci.h
    @@ -508,7 +508,8 @@ struct pci_bus *pci_create_bus(struct device *parent, int bus,
    struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev,
    int busnr);
    struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
    - const char *name);
    + const char *name,
    + struct hotplug_slot *hotplug);
    void pci_destroy_slot(struct pci_slot *slot);
    void pci_renumber_slot(struct pci_slot *slot, int slot_nr);
    int pci_scan_slot(struct pci_bus *bus, int devfn);
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  16. Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

    Note, the comments might be a bit stale...

    Also note the change in rename_slot() so we don't try and rename
    something with the same name.

    /ac

    commit eb00679613b5e5b7d7b4ebd82d4a229fff1ecefa
    Author: Alex Chiang
    Date: Thu Oct 9 20:06:49 2008 -0600

    PCI: prevent duplicate slot names

    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
    etc.

    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. The -EBUSY was getting masked previously by -EEXIST in
    the event of duplicate slot names; this is no longer so.

    This is the permanent fix mentioned in earlier commits d6a9e9b4 and
    167e782e (shpchp/pciehp: Rename duplicate slot name...).

    Finally, we take advantage of the new 'rename' parameter in
    pci_create_slot() to prevent a slot create/rename race between hotplug
    drivers and detection drivers.

    Scenario A:
    hotplug driver detection_driver
    -------------- ----------------
    pci_create_slot(rename=1)
    pci_create_slot(rename=0)

    The hotplug driver creates the slot with its desired name, and then
    releases the semaphore. Now, the detection driver tries to create
    the same slot, but it already exists. We don't care about renaming,
    so return the existing slot.

    Scenario B:
    hotplug driver detection_driver
    -------------- ----------------
    pci_create_slot(rename=0)
    pci_create_slot(rename=1)

    The detection driver creates the slot with name "X". Then the hotplug
    driver tries to create the same slot, but wants the name "Y" instead.
    We detect that we're trying to create the same slot and that we also
    want a rename, so rename the slot to "Y" and return.

    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

    diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
    index 191b58e..b4e3a43 100644
    --- a/drivers/pci/hotplug/pci_hotplug_core.c
    +++ b/drivers/pci/hotplug/pci_hotplug_core.c
    @@ -570,10 +570,6 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
    return -EINVAL;
    }

    - /* Check if we have already registered a slot with the same name. */
    - if (get_slot_from_name(name))
    - return -EEXIST;
    -
    /*
    * No problems if we call this interface from both ACPI_PCI_SLOT
    * driver and call it here again. If we've already created the
    @@ -583,25 +579,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
    if (IS_ERR(pci_slot))
    return PTR_ERR(pci_slot);

    - if (pci_slot->hotplug) {
    - dbg("%s: already claimed\n", __func__);
    - pci_destroy_slot(pci_slot);
    - return -EBUSY;
    - }
    -
    slot->pci_slot = pci_slot;
    - pci_slot->hotplug = slot;
    -
    - /*
    - * Allow pcihp drivers to override the ACPI_PCI_SLOT name.
    - */
    - if (strcmp(kobject_name(&pci_slot->kobj), name)) {
    - result = kobject_rename(&pci_slot->kobj, name);
    - if (result) {
    - pci_destroy_slot(pci_slot);
    - return result;
    - }
    - }

    spin_lock(&pci_hotplug_slot_list_lock);
    list_add(&slot->slot_list, &pci_hotplug_slot_list);
    diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
    index 3ace5e0..af89d7b 100644
    --- a/drivers/pci/hotplug/pciehp_core.c
    +++ b/drivers/pci/hotplug/pciehp_core.c
    @@ -196,7 +196,6 @@ static int init_slots(struct controller *ctrl)
    struct slot *slot;
    struct hotplug_slot *hotplug_slot;
    struct hotplug_slot_info *info;
    - int len, dup = 1;
    int retval = -ENOMEM;

    list_for_each_entry(slot, &ctrl->slot_list, slot_list) {
    @@ -223,25 +222,11 @@ static int init_slots(struct controller *ctrl)
    ctrl_dbg(ctrl, "Registering bus=%x dev=%x hp_slot=%x sun=%x "
    "slot_device_offset=%x\n", slot->bus, slot->device,
    slot->hp_slot, slot->number, ctrl->slot_device_offset);
    -duplicate_name:
    retval = pci_hp_register(hotplug_slot,
    ctrl->pci_dev->subordinate,
    slot->device,
    slot->name);
    if (retval) {
    - /*
    - * If slot N already exists, we'll try to create
    - * slot N-1, N-2 ... N-M, until we overflow.
    - */
    - if (retval == -EEXIST) {
    - len = snprintf(slot->name, SLOT_NAME_SIZE,
    - "%d-%d", slot->number, dup++);
    - if (len < SLOT_NAME_SIZE)
    - goto duplicate_name;
    - else
    - ctrl_err(ctrl, "duplicate slot name "
    - "overflow\n");
    - }
    ctrl_err(ctrl, "pci_hp_register failed with error %d\n",
    retval);
    goto error_info;
    diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
    index bf50966..cfdd079 100644
    --- a/drivers/pci/hotplug/shpchp_core.c
    +++ b/drivers/pci/hotplug/shpchp_core.c
    @@ -102,7 +102,7 @@ static int init_slots(struct controller *ctrl)
    struct hotplug_slot *hotplug_slot;
    struct hotplug_slot_info *info;
    int retval = -ENOMEM;
    - int i, len, dup = 1;
    + int i;

    for (i = 0; i < ctrl->num_slots; i++) {
    slot = kzalloc(sizeof(*slot), GFP_KERNEL);
    @@ -144,23 +144,10 @@ static int init_slots(struct controller *ctrl)
    dbg("Registering bus=%x dev=%x hp_slot=%x sun=%x "
    "slot_device_offset=%x\n", slot->bus, slot->device,
    slot->hp_slot, slot->number, ctrl->slot_device_offset);
    -duplicate_name:
    retval = pci_hp_register(slot->hotplug_slot,
    ctrl->pci_dev->subordinate, slot->device,
    hotplug_slot->name);
    if (retval) {
    - /*
    - * If slot N already exists, we'll try to create
    - * slot N-1, N-2 ... N-M, until we overflow.
    - */
    - if (retval == -EEXIST) {
    - len = snprintf(slot->name, SLOT_NAME_SIZE,
    - "%d-%d", slot->number, dup++);
    - if (len < SLOT_NAME_SIZE)
    - goto duplicate_name;
    - else
    - err("duplicate slot name overflow\n");
    - }
    err("pci_hp_register failed with error %d\n", retval);
    goto error_info;
    }
    diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
    index 0e009c3..0b0a79f 100644
    --- a/drivers/pci/slot.c
    +++ b/drivers/pci/slot.c
    @@ -78,6 +78,77 @@ static struct kobj_type pci_slot_ktype = {
    .default_attrs = pci_slot_default_attrs,
    };

    +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;
    + kfree(new_name);
    + new_name = kmalloc(len, GFP_KERNEL);
    + if (!new_name)
    + break;
    + }
    + sprintf(new_name, "%s-%d", name, dup++);
    + }
    +
    + return new_name;
    +}
    +
    +static int rename_slot(struct pci_slot *slot, const char *name)
    +{
    + int result;
    + char *slot_name;
    +
    + if (strcmp(kobject_name(&slot->kobj), name) == 0)
    + return;
    +
    + slot_name = make_slot_name(name);
    + if (!slot_name)
    + return -ENOMEM;
    +
    + result = kobject_rename(&slot->kobj, slot_name);
    + kfree(slot_name);
    +
    + return result;
    +}
    +
    +static struct pci_slot *get_slot(struct pci_bus *parent, int slot_nr)
    +{
    + struct pci_slot *slot;
    + /*
    + * We already hold pci_bus_sem so don't worry
    + */
    + list_for_each_entry(slot, &parent->slots, list)
    + if (slot->number == slot_nr) {
    + kobject_get(&slot->kobj);
    + return slot;
    + }
    +
    + return NULL;
    +}
    +
    /**
    * pci_create_slot - create or increment refcount for physical PCI slot
    * @parent: struct pci_bus of parent bridge
    @@ -90,7 +161,17 @@ 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.
    + *
    + * There are known platforms with broken firmware that assign the same
    + * name to multiple slots. 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
    + * etc.
    *
    * Placeholder slots:
    * In most cases, @pci_bus, @slot_nr will be sufficient to uniquely identify
    @@ -99,43 +180,43 @@ static struct kobj_type pci_slot_ktype = {
    * the slot. In this scenario, the caller may pass -1 for @slot_nr.
    *
    * The following semantics are imposed when the caller passes @slot_nr ==
    - * -1. First, the check for existing %struct pci_slot is skipped, as the
    - * caller may know about several unpopulated slots on a given %struct
    - * pci_bus, and each slot would have a @slot_nr of -1. Uniqueness for
    - * these slots is then determined by the @name parameter. We expect
    - * kobject_init_and_add() to warn us if the caller attempts to create
    - * multiple slots with the same name. The other change in semantics is
    + * -1. First, we no longer check for an existing %struct pci_slot, as there
    + * may be many slots with @slot_nr of -1. The other change in semantics is
    * user-visible, which is the 'address' parameter presented in sysfs will
    * consist solely of a dddd:bb tuple, where dddd is the PCI domain of the
    * %struct pci_bus and bb is the bus number. In other words, the devfn of
    * the 'placeholder' slot will not be displayed.
    */
    -
    struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
    const char *name,
    struct hotplug_slot *hotplug)
    {
    struct pci_dev *dev;
    struct pci_slot *slot;
    - int err;
    + int err = 0;
    + char *slot_name = NULL;

    down_write(&pci_bus_sem);

    if (slot_nr == -1)
    goto placeholder;

    - /* If we've already created this slot, bump refcount and return. */
    - list_for_each_entry(slot, &parent->slots, list) {
    - if (slot->number == slot_nr) {
    - kobject_get(&slot->kobj);
    - pr_debug("%s: inc refcount to %d on %04x:%02x:%02x\n",
    - __func__,
    - atomic_read(&slot->kobj.kref.refcount),
    - pci_domain_nr(parent), parent->number,
    - slot_nr);
    + /*
    + * Get existing slot and add hotplug callback if necessary.
    + */
    + slot = get_slot(parent, slot_nr);
    + if (slot && hotplug) {
    + if ((err = slot->hotplug ? -EBUSY : 0)
    + || (err = rename_slot(slot,name))) {
    + kobject_put(&slot->kobj);
    + slot = NULL;
    + goto err;
    + } else {
    + slot->hotplug = hotplug;
    goto out;
    }
    - }
    + } else if (slot)
    + goto out;

    placeholder:
    slot = kzalloc(sizeof(*slot), GFP_KERNEL);
    @@ -148,10 +229,17 @@ placeholder:
    slot->number = slot_nr;

    slot->kobj.kset = pci_slots_kset;
    +
    + slot_name = make_slot_name(name);
    + if (!slot_name) {
    + err = -ENOMEM;
    + goto err;
    + }
    +
    err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL,
    - "%s", name);
    + "%s", slot_name);
    if (err) {
    - printk(KERN_ERR "Unable to register kobject %s\n", name);
    + printk(KERN_ERR "Unable to register kobject %s\n", slot_name);
    goto err;
    }

    @@ -166,10 +254,10 @@ placeholder:
    pr_debug("%s: created pci_slot on %04x:%02x:%02x\n",
    __func__, pci_domain_nr(parent), parent->number, slot_nr);

    - out:
    +out:
    up_write(&pci_bus_sem);
    return slot;
    - err:
    +err:
    kfree(slot);
    slot = ERR_PTR(err);
    goto out;
    @@ -210,7 +298,6 @@ EXPORT_SYMBOL_GPL(pci_renumber_slot);
    * just call kobject_put on its kobj and let our release methods do the
    * rest.
    */
    -
    void pci_destroy_slot(struct pci_slot *slot)
    {
    pr_debug("%s: dec refcount to %d on %04x:%02x:%02x\n", __func__,
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  17. Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

    * Kenji Kaneshige :
    > Alex Chiang wrote:
    >> * Kenji Kaneshige :
    >>> Thank your new patches. Very quick!!!

    >>
    >> I'm trying to get into 2.6.28.
    >>
    >>> Though I have not reviewed/tested your patches yet (of course), I have
    >>> one concern as I said in the e-mail soon before. Does the new one
    >>> consider the following senario?
    >>>
    >>> Scenario C:
    >>> hotplug driver(A) hotplug_driver(B)
    >>> -------------- ----------------
    >>> pci_create_slot(name=A, rename=1)
    >>> pci_create_slot(name=B, rename=1)
    >>>
    >>> The hotplug driver (A) creates the slot with name "A". The the hotplug
    >>> driver (B) tries to create the same slot, but wants the name "B" instead.
    >>> In this case, hotplug driver fails to create the slot and the slot name
    >>> should not be changed to "B" from "A".

    >>
    >> Hm... I don't think this is a common scenario but...
    >>

    >
    > It was a common scenario until recently because acpiphp and
    > native hotplug drivers(pciehp, shpchp) had different naming
    > rules. That is, acpiphp used _SUN number, while pciehp/shpchp
    > used bus number and physical slot number pair. Although the
    > pciehp/shpchp driver has been changed not to use bus_number
    > for slot names and acpiphp and pciehp/shpchp has the same
    > names on my system now, but I think the scenario is still
    > possible because naming rule of each hotplug driver could be
    > changed in the future again.
    >
    > By the way, acpiphp was changed to handle 64bit _SUN number
    > recently for new ia64 HP servers, IIRC. Are hotplug slots
    > on that server can also be handled through PCIe controller?
    > If it is yes, I think _SUN doesn't match physical slot number
    > because physical slot number (in Slot Capabilities Register)
    > has only 13bit. In this case, the above scenario will happen.


    Hm, ok, I agree.

    >> int pci_hp_register(...)
    >> {
    >> ...
    >>
    >> pci_slot = pci_create_slot(bus, slot_nr, name, 1);
    >> if (IS_ERR(pci_slot)) return
    >> PTR_ERR(pci_slot);
    >>
    >> if (pci_slot->hotplug) {
    >> dbg("%s: already claimed\n", __func__);
    >> pci_destroy_slot(pci_slot);
    >> return -EBUSY;
    >> }
    >> ...
    >> }
    >>
    >> I could maybe move that check into pci_create_slot() instead.
    >>
    >> struct pci_slot *pci_create_slot(...)
    >> {
    >> ...
    >>
    >> /*
    >> * Get existing slot and rename if desired
    >> */
    >> slot = get_slot(parent, slot_nr);
    >> if (slot && rename) {
    >> if ((err = slot->hotplug ? -EBUSY : 0)
    >> || (err = rename_slot(slot, name))) {
    >> kobject_put(&slot->kobj);
    >> slot = NULL;
    >> goto err;
    >> } else
    >> goto out;
    >> } else if (slot)
    >> goto out;
    >> ...
    >> }
    >>
    >> Seems a little ugly to me, but maybe it's necessary?
    >>

    >
    > I don't like this, and I think it's wrong because callers
    > might get -EBUSY even though they are not related to hotplug.
    >
    > I thought of the following alternative ideas, when I was making
    > sample patches. What do you think about those? My was concerned
    > that both need to add hotplug related code into generic pci slot
    > management code/API.
    >
    > - Add 'hotplug' arg to pci_create_slot(), instead of 'rename'
    > flag. The pci_create_slot() would be as follows:
    >
    > struct pci_slot *pci_create_slot(..., struct hotplug_slot *hotplug)
    > {
    > ...
    > /*
    > * Get existing slot and rename if desired
    > */
    > slot = get_slot(parent, slot_nr);
    > if (slot) {
    > if (hotplug) {
    > if ((err = slot->hotplug ? -EBUSY : 0)
    > || err = rename_slot(slot, name))) {
    > Some cleanups;
    > return err;
    > }
    > }
    > goto out;
    > }
    > ...
    > out:
    > if (hotplug)
    > slot->hotplug = hotplug;
    > ...
    > }


    I like this approach a little better, since the flow of execution
    is easier to understand (vs. pci_create_slot + pci_slot_hp_register).

    I prototyped it, but didn't get a chance to test it (I did
    compile it though).

    I'll send 2 test patches shortly that should replace the earlier
    03/16 and 04/16 patches.

    Thanks.

    /ac

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  18. Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

    Alex Chiang wrote:
    > * Kenji Kaneshige :
    >> Alex Chiang wrote:
    >>> * Kenji Kaneshige :
    >>>> Thank your new patches. Very quick!!!
    >>> I'm trying to get into 2.6.28.
    >>>
    >>>> Though I have not reviewed/tested your patches yet (of course), I have
    >>>> one concern as I said in the e-mail soon before. Does the new one
    >>>> consider the following senario?
    >>>>
    >>>> Scenario C:
    >>>> hotplug driver(A) hotplug_driver(B)
    >>>> -------------- ----------------
    >>>> pci_create_slot(name=A, rename=1)
    >>>> pci_create_slot(name=B, rename=1)
    >>>>
    >>>> The hotplug driver (A) creates the slot with name "A". The the hotplug
    >>>> driver (B) tries to create the same slot, but wants the name "B" instead.
    >>>> In this case, hotplug driver fails to create the slot and the slot name
    >>>> should not be changed to "B" from "A".
    >>> Hm... I don't think this is a common scenario but...
    >>>

    >> It was a common scenario until recently because acpiphp and
    >> native hotplug drivers(pciehp, shpchp) had different naming
    >> rules. That is, acpiphp used _SUN number, while pciehp/shpchp
    >> used bus number and physical slot number pair. Although the
    >> pciehp/shpchp driver has been changed not to use bus_number
    >> for slot names and acpiphp and pciehp/shpchp has the same
    >> names on my system now, but I think the scenario is still
    >> possible because naming rule of each hotplug driver could be
    >> changed in the future again.
    >>
    >> By the way, acpiphp was changed to handle 64bit _SUN number
    >> recently for new ia64 HP servers, IIRC. Are hotplug slots
    >> on that server can also be handled through PCIe controller?
    >> If it is yes, I think _SUN doesn't match physical slot number
    >> because physical slot number (in Slot Capabilities Register)
    >> has only 13bit. In this case, the above scenario will happen.

    >
    > Hm, ok, I agree.
    >
    >>> int pci_hp_register(...)
    >>> {
    >>> ...
    >>>
    >>> pci_slot = pci_create_slot(bus, slot_nr, name, 1);
    >>> if (IS_ERR(pci_slot)) return
    >>> PTR_ERR(pci_slot);
    >>>
    >>> if (pci_slot->hotplug) {
    >>> dbg("%s: already claimed\n", __func__);
    >>> pci_destroy_slot(pci_slot);
    >>> return -EBUSY;
    >>> }
    >>> ...
    >>> }
    >>>
    >>> I could maybe move that check into pci_create_slot() instead.
    >>>
    >>> struct pci_slot *pci_create_slot(...)
    >>> {
    >>> ...
    >>>
    >>> /*
    >>> * Get existing slot and rename if desired
    >>> */
    >>> slot = get_slot(parent, slot_nr);
    >>> if (slot && rename) {
    >>> if ((err = slot->hotplug ? -EBUSY : 0)
    >>> || (err = rename_slot(slot, name))) {
    >>> kobject_put(&slot->kobj);
    >>> slot = NULL;
    >>> goto err;
    >>> } else
    >>> goto out;
    >>> } else if (slot)
    >>> goto out;
    >>> ...
    >>> }
    >>>
    >>> Seems a little ugly to me, but maybe it's necessary?
    >>>

    >> I don't like this, and I think it's wrong because callers
    >> might get -EBUSY even though they are not related to hotplug.
    >>
    >> I thought of the following alternative ideas, when I was making
    >> sample patches. What do you think about those? My was concerned
    >> that both need to add hotplug related code into generic pci slot
    >> management code/API.
    >>
    >> - Add 'hotplug' arg to pci_create_slot(), instead of 'rename'
    >> flag. The pci_create_slot() would be as follows:
    >>
    >> struct pci_slot *pci_create_slot(..., struct hotplug_slot *hotplug)
    >> {
    >> ...
    >> /*
    >> * Get existing slot and rename if desired
    >> */
    >> slot = get_slot(parent, slot_nr);
    >> if (slot) {
    >> if (hotplug) {
    >> if ((err = slot->hotplug ? -EBUSY : 0)
    >> || err = rename_slot(slot, name))) {
    >> Some cleanups;
    >> return err;
    >> }
    >> }
    >> goto out;
    >> }
    >> ...
    >> out:
    >> if (hotplug)
    >> slot->hotplug = hotplug;
    >> ...
    >> }

    >
    > I like this approach a little better, since the flow of execution
    > is easier to understand (vs. pci_create_slot + pci_slot_hp_register).
    >
    > I prototyped it, but didn't get a chance to test it (I did
    > compile it though).
    >
    > I'll send 2 test patches shortly that should replace the earlier
    > 03/16 and 04/16 patches.
    >


    I'm sorry, but I forgot to tell you one important thing. Now we are
    trying to change pci slot management API to setup pci_slot->hotplug.
    We must consider how to implement the counterpart to clean up
    pci_slot->hotplug at the same time. My current idea is adding hotplug
    arg to pci_destroy_slot(), but it seems a little ugly...

    Thanks,
    Kenji Kaneshige

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  19. Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

    * Kenji Kaneshige :
    >
    > I'm sorry, but I forgot to tell you one important thing. Now we
    > are trying to change pci slot management API to setup
    > pci_slot->hotplug. We must consider how to implement the
    > counterpart to clean up pci_slot->hotplug at the same time. My
    > current idea is adding hotplug arg to pci_destroy_slot(), but
    > it seems a little ugly...


    Ugh, I'm not sure which is worse, an unbalanced API vs passing a
    hotplug_slot to pci_destroy_slot.

    pci_destroy_slot should never touch the pci_slot->hotplug
    argument, I think, because it is possible for non-hotplug callers
    to call pci_create_slot.

    I think the rule should just be:

    - all hotplug drivers must use pci_hp_register/deregister
    - hotplug drivers must pass a valid hotplug_slot argument

    - all detection drivers must use pci_create/destroy_slot
    - detection drivers must pass NULL for hotplug_slot

    If you prefer, we could wrap pci_create_slot for detection
    drivers:

    struct pci_slot *pci_slot_register(struct pci_bus *parent, int slot_nr,
    const char *name)

    {
    return pci_create_slot(parent, slot_nr, name, NULL);
    }
    EXPORT_SYMBOL_GPL(pci_slot_register);

    And then do not export pci_create_slot().

    Hm?

    /ac

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  20. Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

    Alex Chiang wrote:
    > * Kenji Kaneshige :
    >> I'm sorry, but I forgot to tell you one important thing. Now we
    >> are trying to change pci slot management API to setup
    >> pci_slot->hotplug. We must consider how to implement the
    >> counterpart to clean up pci_slot->hotplug at the same time. My
    >> current idea is adding hotplug arg to pci_destroy_slot(), but
    >> it seems a little ugly...

    >
    > Ugh, I'm not sure which is worse, an unbalanced API vs passing a
    > hotplug_slot to pci_destroy_slot.
    >
    > pci_destroy_slot should never touch the pci_slot->hotplug
    > argument, I think, because it is possible for non-hotplug callers
    > to call pci_create_slot.
    >
    > I think the rule should just be:
    >
    > - all hotplug drivers must use pci_hp_register/deregister
    > - hotplug drivers must pass a valid hotplug_slot argument
    >
    > - all detection drivers must use pci_create/destroy_slot
    > - detection drivers must pass NULL for hotplug_slot
    >


    I'm still concerned about one thing. Now that we added hotplug
    arg to pci_create_slot(), pci_bus_sem could be used for changing
    pci_slot->hotplug. But if there is no generic API to clean up
    pci_slot->hotplug, we can't use pci_bus_sem because pci hotplug
    core cannot hold pci_bus_sem. As a result, we need to add the
    following rule in addition to the above rules.

    - hotplug core driver must hold some mutex/semaphore when
    calling pci_create_slot()
    - hotplug core driver must hold the same mutex/semaphore
    when cleaning up pci_slot->hotplug.

    That is, even though pci_create_slot provides the interface to
    setup pci_slot->hotplug, caller needs to pay attention to race
    condition about pci_slot->hotplug. I think it's a little strange.

    But I might be overcareful...

    Thanks,
    Kenji Kaneshige



    > If you prefer, we could wrap pci_create_slot for detection
    > drivers:
    >
    > struct pci_slot *pci_slot_register(struct pci_bus *parent, int slot_nr,
    > const char *name)
    >
    > {
    > return pci_create_slot(parent, slot_nr, name, NULL);
    > }
    > EXPORT_SYMBOL_GPL(pci_slot_register);
    >
    > And then do not export pci_create_slot().
    >
    > Hm?
    >
    > /ac
    >
    >
    >



    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread
Page 1 of 2 1 2 LastLast