From: Brian Norris <briannorris@chromium.org>
To: Titouan Ameline de Cadeville <titouan.ameline@gmail.com>
Cc: jwerner@chromium.org, tzungbi@kernel.org,
chrome-platform@lists.linuxfoundation.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] coreboot_table: skip failing entries instead of aborting populate
Date: Thu, 30 Apr 2026 14:07:38 -0700 [thread overview]
Message-ID: <afPEmmOIZRMd7u6r@google.com> (raw)
In-Reply-To: <20260430202305.121680-1-titouan.ameline@gmail.com>
On Thu, Apr 30, 2026 at 10:23:05PM +0200, Titouan Ameline de Cadeville wrote:
> coreboot_table_populate() registers devices one by one. If
> device_register() fails for one entry, the current code returns
> immediately, leaving previously registered devices orphaned on the
> coreboot bus with no cleanup path.
>
> Since coreboot table entries are independent of each other, a failure
> on one entry should not prevent the others from being registered.
> This mirrors the strategy used by of_platform_populate(), which skips
> individual failures rather than aborting.
>
> Log a warning and continue the loop on device_register() failure.
>
> Signed-off-by: Titouan Ameline de Cadeville <titouan.ameline@gmail.com>
> ---
> v2: continue the loop on failure instead of unregistering all devices
> (suggested by Julius Werner, with reference to of_platform_populate()
> from Brian Norris)
>
> drivers/firmware/google/coreboot_table.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
> index c769631ea15d..82be21434be4 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -150,8 +150,9 @@ static int coreboot_table_populate(struct device *dev, void *ptr)
>
> ret = device_register(&device->dev);
> if (ret) {
> + dev_warn(dev, "failed to register coreboot device: %d\n", ret);
> put_device(&device->dev);
> - return ret;
> + continue;
> }
>
> ptr_entry += entry->size;
Isn't it important to still increment ptr_entry on failure/continue?
Otherwise, you may re-populate a new device with the same contents, and
you may not actually register all devices. (You'll be off-by-1, at
least.)
To fix this, you could either drop the 'continue' (let it fall through)
or else move this line up.
Brian
> --
> 2.44.2
>
prev parent reply other threads:[~2026-04-30 21:07 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-30 20:23 [PATCH v2] coreboot_table: skip failing entries instead of aborting populate Titouan Ameline de Cadeville
2026-04-30 20:45 ` Julius Werner
2026-04-30 21:07 ` Brian Norris [this message]
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=afPEmmOIZRMd7u6r@google.com \
--to=briannorris@chromium.org \
--cc=chrome-platform@lists.linuxfoundation.org \
--cc=jwerner@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=titouan.ameline@gmail.com \
--cc=tzungbi@kernel.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.