linux-kernel - Re: [PATCH] mfd: omap-usb-tll: handle clk_prepare return code in usbtll_omap

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: <CAKwoAfqFAXATjzSgKJu+pF=igs3dDJOWakOQ0jt9didArHkKXQ@mail.gmail.com>
Date: Tue, 12 Nov 2024 12:59:15 +0100
From: Karol P <karprzy7@...il.com>
To: Andreas Kemnade <andreas@...nade.info>
Cc: Roger Quadros <rogerq@...nel.org>, aaro.koskinen@....fi, khilman@...libre.com, 
	tony@...mide.com, lee@...nel.org, linux-omap@...r.kernel.org, 
	linux-kernel@...r.kernel.org, skhan@...uxfoundation.org
Subject: Re: [PATCH] mfd: omap-usb-tll: handle clk_prepare return code in usbtll_omap_probe

On Mon, 11 Nov 2024 at 20:57, Andreas Kemnade <andreas@...nade.info> wrote:
>
> Am Mon, 11 Nov 2024 16:41:47 +0200
> schrieb Roger Quadros <rogerq@...nel.org>:
>
> > Hi,
> >
> > On 10/11/2024 01:29, Andreas Kemnade wrote:
> > > Am Thu, 7 Nov 2024 12:12:52 +0100
> > > schrieb Karol P <karprzy7@...il.com>:
> > >
> > >> On Thu, 7 Nov 2024 at 00:15, Andreas Kemnade <andreas@...nade.info> wrote:
> > >>>
> > >>> Am Wed,  6 Nov 2024 23:33:24 +0100
> > >>> schrieb Karol Przybylski <karprzy7@...il.com>:
> > >>>
> > >>>> clk_prepare() is called in usbtll_omap_probe to fill clk array.
> > >>>> Return code is not checked, leaving possible error condition unhandled.
> > >>>>
> > >>>> Added variable to hold return value from clk_prepare() and return statement
> > >>>> when it's not successful.
> > >>>>
> > >>>> Found in coverity scan, CID 1594680
> > >>>>
> > >>>> Signed-off-by: Karol Przybylski <karprzy7@...il.com>
> > >>>> ---
> > >>>>  drivers/mfd/omap-usb-tll.c | 8 ++++++--
> > >>>>  1 file changed, 6 insertions(+), 2 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c
> > >>>> index 0f7fdb99c809..28446b082c85 100644
> > >>>> --- a/drivers/mfd/omap-usb-tll.c
> > >>>> +++ b/drivers/mfd/omap-usb-tll.c
> > >>>> @@ -202,7 +202,7 @@ static int usbtll_omap_probe(struct platform_device *pdev)
> > >>>>       struct device                           *dev =  &pdev->dev;
> > >>>>       struct usbtll_omap                      *tll;
> > >>>>       void __iomem                            *base;
> > >>>> -     int                                     i, nch, ver;
> > >>>> +     int                                     i, nch, ver, err;
> > >>>>
> > >>>>       dev_dbg(dev, "starting TI HSUSB TLL Controller\n");
> > >>>>
> > >>>> @@ -251,7 +251,11 @@ static int usbtll_omap_probe(struct platform_device *pdev)
> > >>>>               if (IS_ERR(tll->ch_clk[i]))
> > >>>>                       dev_dbg(dev, "can't get clock : %s\n", clkname);
> > >>>
> > >>> if you add more intensive error checking, then why is this error
> > >>> ignored and not returned?
> > >>
> > >> Thank you for the feedback. It does seem that elevated error checking
> > >> is not the way
> > >> to go in this case.
> > >
> > > As far as I can see everything checks ch_clk[i] for validity before
> > > usage. Also clk_enable() called later is checked which would catch
> > > clk_prepare() failures, if there were even possible here.
> > >
> > > So the only question which I am not 100% sure about is whether having
> > > ch_clk sparsly populated is normal operation. If that is the case, then
> > > more error checking is not useful. If not, then it might let us better
> > > sleep. As said as far as I can see errors are catched later.
> > >
> > > @Roger: what is your opintion towards this?
> >
> > I don't see usb_tll_hs_usb_ch?_clk in any of the OMAP device trees.
> > Could it be that they are optional?
> > If so then we could convert it to devm_clk_get_optional()?
> >
> They live in drivers/clk/ti/clk-[54]4xx.c
> But nothing about omap3. So apparently we can have valid use cases
> where these clocks are not available. So no real need more anything
> more than dev_dbg output here.
>
> Regards,
> Andreas

Thanks, I will send in a new patch then. I can also take a look at
updating device tree binding.

Best regards,
Karol