Re: [PATCH 1/11] mpt2sas: mpt2sas_base sources

[Matthew Wilcox <matthew@wil.cx>]

On Tue, Mar 03, 2009 at 03:29:16PM -0600, James Bottomley wrote:
> On Wed, 2009-02-25 at 14:12 -0700, Matthew Wilcox wrote:
> > On Wed, Feb 25, 2009 at 01:27:00PM -0700, Moore, Eric wrote:
> > > > > +static void
> > > > > +_base_mask_interrupts(struct MPT2SAS_ADAPTER *ioc)
> > > > > +{
> > > > > + ? ? ? u32 him_register;
> > > > > +
> > > > > + ? ? ? ioc->mask_interrupts = 1;
> > > > > + ? ? ? him_register = readl(&ioc->chip->HostInterruptMask);
> > > > > + ? ? ? him_register |= MPI2_HIM_DIM + MPI2_HIM_RIM + 
> > > > MPI2_HIM_RESET_IRQ_MASK;
> > > > > + ? ? ? writel(him_register, &ioc->chip->HostInterruptMask);
> > > > 
> > > > This is a posted write. Does it need to be flushed?
> > > > ie do other parts of the driver require this take effect immediately
> > > > or will they tolerate some late arriving interrupt?
> > > 
> > > What are you suggesting?   Calling wmb()?
> > 
> > PCI writes can be posted, so the him_register writel() may not complete
> > for a few thousand cycles.  wmb() isn't going to solve the problem.  A
> > readl() from the device would, as would your current solution of
> > ignoring subsequent interrupts.
> 
> This seems to be the most serious issue with the current patch set.  If
> you repost with that fixed, I can put it into scsi-misc and we can
> address the other issues later.

I'm confused.  There's no issue with the code that Eric posted.  Grant
asked if he needs to readl() in order to flush the posted write, but the
answer is no, because if a spurious interrupt comes in, the interrupt
handler will ignore it.  So I don't see what Eric needs to fix.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."