Re: [PATCH 1/2] pci: Block config access during BIST (resend)

Brian King
2005-09-03T17:38:06Z

Grant Grundler wrote:
> On Sat, Sep 03, 2005 at 09:11:30AM +1000, Paul Mackerras wrote:
>
>>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.
>
>
> The block_ucfg_access field is only used when making the choice to
> use saved state or call the PCI bus cfg accessor.
> I don't what problem waiting solves here since any CPU already
> accessing real cfg space will finish what they are doing anyway.
> ie they already made the choice to access real cfg space.
> We just need to make sure successive accesses to cfg space
> for this device only access the saved state. For that, a memory barrier
> around all uses of block_ucfg_access should be sufficient.
> Or do you think I'm still drinking the wrong color cool-aid?

Without the locking, we introduce a race condition.

CPU 0 CPU 1

pci_block_user_cfg_access
pci_save_state
pci_read_user_config_space
check block_ucfg_access
set block_ucfg_access
other code that puts the device
in a state such that it cannot
handle read config i/o, such as
running BIST.

pci read config

In this scenario, If the real read on the left happens after the flag is
set to block user config accesses, then the thread that set the flag
could go off and start BIST or do something else to put the pci device
in a state where it cannot accept real config I/O and we end up with a
target abort, which is exactly what this patch is attempting to fix.

Granted, for the specific usage scenario in ipr, where I am using this
to block config space over BIST, I use a pci config write to start BIST,
which would end up being a point of synchronization, but that seems a
bit non-obvious and limits how the patch can be used by others...

>>>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.
>
>
> One needs to verify pci_lock isn't acquired in pci_save_state()
> (or some other obvious dead lock).

Unfortunately, it is... Every pci config access grabs the lock, so we
would need to use some special code that did not acquire the lock in
pci_save_state if we wanted to do such a thing.

Brian

--
Brian King
eServer Storage I/O
IBM Linux Technology Center