Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: Ray Jui <ray.jui@broadcom.com>
Cc: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>,
	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:45:00 -0500
Message-ID: <20200730164500.GA2046343@bjorn-Precision-5520> (raw)
In-Reply-To: <18c0d88e-8aa0-c0a4-52f7-c9ae9fc5f495@broadcom.com>

On Thu, Jul 30, 2020 at 09:36:14AM -0700, Ray Jui wrote:
> On 7/30/2020 9:09 AM, Bjorn Helgaas wrote:
> > 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?
> 
> I'm in favor of this. I agree with you that we do need the warnings
> because some PCIe config registers that are read/write to clear.
> 
> But the current amount of warning messages generated from these config
> register access is quite massive and often concerns the users who are
> less familiar with the reason/purpose of the warnings. We were asked
> about these warnings by multiple customers. People freaked out when they
> see "corrupt" in the warning messages, :)

Yeah, I'm sure they would.  Hopefully the message makes it all the way
back to the hardware designers ;)

> Limiting the warning to once per device seems to be a reasonable
> compromise to me.

We (you, I mean :)) could also look at the particular warnings.  If
they're triggered by PCI core writes that are 8- or 16-bits when they
*could* be 32-bits, it might make sense to widen them.  I know there
are places that do 8-bit writes to 16-bit registers; maybe there are
similar ones to 32-bit registers.

Bjorn

  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
2020-07-30 16:36     ` Ray Jui
2020-07-30 16:45       ` Bjorn Helgaas [this message]
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=20200730164500.GA2046343@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=ray.jui@broadcom.com \
    --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