linux-kernel - Re: [External] : Re: [PATCH] clk: microchip: mpfs: Fix incorrect MSSPLL ID in error message

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] [day] [month] [year] [list]

Message-ID: <20250625-reconvene-lyricist-8bfbf0c6ee36@spud>
Date: Wed, 25 Jun 2025 14:00:01 +0100
From: Conor Dooley <conor@...nel.org>
To: ALOK TIWARI <alok.a.tiwari@...cle.com>
Cc: conor.dooley@...rochip.com, daire.mcnamara@...rochip.com,
	mturquette@...libre.com, sboyd@...nel.org,
	linux-riscv@...ts.infradead.org, linux-clk@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [External] : Re: [PATCH] clk: microchip: mpfs: Fix incorrect
 MSSPLL ID in error message

On Tue, Jun 24, 2025 at 10:39:16PM +0530, ALOK TIWARI wrote:
> 
> 
> On 6/24/2025 9:29 PM, Conor Dooley wrote:
> > On Sun, Jun 22, 2025 at 11:03:49AM -0700, Alok Tiwari wrote:
> > > The error message in mpfs_clk_register_mssplls() incorrectly
> > > printed a constant CLK_MSSPLL_INTERNAL instead of the actual
> > > PLL ID that failed to register.
> > Huh, that's weird. Did you actually encounter this happening, or is this
> > some sort of patch based on the output from a tool?
> > I ask because I don't see how this could ever actually report a
> > constant, when the array it loops over only has a single element.
> > Feels like we should just do something like the following (if we do
> > anything at all)
> > 
> > Cheers,
> > Conor.
> > 
> > diff --git a/drivers/clk/microchip/clk-mpfs.c b/drivers/clk/microchip/clk-mpfs.c
> > index c22632a7439c5..ed6d5e6ff98ec 100644
> > --- a/drivers/clk/microchip/clk-mpfs.c
> > +++ b/drivers/clk/microchip/clk-mpfs.c
> > @@ -148,22 +148,18 @@ static struct mpfs_msspll_hw_clock mpfs_msspll_clks[] = {
> >   };
> >   static int mpfs_clk_register_mssplls(struct device *dev, struct mpfs_msspll_hw_clock *msspll_hws,
> > -				     unsigned int num_clks, struct mpfs_clock_data *data)
> > +				     struct mpfs_clock_data *data)
> >   {
> > -	unsigned int i;
> > +	struct mpfs_msspll_hw_clock *msspll_hw = &msspll_hws[0];
> >   	int ret;
> > -	for (i = 0; i < num_clks; i++) {
> > -		struct mpfs_msspll_hw_clock *msspll_hw = &msspll_hws[i];
> > +	msspll_hw->base = data->msspll_base;
> > +	ret = devm_clk_hw_register(dev, &msspll_hw->hw);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to register msspll id: %d\n",
> > +				     CLK_MSSPLL_INTERNAL);
> > -		msspll_hw->base = data->msspll_base;
> > -		ret = devm_clk_hw_register(dev, &msspll_hw->hw);
> > -		if (ret)
> > -			return dev_err_probe(dev, ret, "failed to register msspll id: %d\n",
> > -					     CLK_MSSPLL_INTERNAL);
> > -
> > -		data->hw_data.hws[msspll_hw->id] = &msspll_hw->hw;
> > -	}
> > +	data->hw_data.hws[msspll_hw->id] = &msspll_hw->hw;
> >   	return 0;
> >   }
> > @@ -386,8 +382,7 @@ static int mpfs_clk_probe(struct platform_device *pdev)
> >   	clk_data->dev = dev;
> >   	dev_set_drvdata(dev, clk_data);
> > -	ret = mpfs_clk_register_mssplls(dev, mpfs_msspll_clks, ARRAY_SIZE(mpfs_msspll_clks),
> > -					clk_data);
> > +	ret = mpfs_clk_register_mssplls(dev, mpfs_msspll_clks, clk_data);
> >   	if (ret)
> >   		return ret;
> 
> 
> Thanks Conor. This patch based on static tool.
> 
> You are right, there is only a single MSSPLL internal clock, so the loop
> isn't strictly necessary.
> We could either remove the loop entirely (as you suggested),
> or alternatively, just tweak the error message to something like:
> "failed to register MSSPLL internal id: %d\n"

The loop doesn't do anything, so it should probably be removed.

> This would help distinguish it from the error messages used for the MSSPLL
> output and cfg clocks.
> 
> I also noticed that similar generic messages are used elsewhere, like:
> "failed to register clock id: %d\n" in mpfs_clk_register_cfgs()
> "failed to register clock id: %d\n" in mpfs_clk_register_periphs()
> 
> Would it make sense to update those as well for clarity, or do you think
> it's better to keep the patch minimal and leave them as is?

I dunno, I don't think there's anything particularly wrong with those,
the IDs are unique across the whole driver. You need to look up the IDs
anyway to understand the error message, so changing those prints
provides no extra info IMO.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)