[U-Boot] [PATCH v3 2/4] dm: core: Test dev->flags after device_probe() in device_probe_child()

[Bin Meng]

Hi Simon,

On Mon, Aug 24, 2015 at 9:04 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Bin,
>
> On 23 August 2015 at 02:32, Bin Meng <bmeng.cn at gmail.com> wrote:
>> The device might have already been probed during the call to
>> device_probe() on its parent device (e.g. PCI bridge devices).
>> Test the flags again so that we don't mess up the device.
>>
>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>> ---
>>
>> Changes in v3: None
>>
>> drivers/core/device.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>> index e23a872..7a62812 100644
>> --- a/drivers/core/device.c
>> +++ b/drivers/core/device.c
>> @@ -266,6 +266,14 @@ int device_probe_child(struct udevice *dev, void
>> *parent_priv)
>> goto fail;
>> }
>>
>> + /*
>> + * The device might have already been probed during the call to
>> + * device_probe() on its parent device (e.g. PCI bridge devices).
>> + * Test the flags again so that we don't mess up the device.
>> + */
>> + if (dev->flags & DM_FLAG_ACTIVATED)
>> + return 0;
>> +
>> seq = uclass_resolve_seq(dev);
>> if (seq < 0) {
>> ret = seq;
>> --
>> 1.8.2.1
>>
>
> This seems problematic since presumably we reenter device_probe_child() in
> this case. We then re-allocate the memory and so there is a memory leak.
>

Ah, yes! There is a memory leak.

> Firstly I think this code should go inside the if (dev->parent) part.
>
> Secondly I think we need a way to detect thie re-entry and do the right
> thing. But it is pretty ugly - we essentially need to skip the memory
> allocation. Maybe another flag? Ick.

I think we can just test these to see if they are NULL? Like this:

/* Allocate private data if requested */
if (drv->priv_auto_alloc_size && !dev->priv) {
    dev->priv = alloc_priv(drv->priv_auto_alloc_size, drv->flags);
    if (!dev->priv) {
        ret = -ENOMEM;
        goto fail;
    }
}

>
> Can you give me your thoughts on this and maybe we can work out a solution
> that isn't too ugly. The expectation is that probing a parent should not
> probe its children, so we need to deal with it as a special case. We should
> probably also put an assert in to detect this problem.
>

If probing a parent should not probe its children, I am afraid the
pci-uclass driver needs rewritten.

Regards,
Bin