[PATCH] dm: core: Check flags before removing devices

[PATCH] dm: core: Check flags before removing devices

[Marek Vasut]

Calling device_chld_remove() before flags_remove() means all devices
get removed no matter whether they should be removed late or not. This
breaks teardown of eMMC when booting and other critical boot paths.

Fixes: c51d2e704a1 ("dm: core: Avoid partially removing devices")
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Simon Glass <sjg@chromium.org>
---
 drivers/core/device-remove.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
index e6ec6ff4212..0454f55c330 100644
--- a/drivers/core/device-remove.c
+++ b/drivers/core/device-remove.c
@@ -207,14 +207,6 @@ int device_remove(struct udevice *dev, uint flags)
 	if (!(dev_get_flags(dev) & DM_FLAG_ACTIVATED))
 		return 0;
 
-	/*
-	 * If the child returns EKEYREJECTED, continue. It just means that it
-	 * didn't match the flags.
-	 */
-	ret = device_chld_remove(dev, NULL, flags);
-	if (ret && ret != -EKEYREJECTED)
-		return ret;
-
 	/*
 	 * Remove the device if called with the "normal" remove flag set,
 	 * or if the remove flag matches any of the drivers remove flags
@@ -228,6 +220,14 @@ int device_remove(struct udevice *dev, uint flags)
 		return ret;
 	}
 
+	/*
+	 * If the child returns EKEYREJECTED, continue. It just means that it
+	 * didn't match the flags.
+	 */
+	ret = device_chld_remove(dev, NULL, flags);
+	if (ret && ret != -EKEYREJECTED)
+		return ret;
+
 	ret = uclass_pre_remove_device(dev);
 	if (ret)
 		return ret;
-- 
2.34.1

Re: [PATCH] dm: core: Check flags before removing devices

[Simon Glass]

Hi Marek,

On Thu, 27 Jan 2022 at 20:41, Marek Vasut <marex@denx.de> wrote:
>
> Calling device_chld_remove() before flags_remove() means all devices
> get removed no matter whether they should be removed late or not. This
> breaks teardown of eMMC when booting and other critical boot paths.
>
> Fixes: c51d2e704a1 ("dm: core: Avoid partially removing devices")
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  drivers/core/device-remove.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

This means that the children do not get the remove signal if
-EPROBE_DEFER or -EKEYREJECTED are returned by the 'dev' device.

Also it fails several tests ('make qcheck').

>
> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
> index e6ec6ff4212..0454f55c330 100644
> --- a/drivers/core/device-remove.c
> +++ b/drivers/core/device-remove.c
> @@ -207,14 +207,6 @@ int device_remove(struct udevice *dev, uint flags)
>         if (!(dev_get_flags(dev) & DM_FLAG_ACTIVATED))
>                 return 0;
>
> -       /*
> -        * If the child returns EKEYREJECTED, continue. It just means that it
> -        * didn't match the flags.
> -        */
> -       ret = device_chld_remove(dev, NULL, flags);
> -       if (ret && ret != -EKEYREJECTED)
> -               return ret;
> -
>         /*
>          * Remove the device if called with the "normal" remove flag set,
>          * or if the remove flag matches any of the drivers remove flags
> @@ -228,6 +220,14 @@ int device_remove(struct udevice *dev, uint flags)
>                 return ret;
>         }
>
> +       /*
> +        * If the child returns EKEYREJECTED, continue. It just means that it
> +        * didn't match the flags.
> +        */
> +       ret = device_chld_remove(dev, NULL, flags);
> +       if (ret && ret != -EKEYREJECTED)
> +               return ret;
> +
>         ret = uclass_pre_remove_device(dev);
>         if (ret)
>                 return ret;
> --
> 2.34.1
>

Regards,
Simon

Re: [PATCH] dm: core: Check flags before removing devices

[Marek Vasut]

On 2/11/22 16:05, Simon Glass wrote:
> Hi Marek,

Hi,

>> Calling device_chld_remove() before flags_remove() means all devices
>> get removed no matter whether they should be removed late or not. This
>> breaks teardown of eMMC when booting and other critical boot paths.
>>
>> Fixes: c51d2e704a1 ("dm: core: Avoid partially removing devices")
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> ---
>>   drivers/core/device-remove.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> This means that the children do not get the remove signal if
> -EPROBE_DEFER or -EKEYREJECTED are returned by the 'dev' device.
> 
> Also it fails several tests ('make qcheck').

Do you have an idea for a better fix, one which doesn't break booting 
Linux from U-Boot ? I think that's a rather important use-case .

Re: [PATCH] dm: core: Check flags before removing devices

[Simon Glass]

Hi Marek,

On Fri, 11 Feb 2022 at 08:24, Marek Vasut <marex@denx.de> wrote:
>
> On 2/11/22 16:05, Simon Glass wrote:
> > Hi Marek,
>
> Hi,
>
> >> Calling device_chld_remove() before flags_remove() means all devices
> >> get removed no matter whether they should be removed late or not. This
> >> breaks teardown of eMMC when booting and other critical boot paths.
> >>
> >> Fixes: c51d2e704a1 ("dm: core: Avoid partially removing devices")
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Simon Glass <sjg@chromium.org>
> >> ---
> >>   drivers/core/device-remove.c | 16 ++++++++--------
> >>   1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > This means that the children do not get the remove signal if
> > -EPROBE_DEFER or -EKEYREJECTED are returned by the 'dev' device.
> >
> > Also it fails several tests ('make qcheck').
>
> Do you have an idea for a better fix, one which doesn't break booting
> Linux from U-Boot ? I think that's a rather important use-case .

Well the problem is that I don't understand the problem.

Can you explain it in more detail? The commit message does not help
much and you have not added a test for the case you are trying to
enable.

We must remove children before their parents, since children may rely
on their parents to be around until they are removed. This is part of
the device lifecycle as documented.

So what specific devices are children here? Perhaps the output of 'dm
tree' would help.

Regards,
Simon