| Message ID | CAPgLHd_JqYS5DC4tmzgGVEYmJpeM96FAEDvmM3cUUep+1jj7mA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 21, 2013 at 11:34:07AM +0800, Wei Yongjun wrote: > From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > In case of error, the function clk_get_parent() and devm_ioremap_resource() > returns ERR_PTR() and never returns NULL. The NULL test in the return value > check should be replaced with IS_ERR(). > > Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > --- > drivers/gpu/drm/tegra/dsi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) I've applied this, but with the first hunk removed, since looking at the implementation of clk_get_parent() it can actually return NULL. In fact it seems like it will never return ERR_PTR(). I've also updated the commit message to reflect that. Thierry > > diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c > index 1cfbace..7bc2eeb 100644 > --- a/drivers/gpu/drm/tegra/dsi.c > +++ b/drivers/gpu/drm/tegra/dsi.c > @@ -914,7 +914,7 @@ static int tegra_dsi_setup_clocks(struct tegra_dsi *dsi) > int err; > > parent = clk_get_parent(dsi->clk); > - if (!parent) > + if (IS_ERR(parent)) > return -EINVAL; > > err = clk_set_parent(parent, dsi->clk_parent); > @@ -969,8 +969,8 @@ static int tegra_dsi_probe(struct platform_device *pdev) > > regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); > dsi->regs = devm_ioremap_resource(&pdev->dev, regs); > - if (!dsi->regs) > - return -EADDRNOTAVAIL; > + if (IS_ERR(dsi->regs)) > + return PTR_ERR(dsi->regs); > > INIT_LIST_HEAD(&dsi->client.list); > dsi->client.ops = &dsi_client_ops; >
On 10/28/2013 02:53 AM, Thierry Reding wrote: > On Mon, Oct 21, 2013 at 11:34:07AM +0800, Wei Yongjun wrote: >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >> >> In case of error, the function clk_get_parent() and >> devm_ioremap_resource() returns ERR_PTR() and never returns NULL. >> The NULL test in the return value check should be replaced with >> IS_ERR(). >> >> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> --- >> drivers/gpu/drm/tegra/dsi.c | 6 +++--- 1 file changed, 3 >> insertions(+), 3 deletions(-) > > I've applied this, but with the first hunk removed, since looking > at the implementation of clk_get_parent() it can actually return > NULL. In fact it seems like it will never return ERR_PTR(). > > I've also updated the commit message to reflect that. Hmm. The documentation for clk_get() says: /** * clk_get - lookup and obtain a reference to a clock producer. * @dev: device for clock "consumer" * @id: clock consumer ID * * Returns a struct clk corresponding to the clock producer, or * valid IS_ERR() condition containing errno. The implementation * uses @dev and @id to determine the clock consumer, and thereby * the clock producer. (IOW, @id may be identical strings, but * clk_get may return different clock producers depending on @dev.) If the implementation doesn't match that, then it's a bug, and a whole slew of drivers will need changing... On the surface, it looks like the hunk you dropped was correct. NULL may be a perfectly legal return value from a function that returns either valid data or an IS_ERR() value.
On Mon, Oct 28, 2013 at 03:51:32PM -0600, Stephen Warren wrote: > On 10/28/2013 02:53 AM, Thierry Reding wrote: > > On Mon, Oct 21, 2013 at 11:34:07AM +0800, Wei Yongjun wrote: > >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > >> > >> In case of error, the function clk_get_parent() and > >> devm_ioremap_resource() returns ERR_PTR() and never returns NULL. > >> The NULL test in the return value check should be replaced with > >> IS_ERR(). > >> > >> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> --- > >> drivers/gpu/drm/tegra/dsi.c | 6 +++--- 1 file changed, 3 > >> insertions(+), 3 deletions(-) > > > > I've applied this, but with the first hunk removed, since looking > > at the implementation of clk_get_parent() it can actually return > > NULL. In fact it seems like it will never return ERR_PTR(). > > > > I've also updated the commit message to reflect that. > > Hmm. The documentation for clk_get() says: The patch didn't check the return value clk_get() but clk_get_parent(). Here's the implementation: struct clk *__clk_get_parent(struct clk *clk) { return !clk ? NULL : clk->parent; } Note that clk_get_parent() in simply a locked version of the above. That will obviously only return ERR_PTR() if clk->parent happens to be set to one such value, which I don't think will ever happen. Thierry
On 10/28/2013 04:38 PM, Thierry Reding wrote: > On Mon, Oct 28, 2013 at 03:51:32PM -0600, Stephen Warren wrote: >> On 10/28/2013 02:53 AM, Thierry Reding wrote: >>> On Mon, Oct 21, 2013 at 11:34:07AM +0800, Wei Yongjun wrote: >>>> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >>>> >>>> In case of error, the function clk_get_parent() and >>>> devm_ioremap_resource() returns ERR_PTR() and never returns >>>> NULL. The NULL test in the return value check should be >>>> replaced with IS_ERR(). >>>> >>>> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >>>> --- drivers/gpu/drm/tegra/dsi.c | 6 +++--- 1 file changed, 3 >>>> insertions(+), 3 deletions(-) >>> >>> I've applied this, but with the first hunk removed, since >>> looking at the implementation of clk_get_parent() it can >>> actually return NULL. In fact it seems like it will never >>> return ERR_PTR(). >>> >>> I've also updated the commit message to reflect that. >> >> Hmm. The documentation for clk_get() says: > > The patch didn't check the return value clk_get() but > clk_get_parent(). Here's the implementation: > > struct clk *__clk_get_parent(struct clk *clk) { return !clk ? NULL > : clk->parent; } > > Note that clk_get_parent() in simply a locked version of the above. > That will obviously only return ERR_PTR() if clk->parent happens to > be set to one such value, which I don't think will ever happen. Ah. That looks like a bug in __clk_get_parent() then, since !clk doesn't sound like the correct error case for it to be checking. Shouldn't it return IS_ERR(clk) ? clk : clk->parent? Either that, or clk_get() shouldn't return an error value if the rest of the clock code doesn NULL checks.
On Mon, Oct 28, 2013 at 04:48:40PM -0600, Stephen Warren wrote: > On 10/28/2013 04:38 PM, Thierry Reding wrote: > > On Mon, Oct 28, 2013 at 03:51:32PM -0600, Stephen Warren wrote: > >> On 10/28/2013 02:53 AM, Thierry Reding wrote: > >>> On Mon, Oct 21, 2013 at 11:34:07AM +0800, Wei Yongjun wrote: > >>>> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > >>>> > >>>> In case of error, the function clk_get_parent() and > >>>> devm_ioremap_resource() returns ERR_PTR() and never returns > >>>> NULL. The NULL test in the return value check should be > >>>> replaced with IS_ERR(). > >>>> > >>>> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > >>>> --- drivers/gpu/drm/tegra/dsi.c | 6 +++--- 1 file changed, 3 > >>>> insertions(+), 3 deletions(-) > >>> > >>> I've applied this, but with the first hunk removed, since > >>> looking at the implementation of clk_get_parent() it can > >>> actually return NULL. In fact it seems like it will never > >>> return ERR_PTR(). > >>> > >>> I've also updated the commit message to reflect that. > >> > >> Hmm. The documentation for clk_get() says: > > > > The patch didn't check the return value clk_get() but > > clk_get_parent(). Here's the implementation: > > > > struct clk *__clk_get_parent(struct clk *clk) { return !clk ? NULL > > : clk->parent; } > > > > Note that clk_get_parent() in simply a locked version of the above. > > That will obviously only return ERR_PTR() if clk->parent happens to > > be set to one such value, which I don't think will ever happen. > > Ah. That looks like a bug in __clk_get_parent() then, since !clk > doesn't sound like the correct error case for it to be checking. > Shouldn't it return IS_ERR(clk) ? clk : clk->parent? Either that, or > clk_get() shouldn't return an error value if the rest of the clock > code doesn NULL checks. Yes, that would seem to be more consistent. Then again, one could argue that if somebody got an invalid (ERR_PTR()) clock from clk_get(), they shouldn't be calling clk_get_parent() on it in the first place. But the same would be true for NULL, so... Looping in Mike. Thierry
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c index 1cfbace..7bc2eeb 100644 --- a/drivers/gpu/drm/tegra/dsi.c +++ b/drivers/gpu/drm/tegra/dsi.c @@ -914,7 +914,7 @@ static int tegra_dsi_setup_clocks(struct tegra_dsi *dsi) int err; parent = clk_get_parent(dsi->clk); - if (!parent) + if (IS_ERR(parent)) return -EINVAL; err = clk_set_parent(parent, dsi->clk_parent); @@ -969,8 +969,8 @@ static int tegra_dsi_probe(struct platform_device *pdev) regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); dsi->regs = devm_ioremap_resource(&pdev->dev, regs); - if (!dsi->regs) - return -EADDRNOTAVAIL; + if (IS_ERR(dsi->regs)) + return PTR_ERR(dsi->regs); INIT_LIST_HEAD(&dsi->client.list); dsi->client.ops = &dsi_client_ops;