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