| Message ID | 20170406132944.476-1-sr@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
Hi Stefan, On 6 April 2017 at 07:29, Stefan Roese <sr@denx.de> wrote: > 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. I think the right solution is to find out why stdio_deregister_dev() fails. It is probably because the device is in use within the stdio variables. Perhaps you need to remove it first? > > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Simon Glass <sjg@chromium.org> > Cc: Bin Meng <bmeng.cn@gmail.com> > --- > drivers/core/device-remove.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c > index cc0043b990..8b46f3343e 100644 > --- a/drivers/core/device-remove.c > +++ b/drivers/core/device-remove.c > @@ -52,15 +52,11 @@ static int device_chld_unbind(struct udevice *dev) > static int device_chld_remove(struct udevice *dev, uint flags) > { > struct udevice *pos, *n; > - int ret; > > assert(dev); > > - list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) { > - ret = device_remove(pos, flags); > - if (ret) > - return ret; > - } > + list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) > + device_remove(pos, flags); I think we should keep the error checking here. > > return 0; > } > -- > 2.12.2 > Regards, Simon
Hi Simon, sorry for the late replay - just back from vacation.... On 09.04.2017 21:28, Simon Glass wrote: > Hi Stefan, > > On 6 April 2017 at 07:29, Stefan Roese <sr@denx.de> wrote: >> 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. > > I think the right solution is to find out why stdio_deregister_dev() > fails. It is probably because the device is in use within the stdio > variables. This is most likely the case, yes. > Perhaps you need to remove it first? Not sure if this should / could be done in this general case of removal of all devices (or all devices matching a remove-flag)? Please think of dm_uninit() being called. This functions should have no internal knowledge of usage of devices. One thing I could do, probably best in a separate patch, is to use the force flag of stdio_deregister_dev() in serial_pre_remove() to force the serial device(s) to be removed in this case. What do you think? >> >> Signed-off-by: Stefan Roese <sr@denx.de> >> Cc: Simon Glass <sjg@chromium.org> >> Cc: Bin Meng <bmeng.cn@gmail.com> >> --- >> drivers/core/device-remove.c | 8 ++------ >> 1 file changed, 2 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c >> index cc0043b990..8b46f3343e 100644 >> --- a/drivers/core/device-remove.c >> +++ b/drivers/core/device-remove.c >> @@ -52,15 +52,11 @@ static int device_chld_unbind(struct udevice *dev) >> static int device_chld_remove(struct udevice *dev, uint flags) >> { >> struct udevice *pos, *n; >> - int ret; >> >> assert(dev); >> >> - list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) { >> - ret = device_remove(pos, flags); >> - if (ret) >> - return ret; >> - } >> + list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) >> + device_remove(pos, flags); > > I think we should keep the error checking here. The main fix with this patch is, that removing of child devices does not stop once an error is encountered. Even if an error occurs with one child-device of a parent, all other child-devices of this parent should still be removed - or at least tried to. So what does this error code buy us here? Should I print a log message here in this error case? Thanks, Stefan
Hi Stefan, On 19 April 2017 at 03:27, Stefan Roese <sr@denx.de> wrote: > Hi Simon, > > sorry for the late replay - just back from vacation.... > > > On 09.04.2017 21:28, Simon Glass wrote: >> >> Hi Stefan, >> >> On 6 April 2017 at 07:29, Stefan Roese <sr@denx.de> wrote: >>> >>> 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. >> >> >> I think the right solution is to find out why stdio_deregister_dev() >> fails. It is probably because the device is in use within the stdio >> variables. > > > This is most likely the case, yes. > >> Perhaps you need to remove it first? > > > Not sure if this should / could be done in this general case of > removal of all devices (or all devices matching a remove-flag)? > Please think of dm_uninit() being called. This functions should > have no internal knowledge of usage of devices. One thing I could > do, probably best in a separate patch, is to use the force flag > of stdio_deregister_dev() in serial_pre_remove() to force the > serial device(s) to be removed in this case. What do you think? I think that sounds reasonable. I don't know this area very well though. > >>> >>> Signed-off-by: Stefan Roese <sr@denx.de> >>> Cc: Simon Glass <sjg@chromium.org> >>> Cc: Bin Meng <bmeng.cn@gmail.com> >>> --- >>> drivers/core/device-remove.c | 8 ++------ >>> 1 file changed, 2 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c >>> index cc0043b990..8b46f3343e 100644 >>> --- a/drivers/core/device-remove.c >>> +++ b/drivers/core/device-remove.c >>> @@ -52,15 +52,11 @@ static int device_chld_unbind(struct udevice *dev) >>> static int device_chld_remove(struct udevice *dev, uint flags) >>> { >>> struct udevice *pos, *n; >>> - int ret; >>> >>> assert(dev); >>> >>> - list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) >>> { >>> - ret = device_remove(pos, flags); >>> - if (ret) >>> - return ret; >>> - } >>> + list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) >>> + device_remove(pos, flags); >> >> >> I think we should keep the error checking here. > > > The main fix with this patch is, that removing of child devices does > not stop once an error is encountered. Even if an error occurs with > one child-device of a parent, all other child-devices of this > parent should still be removed - or at least tried to. > > So what does this error code buy us here? Should I print a log message > here in this error case? Yes just a debug() will do. I see what you are saying, I'm just nervous of dropping error checking since devices really should remove cleanly. Hopefully as above you can fix the error? Regards, Simon
diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c index cc0043b990..8b46f3343e 100644 --- a/drivers/core/device-remove.c +++ b/drivers/core/device-remove.c @@ -52,15 +52,11 @@ static int device_chld_unbind(struct udevice *dev) static int device_chld_remove(struct udevice *dev, uint flags) { struct udevice *pos, *n; - int ret; assert(dev); - list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) { - ret = device_remove(pos, flags); - if (ret) - return ret; - } + list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) + device_remove(pos, flags); return 0; }
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 <sr@denx.de> Cc: Simon Glass <sjg@chromium.org> Cc: Bin Meng <bmeng.cn@gmail.com> --- drivers/core/device-remove.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)