Re: pci-meson covery issue #1442509

[Bjorn Helgaas <helgaas@kernel.org>]

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.
>