Skip to content

Commit

Permalink
PCI: hotplug: Drop hotplug_slot_info
Browse files Browse the repository at this point in the history 
Ever since the PCI hotplug core was introduced in 2002, drivers had to
allocate and register a struct hotplug_slot_info for every slot:
https://git.kernel.org/tglx/history/c/a8a2069f432c

Apparently the idea was that drivers furnish the hotplug core with an
up-to-date card presence status, power status, latch status and
attention indicator status as well as notify the hotplug core of changes
thereof.  However only 4 out of 12 hotplug drivers bother to notify the
hotplug core with pci_hp_change_slot_info() and the hotplug core never
made any use of the information:  There is just a single macro in
pci_hotplug_core.c, GET_STATUS(), which uses the hotplug_slot_info if
the driver lacks the corresponding callback in hotplug_slot_ops.  The
macro is called when the user reads the attribute via sysfs.

Now, if the callback isn't defined, the attribute isn't exposed in sysfs
in the first place (see e.g. has_power_file()).  There are only two
situations when the hotplug_slot_info would actually be accessed:

* If the driver defines ->enable_slot or ->disable_slot but not
  ->get_power_status.

* If the driver defines ->set_attention_status but not
  ->get_attention_status.

There is no driver doing the former and just a single driver doing the
latter, namely pnv_php.c.  Amend it with a ->get_attention_status
callback.  With that, the hotplug_slot_info becomes completely unused by
the PCI hotplug core.  But a few drivers use it internally as a cache:

cpcihp uses it to cache the latch_status and adapter_status.
cpqhp uses it to cache the adapter_status.
pnv_php and rpaphp use it to cache the attention_status.
shpchp uses it to cache all four values.

Amend these drivers to cache the information in their private slot
struct.  shpchp's slot struct already contains members to cache the
power_status and adapter_status, so additional members are only needed
for the other two values.  In the case of cpqphp, the cached value is
only accessed in a single place, so instead of caching it, read the
current value from the hardware.

Caution:  acpiphp, cpci, cpqhp, shpchp, asus-wmi and eeepc-laptop
populate the hotplug_slot_info with initial values on probe.  That code
is herewith removed.  There is a theoretical chance that the code has
side effects without which the driver fails to function, e.g. if the
ACPI method to read the adapter status needs to be executed at least
once on probe.  That seems unlikely to me, still maintainers should
review the changes carefully for this possibility.

Rafael adds: "I'm not aware of any case in which it will break anything,
[...] but if that happens, it may be necessary to add the execution of
the control methods in question directly to the initialization part."

Signed-off-by: Lukas Wunner <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: Rafael J. Wysocki <[email protected]>
Acked-by: Tyrel Datwyler <[email protected]>  # drivers/pci/hotplug/rpa*
Acked-by: Sebastian Ott <[email protected]>        # drivers/pci/hotplug/s390*
Acked-by: Andy Shevchenko <[email protected]> # drivers/platform/x86
Cc: Len Brown <[email protected]>
Cc: Scott Murray <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Oliver OHalloran <[email protected]>
Cc: Gavin Shan <[email protected]>
Cc: Gerald Schaefer <[email protected]>
Cc: Corentin Chary <[email protected]>
Cc: Darren Hart <[email protected]>
  • Loading branch information
@l1k @bjorn-helgaas
l1k authored and bjorn-helgaas committed Sep 18, 2018
1 parent 81c4b5b commit a7da216
Show file tree
Hide file tree
Showing 24 changed files with 64 additions and 340 deletions.
2 changes: 1 addition & 1 deletion arch/powerpc/include/asm/pnv-pci.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ void pnv_cxl_release_hwirq_ranges(struct cxl_irq_ranges *irqs,

struct pnv_php_slot {
struct hotplug_slot slot;
struct hotplug_slot_info slot_info;
uint64_t id;
char *name;
int slot_no;
Expand All @@ -72,6 +71,7 @@ struct pnv_php_slot {
struct pci_dev *pdev;
struct pci_bus *bus;
bool power_state_check;
u8 attention_state;
void *fdt;
void *dt;
struct of_changeset ocs;
Expand Down
1 change: 0 additions & 1 deletion drivers/pci/hotplug/acpiphp.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ struct acpiphp_slot;
struct slot {
struct hotplug_slot *hotplug_slot;
struct acpiphp_slot *acpi_slot;
struct hotplug_slot_info info;
unsigned int sun; /* ACPI _SUN (Slot User Number) value */
};

Expand Down
6 changes: 0 additions & 6 deletions drivers/pci/hotplug/acpiphp_core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,16 +270,10 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot,
if (!slot->hotplug_slot)
goto error_slot;

slot->hotplug_slot->info = &slot->info;

slot->hotplug_slot->private = slot;
slot->hotplug_slot->ops = &acpi_hotplug_slot_ops;

slot->acpi_slot = acpiphp_slot;
slot->hotplug_slot->info->power_status = acpiphp_get_power_status(slot->acpi_slot);
slot->hotplug_slot->info->attention_status = 0;
slot->hotplug_slot->info->latch_status = acpiphp_get_latch_status(slot->acpi_slot);
slot->hotplug_slot->info->adapter_status = acpiphp_get_adapter_status(slot->acpi_slot);

acpiphp_slot->slot = slot;
slot->sun = sun;
Expand Down
2 changes: 2 additions & 0 deletions drivers/pci/hotplug/cpci_hotplug.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ struct slot {
unsigned int devfn;
struct pci_bus *bus;
struct pci_dev *dev;
unsigned int latch_status:1;
unsigned int adapter_status:1;
unsigned int extracting;
struct hotplug_slot *hotplug_slot;
struct list_head slot_list;
Expand Down
72 changes: 14 additions & 58 deletions drivers/pci/hotplug/cpci_hotplug_core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,26 +67,6 @@ static const struct hotplug_slot_ops cpci_hotplug_slot_ops = {
.get_latch_status = get_latch_status,
};

static int
update_latch_status(struct hotplug_slot *hotplug_slot, u8 value)
{
struct hotplug_slot_info info;

memcpy(&info, hotplug_slot->info, sizeof(struct hotplug_slot_info));
info.latch_status = value;
return pci_hp_change_slot_info(hotplug_slot, &info);
}

static int
update_adapter_status(struct hotplug_slot *hotplug_slot, u8 value)
{
struct hotplug_slot_info info;

memcpy(&info, hotplug_slot->info, sizeof(struct hotplug_slot_info));
info.adapter_status = value;
return pci_hp_change_slot_info(hotplug_slot, &info);
}

static int
enable_slot(struct hotplug_slot *hotplug_slot)
{
Expand Down Expand Up @@ -135,8 +115,7 @@ disable_slot(struct hotplug_slot *hotplug_slot)
goto disable_error;
}

if (update_adapter_status(slot->hotplug_slot, 0))
warn("failure to update adapter file");
slot->adapter_status = 0;

if (slot->extracting) {
slot->extracting = 0;
Expand Down Expand Up @@ -184,20 +163,23 @@ set_attention_status(struct hotplug_slot *hotplug_slot, u8 status)
static int
get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
{
*value = hotplug_slot->info->adapter_status;
struct slot *slot = hotplug_slot->private;

*value = slot->adapter_status;
return 0;
}

static int
get_latch_status(struct hotplug_slot *hotplug_slot, u8 *value)
{
*value = hotplug_slot->info->latch_status;
struct slot *slot = hotplug_slot->private;

*value = slot->latch_status;
return 0;
}

static void release_slot(struct slot *slot)
{
kfree(slot->hotplug_slot->info);
kfree(slot->hotplug_slot);
pci_dev_put(slot->dev);
kfree(slot);
Expand All @@ -210,7 +192,6 @@ 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[SLOT_NAME_SIZE];
int status;
int i;
Expand All @@ -237,13 +218,6 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last)
}
slot->hotplug_slot = hotplug_slot;

info = kzalloc(sizeof(struct hotplug_slot_info), GFP_KERNEL);
if (!info) {
status = -ENOMEM;
goto error_hpslot;
}
hotplug_slot->info = info;

slot->bus = bus;
slot->number = i;
slot->devfn = PCI_DEVFN(i, 0);
Expand All @@ -253,19 +227,11 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last)
hotplug_slot->private = slot;
hotplug_slot->ops = &cpci_hotplug_slot_ops;

/*
* Initialize the slot info structure with some known
* good values.
*/
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", name);
status = pci_hp_register(slot->hotplug_slot, bus, i, name);
if (status) {
err("pci_hp_register failed with error %d", status);
goto error_info;
goto error_hpslot;
}
dbg("slot registered with name: %s", slot_name(slot));

Expand All @@ -276,8 +242,6 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last)
up_write(&list_rwsem);
}
return 0;
error_info:
kfree(info);
error_hpslot:
kfree(hotplug_slot);
error_slot:
Expand Down Expand Up @@ -359,10 +323,8 @@ init_slots(int clear_ins)
__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))
warn("failure to update adapter file");
if (update_latch_status(slot->hotplug_slot, 1))
warn("failure to update latch file");
slot->adapter_status = 1;
slot->latch_status = 1;
slot->dev = dev;
}
}
Expand Down Expand Up @@ -424,11 +386,8 @@ check_slots(void)
dbg("%s - slot %s HS_CSR (2) = %04x",
__func__, slot_name(slot), hs_csr);

if (update_latch_status(slot->hotplug_slot, 1))
warn("failure to update latch file");

if (update_adapter_status(slot->hotplug_slot, 1))
warn("failure to update adapter file");
slot->latch_status = 1;
slot->adapter_status = 1;

cpci_led_off(slot);

Expand All @@ -449,9 +408,7 @@ check_slots(void)
__func__, slot_name(slot), hs_csr);

if (!slot->extracting) {
if (update_latch_status(slot->hotplug_slot, 0))
warn("failure to update latch file");

slot->latch_status = 0;
slot->extracting = 1;
atomic_inc(&extracting);
}
Expand All @@ -465,8 +422,7 @@ check_slots(void)
*/
err("card in slot %s was improperly removed",
slot_name(slot));
if (update_adapter_status(slot->hotplug_slot, 0))
warn("failure to update adapter file");
slot->adapter_status = 0;
slot->extracting = 0;
atomic_dec(&extracting);
}
Expand Down
22 changes: 1 addition & 21 deletions drivers/pci/hotplug/cpqphp_core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,6 @@ static int ctrl_slot_cleanup(struct controller *ctrl)
while (old_slot) {
next_slot = old_slot->next;
pci_hp_deregister(old_slot->hotplug_slot);
kfree(old_slot->hotplug_slot->info);
kfree(old_slot->hotplug_slot);
kfree(old_slot);
old_slot = next_slot;
Expand Down Expand Up @@ -579,7 +578,6 @@ static int ctrl_slot_setup(struct controller *ctrl,
{
struct slot *slot;
struct hotplug_slot *hotplug_slot;
struct hotplug_slot_info *hotplug_slot_info;
struct pci_bus *bus = ctrl->pci_bus;
u8 number_of_slots;
u8 slot_device;
Expand Down Expand Up @@ -613,14 +611,6 @@ static int ctrl_slot_setup(struct controller *ctrl,
}
hotplug_slot = slot->hotplug_slot;

hotplug_slot->info = kzalloc(sizeof(*(hotplug_slot->info)),
GFP_KERNEL);
if (!hotplug_slot->info) {
result = -ENOMEM;
goto error_hpslot;
}
hotplug_slot_info = hotplug_slot->info;

slot->ctrl = ctrl;
slot->bus = ctrl->bus;
slot->device = slot_device;
Expand Down Expand Up @@ -673,14 +663,6 @@ static int ctrl_slot_setup(struct controller *ctrl,
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);
hotplug_slot_info->latch_status =
cpq_get_latch_status(ctrl, slot);
hotplug_slot_info->adapter_status =
get_presence_status(ctrl, slot);

dbg("registering bus %d, dev %d, number %d, ctrl->slot_device_offset %d, slot %d\n",
slot->bus, slot->device,
slot->number, ctrl->slot_device_offset,
Expand All @@ -691,7 +673,7 @@ static int ctrl_slot_setup(struct controller *ctrl,
name);
if (result) {
err("pci_hp_register failed with error %d\n", result);
goto error_info;
goto error_hpslot;
}

slot->next = ctrl->slot;
Expand All @@ -703,8 +685,6 @@ static int ctrl_slot_setup(struct controller *ctrl,
}

return 0;
error_info:
kfree(hotplug_slot_info);
error_hpslot:
kfree(hotplug_slot);
error_slot:
Expand Down
31 changes: 2 additions & 29 deletions drivers/pci/hotplug/cpqphp_ctrl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1130,9 +1130,9 @@ static u8 set_controller_speed(struct controller *ctrl, u8 adapter_speed, u8 hp_
for (slot = ctrl->slot; slot; slot = slot->next) {
if (slot->device == (hp_slot + ctrl->slot_device_offset))
continue;
if (!slot->hotplug_slot || !slot->hotplug_slot->info)
if (!slot->hotplug_slot)
continue;
if (slot->hotplug_slot->info->adapter_status == 0)
if (get_presence_status(ctrl, slot) == 0)
continue;
/* If another adapter is running on the same segment but at a
* lower speed/mode, we allow the new adapter to function at
Expand Down Expand Up @@ -1767,24 +1767,6 @@ void cpqhp_event_stop_thread(void)
}


static int update_slot_info(struct controller *ctrl, struct slot *slot)
{
struct hotplug_slot_info *info;
int result;

info = kmalloc(sizeof(*info), GFP_KERNEL);
if (!info)
return -ENOMEM;

info->power_status = get_slot_enabled(ctrl, slot);
info->attention_status = cpq_get_attention_status(ctrl, slot);
info->latch_status = cpq_get_latch_status(ctrl, slot);
info->adapter_status = get_presence_status(ctrl, slot);
result = pci_hp_change_slot_info(slot->hotplug_slot, info);
kfree(info);
return result;
}

static void interrupt_event_handler(struct controller *ctrl)
{
int loop = 0;
Expand Down Expand Up @@ -1884,9 +1866,6 @@ static void interrupt_event_handler(struct controller *ctrl)
/***********POWER FAULT */
else if (ctrl->event_queue[loop].event_type == INT_POWER_FAULT) {
dbg("power fault\n");
} else {
/* refresh notification */
update_slot_info(ctrl, p_slot);
}

ctrl->event_queue[loop].event_type = 0;
Expand Down Expand Up @@ -2057,9 +2036,6 @@ int cpqhp_process_SI(struct controller *ctrl, struct pci_func *func)
if (rc)
dbg("%s: rc = %d\n", __func__, rc);

if (p_slot)
update_slot_info(ctrl, p_slot);

return rc;
}

Expand Down Expand Up @@ -2125,9 +2101,6 @@ int cpqhp_process_SS(struct controller *ctrl, struct pci_func *func)
rc = 1;
}

if (p_slot)
update_slot_info(ctrl, p_slot);

return rc;
}

Expand Down
Loading

0 comments on commit a7da216

Please sign in to comment.