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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |