| Message ID | 1440318781-11321-3-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Bin, On 23 August 2015 at 02:32, Bin Meng <bmeng.cn@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@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. 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. 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. Regards, Simon
Hi Simon, On Mon, Aug 24, 2015 at 9:04 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 23 August 2015 at 02:32, Bin Meng <bmeng.cn@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@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
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;
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@gmail.com> --- Changes in v3: None drivers/core/device.c | 8 ++++++++ 1 file changed, 8 insertions(+)