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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > ---
> > >   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