[PATCH] power-domain: fix hang in endless loop on i.MX8

[PATCH] power-domain: fix hang in endless loop on i.MX8

[Anatolij Gustschin]

Currently when booting the kernel on i.MX8 U-Boot hangs in an
endless loop when switching off dma, connectivity or lsio power
domains during device removal. It hapens first when removing
gpio0 (gpio at 5d080000) device, here its power domain device
'lsio_gpio0' is obtained for switching off power. Since the
obtained 'lsio_gpio0' device is removed afterwards, its power
domain is also switched off and here the parent power domain
device 'lsio_power_domain' is optained for switching off the
power. Thereafter, when the obtained 'lsio_power_domain' is
removed, device_remove() removes its first child 'lsio_gpio0'.
During this child removal the 'lsio_power_domain' device is
obtained again for switching and when removing it later,
the same child removal is repeated, so we are stuck in an
endless loop. Below is a snippet from dm tree on i.MX8QXP
for better illustration of the DM devices relationship:

 Class     Index  Probed  Driver                Name
-----------------------------------------------------------
 root          0  [ + ]   root_driver           root_driver
...
 simple_bus    0  [ + ]   generic_simple_bus    |-- imx8qx-pm
 power_doma    0  [ + ]   imx8_power_domain     |   |-- lsio_power_domain
 power_doma    1  [ + ]   imx8_power_domain     |   |   |-- lsio_gpio0
 power_doma    2  [ + ]   imx8_power_domain     |   |   |-- lsio_gpio1

Do not remove a power domain device if it is a parent of the
currently controlled device.

Fixes: 52edfed65de9 ("dm: core: device: switch off power domain after device removal")
Signed-off-by: Anatolij Gustschin <agust@denx.de>
Reported-by: Oliver Graute <oliver.graute@gmail.com>
Reported-by: Fabio Estevam <festevam@gmail.com>

---
 drivers/power/domain/power-domain-uclass.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/power/domain/power-domain-uclass.c b/drivers/power/domain/power-domain-uclass.c
index d9c623b56e..d8fe4d4877 100644
--- a/drivers/power/domain/power-domain-uclass.c
+++ b/drivers/power/domain/power-domain-uclass.c
@@ -127,6 +127,18 @@ static int dev_power_domain_ctrl(struct udevice *dev, bool on)
 			ret = power_domain_off(&pd);
 	}
 
+	/*
+	 * For platforms with parent and child power-domain devices
+	 * we may not run device_remove() on the power-domain parent
+	 * because it will result in removing its children and switching
+	 * off their power-domain parent. So we will get here again and
+	 * again and will be stuck in an endless loop.
+	 */
+	if (!on && dev_get_parent(dev) == pd.dev &&
+	    device_get_uclass_id(dev) == UCLASS_POWER_DOMAIN) {
+		return ret;
+	}
+
 	/*
 	 * power_domain_get() bound the device, thus
 	 * we must remove it again to prevent unbinding
-- 
2.17.1

[PATCH] power-domain: fix hang in endless loop on i.MX8

[Simon Glass]

On Sat, 15 Feb 2020 at 04:34, Anatolij Gustschin <agust@denx.de> wrote:
>
> Currently when booting the kernel on i.MX8 U-Boot hangs in an
> endless loop when switching off dma, connectivity or lsio power
> domains during device removal. It hapens first when removing
> gpio0 (gpio at 5d080000) device, here its power domain device
> 'lsio_gpio0' is obtained for switching off power. Since the
> obtained 'lsio_gpio0' device is removed afterwards, its power
> domain is also switched off and here the parent power domain
> device 'lsio_power_domain' is optained for switching off the
> power. Thereafter, when the obtained 'lsio_power_domain' is
> removed, device_remove() removes its first child 'lsio_gpio0'.
> During this child removal the 'lsio_power_domain' device is
> obtained again for switching and when removing it later,
> the same child removal is repeated, so we are stuck in an
> endless loop. Below is a snippet from dm tree on i.MX8QXP
> for better illustration of the DM devices relationship:
>
>  Class     Index  Probed  Driver                Name
> -----------------------------------------------------------
>  root          0  [ + ]   root_driver           root_driver
> ...
>  simple_bus    0  [ + ]   generic_simple_bus    |-- imx8qx-pm
>  power_doma    0  [ + ]   imx8_power_domain     |   |-- lsio_power_domain
>  power_doma    1  [ + ]   imx8_power_domain     |   |   |-- lsio_gpio0
>  power_doma    2  [ + ]   imx8_power_domain     |   |   |-- lsio_gpio1
>
> Do not remove a power domain device if it is a parent of the
> currently controlled device.
>
> Fixes: 52edfed65de9 ("dm: core: device: switch off power domain after device removal")
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Reported-by: Oliver Graute <oliver.graute@gmail.com>
> Reported-by: Fabio Estevam <festevam@gmail.com>
>
> ---
>  drivers/power/domain/power-domain-uclass.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

Can we have a sandbox test for this case?

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

[PATCH] power-domain: fix hang in endless loop on i.MX8

[Anatolij Gustschin]

Hi Simon, Peng,

On Sun, 16 Feb 2020 12:02:55 -0700
Simon Glass sjg at chromium.org wrote:
...
> >  drivers/power/domain/power-domain-uclass.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)  
> 
> Can we have a sandbox test for this case?

I didn't check if there are more platforms with such parent & child
power domain devices in U-Boot. If there are some, then it makes
sense to add a test. But for this i.MX8 case we will probably switch
to another power domain bindings and driver like in Linux (we have
to sync i.MX8 device trees with current Linux implementation which
is different and it doesn't have such parent/child PD devices).

The current i.MX8 PD U-Boot implementation seems wrong anyway, here
the parent power domain devices for dma, connectivity or lsio domains
actually do not control power switching, so there is no need to have
such parent&child PD devices on i.MX8.

@Peng: what do you think?

--
Anatolij

[PATCH] power-domain: fix hang in endless loop on i.MX8

[Peng Fan]

> Subject: Re: [PATCH] power-domain: fix hang in endless loop on i.MX8
> 
> Hi Simon, Peng,
> 
> On Sun, 16 Feb 2020 12:02:55 -0700
> Simon Glass sjg at chromium.org wrote:
> ...
> > >  drivers/power/domain/power-domain-uclass.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> >
> > Can we have a sandbox test for this case?
> 
> I didn't check if there are more platforms with such parent & child power
> domain devices in U-Boot. If there are some, then it makes sense to add a
> test. But for this i.MX8 case we will probably switch to another power domain
> bindings and driver like in Linux (we have to sync i.MX8 device trees with
> current Linux implementation which is different and it doesn't have such
> parent/child PD devices).
> 
> The current i.MX8 PD U-Boot implementation seems wrong anyway, here the
> parent power domain devices for dma, connectivity or lsio domains actually
> do not control power switching, so there is no need to have such parent&child
> PD devices on i.MX8.
> 
> @Peng: what do you think?

The current i.MX8 power domain driver needs finally be dropped. But I am still
waiting the i.MX8 linux dts restructure from Aisheng ready, pending for long time.

With Linux dts synced, there will no such parent/child in dts.

Regards,
Peng.


> 
> --
> Anatolij

[PATCH] power-domain: fix hang in endless loop on i.MX8

[Lokesh Vutla]

On 15/02/20 5:04 PM, Anatolij Gustschin wrote:
> Currently when booting the kernel on i.MX8 U-Boot hangs in an
> endless loop when switching off dma, connectivity or lsio power
> domains during device removal. It hapens first when removing
> gpio0 (gpio at 5d080000) device, here its power domain device
> 'lsio_gpio0' is obtained for switching off power. Since the
> obtained 'lsio_gpio0' device is removed afterwards, its power
> domain is also switched off and here the parent power domain
> device 'lsio_power_domain' is optained for switching off the
> power. Thereafter, when the obtained 'lsio_power_domain' is
> removed, device_remove() removes its first child 'lsio_gpio0'.
> During this child removal the 'lsio_power_domain' device is
> obtained again for switching and when removing it later,
> the same child removal is repeated, so we are stuck in an
> endless loop. Below is a snippet from dm tree on i.MX8QXP
> for better illustration of the DM devices relationship:
> 
>  Class     Index  Probed  Driver                Name
> -----------------------------------------------------------
>  root          0  [ + ]   root_driver           root_driver
> ...
>  simple_bus    0  [ + ]   generic_simple_bus    |-- imx8qx-pm
>  power_doma    0  [ + ]   imx8_power_domain     |   |-- lsio_power_domain
>  power_doma    1  [ + ]   imx8_power_domain     |   |   |-- lsio_gpio0
>  power_doma    2  [ + ]   imx8_power_domain     |   |   |-- lsio_gpio1
> 
> Do not remove a power domain device if it is a parent of the
> currently controlled device.
> 
> Fixes: 52edfed65de9 ("dm: core: device: switch off power domain after device removal")
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Reported-by: Oliver Graute <oliver.graute@gmail.com>
> Reported-by: Fabio Estevam <festevam@gmail.com>
> 
> ---
>  drivers/power/domain/power-domain-uclass.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/power/domain/power-domain-uclass.c b/drivers/power/domain/power-domain-uclass.c
> index d9c623b56e..d8fe4d4877 100644
> --- a/drivers/power/domain/power-domain-uclass.c
> +++ b/drivers/power/domain/power-domain-uclass.c
> @@ -127,6 +127,18 @@ static int dev_power_domain_ctrl(struct udevice *dev, bool on)
>  			ret = power_domain_off(&pd);
>  	}
>  
> +	/*
> +	 * For platforms with parent and child power-domain devices
> +	 * we may not run device_remove() on the power-domain parent
> +	 * because it will result in removing its children and switching
> +	 * off their power-domain parent. So we will get here again and
> +	 * again and will be stuck in an endless loop.
> +	 */
> +	if (!on && dev_get_parent(dev) == pd.dev &&
> +	    device_get_uclass_id(dev) == UCLASS_POWER_DOMAIN) {
> +		return ret;
> +	}

Please drop the braces here. Otherwise:

Reviewed-by: Lokesh Vutla <lokeshvutla@ti.com>

Thanks and regards,
Lokesh

[PATCH] power-domain: fix hang in endless loop on i.MX8

[Oliver Graute]

On 15/02/20, Anatolij Gustschin wrote:
> Currently when booting the kernel on i.MX8 U-Boot hangs in an
> endless loop when switching off dma, connectivity or lsio power
> domains during device removal. It hapens first when removing
> gpio0 (gpio at 5d080000) device, here its power domain device
> 'lsio_gpio0' is obtained for switching off power. Since the
> obtained 'lsio_gpio0' device is removed afterwards, its power
> domain is also switched off and here the parent power domain
> device 'lsio_power_domain' is optained for switching off the
> power. Thereafter, when the obtained 'lsio_power_domain' is
> removed, device_remove() removes its first child 'lsio_gpio0'.
> During this child removal the 'lsio_power_domain' device is
> obtained again for switching and when removing it later,
> the same child removal is repeated, so we are stuck in an
> endless loop. Below is a snippet from dm tree on i.MX8QXP
> for better illustration of the DM devices relationship:
> 
>  Class     Index  Probed  Driver                Name
> -----------------------------------------------------------
>  root          0  [ + ]   root_driver           root_driver
> ...
>  simple_bus    0  [ + ]   generic_simple_bus    |-- imx8qx-pm
>  power_doma    0  [ + ]   imx8_power_domain     |   |-- lsio_power_domain
>  power_doma    1  [ + ]   imx8_power_domain     |   |   |-- lsio_gpio0
>  power_doma    2  [ + ]   imx8_power_domain     |   |   |-- lsio_gpio1
> 
> Do not remove a power domain device if it is a parent of the
> currently controlled device.
> 
> Fixes: 52edfed65de9 ("dm: core: device: switch off power domain after device removal")
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Reported-by: Oliver Graute <oliver.graute@gmail.com>
> Reported-by: Fabio Estevam <festevam@gmail.com>
> 
> ---
>  drivers/power/domain/power-domain-uclass.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/power/domain/power-domain-uclass.c b/drivers/power/domain/power-domain-uclass.c
> index d9c623b56e..d8fe4d4877 100644
> --- a/drivers/power/domain/power-domain-uclass.c
> +++ b/drivers/power/domain/power-domain-uclass.c
> @@ -127,6 +127,18 @@ static int dev_power_domain_ctrl(struct udevice *dev, bool on)
>  			ret = power_domain_off(&pd);
>  	}
>  
> +	/*
> +	 * For platforms with parent and child power-domain devices
> +	 * we may not run device_remove() on the power-domain parent
> +	 * because it will result in removing its children and switching
> +	 * off their power-domain parent. So we will get here again and
> +	 * again and will be stuck in an endless loop.
> +	 */
> +	if (!on && dev_get_parent(dev) == pd.dev &&
> +	    device_get_uclass_id(dev) == UCLASS_POWER_DOMAIN) {
> +		return ret;
> +	}
> +
>  	/*
>  	 * power_domain_get() bound the device, thus
>  	 * we must remove it again to prevent unbinding
> -- 
> 2.17.1

thx, just tested this and now I can drop my revert mentioned here:

https://lists.denx.de/pipermail/u-boot/2020-February/398910.html

Best regards,

Oliver

[PATCH] power-domain: fix hang in endless loop on i.MX8

[Neil Armstrong]

Hi,

On 17/02/2020 10:07, Oliver Graute wrote:
> On 15/02/20, Anatolij Gustschin wrote:
>> Currently when booting the kernel on i.MX8 U-Boot hangs in an
>> endless loop when switching off dma, connectivity or lsio power
>> domains during device removal. It hapens first when removing
>> gpio0 (gpio at 5d080000) device, here its power domain device
>> 'lsio_gpio0' is obtained for switching off power. Since the
>> obtained 'lsio_gpio0' device is removed afterwards, its power
>> domain is also switched off and here the parent power domain
>> device 'lsio_power_domain' is optained for switching off the
>> power. Thereafter, when the obtained 'lsio_power_domain' is
>> removed, device_remove() removes its first child 'lsio_gpio0'.
>> During this child removal the 'lsio_power_domain' device is
>> obtained again for switching and when removing it later,
>> the same child removal is repeated, so we are stuck in an
>> endless loop. Below is a snippet from dm tree on i.MX8QXP
>> for better illustration of the DM devices relationship:
>>
>>  Class     Index  Probed  Driver                Name
>> -----------------------------------------------------------
>>  root          0  [ + ]   root_driver           root_driver
>> ...
>>  simple_bus    0  [ + ]   generic_simple_bus    |-- imx8qx-pm
>>  power_doma    0  [ + ]   imx8_power_domain     |   |-- lsio_power_domain
>>  power_doma    1  [ + ]   imx8_power_domain     |   |   |-- lsio_gpio0
>>  power_doma    2  [ + ]   imx8_power_domain     |   |   |-- lsio_gpio1
>>
>> Do not remove a power domain device if it is a parent of the
>> currently controlled device.
>>
>> Fixes: 52edfed65de9 ("dm: core: device: switch off power domain after device removal")
>> Signed-off-by: Anatolij Gustschin <agust@denx.de>
>> Reported-by: Oliver Graute <oliver.graute@gmail.com>
>> Reported-by: Fabio Estevam <festevam@gmail.com>
>>
>> ---
>>  drivers/power/domain/power-domain-uclass.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/power/domain/power-domain-uclass.c b/drivers/power/domain/power-domain-uclass.c
>> index d9c623b56e..d8fe4d4877 100644
>> --- a/drivers/power/domain/power-domain-uclass.c
>> +++ b/drivers/power/domain/power-domain-uclass.c
>> @@ -127,6 +127,18 @@ static int dev_power_domain_ctrl(struct udevice *dev, bool on)
>>  			ret = power_domain_off(&pd);
>>  	}
>>  
>> +	/*
>> +	 * For platforms with parent and child power-domain devices
>> +	 * we may not run device_remove() on the power-domain parent
>> +	 * because it will result in removing its children and switching
>> +	 * off their power-domain parent. So we will get here again and
>> +	 * again and will be stuck in an endless loop.
>> +	 */
>> +	if (!on && dev_get_parent(dev) == pd.dev &&
>> +	    device_get_uclass_id(dev) == UCLASS_POWER_DOMAIN) {
>> +		return ret;
>> +	}
>> +
>>  	/*
>>  	 * power_domain_get() bound the device, thus
>>  	 * we must remove it again to prevent unbinding
>> -- 
>> 2.17.1
> 
> thx, just tested this and now I can drop my revert mentioned here:
> 
> https://lists.denx.de/pipermail/u-boot/2020-February/398910.html

The revert is still needed... it breaks multiple amlogic boards and the fix
is not as simple as this one.

Neil

> 
> Best regards,
> 
> Oliver
>