| Message ID | 1492624001-3758-13-git-send-email-ykaneko0929@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Thu, Apr 20, 2017 at 02:46:33AM +0900, Yoshihiro Kaneko wrote: > From: Takeshi Kihara <takeshi.kihara.df@renesas.com> > > In .recalc_rate of struct clk_ops, it is desirable to return 0 if an > error occurs, but -EINVAL is returned. This patch fixes it. > > Fixes: 5b1defde7054 ("clk: renesas: cpg-mssr: Extract common R-Car Gen3 support code") > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Hi Wolfram, Kaneko-san, On Fri, Jul 14, 2017 at 4:25 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > On Thu, Apr 20, 2017 at 02:46:33AM +0900, Yoshihiro Kaneko wrote: >> From: Takeshi Kihara <takeshi.kihara.df@renesas.com> >> >> In .recalc_rate of struct clk_ops, it is desirable to return 0 if an >> error occurs, but -EINVAL is returned. This patch fixes it. >> >> Fixes: 5b1defde7054 ("clk: renesas: cpg-mssr: Extract common R-Car Gen3 support code") >> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> >> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Why is it desirable to return 0 if an error occurs? Because the return type is unsigned long? Is this routine allowed to fail? I don't see any handling of zero by its callers. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, > >> In .recalc_rate of struct clk_ops, it is desirable to return 0 if an > >> error occurs, but -EINVAL is returned. This patch fixes it. > >> > >> Fixes: 5b1defde7054 ("clk: renesas: cpg-mssr: Extract common R-Car Gen3 support code") > >> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > >> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > > > > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Why is it desirable to return 0 if an error occurs? Because the return type is > unsigned long? Yes, I'd think so. Instead of an arbitrary high, kind-of-random, number, just return 0 which also indicates that something unexpected happened? Also, all other drivers return zero in an error case (did some quick coccinelle grepping before). > > Is this routine allowed to fail? I don't see any handling of zero by > its callers. From clk-provider.h: * @recalc_rate Recalculate the rate of this clock, by querying hardware. The * parent rate is an input parameter. It is up to the caller to * ensure that the prepare_mutex is held across this call. * Returns the calculated rate. Optional, but recommended - if * this op is not set then clock rate will be initialized to 0. What would be the benefit of keeping -EINVAL in an unsigned long? Regards, Wolfram
Hi Wolfram, On Mon, Jul 17, 2017 at 11:18 AM, Wolfram Sang <wsa@the-dreams.de> wrote: >> >> In .recalc_rate of struct clk_ops, it is desirable to return 0 if an >> >> error occurs, but -EINVAL is returned. This patch fixes it. >> >> >> >> Fixes: 5b1defde7054 ("clk: renesas: cpg-mssr: Extract common R-Car Gen3 support code") >> >> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> >> >> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> >> > >> > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> >> >> Why is it desirable to return 0 if an error occurs? Because the return type is >> unsigned long? > > Yes, I'd think so. Instead of an arbitrary high, kind-of-random, number, just > return 0 which also indicates that something unexpected happened? Also, all > other drivers return zero in an error case (did some quick coccinelle > grepping before). OK. >> Is this routine allowed to fail? I don't see any handling of zero by >> its callers. > > From clk-provider.h: > > * @recalc_rate Recalculate the rate of this clock, by querying hardware. The > * parent rate is an input parameter. It is up to the caller to > * ensure that the prepare_mutex is held across this call. > * Returns the calculated rate. Optional, but recommended - if > * this op is not set then clock rate will be initialized to 0. > > What would be the benefit of keeping -EINVAL in an unsigned long? I do not mean that -EINVAL is correct. Obviously it isn't. But blindly replacing -EINVAL by zero may not be the right solution neither. If recalc_rate cannot return an error value, perhaps there is a good reason for that? I see there's a similar check in cpg_sd_clock_enable(), so the clock also cannot be enabled if this condition is met? When does this happen? If the firmware leaves a invalid/unsupported setting in the register? If we can't recover from that, perhaps the clock's probe should just fail instead? It looks like the only writer of that field is cpg_sd_clock_set_rate(), which always writes a valid/supported value. Is it guaranteed that this function is always called first? If yes, the checks can just be removed instead? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
> I do not mean that -EINVAL is correct. Obviously it isn't. But blindly > replacing -EINVAL by zero may not be the right solution neither. Okay, it may not be perfect but it is definately better :)
Scrap my response from before, please. Not sure myself what I was trying to defend :/
On 07/17, Geert Uytterhoeven wrote: > Hi Wolfram, > > On Mon, Jul 17, 2017 at 11:18 AM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> >> In .recalc_rate of struct clk_ops, it is desirable to return 0 if an > >> >> error occurs, but -EINVAL is returned. This patch fixes it. > >> >> > >> >> Fixes: 5b1defde7054 ("clk: renesas: cpg-mssr: Extract common R-Car Gen3 support code") > >> >> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > >> >> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > >> > > >> > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > >> > >> Why is it desirable to return 0 if an error occurs? Because the return type is > >> unsigned long? > > > > Yes, I'd think so. Instead of an arbitrary high, kind-of-random, number, just > > return 0 which also indicates that something unexpected happened? Also, all > > other drivers return zero in an error case (did some quick coccinelle > > grepping before). > > OK. > > >> Is this routine allowed to fail? I don't see any handling of zero by > >> its callers. > > > > From clk-provider.h: > > > > * @recalc_rate Recalculate the rate of this clock, by querying hardware. The > > * parent rate is an input parameter. It is up to the caller to > > * ensure that the prepare_mutex is held across this call. > > * Returns the calculated rate. Optional, but recommended - if > > * this op is not set then clock rate will be initialized to 0. > > > > What would be the benefit of keeping -EINVAL in an unsigned long? > > I do not mean that -EINVAL is correct. Obviously it isn't. But blindly > replacing -EINVAL by zero may not be the right solution neither. > > If recalc_rate cannot return an error value, perhaps there is a good reason > for that? Presumably you can always figure out what the rate of the clk is, or at least return the rate of the parent clk if it can't be figured out for some odd reason. In this case it looks like we don't have some divider mapping? Why not make it a WARN_ON() and return 0? Then we can fix the warning by adding the appropriate mapping in the table and not return some really large frequency. > > I see there's a similar check in cpg_sd_clock_enable(), so the clock also > cannot be enabled if this condition is met? > > When does this happen? If the firmware leaves a invalid/unsupported setting > in the register? If we can't recover from that, perhaps the clock's probe > should just fail instead? > > It looks like the only writer of that field is cpg_sd_clock_set_rate(), > which always writes a valid/supported value. Is it guaranteed that this > function is always called first? > If yes, the checks can just be removed instead? > This also works. I'm dropping this patch from my queue for now.
diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c index 4ab76b1..48c6e98 100644 --- a/drivers/clk/renesas/rcar-gen3-cpg.c +++ b/drivers/clk/renesas/rcar-gen3-cpg.c @@ -287,7 +287,7 @@ static unsigned long cpg_sd_clock_recalc_rate(struct clk_hw *hw, break; if (i >= clock->div_num) - return -EINVAL; + return 0; return DIV_ROUND_CLOSEST(rate, clock->div_table[i].div); }