Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: Andrew Murray <andrew.murray@arm.com>
Cc: linux-pci@vger.kernel.org,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Keith Busch <keith.busch@intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
	"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>
Subject: Re: [PATCH 1/3] PCI: Add PCI_ERROR_RESPONSE definition
Date: Fri, 23 Aug 2019 14:29:20 -0500
Message-ID: <20190823192920.GA127465@google.com> (raw)
In-Reply-To: <20190823104415.GC14582@e119886-lin.cambridge.arm.com>

On Fri, Aug 23, 2019 at 11:44:15AM +0100, Andrew Murray wrote:
> On Thu, Aug 22, 2019 at 03:05:49PM -0500, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > An MMIO read from a PCI device that doesn't exist or doesn't respond causes
> > a PCI error.  There's no real data to return to satisfy the CPU read, so
> > most hardware fabricates ~0 data.
> > 
> > Add a PCI_ERROR_RESPONSE definition for that and use it where appropriate
> > to make these checks consistent and easier to find.
> > 
> > Note that successful reads *also* may return ~0 data, so additional
> > information (e.g., knowledge that ~0 is not a valid register value) is
> > needed to reliably identify errors.

> > -	 * generally synthesize ~0 data to complete the read (except when
> > -	 * CRS SV is enabled and the read was for the Vendor ID; in that
> > -	 * case it synthesizes 0x0001 data).
> > +	 * generally synthesize ~0 data (PCI_ERROR_RESPONSE) to complete
> > +	 * the read (except when CRS SV is enabled and the read was for the
> > +	 * Vendor ID; in that case it synthesizes 0x0001 data).
> 
> There are some other areas in drivers/pci where comments also refer to ~0
> and similar:
> 
> $ git grep -i ffffffff drivers/pci/ | grep \*
> drivers/pci/access.c:            * have been written as 0xFFFFFFFF if hardware error happens
> drivers/pci/controller/dwc/pci-keystone.c: * bus error instead of returning 0xffffffff. This handler always returns 0
> drivers/pci/controller/pci-xgene.c:      * ready") instead of 0xFFFFFFFF ("device does not exist").  This
> drivers/pci/controller/pcie-iproc.c:     * eventually return the wrong data (0xffffffff).
> drivers/pci/pci.c: * FFFFFFFFs on the command line.)
> 
> I've removed anything in the above list that doesn't look like a good candidate
> for PCI_ERROR_RESPONSE.
> 
> Perhaps there is some value for replacing "~0" with "~0 (PCI_ERROR_RESPONSE)"
> in the comments too?

Good idea, I'll take a look at those.

> >  		pdev->cfg_size = PCI_CFG_SPACE_EXP_SIZE;
> >  		if (pci_read_config_dword(pdev, PCI_CFG_SPACE_SIZE, &status) !=
> > -		    PCIBIOS_SUCCESSFUL || (status == 0xffffffff))
> > +		    PCIBIOS_SUCCESSFUL || (status == (u32) PCI_ERROR_RESPONSE))
> 
> The casts are necessary but slightly annoying. Have you considered something
> like:
> 
> #define SET_PCI_ERROR_RESPONSE(val)	(val = ((typeof(val))(~0ULL)))
> #define RESPONSE_IS_PCI_ERROR(val)	(val == ((typeof(val))(~0ULL)))

I hadn't thought of that, but I really like the idea.  Thanks, I think
I'll try that out!

> >  			pdev->cfg_size = PCI_CFG_SPACE_SIZE;
> >  
> >  		if (pci_find_saved_cap(pdev, PCI_CAP_ID_EXP))
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 9e700d9f9f28..d64fd3788061 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -123,6 +123,13 @@ enum pci_interrupt_pin {
> >  /* The number of legacy PCI INTx interrupts */
> >  #define PCI_NUM_INTX	4
> >  
> > +/*
> > + * Reading from a device that doesn't respond typically returns ~0.  A
> > + * successful read from a device may also return ~0, so you need additional
> > + * information to reliably identify errors.
> > + */
> > +#define PCI_ERROR_RESPONSE		(~0ULL)
> > +
> >  /*
> >   * pci_power_t values must match the bits in the Capabilities PME_Support
> >   * and Control/Status PowerState fields in the Power Management capability.
> > -- 
> > 2.23.0.187.g17f5b7556c-goog
> > 

  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-22 20:05 [PATCH v2 0/3] PCI: Add PCI_ERROR_RESPONSE, check for errors Bjorn Helgaas
2019-08-22 20:05 ` [PATCH 1/3] PCI: Add PCI_ERROR_RESPONSE definition Bjorn Helgaas
2019-08-23 10:44   ` Andrew Murray
2019-08-23 19:29     ` Bjorn Helgaas [this message]
2019-08-22 20:05 ` [PATCH 2/3] PCI / PM: Decode D3cold power state correctly Bjorn Helgaas
2019-08-22 20:05 ` [PATCH 3/3] PCI / PM: Return error when changing power state from D3cold Bjorn Helgaas
2019-08-22 21:10   ` Rafael J. Wysocki
2019-08-23  7:22 ` [PATCH v2 0/3] PCI: Add PCI_ERROR_RESPONSE, check for errors Mika Westerberg
2019-08-23 15:04 ` Keith Busch
2019-11-14 20:19 ` Bjorn Helgaas

Reply instructions:

You may reply publically 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=20190823192920.GA127465@google.com \
    --to=helgaas@kernel.org \
    --cc=andrew.murray@arm.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=keith.busch@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mika.westerberg@linux.intel.com \
    --cc=rafael.j.wysocki@intel.com \
    --cc=rjw@rjwysocki.net \
    /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

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git