Message-ID: <96040456fd5c127497de93980eb7db83.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <63e6e809.050a0220.af3df.d908@mx.google.com>
References: <20230131160829.23369-1-ansuelsmth@gmail.com> <e5b71c243022d341022b4f172060268b.sboyd@kernel.org> <63e6e809.050a0220.af3df.d908@mx.google.com>
Subject: Re: [PATCH v2 1/2] clk: Warn and add workaround on misuse of .parent_data with .name only
From:   Stephen Boyd <sboyd@kernel.org>
Cc:     Michael Turquette <mturquette@baylibre.com>,
        linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org
To:     Christian Marangi <ansuelsmth@gmail.com>
Date:   Wed, 15 Feb 2023 10:54:56 -0800
User-Agent: alot/0.10
Precedence: bulk

Quoting Christian Marangi (2023-02-10 10:34:11)
> On Fri, Feb 10, 2023 at 04:40:29PM -0800, Stephen Boyd wrote:
> > Quoting Christian Marangi (2023-01-31 08:08:28)
> > > By a simple mistake in a .parent_names to .parent_data conversion it =
was
> > > found that clk core assume fw_name is always provided with a parent_d=
ata
> > > struct for each parent and never fallback to .name to get parent name=
 even
> > > if declared.
> >=20
> > It sounds like you have clk_parent_data and the .index member is 0? Can
> > you show an example structure? I'm guessing it is like this:
> >=20
> >       struct clk_parent_data pdata =3D { .name =3D "global_name" };
> >
>=20
> An example of this problem and the relative fix is here
> 35dc8e101a8e08f69f4725839b98ec0f11a8e2d3
>=20
> You example is also ok and this patch wants to handle just a case like
> that.

Ok, so you have a firmware .index of 0. The .name is a fallback. I
suppose you want the .name to be a fallback if there isn't a clocks
property in the registering device node? I thought that should already
work but maybe there is a bug somewhere. Presumably you have a gcc node
that doesn't have a clocks property

                gcc: gcc@1800000 {
                        compatible =3D "qcom,gcc-ipq8074";
                        reg =3D <0x01800000 0x80000>;
                        #clock-cells =3D <0x1>;
                        #power-domain-cells =3D <1>;
                        #reset-cells =3D <0x1>;
                };=09

Looking at clk_core_get() we'll call of_parse_clkspec() and that should fail

	struct clk_hw *hw =3D ERR_PTR(-ENOENT);

	...

        if (np && (name || index >=3D 0) &&
            !of_parse_clkspec(np, index, name, &clkspec)) {
		...
	} else if (name) {
		...
	}

        if (IS_ERR(hw))
                return ERR_CAST(hw);

so we should have a -ENOENT clk_hw pointer in
clk_core_fill_parent_index(). That should land in this if condition in
clk_core_fill_parent_index()

                parent =3D clk_core_get(core, index);
                if (PTR_ERR(parent) =3D=3D -ENOENT && entry->name)
                        parent =3D clk_core_lookup(entry->name);

and then entry->name should be used.=20

>=20
> > >=20
> > > This is caused by clk_core_get that only checks for parent .fw_name a=
nd
> > > doesn't handle .name.
> >=20
> > clk_core_get() is not supposed to operate on the .name member. It is a
> > firmware based lookup with clkdev as a fallback because clkdev is a
> > psudeo-firmware interface to assign a name to a clk when some device
> > pointer is used in conjunction with it.
> >=20
>=20
> And the problem is just that. We currently permit to have a
> configuration with .name but no .fw_name. In a case like that a dev may
> think that this configuration is valid but in reality the clk is
> silently ignored/not found and cause clk problem with selecting a
> parent.

It is valid though.

>=20
> Took some good hours to discover this and to me it seems an error that
> everybody can do since nowhere is specificed that the following
> parent_data configuration is illegal.=20
>=20

I'll look at adding a test. Seems to be the best way to solve this.