Hi Paul, On Sat, 15 Oct 2022 at 03:19, Paul Barker <paul.bar...@sancloud.com> wrote: > > We should only perform additional iteration steps when needed to > initialize the parent of a device. Other binding errors (such as a > missing driver) should not lead to additional iteration steps. > > Unnecessary iteration steps can cause issues when memory is tightly > constrained (such as in the TPL/SPL) since device_bind_by_name() > unconditionally allocates memory for a struct udevice. On the SanCloud > BBE this led to boot failure caused by memory exhaustion in the SPL > when booting from SPI flash. > > Signed-off-by: Paul Barker <paul.bar...@sancloud.com> > --- > drivers/core/lists.c | 17 ++++++----------- > 1 file changed, 6 insertions(+), 11 deletions(-)
Are you able to construct a test for this? See test/dm/core.c or test-fdt.c Also, I think bind_drivers_pass() needs a function comment that describes in detail what is going on. Would it be possible to achieve the same effect while keeping that function the same as now? See below. > > diff --git a/drivers/core/lists.c b/drivers/core/lists.c > index c49695b24f00..82d479564121 100644 > --- a/drivers/core/lists.c > +++ b/drivers/core/lists.c > @@ -51,13 +51,13 @@ struct uclass_driver *lists_uclass_lookup(enum uclass_id > id) > return NULL; > } > > -static int bind_drivers_pass(struct udevice *parent, bool pre_reloc_only) > +static bool bind_drivers_pass(struct udevice *parent, bool pre_reloc_only, > + int *result) > { > struct driver_info *info = > ll_entry_start(struct driver_info, driver_info); > const int n_ents = ll_entry_count(struct driver_info, driver_info); > bool missing_parent = false; > - int result = 0; > int idx; > > /* > @@ -98,12 +98,12 @@ static int bind_drivers_pass(struct udevice *parent, bool > pre_reloc_only) > drt->dev = dev; > } else if (ret != -EPERM) { > dm_warn("No match for driver '%s'\n", entry->name); > - if (!result || ret != -ENOENT) > - result = ret; > + if (!*result || ret != -ENOENT) > + *result = ret; > } > } > > - return result ? result : missing_parent ? -EAGAIN : 0; > + return missing_parent; > } > > int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only) > @@ -117,13 +117,8 @@ int lists_bind_drivers(struct udevice *parent, bool > pre_reloc_only) > * always succeed on the first pass. > */ > for (pass = 0; pass < 10; pass++) { > - int ret; > - > - ret = bind_drivers_pass(parent, pre_reloc_only); > - if (!ret) > + if (!bind_drivers_pass(parent, pre_reloc_only, &result)) > break; > - if (ret != -EAGAIN && !result) > - result = ret; If there were something like this, could we drop the other changes? if (ret) { if (ret == -EAGAIN) continue; else if (!result) result = ret; } > } > > return result; > -- > 2.25.1 >