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

[Simon Glass]

Hi,

On Thu, 19 Aug 2021 at 08:16, Wolfgang Denk <wd at denx.de> wrote:
>
> Dear Tom,
>
> In message <20210819130806.GW858 at bill-the-cat> you wrote:
> >
> > > So we have now a policy to wave through code, and ask others to
> > > clean it up later?  That's ... sad.
> >
> > No, we continue to have the policy of expecting reviewers to follow the
> > whole discussion and relevant subsystems.
>
> Once upon a time there has also been a policy that if a function
> might return error codes, these need to be checked and handled.
>
> > Changing _every_ caller of dev_get_priv to check for NULL and
> > then, what? is clearly not the right answer.

Note that we should not check these calls, as the priv data is
allocated by driver model and a device cannot be probed until it gets
past this step.

I know that sometimes people add this check, but whenever I see it I
ask them to remove it. It is pointless and confusing, since it
suggests that driver model may not have allocated it yet. This sort of
confusion can really make things hard to understand for new people.

>
> Then what is the right answer in your opinion?

If a device is probed, you can rely on the priv data being set up. The
only way to access a probed device is via the device-internal.h or
uclass-internal.h APIs. Be careful with those if you must use them.

>
> 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.  So why do we check it
> at all?
>
> The same applies for all functions in "drivers/core/device.c" - they
> all check for valid input parameters, like any code should do.

I think did a series on this many years ago - a way to turn on checks
for this and that the right struct is obtained when you call
dev_get_priv(), etc. We could certainly add this with a CONFIG option
for debugging purposes. The runtime cost is actually not terrible but
it does require collusion with malloc() to be efficient.

>
> > If you think you see a problem here please go audit the DM code
> > itself more and propose some changes.
>
> I can see that the DM code itself implements proper error checking
> and reporting; it's the callers where negligent code prevails.
>
>
> If you are consequent, you must decide what you want:
>
> - Either we want robust and reliable code - then we have to handle
>   the error codes which functions like dev_get_priv() etc. return.
>
> - Or you don't care about software quality, then we can omit such
>   handling, but then it would also be consequent to remove all the
>   error checking from "drivers/core/device.c" etc. - hey, that would
>   even save a few hundred bytes of code size.
>
>
> Sugarcoating code which fails to handle error codes because "these
> errors can never happen" does not seem to be a clever approach to
> software engineering to me.
>
>
> I stop here.  You know my opinion.  You are the maintainer.

There is a wider issue here about arrg checking. We sometimes use
assert but at present that only appears in the code if DEBUG is
enabled. Also it just halts.

OTOH if we add arg checking everywhere it cluttles the code and can
substantially increase the size (I know of a project where it doubled
the size). I like to distinguish between:

- programming errors
- security errors where user input is insufficiently checked

IMO the former should not be present if you have sufficient tests and
trying to catch them in the field at runtime is not very kind to your
users.

Regards,
Simon