From: Simon Glass <sjg@chromium.org>
To: "Michal Suchánek" <msuchanek@suse.de>
Cc: "U-Boot Mailing List" <u-boot@lists.denx.de>,
"Marek Vasut" <marex@denx.de>,
"Viktor Křivák" <viktor.krivak@gmail.com>,
"Pavel Herrmann" <morpheus.ibis@gmail.com>,
"Tomas Hlavacek" <tmshlvck@gmail.com>
Subject: Re: [PATCH v3] dm: core: Do not stop uclass iteration on error
Date: Tue, 30 Aug 2022 21:15:12 -0600 [thread overview]
Message-ID: <CAPnjgZ3EGKfQ7op-zC+-skaY3KV51vQOypX-p+ernSkgsnzfLw@mail.gmail.com> (raw)
In-Reply-To: <20220830164810.GP28810@kitsune.suse.cz>
Hi Michal,
On Tue, 30 Aug 2022 at 10:48, Michal Suchánek <msuchanek@suse.de> wrote:
>
> On Tue, Aug 30, 2022 at 09:56:52AM -0600, Simon Glass wrote:
> > Hi Michal,
> >
> > On Tue, 30 Aug 2022 at 04:23, Michal Suchánek <msuchanek@suse.de> wrote:
> > >
> > > On Sat, Aug 27, 2022 at 07:52:27PM -0600, Simon Glass wrote:
> > > > Hi Michal,
> > > >
> > > > On Fri, 19 Aug 2022 at 14:23, Michal Suchanek <msuchanek@suse.de> wrote:
> > > > >
> > > > > When probing a device fails NULL pointer is returned, and other devices
> > > > > cannot be iterated. Skip to next device on error instead.
> > > > >
> > > > > Fixes: 6494d708bf ("dm: Add base driver model support")
> > > >
> > > > I think you should drop this as you are doing a change of behaviour,
> > > > not fixing a bug!
> > >
> > > You can hardly fix a bug without a change in behavior.
> > >
> > > These functions are used for iterating devices, and are not iterating
> > > devices. That's clearly a bug.
> >
> > If it were clear I would have changed this long ago. The new way you
> > have this function ignores errors, so they cannot be reported.
> >
> > We should almost always report errors, which is why I think your
> > methods should be named differently.
> >
> > >
> > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > ---
> > > > > v2: - Fix up tests
> > > > > v3: - Fix up API doc
> > > > > - Correctly forward error from uclass_get
> > > > > - Do not return an error when last device fails to probe
> > > > > - Drop redundant initialization
> > > > > - Wrap at 80 columns
> > > > > ---
> > > > > drivers/core/uclass.c | 32 ++++++++++++++++++++++++--------
> > > > > include/dm/uclass.h | 13 ++++++++-----
> > > > > test/dm/test-fdt.c | 20 ++++++++++++++++----
> > > > > 3 files changed, 48 insertions(+), 17 deletions(-)
> > > >
> > > > Unfortunately this still fails one test. Try 'make qcheck' to see it -
> > > > it is ethernet.
> > >
> > > I will look at that.
> > >
> > > > I actually think you should create new functions for this feature,
> > > > e.g.uclass_first_device_ok(), since it makes it impossible to see what
> > > > when wrong with a device in the middle.
> > > >
> > > > I have long had all this in my mind. One idea for a future change is
> > > > to return the error, but set dev, so that the caller knows there is a
> > > > device, which failed. When we are at the end, dev is set to NULL.
> > >
> > > We already have uclass_first_device_check() and
> > > uclass_next_device_check() to iterate all devices, including broken
> > > ones, and getting the errors as well.
> > >
> > > That's for the case you want all the details, and these are for the case
> > > you just want to get devices and don't care about the details.
> > >
> > > That's AFAICT as much as this iteration interface can provide, and we
> > > have both cases covered.
> >
> > I see three cases:
> > - want to see the next device, returning the error if it cannot be
> > probed - uclass_first_device()
>
> And the point of this is what exactly?
Please can you adjust your tone, It seems too aggressive for this
mailing list. Thank you.
>
> The device order in the uclass is not well defined - at any time a new
> device which will become the first can be added, fail probe, and block
> what was assumed a loop iterating the uclass from returning any devices
> at all. That's exactly what happened with the new sysreset.
The order only changes if the device is unbound and rebound. Otherwise
the order set by the device tree is used.
>
> What is exactly the point of returning the error and not the pointer to
> the next device?
Partly, we have existing code which uses the interface, checking 'dev'
to see if the device is valid. I would be happy to change that, so
that the device is always returned. In fact I think it would be
better. But it does need a bit of work with coccinelle, etc.
>
> The only point of these simplified iterators is that the caller can
> check only one value (device pointer) and then not check the error
> because they don't care. If they do cate uclass_first_device_check()
> provides all the details available.
Yes I think we can have just two sets of iterators, but in that case
it should be:
- want to see the next device, returning the error if it cannot be
probed, with dev updated to the next device in any case - new version
of uclass_first_device() - basically rename
uclass_first_device_check() to that
- want to see next device which probes OK - your new function, perhaps
uclass_first_device_ok() ?
>
> > - want to get the next device that can successfully probe - your new functions
> > - want to see each device, along with any errors - uclass_first_device_check()
Regards,
SImon
next prev parent reply other threads:[~2022-08-31 3:15 UTC|newest]
Thread overview: 133+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-04 17:59 [PATCH] dm: core: Do not stop uclass iteration on error Michal Suchanek
2022-08-04 19:22 ` Simon Glass
2022-08-04 19:36 ` Michal Suchánek
2022-08-04 20:30 ` Simon Glass
2022-08-17 8:27 ` [PATCH v2] " Michal Suchanek
2022-08-18 17:49 ` Simon Glass
2022-08-18 19:46 ` Michal Suchánek
2022-08-19 20:23 ` [PATCH v3] " Michal Suchanek
2022-08-28 1:52 ` Simon Glass
2022-08-30 10:23 ` Michal Suchánek
2022-08-30 15:56 ` Simon Glass
2022-08-30 16:48 ` Michal Suchánek
2022-08-31 3:15 ` Simon Glass [this message]
2022-08-31 7:39 ` Michal Suchánek
2022-08-31 17:44 ` Simon Glass
2022-09-17 15:02 ` Simon Glass
2022-09-17 17:04 ` Michal Suchánek
2022-09-24 20:09 ` Michal Suchánek
2022-09-25 14:15 ` Simon Glass
2022-09-25 16:38 ` Michal Suchánek
2022-09-25 8:27 ` [PATCH v4 00/21] " Michal Suchanek
2022-09-25 8:27 ` [PATCH v4 01/21] dm: pci: Fix doc typo first -> next Michal Suchanek
2022-09-29 9:59 ` Simon Glass
2022-10-22 1:06 ` Simon Glass
2022-09-25 8:27 ` [PATCH v4 02/21] dm: core: Add note about device_probe idempotence Michal Suchanek
2022-09-29 9:59 ` Simon Glass
2022-09-25 8:27 ` [PATCH v4 03/21] dm: core: Document return value of device bind functions Michal Suchanek
2022-09-29 9:59 ` Simon Glass
2022-09-25 8:27 ` [PATCH v4 04/21] dm: blk: Add probe in blk_first_device/blk_next_device Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-10-02 19:34 ` Michal Suchánek
2022-10-03 1:10 ` Simon Glass
2022-10-10 19:49 ` Michal Suchánek
2022-10-10 21:33 ` Michal Suchánek
2022-10-10 22:33 ` Simon Glass
2022-11-10 2:15 ` Simon Glass
2022-09-25 8:27 ` [PATCH v4 05/21] dm: core: Fix uclass_probe_all to really probe all devices Michal Suchanek
2022-09-25 8:27 ` [PATCH v4 06/21] dm: treewide: Do not opencode uclass_probe_all() Michal Suchanek
2022-09-25 8:28 ` [PATCH v4 07/21] dm: pci: Fix device PCI iteration Michal Suchanek
2022-09-25 8:28 ` [PATCH v4 08/21] bootstd: Fix listing boot devices Michal Suchanek
2022-09-25 8:28 ` [PATCH v4 09/21] usb: ether: Fix error handling in usb_ether_init Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-09-25 8:28 ` [PATCH v4 10/21] stdio: Fix class iteration in stdio_add_devices() Michal Suchanek
2022-09-25 8:28 ` [PATCH v4 11/21] video: ipuv3: Fix error handling when getting the display Michal Suchanek
2022-09-25 8:28 ` [PATCH v4 12/21] w1: Fix bus counting in w1_get_bus Michal Suchanek
2022-09-25 8:28 ` [PATCH v4 13/21] w1: Clean up device iteration in w1_bus_find_dev Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-09-25 8:28 ` [PATCH v4 14/21] dma: Eliminate unused variable in dma_get_cfg() Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-09-25 8:28 ` [PATCH v4 15/21] cmd: List all uclass devices regardless of probe error Michal Suchanek
2022-09-25 8:28 ` [PATCH v4 16/21] dm: treewide: Use uclass_first_device_err when accessing one device Michal Suchanek
2022-09-25 8:28 ` [PATCH v4 17/21] dm: treewide: Use uclass_next_device_err when accessing second device Michal Suchanek
2022-09-25 8:28 ` [PATCH v4 18/21] dm: blk: Do not use uclass_next_device_err Michal Suchanek
2022-09-25 8:28 ` [PATCH v4 19/21] dm: treewide: Do not use the return value of simple uclass iterator Michal Suchanek
2022-09-25 8:28 ` [PATCH v4 20/21] dm: core: Switch uclass_*_device_err to use uclass_*_device_check Michal Suchanek
2022-09-25 11:08 ` [PATCH] fixup: " Michal Suchanek
2022-09-29 10:00 ` [PATCH v4 20/21] " Simon Glass
2022-09-25 8:28 ` [PATCH v4 21/21] dm: core: Do not stop uclass iteration on error Michal Suchanek
2022-09-27 21:37 ` [PATCH v5 00/15] " Michal Suchanek
2022-09-27 21:37 ` [PATCH v5 01/15] dm: core: Fix uclass_probe_all to really probe all devices Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-09-27 21:37 ` [PATCH v5 02/15] dm: treewide: Do not opencode uclass_probe_all() Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-09-27 21:37 ` [PATCH v5 03/15] dm: pci: Fix device PCI iteration Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-09-27 21:37 ` [PATCH v5 04/15] bootstd: Fix listing boot devices Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-10-02 19:19 ` Michal Suchánek
2022-09-27 21:37 ` [PATCH v5 05/15] usb: ether: Fix error handling in usb_ether_init Michal Suchanek
2022-09-27 21:37 ` [PATCH v5 06/15] stdio: Fix class iteration in stdio_add_devices() Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-09-27 21:37 ` [PATCH v5 07/15] video: ipuv3: Fix error handling when getting the display Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-09-27 21:38 ` [PATCH v5 08/15] w1: Fix bus counting in w1_get_bus Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-09-27 21:38 ` [PATCH v5 09/15] cmd: List all uclass devices regardless of probe error Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-10-02 19:10 ` Michal Suchánek
2022-10-03 1:10 ` Simon Glass
2022-09-27 21:38 ` [PATCH v5 10/15] dm: treewide: Use uclass_first_device_err when accessing one device Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-09-27 21:38 ` [PATCH v5 11/15] dm: treewide: Use uclass_next_device_err when accessing second device Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-09-27 21:38 ` [PATCH v5 12/15] dm: blk: Do not use uclass_next_device_err Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-09-27 21:38 ` [PATCH v5 13/15] dm: core: Switch uclass_*_device_err to use uclass_*_device_check Michal Suchanek
2022-09-27 21:38 ` [PATCH v5 14/15] dm: treewide: Do not use the return value of simple uclass iterator Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-09-27 21:38 ` [PATCH v5 15/15] dm: core: Do not stop uclass iteration on error Michal Suchanek
2022-09-29 10:00 ` Simon Glass
2022-09-29 10:00 ` [PATCH v5 00/15] " Simon Glass
2022-10-12 19:57 ` [PATCH v6 00/20] " Michal Suchanek
2022-10-12 19:57 ` [PATCH v6 01/20] dm: core: Fix uclass_probe_all to really probe all devices Michal Suchanek
2022-10-12 22:14 ` Simon Glass
2022-10-17 21:29 ` Simon Glass
2022-10-12 19:57 ` [PATCH v6 02/20] dm: treewide: Do not opencode uclass_probe_all() Michal Suchanek
2022-10-12 19:57 ` [PATCH v6 03/20] dm: pci: Fix device PCI iteration Michal Suchanek
2022-10-12 19:57 ` [PATCH v6 04/20] bootstd: Fix listing boot devices Michal Suchanek
2022-10-12 19:57 ` [PATCH v6 05/20] usb: ether: Fix error handling in usb_ether_init Michal Suchanek
2022-10-12 19:57 ` [PATCH v6 06/20] stdio: Fix class iteration in stdio_add_devices() Michal Suchanek
2022-10-12 19:57 ` [PATCH v6 07/20] video: ipuv3: Fix error handling when getting the display Michal Suchanek
2022-10-12 19:57 ` [PATCH v6 08/20] w1: Fix bus counting in w1_get_bus Michal Suchanek
2022-10-12 19:57 ` [PATCH v6 09/20] cmd: List all uclass devices regardless of probe error Michal Suchanek
2022-10-12 19:57 ` [PATCH v6 10/20] dm: treewide: Use uclass_first_device_err when accessing one device Michal Suchanek
2022-10-12 19:58 ` [PATCH v6 11/20] dm: treewide: Use uclass_next_device_err when accessing second device Michal Suchanek
2022-10-12 19:58 ` [PATCH v6 12/20] dm: blk: Do not use uclass_next_device_err Michal Suchanek
2022-10-12 19:58 ` [PATCH v6 13/20] net: eth-uclass: Do not set device on error Michal Suchanek
2022-10-12 19:58 ` [PATCH v6 14/20] dm: pci: Update error handling in pci_sriov_init Michal Suchanek
2022-10-12 22:14 ` Simon Glass
2022-10-12 19:58 ` [PATCH v6 15/20] mpc83xx: gazerbeam: Update sysinfo_get error handling Michal Suchanek
2022-10-12 22:14 ` Simon Glass
2022-10-17 21:29 ` Simon Glass
2022-10-12 19:58 ` [PATCH v6 16/20] dm: core: Switch uclass_foreach_dev_probe to use simple iterator Michal Suchanek
2022-10-12 22:14 ` Simon Glass
2022-10-17 21:29 ` Simon Glass
2022-10-12 19:58 ` [PATCH v6 17/20] dm: core: Switch uclass_*_device_err to use uclass_*_device_check Michal Suchanek
2022-10-12 19:58 ` [PATCH v6 18/20] dm: core: Non-activated device may be returned from uclass iterators that provide error handling Michal Suchanek
2022-10-12 22:14 ` Simon Glass
2022-10-12 19:58 ` [PATCH v6 19/20] dm: treewide: Do not use the return value of simple uclass iterator Michal Suchanek
2022-10-12 19:58 ` [PATCH v6 20/20] dm: core: Do not stop uclass iteration on error Michal Suchanek
2022-10-17 21:29 ` [PATCH v6 17/20] dm: core: Switch uclass_*_device_err to use uclass_*_device_check Simon Glass
2022-10-17 21:29 ` [PATCH v6 13/20] net: eth-uclass: Do not set device on error Simon Glass
2022-10-17 21:29 ` [PATCH v6 12/20] dm: blk: Do not use uclass_next_device_err Simon Glass
2022-10-17 21:29 ` [PATCH v6 11/20] dm: treewide: Use uclass_next_device_err when accessing second device Simon Glass
2022-10-17 21:29 ` [PATCH v6 10/20] dm: treewide: Use uclass_first_device_err when accessing one device Simon Glass
2022-10-17 21:29 ` [PATCH v6 09/20] cmd: List all uclass devices regardless of probe error Simon Glass
2022-10-17 21:29 ` [PATCH v6 07/20] video: ipuv3: Fix error handling when getting the display Simon Glass
2022-10-17 21:29 ` [PATCH v6 08/20] w1: Fix bus counting in w1_get_bus Simon Glass
2022-10-17 21:29 ` [PATCH v6 06/20] stdio: Fix class iteration in stdio_add_devices() Simon Glass
2022-10-17 21:29 ` [PATCH v6 05/20] usb: ether: Fix error handling in usb_ether_init Simon Glass
2022-10-17 21:29 ` [PATCH v6 04/20] bootstd: Fix listing boot devices Simon Glass
2022-10-17 21:29 ` [PATCH v6 03/20] dm: pci: Fix device PCI iteration Simon Glass
2022-10-17 21:29 ` [PATCH v6 02/20] dm: treewide: Do not opencode uclass_probe_all() Simon Glass
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPnjgZ3EGKfQ7op-zC+-skaY3KV51vQOypX-p+ernSkgsnzfLw@mail.gmail.com \
--to=sjg@chromium.org \
--cc=marex@denx.de \
--cc=morpheus.ibis@gmail.com \
--cc=msuchanek@suse.de \
--cc=tmshlvck@gmail.com \
--cc=u-boot@lists.denx.de \
--cc=viktor.krivak@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).