[PATCH] clk: renesas: rcar-gen3: Fix error return code in cpg_sd_clock_recalc_rate

[PATCH] clk: renesas: rcar-gen3: Fix error return code in cpg_sd_clock_recalc_rate

[Yoshihiro Kaneko]

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>
---
This patch is based on the clk-next branch of linux-clk tree.

 drivers/clk/renesas/rcar-gen3-cpg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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);
 }
-- 
1.9.1

Re: clk: renesas: rcar-gen3: Fix error return code in cpg_sd_clock_recalc_rate

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 546 bytes --]

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>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: clk: renesas: rcar-gen3: Fix error return code in cpg_sd_clock_recalc_rate

[Geert Uytterhoeven]

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

Re: clk: renesas: rcar-gen3: Fix error return code in cpg_sd_clock_recalc_rate

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: clk: renesas: rcar-gen3: Fix error return code in cpg_sd_clock_recalc_rate

[Geert Uytterhoeven]

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

Re: clk: renesas: rcar-gen3: Fix error return code in cpg_sd_clock_recalc_rate

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 202 bytes --]


> 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 :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: clk: renesas: rcar-gen3: Fix error return code in cpg_sd_clock_recalc_rate

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 88 bytes --]


Scrap my response from before, please. Not sure myself what I was trying
to defend :/


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: clk: renesas: rcar-gen3: Fix error return code in cpg_sd_clock_recalc_rate

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project