PCI: pciehp: Check for valid PCI_BRIDGE_CONTROL in pciehp_unconfigure_device()
PATCH v3 1/1 PCI: pciehp: Check for valid PCI_BRIDGE_CONTROL in pciehp_unconfigure_device()
During surprise hot-unplug the device is not there anymore. When that happens we read 0xffffffff from the registers and pciehp_unconfigure_device() might inadvertently think the device is a display device because bridge control register returns 0xff refusing to remove it:
pciehp 0000:00:1c.0:pcie004: Slot(0): Link Down
pciehp 0000:00:1c.0:pcie004: Slot(0): Card present
pciehp 0000:00:1c.0:pcie004: Cannot remove display device 0000:01:00.0
This causes the hotplug functionality to leave the hierarcy untouched preventing further hotplug operations.
Fix this so that we first compare the read bctl value against 0xffff (device is not present anymore) before determining whether the device is a display or not.
---
Hi Bjorn,
This is an updated version of the patch 8/8 in my "PCI: Improvements for native PCIe hotplug" patch series and applies on top of pci.git/pci/hotplug.
We now just check bctl against 0xffff before preventing removal of the "display" device.
I'm inclined to remove the whole check of "display" device because I don't see any reason for it to be there in the first place. This code is pretty much duplicated from shpchp_pci.c where it might make some sense.
drivers/pci/hotplug/pciehp_pci.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/pci/hotplug/pciehp_pci.c +4 -3
@@ -79,7 +79,6 @@ int pciehp_configure_device(struct slot *p_slot)int pciehp_unconfigure_device (struct slot *p_slot) {int rc =0 ; -u8 bctl =0 ;u8 presence =0 ;struct pci_dev *dev, *temp;struct pci_bus *parent = p_slot->ctrl->pcie->port->subordinate;
@@ -102,8 +101,10 @@ int pciehp_unconfigure_device(struct slot *p_slot) bus_list) {pci_dev_get (dev);if (dev->hdr_type ==PCI_HEADER_TYPE_BRIDGE && presence) { -pci_read_config_byte (dev,PCI_BRIDGE_CONTROL , &bctl); -if (bctl &PCI_BRIDGE_CTL_VGA ) { +u16 bctl =0 ; + +pci_read_config_word (dev,PCI_BRIDGE_CONTROL , &bctl); +if (bctl !=0xffff && (bctl &PCI_BRIDGE_CTL_VGA )) {ctrl_err (ctrl,"Cannot remove display device %s\n" ,pci_name (dev));
Signature · 1 line
2.14.2
Quoted 23 lines · Mika Westerberg
During surprise hot-unplug the device is not there anymore. When that happens we read
0xfffffffffrom the registers andpciehp_unconfigure_device()might inadvertently think the device is a display device because bridge control register returns0xffrefusing to remove it:pciehp 0000:00:1c.0:pcie004: Slot(0): Link Down
pciehp 0000:00:1c.0:pcie004: Slot(0): Card present
pciehp 0000:00:1c.0:pcie004: Cannot remove display device 0000:01:00.0This causes the hotplug functionality to leave the hierarcy untouched preventing further hotplug operations.
Fix this so that we first compare the read bctl value against
0xffff(device is not present anymore) before determining whether the device is a display or not.Signed-off-by Mika Westerberg <[email protected]>---
Hi Bjorn,This is an updated version of the patch 8/8 in my "PCI: Improvements for native PCIe hotplug" patch series and applies on top of pci.git/pci/hotplug.
Quoted 6 lines
We now just check bctl against
0xffffbefore preventing removal of the "display" device.I'm inclined to remove the whole check of "display" device because I don't see any reason for it to be there in the first place. This code is pretty much duplicated from
shpchp_pci.cwhere it might make some sense.
Yeah, I don't understand this at all. If the device is gone, it's gone, and complaining about it being a display device isn't going to make it come back. I don't know the history of that check, but I agree that it might make sense to remove it altogether.
The message (and consequently your changelog) also seem misleading because we're removing *dev*, which in this case is a bridge that might lead to a display device. But it's not a display device itself.
Quoted 31 lines
drivers/pci/hotplug/pciehp_pci.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)drivers/pci/hotplug/pciehp_pci.c +4
-3@@ -79,7 +79,6 @@ int pciehp_configure_device(struct slot *p_slot)int pciehp_unconfigure_device (struct slot *p_slot) {int rc =0 ; -u8 bctl =0 ;u8 presence =0 ;struct pci_dev *dev, *temp;struct pci_bus *parent = p_slot->ctrl->pcie->port->subordinate;@@ -102,8 +101,10 @@ int pciehp_unconfigure_device(struct slot *p_slot) bus_list) {pci_dev_get (dev);if (dev->hdr_type ==PCI_HEADER_TYPE_BRIDGE && presence) { -pci_read_config_byte (dev,PCI_BRIDGE_CONTROL , &bctl); -if (bctl &PCI_BRIDGE_CTL_VGA ) { +u16 bctl =0 ; + +pci_read_config_word (dev,PCI_BRIDGE_CONTROL , &bctl); +if (bctl !=0xffff && (bctl &PCI_BRIDGE_CTL_VGA )) {ctrl_err (ctrl,"Cannot remove display device %s\n" ,pci_name (dev));Signature · 1 line
2.14.2