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

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

[Wenyou Yang]

Add check ops pointer before use it. Otherwise, it will cause
the runtime error for the clk devices without ops callback.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---

 drivers/clk/clk-uclass.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index 4d78e3f..13b8739 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -82,7 +82,7 @@ int clk_get_by_index(struct udevice *dev, int index, struct clk *clk)
 	}
 	ops = clk_dev_ops(dev_clk);
 
-	if (ops->of_xlate)
+	if (ops && ops->of_xlate)
 		ret = ops->of_xlate(clk, &args);
 	else
 		ret = clk_of_xlate_default(clk, &args);
@@ -120,7 +120,7 @@ int clk_request(struct udevice *dev, struct clk *clk)
 
 	clk->dev = dev;
 
-	if (!ops->request)
+	if (!ops || !ops->request)
 		return 0;
 
 	return ops->request(clk);
@@ -132,7 +132,7 @@ int clk_free(struct clk *clk)
 
 	debug("%s(clk=%p)\n", __func__, clk);
 
-	if (!ops->free)
+	if (!ops || !ops->free)
 		return 0;
 
 	return ops->free(clk);
@@ -144,7 +144,7 @@ ulong clk_get_rate(struct clk *clk)
 
 	debug("%s(clk=%p)\n", __func__, clk);
 
-	if (!ops->get_rate)
+	if (!ops || !ops->get_rate)
 		return -ENOSYS;
 
 	return ops->get_rate(clk);
@@ -156,7 +156,7 @@ ulong clk_set_rate(struct clk *clk, ulong rate)
 
 	debug("%s(clk=%p, rate=%lu)\n", __func__, clk, rate);
 
-	if (!ops->set_rate)
+	if (!ops || !ops->set_rate)
 		return -ENOSYS;
 
 	return ops->set_rate(clk, rate);
@@ -168,7 +168,7 @@ int clk_enable(struct clk *clk)
 
 	debug("%s(clk=%p)\n", __func__, clk);
 
-	if (!ops->enable)
+	if (!ops || !ops->enable)
 		return -ENOSYS;
 
 	return ops->enable(clk);
@@ -180,7 +180,7 @@ int clk_disable(struct clk *clk)
 
 	debug("%s(clk=%p)\n", __func__, clk);
 
-	if (!ops->disable)
+	if (!ops || !ops->disable)
 		return -ENOSYS;
 
 	return ops->disable(clk);
-- 
2.7.4

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

[Stephen Warren]

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.

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

[Wenyou.Yang at microchip.com]

HI Stephen,

> -----Original Message-----
> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
> Sent: 2016?8?17? 23:59
> To: Wenyou Yang <wenyou.yang@atmel.com>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stephen Warren
> <swarren@nvidia.com>; Michal Simek <michal.simek@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.


Best Regards,
Wenyou Yang

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

[Stephen Warren]

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@atmel.com>
>> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stephen Warren
>> <swarren@nvidia.com>; Michal Simek <michal.simek@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?

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

[Wenyou.Yang at microchip.com]

Hi Stephen,

> -----Original Message-----
> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
> Sent: 2016?8?18? 11:46
> To: Wenyou Yang - A41535 <Wenyou.Yang@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@atmel.com>
> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stephen Warren
> >> <swarren@nvidia.com>; Michal Simek <michal.simek@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.


Best Regards,
Wenyou Yang 

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

[Stephen Warren]

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@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@atmel.com>
>>>> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stephen Warren
>>>> <swarren@nvidia.com>; Michal Simek <michal.simek@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.

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

[Wenyou.Yang at microchip.com]

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@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@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@atmel.com>
> >>>> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stephen Warren
> >>>> <swarren@nvidia.com>; Michal Simek <michal.simek@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.


Best Regards,
Wenyou Yang

[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@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@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@atmel.com>
>>>>>> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stephen Warren
>>>>>> <swarren@nvidia.com>; Michal Simek <michal.simek@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.

[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@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@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@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@atmel.com>
>>>>>>> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stephen Warren
>>>>>>> <swarren@nvidia.com>; Michal Simek <michal.simek@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

[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@wwwdotorg.org>
> Cc: Wenyou Yang - A41535 <Wenyou.Yang@microchip.com>; Wenyou Yang -
> A41535 <Wenyou.Yang@microchip.com>; U-Boot Mailing List <u-
> boot at lists.denx.de>; Stephen Warren <swarren@nvidia.com>; Michal Simek
> <michal.simek@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@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@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@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@atmel.com>
> >>>>>>> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stephen Warren
> >>>>>>> <swarren@nvidia.com>; Michal Simek <michal.simek@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

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

[Simon Glass]

Hi,

On 17 August 2016 at 09:59, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 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.

Yes, we expect drivers to implement these operations. If it is
missing, that is an error. I think assert() would be OK, but we really
don't want to add too many of these sorts of checks.

Regards,
Simon