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

[Brian King <brking@us.ibm.com>]

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