| Message ID | 20230328045122.25850-7-decui@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pci-hyper: fix race condition bugs for fast device hotplug | expand |
From: Dexuan Cui <decui@microsoft.com> > > Commit 414428c5da1c ("PCI: hv: Lock PCI bus on device eject") added the > pci_lock_rescan_remove() and pci_unlock_rescan_remove() to address the > race between create_root_hv_pci_bus() and hv_eject_device_work(), but it > doesn't really work well. > > Now with hbus->state_lock and other fixes, the race is resolved, so > remove pci_{lock,unlock}_rescan_remove(). Commit 414428c5da1c added the calls to pci_lock/unlock_rescan_remove() in both create_root_hv_pci_bus() and in hv_eject_device_work(). This patch removes the calls only in create_reboot_hv_pci_bus(), but leaves them in hv_eject_device_work(), in hv_pci_remove(), and in pci_devices_present_work(). So evidently it is still needed to provide exclusion for other cases. Perhaps your commit message could clarify that only the exclusion of create_root_hv_pci_bus() is now superfluous because of the hbus->state_lock. And commit 414428c5da1c isn't fully reverted because evidently the lock is still needed in hv_eject_device_work(). Michael > > Also enable async probing to reduce boot time. > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > --- > drivers/pci/controller/pci-hyperv.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index 2c0b86b20408..08ab389e27cc 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -2312,12 +2312,16 @@ static int create_root_hv_pci_bus(struct > hv_pcibus_device *hbus) > if (error) > return error; > > - pci_lock_rescan_remove(); > + /* > + * pci_lock_rescan_remove() and pci_unlock_rescan_remove() are > + * unnecessary here, because we hold the hbus->state_lock, meaning > + * hv_eject_device_work() and pci_devices_present_work() can't race > + * with create_root_hv_pci_bus(). > + */ > hv_pci_assign_numa_node(hbus); > pci_bus_assign_resources(bridge->bus); > hv_pci_assign_slots(hbus); > pci_bus_add_devices(bridge->bus); > - pci_unlock_rescan_remove(); > hbus->state = hv_pcibus_installed; > return 0; > } > @@ -4003,6 +4007,9 @@ static struct hv_driver hv_pci_drv = { > .remove = hv_pci_remove, > .suspend = hv_pci_suspend, > .resume = hv_pci_resume, > + .driver = { > + .probe_type = PROBE_PREFER_ASYNCHRONOUS, > + }, > }; > > static void __exit exit_hv_pci_drv(void) > -- > 2.25.1
> From: Michael Kelley (LINUX) <mikelley@microsoft.com> > Sent: Wednesday, March 29, 2023 9:55 AM > > From: Dexuan Cui <decui@microsoft.com> > > > > Commit 414428c5da1c ("PCI: hv: Lock PCI bus on device eject") added the > > pci_lock_rescan_remove() and pci_unlock_rescan_remove() to address the > > race between create_root_hv_pci_bus() and hv_eject_device_work(), but it > > doesn't really work well. > > > > Now with hbus->state_lock and other fixes, the race is resolved, so > > remove pci_{lock,unlock}_rescan_remove(). > > Commit 414428c5da1c added the calls to pci_lock/unlock_rescan_remove() > in both create_root_hv_pci_bus() and in hv_eject_device_work(). This > patch removes the calls only in create_reboot_hv_pci_bus(), but leaves > them in hv_eject_device_work(), in hv_pci_remove(), and in > pci_devices_present_work(). > So evidently it is still needed to provide exclusion for other cases. Perhaps > your commit message could clarify that only the exclusion of > create_root_hv_pci_bus() > is now superfluous because of the > hbus->state_lock. And commit 414428c5da1c > isn't fully reverted > because evidently the lock is still needed in hv_eject_device_work(). > > Michael Thanks! I'll update the commit message in v2.
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c index 2c0b86b20408..08ab389e27cc 100644 --- a/drivers/pci/controller/pci-hyperv.c +++ b/drivers/pci/controller/pci-hyperv.c @@ -2312,12 +2312,16 @@ static int create_root_hv_pci_bus(struct hv_pcibus_device *hbus) if (error) return error; - pci_lock_rescan_remove(); + /* + * pci_lock_rescan_remove() and pci_unlock_rescan_remove() are + * unnecessary here, because we hold the hbus->state_lock, meaning + * hv_eject_device_work() and pci_devices_present_work() can't race + * with create_root_hv_pci_bus(). + */ hv_pci_assign_numa_node(hbus); pci_bus_assign_resources(bridge->bus); hv_pci_assign_slots(hbus); pci_bus_add_devices(bridge->bus); - pci_unlock_rescan_remove(); hbus->state = hv_pcibus_installed; return 0; } @@ -4003,6 +4007,9 @@ static struct hv_driver hv_pci_drv = { .remove = hv_pci_remove, .suspend = hv_pci_suspend, .resume = hv_pci_resume, + .driver = { + .probe_type = PROBE_PREFER_ASYNCHRONOUS, + }, }; static void __exit exit_hv_pci_drv(void)
Commit 414428c5da1c ("PCI: hv: Lock PCI bus on device eject") added the pci_lock_rescan_remove() and pci_unlock_rescan_remove() to address the race between create_root_hv_pci_bus() and hv_eject_device_work(), but it doesn't really work well. Now with hbus->state_lock and other fixes, the race is resolved, so remove pci_{lock,unlock}_rescan_remove(). Also enable async probing to reduce boot time. Signed-off-by: Dexuan Cui <decui@microsoft.com> --- drivers/pci/controller/pci-hyperv.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)