[U-Boot] [PATCH v1] clk: clk-uclass: Check ops pointer before use it
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);
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.
HI Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@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
On 08/17/2016 06:30 PM, Wenyou.Yang@microchip.com wrote:
HI Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@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?
Hi Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: 2016年8月18日 11:46 To: Wenyou Yang - A41535 Wenyou.Yang@microchip.com; wenyou.yang@atmel.com Cc: u-boot@lists.denx.de; swarren@nvidia.com; michal.simek@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@microchip.com wrote:
HI Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@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
On 08/17/2016 09:53 PM, Wenyou.Yang@microchip.com wrote:
Hi Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: 2016年8月18日 11:46 To: Wenyou Yang - A41535 Wenyou.Yang@microchip.com; wenyou.yang@atmel.com Cc: u-boot@lists.denx.de; swarren@nvidia.com; michal.simek@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@microchip.com wrote:
HI Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@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.
Add Simon and Andreas in loop
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: 2016年8月18日 11:56 To: Wenyou Yang - A41535 Wenyou.Yang@microchip.com; wenyou.yang@atmel.com Cc: u-boot@lists.denx.de; swarren@nvidia.com; michal.simek@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@microchip.com wrote:
Hi Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: 2016年8月18日 11:46 To: Wenyou Yang - A41535 Wenyou.Yang@microchip.com; wenyou.yang@atmel.com Cc: u-boot@lists.denx.de; swarren@nvidia.com; michal.simek@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@microchip.com wrote:
HI Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@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
On 08/18/2016 07:49 PM, Wenyou.Yang@microchip.com wrote:
Add Simon and Andreas in loop
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: 2016年8月18日 11:56 To: Wenyou Yang - A41535 Wenyou.Yang@microchip.com; wenyou.yang@atmel.com Cc: u-boot@lists.denx.de; swarren@nvidia.com; michal.simek@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@microchip.com wrote:
Hi Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: 2016年8月18日 11:46 To: Wenyou Yang - A41535 Wenyou.Yang@microchip.com; wenyou.yang@atmel.com Cc: u-boot@lists.denx.de; swarren@nvidia.com; michal.simek@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@microchip.com wrote:
HI Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@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.
2016-08-19 12:52 GMT+09:00 Stephen Warren swarren@wwwdotorg.org:
On 08/18/2016 07:49 PM, Wenyou.Yang@microchip.com wrote:
Add Simon and Andreas in loop
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: 2016年8月18日 11:56 To: Wenyou Yang - A41535 Wenyou.Yang@microchip.com; wenyou.yang@atmel.com Cc: u-boot@lists.denx.de; swarren@nvidia.com; michal.simek@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@microchip.com wrote:
Hi Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: 2016年8月18日 11:46 To: Wenyou Yang - A41535 Wenyou.Yang@microchip.com; wenyou.yang@atmel.com Cc: u-boot@lists.denx.de; swarren@nvidia.com; michal.simek@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@microchip.com wrote:
HI Stephen,
> -----Original Message----- > From: Stephen Warren [mailto:swarren@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.
-----Original Message----- From: Masahiro Yamada [mailto:yamada.masahiro@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@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@microchip.com wrote:
Add Simon and Andreas in loop
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: 2016年8月18日 11:56 To: Wenyou Yang - A41535 Wenyou.Yang@microchip.com; wenyou.yang@atmel.com Cc: u-boot@lists.denx.de; swarren@nvidia.com; michal.simek@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@microchip.com wrote:
Hi Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: 2016年8月18日 11:46 To: Wenyou Yang - A41535 Wenyou.Yang@microchip.com; wenyou.yang@atmel.com Cc: u-boot@lists.denx.de; swarren@nvidia.com; michal.simek@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@microchip.com wrote: > > HI Stephen, > >> -----Original Message----- >> From: Stephen Warren [mailto:swarren@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
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
participants (5)
-
Masahiro Yamada -
Simon Glass -
Stephen Warren -
Wenyou Yang -
Wenyou.Yang＠microchip.com