* [U-Boot] [PATCH] net: eth-uclass: ignore unavailable devices
@ 2019-10-21 23:03 Michael Walle
2019-11-30 1:09 ` Joe Hershberger
2019-12-01 0:55 ` Alexandru Marginean
0 siblings, 2 replies; 5+ messages in thread
From: Michael Walle @ 2019-10-21 23:03 UTC (permalink / raw)
To: u-boot
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');
}
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] net: eth-uclass: ignore unavailable devices
2019-10-21 23:03 [U-Boot] [PATCH] net: eth-uclass: ignore unavailable devices Michael Walle
@ 2019-11-30 1:09 ` Joe Hershberger
2019-12-01 0:55 ` Alexandru Marginean
1 sibling, 0 replies; 5+ messages in thread
From: Joe Hershberger @ 2019-11-30 1:09 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] net: eth-uclass: ignore unavailable devices
2019-10-21 23:03 [U-Boot] [PATCH] net: eth-uclass: ignore unavailable devices Michael Walle
2019-11-30 1:09 ` Joe Hershberger
@ 2019-12-01 0:55 ` Alexandru Marginean
2019-12-01 16:43 ` Michael Walle
1 sibling, 1 reply; 5+ messages in thread
From: Alexandru Marginean @ 2019-12-01 0:55 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] net: eth-uclass: ignore unavailable devices
2019-12-01 0:55 ` Alexandru Marginean
@ 2019-12-01 16:43 ` Michael Walle
2019-12-02 11:38 ` Alexandru Marginean
0 siblings, 1 reply; 5+ messages in thread
From: Michael Walle @ 2019-12-01 16:43 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] net: eth-uclass: ignore unavailable devices
2019-12-01 16:43 ` Michael Walle
@ 2019-12-02 11:38 ` Alexandru Marginean
0 siblings, 0 replies; 5+ messages in thread
From: Alexandru Marginean @ 2019-12-02 11:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-02 11:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-21 23:03 [U-Boot] [PATCH] net: eth-uclass: ignore unavailable devices Michael Walle
2019-11-30 1:09 ` Joe Hershberger
2019-12-01 0:55 ` Alexandru Marginean
2019-12-01 16:43 ` Michael Walle
2019-12-02 11:38 ` Alexandru Marginean
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.