| Message ID | 20240911025058.51588-1-qianqiang.liu@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: brcmstb: Fix control flow issue | expand |
On Wed, Sep 11, 2024 at 10:50:59AM +0800, Qianqiang Liu wrote: > The type of "num_inbound_wins" is "u8", so the less-than-zero > comparison of an unsigned value is never true. I think this was fixed slightly differently but with the same effect; please check this to make sure: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n1034 > --- > drivers/pci/controller/pcie-brcmstb.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 55311dc47615..3e4572c3eeb1 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -1090,9 +1090,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) > u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK); > writel(tmp, base + PCIE_MISC_MISC_CTRL); > > - num_inbound_wins = brcm_pcie_get_inbound_wins(pcie, inbound_wins); > - if (num_inbound_wins < 0) > - return num_inbound_wins; > + ret = brcm_pcie_get_inbound_wins(pcie, inbound_wins); > + if (ret < 0) > + return ret; > + > + num_inbound_wins = (u8)ret; > > set_inbound_win_registers(pcie, inbound_wins, num_inbound_wins); > > -- > 2.39.2 >
On Wed, Sep 11, 2024 at 08:44:36AM -0500, Bjorn Helgaas wrote: > On Wed, Sep 11, 2024 at 10:50:59AM +0800, Qianqiang Liu wrote: > > The type of "num_inbound_wins" is "u8", so the less-than-zero > > comparison of an unsigned value is never true. > > I think this was fixed slightly differently but with the same effect; > please check this to make sure: > > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n1034 > > > --- > > drivers/pci/controller/pcie-brcmstb.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > index 55311dc47615..3e4572c3eeb1 100644 > > --- a/drivers/pci/controller/pcie-brcmstb.c > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > @@ -1090,9 +1090,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) > > u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK); > > writel(tmp, base + PCIE_MISC_MISC_CTRL); > > > > - num_inbound_wins = brcm_pcie_get_inbound_wins(pcie, inbound_wins); > > - if (num_inbound_wins < 0) > > - return num_inbound_wins; > > + ret = brcm_pcie_get_inbound_wins(pcie, inbound_wins); > > + if (ret < 0) > > + return ret; > > + > > + num_inbound_wins = (u8)ret; > > > > set_inbound_win_registers(pcie, inbound_wins, num_inbound_wins); > > > > -- > > 2.39.2 > > Yes, they have the same effect.
On Wed, Sep 11, 2024 at 9:56 AM Qianqiang Liu <qianqiang.liu@163.com> wrote: > > On Wed, Sep 11, 2024 at 08:44:36AM -0500, Bjorn Helgaas wrote: > > On Wed, Sep 11, 2024 at 10:50:59AM +0800, Qianqiang Liu wrote: > > > The type of "num_inbound_wins" is "u8", so the less-than-zero > > > comparison of an unsigned value is never true. > > > > I think this was fixed slightly differently but with the same effect; > > please check this to make sure: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n1034 > > > > > --- > > > drivers/pci/controller/pcie-brcmstb.c | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > > index 55311dc47615..3e4572c3eeb1 100644 > > > --- a/drivers/pci/controller/pcie-brcmstb.c > > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > > @@ -1090,9 +1090,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) > > > u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK); > > > writel(tmp, base + PCIE_MISC_MISC_CTRL); > > > > > > - num_inbound_wins = brcm_pcie_get_inbound_wins(pcie, inbound_wins); > > > - if (num_inbound_wins < 0) > > > - return num_inbound_wins; > > > + ret = brcm_pcie_get_inbound_wins(pcie, inbound_wins); > > > + if (ret < 0) > > > + return ret; > > > + > > > + num_inbound_wins = (u8)ret; > > > > > > set_inbound_win_registers(pcie, inbound_wins, num_inbound_wins); > > > > > > -- > > > 2.39.2 > > > > > Yes, they have the same effect. Qianqiang's fixup has the slight advantage of keeping the var a u8, which is what a reviewer wanted in the first place. I found it surprising that "if (u8var < 0)" does not cause a compiler warning, but it appears that W=1 does not include "-Wtype-limits". That's no excuse for this mistake, of course :-( Thanks and regards, Jim Quinlan Broadcom STB/CM > > -- > Best, > Qianqiang Liu >
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 55311dc47615..3e4572c3eeb1 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -1090,9 +1090,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK); writel(tmp, base + PCIE_MISC_MISC_CTRL); - num_inbound_wins = brcm_pcie_get_inbound_wins(pcie, inbound_wins); - if (num_inbound_wins < 0) - return num_inbound_wins; + ret = brcm_pcie_get_inbound_wins(pcie, inbound_wins); + if (ret < 0) + return ret; + + num_inbound_wins = (u8)ret; set_inbound_win_registers(pcie, inbound_wins, num_inbound_wins);
The type of "num_inbound_wins" is "u8", so the less-than-zero comparison of an unsigned value is never true. Signed-off-by: Qianqiang Liu <qianqiang.liu@163.com> --- drivers/pci/controller/pcie-brcmstb.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)