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/
