Re: [PATCH v4 2/2] IB/qib: use arch_phys_wc_add()

[Doug Ledford <dledford@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3240 bytes --]

On Wed, 2015-04-22 at 19:37 +0200, Luis R. Rodriguez wrote:
> On Wed, Apr 22, 2015 at 12:57:18PM -0400, Doug Ledford wrote:
> > On Wed, 2015-04-22 at 17:33 +0200, Luis R. Rodriguez wrote:
> > > On Wed, Apr 22, 2015 at 09:54:38AM -0400, Doug Ledford wrote:
> > > > On Tue, 2015-04-21 at 14:50 -0700, Luis R. Rodriguez wrote:
> > > > 
> > > > This:
> > > > > +	/* MTRR was used if this is non-zero */
> > > > > +	if (!dd->wc_cookie)
> > > > >  		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> > > > 
> > > > And this:
> > > > > +		dd->wc_cookie = arch_phys_wc_add(pioaddr, piolen);
> > > > > +		if (dd->wc_cookie < 0)
> > > > > +			ret = -EINVAL;
> > > > 
> > > > don't agree on what wc_cookie will be on error.
> > > 
> > > Can you elaborate? The one below is the one that starts things,
> > > and arch_phys_wc_add() will return 0 on PAT systems. For non-PAT
> > > systems it will return a number > 0 *iff* a valid MTRR was added.
> > > It will return negative onloy on error then.
> > > 
> > > The change above is meant to replace a check put in place to see
> > > if PAT was enabled. The way we replace this is to ensure that
> > > arch_phys_wc_add() returned 0.
> > > 
> > > If you disagree it'd be great if you can elaborate why.
> > 
> > Maybe I'm missing something, but in qib_enable_wc() you store the return
> > from arch_phys_wc_add into wc_cookie.  That return is negative,
> 
> If and only if the system was non-PAT and mtrr_add() failed.
> 
> >  so you
> > return from qib_enable_wc() to qib_init_one(), they see the ret value,
> > they print out a warning about bad performance, then they clear the
> > return value and continue with device initialization.
> > 
> > In all of this though, wc_cookie is never cleared and so it still has
> > the error condition in it.  Then, much later at run time, you call
> > mmap_piobufs() and you check the contents of wc_cookie, and if it's
> > non-0 (which is still will be), you do the wrong thing, right?
> 
> Originally the code had it to run pgprot_writecombine() if PAT was going to be
> used. After the code changes we check for !cookie which will be true when
> cookie is 0 only. In case the cookie was an error, that is if mtrr_add()
> failed, then this code would not run because (!negative) is false. The goal was
> to trigger a run if the cookie was 0, which can only happen if PAT was enabled.

OK, the logic works, but as much as anything, it's the comment that's
misleading.  The code would be clearer with a comment like this:

/* We used PAT if wc_cookie == 0 */
if (!dd->wc_cookie) {

That would be more accurate as well since the original comment didn't
account for the possible error code in wc_cookie, so it's possible you
didn't use either PAT or wc if you have that error code.

> Please let me know, I'd like to get this right too.
> 
> > And what
> > about at shutdown when you call qib_disable_wc() and your cookie still
> > has an error code in it as well?
> 
> Well fortunately arch_phys_wc_del(negative) and arch_phys_wc_del(0) will be
> a no-op. Its what helps us remove so much clutter.

OK.

-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]