linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: Ryder Lee <ryder.lee@mediatek.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, linux-mediatek@lists.infradead.org
Subject: Re: [PATCH] pci: mediatek: fix wrong operator used
Date: Tue, 3 Nov 2020 15:53:03 -0600	[thread overview]
Message-ID: <20201103215303.GA267664@bjorn-Precision-5520> (raw)
In-Reply-To: <2418edb8c8c81bc646ce9c508939dc27e848dcd7.1603817008.git.ryder.lee@mediatek.com>

  $ git log --oneline drivers/pci/controller/pcie-mediatek.c
  ...
  0cccd42e6193 ("PCI: mediatek: Add controller support for MT7629")
  6be22343cc54 ("PCI: mediatek: Get optional clocks with devm_clk_get_optional()")
  ff7a5a0a8562 ("PCI: mediatek: Fix a leaked reference by adding missing of_node_put()")
  cbe3a7728c7a ("PCI: mediatek: Enlarge PCIe2AHB window size to support 4GB DRAM")

Make yours match in capitalization and sentence structure, e.g.,

  PCI: mediatek: Fix ...

On Wed, Oct 28, 2020 at 12:51:48AM +0800, Ryder Lee wrote:
> Fix the issue reported by Coverity:
> Wrong operator used (CONSTANT_EXPRESSION_RESULT) operator_confusion:
> (port->slot << 3) & 7 is always 0 regardless of the values of its operands.
> This occurs as an initializer.

The important thing here is *not* fixing the Coverity warning.  The
important thing is fixing the *bug*.

The bug is that we computed "func" incorrectly.  The commit log should
mention what bad things happened because "func" was incorrect.

  #define PCI_FUNC(devfn)         ((devfn) & 0x07)

  func = PCI_FUNC(port->slot << 3);

So "func" was always 0 before, and from the code, it looks like that
means we only configured FC credits and FTS for function 0, so any
other functions may not have been configured correctly.

And it looks like this only affects MT2701 and MT7623, since those are
the only devices that use mtk_pcie_startup_port().

This is all info that should be in the commit log so users can tell
whether they are affected.

It's nice to still *mention* Coverity since it gave us useful
information, but all we need is a reference like this:

  Addresses-Coverity-ID: 1437218 ("Wrong operator used")

> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index cf4c18f0c25a..1980b01cee35 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -760,7 +760,7 @@ static struct pci_ops mtk_pcie_ops = {
>  static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
>  {
>  	struct mtk_pcie *pcie = port->pcie;
> -	u32 func = PCI_FUNC(port->slot << 3);
> +	u32 func = PCI_FUNC(port->slot);
>  	u32 slot = PCI_SLOT(port->slot << 3);
>  	u32 val;
>  	int err;
> -- 
> 2.18.0

      reply	other threads:[~2020-11-03 21:53 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-27 16:51 Ryder Lee
2020-11-03 21:53 ` Bjorn Helgaas [this message]

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=20201103215303.GA267664@bjorn-Precision-5520 \
    --to=helgaas@kernel.org \
    --cc=bhelgaas@google.com \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=ryder.lee@mediatek.com \
    --subject='Re: [PATCH] pci: mediatek: fix wrong operator used' \
    /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).