Message-ID: <63e6e809.050a0220.af3df.d908@mx.google.com>
Date:   Fri, 10 Feb 2023 19:34:11 +0100
From:   Christian Marangi <ansuelsmth@gmail.com>
To:     Stephen Boyd <sboyd@kernel.org>
Cc:     Michael Turquette <mturquette@baylibre.com>,
        linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] clk: Warn and add workaround on misuse of
 .parent_data with .name only
References: <20230131160829.23369-1-ansuelsmth@gmail.com>
 <e5b71c243022d341022b4f172060268b.sboyd@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e5b71c243022d341022b4f172060268b.sboyd@kernel.org>
Precedence: bulk

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_data
> > struct for each parent and never fallback to .name to get parent name even
> > if declared.
> 
> 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:
> 
> 	struct clk_parent_data pdata = { .name = "global_name" };
>

An example of this problem and the relative fix is here
35dc8e101a8e08f69f4725839b98ec0f11a8e2d3

You example is also ok and this patch wants to handle just a case like
that.

> > 
> > This is caused by clk_core_get that only checks for parent .fw_name and
> > doesn't handle .name.
> 
> 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.
> 

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.

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. 

> > 
> > While it's sane to request the dev to correctly do the conversion and
> > add both .fw_name and .name in a parent_data struct, it's not sane to
> > silently drop parents without a warning.
> 
> I suppose we can do
> 
> 	WARN(parent->index >= 0 && !parent_data[i].fw_name && parent_data[i].name, ...);
> 
> or maybe better would be to make the clk registration fail if there's a
> .name field and the index is non-negative and the fw_name is NULL.
> 
> Can you grep the code and see if anyone is assigning a .name without a
> .fw_name or .index?
> 

I can check and have some fun with a good regex.

Reject registration may be an option but consider that this may cause
some device to not boot at all if the error is done on a core clock
driver like a gcc driver.

What I would love is if there is a way to cause a compilation error but
I don't think that is doable with a C macro?

> > 
> > Fix this in 2 ways. Add a kernel warning when a wrong implementation is
> > used and copy .name in .fw_name in parent map populate function to
> > handle clk problems and malfunctions.
> 
> We shouldn't be copying .name to .fw_name. They're different things.

The idea here was that in theory the global name should not be that
different than fw_name. But I understand this can have drammatic side
effect so I agree that we should only WARN that there is something
wrong.

Hope with these expleination it's more clear what this patch is trying
to achieve. The referenced commit should make the problem clear.

-- 
	Ansuel