+Joe and Stephen who might have comments
On 14 July 2015 at 09:47, Simon Glass <s...@chromium.org> wrote: > Hi Masahiro, > > On 13 July 2015 at 22:36, Masahiro Yamada <yamada.masah...@socionext.com> > wrote: >> Hi Simon, >> >> >> >> >> 2015-07-09 23:15 GMT+09:00 Simon Glass <s...@chromium.org>: >>> 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. >> >> This is also true for driver private data in Linux. >> > > Yes. > >> >> >>> 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: >> >> In which scenario do things go wrong? >> It is still unclear to me why we need this sanity check. > > Here struct eth_pdata is the structure for the platform data. So you should > use: > > struct eth_pdata *plat = dev_get_platdata(dev, struct eth_pdata); > > That is the error - we are using the platdata structure to access > priv. At present there is no way to detect these errors except by > inspection (or perhaps crash / strange behaviour). > >> >> >> >>> Invalid access to device priv: dev=eth@10002000, expecting >>> 'struct eth_sandbox_priv', requested 'struct eth_pdata' >>> >>> 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. >>> >>> 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> >>> --- >> >> >> >> >> >> >> >> >>> diff --git a/arch/x86/cpu/mp_init.c b/arch/x86/cpu/mp_init.c >>> index e686b28..e835b43 100644 >>> --- a/arch/x86/cpu/mp_init.c >>> +++ b/arch/x86/cpu/mp_init.c >>> @@ -238,7 +238,7 @@ static int load_sipi_vector(atomic_t **ap_countp) >>> >>> params->stack_size = CONFIG_AP_STACK_SIZE; >>> size = params->stack_size * CONFIG_MAX_CPUS; >>> - stack = memalign(size, 4096); >>> + stack = memalign(4096, size); >>> if (!stack) >>> return -ENOMEM; >>> params->stack_top = (u32)(stack + size); >> >> >> This change seems unrelated to the subject of this patch. >> > > Yes, I'll respin it properly when we work out what to do. > > Regards, > Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
