| Message ID | 20210508220021.1778080-17-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Fix various coverity warnings | expand |
On 5/8/21 6:00 PM, Simon Glass wrote: > When the default clocks cannot be set, the clock is silently probed and > the error is ignored. This is incorrect, since having the clocks at the > correct speed may be important for operation of the system. > > Fix it by checking the return code. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > drivers/clk/clk-uclass.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c > index 4ab3c402ed8..2a2e1cfbd61 100644 > --- a/drivers/clk/clk-uclass.c > +++ b/drivers/clk/clk-uclass.c > @@ -796,13 +796,17 @@ void devm_clk_put(struct udevice *dev, struct clk *clk) > > int clk_uclass_post_probe(struct udevice *dev) > { > + int ret; > + > /* > * when a clock provider is probed. Call clk_set_defaults() > * also after the device is probed. This takes care of cases > * where the DT is used to setup default parents and rates > * using assigned-clocks > */ > - clk_set_defaults(dev, 1); > + ret = clk_set_defaults(dev, 1); > + if (ret) > + return log_ret(ret); > > return 0; > } > See also: https://patchwork.ozlabs.org/project/uboot/patch/20210409021313.433558-2-seanga2@gmail.com/ Reviewed-by: Sean Anderson <seanga2@gmail.com>
Hi Sean, On Sat, 8 May 2021 at 18:40, Sean Anderson <seanga2@gmail.com> wrote: > > On 5/8/21 6:00 PM, Simon Glass wrote: > > When the default clocks cannot be set, the clock is silently probed and > > the error is ignored. This is incorrect, since having the clocks at the > > correct speed may be important for operation of the system. > > > > Fix it by checking the return code. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > drivers/clk/clk-uclass.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c > > index 4ab3c402ed8..2a2e1cfbd61 100644 > > --- a/drivers/clk/clk-uclass.c > > +++ b/drivers/clk/clk-uclass.c > > @@ -796,13 +796,17 @@ void devm_clk_put(struct udevice *dev, struct clk *clk) > > > > int clk_uclass_post_probe(struct udevice *dev) > > { > > + int ret; > > + > > /* > > * when a clock provider is probed. Call clk_set_defaults() > > * also after the device is probed. This takes care of cases > > * where the DT is used to setup default parents and rates > > * using assigned-clocks > > */ > > - clk_set_defaults(dev, 1); > > + ret = clk_set_defaults(dev, 1); > > + if (ret) > > + return log_ret(ret); > > > > return 0; > > } > > > > See also: https://patchwork.ozlabs.org/project/uboot/patch/20210409021313.433558-2-seanga2@gmail.com/ So which should we do? My feeling is that a failure that is programmatically silent could cause things to fail, but is there a reason why this might be wrong but everything is still OK? Regards, Simon > Reviewed-by: Sean Anderson <seanga2@gmail.com>
On 5/10/21 12:28 PM, Simon Glass wrote: > Hi Sean, > > On Sat, 8 May 2021 at 18:40, Sean Anderson <seanga2@gmail.com> wrote: >> >> On 5/8/21 6:00 PM, Simon Glass wrote: >>> When the default clocks cannot be set, the clock is silently probed and >>> the error is ignored. This is incorrect, since having the clocks at the >>> correct speed may be important for operation of the system. >>> >>> Fix it by checking the return code. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> --- >>> >>> drivers/clk/clk-uclass.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c >>> index 4ab3c402ed8..2a2e1cfbd61 100644 >>> --- a/drivers/clk/clk-uclass.c >>> +++ b/drivers/clk/clk-uclass.c >>> @@ -796,13 +796,17 @@ void devm_clk_put(struct udevice *dev, struct clk *clk) >>> >>> int clk_uclass_post_probe(struct udevice *dev) >>> { >>> + int ret; >>> + >>> /* >>> * when a clock provider is probed. Call clk_set_defaults() >>> * also after the device is probed. This takes care of cases >>> * where the DT is used to setup default parents and rates >>> * using assigned-clocks >>> */ >>> - clk_set_defaults(dev, 1); >>> + ret = clk_set_defaults(dev, 1); >>> + if (ret) >>> + return log_ret(ret); >>> >>> return 0; >>> } >>> >> >> See also: https://patchwork.ozlabs.org/project/uboot/patch/20210409021313.433558-2-seanga2@gmail.com/ > > So which should we do? My feeling is that a failure that is > programmatically silent could cause things to fail, but is there a > reason why this might be wrong but everything is still OK? I think both are fine. I think this is definitely a situation where we should complain loudly so that when things break there is some relevant output. --Sean > > Regards, > Simon > >> Reviewed-by: Sean Anderson <seanga2@gmail.com>
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 4ab3c402ed8..2a2e1cfbd61 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -796,13 +796,17 @@ void devm_clk_put(struct udevice *dev, struct clk *clk) int clk_uclass_post_probe(struct udevice *dev) { + int ret; + /* * when a clock provider is probed. Call clk_set_defaults() * also after the device is probed. This takes care of cases * where the DT is used to setup default parents and rates * using assigned-clocks */ - clk_set_defaults(dev, 1); + ret = clk_set_defaults(dev, 1); + if (ret) + return log_ret(ret); return 0; }
When the default clocks cannot be set, the clock is silently probed and the error is ignored. This is incorrect, since having the clocks at the correct speed may be important for operation of the system. Fix it by checking the return code. Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/clk/clk-uclass.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)