dm: device_remove: Don't return in device_chld_remove() upon error

On my x86 platform I've noticed, that calling dm_uninit() or the new function dm_remove_devices_flags() does not remove the desired device at all. Debugging showed, that the serial uclass returns -EPERM in serial_pre_remove(). This patch sets the force parameter when calling stdio_deregister_dev() resulting in a removal of the device.

Signed-off-by Stefan Roese <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

---
v2:
- New patch

 drivers/serial/serial-uclass.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 13 lines · Stefan Roese

On my x86 platform I've noticed, that calling dm_uninit() or the new function dm_remove_devices_flags() does not remove the desired device at all. Debugging showed, that the serial uclass returns -EPERM in serial_pre_remove(). This patch sets the force parameter when calling stdio_deregister_dev() resulting in a removal of the device.

Signed-off-by Stefan Roese <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

---
v2:
- New patch

Quoted 2 lines

 drivers/serial/serial-uclass.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by Simon Glass <[email protected]>

This new flag can be added to DM device drivers, which need to do some final configuration before U-Boot exits and the OS (e.g. Linux) is started. The remove functions of those drivers will get called at this stage to do these last-stage configuration steps.

Signed-off-by Stefan Roese <[email protected]>

Reviewed-by Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

---
- Renamed flag to DM_FLAG_OS_PREPARE

 drivers/core/device-remove.c | 16 +++++++++++-----
 include/dm/device.h          | 11 ++++++++++-
 2 files changed, 21 insertions(+), 6 deletions(-)

This patch adds a call to dm_remove_devices_flags() to bootm_announce_and_cleanup() so that drivers that have one of the removal flags set (e.g. DM_FLAG_ACTIVE_DMA_REMOVE) in their driver struct, may do some last-stage cleanup before the OS is started.

Signed-off-by Stefan Roese <[email protected]>

Cc Bin Meng <[email protected]>

Reviewed-by Simon Glass <[email protected]>

---
v2:
- Added Simons RB line

 arch/x86/lib/bootm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

This patch adds a remove function to the Intel ICH SPI driver, that will be called upon U-Boot exit, directly before the OS (Linux) is started. This function takes care of configuring the BIOS registers in the SPI controller (similar to what a "standard" BIOS or coreboot does), so that the Linux MTD device driver is able to correctly read/write to the SPI NOR chip. Without this, the chip is not detected at all.

Signed-off-by Stefan Roese <[email protected]>

Reviewed-by Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Jagan Teki <[email protected]>

---
v2:
- Added Simons RB line

 drivers/spi/ich.c | 18 ++++++++++++++++++
 drivers/spi/ich.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 65 insertions(+), 7 deletions(-)

This patch adds a remove function to the Intel ICH SPI driver, that will be called upon U-Boot exit, directly before the OS (Linux) is started. This function takes care of configuring the BIOS registers in the SPI controller (similar to what a "standard" BIOS or coreboot does), so that the Linux MTD device driver is able to correctly read/write to the SPI NOR chip. Without this, the chip is not detected at all.

Signed-off-by Stefan Roese <[email protected]>

Reviewed-by Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Jagan Teki <[email protected]>

---
v2:
- Added Simons RB line

 drivers/spi/ich.c | 18 ++++++++++++++++++
 drivers/spi/ich.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 65 insertions(+), 7 deletions(-)

drivers/spi/ich.c +18 -0

index 893fe33b66..bf2e99b5cc 100644
@@ -617,6 +617,22 @@ static int ich_spi_probe(struct udevice *dev)
        return 0;
 }
 
+static int ich_spi_remove(struct udevice *bus)
+{
+       struct ich_spi_priv *ctlr = dev_get_priv(bus);
+
+       /*
+        * Configure SPI controller so that the Linux MTD driver can fully
+        * access the SPI NOR chip
+        */
+       ich_writew(ctlr, SPI_OPPREFIX, ctlr->preop);
+       ich_writew(ctlr, SPI_OPTYPE, ctlr->optype);
+       ich_writel(ctlr, SPI_OPMENU_LOWER, ctlr->opmenu);
+       ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32));
+
+       return 0;
+}
+
 static int ich_spi_set_speed(struct udevice *bus, uint speed)
 {
        struct ich_spi_priv *priv = dev_get_priv(bus);

@@ -700,4 +716,6 @@ U_BOOT_DRIVER(ich_spi) = {
        .priv_auto_alloc_size = sizeof(struct ich_spi_priv),
        .child_pre_probe = ich_spi_child_pre_probe,
        .probe  = ich_spi_probe,
+       .remove = ich_spi_remove,
+       .flags  = DM_FLAG_OS_PREPARE,
 };

drivers/spi/ich.h +0 -7

index bd0a820809..dcb8a9048f 100644
@@ -102,13 +102,6 @@ enum {
 };
 
 enum {
-       SPI_OPCODE_TYPE_READ_NO_ADDRESS =       0,
-       SPI_OPCODE_TYPE_WRITE_NO_ADDRESS =      1,
-       SPI_OPCODE_TYPE_READ_WITH_ADDRESS =     2,
-       SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS =    3
-};
-
-enum {
        ICH_MAX_CMD_LEN         = 5,
 };
 

@@ -124,8 +117,55 @@ struct spi_trans {
        uint32_t offset;
 };
 

Quoted 75 lines · Stefan Roese

This patch adds a remove function to the Intel ICH SPI driver, that will be called upon U-Boot exit, directly before the OS (Linux) is started. This function takes care of configuring the BIOS registers in the SPI controller (similar to what a "standard" BIOS or coreboot does), so that the Linux MTD device driver is able to correctly read/write to the SPI NOR chip. Without this, the chip is not detected at all.

Signed-off-by Stefan Roese <[email protected]>

Reviewed-by Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Jagan Teki <[email protected]>

---
v2:
- Added Simons RB line

 drivers/spi/ich.c | 18 ++++++++++++++++++
 drivers/spi/ich.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 65 insertions(+), 7 deletions(-)

drivers/spi/ich.c +18 -0

index 893fe33b66..bf2e99b5cc 100644
@@ -617,6 +617,22 @@ static int ich_spi_probe(struct udevice *dev)
        return 0;
 }
 
+static int ich_spi_remove(struct udevice *bus)
+{
+       struct ich_spi_priv *ctlr = dev_get_priv(bus);
+
+       /*
+        * Configure SPI controller so that the Linux MTD driver can fully
+        * access the SPI NOR chip
+        */
+       ich_writew(ctlr, SPI_OPPREFIX, ctlr->preop);
+       ich_writew(ctlr, SPI_OPTYPE, ctlr->optype);
+       ich_writel(ctlr, SPI_OPMENU_LOWER, ctlr->opmenu);
+       ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32));
+
+       return 0;
+}
+
 static int ich_spi_set_speed(struct udevice *bus, uint speed)
 {
        struct ich_spi_priv *priv = dev_get_priv(bus);

@@ -700,4 +716,6 @@ U_BOOT_DRIVER(ich_spi) = {
        .priv_auto_alloc_size = sizeof(struct ich_spi_priv),
        .child_pre_probe = ich_spi_child_pre_probe,
        .probe  = ich_spi_probe,
+       .remove = ich_spi_remove,
+       .flags  = DM_FLAG_OS_PREPARE,
 };

drivers/spi/ich.h +0 -7

index bd0a820809..dcb8a9048f 100644
@@ -102,13 +102,6 @@ enum {
 };
 
 enum {
-       SPI_OPCODE_TYPE_READ_NO_ADDRESS =       0,
-       SPI_OPCODE_TYPE_WRITE_NO_ADDRESS =      1,
-       SPI_OPCODE_TYPE_READ_WITH_ADDRESS =     2,
-       SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS =    3
-};
-
-enum {
        ICH_MAX_CMD_LEN         = 5,
 };
 

@@ -124,8 +117,55 @@ struct spi_trans {
        uint32_t offset;
 };
 

drivers/spi/ich.h struct spi_trans { · line 129

Quoted 10 lines

drivers/spi/ich.h +8 -0

+#define SPI_OPCODE_WRSR                0x01
+#define SPI_OPCODE_PAGE_PROGRAM        0x02
+#define SPI_OPCODE_READ                0x03
+#define SPI_OPCODE_WRDIS       0x04
+#define SPI_OPCODE_RDSR                0x05
 #define SPI_OPCODE_WREN                0x06
 #define SPI_OPCODE_FAST_READ   0x0b
+#define SPI_OPCODE_ERASE_SECT  0x20
+#define SPI_OPCODE_READ_ID     0x9f
+#define SPI_OPCODE_ERASE_BLOCK 0xd8

Wonder why the flash part should be part of SPI, can't we use existing spi_flash through command interface if there is specific stuff like this?

(Added Simon to Cc)

Quoted 80 lines · Stefan Roese

Quoted 78 lines

This patch adds a remove function to the Intel ICH SPI driver, that will be called upon U-Boot exit, directly before the OS (Linux) is started. This function takes care of configuring the BIOS registers in the SPI controller (similar to what a "standard" BIOS or coreboot does), so that the Linux MTD device driver is able to correctly read/write to the SPI NOR chip. Without this, the chip is not detected at all.

Signed-off-by Stefan Roese <[email protected]>

Reviewed-by Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Jagan Teki <[email protected]>

---
v2:
- Added Simons RB line

 drivers/spi/ich.c | 18 ++++++++++++++++++
 drivers/spi/ich.h | 54
+++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 65 insertions(+), 7 deletions(-)

drivers/spi/ich.c +6 -0

index 893fe33b66..bf2e99b5cc 100644
@@ -617,6 +617,22 @@ static int ich_spi_probe(struct udevice *dev)
        return 0;
 }
 
+static int ich_spi_remove(struct udevice *bus)
+{
+       struct ich_spi_priv *ctlr = dev_get_priv(bus);
+
+       /*
+        * Configure SPI controller so that the Linux MTD driver can

fully

drivers/spi/ich.c +12 -0

+        * access the SPI NOR chip
+        */
+       ich_writew(ctlr, SPI_OPPREFIX, ctlr->preop);
+       ich_writew(ctlr, SPI_OPTYPE, ctlr->optype);
+       ich_writel(ctlr, SPI_OPMENU_LOWER, ctlr->opmenu);
+       ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32));
+
+       return 0;
+}
+
 static int ich_spi_set_speed(struct udevice *bus, uint speed)
 {
        struct ich_spi_priv *priv = dev_get_priv(bus);

@@ -700,4 +716,6 @@ U_BOOT_DRIVER(ich_spi) = {
        .priv_auto_alloc_size = sizeof(struct ich_spi_priv),
        .child_pre_probe = ich_spi_child_pre_probe,
        .probe  = ich_spi_probe,
+       .remove = ich_spi_remove,
+       .flags  = DM_FLAG_OS_PREPARE,
 };

drivers/spi/ich.h +0 -7

index bd0a820809..dcb8a9048f 100644
@@ -102,13 +102,6 @@ enum {
 };
 
 enum {
-       SPI_OPCODE_TYPE_READ_NO_ADDRESS =       0,
-       SPI_OPCODE_TYPE_WRITE_NO_ADDRESS =      1,
-       SPI_OPCODE_TYPE_READ_WITH_ADDRESS =     2,
-       SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS =    3
-};
-
-enum {
        ICH_MAX_CMD_LEN         = 5,
 };
 

@@ -124,8 +117,55 @@ struct spi_trans {
        uint32_t offset;
 };
 

Quoted 7 lines

Quoted 7 lines

drivers/spi/ich.h +5 -0

+#define SPI_OPCODE_WRSR                0x01
+#define SPI_OPCODE_PAGE_PROGRAM        0x02
+#define SPI_OPCODE_READ                0x03
+#define SPI_OPCODE_WRDIS       0x04
+#define SPI_OPCODE_RDSR                0x05
 #define SPI_OPCODE_WREN                0x06
 #define SPI_OPCODE_FAST_READ   0x0b

Quoted 8 lines

drivers/spi/ich.h struct spi_trans { · line 129

Quoted 3 lines

drivers/spi/ich.h +3 -0

+#define SPI_OPCODE_ERASE_SECT  0x20
+#define SPI_OPCODE_READ_ID     0x9f
+#define SPI_OPCODE_ERASE_BLOCK 0xd8

Wonder why the flash part should be part of SPI, can't we use existing spi_flash through command interface if there is specific stuff like this?

This patch only changes some defines here and passes some allowed opcodes via some configuration registers to the Linux driver.

I didn't look closely into this U-Boot driver and how it interacts with the SPI NOR. Simon is most likely the best person to answer on your questions regarding the usage of spi_flash. Simon could you please answer Jagan's questions?

Quoted 1 line · Stefan Roese

(Added Simon to Cc)

Really added Simon :-)

Quoted 83 lines · Jagan Teki

Quoted 80 lines · Stefan Roese

Quoted 78 lines

This patch adds a remove function to the Intel ICH SPI driver, that will be called upon U-Boot exit, directly before the OS (Linux) is started. This function takes care of configuring the BIOS registers in the SPI controller (similar to what a "standard" BIOS or coreboot does), so that the Linux MTD device driver is able to correctly read/write to the SPI NOR chip. Without this, the chip is not detected at all.

Signed-off-by Stefan Roese <[email protected]>

Reviewed-by Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Jagan Teki <[email protected]>

---
v2:
- Added Simons RB line

 drivers/spi/ich.c | 18 ++++++++++++++++++
 drivers/spi/ich.h | 54
+++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 65 insertions(+), 7 deletions(-)

drivers/spi/ich.c +6 -0

index 893fe33b66..bf2e99b5cc 100644
@@ -617,6 +617,22 @@ static int ich_spi_probe(struct udevice *dev)
        return 0;
 }
 
+static int ich_spi_remove(struct udevice *bus)
+{
+       struct ich_spi_priv *ctlr = dev_get_priv(bus);
+
+       /*
+        * Configure SPI controller so that the Linux MTD driver can

fully

drivers/spi/ich.c +12 -0

+        * access the SPI NOR chip
+        */
+       ich_writew(ctlr, SPI_OPPREFIX, ctlr->preop);
+       ich_writew(ctlr, SPI_OPTYPE, ctlr->optype);
+       ich_writel(ctlr, SPI_OPMENU_LOWER, ctlr->opmenu);
+       ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32));
+
+       return 0;
+}
+
 static int ich_spi_set_speed(struct udevice *bus, uint speed)
 {
        struct ich_spi_priv *priv = dev_get_priv(bus);

@@ -700,4 +716,6 @@ U_BOOT_DRIVER(ich_spi) = {
        .priv_auto_alloc_size = sizeof(struct ich_spi_priv),
        .child_pre_probe = ich_spi_child_pre_probe,
        .probe  = ich_spi_probe,
+       .remove = ich_spi_remove,
+       .flags  = DM_FLAG_OS_PREPARE,
 };

drivers/spi/ich.h +0 -7

index bd0a820809..dcb8a9048f 100644
@@ -102,13 +102,6 @@ enum {
 };
 
 enum {
-       SPI_OPCODE_TYPE_READ_NO_ADDRESS =       0,
-       SPI_OPCODE_TYPE_WRITE_NO_ADDRESS =      1,
-       SPI_OPCODE_TYPE_READ_WITH_ADDRESS =     2,
-       SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS =    3
-};
-
-enum {
        ICH_MAX_CMD_LEN         = 5,
 };
 

@@ -124,8 +117,55 @@ struct spi_trans {
        uint32_t offset;
 };
 

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

drivers/spi/ich.h +5 -0

+#define SPI_OPCODE_WRSR                0x01
+#define SPI_OPCODE_PAGE_PROGRAM        0x02
+#define SPI_OPCODE_READ                0x03
+#define SPI_OPCODE_WRDIS       0x04
+#define SPI_OPCODE_RDSR                0x05
 #define SPI_OPCODE_WREN                0x06
 #define SPI_OPCODE_FAST_READ   0x0b

Quoted 8 lines

drivers/spi/ich.h struct spi_trans { · line 129

Quoted 3 lines

drivers/spi/ich.h +3 -0

+#define SPI_OPCODE_ERASE_SECT  0x20
+#define SPI_OPCODE_READ_ID     0x9f
+#define SPI_OPCODE_ERASE_BLOCK 0xd8

Wonder why the flash part should be part of SPI, can't we use existing spi_flash through command interface if there is specific stuff like this?

These flash commands need to be programmed to SPI controller register, by Intel's design, to make Linux MTD driver happy. Possibly we may do like this, like get such value dynamically from spi_flash to program this to SPI controller? I once asked a question about this: if some other flash part that does not have the exact same command set as what was programmed to the SPI controller, will Linux MTD driver still work?

Quoted 1 line · Stefan Roese

(Added Simon to Cc)

Really added Simon :-)

Quoted 98 lines · Jagan Teki

Quoted 95 lines · Stefan Roese

Quoted 78 lines

This patch adds a remove function to the Intel ICH SPI driver, that will be called upon U-Boot exit, directly before the OS (Linux) is started. This function takes care of configuring the BIOS registers in the SPI controller (similar to what a "standard" BIOS or coreboot does), so that the Linux MTD device driver is able to correctly read/write to the SPI NOR chip. Without this, the chip is not detected at all.

Signed-off-by Stefan Roese <[email protected]>

Reviewed-by Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Jagan Teki <[email protected]>

---
v2:
- Added Simons RB line

 drivers/spi/ich.c | 18 ++++++++++++++++++
 drivers/spi/ich.h | 54
+++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 65 insertions(+), 7 deletions(-)

drivers/spi/ich.c +6 -0

index 893fe33b66..bf2e99b5cc 100644
@@ -617,6 +617,22 @@ static int ich_spi_probe(struct udevice *dev)
        return 0;
 }
 
+static int ich_spi_remove(struct udevice *bus)
+{
+       struct ich_spi_priv *ctlr = dev_get_priv(bus);
+
+       /*
+        * Configure SPI controller so that the Linux MTD driver can

fully

drivers/spi/ich.c +12 -0

+        * access the SPI NOR chip
+        */
+       ich_writew(ctlr, SPI_OPPREFIX, ctlr->preop);
+       ich_writew(ctlr, SPI_OPTYPE, ctlr->optype);
+       ich_writel(ctlr, SPI_OPMENU_LOWER, ctlr->opmenu);
+       ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32));
+
+       return 0;
+}
+
 static int ich_spi_set_speed(struct udevice *bus, uint speed)
 {
        struct ich_spi_priv *priv = dev_get_priv(bus);

@@ -700,4 +716,6 @@ U_BOOT_DRIVER(ich_spi) = {
        .priv_auto_alloc_size = sizeof(struct ich_spi_priv),
        .child_pre_probe = ich_spi_child_pre_probe,
        .probe  = ich_spi_probe,
+       .remove = ich_spi_remove,
+       .flags  = DM_FLAG_OS_PREPARE,
 };

drivers/spi/ich.h +0 -7

index bd0a820809..dcb8a9048f 100644
@@ -102,13 +102,6 @@ enum {
 };
 
 enum {
-       SPI_OPCODE_TYPE_READ_NO_ADDRESS =       0,
-       SPI_OPCODE_TYPE_WRITE_NO_ADDRESS =      1,
-       SPI_OPCODE_TYPE_READ_WITH_ADDRESS =     2,
-       SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS =    3
-};
-
-enum {
        ICH_MAX_CMD_LEN         = 5,
 };
 

@@ -124,8 +117,55 @@ struct spi_trans {
        uint32_t offset;
 };
 

drivers/spi/ich.h struct spi_trans { · line 129

Quoted 10 lines

drivers/spi/ich.h +8 -0

+#define SPI_OPCODE_WRSR                0x01
+#define SPI_OPCODE_PAGE_PROGRAM        0x02
+#define SPI_OPCODE_READ                0x03
+#define SPI_OPCODE_WRDIS       0x04
+#define SPI_OPCODE_RDSR                0x05
 #define SPI_OPCODE_WREN                0x06
 #define SPI_OPCODE_FAST_READ   0x0b
+#define SPI_OPCODE_ERASE_SECT  0x20
+#define SPI_OPCODE_READ_ID     0x9f
+#define SPI_OPCODE_ERASE_BLOCK 0xd8

Wonder why the flash part should be part of SPI, can't we use existing spi_flash through command interface if there is specific stuff like this?

Quoted 1 line

These flash commands need to be programmed to SPI controller register, by Intel's design, to make Linux MTD driver happy. Possibly we may do like this, like get such value dynamically from spi_flash to program this to SPI controller? I once asked a question about this: if some other flash part that does not have the exact same command set as what was programmed to the SPI controller, will Linux MTD driver still work?

Quoted 4 lines

This patch only changes some defines here and passes some allowed opcodes via some configuration registers to the Linux driver.

Quoted 4 lines

I didn't look closely into this U-Boot driver and how it interacts with the SPI NOR. Simon is most likely the best person to answer on your questions regarding the usage of spi_flash. Simon could you please answer Jagan's questions?

Regards,
Bin

On my x86 platform I've noticed, that calling dm_uninit() or the new function dm_remove_devices_flags() does not remove the desired device at all. Debugging showed, that the serial uclass returns -EPERM in serial_pre_remove() and this leads to a complete stop of the device removal pretty early, as the serial device is one of the first ones in the DM. Here the dm tree output:

=> dm tree
Class Probed Name
----------------------------------------

 root        [ + ]    root_driver
 rsa_mod_exp [   ]    |-- mod_exp_sw
 serial      [ + ]    |-- serial
 rtc         [   ]    |-- rtc
 timer       [ + ]    |-- tsc-timer
 syscon      [ + ]    |-- pch_pinctrl

...

In this example, device_remove(root) will stop directly after trying to remove the "serial" device.

To solve this problem, this patch removes the return upon error check in the device_remove() call in device_chld_remove(). This leads to device_chld_remove() continuing with the device_remove() call to the following child devices.

Signed-off-by Stefan Roese <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

---
v2:
- Add debug() output in error case

 drivers/core/device-remove.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/core/device-remove.c +8 -3

index cc0043b990..a1c0103af0 100644
@@ -58,8 +58,14 @@ static int device_chld_remove(struct udevice *dev, uint flags)
 
        list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) {
                ret = device_remove(pos, flags);
-               if (ret)
-                       return ret;
+               /*
+                * Don't stop on error here, the remaining child devices still
+                * need to get removed.
+                */
+               if (ret) {
+                       debug("%s: device_remove(%s) returned %d\n",
+                             __func__, pos->name, ret);
+               }
        }
 
        return 0;
--

2.12.2

Quoted 33 lines · Stefan Roese

On my x86 platform I've noticed, that calling dm_uninit() or the new function dm_remove_devices_flags() does not remove the desired device at all. Debugging showed, that the serial uclass returns -EPERM in serial_pre_remove() and this leads to a complete stop of the device removal pretty early, as the serial device is one of the first ones in the DM. Here the dm tree output:

=> dm tree
Class Probed Name
----------------------------------------

 root        [ + ]    root_driver
 rsa_mod_exp [   ]    |-- mod_exp_sw
 serial      [ + ]    |-- serial
 rtc         [   ]    |-- rtc
 timer       [ + ]    |-- tsc-timer
 syscon      [ + ]    |-- pch_pinctrl

...

In this example, device_remove(root) will stop directly after trying to remove the "serial" device.

To solve this problem, this patch removes the return upon error check in the device_remove() call in device_chld_remove(). This leads to device_chld_remove() continuing with the device_remove() call to the following child devices.

Signed-off-by Stefan Roese <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

---
v2:
- Add debug() output in error case

Quoted 2 lines

 drivers/core/device-remove.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

I thought that your change to force removal of the stdio dev would make this change unnecessary?

I really would rather fix the root cause if we can.

Hi Simon,

Quoted 36 lines · Simon Glass

Quoted 34 lines · Stefan Roese

On my x86 platform I've noticed, that calling dm_uninit() or the new function dm_remove_devices_flags() does not remove the desired device at all. Debugging showed, that the serial uclass returns -EPERM in serial_pre_remove() and this leads to a complete stop of the device removal pretty early, as the serial device is one of the first ones in the DM. Here the dm tree output:

=> dm tree
Class Probed Name
----------------------------------------

 root        [ + ]    root_driver
 rsa_mod_exp [   ]    |-- mod_exp_sw
 serial      [ + ]    |-- serial
 rtc         [   ]    |-- rtc
 timer       [ + ]    |-- tsc-timer
 syscon      [ + ]    |-- pch_pinctrl

...

In this example, device_remove(root) will stop directly after trying to remove the "serial" device.

To solve this problem, this patch removes the return upon error check in the device_remove() call in device_chld_remove(). This leads to device_chld_remove() continuing with the device_remove() call to the following child devices.

Signed-off-by Stefan Roese <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

---
v2:
- Add debug() output in error case

Quoted 6 lines

Quoted 2 lines

 drivers/core/device-remove.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

I thought that your change to force removal of the stdio dev would make this change unnecessary?

Yes, the force removal made this change unnecessary in this specific case. But...

Quoted 1 line

I really would rather fix the root cause if we can.

... the current implementation to exit the loop over all children upon error and not remove the remaining children is wrong IMO. All devices should at least be tried to get removed, even if one fails to get removed. This is what this patch makes sure of.

Quoted 54 lines · Stefan Roese

Hi Simon,

Quoted 36 lines · Simon Glass

Quoted 34 lines · Stefan Roese

On my x86 platform I've noticed, that calling dm_uninit() or the new function dm_remove_devices_flags() does not remove the desired device at all. Debugging showed, that the serial uclass returns -EPERM in serial_pre_remove() and this leads to a complete stop of the device removal pretty early, as the serial device is one of the first ones in the DM. Here the dm tree output:

=> dm tree
Class Probed Name
----------------------------------------

 root        [ + ]    root_driver
 rsa_mod_exp [   ]    |-- mod_exp_sw
 serial      [ + ]    |-- serial
 rtc         [   ]    |-- rtc
 timer       [ + ]    |-- tsc-timer
 syscon      [ + ]    |-- pch_pinctrl

...

In this example, device_remove(root) will stop directly after trying to remove the "serial" device.

To solve this problem, this patch removes the return upon error check in the device_remove() call in device_chld_remove(). This leads to device_chld_remove() continuing with the device_remove() call to the following child devices.

Signed-off-by Stefan Roese <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

---
v2:
- Add debug() output in error case

Quoted 6 lines

Quoted 2 lines

 drivers/core/device-remove.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

I thought that your change to force removal of the stdio dev would make this change unnecessary?

Yes, the force removal made this change unnecessary in this specific case. But...

Quoted 1 line

I really would rather fix the root cause if we can.

Quoted 4 lines

... the current implementation to exit the loop over all children upon error and not remove the remaining children is wrong IMO. All devices should at least be tried to get removed, even if one fails to get removed. This is what this patch makes sure of.

Yes I see that, but not being able to remove is actually an error. In the normal course of events, a device that will not remove itself is likely buggy.

What do you think about adding a new remove flag to indicate that failures should be skipped?

Hi Simon,

Quoted 70 lines · Simon Glass

Quoted 58 lines · Stefan Roese

Hi Simon,

Quoted 40 lines · Simon Glass

Quoted 37 lines · Stefan Roese

On my x86 platform I've noticed, that calling dm_uninit() or the new function dm_remove_devices_flags() does not remove the desired device at
all. Debugging showed, that the serial uclass returns -EPERM in serial_pre_remove() and this leads to a complete stop of the device removal pretty early, as the serial device is one of the first ones in the DM. Here the dm tree output:

=> dm tree
Class Probed Name
----------------------------------------

 root        [ + ]    root_driver
 rsa_mod_exp [   ]    |-- mod_exp_sw
 serial      [ + ]    |-- serial
 rtc         [   ]    |-- rtc
 timer       [ + ]    |-- tsc-timer
 syscon      [ + ]    |-- pch_pinctrl

...

In this example, device_remove(root) will stop directly after trying to remove the "serial" device.

To solve this problem, this patch removes the return upon error check in
the device_remove() call in device_chld_remove(). This leads to device_chld_remove() continuing with the device_remove() call to the following child devices.

Signed-off-by Stefan Roese <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

---
v2:
- Add debug() output in error case

Quoted 7 lines

Quoted 2 lines

 drivers/core/device-remove.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

I thought that your change to force removal of the stdio dev would make this change unnecessary?

Yes, the force removal made this change unnecessary in this specific case. But...

Quoted 8 lines

Quoted 1 line

I really would rather fix the root cause if we can.

... the current implementation to exit the loop over all children upon error and not remove the remaining children is wrong IMO. All devices should at least be tried to get removed, even if one fails to get removed. This is what this patch makes sure of.

Quoted 3 lines

Yes I see that, but not being able to remove is actually an error. In the normal course of events, a device that will not remove itself is likely buggy.

Isn't it enough then to just print an error message in this case in this loop - change debug() to printf() in this current patch version? Then "users" of this code will be aware of such remove failures and can take appropriate actions (fix bug etc in their setup).

Quoted 3 lines

What do you think about adding a new remove flag to indicate that failures should be skipped?

I'm a bit afraid that this makes the code overly complex. But if you prefer to have it this way, then I can come up with such a version as well. Just let me know.

Thanks,
Stefan

Quoted 11 lines

Quoted 9 lines

Quoted 4 lines

... the current implementation to exit the loop over all children upon error and not remove the remaining children is wrong IMO. All devices should at least be tried to get removed, even if one fails to get removed. This is what this patch makes sure of.

Yes I see that, but not being able to remove is actually an error. In the normal course of events, a device that will not remove itself is likely buggy.

Quoted 5 lines

Isn't it enough then to just print an error message in this case in this loop - change debug() to printf() in this current patch version? Then "users" of this code will be aware of such remove failures and can take appropriate actions (fix bug etc in their setup).

Possibly, but programmatically it becomes impossible to detect a failure. Say the USB fails to stop its DMA, we might want to reboot rather than continue to boot Linux and crash.

Quoted 5 lines

Quoted 3 lines

What do you think about adding a new remove flag to indicate that failures should be skipped?

Quoted 3 lines

I'm a bit afraid that this makes the code overly complex. But if you prefer to have it this way, then I can come up with such a version as well. Just let me know.

I don't think the complexity is too great. It does need a new test. But I think we should hold the line on error checking even with remove(), by default.

Hi Simon,

On 04.05.2017 18:50, Simon Glass wrote:

<snip>

Quoted 20 lines

Quoted 13 lines

Quoted 10 lines

Quoted 4 lines

... the current implementation to exit the loop over all children upon error and not remove the remaining children is wrong IMO. All devices should at least be tried to get removed, even if one fails to get removed. This is what this patch makes sure of.

Yes I see that, but not being able to remove is actually an error. In the normal course of events, a device that will not remove itself is likely buggy.

Quoted 5 lines

Isn't it enough then to just print an error message in this case in this loop - change debug() to printf() in this current patch version? Then "users" of this code will be aware of such remove failures and can take appropriate actions (fix bug etc in their setup).

Quoted 3 lines

Possibly, but programmatically it becomes impossible to detect a failure. Say the USB fails to stop its DMA, we might want to reboot rather than continue to boot Linux and crash.

Okay, I see your point.

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

What do you think about adding a new remove flag to indicate that failures should be skipped?

Quoted 8 lines

Quoted 3 lines

I'm a bit afraid that this makes the code overly complex. But if you prefer to have it this way, then I can come up with such a version as well. Just let me know.

I don't think the complexity is too great. It does need a new test. But I think we should hold the line on error checking even with remove(), by default.

Understood. How about doing it this way: You drop this patch from the series for now (it still works for us with the force remove of the serial driver) and apply the remaining patches. I'll try to get back to this skip-failures flag implementation in a few days / weeks.

Is this okay for you?

Thanks,
Stefan