| Message ID | 20191021230310.9990-1-michael@walle.cc |
|---|---|
| State | Accepted |
| Commit | 19820db0bd97fd4e202019594abe9a9604aa5534 |
| Delegated to: | Joe Hershberger |
| Headers | show |
| Series | [U-Boot] net: eth-uclass: ignore unavailable devices | expand |
On Mon, Oct 21, 2019 at 6:03 PM Michael Walle <michael@walle.cc> wrote: > > device_probe() may fail in which case the seq_id will be -1. Don't > display these devices during startup. While this is only a cosmetic > change, the return value of eth_initialize() will also change to the > actual number of available devices. The return value is only used in > spl_net to decide whether there are any devices to boot from. So > returning only available devices is also more correct in that case. > > Signed-off-by: Michael Walle <michael@walle.cc> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
Hi Michael, On 10/22/2019 1:03 AM, Michael Walle wrote: > device_probe() may fail in which case the seq_id will be -1. Don't > display these devices during startup. While this is only a cosmetic > change, the return value of eth_initialize() will also change to the > actual number of available devices. The return value is only used in > spl_net to decide whether there are any devices to boot from. So > returning only available devices is also more correct in that case. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > net/eth-uclass.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/net/eth-uclass.c b/net/eth-uclass.c > index 3bd98b01ad..acd59216e3 100644 > --- a/net/eth-uclass.c > +++ b/net/eth-uclass.c > @@ -420,20 +420,25 @@ int eth_initialize(void) > > bootstage_mark(BOOTSTAGE_ID_NET_ETH_INIT); > do { > - if (num_devices) > - printf(", "); > + if (dev->seq != -1) { > + if (num_devices) > + printf(", "); > > - printf("eth%d: %s", dev->seq, dev->name); > + printf("eth%d: %s", dev->seq, dev->name); > > - if (ethprime && dev == prime_dev) > - printf(" [PRIME]"); > + if (ethprime && dev == prime_dev) > + printf(" [PRIME]"); > + } > > eth_write_hwaddr(dev); > > + if (dev->seq != -1) > + num_devices++; > uclass_next_device_check(&dev); > - num_devices++; > } while (dev); > > + if (!num_devices) > + printf("No ethernet found.\n"); > putc('\n'); > } > > What would you say about something like this instead? It's a bit more compact and should be functionally equivalent: net/eth-uclass.c | 54 ++++++++++++++++++++++-------------------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 1bc8947749..4d4eaeb371 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -390,10 +390,16 @@ int eth_rx(void) int eth_initialize(void) { + struct udevice *dev, *prime_dev = NULL; + char *ethprime = env_get("ethprime"); int num_devices = 0; - struct udevice *dev; + struct uclass *uc; eth_common_init(); + eth_set_dev(NULL); + + if (ethprime) + prime_dev = eth_get_dev_by_name(ethprime); /* * Devices need to write the hwaddr even if not started so that Linux @@ -401,40 +407,30 @@ int eth_initialize(void) * This is accomplished by attempting to probe each device and calling * their write_hwaddr() operation. */ - uclass_first_device_check(UCLASS_ETH, &dev); - if (!dev) { - printf("No ethernet found.\n"); - bootstage_error(BOOTSTAGE_ID_NET_ETH_START); - } else { - char *ethprime = env_get("ethprime"); - struct udevice *prime_dev = NULL; + uclass_get(UCLASS_ETH, &uc); + uclass_foreach_dev(dev, uc) { + if (device_probe(dev)) + continue; + + eth_write_hwaddr(dev); + + if (num_devices) + printf(", "); + printf("eth%d: %s", dev->seq, dev->name); - if (ethprime) - prime_dev = eth_get_dev_by_name(ethprime); - if (prime_dev) { + if (dev == prime_dev) { eth_set_dev(prime_dev); eth_current_changed(); - } else { - eth_set_dev(NULL); + printf(" [PRIME]"); } + num_devices++; + } + if (!num_devices) { + bootstage_error(BOOTSTAGE_ID_NET_ETH_START); + printf("No ethernet found.\n"); + } else { bootstage_mark(BOOTSTAGE_ID_NET_ETH_INIT); - do { - if (device_active(dev)) { - if (num_devices) - printf(", "); - - printf("eth%d: %s", dev->seq, dev->name); - - if (ethprime && dev == prime_dev) - printf(" [PRIME]"); - eth_write_hwaddr(dev); - } - - uclass_next_device_check(&dev); - num_devices++; - } while (dev); - putc('\n'); } Alex
Am 2019-12-01 01:55, schrieb Alexandru Marginean: > Hi Michael, > > On 10/22/2019 1:03 AM, Michael Walle wrote: >> device_probe() may fail in which case the seq_id will be -1. Don't >> display these devices during startup. While this is only a cosmetic >> change, the return value of eth_initialize() will also change to the >> actual number of available devices. The return value is only used in >> spl_net to decide whether there are any devices to boot from. So >> returning only available devices is also more correct in that case. >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> net/eth-uclass.c | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/net/eth-uclass.c b/net/eth-uclass.c >> index 3bd98b01ad..acd59216e3 100644 >> --- a/net/eth-uclass.c >> +++ b/net/eth-uclass.c >> @@ -420,20 +420,25 @@ int eth_initialize(void) >> bootstage_mark(BOOTSTAGE_ID_NET_ETH_INIT); >> do { >> - if (num_devices) >> - printf(", "); >> + if (dev->seq != -1) { >> + if (num_devices) >> + printf(", "); >> - printf("eth%d: %s", dev->seq, dev->name); >> + printf("eth%d: %s", dev->seq, dev->name); >> - if (ethprime && dev == prime_dev) >> - printf(" [PRIME]"); >> + if (ethprime && dev == prime_dev) >> + printf(" [PRIME]"); >> + } >> eth_write_hwaddr(dev); >> + if (dev->seq != -1) >> + num_devices++; >> uclass_next_device_check(&dev); >> - num_devices++; >> } while (dev); >> + if (!num_devices) >> + printf("No ethernet found.\n"); >> putc('\n'); >> } >> > > What would you say about something like this instead? It's a bit more > compact and should be functionally equivalent: sure, but I've noticed that this is only part of the fix (or isn't even necessary). At least, it doesn't work if the first device is unavailable, because eth_get_dev() will then fail. You should be able to reproduce the issue if you disable the first enetc of the LS1028A. Then network should only work if ethact is set. I've traced that back to the pci enumeration which doesn't respect the 'status = "disabled"' property. But instead someone added that check to the enetc driver, but that doesn't really work, because the device is already included in the uclass list. See separate patch, I'll send in a minute. -michael > > net/eth-uclass.c | 54 ++++++++++++++++++++++-------------------------- > 1 file changed, 25 insertions(+), 29 deletions(-) > > diff --git a/net/eth-uclass.c b/net/eth-uclass.c > index 1bc8947749..4d4eaeb371 100644 > --- a/net/eth-uclass.c > +++ b/net/eth-uclass.c > @@ -390,10 +390,16 @@ int eth_rx(void) > > int eth_initialize(void) > { > + struct udevice *dev, *prime_dev = NULL; > + char *ethprime = env_get("ethprime"); > int num_devices = 0; > - struct udevice *dev; > + struct uclass *uc; > > eth_common_init(); > + eth_set_dev(NULL); > + > + if (ethprime) > + prime_dev = eth_get_dev_by_name(ethprime); > > /* > * Devices need to write the hwaddr even if not started so that Linux > @@ -401,40 +407,30 @@ int eth_initialize(void) > * This is accomplished by attempting to probe each device and > calling > * their write_hwaddr() operation. > */ > - uclass_first_device_check(UCLASS_ETH, &dev); > - if (!dev) { > - printf("No ethernet found.\n"); > - bootstage_error(BOOTSTAGE_ID_NET_ETH_START); > - } else { > - char *ethprime = env_get("ethprime"); > - struct udevice *prime_dev = NULL; > + uclass_get(UCLASS_ETH, &uc); > + uclass_foreach_dev(dev, uc) { > + if (device_probe(dev)) > + continue; > + > + eth_write_hwaddr(dev); > + > + if (num_devices) > + printf(", "); > + printf("eth%d: %s", dev->seq, dev->name); > > - if (ethprime) > - prime_dev = eth_get_dev_by_name(ethprime); > - if (prime_dev) { > + if (dev == prime_dev) { > eth_set_dev(prime_dev); > eth_current_changed(); > - } else { > - eth_set_dev(NULL); > + printf(" [PRIME]"); > } > + num_devices++; > + } > > + if (!num_devices) { > + bootstage_error(BOOTSTAGE_ID_NET_ETH_START); > + printf("No ethernet found.\n"); > + } else { > bootstage_mark(BOOTSTAGE_ID_NET_ETH_INIT); > - do { > - if (device_active(dev)) { > - if (num_devices) > - printf(", "); > - > - printf("eth%d: %s", dev->seq, dev->name); > - > - if (ethprime && dev == prime_dev) > - printf(" [PRIME]"); > - eth_write_hwaddr(dev); > - } > - > - uclass_next_device_check(&dev); > - num_devices++; > - } while (dev); > - > putc('\n'); > } > > Alex
On 12/1/2019 5:43 PM, Michael Walle wrote: > Am 2019-12-01 01:55, schrieb Alexandru Marginean: >> Hi Michael, >> >> On 10/22/2019 1:03 AM, Michael Walle wrote: >>> device_probe() may fail in which case the seq_id will be -1. Don't >>> display these devices during startup. While this is only a cosmetic >>> change, the return value of eth_initialize() will also change to the >>> actual number of available devices. The return value is only used in >>> spl_net to decide whether there are any devices to boot from. So >>> returning only available devices is also more correct in that case. >>> >>> Signed-off-by: Michael Walle <michael@walle.cc> >>> --- >>> net/eth-uclass.c | 17 +++++++++++------ >>> 1 file changed, 11 insertions(+), 6 deletions(-) >>> >>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c >>> index 3bd98b01ad..acd59216e3 100644 >>> --- a/net/eth-uclass.c >>> +++ b/net/eth-uclass.c >>> @@ -420,20 +420,25 @@ int eth_initialize(void) >>> bootstage_mark(BOOTSTAGE_ID_NET_ETH_INIT); >>> do { >>> - if (num_devices) >>> - printf(", "); >>> + if (dev->seq != -1) { >>> + if (num_devices) >>> + printf(", "); >>> - printf("eth%d: %s", dev->seq, dev->name); >>> + printf("eth%d: %s", dev->seq, dev->name); >>> - if (ethprime && dev == prime_dev) >>> - printf(" [PRIME]"); >>> + if (ethprime && dev == prime_dev) >>> + printf(" [PRIME]"); >>> + } >>> eth_write_hwaddr(dev); >>> + if (dev->seq != -1) >>> + num_devices++; >>> uclass_next_device_check(&dev); >>> - num_devices++; >>> } while (dev); >>> + if (!num_devices) >>> + printf("No ethernet found.\n"); >>> putc('\n'); >>> } >>> >> >> What would you say about something like this instead? It's a bit more >> compact and should be functionally equivalent: > > sure, but I've noticed that this is only part of the fix (or isn't even > necessary). At least, it doesn't work if the first device is unavailable, > because eth_get_dev() will then fail. You should be able to reproduce > the issue if you disable the first enetc of the LS1028A. Then network > should only work if ethact is set. > > I've traced that back to the pci enumeration which doesn't respect the > 'status = "disabled"' property. But instead someone added that check to > the enetc driver, but that doesn't really work, because the device is > already included in the uclass list. See separate patch, I'll send in a > minute. > > -michael For disabled nodes the pci_find_and_bind_driver patch is sufficient. We should probably deal with drivers failing to probe for other reasons too, there could be other legitimate reasons for that to happen. how about the eth_initialize patch plus: --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -69,8 +69,8 @@ struct udevice *eth_get_dev(void) uc_priv = eth_get_uclass_priv(); if (!uc_priv->current) - eth_errno = uclass_first_device(UCLASS_ETH, - &uc_priv->current); + uclass_get_device_by_seq(UCLASS_ETH, 0, &uc_priv->current); + return uc_priv->current; } Since seq == -1 on devices that fail to probe this code will ignore them. Or add uclass helpers that only deal with devices that probe successfully: uclass_get_first_device_active, uclass_get_next_device_active and uclass_foreach_dev_active. Alex > >> >> net/eth-uclass.c | 54 ++++++++++++++++++++++-------------------------- >> 1 file changed, 25 insertions(+), 29 deletions(-) >> >> diff --git a/net/eth-uclass.c b/net/eth-uclass.c >> index 1bc8947749..4d4eaeb371 100644 >> --- a/net/eth-uclass.c >> +++ b/net/eth-uclass.c >> @@ -390,10 +390,16 @@ int eth_rx(void) >> >> int eth_initialize(void) >> { >> + struct udevice *dev, *prime_dev = NULL; >> + char *ethprime = env_get("ethprime"); >> int num_devices = 0; >> - struct udevice *dev; >> + struct uclass *uc; >> >> eth_common_init(); >> + eth_set_dev(NULL); >> + >> + if (ethprime) >> + prime_dev = eth_get_dev_by_name(ethprime); >> >> /* >> * Devices need to write the hwaddr even if not started so that >> Linux >> @@ -401,40 +407,30 @@ int eth_initialize(void) >> * This is accomplished by attempting to probe each device and >> calling >> * their write_hwaddr() operation. >> */ >> - uclass_first_device_check(UCLASS_ETH, &dev); >> - if (!dev) { >> - printf("No ethernet found.\n"); >> - bootstage_error(BOOTSTAGE_ID_NET_ETH_START); >> - } else { >> - char *ethprime = env_get("ethprime"); >> - struct udevice *prime_dev = NULL; >> + uclass_get(UCLASS_ETH, &uc); >> + uclass_foreach_dev(dev, uc) { >> + if (device_probe(dev)) >> + continue; >> + >> + eth_write_hwaddr(dev); >> + >> + if (num_devices) >> + printf(", "); >> + printf("eth%d: %s", dev->seq, dev->name); >> >> - if (ethprime) >> - prime_dev = eth_get_dev_by_name(ethprime); >> - if (prime_dev) { >> + if (dev == prime_dev) { >> eth_set_dev(prime_dev); >> eth_current_changed(); >> - } else { >> - eth_set_dev(NULL); >> + printf(" [PRIME]"); >> } >> + num_devices++; >> + } >> >> + if (!num_devices) { >> + bootstage_error(BOOTSTAGE_ID_NET_ETH_START); >> + printf("No ethernet found.\n"); >> + } else { >> bootstage_mark(BOOTSTAGE_ID_NET_ETH_INIT); >> - do { >> - if (device_active(dev)) { >> - if (num_devices) >> - printf(", "); >> - >> - printf("eth%d: %s", dev->seq, dev->name); >> - >> - if (ethprime && dev == prime_dev) >> - printf(" [PRIME]"); >> - eth_write_hwaddr(dev); >> - } >> - >> - uclass_next_device_check(&dev); >> - num_devices++; >> - } while (dev); >> - >> putc('\n'); >> } >> >> Alex
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 3bd98b01ad..acd59216e3 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -420,20 +420,25 @@ int eth_initialize(void) bootstage_mark(BOOTSTAGE_ID_NET_ETH_INIT); do { - if (num_devices) - printf(", "); + if (dev->seq != -1) { + if (num_devices) + printf(", "); - printf("eth%d: %s", dev->seq, dev->name); + printf("eth%d: %s", dev->seq, dev->name); - if (ethprime && dev == prime_dev) - printf(" [PRIME]"); + if (ethprime && dev == prime_dev) + printf(" [PRIME]"); + } eth_write_hwaddr(dev); + if (dev->seq != -1) + num_devices++; uclass_next_device_check(&dev); - num_devices++; } while (dev); + if (!num_devices) + printf("No ethernet found.\n"); putc('\n'); }
device_probe() may fail in which case the seq_id will be -1. Don't display these devices during startup. While this is only a cosmetic change, the return value of eth_initialize() will also change to the actual number of available devices. The return value is only used in spl_net to decide whether there are any devices to boot from. So returning only available devices is also more correct in that case. Signed-off-by: Michael Walle <michael@walle.cc> --- net/eth-uclass.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)