LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: "Alex G." <mr.nuke.me@gmail.com>
Cc: bhelgaas@google.com, linux-pci@vger.kernel.org,
	gregkh@linuxfoundation.org, fred@fredlawl.com,
	linux-kernel@vger.kernel.org, alex_gagniuc@dellteam.com,
	austin_bolen@dell.com, keith.busch@intel.com
Subject: Re: [PATCH RESEND] PCI/AER: Use a common function to print AER error bits
Date: Mon, 30 Apr 2018 12:15:31 -0500	[thread overview]
Message-ID: <20180430171531.GB95643@bhelgaas-glaptop.roam.corp.google.com> (raw)
In-Reply-To: <220bb125-b933-abf3-7b30-63446634e8d6@gmail.com>

On Sat, Apr 28, 2018 at 12:07:48PM -0500, Alex G. wrote:
> On 04/28/2018 11:46 AM, Alex G. wrote:
> > On 04/27/2018 05:43 PM, Bjorn Helgaas wrote:
> > > On Tue, Apr 17, 2018 at 12:09:43PM -0500, Alexandru Gagniuc wrote:
> (snip)
> > > > +    memset(&info, 0, sizeof(info));
> > > > +    info.severity = aer_severity;
> > > > +    info.status = status;
> > > > +    info.mask = mask;
> > > > +    info.first_error = 0x1f;
> > > 
> > > I like this patch a lot, but where does this "first_error = 0x1f" come
> > > from?
> > 
> > aer_(un)correctable_error_string don't go to [0x1f], so this guarantees
> > us we don't print "(First)".
> > 
> > > I assume this is supposed to be the "First Error Pointer" in the
> > > Advanced Error Capabilities and Control register (PCIe r4.0, sec
> > > 7.8.4.7).  There is a "cap_control" field in struct
> > > aer_capability_regs; should we be using that here?
> > 
> > There is a way to extract it from the PCI regs, and it's quite simple.
> > IIRC, it should be all f's when the capability is not implemented. I
> > wanted to avoid any further parsing of PCI regs in this patch.
> 
> I could update the offending line to say:
>  +	info.first_error = PCI_ERR_CAP_FEP(aer->cap_control);

That's what I would have expected.  So I'd say either do this, or add
a comment about why it's not the right thing to do.

> Though I still have the concerns with validating CPER data:
> 
> > I can see a way to use even more common printk code, but that requires
> > validating the PCI regs we get from firmware. That means we need to make
> > a guarantee about CPER that is beyond the scope of this patch.

Sounds like this is material for another patch, but if/when you do
that, I'd like to understand your concern about validating the
registers we get from firmware.  Are you worried about getting
incorrect register contents, then printing the wrong info, making
the wrong decision about how to recover, something else?

Bjorn

  reply	other threads:[~2018-04-30 17:15 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-17 17:09 Alexandru Gagniuc
2018-04-26 17:27 ` Tyler Baicar
2018-04-27 22:43 ` Bjorn Helgaas
2018-04-28 16:46   ` Alex G.
2018-04-28 17:07     ` Alex G.
2018-04-30 17:15       ` Bjorn Helgaas [this message]
2018-04-30 17:41         ` Alex G.
2018-05-07 22:06           ` Bjorn Helgaas
2018-04-30 19:52 ` [PATCH v2] " Alexandru Gagniuc
2018-05-07 22:13   ` Bjorn Helgaas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180430171531.GB95643@bhelgaas-glaptop.roam.corp.google.com \
    --to=helgaas@kernel.org \
    --cc=alex_gagniuc@dellteam.com \
    --cc=austin_bolen@dell.com \
    --cc=bhelgaas@google.com \
    --cc=fred@fredlawl.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=keith.busch@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=mr.nuke.me@gmail.com \
    --subject='Re: [PATCH RESEND] PCI/AER: Use a common function to print AER error bits' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).