* [PATCH 0/4] PCI: pciehp cleanup
@ 2015-06-18 16:12 Bjorn Helgaas
2015-06-18 16:12 ` [PATCH 1/4] PCI: pciehp: Clean up debug logging Bjorn Helgaas
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Bjorn Helgaas @ 2015-06-18 16:12 UTC (permalink / raw)
To: linux-pci; +Cc: Rajat Jain, Yinghai Lu, linux-kernel
These patches don't *do* anything except get rid of some overly verbose
debug logging and a bunch of boilerplate code by inlining trivial things.
Unless I made a mistake, this should not change the way anything works at
all, although I did change some of the message text.
---
Bjorn Helgaas (4):
PCI: pciehp: Clean up debug logging
PCI: pciehp: Make queue_interrupt_event() void
PCI: pciehp: Rename queue_interrupt_event() to pciehp_queue_interrupt_event()
PCI: pciehp: Inline the "handle event" functions into the ISR
drivers/pci/hotplug/pciehp.h | 6 -
drivers/pci/hotplug/pciehp_core.c | 37 +--------
drivers/pci/hotplug/pciehp_ctrl.c | 154 +++----------------------------------
drivers/pci/hotplug/pciehp_hpc.c | 89 +++++++++------------
4 files changed, 53 insertions(+), 233 deletions(-)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/4] PCI: pciehp: Clean up debug logging
2015-06-18 16:12 [PATCH 0/4] PCI: pciehp cleanup Bjorn Helgaas
@ 2015-06-18 16:12 ` Bjorn Helgaas
2015-06-18 17:27 ` Rajat Jain
2015-06-18 16:12 ` [PATCH 2/4] PCI: pciehp: Make queue_interrupt_event() void Bjorn Helgaas
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Bjorn Helgaas @ 2015-06-18 16:12 UTC (permalink / raw)
To: linux-pci; +Cc: Rajat Jain, Yinghai Lu, linux-kernel
The pciehp debug logging is overly verbose and often redundant. Almost all
of the information printed by dbg_ctrl() is also printed by the normal PCI
core enumeration code and by pcie_init().
Remove the redundant debug info.
When claiming a pciehp bridge, we print the slot characteristics, e.g.,
Slot #6 AttnBtn- AttnInd- PwrInd- PwrCtrl- MRL- Interlock- NoCompl+ LLActRep+
Add the Hot-Plug Capable and Hot-Plug Surprise bits to this information,
and print it all in the same order as lspci does.
No functional change except the message text changes.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/hotplug/pciehp_core.c | 39 ++-------------------------
drivers/pci/hotplug/pciehp_ctrl.c | 38 +++++---------------------
drivers/pci/hotplug/pciehp_hpc.c | 54 +++++++------------------------------
3 files changed, 20 insertions(+), 111 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 4597f6b..612b21a 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -77,11 +77,6 @@ static int reset_slot (struct hotplug_slot *slot, int probe);
*/
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(hotplug_slot));
-
kfree(hotplug_slot->ops);
kfree(hotplug_slot->info);
kfree(hotplug_slot);
@@ -129,14 +124,10 @@ static int init_slot(struct controller *ctrl)
slot->hotplug_slot = hotplug;
snprintf(name, SLOT_NAME_SIZE, "%u", PSN(ctrl));
- ctrl_dbg(ctrl, "Registering domain:bus:dev=%04x:%02x:00 sun=%x\n",
- pci_domain_nr(ctrl->pcie->port->subordinate),
- ctrl->pcie->port->subordinate->number, PSN(ctrl));
retval = pci_hp_register(hotplug,
ctrl->pcie->port->subordinate, 0, name);
if (retval)
- ctrl_err(ctrl,
- "pci_hp_register failed with error %d\n", retval);
+ ctrl_err(ctrl, "pci_hp_register failed: error %d\n", retval);
out:
if (retval) {
kfree(ops);
@@ -158,9 +149,6 @@ 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__, slot_name(slot));
-
pciehp_set_attention_status(slot, status);
return 0;
}
@@ -170,9 +158,6 @@ 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__, slot_name(slot));
-
return pciehp_sysfs_enable_slot(slot);
}
@@ -181,9 +166,6 @@ 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__, slot_name(slot));
-
return pciehp_sysfs_disable_slot(slot);
}
@@ -191,9 +173,6 @@ static int get_power_status(struct hotplug_slot *hotplug_slot, u8 *value)
{
struct slot *slot = hotplug_slot->private;
- ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
- __func__, slot_name(slot));
-
pciehp_get_power_status(slot, value);
return 0;
}
@@ -202,9 +181,6 @@ static int get_attention_status(struct hotplug_slot *hotplug_slot, u8 *value)
{
struct slot *slot = hotplug_slot->private;
- ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
- __func__, slot_name(slot));
-
pciehp_get_attention_status(slot, value);
return 0;
}
@@ -213,9 +189,6 @@ static int get_latch_status(struct hotplug_slot *hotplug_slot, u8 *value)
{
struct slot *slot = hotplug_slot->private;
- ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
- __func__, slot_name(slot));
-
pciehp_get_latch_status(slot, value);
return 0;
}
@@ -224,9 +197,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
{
struct slot *slot = hotplug_slot->private;
- ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
- __func__, slot_name(slot));
-
pciehp_get_adapter_status(slot, value);
return 0;
}
@@ -235,9 +205,6 @@ static int reset_slot(struct hotplug_slot *hotplug_slot, int probe)
{
struct slot *slot = hotplug_slot->private;
- ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
- __func__, slot_name(slot));
-
return pciehp_reset_slot(slot, probe);
}
@@ -272,14 +239,14 @@ static int pciehp_probe(struct pcie_device *dev)
if (rc == -EBUSY)
ctrl_warn(ctrl, "Slot already registered by another hotplug driver\n");
else
- ctrl_err(ctrl, "Slot initialization failed\n");
+ ctrl_err(ctrl, "Slot initialization failed (%d)\n", rc);
goto err_out_release_ctlr;
}
/* Enable events after we have setup the data structures */
rc = pcie_init_notification(ctrl);
if (rc) {
- ctrl_err(ctrl, "Notification initialization failed\n");
+ ctrl_err(ctrl, "Notification initialization failed (%d)\n", rc);
goto err_out_free_ctrl_slot;
}
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index f052e95..4bdaf62 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -59,9 +59,6 @@ u8 pciehp_handle_attention_button(struct slot *p_slot)
u32 event_type;
struct controller *ctrl = p_slot->ctrl;
- /* Attention Button Change */
- ctrl_dbg(ctrl, "Attention button interrupt received\n");
-
/*
* Button pressed - See if need to TAKE ACTION!!!
*/
@@ -79,9 +76,6 @@ u8 pciehp_handle_switch_change(struct slot *p_slot)
u32 event_type;
struct controller *ctrl = p_slot->ctrl;
- /* Switch Change */
- ctrl_dbg(ctrl, "Switch interrupt received\n");
-
pciehp_get_latch_status(p_slot, &getstatus);
if (getstatus) {
/*
@@ -108,9 +102,6 @@ u8 pciehp_handle_presence_change(struct slot *p_slot)
u8 presence_save;
struct controller *ctrl = p_slot->ctrl;
- /* Presence Change */
- ctrl_dbg(ctrl, "Presence/Notify input change\n");
-
/* Switch is open, assume a presence change
* Save the presence state
*/
@@ -140,8 +131,6 @@ u8 pciehp_handle_power_fault(struct slot *p_slot)
u32 event_type;
struct controller *ctrl = p_slot->ctrl;
- /* power fault */
- ctrl_dbg(ctrl, "Power fault interrupt received\n");
ctrl_err(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);
@@ -155,9 +144,6 @@ void pciehp_handle_linkstate_change(struct slot *p_slot)
u32 event_type;
struct controller *ctrl = p_slot->ctrl;
- /* Link Status Change */
- ctrl_dbg(ctrl, "Data Link Layer State change\n");
-
if (pciehp_check_link_active(ctrl)) {
ctrl_info(ctrl, "slot(%s): Link Up event\n",
slot_name(p_slot));
@@ -298,10 +284,6 @@ static void pciehp_power_thread(struct work_struct *work)
switch (info->req) {
case DISABLE_REQ:
- ctrl_dbg(p_slot->ctrl,
- "Disabling domain:bus:device=%04x:%02x:00\n",
- pci_domain_nr(p_slot->ctrl->pcie->port->subordinate),
- p_slot->ctrl->pcie->port->subordinate->number);
mutex_lock(&p_slot->hotplug_lock);
pciehp_disable_slot(p_slot);
mutex_unlock(&p_slot->hotplug_lock);
@@ -310,10 +292,6 @@ static void pciehp_power_thread(struct work_struct *work)
mutex_unlock(&p_slot->lock);
break;
case ENABLE_REQ:
- ctrl_dbg(p_slot->ctrl,
- "Enabling domain:bus:device=%04x:%02x:00\n",
- pci_domain_nr(p_slot->ctrl->pcie->port->subordinate),
- p_slot->ctrl->pcie->port->subordinate->number);
mutex_lock(&p_slot->hotplug_lock);
ret = pciehp_enable_slot(p_slot);
mutex_unlock(&p_slot->hotplug_lock);
@@ -416,7 +394,7 @@ static void handle_button_press_event(struct slot *p_slot)
ctrl_info(ctrl, "Button ignore on Slot(%s)\n", slot_name(p_slot));
break;
default:
- ctrl_warn(ctrl, "Not a valid state\n");
+ ctrl_warn(ctrl, "ignoring invalid state %#x\n", p_slot->state);
break;
}
}
@@ -507,8 +485,8 @@ static void handle_link_event(struct slot *p_slot, u32 event)
}
break;
default:
- ctrl_err(ctrl, "Not a valid state on slot(%s)\n",
- slot_name(p_slot));
+ ctrl_err(ctrl, "ignoring invalid state %#x on slot(%s)\n",
+ p_slot->state, slot_name(p_slot));
kfree(info);
break;
}
@@ -532,7 +510,6 @@ static void interrupt_event_handler(struct work_struct *work)
pciehp_green_led_off(p_slot);
break;
case INT_PRESENCE_ON:
- ctrl_dbg(ctrl, "Surprise Insertion\n");
handle_surprise_event(p_slot);
break;
case INT_PRESENCE_OFF:
@@ -540,7 +517,6 @@ static void interrupt_event_handler(struct work_struct *work)
* Regardless of surprise capability, we need to
* definitely remove a card that has been pulled out!
*/
- ctrl_dbg(ctrl, "Surprise Removal\n");
handle_surprise_event(p_slot);
break;
case INT_LINK_UP:
@@ -647,8 +623,8 @@ int pciehp_sysfs_enable_slot(struct slot *p_slot)
slot_name(p_slot));
break;
default:
- ctrl_err(ctrl, "Not a valid state on slot %s\n",
- slot_name(p_slot));
+ ctrl_err(ctrl, "invalid state %#x on slot %s\n",
+ p_slot->state, slot_name(p_slot));
break;
}
mutex_unlock(&p_slot->lock);
@@ -682,8 +658,8 @@ int pciehp_sysfs_disable_slot(struct slot *p_slot)
slot_name(p_slot));
break;
default:
- ctrl_err(ctrl, "Not a valid state on slot %s\n",
- slot_name(p_slot));
+ ctrl_err(ctrl, "invalid state %#x on slot %s\n",
+ p_slot->state, 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 6d68688..e9daaa3 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -312,7 +312,8 @@ int pciehp_check_link_status(struct controller *ctrl)
ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
if ((lnk_status & PCI_EXP_LNKSTA_LT) ||
!(lnk_status & PCI_EXP_LNKSTA_NLW)) {
- ctrl_err(ctrl, "Link Training Error occurs\n");
+ ctrl_err(ctrl, "link training error: status %#06x\n",
+ lnk_status);
return -1;
}
@@ -556,7 +557,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
intr_loc);
} while (detected);
- ctrl_dbg(ctrl, "%s: intr_loc %x\n", __func__, intr_loc);
+ ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", intr_loc);
/* Check Command Complete Interrupt Pending */
if (intr_loc & PCI_EXP_SLTSTA_CC) {
@@ -748,48 +749,13 @@ static void pcie_cleanup_slot(struct controller *ctrl)
static inline void dbg_ctrl(struct controller *ctrl)
{
- int i;
- u16 reg16;
struct pci_dev *pdev = ctrl->pcie->port;
+ u16 reg16;
if (!pciehp_debug)
return;
- ctrl_info(ctrl, "Hotplug Controller:\n");
- ctrl_info(ctrl, " Seg/Bus/Dev/Func/IRQ : %s IRQ %d\n",
- pci_name(pdev), pdev->irq);
- ctrl_info(ctrl, " Vendor ID : 0x%04x\n", pdev->vendor);
- ctrl_info(ctrl, " Device ID : 0x%04x\n", pdev->device);
- ctrl_info(ctrl, " Subsystem ID : 0x%04x\n",
- pdev->subsystem_device);
- ctrl_info(ctrl, " Subsystem Vendor ID : 0x%04x\n",
- pdev->subsystem_vendor);
- ctrl_info(ctrl, " PCIe Cap offset : 0x%02x\n",
- pci_pcie_cap(pdev));
- for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
- if (!pci_resource_len(pdev, i))
- continue;
- ctrl_info(ctrl, " PCI resource [%d] : %pR\n",
- i, &pdev->resource[i]);
- }
ctrl_info(ctrl, "Slot Capabilities : 0x%08x\n", ctrl->slot_cap);
- ctrl_info(ctrl, " Physical Slot Number : %d\n", PSN(ctrl));
- ctrl_info(ctrl, " Attention Button : %3s\n",
- ATTN_BUTTN(ctrl) ? "yes" : "no");
- ctrl_info(ctrl, " Power Controller : %3s\n",
- POWER_CTRL(ctrl) ? "yes" : "no");
- ctrl_info(ctrl, " MRL Sensor : %3s\n",
- MRL_SENS(ctrl) ? "yes" : "no");
- ctrl_info(ctrl, " Attention Indicator : %3s\n",
- ATTN_LED(ctrl) ? "yes" : "no");
- ctrl_info(ctrl, " Power Indicator : %3s\n",
- PWR_LED(ctrl) ? "yes" : "no");
- ctrl_info(ctrl, " Hot-Plug Surprise : %3s\n",
- HP_SUPR_RM(ctrl) ? "yes" : "no");
- ctrl_info(ctrl, " EMI Present : %3s\n",
- EMI(ctrl) ? "yes" : "no");
- ctrl_info(ctrl, " Command Completed : %3s\n",
- NO_CMD_CMPL(ctrl) ? "no" : "yes");
pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, ®16);
ctrl_info(ctrl, "Slot Status : 0x%04x\n", reg16);
pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, ®16);
@@ -818,10 +784,8 @@ struct controller *pcie_init(struct pcie_device *dev)
/* Check if Data Link Layer Link Active Reporting is implemented */
pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &link_cap);
- if (link_cap & PCI_EXP_LNKCAP_DLLLARC) {
- ctrl_dbg(ctrl, "Link Active Reporting supported\n");
+ if (link_cap & PCI_EXP_LNKCAP_DLLLARC)
ctrl->link_active_reporting = 1;
- }
/* Clear all remaining event bits in Slot Status register */
pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
@@ -829,13 +793,15 @@ struct controller *pcie_init(struct pcie_device *dev)
PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
- ctrl_info(ctrl, "Slot #%d AttnBtn%c AttnInd%c PwrInd%c PwrCtrl%c MRL%c Interlock%c NoCompl%c LLActRep%c\n",
+ ctrl_info(ctrl, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c LLActRep%c\n",
(slot_cap & PCI_EXP_SLTCAP_PSN) >> 19,
FLAG(slot_cap, PCI_EXP_SLTCAP_ABP),
- FLAG(slot_cap, PCI_EXP_SLTCAP_AIP),
- FLAG(slot_cap, PCI_EXP_SLTCAP_PIP),
FLAG(slot_cap, PCI_EXP_SLTCAP_PCP),
FLAG(slot_cap, PCI_EXP_SLTCAP_MRLSP),
+ FLAG(slot_cap, PCI_EXP_SLTCAP_AIP),
+ FLAG(slot_cap, PCI_EXP_SLTCAP_PIP),
+ FLAG(slot_cap, PCI_EXP_SLTCAP_HPC),
+ FLAG(slot_cap, PCI_EXP_SLTCAP_HPS),
FLAG(slot_cap, PCI_EXP_SLTCAP_EIP),
FLAG(slot_cap, PCI_EXP_SLTCAP_NCCS),
FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC));
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/4] PCI: pciehp: Make queue_interrupt_event() void
2015-06-18 16:12 [PATCH 0/4] PCI: pciehp cleanup Bjorn Helgaas
2015-06-18 16:12 ` [PATCH 1/4] PCI: pciehp: Clean up debug logging Bjorn Helgaas
@ 2015-06-18 16:12 ` Bjorn Helgaas
2015-06-18 16:12 ` [PATCH 3/4] PCI: pciehp: Rename queue_interrupt_event() to pciehp_queue_interrupt_event() Bjorn Helgaas
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Bjorn Helgaas @ 2015-06-18 16:12 UTC (permalink / raw)
To: linux-pci; +Cc: Rajat Jain, Yinghai Lu, linux-kernel
Nobody looks at the return value from queue_interrupt_event(), so errors
were silently ignored. Convert it to a "void" function and note the error
in the dmesg log.
No functional change except the new message.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/hotplug/pciehp_ctrl.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 4bdaf62..1086041 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -37,21 +37,20 @@
static void interrupt_event_handler(struct work_struct *work);
-static int queue_interrupt_event(struct slot *p_slot, u32 event_type)
+static void queue_interrupt_event(struct slot *p_slot, u32 event_type)
{
struct event_info *info;
info = kmalloc(sizeof(*info), GFP_ATOMIC);
- if (!info)
- return -ENOMEM;
+ if (!info) {
+ ctrl_err(p_slot->ctrl, "dropped event %d (ENOMEM)\n", event_type);
+ return;
+ }
+ INIT_WORK(&info->work, interrupt_event_handler);
info->event_type = event_type;
info->p_slot = p_slot;
- INIT_WORK(&info->work, interrupt_event_handler);
-
queue_work(p_slot->wq, &info->work);
-
- return 0;
}
u8 pciehp_handle_attention_button(struct slot *p_slot)
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/4] PCI: pciehp: Rename queue_interrupt_event() to pciehp_queue_interrupt_event()
2015-06-18 16:12 [PATCH 0/4] PCI: pciehp cleanup Bjorn Helgaas
2015-06-18 16:12 ` [PATCH 1/4] PCI: pciehp: Clean up debug logging Bjorn Helgaas
2015-06-18 16:12 ` [PATCH 2/4] PCI: pciehp: Make queue_interrupt_event() void Bjorn Helgaas
@ 2015-06-18 16:12 ` Bjorn Helgaas
2015-06-18 17:59 ` Rajat Jain
2015-06-18 16:12 ` [PATCH 4/4] PCI: pciehp: Inline the "handle event" functions into the ISR Bjorn Helgaas
2015-06-18 21:12 ` [PATCH 0/4] PCI: pciehp cleanup Bjorn Helgaas
4 siblings, 1 reply; 15+ messages in thread
From: Bjorn Helgaas @ 2015-06-18 16:12 UTC (permalink / raw)
To: linux-pci; +Cc: Rajat Jain, Yinghai Lu, linux-kernel
Rename queue_interrupt_event() to pciehp_queue_interrupt_event() so we can
make it extern and call it from pcie_isr().
No functional change.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/hotplug/pciehp_ctrl.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 1086041..7ed37dc 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -37,7 +37,7 @@
static void interrupt_event_handler(struct work_struct *work);
-static void queue_interrupt_event(struct slot *p_slot, u32 event_type)
+static void pciehp_queue_interrupt_event(struct slot *p_slot, u32 event_type)
{
struct event_info *info;
@@ -64,7 +64,7 @@ u8 pciehp_handle_attention_button(struct slot *p_slot)
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);
+ pciehp_queue_interrupt_event(p_slot, event_type);
return 0;
}
@@ -90,7 +90,7 @@ u8 pciehp_handle_switch_change(struct slot *p_slot)
event_type = INT_SWITCH_CLOSE;
}
- queue_interrupt_event(p_slot, event_type);
+ pciehp_queue_interrupt_event(p_slot, event_type);
return 1;
}
@@ -120,7 +120,7 @@ u8 pciehp_handle_presence_change(struct slot *p_slot)
event_type = INT_PRESENCE_OFF;
}
- queue_interrupt_event(p_slot, event_type);
+ pciehp_queue_interrupt_event(p_slot, event_type);
return 1;
}
@@ -133,7 +133,7 @@ u8 pciehp_handle_power_fault(struct slot *p_slot)
ctrl_err(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);
- queue_interrupt_event(p_slot, event_type);
+ pciehp_queue_interrupt_event(p_slot, event_type);
return 1;
}
@@ -153,7 +153,7 @@ void pciehp_handle_linkstate_change(struct slot *p_slot)
event_type = INT_LINK_DOWN;
}
- queue_interrupt_event(p_slot, event_type);
+ pciehp_queue_interrupt_event(p_slot, event_type);
}
/* The following routines constitute the bulk of the
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 4/4] PCI: pciehp: Inline the "handle event" functions into the ISR
2015-06-18 16:12 [PATCH 0/4] PCI: pciehp cleanup Bjorn Helgaas
` (2 preceding siblings ...)
2015-06-18 16:12 ` [PATCH 3/4] PCI: pciehp: Rename queue_interrupt_event() to pciehp_queue_interrupt_event() Bjorn Helgaas
@ 2015-06-18 16:12 ` Bjorn Helgaas
2015-06-18 18:02 ` Rajat Jain
2015-06-18 18:28 ` Yinghai Lu
2015-06-18 21:12 ` [PATCH 0/4] PCI: pciehp cleanup Bjorn Helgaas
4 siblings, 2 replies; 15+ messages in thread
From: Bjorn Helgaas @ 2015-06-18 16:12 UTC (permalink / raw)
To: linux-pci; +Cc: Rajat Jain, Yinghai Lu, linux-kernel
The pciehp_handle_*() functions (pciehp_handle_attention_button(), etc.)
only contain a line or two of useful code, so it's clumsy to put
them in separate functions. All they so is add an event to a work queue,
and it's clearer to see that directly in the ISR.
Inline them directly into pcie_isr(). No functional change.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/hotplug/pciehp.h | 6 --
drivers/pci/hotplug/pciehp_ctrl.c | 105 -------------------------------------
drivers/pci/hotplug/pciehp_hpc.c | 39 +++++++++++---
3 files changed, 32 insertions(+), 118 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index ce4d12c..57cd132 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -132,11 +132,7 @@ struct controller {
int pciehp_sysfs_enable_slot(struct slot *slot);
int pciehp_sysfs_disable_slot(struct slot *slot);
-u8 pciehp_handle_attention_button(struct slot *p_slot);
-u8 pciehp_handle_switch_change(struct slot *p_slot);
-u8 pciehp_handle_presence_change(struct slot *p_slot);
-u8 pciehp_handle_power_fault(struct slot *p_slot);
-void pciehp_handle_linkstate_change(struct slot *p_slot);
+void pciehp_queue_interrupt_event(struct slot *slot, u32 event_type);
int pciehp_configure_device(struct slot *p_slot);
int pciehp_unconfigure_device(struct slot *p_slot);
void pciehp_queue_pushbutton_work(struct work_struct *work);
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 7ed37dc..f379612 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -37,7 +37,7 @@
static void interrupt_event_handler(struct work_struct *work);
-static void pciehp_queue_interrupt_event(struct slot *p_slot, u32 event_type)
+void pciehp_queue_interrupt_event(struct slot *p_slot, u32 event_type)
{
struct event_info *info;
@@ -53,109 +53,6 @@ static void pciehp_queue_interrupt_event(struct slot *p_slot, u32 event_type)
queue_work(p_slot->wq, &info->work);
}
-u8 pciehp_handle_attention_button(struct slot *p_slot)
-{
- u32 event_type;
- struct controller *ctrl = p_slot->ctrl;
-
- /*
- * Button pressed - See if need to TAKE ACTION!!!
- */
- ctrl_info(ctrl, "Button pressed on Slot(%s)\n", slot_name(p_slot));
- event_type = INT_BUTTON_PRESS;
-
- pciehp_queue_interrupt_event(p_slot, event_type);
-
- return 0;
-}
-
-u8 pciehp_handle_switch_change(struct slot *p_slot)
-{
- u8 getstatus;
- u32 event_type;
- struct controller *ctrl = p_slot->ctrl;
-
- pciehp_get_latch_status(p_slot, &getstatus);
- if (getstatus) {
- /*
- * Switch opened
- */
- 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", slot_name(p_slot));
- event_type = INT_SWITCH_CLOSE;
- }
-
- pciehp_queue_interrupt_event(p_slot, event_type);
-
- return 1;
-}
-
-u8 pciehp_handle_presence_change(struct slot *p_slot)
-{
- u32 event_type;
- u8 presence_save;
- struct controller *ctrl = p_slot->ctrl;
-
- /* Switch is open, assume a presence change
- * Save the presence state
- */
- pciehp_get_adapter_status(p_slot, &presence_save);
- if (presence_save) {
- /*
- * Card Present
- */
- 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",
- slot_name(p_slot));
- event_type = INT_PRESENCE_OFF;
- }
-
- pciehp_queue_interrupt_event(p_slot, event_type);
-
- return 1;
-}
-
-u8 pciehp_handle_power_fault(struct slot *p_slot)
-{
- u32 event_type;
- struct controller *ctrl = p_slot->ctrl;
-
- ctrl_err(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);
- pciehp_queue_interrupt_event(p_slot, event_type);
-
- return 1;
-}
-
-void pciehp_handle_linkstate_change(struct slot *p_slot)
-{
- u32 event_type;
- struct controller *ctrl = p_slot->ctrl;
-
- if (pciehp_check_link_active(ctrl)) {
- ctrl_info(ctrl, "slot(%s): Link Up event\n",
- slot_name(p_slot));
- event_type = INT_LINK_UP;
- } else {
- ctrl_info(ctrl, "slot(%s): Link Down event\n",
- slot_name(p_slot));
- event_type = INT_LINK_DOWN;
- }
-
- pciehp_queue_interrupt_event(p_slot, event_type);
-}
-
/* The following routines constitute the bulk of the
hotplug controller logic
*/
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index e9daaa3..2913f7e 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -535,6 +535,8 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
struct pci_dev *dev;
struct slot *slot = ctrl->slot;
u16 detected, intr_loc;
+ u8 open, present;
+ bool link;
/*
* In order to guarantee that all interrupt events are
@@ -580,25 +582,44 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
return IRQ_HANDLED;
/* Check MRL Sensor Changed */
- if (intr_loc & PCI_EXP_SLTSTA_MRLSC)
- pciehp_handle_switch_change(slot);
+ if (intr_loc & PCI_EXP_SLTSTA_MRLSC) {
+ pciehp_get_latch_status(slot, &open);
+ ctrl_info(ctrl, "Latch %s on Slot(%s)\n",
+ open ? "open" : "close", slot_name(slot));
+ pciehp_queue_interrupt_event(slot, open ? INT_SWITCH_OPEN :
+ INT_SWITCH_CLOSE);
+ }
/* Check Attention Button Pressed */
- if (intr_loc & PCI_EXP_SLTSTA_ABP)
- pciehp_handle_attention_button(slot);
+ if (intr_loc & PCI_EXP_SLTSTA_ABP) {
+ ctrl_info(ctrl, "Button pressed on Slot(%s)\n",
+ slot_name(slot));
+ pciehp_queue_interrupt_event(slot, INT_BUTTON_PRESS);
+ }
/* Check Presence Detect Changed */
- if (intr_loc & PCI_EXP_SLTSTA_PDC)
- pciehp_handle_presence_change(slot);
+ if (intr_loc & PCI_EXP_SLTSTA_PDC) {
+ pciehp_get_adapter_status(slot, &present);
+ ctrl_info(ctrl, "Card %spresent on Slot(%s)\n",
+ present ? "" : "not ", slot_name(slot));
+ pciehp_queue_interrupt_event(slot, present ? INT_PRESENCE_ON :
+ INT_PRESENCE_OFF);
+ }
/* Check Power Fault Detected */
if ((intr_loc & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
ctrl->power_fault_detected = 1;
- pciehp_handle_power_fault(slot);
+ ctrl_err(ctrl, "Power fault on slot %s\n", slot_name(slot));
+ pciehp_queue_interrupt_event(slot, INT_POWER_FAULT);
}
- if (intr_loc & PCI_EXP_SLTSTA_DLLSC)
- pciehp_handle_linkstate_change(slot);
+ if (intr_loc & PCI_EXP_SLTSTA_DLLSC) {
+ link = pciehp_check_link_active(ctrl);
+ ctrl_info(ctrl, "slot(%s): Link %s event\n",
+ slot_name(slot), link ? "Up" : "Down");
+ pciehp_queue_interrupt_event(slot, link ? INT_LINK_UP :
+ INT_LINK_DOWN);
+ }
return IRQ_HANDLED;
}
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] PCI: pciehp: Clean up debug logging
2015-06-18 16:12 ` [PATCH 1/4] PCI: pciehp: Clean up debug logging Bjorn Helgaas
@ 2015-06-18 17:27 ` Rajat Jain
2015-06-18 18:01 ` Bjorn Helgaas
0 siblings, 1 reply; 15+ messages in thread
From: Rajat Jain @ 2015-06-18 17:27 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Yinghai Lu, linux-kernel
Hi,
On Thu, Jun 18, 2015 at 9:12 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> The pciehp debug logging is overly verbose and often redundant. Almost all
> of the information printed by dbg_ctrl() is also printed by the normal PCI
> core enumeration code and by pcie_init().
>
> Remove the redundant debug info.
>
> When claiming a pciehp bridge, we print the slot characteristics, e.g.,
>
> Slot #6 AttnBtn- AttnInd- PwrInd- PwrCtrl- MRL- Interlock- NoCompl+ LLActRep+
>
> Add the Hot-Plug Capable and Hot-Plug Surprise bits to this information,
If the slot is not hotplug capable. then pciehp wouldn't claim it in
the first place.
So printing of "hotplug capable" may really not be needed..
Thanks,
Rajat
> and print it all in the same order as lspci does.
>
> No functional change except the message text changes.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> drivers/pci/hotplug/pciehp_core.c | 39 ++-------------------------
> drivers/pci/hotplug/pciehp_ctrl.c | 38 +++++---------------------
> drivers/pci/hotplug/pciehp_hpc.c | 54 +++++++------------------------------
> 3 files changed, 20 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 4597f6b..612b21a 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -77,11 +77,6 @@ static int reset_slot (struct hotplug_slot *slot, int probe);
> */
> 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(hotplug_slot));
> -
> kfree(hotplug_slot->ops);
> kfree(hotplug_slot->info);
> kfree(hotplug_slot);
> @@ -129,14 +124,10 @@ static int init_slot(struct controller *ctrl)
> slot->hotplug_slot = hotplug;
> snprintf(name, SLOT_NAME_SIZE, "%u", PSN(ctrl));
>
> - ctrl_dbg(ctrl, "Registering domain:bus:dev=%04x:%02x:00 sun=%x\n",
> - pci_domain_nr(ctrl->pcie->port->subordinate),
> - ctrl->pcie->port->subordinate->number, PSN(ctrl));
> retval = pci_hp_register(hotplug,
> ctrl->pcie->port->subordinate, 0, name);
> if (retval)
> - ctrl_err(ctrl,
> - "pci_hp_register failed with error %d\n", retval);
> + ctrl_err(ctrl, "pci_hp_register failed: error %d\n", retval);
> out:
> if (retval) {
> kfree(ops);
> @@ -158,9 +149,6 @@ 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__, slot_name(slot));
> -
> pciehp_set_attention_status(slot, status);
> return 0;
> }
> @@ -170,9 +158,6 @@ 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__, slot_name(slot));
> -
> return pciehp_sysfs_enable_slot(slot);
> }
>
> @@ -181,9 +166,6 @@ 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__, slot_name(slot));
> -
> return pciehp_sysfs_disable_slot(slot);
> }
>
> @@ -191,9 +173,6 @@ static int get_power_status(struct hotplug_slot *hotplug_slot, u8 *value)
> {
> struct slot *slot = hotplug_slot->private;
>
> - ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
> - __func__, slot_name(slot));
> -
> pciehp_get_power_status(slot, value);
> return 0;
> }
> @@ -202,9 +181,6 @@ static int get_attention_status(struct hotplug_slot *hotplug_slot, u8 *value)
> {
> struct slot *slot = hotplug_slot->private;
>
> - ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
> - __func__, slot_name(slot));
> -
> pciehp_get_attention_status(slot, value);
> return 0;
> }
> @@ -213,9 +189,6 @@ static int get_latch_status(struct hotplug_slot *hotplug_slot, u8 *value)
> {
> struct slot *slot = hotplug_slot->private;
>
> - ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
> - __func__, slot_name(slot));
> -
> pciehp_get_latch_status(slot, value);
> return 0;
> }
> @@ -224,9 +197,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
> {
> struct slot *slot = hotplug_slot->private;
>
> - ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
> - __func__, slot_name(slot));
> -
> pciehp_get_adapter_status(slot, value);
> return 0;
> }
> @@ -235,9 +205,6 @@ static int reset_slot(struct hotplug_slot *hotplug_slot, int probe)
> {
> struct slot *slot = hotplug_slot->private;
>
> - ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
> - __func__, slot_name(slot));
> -
> return pciehp_reset_slot(slot, probe);
> }
>
> @@ -272,14 +239,14 @@ static int pciehp_probe(struct pcie_device *dev)
> if (rc == -EBUSY)
> ctrl_warn(ctrl, "Slot already registered by another hotplug driver\n");
> else
> - ctrl_err(ctrl, "Slot initialization failed\n");
> + ctrl_err(ctrl, "Slot initialization failed (%d)\n", rc);
> goto err_out_release_ctlr;
> }
>
> /* Enable events after we have setup the data structures */
> rc = pcie_init_notification(ctrl);
> if (rc) {
> - ctrl_err(ctrl, "Notification initialization failed\n");
> + ctrl_err(ctrl, "Notification initialization failed (%d)\n", rc);
> goto err_out_free_ctrl_slot;
> }
>
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index f052e95..4bdaf62 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -59,9 +59,6 @@ u8 pciehp_handle_attention_button(struct slot *p_slot)
> u32 event_type;
> struct controller *ctrl = p_slot->ctrl;
>
> - /* Attention Button Change */
> - ctrl_dbg(ctrl, "Attention button interrupt received\n");
> -
> /*
> * Button pressed - See if need to TAKE ACTION!!!
> */
> @@ -79,9 +76,6 @@ u8 pciehp_handle_switch_change(struct slot *p_slot)
> u32 event_type;
> struct controller *ctrl = p_slot->ctrl;
>
> - /* Switch Change */
> - ctrl_dbg(ctrl, "Switch interrupt received\n");
> -
> pciehp_get_latch_status(p_slot, &getstatus);
> if (getstatus) {
> /*
> @@ -108,9 +102,6 @@ u8 pciehp_handle_presence_change(struct slot *p_slot)
> u8 presence_save;
> struct controller *ctrl = p_slot->ctrl;
>
> - /* Presence Change */
> - ctrl_dbg(ctrl, "Presence/Notify input change\n");
> -
> /* Switch is open, assume a presence change
> * Save the presence state
> */
> @@ -140,8 +131,6 @@ u8 pciehp_handle_power_fault(struct slot *p_slot)
> u32 event_type;
> struct controller *ctrl = p_slot->ctrl;
>
> - /* power fault */
> - ctrl_dbg(ctrl, "Power fault interrupt received\n");
> ctrl_err(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);
> @@ -155,9 +144,6 @@ void pciehp_handle_linkstate_change(struct slot *p_slot)
> u32 event_type;
> struct controller *ctrl = p_slot->ctrl;
>
> - /* Link Status Change */
> - ctrl_dbg(ctrl, "Data Link Layer State change\n");
> -
> if (pciehp_check_link_active(ctrl)) {
> ctrl_info(ctrl, "slot(%s): Link Up event\n",
> slot_name(p_slot));
> @@ -298,10 +284,6 @@ static void pciehp_power_thread(struct work_struct *work)
>
> switch (info->req) {
> case DISABLE_REQ:
> - ctrl_dbg(p_slot->ctrl,
> - "Disabling domain:bus:device=%04x:%02x:00\n",
> - pci_domain_nr(p_slot->ctrl->pcie->port->subordinate),
> - p_slot->ctrl->pcie->port->subordinate->number);
> mutex_lock(&p_slot->hotplug_lock);
> pciehp_disable_slot(p_slot);
> mutex_unlock(&p_slot->hotplug_lock);
> @@ -310,10 +292,6 @@ static void pciehp_power_thread(struct work_struct *work)
> mutex_unlock(&p_slot->lock);
> break;
> case ENABLE_REQ:
> - ctrl_dbg(p_slot->ctrl,
> - "Enabling domain:bus:device=%04x:%02x:00\n",
> - pci_domain_nr(p_slot->ctrl->pcie->port->subordinate),
> - p_slot->ctrl->pcie->port->subordinate->number);
> mutex_lock(&p_slot->hotplug_lock);
> ret = pciehp_enable_slot(p_slot);
> mutex_unlock(&p_slot->hotplug_lock);
> @@ -416,7 +394,7 @@ static void handle_button_press_event(struct slot *p_slot)
> ctrl_info(ctrl, "Button ignore on Slot(%s)\n", slot_name(p_slot));
> break;
> default:
> - ctrl_warn(ctrl, "Not a valid state\n");
> + ctrl_warn(ctrl, "ignoring invalid state %#x\n", p_slot->state);
> break;
> }
> }
> @@ -507,8 +485,8 @@ static void handle_link_event(struct slot *p_slot, u32 event)
> }
> break;
> default:
> - ctrl_err(ctrl, "Not a valid state on slot(%s)\n",
> - slot_name(p_slot));
> + ctrl_err(ctrl, "ignoring invalid state %#x on slot(%s)\n",
> + p_slot->state, slot_name(p_slot));
> kfree(info);
> break;
> }
> @@ -532,7 +510,6 @@ static void interrupt_event_handler(struct work_struct *work)
> pciehp_green_led_off(p_slot);
> break;
> case INT_PRESENCE_ON:
> - ctrl_dbg(ctrl, "Surprise Insertion\n");
> handle_surprise_event(p_slot);
> break;
> case INT_PRESENCE_OFF:
> @@ -540,7 +517,6 @@ static void interrupt_event_handler(struct work_struct *work)
> * Regardless of surprise capability, we need to
> * definitely remove a card that has been pulled out!
> */
> - ctrl_dbg(ctrl, "Surprise Removal\n");
> handle_surprise_event(p_slot);
> break;
> case INT_LINK_UP:
> @@ -647,8 +623,8 @@ int pciehp_sysfs_enable_slot(struct slot *p_slot)
> slot_name(p_slot));
> break;
> default:
> - ctrl_err(ctrl, "Not a valid state on slot %s\n",
> - slot_name(p_slot));
> + ctrl_err(ctrl, "invalid state %#x on slot %s\n",
> + p_slot->state, slot_name(p_slot));
> break;
> }
> mutex_unlock(&p_slot->lock);
> @@ -682,8 +658,8 @@ int pciehp_sysfs_disable_slot(struct slot *p_slot)
> slot_name(p_slot));
> break;
> default:
> - ctrl_err(ctrl, "Not a valid state on slot %s\n",
> - slot_name(p_slot));
> + ctrl_err(ctrl, "invalid state %#x on slot %s\n",
> + p_slot->state, 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 6d68688..e9daaa3 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -312,7 +312,8 @@ int pciehp_check_link_status(struct controller *ctrl)
> ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
> if ((lnk_status & PCI_EXP_LNKSTA_LT) ||
> !(lnk_status & PCI_EXP_LNKSTA_NLW)) {
> - ctrl_err(ctrl, "Link Training Error occurs\n");
> + ctrl_err(ctrl, "link training error: status %#06x\n",
> + lnk_status);
> return -1;
> }
>
> @@ -556,7 +557,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
> intr_loc);
> } while (detected);
>
> - ctrl_dbg(ctrl, "%s: intr_loc %x\n", __func__, intr_loc);
> + ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", intr_loc);
>
> /* Check Command Complete Interrupt Pending */
> if (intr_loc & PCI_EXP_SLTSTA_CC) {
> @@ -748,48 +749,13 @@ static void pcie_cleanup_slot(struct controller *ctrl)
>
> static inline void dbg_ctrl(struct controller *ctrl)
> {
> - int i;
> - u16 reg16;
> struct pci_dev *pdev = ctrl->pcie->port;
> + u16 reg16;
>
> if (!pciehp_debug)
> return;
>
> - ctrl_info(ctrl, "Hotplug Controller:\n");
> - ctrl_info(ctrl, " Seg/Bus/Dev/Func/IRQ : %s IRQ %d\n",
> - pci_name(pdev), pdev->irq);
> - ctrl_info(ctrl, " Vendor ID : 0x%04x\n", pdev->vendor);
> - ctrl_info(ctrl, " Device ID : 0x%04x\n", pdev->device);
> - ctrl_info(ctrl, " Subsystem ID : 0x%04x\n",
> - pdev->subsystem_device);
> - ctrl_info(ctrl, " Subsystem Vendor ID : 0x%04x\n",
> - pdev->subsystem_vendor);
> - ctrl_info(ctrl, " PCIe Cap offset : 0x%02x\n",
> - pci_pcie_cap(pdev));
> - for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> - if (!pci_resource_len(pdev, i))
> - continue;
> - ctrl_info(ctrl, " PCI resource [%d] : %pR\n",
> - i, &pdev->resource[i]);
> - }
> ctrl_info(ctrl, "Slot Capabilities : 0x%08x\n", ctrl->slot_cap);
> - ctrl_info(ctrl, " Physical Slot Number : %d\n", PSN(ctrl));
> - ctrl_info(ctrl, " Attention Button : %3s\n",
> - ATTN_BUTTN(ctrl) ? "yes" : "no");
> - ctrl_info(ctrl, " Power Controller : %3s\n",
> - POWER_CTRL(ctrl) ? "yes" : "no");
> - ctrl_info(ctrl, " MRL Sensor : %3s\n",
> - MRL_SENS(ctrl) ? "yes" : "no");
> - ctrl_info(ctrl, " Attention Indicator : %3s\n",
> - ATTN_LED(ctrl) ? "yes" : "no");
> - ctrl_info(ctrl, " Power Indicator : %3s\n",
> - PWR_LED(ctrl) ? "yes" : "no");
> - ctrl_info(ctrl, " Hot-Plug Surprise : %3s\n",
> - HP_SUPR_RM(ctrl) ? "yes" : "no");
> - ctrl_info(ctrl, " EMI Present : %3s\n",
> - EMI(ctrl) ? "yes" : "no");
> - ctrl_info(ctrl, " Command Completed : %3s\n",
> - NO_CMD_CMPL(ctrl) ? "no" : "yes");
> pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, ®16);
> ctrl_info(ctrl, "Slot Status : 0x%04x\n", reg16);
> pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, ®16);
> @@ -818,10 +784,8 @@ struct controller *pcie_init(struct pcie_device *dev)
>
> /* Check if Data Link Layer Link Active Reporting is implemented */
> pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &link_cap);
> - if (link_cap & PCI_EXP_LNKCAP_DLLLARC) {
> - ctrl_dbg(ctrl, "Link Active Reporting supported\n");
> + if (link_cap & PCI_EXP_LNKCAP_DLLLARC)
> ctrl->link_active_reporting = 1;
> - }
>
> /* Clear all remaining event bits in Slot Status register */
> pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
> @@ -829,13 +793,15 @@ struct controller *pcie_init(struct pcie_device *dev)
> PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
> PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
>
> - ctrl_info(ctrl, "Slot #%d AttnBtn%c AttnInd%c PwrInd%c PwrCtrl%c MRL%c Interlock%c NoCompl%c LLActRep%c\n",
> + ctrl_info(ctrl, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c LLActRep%c\n",
> (slot_cap & PCI_EXP_SLTCAP_PSN) >> 19,
> FLAG(slot_cap, PCI_EXP_SLTCAP_ABP),
> - FLAG(slot_cap, PCI_EXP_SLTCAP_AIP),
> - FLAG(slot_cap, PCI_EXP_SLTCAP_PIP),
> FLAG(slot_cap, PCI_EXP_SLTCAP_PCP),
> FLAG(slot_cap, PCI_EXP_SLTCAP_MRLSP),
> + FLAG(slot_cap, PCI_EXP_SLTCAP_AIP),
> + FLAG(slot_cap, PCI_EXP_SLTCAP_PIP),
> + FLAG(slot_cap, PCI_EXP_SLTCAP_HPC),
> + FLAG(slot_cap, PCI_EXP_SLTCAP_HPS),
> FLAG(slot_cap, PCI_EXP_SLTCAP_EIP),
> FLAG(slot_cap, PCI_EXP_SLTCAP_NCCS),
> FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC));
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] PCI: pciehp: Rename queue_interrupt_event() to pciehp_queue_interrupt_event()
2015-06-18 16:12 ` [PATCH 3/4] PCI: pciehp: Rename queue_interrupt_event() to pciehp_queue_interrupt_event() Bjorn Helgaas
@ 2015-06-18 17:59 ` Rajat Jain
0 siblings, 0 replies; 15+ messages in thread
From: Rajat Jain @ 2015-06-18 17:59 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Yinghai Lu, linux-kernel
On Thu, Jun 18, 2015 at 9:12 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> Rename queue_interrupt_event() to pciehp_queue_interrupt_event() so we can
> make it extern and call it from pcie_isr().
>
> No functional change.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Ok to add:
Reviewed-by: Rajat Jain <rajatja@google.com>
> ---
> drivers/pci/hotplug/pciehp_ctrl.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 1086041..7ed37dc 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -37,7 +37,7 @@
>
> static void interrupt_event_handler(struct work_struct *work);
>
> -static void queue_interrupt_event(struct slot *p_slot, u32 event_type)
> +static void pciehp_queue_interrupt_event(struct slot *p_slot, u32 event_type)
> {
> struct event_info *info;
>
> @@ -64,7 +64,7 @@ u8 pciehp_handle_attention_button(struct slot *p_slot)
> 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);
> + pciehp_queue_interrupt_event(p_slot, event_type);
>
> return 0;
> }
> @@ -90,7 +90,7 @@ u8 pciehp_handle_switch_change(struct slot *p_slot)
> event_type = INT_SWITCH_CLOSE;
> }
>
> - queue_interrupt_event(p_slot, event_type);
> + pciehp_queue_interrupt_event(p_slot, event_type);
>
> return 1;
> }
> @@ -120,7 +120,7 @@ u8 pciehp_handle_presence_change(struct slot *p_slot)
> event_type = INT_PRESENCE_OFF;
> }
>
> - queue_interrupt_event(p_slot, event_type);
> + pciehp_queue_interrupt_event(p_slot, event_type);
>
> return 1;
> }
> @@ -133,7 +133,7 @@ u8 pciehp_handle_power_fault(struct slot *p_slot)
> ctrl_err(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);
> - queue_interrupt_event(p_slot, event_type);
> + pciehp_queue_interrupt_event(p_slot, event_type);
>
> return 1;
> }
> @@ -153,7 +153,7 @@ void pciehp_handle_linkstate_change(struct slot *p_slot)
> event_type = INT_LINK_DOWN;
> }
>
> - queue_interrupt_event(p_slot, event_type);
> + pciehp_queue_interrupt_event(p_slot, event_type);
> }
>
> /* The following routines constitute the bulk of the
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] PCI: pciehp: Clean up debug logging
2015-06-18 17:27 ` Rajat Jain
@ 2015-06-18 18:01 ` Bjorn Helgaas
2015-06-18 18:08 ` Rajat Jain
0 siblings, 1 reply; 15+ messages in thread
From: Bjorn Helgaas @ 2015-06-18 18:01 UTC (permalink / raw)
To: Rajat Jain; +Cc: linux-pci, Yinghai Lu, linux-kernel
On Thu, Jun 18, 2015 at 12:27 PM, Rajat Jain <rajatja@google.com> wrote:
> Hi,
>
> On Thu, Jun 18, 2015 at 9:12 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> The pciehp debug logging is overly verbose and often redundant. Almost all
>> of the information printed by dbg_ctrl() is also printed by the normal PCI
>> core enumeration code and by pcie_init().
>>
>> Remove the redundant debug info.
>>
>> When claiming a pciehp bridge, we print the slot characteristics, e.g.,
>>
>> Slot #6 AttnBtn- AttnInd- PwrInd- PwrCtrl- MRL- Interlock- NoCompl+ LLActRep+
>>
>> Add the Hot-Plug Capable and Hot-Plug Surprise bits to this information,
>
> If the slot is not hotplug capable. then pciehp wouldn't claim it in
> the first place.
>
> So printing of "hotplug capable" may really not be needed..
Yes, I did think about that, and you're right that it probably isn't
needed. But the criteria for claiming a slot and deciding whether
acpiphp or pciehp should manage it are not 100% clear yet, so I
figured it wouldn't hurt to be a bit more transparent.
Bjorn
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] PCI: pciehp: Inline the "handle event" functions into the ISR
2015-06-18 16:12 ` [PATCH 4/4] PCI: pciehp: Inline the "handle event" functions into the ISR Bjorn Helgaas
@ 2015-06-18 18:02 ` Rajat Jain
2015-06-18 18:28 ` Yinghai Lu
1 sibling, 0 replies; 15+ messages in thread
From: Rajat Jain @ 2015-06-18 18:02 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Yinghai Lu, linux-kernel
Ok to add:
Reviewed-by: Rajat Jain <rajatja@google.com>
On Thu, Jun 18, 2015 at 9:12 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> The pciehp_handle_*() functions (pciehp_handle_attention_button(), etc.)
> only contain a line or two of useful code, so it's clumsy to put
> them in separate functions. All they so is add an event to a work queue,
> and it's clearer to see that directly in the ISR.
>
> Inline them directly into pcie_isr(). No functional change.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> drivers/pci/hotplug/pciehp.h | 6 --
> drivers/pci/hotplug/pciehp_ctrl.c | 105 -------------------------------------
> drivers/pci/hotplug/pciehp_hpc.c | 39 +++++++++++---
> 3 files changed, 32 insertions(+), 118 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index ce4d12c..57cd132 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -132,11 +132,7 @@ struct controller {
>
> int pciehp_sysfs_enable_slot(struct slot *slot);
> int pciehp_sysfs_disable_slot(struct slot *slot);
> -u8 pciehp_handle_attention_button(struct slot *p_slot);
> -u8 pciehp_handle_switch_change(struct slot *p_slot);
> -u8 pciehp_handle_presence_change(struct slot *p_slot);
> -u8 pciehp_handle_power_fault(struct slot *p_slot);
> -void pciehp_handle_linkstate_change(struct slot *p_slot);
> +void pciehp_queue_interrupt_event(struct slot *slot, u32 event_type);
> int pciehp_configure_device(struct slot *p_slot);
> int pciehp_unconfigure_device(struct slot *p_slot);
> void pciehp_queue_pushbutton_work(struct work_struct *work);
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 7ed37dc..f379612 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -37,7 +37,7 @@
>
> static void interrupt_event_handler(struct work_struct *work);
>
> -static void pciehp_queue_interrupt_event(struct slot *p_slot, u32 event_type)
> +void pciehp_queue_interrupt_event(struct slot *p_slot, u32 event_type)
> {
> struct event_info *info;
>
> @@ -53,109 +53,6 @@ static void pciehp_queue_interrupt_event(struct slot *p_slot, u32 event_type)
> queue_work(p_slot->wq, &info->work);
> }
>
> -u8 pciehp_handle_attention_button(struct slot *p_slot)
> -{
> - u32 event_type;
> - struct controller *ctrl = p_slot->ctrl;
> -
> - /*
> - * Button pressed - See if need to TAKE ACTION!!!
> - */
> - ctrl_info(ctrl, "Button pressed on Slot(%s)\n", slot_name(p_slot));
> - event_type = INT_BUTTON_PRESS;
> -
> - pciehp_queue_interrupt_event(p_slot, event_type);
> -
> - return 0;
> -}
> -
> -u8 pciehp_handle_switch_change(struct slot *p_slot)
> -{
> - u8 getstatus;
> - u32 event_type;
> - struct controller *ctrl = p_slot->ctrl;
> -
> - pciehp_get_latch_status(p_slot, &getstatus);
> - if (getstatus) {
> - /*
> - * Switch opened
> - */
> - 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", slot_name(p_slot));
> - event_type = INT_SWITCH_CLOSE;
> - }
> -
> - pciehp_queue_interrupt_event(p_slot, event_type);
> -
> - return 1;
> -}
> -
> -u8 pciehp_handle_presence_change(struct slot *p_slot)
> -{
> - u32 event_type;
> - u8 presence_save;
> - struct controller *ctrl = p_slot->ctrl;
> -
> - /* Switch is open, assume a presence change
> - * Save the presence state
> - */
> - pciehp_get_adapter_status(p_slot, &presence_save);
> - if (presence_save) {
> - /*
> - * Card Present
> - */
> - 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",
> - slot_name(p_slot));
> - event_type = INT_PRESENCE_OFF;
> - }
> -
> - pciehp_queue_interrupt_event(p_slot, event_type);
> -
> - return 1;
> -}
> -
> -u8 pciehp_handle_power_fault(struct slot *p_slot)
> -{
> - u32 event_type;
> - struct controller *ctrl = p_slot->ctrl;
> -
> - ctrl_err(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);
> - pciehp_queue_interrupt_event(p_slot, event_type);
> -
> - return 1;
> -}
> -
> -void pciehp_handle_linkstate_change(struct slot *p_slot)
> -{
> - u32 event_type;
> - struct controller *ctrl = p_slot->ctrl;
> -
> - if (pciehp_check_link_active(ctrl)) {
> - ctrl_info(ctrl, "slot(%s): Link Up event\n",
> - slot_name(p_slot));
> - event_type = INT_LINK_UP;
> - } else {
> - ctrl_info(ctrl, "slot(%s): Link Down event\n",
> - slot_name(p_slot));
> - event_type = INT_LINK_DOWN;
> - }
> -
> - pciehp_queue_interrupt_event(p_slot, event_type);
> -}
> -
> /* The following routines constitute the bulk of the
> hotplug controller logic
> */
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index e9daaa3..2913f7e 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -535,6 +535,8 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
> struct pci_dev *dev;
> struct slot *slot = ctrl->slot;
> u16 detected, intr_loc;
> + u8 open, present;
> + bool link;
>
> /*
> * In order to guarantee that all interrupt events are
> @@ -580,25 +582,44 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
> return IRQ_HANDLED;
>
> /* Check MRL Sensor Changed */
> - if (intr_loc & PCI_EXP_SLTSTA_MRLSC)
> - pciehp_handle_switch_change(slot);
> + if (intr_loc & PCI_EXP_SLTSTA_MRLSC) {
> + pciehp_get_latch_status(slot, &open);
> + ctrl_info(ctrl, "Latch %s on Slot(%s)\n",
> + open ? "open" : "close", slot_name(slot));
> + pciehp_queue_interrupt_event(slot, open ? INT_SWITCH_OPEN :
> + INT_SWITCH_CLOSE);
> + }
>
> /* Check Attention Button Pressed */
> - if (intr_loc & PCI_EXP_SLTSTA_ABP)
> - pciehp_handle_attention_button(slot);
> + if (intr_loc & PCI_EXP_SLTSTA_ABP) {
> + ctrl_info(ctrl, "Button pressed on Slot(%s)\n",
> + slot_name(slot));
> + pciehp_queue_interrupt_event(slot, INT_BUTTON_PRESS);
> + }
>
> /* Check Presence Detect Changed */
> - if (intr_loc & PCI_EXP_SLTSTA_PDC)
> - pciehp_handle_presence_change(slot);
> + if (intr_loc & PCI_EXP_SLTSTA_PDC) {
> + pciehp_get_adapter_status(slot, &present);
> + ctrl_info(ctrl, "Card %spresent on Slot(%s)\n",
> + present ? "" : "not ", slot_name(slot));
> + pciehp_queue_interrupt_event(slot, present ? INT_PRESENCE_ON :
> + INT_PRESENCE_OFF);
> + }
>
> /* Check Power Fault Detected */
> if ((intr_loc & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
> ctrl->power_fault_detected = 1;
> - pciehp_handle_power_fault(slot);
> + ctrl_err(ctrl, "Power fault on slot %s\n", slot_name(slot));
> + pciehp_queue_interrupt_event(slot, INT_POWER_FAULT);
> }
>
> - if (intr_loc & PCI_EXP_SLTSTA_DLLSC)
> - pciehp_handle_linkstate_change(slot);
> + if (intr_loc & PCI_EXP_SLTSTA_DLLSC) {
> + link = pciehp_check_link_active(ctrl);
> + ctrl_info(ctrl, "slot(%s): Link %s event\n",
> + slot_name(slot), link ? "Up" : "Down");
> + pciehp_queue_interrupt_event(slot, link ? INT_LINK_UP :
> + INT_LINK_DOWN);
> + }
>
> return IRQ_HANDLED;
> }
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] PCI: pciehp: Clean up debug logging
2015-06-18 18:01 ` Bjorn Helgaas
@ 2015-06-18 18:08 ` Rajat Jain
2015-06-18 21:22 ` Bjorn Helgaas
0 siblings, 1 reply; 15+ messages in thread
From: Rajat Jain @ 2015-06-18 18:08 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Yinghai Lu, linux-kernel
On Thu, Jun 18, 2015 at 11:01 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Jun 18, 2015 at 12:27 PM, Rajat Jain <rajatja@google.com> wrote:
>> Hi,
>>
>> On Thu, Jun 18, 2015 at 9:12 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> The pciehp debug logging is overly verbose and often redundant. Almost all
>>> of the information printed by dbg_ctrl() is also printed by the normal PCI
>>> core enumeration code and by pcie_init().
>>>
>>> Remove the redundant debug info.
>>>
>>> When claiming a pciehp bridge, we print the slot characteristics, e.g.,
>>>
>>> Slot #6 AttnBtn- AttnInd- PwrInd- PwrCtrl- MRL- Interlock- NoCompl+ LLActRep+
>>>
>>> Add the Hot-Plug Capable and Hot-Plug Surprise bits to this information,
>>
>> If the slot is not hotplug capable. then pciehp wouldn't claim it in
>> the first place.
>>
>> So printing of "hotplug capable" may really not be needed..
>
> Yes, I did think about that, and you're right that it probably isn't
> needed. But the criteria for claiming a slot and deciding whether
> acpiphp or pciehp should manage it are not 100% clear yet, so I
> figured it wouldn't hurt to be a bit more transparent.
Sounds right.
Reviewed-by : Rajat Jain <rajatja@google.com>
Side note: To clarify when and why the slot was claimed by pciehp or
acpihp, do you think we need some mumbling / logging in
acpi_pci_detect_ejectable() or pciehp_acpi_slot_detection_check()?
>
> Bjorn
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] PCI: pciehp: Inline the "handle event" functions into the ISR
2015-06-18 16:12 ` [PATCH 4/4] PCI: pciehp: Inline the "handle event" functions into the ISR Bjorn Helgaas
2015-06-18 18:02 ` Rajat Jain
@ 2015-06-18 18:28 ` Yinghai Lu
1 sibling, 0 replies; 15+ messages in thread
From: Yinghai Lu @ 2015-06-18 18:28 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Rajat Jain, Linux Kernel Mailing List
On Thu, Jun 18, 2015 at 9:12 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> The pciehp_handle_*() functions (pciehp_handle_attention_button(), etc.)
> only contain a line or two of useful code, so it's clumsy to put
> them in separate functions. All they so is add an event to a work queue,
> and it's clearer to see that directly in the ISR.
Yes. That make the code more readable.
For all 4 patches:
Acked-by: Yinghai Lu <yinghai@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/4] PCI: pciehp cleanup
2015-06-18 16:12 [PATCH 0/4] PCI: pciehp cleanup Bjorn Helgaas
` (3 preceding siblings ...)
2015-06-18 16:12 ` [PATCH 4/4] PCI: pciehp: Inline the "handle event" functions into the ISR Bjorn Helgaas
@ 2015-06-18 21:12 ` Bjorn Helgaas
4 siblings, 0 replies; 15+ messages in thread
From: Bjorn Helgaas @ 2015-06-18 21:12 UTC (permalink / raw)
To: linux-pci; +Cc: Rajat Jain, Yinghai Lu, linux-kernel
On Thu, Jun 18, 2015 at 11:12 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> These patches don't *do* anything except get rid of some overly verbose
> debug logging and a bunch of boilerplate code by inlining trivial things.
>
> Unless I made a mistake, this should not change the way anything works at
> all, although I did change some of the message text.
>
> ---
>
> Bjorn Helgaas (4):
> PCI: pciehp: Clean up debug logging
> PCI: pciehp: Make queue_interrupt_event() void
> PCI: pciehp: Rename queue_interrupt_event() to pciehp_queue_interrupt_event()
> PCI: pciehp: Inline the "handle event" functions into the ISR
>
>
> drivers/pci/hotplug/pciehp.h | 6 -
> drivers/pci/hotplug/pciehp_core.c | 37 +--------
> drivers/pci/hotplug/pciehp_ctrl.c | 154 +++----------------------------------
> drivers/pci/hotplug/pciehp_hpc.c | 89 +++++++++------------
> 4 files changed, 53 insertions(+), 233 deletions(-)
I applied these to pci/hotplug for v4.2 with acks/reviewed-by from
Rajat and Yinghai. Thanks for taking a look!
Bjorn
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] PCI: pciehp: Clean up debug logging
2015-06-18 18:08 ` Rajat Jain
@ 2015-06-18 21:22 ` Bjorn Helgaas
2015-06-18 21:52 ` Rafael J. Wysocki
0 siblings, 1 reply; 15+ messages in thread
From: Bjorn Helgaas @ 2015-06-18 21:22 UTC (permalink / raw)
To: Rajat Jain; +Cc: linux-pci, Yinghai Lu, linux-kernel, Rafael Wysocki
[+cc Rafael]
On Thu, Jun 18, 2015 at 1:08 PM, Rajat Jain <rajatja@google.com> wrote:
> On Thu, Jun 18, 2015 at 11:01 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Thu, Jun 18, 2015 at 12:27 PM, Rajat Jain <rajatja@google.com> wrote:
>>> Hi,
>>>
>>> On Thu, Jun 18, 2015 at 9:12 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>> The pciehp debug logging is overly verbose and often redundant. Almost all
>>>> of the information printed by dbg_ctrl() is also printed by the normal PCI
>>>> core enumeration code and by pcie_init().
>>>>
>>>> Remove the redundant debug info.
>>>>
>>>> When claiming a pciehp bridge, we print the slot characteristics, e.g.,
>>>>
>>>> Slot #6 AttnBtn- AttnInd- PwrInd- PwrCtrl- MRL- Interlock- NoCompl+ LLActRep+
>>>>
>>>> Add the Hot-Plug Capable and Hot-Plug Surprise bits to this information,
>>>
>>> If the slot is not hotplug capable. then pciehp wouldn't claim it in
>>> the first place.
>>>
>>> So printing of "hotplug capable" may really not be needed..
>>
>> Yes, I did think about that, and you're right that it probably isn't
>> needed. But the criteria for claiming a slot and deciding whether
>> acpiphp or pciehp should manage it are not 100% clear yet, so I
>> figured it wouldn't hurt to be a bit more transparent.
>
> Sounds right.
>
> Reviewed-by : Rajat Jain <rajatja@google.com>
>
> Side note: To clarify when and why the slot was claimed by pciehp or
> acpihp, do you think we need some mumbling / logging in
> acpi_pci_detect_ejectable() or pciehp_acpi_slot_detection_check()?
Maybe so (but I haven't added anything).
My intuition is that acpiphp and pciehp are not really symmetric. I
think pciehp should claim PCIe downstream ports (Root Ports and
Downstream Ports) when _OSC has granted us control.
But it doesn't seem like acpiphp should decide whether to claim
certain devices based on whether they have _ADR, _EJ0, _RMV, etc.
Shouldn't it just be integrated with the ACPI core so it can field
notifications from the platform, no matter what methods are present,
and even if pciehp has claimed a bridge in that scope?
Bjorn
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] PCI: pciehp: Clean up debug logging
2015-06-18 21:22 ` Bjorn Helgaas
@ 2015-06-18 21:52 ` Rafael J. Wysocki
2015-06-18 23:17 ` Bjorn Helgaas
0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2015-06-18 21:52 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Rajat Jain, linux-pci, Yinghai Lu, linux-kernel
On Thursday, June 18, 2015 04:22:53 PM Bjorn Helgaas wrote:
> [+cc Rafael]
>
> On Thu, Jun 18, 2015 at 1:08 PM, Rajat Jain <rajatja@google.com> wrote:
> > On Thu, Jun 18, 2015 at 11:01 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> On Thu, Jun 18, 2015 at 12:27 PM, Rajat Jain <rajatja@google.com> wrote:
> >>> Hi,
> >>>
> >>> On Thu, Jun 18, 2015 at 9:12 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >>>> The pciehp debug logging is overly verbose and often redundant. Almost all
> >>>> of the information printed by dbg_ctrl() is also printed by the normal PCI
> >>>> core enumeration code and by pcie_init().
> >>>>
> >>>> Remove the redundant debug info.
> >>>>
> >>>> When claiming a pciehp bridge, we print the slot characteristics, e.g.,
> >>>>
> >>>> Slot #6 AttnBtn- AttnInd- PwrInd- PwrCtrl- MRL- Interlock- NoCompl+ LLActRep+
> >>>>
> >>>> Add the Hot-Plug Capable and Hot-Plug Surprise bits to this information,
> >>>
> >>> If the slot is not hotplug capable. then pciehp wouldn't claim it in
> >>> the first place.
> >>>
> >>> So printing of "hotplug capable" may really not be needed..
> >>
> >> Yes, I did think about that, and you're right that it probably isn't
> >> needed. But the criteria for claiming a slot and deciding whether
> >> acpiphp or pciehp should manage it are not 100% clear yet, so I
> >> figured it wouldn't hurt to be a bit more transparent.
> >
> > Sounds right.
> >
> > Reviewed-by : Rajat Jain <rajatja@google.com>
> >
> > Side note: To clarify when and why the slot was claimed by pciehp or
> > acpihp, do you think we need some mumbling / logging in
> > acpi_pci_detect_ejectable() or pciehp_acpi_slot_detection_check()?
>
> Maybe so (but I haven't added anything).
>
> My intuition is that acpiphp and pciehp are not really symmetric. I
> think pciehp should claim PCIe downstream ports (Root Ports and
> Downstream Ports) when _OSC has granted us control.
>
> But it doesn't seem like acpiphp should decide whether to claim
> certain devices based on whether they have _ADR, _EJ0, _RMV, etc.
But it doesn't do that, does it?
> Shouldn't it just be integrated with the ACPI core so it can field
> notifications from the platform, no matter what methods are present,
> and even if pciehp has claimed a bridge in that scope?
That's how it is implemented today AFAICS.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] PCI: pciehp: Clean up debug logging
2015-06-18 21:52 ` Rafael J. Wysocki
@ 2015-06-18 23:17 ` Bjorn Helgaas
0 siblings, 0 replies; 15+ messages in thread
From: Bjorn Helgaas @ 2015-06-18 23:17 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Rajat Jain, linux-pci, Yinghai Lu, linux-kernel
On Thu, Jun 18, 2015 at 4:52 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, June 18, 2015 04:22:53 PM Bjorn Helgaas wrote:
>> [+cc Rafael]
>>
>> On Thu, Jun 18, 2015 at 1:08 PM, Rajat Jain <rajatja@google.com> wrote:
>> > On Thu, Jun 18, 2015 at 11:01 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> >> On Thu, Jun 18, 2015 at 12:27 PM, Rajat Jain <rajatja@google.com> wrote:
>> >>> Hi,
>> >>>
>> >>> On Thu, Jun 18, 2015 at 9:12 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> >>>> The pciehp debug logging is overly verbose and often redundant. Almost all
>> >>>> of the information printed by dbg_ctrl() is also printed by the normal PCI
>> >>>> core enumeration code and by pcie_init().
>> >>>>
>> >>>> Remove the redundant debug info.
>> >>>>
>> >>>> When claiming a pciehp bridge, we print the slot characteristics, e.g.,
>> >>>>
>> >>>> Slot #6 AttnBtn- AttnInd- PwrInd- PwrCtrl- MRL- Interlock- NoCompl+ LLActRep+
>> >>>>
>> >>>> Add the Hot-Plug Capable and Hot-Plug Surprise bits to this information,
>> >>>
>> >>> If the slot is not hotplug capable. then pciehp wouldn't claim it in
>> >>> the first place.
>> >>>
>> >>> So printing of "hotplug capable" may really not be needed..
>> >>
>> >> Yes, I did think about that, and you're right that it probably isn't
>> >> needed. But the criteria for claiming a slot and deciding whether
>> >> acpiphp or pciehp should manage it are not 100% clear yet, so I
>> >> figured it wouldn't hurt to be a bit more transparent.
>> >
>> > Sounds right.
>> >
>> > Reviewed-by : Rajat Jain <rajatja@google.com>
>> >
>> > Side note: To clarify when and why the slot was claimed by pciehp or
>> > acpihp, do you think we need some mumbling / logging in
>> > acpi_pci_detect_ejectable() or pciehp_acpi_slot_detection_check()?
>>
>> Maybe so (but I haven't added anything).
>>
>> My intuition is that acpiphp and pciehp are not really symmetric. I
>> think pciehp should claim PCIe downstream ports (Root Ports and
>> Downstream Ports) when _OSC has granted us control.
>>
>> But it doesn't seem like acpiphp should decide whether to claim
>> certain devices based on whether they have _ADR, _EJ0, _RMV, etc.
>
> But it doesn't do that, does it?
I guess it's actually *pciehp* that looks at that stuff via this path:
pciehp_probe
pciehp_acpi_slot_detection_check
acpi_pci_detect_ejectable
acpi_walk_namespace(..., check_hotplug, ...)
check_hotplug
pcihp_is_ejectable
acpi_has_method(..., "_ADR")
...
Oh, wait a minute, you removed all that junk with e705c2959b06 ("PCI:
pciehp: Drop pointless ACPI-based "slot detection" check"), so ...
never mind :)
And THANKS AGAIN for getting rid of all that crud!
>> Shouldn't it just be integrated with the ACPI core so it can field
>> notifications from the platform, no matter what methods are present,
>> and even if pciehp has claimed a bridge in that scope?
>
> That's how it is implemented today AFAICS.
Great, that makes a lot of sense to me.
Bjorn
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2015-06-18 23:18 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-18 16:12 [PATCH 0/4] PCI: pciehp cleanup Bjorn Helgaas
2015-06-18 16:12 ` [PATCH 1/4] PCI: pciehp: Clean up debug logging Bjorn Helgaas
2015-06-18 17:27 ` Rajat Jain
2015-06-18 18:01 ` Bjorn Helgaas
2015-06-18 18:08 ` Rajat Jain
2015-06-18 21:22 ` Bjorn Helgaas
2015-06-18 21:52 ` Rafael J. Wysocki
2015-06-18 23:17 ` Bjorn Helgaas
2015-06-18 16:12 ` [PATCH 2/4] PCI: pciehp: Make queue_interrupt_event() void Bjorn Helgaas
2015-06-18 16:12 ` [PATCH 3/4] PCI: pciehp: Rename queue_interrupt_event() to pciehp_queue_interrupt_event() Bjorn Helgaas
2015-06-18 17:59 ` Rajat Jain
2015-06-18 16:12 ` [PATCH 4/4] PCI: pciehp: Inline the "handle event" functions into the ISR Bjorn Helgaas
2015-06-18 18:02 ` Rajat Jain
2015-06-18 18:28 ` Yinghai Lu
2015-06-18 21:12 ` [PATCH 0/4] PCI: pciehp cleanup Bjorn Helgaas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.