Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Rasmus Villemoes <rasmus.villemoes@prevas.dk>]

On 19/08/2021 14.32, Wolfgang Denk wrote:

> The existence of bad code is not a justification to add more of it.

Obviously true and I agree.

However, it is at the same time completely irrelevant in this context,
because the pattern of using the return value of dev_get_priv() without
a NULL check is neither bad or wrong, as has now been explained to you
several times.

If you really think checking the return value of dev_get_priv() must be
done religiously, perhaps you could tap Stefan (737c3de09984), Marek
(7e1f1e16fe75), or Heiko (6e31c62a175c) on the shoulder and tell them to
stop cranking out "bad" code.

On 19/08/2021 16.16, Wolfgang Denk wrote:

> I mean, look at the implementation of dev_get_priv():
>
>  628 void *dev_get_priv(const struct udevice *dev)
>  629 {
>  630         if (!dev) {
>  631                 dm_warn("%s: null device\n", __func__);
>  632                 return NULL;
>  633         }
>  634
>  635         return dm_priv_to_rw(dev->priv_);
>  636 }
>
> If there is guaranteed no way that dev_get_priv() can return a NULL
> pointer, that means that it must be guaranteed that the "dev"
> argument can never be a NULL pointer, either.

There's another logical fallacy right here. Sure, you've found an input
value for which dev_get_priv() would return NULL. But any caller who
knows they're not passing a NULL dev also know they won't follow that
code path.

A driver which doesn't populate the priv field by via a non-zero
.priv_auto field may need to check the return value of dev_get_priv().
I'm not claiming that checking that is always redundant. However,
neither is it anywhere near true that checking is always required.

Rasmus