[U-Boot] [PATCH v1] clk: clk-uclass: Check ops pointer before use it

[Masahiro Yamada]

2016-08-19 12:52 GMT+09:00 Stephen Warren <swarren at wwwdotorg.org>:
> On 08/18/2016 07:49 PM, Wenyou.Yang at microchip.com wrote:
>>
>> Add Simon and Andreas in loop
>>
>>> -----Original Message-----
>>> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
>>> Sent: 2016年8月18日 11:56
>>> To: Wenyou Yang - A41535 <Wenyou.Yang at microchip.com>;
>>> wenyou.yang at atmel.com
>>> Cc: u-boot at lists.denx.de; swarren at nvidia.com; michal.simek at xilinx.com
>>> Subject: Re: [U-Boot] [PATCH v1] clk: clk-uclass: Check ops pointer
>>> before use it
>>>
>>> On 08/17/2016 09:53 PM, Wenyou.Yang at microchip.com wrote:
>>>>
>>>> Hi Stephen,
>>>>
>>>>> -----Original Message-----
>>>>> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
>>>>> Sent: 2016年8月18日 11:46
>>>>> To: Wenyou Yang - A41535 <Wenyou.Yang at microchip.com>;
>>>>> wenyou.yang at atmel.com
>>>>> Cc: u-boot at lists.denx.de; swarren at nvidia.com; michal.simek at xilinx.com
>>>>> Subject: Re: [U-Boot] [PATCH v1] clk: clk-uclass: Check ops pointer
>>>>> before use it
>>>>>
>>>>> On 08/17/2016 06:30 PM, Wenyou.Yang at microchip.com wrote:
>>>>>>
>>>>>> HI Stephen,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
>>>>>>> Sent: 2016年8月17日 23:59
>>>>>>> To: Wenyou Yang <wenyou.yang at atmel.com>
>>>>>>> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Stephen Warren
>>>>>>> <swarren at nvidia.com>; Michal Simek <michal.simek at xilinx.com>
>>>>>>> Subject: Re: [U-Boot] [PATCH v1] clk: clk-uclass: Check ops pointer
>>>>>>> before use it
>>>>>>>
>>>>>>> On 08/17/2016 01:05 AM, Wenyou Yang wrote:
>>>>>>>>
>>>>>>>> Add check ops pointer before use it. Otherwise, it will cause the
>>>>>>>> runtime error for the clk devices without ops callback.
>>>>>>>
>>>>>>>
>>>>>>> Other uclasses like reset, power domain, and mailbox don't do this.
>>>>>>> All drivers must have an ops pointer, or they can't be useful. I'm
>>>>>>> not sure this patch is necessary. Is it just a debugging aid so if
>>>>>>> the driver writer forgets to set the ops pointer the system will
>>>>>>> error out rather than crashing? If so, a post-bind hook in the
>>>>>>> uclass that
>>>>>
>>>>> refuses the driver if it hasn't set the ops pointer would be better.
>>>>>>
>>>>>>
>>>>>> Sorry, I don't agree with you.
>>>>>>
>>>>>> Not all drivers have an ops pointer.
>>>>>>
>>>>>> If you grep U_BOOT_DRIVER , you will find that there are some
>>>>>> drivers without
>>>>>
>>>>> an ops pointer.
>>>>>>
>>>>>>
>>>>>> We should not suppose a driver should have something, I think.
>>>>>
>>>>>
>>>>> But without an ops pointer, the driver can do nothing and provide no
>>>>> services.
>>>>> Why is that useful?
>>>>
>>>>
>>>> There are some nodes without compatible in device tree,  as a child of
>>>> some node, for example, pinctrl child node or for my code peripheral
>>>> clock node.
>>>>
>>>> These nodes also need to be bound before using them. They require such
>>>> driver
>>>
>>> to bind them.
>>>
>>> That seems unrelated. A node without a compatible value won't instantiate
>>> a
>>> device object, and hence needs no driver.
>>
>>
>> But there are such nodes and drivers (i.e., U_BOOT_DRIVER) existed in
>> U-Boot, as I said before.
>>
>> Is it harmful to add this check?
>>
>> I know, if not, it will produce a wild pointer, such as NULL->of_xlate.
>
>
> I see this in drivers/clk/at91/{pmc,sckc}.c. The drivers in those files
> exist solely to cause DT sub-nodes to be enumerated, and aren't clock
> providers themselves. I believe the fix is to change them from UCLASS_CLK to
> UCLASS_SOMETHING_ELSE, e.g. UCLASS_MISC. ops are already optional for that
> uclass.

Right.
drivers/clk/at91/{pmc,sckc}.c are doing wrong.

I think UCLASS_SIMPLE_BUS is the best
because you do not have to bind child nodes explicitly.
simple_bus_post_bind() will do it for you.



-- 
Best Regards
Masahiro Yamada