| Message ID | 1392005051-54508-6-git-send-email-wangyijing@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, 2014-02-10 at 12:04 +0800, Yijing Wang wrote: > +static DEFINE_SPINLOCK(pci_freeze_lock); The lock is used only here. > +/** > + * pci_bus_freeze_device - freeze pci bus to access pci device > + * @bus: the pci bus to freeze > + * > + * Replace pci bus ops by pci_dummy_ops, protect system from > + * accessing pci devices. > + */ > +void pci_bus_freeze_device(struct pci_bus *bus) > +{ > + struct pci_ops *ops; > + unsigned long flags; > + > + spin_lock_irqsave(&pci_freeze_lock, flags); > + ops = pci_bus_set_ops(bus, &pci_dummy_ops); > + bus->save_ops = ops; > + spin_unlock_irqrestore(&pci_freeze_lock, flags); Against what exactly are you locking here? Regards Oliver -- 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
Hi Oliver, Thanks for your review and comments! >> +static DEFINE_SPINLOCK(pci_freeze_lock); > > The lock is used only here. Also be used in pci_bus_unfreeze_device(); > >> +/** >> + * pci_bus_freeze_device - freeze pci bus to access pci device >> + * @bus: the pci bus to freeze >> + * >> + * Replace pci bus ops by pci_dummy_ops, protect system from >> + * accessing pci devices. >> + */ >> +void pci_bus_freeze_device(struct pci_bus *bus) >> +{ >> + struct pci_ops *ops; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&pci_freeze_lock, flags); >> + ops = pci_bus_set_ops(bus, &pci_dummy_ops); >> + bus->save_ops = ops; >> + spin_unlock_irqrestore(&pci_freeze_lock, flags); > > Against what exactly are you locking here? I want to use this spin lock to serialize freeze device and unfreeze device. Thanks! Yijing. > > > >
On Mon, 2014-02-10 at 15:59 +0800, Yijing Wang wrote: > Hi Oliver, > Thanks for your review and comments! > > >> +static DEFINE_SPINLOCK(pci_freeze_lock); > > > > The lock is used only here. > > Also be used in pci_bus_unfreeze_device(); Sorry, I meant only in this patch. > > > > >> +/** > >> + * pci_bus_freeze_device - freeze pci bus to access pci device > >> + * @bus: the pci bus to freeze > >> + * > >> + * Replace pci bus ops by pci_dummy_ops, protect system from > >> + * accessing pci devices. > >> + */ > >> +void pci_bus_freeze_device(struct pci_bus *bus) > >> +{ > >> + struct pci_ops *ops; > >> + unsigned long flags; > >> + > >> + spin_lock_irqsave(&pci_freeze_lock, flags); > >> + ops = pci_bus_set_ops(bus, &pci_dummy_ops); > >> + bus->save_ops = ops; > >> + spin_unlock_irqrestore(&pci_freeze_lock, flags); > > > > Against what exactly are you locking here? > > I want to use this spin lock to serialize freeze device and unfreeze device. Yes, but against what? I am sorry I should have been more explicit. You are using these functions only in pci_scan_single_device() CPU A CPU B pci_bus_freeze_device() wait bus->save_ops = ops {valid} wait ... pci_bus_freeze_device() wait bus->save_ops = ops {pci_dummy_ops !} pci_bus_unfreeze_device() wait pci_bus_set_ops(bus, bus->save_ops) You see the problem? If this function ever races with itself, the locking is useless. If it doesn't race with itself, the locking is not needed. If this function can really race with itself, you need a refcount for freezing. Regards Oliver -- 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
>>>> + spin_lock_irqsave(&pci_freeze_lock, flags); >>>> + ops = pci_bus_set_ops(bus, &pci_dummy_ops); >>>> + bus->save_ops = ops; >>>> + spin_unlock_irqrestore(&pci_freeze_lock, flags); >>> >>> Against what exactly are you locking here? >> >> I want to use this spin lock to serialize freeze device and unfreeze device. > > Yes, but against what? I am sorry I should have been more explicit. > You are using these functions only in pci_scan_single_device() Hi Oliver, thanks very much for your detailed analysis. My original intention to use pci_freeze_lock to serialize pci_bus_freeze_device() and pci_bus_unfreeze_device(), because I think these two functions maybe used in other places, although currently only used in pci_scan_single_device(). Like: CPU A CPU B pci_bus_freeze_device() pci_bus_unfreeze_device() pci_bus_set_ops(bus, &pci_dummy_ops); pci_bus_set_ops(bus, bus->save_ops); ---> here, save_ops is NULL, it's bad. bus->save_ops = ops; > > > CPU A CPU B > pci_bus_freeze_device() wait > bus->save_ops = ops {valid} wait > ... pci_bus_freeze_device() > wait bus->save_ops = ops > {pci_dummy_ops !} > pci_bus_unfreeze_device() wait > pci_bus_set_ops(bus, bus->save_ops) > > You see the problem? > Yes, this is a issue, good catch. I should add a refcount to avoid this situation. > If this function ever races with itself, the locking is useless. > If it doesn't race with itself, the locking is not needed. > If this function can really race with itself, you need a refcount > for freezing. Thanks again! > > > > . >
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 8f31ab3..bbef95e 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -2225,6 +2225,60 @@ bool pci_serial_number_changed(struct pci_dev *pdev) } EXPORT_SYMBOL(pci_serial_number_changed); +static int pci_dummy_read(struct pci_bus *bus, unsigned int devfn, + int where, int size, u32 *val) +{ + return PCIBIOS_DEVICE_NOT_FOUND; +} + +static int pci_dummy_write(struct pci_bus *bus, unsigned int devfn, + int reg, int size, u32 val) +{ + return PCIBIOS_DEVICE_NOT_FOUND; +} + +static struct pci_ops pci_dummy_ops = { + .read = pci_dummy_read, + .write = pci_dummy_write, +}; + +static DEFINE_SPINLOCK(pci_freeze_lock); +/** + * pci_bus_freeze_device - freeze pci bus to access pci device + * @bus: the pci bus to freeze + * + * Replace pci bus ops by pci_dummy_ops, protect system from + * accessing pci devices. + */ +void pci_bus_freeze_device(struct pci_bus *bus) +{ + struct pci_ops *ops; + unsigned long flags; + + spin_lock_irqsave(&pci_freeze_lock, flags); + ops = pci_bus_set_ops(bus, &pci_dummy_ops); + bus->save_ops = ops; + spin_unlock_irqrestore(&pci_freeze_lock, flags); +} + +/** + * pci_bus_unfreeze_device - unfreeze pci bus to acess pci devices + * @bus: the pci bus to unfreeze + * + * Restore pci bus original ops, so pci bus can access + * pci devices normally. + */ +void pci_bus_unfreeze_device(struct pci_bus *bus) +{ + unsigned long flags; + + BUG_ON(!bus->save_ops); + spin_lock_irqsave(&pci_freeze_lock, flags); + pci_bus_set_ops(bus, bus->save_ops); + bus->save_ops = NULL; + spin_unlock_irqrestore(&pci_freeze_lock, flags); +} + /** * pci_configure_ari - enable or disable ARI forwarding * @dev: the PCI device diff --git a/include/linux/pci.h b/include/linux/pci.h index d60c0b6..74dd968 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -442,6 +442,7 @@ struct pci_bus { struct resource busn_res; /* bus numbers routed to this bus */ struct pci_ops *ops; /* configuration access functions */ + struct pci_ops *save_ops; /* save configuration access functions */ struct msi_chip *msi; /* MSI controller */ void *sysdata; /* hook for sys-specific extension */ struct proc_dir_entry *procdir; /* directory entry in /proc/bus/pci */ @@ -1026,6 +1027,8 @@ ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf); ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf); bool pci_serial_number_changed(struct pci_dev *pdev); +void pci_bus_freeze_device(struct pci_bus *bus); +void pci_bus_unfreeze_device(struct pci_bus *bus); /* Helper functions for low-level code (drivers/pci/setup-[bus,res].c) */ resource_size_t pcibios_retrieve_fw_addr(struct pci_dev *dev, int idx);
Pci_dummy_ops does nothing when we use it to read/write pci_device. So we can isolate pci device by replace its bus pci_ops by pci_dummy_ops. This is preparation for the later patch. Signed-off-by: Yijing Wang <wangyijing@huawei.com> --- drivers/pci/pci.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 3 ++ 2 files changed, 57 insertions(+), 0 deletions(-)