| Message ID | 20200918094642.108070-1-zhangqilong3@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [-next] clk: tegra: clk-dfll: indicate correct error reason in tegra_dfll_register | expand |
Quoting Qilong Zhang (2020-09-18 02:46:42) > From: Zhang Qilong <zhangqilong3@huawei.com> > > Calling devm_ioremap means getting devices resource have been > successful. When remap operation failed, we should return '-ENOMEM' > instead of '-ENODEV' to differentiate between getting resource and > mapping memory for reminding callers. Moreover, it is not consistent > with devm_kzalloc operation. > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > --- > drivers/clk/tegra/clk-dfll.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c > index cfbaa90c7adb..6637b73be9f1 100644 > --- a/drivers/clk/tegra/clk-dfll.c > +++ b/drivers/clk/tegra/clk-dfll.c > @@ -1993,7 +1993,7 @@ int tegra_dfll_register(struct platform_device *pdev, > td->base = devm_ioremap(td->dev, mem->start, resource_size(mem)); > if (!td->base) { > dev_err(td->dev, "couldn't ioremap DFLL control registers\n"); > - return -ENODEV; > + return -ENOMEM; Can you remove the dev_err() lines too? They're pretty much useless.
On Tue, Sep 22, 2020 at 12:57:46PM -0700, Stephen Boyd wrote: > Quoting Qilong Zhang (2020-09-18 02:46:42) > > From: Zhang Qilong <zhangqilong3@huawei.com> > > > > Calling devm_ioremap means getting devices resource have been > > successful. When remap operation failed, we should return '-ENOMEM' > > instead of '-ENODEV' to differentiate between getting resource and > > mapping memory for reminding callers. Moreover, it is not consistent > > with devm_kzalloc operation. > > > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > > --- > > drivers/clk/tegra/clk-dfll.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c > > index cfbaa90c7adb..6637b73be9f1 100644 > > --- a/drivers/clk/tegra/clk-dfll.c > > +++ b/drivers/clk/tegra/clk-dfll.c > > @@ -1993,7 +1993,7 @@ int tegra_dfll_register(struct platform_device *pdev, > > td->base = devm_ioremap(td->dev, mem->start, resource_size(mem)); > > if (!td->base) { > > dev_err(td->dev, "couldn't ioremap DFLL control registers\n"); > > - return -ENODEV; > > + return -ENOMEM; > > Can you remove the dev_err() lines too? They're pretty much useless. I find them somewhat useful because they indicate which particular resource wasn't properly mapped. If we get an -ENOMEM without the error message, we'll have to go and guess which one it is. Thierry
Quoting Thierry Reding (2020-09-23 01:16:54) > On Tue, Sep 22, 2020 at 12:57:46PM -0700, Stephen Boyd wrote: > > Quoting Qilong Zhang (2020-09-18 02:46:42) > > > From: Zhang Qilong <zhangqilong3@huawei.com> > > > > > > Calling devm_ioremap means getting devices resource have been > > > successful. When remap operation failed, we should return '-ENOMEM' > > > instead of '-ENODEV' to differentiate between getting resource and > > > mapping memory for reminding callers. Moreover, it is not consistent > > > with devm_kzalloc operation. > > > > > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > > > --- > > > drivers/clk/tegra/clk-dfll.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c > > > index cfbaa90c7adb..6637b73be9f1 100644 > > > --- a/drivers/clk/tegra/clk-dfll.c > > > +++ b/drivers/clk/tegra/clk-dfll.c > > > @@ -1993,7 +1993,7 @@ int tegra_dfll_register(struct platform_device *pdev, > > > td->base = devm_ioremap(td->dev, mem->start, resource_size(mem)); > > > if (!td->base) { > > > dev_err(td->dev, "couldn't ioremap DFLL control registers\n"); > > > - return -ENODEV; > > > + return -ENOMEM; > > > > Can you remove the dev_err() lines too? They're pretty much useless. > > I find them somewhat useful because they indicate which particular > resource wasn't properly mapped. If we get an -ENOMEM without the error > message, we'll have to go and guess which one it is. > Doesn't that print the stacktrace when we run out of memory?
On Wed, Sep 23, 2020 at 04:32:40PM -0700, Stephen Boyd wrote: > Quoting Thierry Reding (2020-09-23 01:16:54) > > On Tue, Sep 22, 2020 at 12:57:46PM -0700, Stephen Boyd wrote: > > > Quoting Qilong Zhang (2020-09-18 02:46:42) > > > > From: Zhang Qilong <zhangqilong3@huawei.com> > > > > > > > > Calling devm_ioremap means getting devices resource have been > > > > successful. When remap operation failed, we should return '-ENOMEM' > > > > instead of '-ENODEV' to differentiate between getting resource and > > > > mapping memory for reminding callers. Moreover, it is not consistent > > > > with devm_kzalloc operation. > > > > > > > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> > > > > --- > > > > drivers/clk/tegra/clk-dfll.c | 8 ++++---- > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c > > > > index cfbaa90c7adb..6637b73be9f1 100644 > > > > --- a/drivers/clk/tegra/clk-dfll.c > > > > +++ b/drivers/clk/tegra/clk-dfll.c > > > > @@ -1993,7 +1993,7 @@ int tegra_dfll_register(struct platform_device *pdev, > > > > td->base = devm_ioremap(td->dev, mem->start, resource_size(mem)); > > > > if (!td->base) { > > > > dev_err(td->dev, "couldn't ioremap DFLL control registers\n"); > > > > - return -ENODEV; > > > > + return -ENOMEM; > > > > > > Can you remove the dev_err() lines too? They're pretty much useless. > > > > I find them somewhat useful because they indicate which particular > > resource wasn't properly mapped. If we get an -ENOMEM without the error > > message, we'll have to go and guess which one it is. > > > > Doesn't that print the stacktrace when we run out of memory? slab allocator functions like kmalloc() and friends do, but I'm not aware of ioremap() doing so. Perhaps if it runs out of virtual memory for the mapping it would, but there are other reasons why this can fail. Thierry
Quoting Thierry Reding (2020-09-24 00:17:06) > On Wed, Sep 23, 2020 at 04:32:40PM -0700, Stephen Boyd wrote: > > Quoting Thierry Reding (2020-09-23 01:16:54) > > > On Tue, Sep 22, 2020 at 12:57:46PM -0700, Stephen Boyd wrote: > > > > Quoting Qilong Zhang (2020-09-18 02:46:42) > > > > > td->base = devm_ioremap(td->dev, mem->start, resource_size(mem)); > > > > > if (!td->base) { > > > > > dev_err(td->dev, "couldn't ioremap DFLL control registers\n"); > > > > > - return -ENODEV; > > > > > + return -ENOMEM; > > > > > > > > Can you remove the dev_err() lines too? They're pretty much useless. > > > > > > I find them somewhat useful because they indicate which particular > > > resource wasn't properly mapped. If we get an -ENOMEM without the error > > > message, we'll have to go and guess which one it is. > > > > > > > Doesn't that print the stacktrace when we run out of memory? > > slab allocator functions like kmalloc() and friends do, but I'm not > aware of ioremap() doing so. Perhaps if it runs out of virtual memory > for the mapping it would, but there are other reasons why this can fail. > Ok, but the other failure modes are going to happen outside of developing the driver? What failure of ioremap() are you trying to catch? And even if knowing which resource didn't map properly, wouldn't we be better off moving the error message closer to the actual problem in ioremap() so it can tell us the problem that was seen? Otherwise I fear the driver is going to tell the user that some error happened but we won't be able to really figure out what the error is.
diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c index cfbaa90c7adb..6637b73be9f1 100644 --- a/drivers/clk/tegra/clk-dfll.c +++ b/drivers/clk/tegra/clk-dfll.c @@ -1993,7 +1993,7 @@ int tegra_dfll_register(struct platform_device *pdev, td->base = devm_ioremap(td->dev, mem->start, resource_size(mem)); if (!td->base) { dev_err(td->dev, "couldn't ioremap DFLL control registers\n"); - return -ENODEV; + return -ENOMEM; } mem = platform_get_resource(pdev, IORESOURCE_MEM, 1); @@ -2005,7 +2005,7 @@ int tegra_dfll_register(struct platform_device *pdev, td->i2c_base = devm_ioremap(td->dev, mem->start, resource_size(mem)); if (!td->i2c_base) { dev_err(td->dev, "couldn't ioremap i2c_base resource\n"); - return -ENODEV; + return -ENOMEM; } mem = platform_get_resource(pdev, IORESOURCE_MEM, 2); @@ -2019,7 +2019,7 @@ int tegra_dfll_register(struct platform_device *pdev, if (!td->i2c_controller_base) { dev_err(td->dev, "couldn't ioremap i2c_controller_base resource\n"); - return -ENODEV; + return -ENOMEM; } mem = platform_get_resource(pdev, IORESOURCE_MEM, 3); @@ -2032,7 +2032,7 @@ int tegra_dfll_register(struct platform_device *pdev, if (!td->lut_base) { dev_err(td->dev, "couldn't ioremap lut_base resource\n"); - return -ENODEV; + return -ENOMEM; } ret = dfll_init_clks(td);