Re: [PATCH v1 1/3] drivers: reset: Handle gracefully NULL pointers

[Simon Glass]

Hi Pratyush,

On Wed, 27 May 2020 at 11:12, Pratyush Yadav  wrote:
>
> Hi Simon,
>
> On 29/10/19 07:48PM, Simon Glass wrote:
> > Hi Jean-Jacques,
> >
> > On Mon, 30 Sep 2019 at 08:31, Jean-Jacques Hiblot  
> > wrote:
> > >
> > > Prepare the way for a managed reset API by handling NULL pointers without
> > > crashing nor failing.
> > >
> > > Signed-off-by: Jean-Jacques Hiblot 
> > > ---
> > >
> > >  drivers/reset/reset-uclass.c | 30 +-
> > >  1 file changed, 17 insertions(+), 13 deletions(-)
> >
> > Same comment here about code size / Kconfig option.
>
> A lot of the changes below are ternary operators. I'm not sure how to
> put them behind a Kconfig option to reduce code size. Do you want me to
> change the NULL check to a separate if() block to put behind an #ifdef?
>
> One way of doing this is like in this patch [0]. The other would be to
> wrap individual if statements in #ifdef, which tends to hurt
> readability.

Well for this I would really favour:

  int reset_request(struct reset_ctl *reset_ctl)
  {
 struct reset_ops *ops;

if (!result_ctl)
return 0;

ops = reset_dev_ops(reset_ctl->dev);

But why allow result_ctl to be NULL? You should explain that in your
commit message.

> > - 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;
> >  }
> >
>
> [0] 
> https://patchwork.ozlabs.org/project/uboot/patch/20191001115130.18886-2-jjhib...@ti.com/

[..]

Regards,
Simon

Re: [PATCH v1 1/3] drivers: reset: Handle gracefully NULL pointers

[Pratyush Yadav]

Hi Simon,

On 29/10/19 07:48PM, Simon Glass wrote:
> Hi Jean-Jacques,
> 
> On Mon, 30 Sep 2019 at 08:31, Jean-Jacques Hiblot  wrote:
> >
> > Prepare the way for a managed reset API by handling NULL pointers without
> > crashing nor failing.
> >
> > Signed-off-by: Jean-Jacques Hiblot 
> > ---
> >
> >  drivers/reset/reset-uclass.c | 30 +-
> >  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> Same comment here about code size / Kconfig option.

A lot of the changes below are ternary operators. I'm not sure how to 
put them behind a Kconfig option to reduce code size. Do you want me to 
change the NULL check to a separate if() block to put behind an #ifdef?

One way of doing this is like in this patch [0]. The other would be to 
wrap individual if statements in #ifdef, which tends to hurt 
readability.

[0] 
https://patchwork.ozlabs.org/project/uboot/patch/20191001115130.18886-2-jjhib...@ti.com/

> diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
> index ee1a423ffb..1cfcc8b367 100644
> --- a/drivers/reset/reset-uclass.c
> +++ b/drivers/reset/reset-uclass.c
> @@ -9,9 +9,12 @@ 
>  #include 
>  #include 
>  
> -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 : _reset_ops;
>  }
>  
>  static int reset_of_xlate_default(struct reset_ctl *reset_ctl,
> @@ -50,9 +53,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
> @@ -151,29 +155,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->free(reset_ctl);
> + return ops->free ? ops->free(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)
> @@ -191,11 +195,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)
> @@ -213,11 +217,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)

-- 
Regards,
Pratyush Yadav
Texas Instruments India

Re: [U-Boot] [PATCH v1 1/3] drivers: reset: Handle gracefully NULL pointers

[Simon Glass]

Hi Jean-Jacques,

On Mon, 30 Sep 2019 at 08:31, Jean-Jacques Hiblot  wrote:
>
> Prepare the way for a managed reset API by handling NULL pointers without
> crashing nor failing.
>
> Signed-off-by: Jean-Jacques Hiblot 
> ---
>
>  drivers/reset/reset-uclass.c | 30 +-
>  1 file changed, 17 insertions(+), 13 deletions(-)

Same comment here about code size / Kconfig option.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v1 1/3] drivers: reset: Handle gracefully NULL pointers

[Jean-Jacques Hiblot]

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

Signed-off-by: Jean-Jacques Hiblot 
---

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

diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
index ee1a423ffb..1cfcc8b367 100644
--- a/drivers/reset/reset-uclass.c
+++ b/drivers/reset/reset-uclass.c
@@ -9,9 +9,12 @@
 #include 
 #include 
 
-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 : _reset_ops;
 }
 
 static int reset_of_xlate_default(struct reset_ctl *reset_ctl,
@@ -50,9 +53,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
@@ -151,29 +155,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->free(reset_ctl);
+   return ops->free ? ops->free(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)
@@ -191,11 +195,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)
@@ -213,11 +217,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)
-- 
2.17.1

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot