Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
Cc: bhelgaas@google.com, rjui@broadcom.com, sbranden@broadcom.com,
	f.fainelli@gmail.com, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Rob Herring <robh@kernel.org>
Subject: Re: [PATCH 2/3] PCI: iproc: Stop using generic config read/write functions
Date: Thu, 30 Jul 2020 11:09:58 -0500
Message-ID: <20200730160958.GA2038661@bjorn-Precision-5520> (raw)
In-Reply-To: <20200730033747.18931-2-mark.tomlinson@alliedtelesis.co.nz>

[+cc Lorenzo, Rob]

On Thu, Jul 30, 2020 at 03:37:46PM +1200, Mark Tomlinson wrote:
> The pci_generic_config_write32() function will give warning messages
> whenever writing less than 4 bytes at a time. As there is nothing we can
> do about this without changing the hardware, the message is just a
> nuisance. So instead of using the generic functions, use the functions
> that have already been written for reading/writing the config registers.

The reason that pci_generic_config_write32() message is there is
because, as the message says, a read/modify/write may corrupt bits in
adjacent registers.  

It makes me a little queasy to do these read/modify/write sequences
silently.  A generic driver doing an 8- or 16-bit config write has no
idea that the write may corrupt an adjacent register.  That leads to
bugs that are very difficult to debug and only reproducible on iProc.

The ratelimiting on the current pci_generic_config_write32() message
is based on the call site, not on the device.  That's not ideal: we
may emit several messages for device A, trigger ratelimiting, then do
a write for device B that doesn't generate a message.

I think it would be better to have a warning once per device, so if
XYZ device has a problem and we look at the dmesg log, we would find a
single message for device XYZ as a hint.  Would that reduce the
nuisance level enough?

So I think I did it wrong in fb2659230120 ("PCI: Warn on possible RW1C
corruption for sub-32 bit config writes").  Ratelimiting is the wrong
concept because what we want is a single warning per device, not a
limit on the similar messages for *all* devices, maybe something like
this:

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 79c4a2ef269a..e5f956b7e3b7 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -160,9 +160,12 @@ int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn,
 	 * write happen to have any RW1C (write-one-to-clear) bits set, we
 	 * just inadvertently cleared something we shouldn't have.
 	 */
-	dev_warn_ratelimited(&bus->dev, "%d-byte config write to %04x:%02x:%02x.%d offset %#x may corrupt adjacent RW1C bits\n",
+	if (!(bus->unsafe_warn & (1 << devfn))) {
+		dev_warn(&bus->dev, "%d-byte config write to %04x:%02x:%02x.%d offset %#x may corrupt adjacent RW1C bits\n",
 			     size, pci_domain_nr(bus), bus->number,
 			     PCI_SLOT(devfn), PCI_FUNC(devfn), where);
+		bus->unsafe_warn |= 1 << devfn;
+	}
 
 	mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8));
 	tmp = readl(addr) & mask;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c79d83304e52..264b009fa4a6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -613,6 +613,7 @@ struct pci_bus {
 	unsigned char	primary;	/* Number of primary bridge */
 	unsigned char	max_bus_speed;	/* enum pci_bus_speed */
 	unsigned char	cur_bus_speed;	/* enum pci_bus_speed */
+	u8		unsafe_warn;	/* warned about R/M/W config write */
 #ifdef CONFIG_PCI_DOMAINS_GENERIC
 	int		domain_nr;
 #endif

> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
> ---
>  drivers/pci/controller/pcie-iproc.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> index 2c836eede42c..68ecd3050529 100644
> --- a/drivers/pci/controller/pcie-iproc.c
> +++ b/drivers/pci/controller/pcie-iproc.c
> @@ -709,12 +709,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>  {
>  	int ret;
>  	struct iproc_pcie *pcie = iproc_data(bus);
> +	int busno = bus->number;
>  
>  	iproc_pcie_apb_err_disable(bus, true);
>  	if (pcie->iproc_cfg_read)
>  		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
>  	else
> -		ret = pci_generic_config_read32(bus, devfn, where, size, val);
> +		ret = iproc_pci_raw_config_read32(pcie, busno, devfn, where, size, val);
>  	iproc_pcie_apb_err_disable(bus, false);
>  
>  	return ret;
> @@ -724,9 +725,11 @@ static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
>  				     int where, int size, u32 val)
>  {
>  	int ret;
> +	struct iproc_pcie *pcie = iproc_data(bus);
> +	int busno = bus->number;
>  
>  	iproc_pcie_apb_err_disable(bus, true);
> -	ret = pci_generic_config_write32(bus, devfn, where, size, val);
> +	ret = iproc_pci_raw_config_write32(pcie, busno, devfn, where, size, val);
>  	iproc_pcie_apb_err_disable(bus, false);
>  
>  	return ret;
> -- 
> 2.28.0
> 

  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-30  3:37 [PATCH 1/3] PCI: iproc: Add bus number parameter to " Mark Tomlinson
2020-07-30  3:37 ` [PATCH 2/3] PCI: iproc: Stop using generic config " Mark Tomlinson
2020-07-30 16:09   ` Bjorn Helgaas [this message]
2020-07-30 16:36     ` Ray Jui
2020-07-30 16:45       ` Bjorn Helgaas
2020-07-30 22:58     ` Mark Tomlinson
2020-07-30 23:06       ` Bjorn Helgaas
2020-07-30  3:37 ` [PATCH 3/3] PCI: iproc: Set affinity mask on MSI interrupts Mark Tomlinson
2020-07-30 16:45   ` Ray Jui
2020-07-30 17:07   ` Scott Branden

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=20200730160958.GA2038661@bjorn-Precision-5520 \
    --to=helgaas@kernel.org \
    --cc=bhelgaas@google.com \
    --cc=f.fainelli@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mark.tomlinson@alliedtelesis.co.nz \
    --cc=rjui@broadcom.com \
    --cc=robh@kernel.org \
    --cc=sbranden@broadcom.com \
    /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-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/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-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

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


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