* 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 related [flat|nested] 2+ messages in thread