| Message ID | 20140211231835.GA21057@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
At Tue, 11 Feb 2014 16:18:35 -0700, Bjorn Helgaas wrote: > > On Tue, Feb 11, 2014 at 06:52:30PM +0000, Rajat Jain wrote: > > Hello Takashi, > > > > > -----Original Message----- > > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > > > owner@vger.kernel.org] On Behalf Of Takashi Iwai > > > Sent: Tuesday, February 11, 2014 3:39 AM > > > To: Bjorn Helgaas > > > Cc: Alex Williamson; linux-pci@vger.kernel.org; linux- > > > kernel@vger.kernel.org > > > Subject: [PATCH] PCI: pciehp: Remove surprise bit checks > > > > > > Currently pciehp driver checks the surprise removal bit and handles > > > the hotplug event only when the bit is set. But there are devices > > > that don't set that bit but yet expect the hotplug working, e.g. the > > > PCI card reader on HP ProBook 445 and 455 laptops appears only when > > > you insert a card, and it needs the hotplug event handling. > > > > > > For fixing this, basically we may ignore the surprise bit and always > > > handle the event. > > > > Some of this has been taken care of in the recent changes to pciehcp (last night). > > > > Please see > > > > https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/pciehp&id=a3eba3887960d00abb912fe3703cd3a058c721b8 > > > > Also, link state hotplug has been added as part of this patch set. Can you please try with Bjorn's latest pci tree? > > > > With that, I think you should not see the issue you are describing, because if the PCIe Link comes up when you insert the card, it shall be added (now) irrespective of whether or not you have the surprise bit set. > > Takashi's patch removed more HP_SUPR_RM() usage than yours, and I think > those additional changes are probably necessary. I rebased Takashi's patch > on top of my pci/pciehp branch, resulting in the patch below. > > I pushed this, so you can try the whole thing out at: > > http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/pciehp&id=237e0218bcad52d3df6746cd8d15a0d353bae84f Thanks, I confirmed that the tree works. Takashi > > Bjorn > > > > PCI: pciehp: Remove surprise bit checks > > From: Takashi Iwai <tiwai@suse.de> > > Currently pciehp driver checks the surprise removal bit and handles the > hotplug event only when the bit is set. But there are devices that don't > set that bit, yet expect hotplug to work, e.g., the SD/MMC card reader on > HP ProBook 445 and 455 laptops appears only when you insert a card, and it > needs the hotplug event handling. > > For fixing this, basically we may ignore the surprise bit and always handle > the event. The only big concern in the past was the KVM device assignment, > but this has been fixed in KVM side (ignoring hotplug events during > secondary bus resets), there should be no obstacle ahead. > > The earlier discussion thread is found at: > https://lkml.kernel.org/r/s5h38vqjhk6.wl%25tiwai@suse.de > > [bhelgaas: rebase on Rajat's changes, remove HP_SUPR_RM() completely] > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=70261 > Signed-off-by: Takashi Iwai <tiwai@suse.de> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/hotplug/pciehp.h | 1 - > drivers/pci/hotplug/pciehp_ctrl.c | 2 -- > drivers/pci/hotplug/pciehp_hpc.c | 11 +++++------ > 3 files changed, 5 insertions(+), 9 deletions(-) > > diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h > index 8a66866b8cf1..d2a67aa0e49d 100644 > --- a/drivers/pci/hotplug/pciehp.h > +++ b/drivers/pci/hotplug/pciehp.h > @@ -124,7 +124,6 @@ struct controller { > #define MRL_SENS(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_MRLSP) > #define ATTN_LED(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_AIP) > #define PWR_LED(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_PIP) > -#define HP_SUPR_RM(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_HPS) > #define EMI(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_EIP) > #define NO_CMD_CMPL(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS) > #define PSN(ctrl) ((ctrl)->slot_cap >> 19) > diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c > index fec99a164d93..a4d29d24057d 100644 > --- a/drivers/pci/hotplug/pciehp_ctrl.c > +++ b/drivers/pci/hotplug/pciehp_ctrl.c > @@ -535,8 +535,6 @@ static void interrupt_event_handler(struct work_struct *work) > break; > case INT_PRESENCE_ON: > case INT_PRESENCE_OFF: > - if (!HP_SUPR_RM(ctrl)) > - break; > ctrl_dbg(ctrl, "Surprise Removal\n"); > handle_surprise_event(p_slot); > break; > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > index da4b0204b4f7..1076a51623b5 100644 > --- a/drivers/pci/hotplug/pciehp_hpc.c > +++ b/drivers/pci/hotplug/pciehp_hpc.c > @@ -628,17 +628,18 @@ int pciehp_reset_slot(struct slot *slot, int probe) > { > struct controller *ctrl = slot->ctrl; > struct pci_dev *pdev = ctrl_dev(ctrl); > - u16 stat_mask = 0, ctrl_mask = 0; > + u16 stat_mask, ctrl_mask; > > if (probe) > return 0; > > - if (HP_SUPR_RM(ctrl) && !ATTN_BUTTN(ctrl)) { > + ctrl_mask = PCI_EXP_SLTCTL_DLLSCE; > + stat_mask = PCI_EXP_SLTSTA_DLLSC; > + > + if (!ATTN_BUTTN(ctrl)) { /* as in pcie_enable_notification() */ > ctrl_mask |= PCI_EXP_SLTCTL_PDCE; > stat_mask |= PCI_EXP_SLTSTA_PDC; > } > - ctrl_mask |= PCI_EXP_SLTCTL_DLLSCE; > - stat_mask |= PCI_EXP_SLTSTA_DLLSC; > > pcie_write_cmd(ctrl, 0, ctrl_mask); > if (pciehp_poll_mode) > @@ -741,8 +742,6 @@ static inline void dbg_ctrl(struct controller *ctrl) > 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", > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h index 8a66866b8cf1..d2a67aa0e49d 100644 --- a/drivers/pci/hotplug/pciehp.h +++ b/drivers/pci/hotplug/pciehp.h @@ -124,7 +124,6 @@ struct controller { #define MRL_SENS(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_MRLSP) #define ATTN_LED(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_AIP) #define PWR_LED(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_PIP) -#define HP_SUPR_RM(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_HPS) #define EMI(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_EIP) #define NO_CMD_CMPL(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS) #define PSN(ctrl) ((ctrl)->slot_cap >> 19) diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c index fec99a164d93..a4d29d24057d 100644 --- a/drivers/pci/hotplug/pciehp_ctrl.c +++ b/drivers/pci/hotplug/pciehp_ctrl.c @@ -535,8 +535,6 @@ static void interrupt_event_handler(struct work_struct *work) break; case INT_PRESENCE_ON: case INT_PRESENCE_OFF: - if (!HP_SUPR_RM(ctrl)) - break; ctrl_dbg(ctrl, "Surprise Removal\n"); handle_surprise_event(p_slot); break; diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index da4b0204b4f7..1076a51623b5 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -628,17 +628,18 @@ int pciehp_reset_slot(struct slot *slot, int probe) { struct controller *ctrl = slot->ctrl; struct pci_dev *pdev = ctrl_dev(ctrl); - u16 stat_mask = 0, ctrl_mask = 0; + u16 stat_mask, ctrl_mask; if (probe) return 0; - if (HP_SUPR_RM(ctrl) && !ATTN_BUTTN(ctrl)) { + ctrl_mask = PCI_EXP_SLTCTL_DLLSCE; + stat_mask = PCI_EXP_SLTSTA_DLLSC; + + if (!ATTN_BUTTN(ctrl)) { /* as in pcie_enable_notification() */ ctrl_mask |= PCI_EXP_SLTCTL_PDCE; stat_mask |= PCI_EXP_SLTSTA_PDC; } - ctrl_mask |= PCI_EXP_SLTCTL_DLLSCE; - stat_mask |= PCI_EXP_SLTSTA_DLLSC; pcie_write_cmd(ctrl, 0, ctrl_mask); if (pciehp_poll_mode) @@ -741,8 +742,6 @@ static inline void dbg_ctrl(struct controller *ctrl) 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",