linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thierry Reding <thierry.reding@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Vidya Sagar <vidyas@nvidia.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Krishna Kishore <kthota@nvidia.com>,
	Manikanta Maddireddy <mmaddireddy@nvidia.com>,
	Vidya Sagar <sagar.tv@gmail.com>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [PATCH v3 1/3] PCI/MSI: Move MSI/MSI-X init to msi.c
Date: Fri, 4 Dec 2020 12:05:15 +0100	[thread overview]
Message-ID: <X8oX61zRwV7ykLAy@ulmo> (raw)
In-Reply-To: <20201203185110.1583077-2-helgaas@kernel.org>

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

On Thu, Dec 03, 2020 at 12:51:08PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Move pci_msi_setup_pci_dev(), which disables MSI and MSI-X interrupts, from
> probe.c to msi.c so it's with all the other MSI code and more consistent
> with other capability initialization.  This means we must compile msi.c
> always, even without CONFIG_PCI_MSI, so wrap the rest of msi.c in an #ifdef
> and adjust the Makefile accordingly.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/Makefile |  3 +--
>  drivers/pci/msi.c    | 36 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h    |  2 ++
>  drivers/pci/probe.c  | 21 ++-------------------
>  4 files changed, 41 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 522d2b974e91..11cc79411e2d 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -5,7 +5,7 @@
>  obj-$(CONFIG_PCI)		+= access.o bus.o probe.o host-bridge.o \
>  				   remove.o pci.o pci-driver.o search.o \
>  				   pci-sysfs.o rom.o setup-res.o irq.o vpd.o \
> -				   setup-bus.o vc.o mmap.o setup-irq.o
> +				   setup-bus.o vc.o mmap.o setup-irq.o msi.o
>  
>  obj-$(CONFIG_PCI)		+= pcie/
>  
> @@ -18,7 +18,6 @@ endif
>  obj-$(CONFIG_OF)		+= of.o
>  obj-$(CONFIG_PCI_QUIRKS)	+= quirks.o
>  obj-$(CONFIG_HOTPLUG_PCI)	+= hotplug/
> -obj-$(CONFIG_PCI_MSI)		+= msi.o
>  obj-$(CONFIG_PCI_ATS)		+= ats.o
>  obj-$(CONFIG_PCI_IOV)		+= iov.o
>  obj-$(CONFIG_PCI_BRIDGE_EMUL)	+= pci-bridge-emul.o
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index d52d118979a6..555791c0ee1a 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -26,6 +26,8 @@
>  
>  #include "pci.h"
>  
> +#ifdef CONFIG_MSI
> +
>  static int pci_msi_enable = 1;
>  int pci_msi_ignore_mask;
>  
> @@ -1577,3 +1579,37 @@ bool pci_dev_has_special_msi_domain(struct pci_dev *pdev)
>  }
>  
>  #endif /* CONFIG_PCI_MSI_IRQ_DOMAIN */
> +#endif /* CONFIG_PCI_MSI */
> +
> +void pci_msi_init(struct pci_dev *dev)
> +{
> +	u16 ctrl;
> +
> +	/*
> +	 * Disable the MSI hardware to avoid screaming interrupts
> +	 * during boot.  This is the power on reset default so
> +	 * usually this should be a noop.
> +	 */
> +	dev->msi_cap = pci_find_capability(dev, PCI_CAP_ID_MSI);
> +	if (!dev->msi_cap)
> +		return;
> +
> +	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &ctrl);
> +	if (ctrl & PCI_MSI_FLAGS_ENABLE)
> +		pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS,
> +				      ctrl & ~PCI_MSI_FLAGS_ENABLE);
> +}

The old code used the pci_msi_set_enable() helper here...

> +
> +void pci_msix_init(struct pci_dev *dev)
> +{
> +	u16 ctrl;
> +
> +	dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);
> +	if (!dev->msix_cap)
> +		return;
> +
> +	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &ctrl);
> +	if (ctrl & PCI_MSIX_FLAGS_ENABLE)
> +		pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS,
> +				      ctrl & ~PCI_MSIX_FLAGS_ENABLE);
> +}

... and pci_msix_clear_and_set_ctrl() here. I like your version here
better because it avoids the unnecessary write in case the flag isn't
set. But it got me thinking if perhaps the helpers aren't very useful
and perhaps should be dropped in favour of open-coded variants.
Especially since there seem to be only 4 and 6 occurrences of them after
this patch.

Anyway, this patch looks correct to me and is a nice improvement, so:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2020-12-04 11:06 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-03 18:51 [PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking Bjorn Helgaas
2020-12-03 18:51 ` [PATCH v3 1/3] PCI/MSI: Move MSI/MSI-X init to msi.c Bjorn Helgaas
2020-12-04 11:05   ` Thierry Reding [this message]
2020-12-04 18:11     ` Bjorn Helgaas
2020-12-03 18:51 ` [PATCH v3 2/3] PCI/MSI: Move MSI/MSI-X flags updaters " Bjorn Helgaas
2020-12-04 11:06   ` Thierry Reding
2020-12-03 18:51 ` [PATCH v3 3/3] PCI/MSI: Set device flag indicating only 32-bit MSI support Bjorn Helgaas
2020-12-04 11:07   ` Thierry Reding
2020-12-04 18:21 ` [PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking 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=X8oX61zRwV7ykLAy@ulmo \
    --to=thierry.reding@gmail.com \
    --cc=bhelgaas@google.com \
    --cc=helgaas@kernel.org \
    --cc=jonathanh@nvidia.com \
    --cc=kthota@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mmaddireddy@nvidia.com \
    --cc=sagar.tv@gmail.com \
    --cc=vidyas@nvidia.com \
    --subject='Re: [PATCH v3 1/3] PCI/MSI: Move MSI/MSI-X init to msi.c' \
    /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).