On Tue, Sep 29, 2020 at 11:42:04AM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
>
> On 9/29/20 9:32 AM, Sascha Hauer wrote:
> > On Mon, Sep 28, 2020 at 05:42:41PM +0200, Ahmad Fatoum wrote:
> >> We use dev_get_drvdata to get the driver match data associated with a
> >> device. This has two shortcomings:
> >>
> >> - Linux has dev_get_drvdata too, which returns a private pointer for
> >> driver specific info to associate with a device. We use dev->priv
> >> (or more often container_of) for that in barebox instead
> >>
> >> - It nearly always involves a cast to a double pointer, which is
> >> error-prone as size and alignment match need to be ensured
> >> on the programmer's part and can easily be gotten wrong:
> >> enum dev_type type
> >> dev_get_drvdata(dev, (const void **)&type);
> >>
> >> Add a new function that instead of using a double pointer argument,
> >> returns the pointer directly:
> >>
> >> - For normal pointer driver data, no cast is necessary
> >> - For integer driver data casted to a pointer for storage,
> >> the cast is still necessary, but
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>
> >> ---
> >> drivers/base/driver.c | 11 +++++++++++
> >> include/driver.h | 18 ++++++++++++++++++
> >> 2 files changed, 29 insertions(+)
> >>
> >> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> >> index 412db6c40699..3205bbc3c33b 100644
> >> --- a/drivers/base/driver.c
> >> +++ b/drivers/base/driver.c
> >> @@ -500,3 +500,14 @@ int dev_get_drvdata(struct device_d *dev, const void **data)
> >>
> >> return -ENODEV;
> >> }
> >> +
> >> +const void *device_get_match_data(struct device_d *dev)
> >> +{
> >> + if (dev->of_id_entry)
> >> + return dev->of_id_entry->data;
> >> +
> >> + if (dev->id_entry)
> >> + return (void *)dev->id_entry->driver_data;
> >> +
> >> + return NULL;
> >> +}
> >
> > Can we please give the caller a possibility to distinguish between
> > errors and a valid NULL pointer?
>
> The only error possible is that we matched the device by name
> and not of_compatible or platform id entry.
>
> If the match data is a valid pointer:
> -> It doesn't matter, why we have no match data either way.
>
> If the match data is a casted integer (e.g. enum):
> The driver author should either:
> -> place the default enum value as first one,
> so no match data => default
You know that, but "The driver Author" probably doesn't.
> -> should add an initial DEVICE_TYPE_UNKNOWN = 0 in the enum
> and handle it appropriately
>
> I like the function signature like that, I don't really see
> a need to adjust it.
>
> > As you realize in your series some drivers cast the returned value into
> > an integer type and use 0 as a valid possibility. These need an extra
> > explanation why we can accept that case. I can think of different
> > possibilies to get that straight:
> >
> > - Put a real pointer into matchdata. This is really preferred as it
> > motivates people to put flags into a (struct type) matchdata which
> > doesn't lead to excessive if (type == foo || type == bar || type ==
> > baz) we sometimes see in drivers.
>
> We have a real pointer there already. The problem is migrating the
> existing drivers.
Yes, existing drivers would have to be migrated, that is exactly what I
am proposing.
>
> > - Return an ERR_PTR from device_get_match_data(). this is less likely
> > interpreted as a valid int value
>
> Doesn't cover all cases. Also for the normal use, it means
> you need to have to check with IS_ERR_OR_NULL everywhere to
> be sure you don't dereference a NULL pointer.
Why that? Just don't return NULL when there's no match data, but return
-ESOMETHING.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
