[PATCH v2 03/17] drivers: reset: Handle gracefully NULL pointers

[Kishon Vijay Abraham I]

Hi Simon,

On 15/04/21 1:08 am, Simon Glass wrote:
> Hi Kishon,
> 
> On Mon, 5 Apr 2021 at 11:28, Kishon Vijay Abraham I <kishon at ti.com> wrote:
>>
>> From: Jean-Jacques Hiblot <jjhiblot at ti.com>
>>
>> The reset framework provides devm_reset_control_get_optional()
>> which can return NULL (not an error case). So all the other reset_ops
>> should handle NULL gracefully. Prepare the way for a managed reset
>> API by handling NULL pointers without crashing nor failing.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com>
>> Signed-off-by: Vignesh Raghavendra <vigneshr at ti.com>
>> Signed-off-by: Kishon Vijay Abraham I <kishon at ti.com>
>> ---
>>  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 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;
> 
> This behaviour still seems odd to me. Why do you have a reset driver
> with no ops? That is not allowed.

Okay. I'll re-work this patch and post a fresh series.

Thanks
Kishon

> 
>>  }
>>
>>  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;
> 
> Can you check this first and return -ENOSYS ? E.g.
> 
> if (!ops->request)
>    return -ENOSYS;
> 
> return ops->request(reset_ctl);
> 
> Same below
> 
>>  }
>>
>>  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)
>> --
>> 2.17.1
>>
> 
> Regards,
> Simon
>