On Fri, Sep 02, 2005 at 06:56:35PM -0500, Brian King wrote: > Andrew Morton wrote: > >Brian King <[EMAIL PROTECTED]> wrote: > > > >>+void pci_block_user_cfg_access(struct pci_dev *dev) > >>+{ > >>+ unsigned long flags; > >>+ > >>+ pci_save_state(dev); > >>+ spin_lock_irqsave(&pci_lock, flags); > >>+ dev->block_ucfg_access = 1; > >>+ spin_unlock_irqrestore(&pci_lock, flags); > > > > > >Are you sure the locking in here is meaningful? All it will really do is > >give you a couple of barriers. > > Actually, it is meaningful. It synchronizes the blocking of pci config > accesses with other pci config accesses that may be going on when this > function is called. Without the locking, the API cannot guarantee that > no further user initiated PCI config accesses will be initiated after > this function is called.
I don't have the impression you understood what Andrew wrote. dev->block_ucfg_access = 1 is essentially an atomic operation. AFAIK, Use of the pci_lock doesn't solve any race conditions that mb() wouldn't solve. If you had: spin_lock_irqsave(&pci_lock, flags); pci_save_state(dev); dev->block_ucfg_access = 1; spin_unlock_irqrestore(&pci_lock, flags); Then I could buy your arguement since the flag now implies we need to atomically save state and set the flag. grant - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/