Hi Rasmus,
On Fri, 20 Aug 2021 at 00:22, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
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.
Just on this point, drivers should use the priv pointer without priv_auto. If we do introduce run-time checks, it would fail with those.
Regards, Simon