Re: [PATCH] mfd: omap-usb-tll: handle clk_prepare return code in usbtll_omap_probe
From: Andreas Kemnade
Date: Mon Nov 11 2024 - 14:57:26 EST
Am Mon, 11 Nov 2024 16:41:47 +0200
schrieb Roger Quadros <rogerq@xxxxxxxxxx>:
> Hi,
>
> On 10/11/2024 01:29, Andreas Kemnade wrote:
> > Am Thu, 7 Nov 2024 12:12:52 +0100
> > schrieb Karol P <karprzy7@xxxxxxxxx>:
> >
> >> On Thu, 7 Nov 2024 at 00:15, Andreas Kemnade <andreas@xxxxxxxxxxxx> wrote:
> >>>
> >>> Am Wed, 6 Nov 2024 23:33:24 +0100
> >>> schrieb Karol Przybylski <karprzy7@xxxxxxxxx>:
> >>>
> >>>> 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@xxxxxxxxx>
> >>>> ---
> >>>> 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