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

[Stephen Warren]

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.