linux-kernel - Re: [PATCH v4 2/2] clk: ingenic-tcu: Fix missing TCU clock for X1000 SoCs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <LSO6AR.0DFDW2C6UEWT@crapouillou.net>
Date:   Mon, 11 Apr 2022 17:36:21 +0100
From:   Paul Cercueil <paul@...pouillou.net>
To:     Aidan MacDonald <aidanmacdonald.0x0@...il.com>
Cc:     robh+dt@...nel.org, krzk+dt@...nel.org, tsbogend@...ha.franken.de,
        mturquette@...libre.com, sboyd@...nel.org,
        linux-mips@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH v4 2/2] clk: ingenic-tcu: Fix missing TCU clock for X1000
 SoCs

Hi Aidan,

Le lun., avril 11 2022 at 17:08:41 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@...il.com> a écrit :
> On Mon, Apr 11, 2022 at 04:48:15PM +0100, Paul Cercueil wrote:
>>  Hi Aidan,
>> 
>>  Le lun., avril 11 2022 at 16:42:41 +0100, Aidan MacDonald
>>  <aidanmacdonald.0x0@...il.com> a écrit :
>>  > The TCU clock gate on X1000 wasn't requested by the driver and 
>> could
>>  > be gated automatically later on in boot, which prevents timers 
>> from
>>  > running and breaks PWM.
>>  >
>>  > Add a workaround to support old device trees that don't specify 
>> the
>>  > "tcu" clock gate. In this case the kernel will print a warning and
>>  > attempt to continue without the clock, which is wrong, but it 
>> could
>>  > work if "clk_ignore_unused" is in the kernel arguments.
>>  >
>>  > Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@...il.com>
>>  > ---
>>  >  drivers/clk/ingenic/tcu.c | 38 
>> ++++++++++++++++++++++++++------------
>>  >  1 file changed, 26 insertions(+), 12 deletions(-)
>>  >
>>  > diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c
>>  > index 77acfbeb4830..ce8c768db997 100644
>>  > --- a/drivers/clk/ingenic/tcu.c
>>  > +++ b/drivers/clk/ingenic/tcu.c
>>  > @@ -31,6 +31,7 @@ struct ingenic_soc_info {
>>  >  	unsigned int num_channels;
>>  >  	bool has_ost;
>>  >  	bool has_tcu_clk;
>>  > +	bool allow_missing_tcu_clk;
>>  >  };
>>  >
>>  >  struct ingenic_tcu_clk_info {
>>  > @@ -320,7 +321,8 @@ static const struct ingenic_soc_info
>>  > jz4770_soc_info = {
>>  >  static const struct ingenic_soc_info x1000_soc_info = {
>>  >  	.num_channels = 8,
>>  >  	.has_ost = false, /* X1000 has OST, but it not belong TCU */
>>  > -	.has_tcu_clk = false,
>>  > +	.has_tcu_clk = true,
>>  > +	.allow_missing_tcu_clk = true,
>>  >  };
>>  >
>>  >  static const struct of_device_id __maybe_unused
>>  > ingenic_tcu_of_match[] __initconst = {
>>  > @@ -354,15 +356,27 @@ static int __init ingenic_tcu_probe(struct
>>  > device_node *np)
>>  >  	if (tcu->soc_info->has_tcu_clk) {
>>  >  		tcu->clk = of_clk_get_by_name(np, "tcu");
>>  >  		if (IS_ERR(tcu->clk)) {
>>  > -			ret = PTR_ERR(tcu->clk);
>>  > -			pr_crit("Cannot get TCU clock\n");
>>  > -			goto err_free_tcu;
>>  > -		}
>>  > -
>>  > -		ret = clk_prepare_enable(tcu->clk);
>>  > -		if (ret) {
>>  > -			pr_crit("Unable to enable TCU clock\n");
>>  > -			goto err_put_clk;
>>  > +			/*
>>  > +			 * Old device trees for some SoCs did not include the
>>  > +			 * TCU clock because this driver (incorrectly) didn't
>>  > +			 * use it. In this case we complain loudly and attempt
>>  > +			 * to continue without the clock, which might work if
>>  > +			 * booting with workarounds like "clk_ignore_unused".
>>  > +			 */
>> 
>>  Why not unconditionally enable it instead? Then it would boot 
>> without
>>  clk_ignore_unused.
>> 
>>  Cheers,
>>  -Paul
> 
> I could, but why add essentially dead code to the kernel? Maintaining 
> the
> old behavior has the "advantage" that it remains broken in the same 
> way as
> before, so any workarounds anyone was using will continue to work the 
> same.
> And if they were not using workarounds and got a broken kernel, this 
> patch
> will not make anything *more* broken, in fact it will not cause any 
> change
> in behavior in that case (aside from the warning message).

OK.

-Paul

> But if you think it's best to just enable the clock anyway, let me 
> know
> and I'll send a new patch.
> 
> Regards,
> Aidan
> 
>> 
>>  > +			if (tcu->soc_info->allow_missing_tcu_clk &&
>>  > +			    PTR_ERR(tcu->clk) == -EINVAL) {
>>  > +				pr_warn("TCU clock missing from device tree, please update 
>> your
>>  > device tree\n");
>>  > +				tcu->clk = NULL;
>>  > +			} else {
>>  > +				pr_crit("Cannot get TCU clock from device tree\n");
>>  > +				goto err_free_tcu;
>>  > +			}
>>  > +		} else {
>>  > +			ret = clk_prepare_enable(tcu->clk);
>>  > +			if (ret) {
>>  > +				pr_crit("Unable to enable TCU clock\n");
>>  > +				goto err_put_clk;
>>  > +			}
>>  >  		}
>>  >  	}
>>  >
>>  > @@ -432,10 +446,10 @@ static int __init ingenic_tcu_probe(struct
>>  > device_node *np)
>>  >  			clk_hw_unregister(tcu->clocks->hws[i]);
>>  >  	kfree(tcu->clocks);
>>  >  err_clk_disable:
>>  > -	if (tcu->soc_info->has_tcu_clk)
>>  > +	if (tcu->clk)
>>  >  		clk_disable_unprepare(tcu->clk);
>>  >  err_put_clk:
>>  > -	if (tcu->soc_info->has_tcu_clk)
>>  > +	if (tcu->clk)
>>  >  		clk_put(tcu->clk);
>>  >  err_free_tcu:
>>  >  	kfree(tcu);
>>  > --
>>  > 2.35.1
>>  >
>> 
>>