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