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

[Wenyou.Yang at microchip.com]

> -----Original Message-----
> From: Masahiro Yamada [mailto:yamada.masahiro at socionext.com]
> Sent: 2016年8月22日 0:54
> To: Stephen Warren <swarren at wwwdotorg.org>
> Cc: Wenyou Yang - A41535 <Wenyou.Yang at microchip.com>; Wenyou Yang -
> A41535 <Wenyou.Yang at microchip.com>; 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
> 
> 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.

Thank you for your advice, I will try it.

> 
> 
> 
> --
> Best Regards
> Masahiro Yamada


Best Regards,
Wenyou Yang