[U-Boot] [PATCH 4/5] dm: core: Remove the call to device_free()

[Simon Glass]

Hi Bin,

On Sep 10, 2015 5:47 PM, "Bin Meng" <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Fri, Sep 11, 2015 at 8:38 AM, Simon Glass <sjg at chromium.org> wrote:
> > Hi Bin,
> >
> > On 10 September 2015 at 00:07, Bin Meng <bmeng.cn at gmail.com> wrote:
> >> Hi Simon,
> >>
> >> On Thu, Sep 10, 2015 at 2:07 AM, Simon Glass <sjg at chromium.org> wrote:
> >>> Hi Bin,
> >>>
> >>> On Friday, 4 September 2015, Bin Meng <bmeng.cn at gmail.com> wrote:
> >>>>
> >>>> When something goes wrong during device_probe(), we just need do
> >>>> device_remove() which calls device_free() internally.
> >>>>
> >>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> >>>> ---
> >>>>
> >>>>  drivers/core/device.c | 9 ++-------
> >>>>  1 file changed, 2 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/drivers/core/device.c b/drivers/core/device.c
> >>>> index a6cd936..061a7ef 100644
> >>>> --- a/drivers/core/device.c
> >>>> +++ b/drivers/core/device.c
> >>>> @@ -316,19 +316,14 @@ int device_probe_child(struct udevice *dev,
void *parent_priv)
> >>>>
> >>>>         ret = uclass_post_probe_device(dev);
> >>>>         if (ret)
> >>>> -               goto fail_uclass;
> >>>> +               goto fail;
> >>>>
> >>>>         return 0;
> >>>> -fail_uclass:
> >>>> +fail:
> >>>>         if (device_remove(dev)) {
> >>>>                 dm_warn("%s: Device '%s' failed to remove on error
path\n",
> >>>>                         __func__, dev->name);
> >>>>         }
> >>>> -fail:
> >>>> -       dev->flags &= ~DM_FLAG_ACTIVATED;
> >>>> -
> >>>> -       dev->seq = -1;
> >>>> -       device_free(dev);
> >>>>
> >>>>         return ret;
> >>>>  }
> >>>
> >>>
> >>> The problem is that in this case you end up calling functions that
> >>> should not be called. For example uclass_pre_remove_device() will be
> >>> called even if uclass_post_probe_device() was not.
> >>>
> >>
> >> Yes, but doing uclass_pre_remove_device() should not bring any harm,
> >> given we already want to remove this device, right?
> >
> > That function is entitled to assume that the device was set up at
> > least to the post_remove uclass state. Here that would not be try, so
> > we are creating an entirely new test case.
> >
> >>
> >>> There is definitely room for improvement here - we could/should call
> >>> only those 'remove' functions that mirror the 'probe' ones we called.
> >>> But that is quite a lot of code for little benefit.
> >
>
> OK, will drop this.

Sorry, that was complete gibberish.

That function is entitled to assume that the device was set up at least to
the post_probe uclass state. Here that would not be true, so we are
creating an entirely new test case.

Regards,
Simon