Re: [PATCH] mfd: omap-usb-tll: handle clk_prepare return code in usbtll_omap_probe

[Roger Quadros <rogerq@kernel.org>]

Hi,

On 10/11/2024 01:29, Andreas Kemnade wrote:
> Am Thu, 7 Nov 2024 12:12:52 +0100
> schrieb Karol P <karprzy7@gmail.com>:
> 
>> On Thu, 7 Nov 2024 at 00:15, Andreas Kemnade <andreas@kemnade.info> wrote:
>>>
>>> Am Wed,  6 Nov 2024 23:33:24 +0100
>>> schrieb Karol Przybylski <karprzy7@gmail.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@gmail.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()?

While at that, maybe the device tree binding could also be updated and
converted to yaml.

> 
> BTW: If you do this kind of work, you could also use W=1 or
> CONFIG_WERROR during compiling to catch easy things. At least I see new
> compile warnings with your patch. 
> 
> Regards,
> Andreas

-- 
cheers,
-roger