[02/16] drivers: reset: Handle gracefully NULL pointers

Message ID 20210309122748.31842-3-kishon@ti.com
State New
Headers show
Series
  • TI/Cadence: Add Sierra/Torrent SERDES driver
Related show

Commit Message

Kishon Vijay Abraham I March 9, 2021, 12:27 p.m.
From: Jean-Jacques Hiblot <jjhiblot@ti.com>


Prepare the way for a managed reset API by handling NULL pointers without
crashing nor failing.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

---
 drivers/reset/reset-uclass.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

-- 
2.17.1

Comments

Simon Glass March 12, 2021, 4:45 a.m. | #1
Hi Kishon,

On Tue, 9 Mar 2021 at 05:27, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>

> From: Jean-Jacques Hiblot <jjhiblot@ti.com>

>

> Prepare the way for a managed reset API by handling NULL pointers without

> crashing nor failing.

>

> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>

> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

> ---

>  drivers/reset/reset-uclass.c | 30 +++++++++++++++++-------------

>  1 file changed, 17 insertions(+), 13 deletions(-)


Why do you want this? This patch is missing the motivation which
should be at the start of the commit message.

Regards,
Simon
Kishon Vijay Abraham I March 22, 2021, 5:11 a.m. | #2
Hi Simon,

On 12/03/21 10:15 am, Simon Glass wrote:
> Hi Kishon,

> 

> On Tue, 9 Mar 2021 at 05:27, Kishon Vijay Abraham I <kishon@ti.com> wrote:

>>

>> From: Jean-Jacques Hiblot <jjhiblot@ti.com>

>>

>> Prepare the way for a managed reset API by handling NULL pointers without

>> crashing nor failing.

>>

>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>

>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

>> ---

>>  drivers/reset/reset-uclass.c | 30 +++++++++++++++++-------------

>>  1 file changed, 17 insertions(+), 13 deletions(-)

> 

> Why do you want this? This patch is missing the motivation which

> should be at the start of the commit message.


This is for "optional" reset controllers used by peripheral drivers.
This will help avoid adding checks in peripheral drivers.

Thanks
Kishon
Simon Glass March 23, 2021, 12:56 a.m. | #3
Hi Kishon,

On Mon, 22 Mar 2021 at 18:11, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>

> Hi Simon,

>

> On 12/03/21 10:15 am, Simon Glass wrote:

> > Hi Kishon,

> >

> > On Tue, 9 Mar 2021 at 05:27, Kishon Vijay Abraham I <kishon@ti.com> wrote:

> >>

> >> From: Jean-Jacques Hiblot <jjhiblot@ti.com>

> >>

> >> Prepare the way for a managed reset API by handling NULL pointers without

> >> crashing nor failing.

> >>

> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

> >> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>

> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

> >> ---

> >>  drivers/reset/reset-uclass.c | 30 +++++++++++++++++-------------

> >>  1 file changed, 17 insertions(+), 13 deletions(-)

> >

> > Why do you want this? This patch is missing the motivation which

> > should be at the start of the commit message.

>

> This is for "optional" reset controllers used by peripheral drivers.

> This will help avoid adding checks in peripheral drivers.


Can you please be more specific?

Reset drivers are required to have operations. Only a very few
uclasses allow the operations pointer to be NULL.

Regards,
Simon
Kishon Vijay Abraham I March 23, 2021, 7:16 a.m. | #4
Hi Simon,

On 23/03/21 6:26 am, Simon Glass wrote:
> Hi Kishon,

> 

> On Mon, 22 Mar 2021 at 18:11, Kishon Vijay Abraham I <kishon@ti.com> wrote:

>>

>> Hi Simon,

>>

>> On 12/03/21 10:15 am, Simon Glass wrote:

>>> Hi Kishon,

>>>

>>> On Tue, 9 Mar 2021 at 05:27, Kishon Vijay Abraham I <kishon@ti.com> wrote:

>>>>

>>>> From: Jean-Jacques Hiblot <jjhiblot@ti.com>

>>>>

>>>> Prepare the way for a managed reset API by handling NULL pointers without

>>>> crashing nor failing.

>>>>

>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

>>>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>

>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

>>>> ---

>>>>  drivers/reset/reset-uclass.c | 30 +++++++++++++++++-------------

>>>>  1 file changed, 17 insertions(+), 13 deletions(-)

>>>

>>> Why do you want this? This patch is missing the motivation which

>>> should be at the start of the commit message.

>>

>> This is for "optional" reset controllers used by peripheral drivers.

>> This will help avoid adding checks in peripheral drivers.

> 

> Can you please be more specific?


Sorry for not being clear.

Cadence Torrent SERDES has a reset line for APB (APB_RESET). This reset
line is connected in Cadence platform but not connected in TI platform
which also uses the Torrent SERDES.

So in the Cadence Torrent driver we use
devm_reset_control_get_optional() which returns NULL when used in TI
platform. However if we use the return value of
devm_reset_control_get_optional() directly in reset_ops (for TI), it
will result in abort.

This patch helps to avoid adding additional checks before invoking
reset_ops.

Thanks
Kishon

Patch

diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
index 071c389ca0..98304bc0ee 100644
--- a/drivers/reset/reset-uclass.c
+++ b/drivers/reset/reset-uclass.c
@@ -13,9 +13,12 @@ 
 #include <dm/devres.h>
 #include <dm/lists.h>
 
-static inline struct reset_ops *reset_dev_ops(struct udevice *dev)
+struct reset_ops nop_reset_ops = {
+};
+
+static inline struct reset_ops *reset_dev_ops(struct reset_ctl *r)
 {
-	return (struct reset_ops *)dev->driver->ops;
+	return r ? (struct reset_ops *)r->dev->driver->ops : &nop_reset_ops;
 }
 
 static int reset_of_xlate_default(struct reset_ctl *reset_ctl,
@@ -54,9 +57,10 @@  static int reset_get_by_index_tail(int ret, ofnode node,
 		debug("%s %d\n", ofnode_get_name(args->node), args->args[0]);
 		return ret;
 	}
-	ops = reset_dev_ops(dev_reset);
 
 	reset_ctl->dev = dev_reset;
+	ops = reset_dev_ops(reset_ctl);
+
 	if (ops->of_xlate)
 		ret = ops->of_xlate(reset_ctl, args);
 	else
@@ -162,29 +166,29 @@  int reset_get_by_name(struct udevice *dev, const char *name,
 
 int reset_request(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops = reset_dev_ops(reset_ctl);
 
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
-	return ops->request(reset_ctl);
+	return ops->request ? ops->request(reset_ctl) : 0;
 }
 
 int reset_free(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops = reset_dev_ops(reset_ctl);
 
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
-	return ops->rfree(reset_ctl);
+	return ops->rfree ? ops->rfree(reset_ctl) : 0;
 }
 
 int reset_assert(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops = reset_dev_ops(reset_ctl);
 
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
-	return ops->rst_assert(reset_ctl);
+	return ops->rst_assert ? ops->rst_assert(reset_ctl) : 0;
 }
 
 int reset_assert_bulk(struct reset_ctl_bulk *bulk)
@@ -202,11 +206,11 @@  int reset_assert_bulk(struct reset_ctl_bulk *bulk)
 
 int reset_deassert(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops = reset_dev_ops(reset_ctl);
 
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
-	return ops->rst_deassert(reset_ctl);
+	return ops->rst_deassert ? ops->rst_deassert(reset_ctl) : 0;
 }
 
 int reset_deassert_bulk(struct reset_ctl_bulk *bulk)
@@ -224,11 +228,11 @@  int reset_deassert_bulk(struct reset_ctl_bulk *bulk)
 
 int reset_status(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops = reset_dev_ops(reset_ctl);
 
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
-	return ops->rst_status(reset_ctl);
+	return ops->rst_status ? ops->rst_status(reset_ctl) : 0;
 }
 
 int reset_release_all(struct reset_ctl *reset_ctl, int count)