Messages in this thread | | | Subject | Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 | From | Chris Ruehl <> | Date | Fri, 4 Dec 2020 11:01:40 +0800 |
| |
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
| |