[PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0

[PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0

[Chris Ruehl]

rockchip_emmc_phy_init() return variable is not set with the error value
if clk_get() failed. The debug message print 0 on error and the function
always return 0.
Fix it using PTR_ERR().

Fixes: 52c0624a10cce phy: rockchip-emmc: Set phyctrl_frqsel based on card clock

Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
---
 drivers/phy/rockchip/phy-rockchip-emmc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 48e2d75b1004..75faee5c0d27 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -253,6 +253,7 @@ static int rockchip_emmc_phy_init(struct phy *phy)
 	 */
 	rk_phy->emmcclk = clk_get(&phy->dev, "emmcclk");
 	if (IS_ERR(rk_phy->emmcclk)) {
+		ret = PTR_ERR(rk_phy->emmcclk);
 		dev_dbg(&phy->dev, "Error getting emmcclk: %d\n", ret);
 		rk_phy->emmcclk = NULL;
 	}
-- 
2.20.1

Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0

[Doug Anderson]

Hi,

On Mon, Nov 30, 2020 at 7:10 PM Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote:
>
> rockchip_emmc_phy_init() return variable is not set with the error value
> if clk_get() failed. The debug message print 0 on error and the function
> always return 0.
> Fix it using PTR_ERR().
>
> Fixes: 52c0624a10cce phy: rockchip-emmc: Set phyctrl_frqsel based on card clock
>
> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> ---
>  drivers/phy/rockchip/phy-rockchip-emmc.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
> index 48e2d75b1004..75faee5c0d27 100644
> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> @@ -253,6 +253,7 @@ static int rockchip_emmc_phy_init(struct phy *phy)
>          */
>         rk_phy->emmcclk = clk_get(&phy->dev, "emmcclk");
>         if (IS_ERR(rk_phy->emmcclk)) {
> +               ret = PTR_ERR(rk_phy->emmcclk);

I'm pretty sure your patch isn't correct and it would break use cases.
Is it fixing some bug that you're aware of, or you found it via code
inspection?

Specifically:

* The big comment block in this function says that the clock is
optional and that we're ignoring errors.

* The printout in this function is "dbg" level, which is an extra
indication that we aren't concerned with these errors.

Arguably the code could be made better.  If you want to improve it,
you could check for just the error we expect if the clock isn't
specified (probably -ENODEV, but you should check) and treat all other
failures as real errors.


-Doug

Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0

[Chris Ruehl]

On 2/12/2020 12:05 am, Doug Anderson wrote:
> Hi,
> 
> On Mon, Nov 30, 2020 at 7:10 PM Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote:
>>
>> rockchip_emmc_phy_init() return variable is not set with the error value
>> if clk_get() failed. The debug message print 0 on error and the function
>> always return 0.
>> Fix it using PTR_ERR().
>>
>> Fixes: 52c0624a10cce phy: rockchip-emmc: Set phyctrl_frqsel based on card clock
>>
>> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
>> ---
>>   drivers/phy/rockchip/phy-rockchip-emmc.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> index 48e2d75b1004..75faee5c0d27 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> @@ -253,6 +253,7 @@ static int rockchip_emmc_phy_init(struct phy *phy)
>>           */
>>          rk_phy->emmcclk = clk_get(&phy->dev, "emmcclk");
>>          if (IS_ERR(rk_phy->emmcclk)) {
>> +               ret = PTR_ERR(rk_phy->emmcclk);
> 
> I'm pretty sure your patch isn't correct and it would break use cases.
> Is it fixing some bug that you're aware of, or you found it via code
> inspection?
> 
> Specifically:
> 
> * The big comment block in this function says that the clock is
> optional and that we're ignoring errors.
> 
> * The printout in this function is "dbg" level, which is an extra
> indication that we aren't concerned with these errors.
> 
> Arguably the code could be made better.  If you want to improve it,
> you could check for just the error we expect if the clock isn't
> specified (probably -ENODEV, but you should check) and treat all other
> failures as real errors.
> 
> 
> -Doug
>

Hi Doug,
  I reviewed the code while hunting behind an other bug, with hs400
and yes I saw the comment that they don't care about the problem
if the clk_get() return an error, and set the rk_phy->emmcclk = NULL
regardless, not using the ret variable but define it isn't useful.

If return a error code break something on the other hand, better it
hit it rather then suppress it in IMHO.

Let me follow the caller of the function and see how they treat the
err != 0.

If something is in danger, I will be effected with my rk3399 rollout :)

Chris

Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0

[Chris Ruehl]

Hi,

On 2/12/2020 4:36 pm, Chris Ruehl wrote:
> 
> On 2/12/2020 12:05 am, Doug Anderson wrote:
>> Hi,
>>
>> On Mon, Nov 30, 2020 at 7:10 PM Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote:
>>>
>>> rockchip_emmc_phy_init() return variable is not set with the error value
>>> if clk_get() failed. The debug message print 0 on error and the function
>>> always return 0.
>>> Fix it using PTR_ERR().
>>>
>>> Fixes: 52c0624a10cce phy: rockchip-emmc: Set phyctrl_frqsel based on card clock
>>>
>>> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
>>> ---
>>>   drivers/phy/rockchip/phy-rockchip-emmc.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
>>> b/drivers/phy/rockchip/phy-rockchip-emmc.c
>>> index 48e2d75b1004..75faee5c0d27 100644
>>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>>> @@ -253,6 +253,7 @@ static int rockchip_emmc_phy_init(struct phy *phy)
>>>           */
>>>          rk_phy->emmcclk = clk_get(&phy->dev, "emmcclk");
>>>          if (IS_ERR(rk_phy->emmcclk)) {
>>> +               ret = PTR_ERR(rk_phy->emmcclk);
>>
>> I'm pretty sure your patch isn't correct and it would break use cases.
>> Is it fixing some bug that you're aware of, or you found it via code
>> inspection?
>>
>> Specifically:
>>
>> * The big comment block in this function says that the clock is
>> optional and that we're ignoring errors.
>>
>> * The printout in this function is "dbg" level, which is an extra
>> indication that we aren't concerned with these errors.
>>
>> Arguably the code could be made better.  If you want to improve it,
>> you could check for just the error we expect if the clock isn't
>> specified (probably -ENODEV, but you should check) and treat all other
>> failures as real errors.
>>
>>
>> -Doug
>>
> 
> Hi Doug,
>   I reviewed the code while hunting behind an other bug, with hs400
> and yes I saw the comment that they don't care about the problem
> if the clk_get() return an error, and set the rk_phy->emmcclk = NULL
> regardless, not using the ret variable but define it isn't useful.
> 
> If return a error code break something on the other hand, better it
> hit it rather then suppress it in IMHO.
> 
> Let me follow the caller of the function and see how they treat the
> err != 0.
> 
> If something is in danger, I will be effected with my rk3399 rollout :)
> 
> Chris
> 

I check my case, the dts properties emmcclk is defined for the rk3399.
(checked it I do not have an error, clk_get() works)
If clk_get() failed and we propagate error<0 to the phy-core.c which then
not increase the phy->init_count, but throw error message that something goes
wrong.

Someone should explain to me, why we should cover up an error.

Chris


Otherwise

Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0

[Doug Anderson]

Hi,

On Thu, Dec 3, 2020 at 7:01 PM Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote:
>
> Hi,
>
> On 2/12/2020 4:36 pm, Chris Ruehl wrote:
> >
> > On 2/12/2020 12:05 am, Doug Anderson wrote:
> >> Hi,
> >>
> >> On Mon, Nov 30, 2020 at 7:10 PM Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote:
> >>>
> >>> rockchip_emmc_phy_init() return variable is not set with the error value
> >>> if clk_get() failed. The debug message print 0 on error and the function
> >>> always return 0.
> >>> Fix it using PTR_ERR().
> >>>
> >>> Fixes: 52c0624a10cce phy: rockchip-emmc: Set phyctrl_frqsel based on card clock
> >>>
> >>> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> >>> ---
> >>>   drivers/phy/rockchip/phy-rockchip-emmc.c | 1 +
> >>>   1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c
> >>> b/drivers/phy/rockchip/phy-rockchip-emmc.c
> >>> index 48e2d75b1004..75faee5c0d27 100644
> >>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> >>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> >>> @@ -253,6 +253,7 @@ static int rockchip_emmc_phy_init(struct phy *phy)
> >>>           */
> >>>          rk_phy->emmcclk = clk_get(&phy->dev, "emmcclk");
> >>>          if (IS_ERR(rk_phy->emmcclk)) {
> >>> +               ret = PTR_ERR(rk_phy->emmcclk);
> >>
> >> I'm pretty sure your patch isn't correct and it would break use cases.
> >> Is it fixing some bug that you're aware of, or you found it via code
> >> inspection?
> >>
> >> Specifically:
> >>
> >> * The big comment block in this function says that the clock is
> >> optional and that we're ignoring errors.
> >>
> >> * The printout in this function is "dbg" level, which is an extra
> >> indication that we aren't concerned with these errors.
> >>
> >> Arguably the code could be made better.  If you want to improve it,
> >> you could check for just the error we expect if the clock isn't
> >> specified (probably -ENODEV, but you should check) and treat all other
> >> failures as real errors.
> >>
> >>
> >> -Doug
> >>
> >
> > Hi Doug,
> >   I reviewed the code while hunting behind an other bug, with hs400
> > and yes I saw the comment that they don't care about the problem
> > if the clk_get() return an error, and set the rk_phy->emmcclk = NULL
> > regardless, not using the ret variable but define it isn't useful.
> >
> > If return a error code break something on the other hand, better it
> > hit it rather then suppress it in IMHO.
> >
> > Let me follow the caller of the function and see how they treat the
> > err != 0.
> >
> > If something is in danger, I will be effected with my rk3399 rollout :)
> >
> > Chris
> >
>
> I check my case, the dts properties emmcclk is defined for the rk3399.
> (checked it I do not have an error, clk_get() works)
> If clk_get() failed and we propagate error<0 to the phy-core.c which then
> not increase the phy->init_count, but throw error message that something goes
> wrong.
>
> Someone should explain to me, why we should cover up an error.

If you want to change this I'd guess you'd want to change the bindings
in a backward-incompatible way.  The bindings
(Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt) say:

 - clock-names: Should contain "emmcclk".  Although this is listed as optional
                (because most boards can get basic functionality without having
                access to it), it is strongly suggested.

Since it's officially listed as optional then the driver needs to
function without it.  It was a long time ago, but my guess about why I
needed to list it as optional was for the whole "device tree purity"
reasons.  In other words the goal is always that old device trees
should continue to work, so if you're adding new properties they need
to be optional.

It seems highly unlikely anyone actually has a device tree that
doesn't specify this clock, though.  If you want to cleanly go through
and mark this as required everywhere I won't personally object.

-Doug