Re: Is msix_flush_writes() really needed? And multi_msi_*() flawed?

[Grant Grundler <grundler@parisc-linux.org>]

On Mon, Nov 10, 2008 at 10:07:50AM +1100, Michael Ellerman wrote:
> On Fri, 2008-11-07 at 08:53 +0000, Jan Beulich wrote:
> > msix_flush_writes() is being called exclusively after calling msi_set_mask_bit(),
> > and that function already does follow writel() by readl() in the MSI-X case.
> 
> Which makes me wonder why the initial patch was necessary?

"This patch performs a read flush after writes to the MSI-X table for
mask and unmask operations. "

The msi_set_mask_bit() and msi_set_enable() only use config space writes.
Are config space writes postable?

The original patch seems to suggest that.
I thought only MMIO space writes are postable and this chunk of PCI 2.3 spec
suggests that as well:

3.3.3.3.1. Basic Operation of a Delayed Transaction
    All bus commands that must complete on the destination bus before
    completing on the originating bus may be completed as a Delayed
    Transaction. These include Interrupt Acknowledge, I/O Read, I/O Write,
    Configuration Read, Configuration Write, Memory Read, Memory Read Line,
    and Memory Read Multiple commands. Memory Write and Memory Write and
    Invalidate commands can complete on the originating bus before completing
    on the destination bus (i.e., can be posted).


> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=988cbb15e00e6f924d052874b40c6a5447f9fdd7
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/pci/msi.c;h=a4ef93ea4c547b78bf80b5b12c9c20101bd3a1ec;hb=988cbb15e00e6f924d052874b40c6a5447f9fdd7
> 
> AFAICS there was already a readl() in msi_set_mask_bit(), so either the
> initial patch didn't do anything useful or we're missing the point.

My impression was the former.

It's possible the arch this was discovered on implements postable
config space writes.  I'm looking at drivers/parisc/dino.c and it
looks to me like it's using posted writes to generate config space
writes (see dino_cfg_write()). lba_pci.c in the same directory seems
to enforce the semantics correctly (see elroy_cfg_write()).
*Cough* so where did I put the brown paper bag last time...

thanks,
grant

> 
> cheers
> 
> -- 
> Michael Ellerman
> OzLabs, IBM Australia Development Lab
> 
> wwweb: http://michael.ellerman.id.au
> phone: +61 2 6212 1183 (tie line 70 21183)
> 
> We do not inherit the earth from our ancestors,
> we borrow it from our children. - S.M.A.R.T Person