[U-Boot] [PATCH] RFC: dm: Add pointer checking for allocated data

[Albert ARIBAUD]

Hello Simon,

On Thu,  9 Jul 2015 08:15:51 -0600, Simon Glass <sjg at 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 at 10002000, expecting
>    'struct eth_sandbox_priv', requested 'struct eth_pdata'

'Requested' seems too synonymous to 'expecting' for me.

> A new Kconfg option is added to turn this on, since it bloats the code a
> little.

Sounds like some kind of RTTI to me. Do we really need this? Can we not
check this at build time ? (for both answers -- which I assume will be
"yes" and "no" respectively -- a concrete example would be welcome to
help me figure out what the specific issue is, as opposed to generic
C type casting issues).

Plus, IIUC, a mismatch warning is pretty much an indication that things
have gone Horribly Wrong -- which makes the feature sound like a debug
helper to me, see below.

> 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.

Sounds like a debug helper to me, as when a corrupt pointer is
detected, there is not much you can do but whine about it on the
console (assuming the faulty pointer won't kill the console).

> Comments welcome. I'd like to figure this out soon as it involves trivial
> but invasive patches to change each driver.

If, as I believe they are, both feature above are debug helpers,
then I would suggest that we organize them and other debug helpers)
under the general 'DEBUG' option, with a sub-option for each helper,
like 'DEBUG_DRIVER_STRUCTS' and 'DEBUG_ALLOCS'. This could also help
organize debug messages, e.g. DEBUG_LOG, DEBUG_LOG_USB, DEBUG_LOG_ENV,
etc.

That would make a central point where developers could find (hopefully
documented) debug features, enable and disable them easily (I am
thinking about those development environment with 'release' vs 'debug'
builds) and which would become a natural landing point for new debugging
features.

Amicalement,
-- 
Albert.