Hi Simon, On Wed, Jul 22, 2015 at 9:31 AM, Simon Glass <s...@chromium.org> wrote: > +Masahiro and Albert > > Hi Joe, > > On 15 July 2015 at 11:41, Joe Hershberger <joe.hershber...@gmail.com> wrote: >> Hi Simon, >> >> On Thu, Jul 9, 2015 at 9:15 AM, Simon Glass <s...@chromium.org> wrote: >>> With driver model drivers can have things stored in several places. There is >>> driver-private data, then the uclass can attach things to a device. If the >>> device is on a bus then its bus may attach parent data to the device too. >>> >>> At present everything is done through void pointers. It would be nice to >>> have a way to check that the correct C struct is used in each case. >>> >>> Here is a proposed implementation of a way of checking structures in driver >>> model. It relies on turning the existing dev_get_priv() function into a >>> macro which (if checking is enabled) checks that the structure names match. >>> Each xxx_auto_alloc_size turns into a structure containing a string (the >>> structure name) and the size. >>> >>> The dev_get_priv() macro has an extra parameter which is the structure being >>> accessed: >>> >>> struct eth_pdata *priv = dev_get_priv(dev, struct eth_pdata); >>> >>> and you get an error like this when things are wrong: >>> >>> Invalid access to device priv: dev=eth@10002000, expecting >>> 'struct eth_sandbox_priv', requested 'struct eth_pdata' >> >> This is interesting, but seems like something that could just be >> handled with good naming of structs. Type safety is great, but this is >> fairly heavy-handed way of doing it and only helps after hitting that >> code path in testing. Is there not a way to make the compiler help >> out? Maybe each uclass / driver should have wrapper functions that add >> type. Then the various uses all get type safety from the compiler and >> the driver only has to verify that the wrappers are correct. E.g.: >> >> diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c >> index 4e083d3..b01ae0c 100644 >> --- a/drivers/net/sandbox.c >> +++ b/drivers/net/sandbox.c >> @@ -33,6 +33,11 @@ struct eth_sandbox_priv { >> static bool disabled[8] = {false}; >> static bool skip_timeout; >> >> +static struct eth_sandbox_priv *eth_sandbox_get_priv(struct udevice *dev) >> +{ >> + return dev_get_priv(dev); >> +} >> + >> /* >> * sandbox_eth_disable_response() >> * >> @@ -56,7 +61,7 @@ void sandbox_eth_skip_timeout(void) >> >> static int sb_eth_start(struct udevice *dev) >> { >> - struct eth_sandbox_priv *priv = dev_get_priv(dev); >> + struct eth_sandbox_priv *priv = sandbox_eth_get_priv(dev); >> >> debug("eth_sandbox: Start\n"); >> > > Yes we could do this, and then the onus is on whoever writes these > functions to get them right. But this would avoid getting it right in > 9 out of 10 places and then messing up in one. > > Unfortunately it requires writing these functions for every driver. > But that might be preferable to adding to the declaration overhead at > every site the data is declared. The functions could be static inline > and placed at the top of each driver. Essentially we would be > mandating a certain code style for drivers.
That's what I was thinking. It also seems less magic, which I generally prefer. >>> A new Kconfg option is added to turn this on, since it bloats the code a >>> little. >>> >>> The next step would be to extend it to all pointers in the device and >>> uclass. This is mostly a mechanical code change. >>> >>> Finally we should have a way of checking that the device pointer itself is >>> valid. For example if someone passes an invalid address like 0x12345 as the >>> 'struct udevice' then at present we will dutifully look for the devices' >>> driver and perform an invalid memory access. >>> >>> If we want to check for memory corruption one way would be to add a magic >>> numnber before each allocated memory area. Perhaps this is more the role >>> of dlmalloc(), but if required it could be attached to Masahiro's devres >>> feature, which already prepends some data to every allocated area. >> >> It seems like it should be the more generic approach. > > Which approach is more generic? I was thinking the dlmalloc prepending a token would be a fairly generic approach. I assume it would only be enabled in a debug build. >>> Comments welcome. I'd like to figure this out soon as it involves trivial >>> but invasive patches to change each driver. >>> >>> Signed-off-by: Simon Glass <s...@chromium.org> >> >> Cheers, >> -Joe > > Regards, > Simon Cheers, -Joe _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot