diff mbox series

[next] PCI: brcmstb: fix a missing if statement on a return error check

Message ID 20200921144017.334602-1-colin.king@canonical.com (mailing list archive)
State Superseded, archived
Headers show
Series [next] PCI: brcmstb: fix a missing if statement on a return error check | expand

Commit Message

Colin King Sept. 21, 2020, 2:40 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

The error return ret is not being check with an if statement and
currently the code always returns leaving the following code as
dead code. Fix this by adding in the missing if statement.

Addresses-Coverity: ("Structurally dead code")
Fixes: ad3d29c77e1e ("PCI: brcmstb: Add control of rescal reset")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Florian Fainelli Sept. 21, 2020, 7:42 p.m. UTC | #1 
On 9/21/20 7:40 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The error return ret is not being check with an if statement and
> currently the code always returns leaving the following code as
> dead code. Fix this by adding in the missing if statement.
> 
> Addresses-Coverity: ("Structurally dead code")
> Fixes: ad3d29c77e1e ("PCI: brcmstb: Add control of rescal reset")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 7a3ff4632e7c..cb0c11b7308e 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1154,6 +1154,7 @@ static int brcm_pcie_resume(struct device *dev)
>  	clk_prepare_enable(pcie->clk);
>  
>  	ret = brcm_phy_start(pcie);
> +	if (ret)
>  		return ret;

Maybe this should also disable the clock if we failed to start the PHY
somehow.
Jim Quinlan Sept. 21, 2020, 8:53 p.m. UTC | #2 
Hello,
I am fine with Colin's suggested change or Florians as well:

         ret = brcm_phy_start(pcie);
+        if (ret) {
+                clk_disable_unprepare(pcie->clk);
                 return ret;
+        }

Currently, our STB upstream suspend/resume is not functional yet and
this is how this omission slipped by testing.

Thanks,
Jim Quinlan
Broadcom STB

On Mon, Sep 21, 2020 at 3:43 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 9/21/20 7:40 AM, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > The error return ret is not being check with an if statement and
> > currently the code always returns leaving the following code as
> > dead code. Fix this by adding in the missing if statement.
> >
> > Addresses-Coverity: ("Structurally dead code")
> > Fixes: ad3d29c77e1e ("PCI: brcmstb: Add control of rescal reset")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 7a3ff4632e7c..cb0c11b7308e 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -1154,6 +1154,7 @@ static int brcm_pcie_resume(struct device *dev)
> >       clk_prepare_enable(pcie->clk);
> >
> >       ret = brcm_phy_start(pcie);
> > +     if (ret)
> >               return ret;
>
> Maybe this should also disable the clock if we failed to start the PHY
> somehow.

Hi Florian,

I'm fine with Colin's change as


>
> --
> Florian
Colin King Sept. 21, 2020, 9:03 p.m. UTC | #3 
On 21/09/2020 21:53, Jim Quinlan wrote:
> Hello,
> I am fine with Colin's suggested change or Florians as well:
> 
>          ret = brcm_phy_start(pcie);
> +        if (ret) {
> +                clk_disable_unprepare(pcie->clk);
>                  return ret;
> +        }
> 
> Currently, our STB upstream suspend/resume is not functional yet and
> this is how this omission slipped by testing.
> 
> Thanks,
> Jim Quinlan
> Broadcom STB
> 
> On Mon, Sep 21, 2020 at 3:43 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> On 9/21/20 7:40 AM, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> The error return ret is not being check with an if statement and
>>> currently the code always returns leaving the following code as
>>> dead code. Fix this by adding in the missing if statement.
>>>
>>> Addresses-Coverity: ("Structurally dead code")
>>> Fixes: ad3d29c77e1e ("PCI: brcmstb: Add control of rescal reset")
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>>  drivers/pci/controller/pcie-brcmstb.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>>> index 7a3ff4632e7c..cb0c11b7308e 100644
>>> --- a/drivers/pci/controller/pcie-brcmstb.c
>>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>>> @@ -1154,6 +1154,7 @@ static int brcm_pcie_resume(struct device *dev)
>>>       clk_prepare_enable(pcie->clk);
>>>
>>>       ret = brcm_phy_start(pcie);
>>> +     if (ret)
>>>               return ret;
>>
>> Maybe this should also disable the clock if we failed to start the PHY
>> somehow.
> 
> Hi Florian,
> 
> I'm fine with Colin's change as
> 

I'll send a V2 in a short while
> 
>>
>> --
>> Florian
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 7a3ff4632e7c..cb0c11b7308e 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1154,6 +1154,7 @@  static int brcm_pcie_resume(struct device *dev)
 	clk_prepare_enable(pcie->clk);
 
 	ret = brcm_phy_start(pcie);
+	if (ret)
 		return ret;
 
 	/* Take bridge out of reset so we can access the SERDES reg */