Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
* pci-meson covery issue #1442509
@ 2020-10-27 16:40 Bjorn Helgaas
  0 siblings, 0 replies; 2+ messages in thread
From: Bjorn Helgaas @ 2020-10-27 16:40 UTC (permalink / raw)
  To: Yue Wang; +Cc: linux-pci, Kevin Hilman, linux-amlogic

Hi Yue,

Please take a look at this issue reported by Coverity:

340 static int meson_pcie_link_up(struct dw_pcie *pci)
341 {
342        struct meson_pcie *mp = to_meson_pcie(pci);
343        struct device *dev = pci->dev;
344        u32 speed_okay = 0;
345        u32 cnt = 0;
346        u32 state12, state17, smlh_up, ltssm_up, rdlh_up;
347
348        do {
349                state12 = meson_cfg_readl(mp, PCIE_CFG_STATUS12);
350                state17 = meson_cfg_readl(mp, PCIE_CFG_STATUS17);
351                smlh_up = IS_SMLH_LINK_UP(state12);
352                rdlh_up = IS_RDLH_LINK_UP(state12);
353                ltssm_up = IS_LTSSM_UP(state12);
354

CID 1442509 (#1 of 1): Operands don't affect result
(CONSTANT_EXPRESSION_RESULT) result_independent_of_operands: ((state17
>> 7) & 1) < PCIE_GEN3 is always true regardless of the values of its
operands. This occurs as the logical operand of if.

355                if (PM_CURRENT_STATE(state17) < PCIE_GEN3)
356                        speed_okay = 1;


"PM" seems like a funny name for a link speed.  It sounds more like
something related to power management, e.g., D0, D3.

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: pci-meson covery issue #1442509
       [not found] <2020102810175892085918@amlogic.com>
@ 2020-10-28 12:06 ` Bjorn Helgaas
  0 siblings, 0 replies; 2+ messages in thread
From: Bjorn Helgaas @ 2020-10-28 12:06 UTC (permalink / raw)
  To: yue.wang; +Cc: linux-pci, Kevin Hilman, linux-amlogic

It's best if you can reply with plain-text email, because I don't
think multi-part MIME messages are accepted by the mailing lists:
http://vger.kernel.org/majordomo-info.html

On Wed, Oct 28, 2020 at 10:17:59AM +0800, yue.wang@amlogic.com wrote:
> HI Bjorn,
> 
> amlogic PCIE_CFG_STATUS17 register:
> 
> and bit7 mac_phy_rate :
> 
> Amlogic pcie working mode is only GEN1 & GEN2,so mac_phy_rate is 0 or 1;
> 
> PM_CURRENT_STATE(state17)  is related to Amlogic pcie working mode,and  not  related to power management.
> 
> "PM_CURRENT_STATE(state17) < PCIE_GEN3"  is always true. 

There's no point in making a comparision that's always true.  It just
clutters the code without adding value.  IMO you should do something
like the following.  If and when you actually *need* to check the
mac_phy_rate and there's a possibility that it might not be OK, you
can add it back.

diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
index 1913dc2c8fa0..bb7a35c7c57f 100644
--- a/drivers/pci/controller/dwc/pci-meson.c
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -35,9 +35,6 @@
 #define IS_RDLH_LINK_UP(x)		((x) & (1 << 16))
 #define IS_LTSSM_UP(x)			((((x) >> 10) & 0x1f) == 0x11)
 
-#define PCIE_CFG_STATUS17		0x44
-#define PM_CURRENT_STATE(x)		(((x) >> 7) & 0x1)
-
 #define WAIT_LINKUP_TIMEOUT		4000
 #define PORT_CLK_RATE			100000000UL
 #define MAX_PAYLOAD_SIZE		256
@@ -341,30 +338,23 @@ static int meson_pcie_link_up(struct dw_pcie *pci)
 {
 	struct meson_pcie *mp = to_meson_pcie(pci);
 	struct device *dev = pci->dev;
-	u32 speed_okay = 0;
 	u32 cnt = 0;
-	u32 state12, state17, smlh_up, ltssm_up, rdlh_up;
+	u32 state12, smlh_up, ltssm_up, rdlh_up;
 
 	do {
 		state12 = meson_cfg_readl(mp, PCIE_CFG_STATUS12);
-		state17 = meson_cfg_readl(mp, PCIE_CFG_STATUS17);
 		smlh_up = IS_SMLH_LINK_UP(state12);
 		rdlh_up = IS_RDLH_LINK_UP(state12);
 		ltssm_up = IS_LTSSM_UP(state12);
 
-		if (PM_CURRENT_STATE(state17) < PCIE_GEN3)
-			speed_okay = 1;
-
 		if (smlh_up)
 			dev_dbg(dev, "smlh_link_up is on\n");
 		if (rdlh_up)
 			dev_dbg(dev, "rdlh_link_up is on\n");
 		if (ltssm_up)
 			dev_dbg(dev, "ltssm_up is on\n");
-		if (speed_okay)
-			dev_dbg(dev, "speed_okay\n");
 
-		if (smlh_up && rdlh_up && ltssm_up && speed_okay)
+		if (smlh_up && rdlh_up && ltssm_up)
 			return 1;
 
 		cnt++;

> yue.wang@amlogic.com
>  
> From: Bjorn Helgaas
> Date: 2020-10-28 00:40
> To: Yue Wang
> CC: linux-pci; Kevin Hilman; linux-amlogic
> Subject: pci-meson covery issue #1442509
> Hi Yue,
>  
> Please take a look at this issue reported by Coverity:
>  
> 340 static int meson_pcie_link_up(struct dw_pcie *pci)
> 341 {
> 342        struct meson_pcie *mp = to_meson_pcie(pci);
> 343        struct device *dev = pci->dev;
> 344        u32 speed_okay = 0;
> 345        u32 cnt = 0;
> 346        u32 state12, state17, smlh_up, ltssm_up, rdlh_up;
> 347
> 348        do {
> 349                state12 = meson_cfg_readl(mp, PCIE_CFG_STATUS12);
> 350                state17 = meson_cfg_readl(mp, PCIE_CFG_STATUS17);
> 351                smlh_up = IS_SMLH_LINK_UP(state12);
> 352                rdlh_up = IS_RDLH_LINK_UP(state12);
> 353                ltssm_up = IS_LTSSM_UP(state12);
> 354
>  
> CID 1442509 (#1 of 1): Operands don't affect result
> (CONSTANT_EXPRESSION_RESULT) result_independent_of_operands: ((state17
> >> 7) & 1) < PCIE_GEN3 is always true regardless of the values of its
> operands. This occurs as the logical operand of if.
>  
> 355                if (PM_CURRENT_STATE(state17) < PCIE_GEN3)
> 356                        speed_okay = 1;
>  
>  
> "PM" seems like a funny name for a link speed.  It sounds more like
> something related to power management, e.g., D0, D3.
>  




^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-27 16:40 pci-meson covery issue #1442509 Bjorn Helgaas
     [not found] <2020102810175892085918@amlogic.com>
2020-10-28 12:06 ` Bjorn Helgaas

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