Grant Grundler writes: > 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.
Think about it. Taking the lock ensures that we don't do the assignment (dev->block_ucfg_access = 1) while any other cpu has the pci_lock. In other words, the reason for taking the lock is so that we wait until nobody else is doing an access, not to make others wait. > 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. That's probably a good thing to do to. Paul. - 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/