[U-Boot] [PATCH] net: eth: Do sanity test on eth dev before eth_get_ops(dev)->start

[U-Boot] [PATCH] net: eth: Do sanity test on eth dev before eth_get_ops(dev)->start

[Bin Meng]

In eth_init(), eth_get_dev() can return NULL. We should do sanity
test on eth dev before calling its start function.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 net/eth.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/eth.c b/net/eth.c
index 26520d3..6ec3a86 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -370,6 +370,10 @@ int eth_init(void)
 		eth_try_another(0);
 		/* This will ensure the new "current" attempted to probe */
 		current = eth_get_dev();
+		if (!current) {
+			printf("No ethernet found.\n");
+			break;
+		}
 	} while (old_current != current);
 
 	return ret;
-- 
1.8.2.1

[U-Boot] [PATCH] net: eth: Do sanity test on eth dev before eth_get_ops(dev)->start

[Joe Hershberger]

Hi Bin,

On Sat, Sep 5, 2015 at 9:38 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> In eth_init(), eth_get_dev() can return NULL. We should do sanity
> test on eth dev before calling its start function.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  net/eth.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/net/eth.c b/net/eth.c
> index 26520d3..6ec3a86 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -370,6 +370,10 @@ int eth_init(void)
>                 eth_try_another(0);
>                 /* This will ensure the new "current" attempted to probe */
>                 current = eth_get_dev();
> +               if (!current) {
> +                       printf("No ethernet found.\n");
> +                       break;
> +               }

I'm not sure I get the point of this. We already have a check above...

        current = eth_get_dev();
        if (!current) {
                printf("No ethernet found.\n");
                return -ENODEV;
        }

>         } while (old_current != current);
>
>         return ret;
> --
> 1.8.2.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] net: eth: Do sanity test on eth dev before eth_get_ops(dev)->start

[Bin Meng]

Hi Joe,

On Tue, Sep 8, 2015 at 11:32 PM, Joe Hershberger
<joe.hershberger@gmail.com> wrote:
> Hi Bin,
>
> On Sat, Sep 5, 2015 at 9:38 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> In eth_init(), eth_get_dev() can return NULL. We should do sanity
>> test on eth dev before calling its start function.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>  net/eth.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/net/eth.c b/net/eth.c
>> index 26520d3..6ec3a86 100644
>> --- a/net/eth.c
>> +++ b/net/eth.c
>> @@ -370,6 +370,10 @@ int eth_init(void)
>>                 eth_try_another(0);
>>                 /* This will ensure the new "current" attempted to probe */
>>                 current = eth_get_dev();
>> +               if (!current) {
>> +                       printf("No ethernet found.\n");
>> +                       break;
>> +               }
>
> I'm not sure I get the point of this. We already have a check above...
>
>         current = eth_get_dev();
>         if (!current) {
>                 printf("No ethernet found.\n");
>                 return -ENODEV;
>         }
>

But this does not help. Each time eth_get_dev() is called, current can
be NULL as driver's probe can fail.

>>         } while (old_current != current);
>>
>>         return ret;
>> --

Regards,
Bin

[U-Boot] [PATCH] net: eth: Do sanity test on eth dev before eth_get_ops(dev)->start

[Joe Hershberger]

Hi Bin,

On Tue, Sep 8, 2015 at 10:44 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Joe,
>
> On Tue, Sep 8, 2015 at 11:32 PM, Joe Hershberger
> <joe.hershberger@gmail.com> wrote:
>> Hi Bin,
>>
>> On Sat, Sep 5, 2015 at 9:38 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> In eth_init(), eth_get_dev() can return NULL. We should do sanity
>>> test on eth dev before calling its start function.
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>> ---
>>>
>>>  net/eth.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/net/eth.c b/net/eth.c
>>> index 26520d3..6ec3a86 100644
>>> --- a/net/eth.c
>>> +++ b/net/eth.c
>>> @@ -370,6 +370,10 @@ int eth_init(void)
>>>                 eth_try_another(0);
>>>                 /* This will ensure the new "current" attempted to probe */
>>>                 current = eth_get_dev();
>>> +               if (!current) {
>>> +                       printf("No ethernet found.\n");
>>> +                       break;
>>> +               }
>>
>> I'm not sure I get the point of this. We already have a check above...
>>
>>         current = eth_get_dev();
>>         if (!current) {
>>                 printf("No ethernet found.\n");
>>                 return -ENODEV;
>>         }
>>
>
> But this does not help. Each time eth_get_dev() is called, current can
> be NULL as driver's probe can fail.

If that's the issue you are hitting it seems like you should attempt
to skip the device instead of printing the message. It doesn't make
sense to me to move to the next device and then print that there is no
Ethernet.

Also, this is fundamental to the eth subsystem. You should add a unit
test that fails in your case.

>>>         } while (old_current != current);
>>>
>>>         return ret;
>>> --
>
> Regards,
> Bin

Thanks,
-Joe

[U-Boot] [PATCH] net: eth: Do sanity test on eth dev before eth_get_ops(dev)->start

[Bin Meng]

Hi Joe,

On Wed, Sep 9, 2015 at 12:01 AM, Joe Hershberger
<joe.hershberger@gmail.com> wrote:
> Hi Bin,
>
> On Tue, Sep 8, 2015 at 10:44 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Joe,
>>
>> On Tue, Sep 8, 2015 at 11:32 PM, Joe Hershberger
>> <joe.hershberger@gmail.com> wrote:
>>> Hi Bin,
>>>
>>> On Sat, Sep 5, 2015 at 9:38 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>> In eth_init(), eth_get_dev() can return NULL. We should do sanity
>>>> test on eth dev before calling its start function.
>>>>
>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>>> ---
>>>>
>>>>  net/eth.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/net/eth.c b/net/eth.c
>>>> index 26520d3..6ec3a86 100644
>>>> --- a/net/eth.c
>>>> +++ b/net/eth.c
>>>> @@ -370,6 +370,10 @@ int eth_init(void)
>>>>                 eth_try_another(0);
>>>>                 /* This will ensure the new "current" attempted to probe */
>>>>                 current = eth_get_dev();
>>>> +               if (!current) {
>>>> +                       printf("No ethernet found.\n");
>>>> +                       break;
>>>> +               }
>>>
>>> I'm not sure I get the point of this. We already have a check above...
>>>
>>>         current = eth_get_dev();
>>>         if (!current) {
>>>                 printf("No ethernet found.\n");
>>>                 return -ENODEV;
>>>         }
>>>
>>
>> But this does not help. Each time eth_get_dev() is called, current can
>> be NULL as driver's probe can fail.
>
> If that's the issue you are hitting it seems like you should attempt
> to skip the device instead of printing the message. It doesn't make
> sense to me to move to the next device and then print that there is no
> Ethernet.

Do you mean we should not printf("No ethernet found.\n") and just break here?

If it fails, U-Boot just crashes as there is a NULL pointer. I am not
sure if test case is able to handle this?

>
> Also, this is fundamental to the eth subsystem. You should add a unit
> test that fails in your case.
>
>>>>         } while (old_current != current);
>>>>
>>>>         return ret;
>>>> --
>>

Regards,
Bin

[U-Boot] [PATCH] net: eth: Do sanity test on eth dev before eth_get_ops(dev)->start

[Joe Hershberger]

Hi Bin,

On Tue, Sep 8, 2015 at 11:24 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Joe,
>
> On Wed, Sep 9, 2015 at 12:01 AM, Joe Hershberger
> <joe.hershberger@gmail.com> wrote:
>> Hi Bin,
>>
>> On Tue, Sep 8, 2015 at 10:44 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Joe,
>>>
>>> On Tue, Sep 8, 2015 at 11:32 PM, Joe Hershberger
>>> <joe.hershberger@gmail.com> wrote:
>>>> Hi Bin,
>>>>
>>>> On Sat, Sep 5, 2015 at 9:38 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>> In eth_init(), eth_get_dev() can return NULL. We should do sanity
>>>>> test on eth dev before calling its start function.
>>>>>
>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>>>> ---
>>>>>
>>>>>  net/eth.c | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/net/eth.c b/net/eth.c
>>>>> index 26520d3..6ec3a86 100644
>>>>> --- a/net/eth.c
>>>>> +++ b/net/eth.c
>>>>> @@ -370,6 +370,10 @@ int eth_init(void)
>>>>>                 eth_try_another(0);
>>>>>                 /* This will ensure the new "current" attempted to probe */
>>>>>                 current = eth_get_dev();
>>>>> +               if (!current) {
>>>>> +                       printf("No ethernet found.\n");
>>>>> +                       break;
>>>>> +               }
>>>>
>>>> I'm not sure I get the point of this. We already have a check above...
>>>>
>>>>         current = eth_get_dev();
>>>>         if (!current) {
>>>>                 printf("No ethernet found.\n");
>>>>                 return -ENODEV;
>>>>         }
>>>>
>>>
>>> But this does not help. Each time eth_get_dev() is called, current can
>>> be NULL as driver's probe can fail.
>>
>> If that's the issue you are hitting it seems like you should attempt
>> to skip the device instead of printing the message. It doesn't make
>> sense to me to move to the next device and then print that there is no
>> Ethernet.
>
> Do you mean we should not printf("No ethernet found.\n") and just break here?

I think you shouldn't break, but rather should have an if check around
the top half of the loop. I.e.:

diff --git a/net/eth.c b/net/eth.c
index d3ec8d6..78ffb5f 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -343,23 +343,27 @@ int eth_init(void)

        old_current = current;
        do {
-               debug("Trying %s\n", current->name);
-
-               if (device_active(current)) {
-                       ret = eth_get_ops(current)->start(current);
-                       if (ret >= 0) {
-                               struct eth_device_priv *priv =
-                                       current->uclass_priv;
-
-                               priv->state = ETH_STATE_ACTIVE;
-                               return 0;
+               if (current) {
+                       debug("Trying %s\n", current->name);
+
+                       if (device_active(current)) {
+                               ret = eth_get_ops(current)->start(current);
+                               if (ret >= 0) {
+                                       struct eth_device_priv *priv =
+                                               current->uclass_priv;
+
+                                       priv->state = ETH_STATE_ACTIVE;
+                                       return 0;
+                               }
+                       } else {
+                               ret = eth_errno;
                        }
+
+                       debug("FAIL\n");
                } else {
-                       ret = eth_errno;
+                       debug("PROBE FAIL\n");
                }

-               debug("FAIL\n");
-
                /*
                 * If ethrotate is enabled, this will change "current",
                 * otherwise we will drop out of this while loop immediately
---

Note that I have not tested this, it's just what I'm thinking is more
appropriate.

> If it fails, U-Boot just crashes as there is a NULL pointer. I am not
> sure if test case is able to handle this?

I think it's good to have the a test that hits your scenario. The bug
fix will prevent the crash, so it's not like we expect it to crash,
but it will lock down the desired behavior for this condition.

>> Also, this is fundamental to the eth subsystem. You should add a unit
>> test that fails in your case.
>>
>>>>>         } while (old_current != current);
>>>>>
>>>>>         return ret;
>>>>> --
>>>
>
> Regards,
> Bin

[U-Boot] [PATCH] net: eth: Do sanity test on eth dev before eth_get_ops(dev)->start

[Bin Meng]

Hi Joe,

On Wed, Sep 9, 2015 at 1:23 AM, Joe Hershberger
<joe.hershberger@gmail.com> wrote:
> Hi Bin,
>
> On Tue, Sep 8, 2015 at 11:24 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Joe,
>>
>> On Wed, Sep 9, 2015 at 12:01 AM, Joe Hershberger
>> <joe.hershberger@gmail.com> wrote:
>>> Hi Bin,
>>>
>>> On Tue, Sep 8, 2015 at 10:44 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>> Hi Joe,
>>>>
>>>> On Tue, Sep 8, 2015 at 11:32 PM, Joe Hershberger
>>>> <joe.hershberger@gmail.com> wrote:
>>>>> Hi Bin,
>>>>>
>>>>> On Sat, Sep 5, 2015 at 9:38 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>>> In eth_init(), eth_get_dev() can return NULL. We should do sanity
>>>>>> test on eth dev before calling its start function.
>>>>>>
>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>>>>> ---
>>>>>>
>>>>>>  net/eth.c | 4 ++++
>>>>>>  1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/net/eth.c b/net/eth.c
>>>>>> index 26520d3..6ec3a86 100644
>>>>>> --- a/net/eth.c
>>>>>> +++ b/net/eth.c
>>>>>> @@ -370,6 +370,10 @@ int eth_init(void)
>>>>>>                 eth_try_another(0);
>>>>>>                 /* This will ensure the new "current" attempted to probe */
>>>>>>                 current = eth_get_dev();
>>>>>> +               if (!current) {
>>>>>> +                       printf("No ethernet found.\n");
>>>>>> +                       break;
>>>>>> +               }
>>>>>
>>>>> I'm not sure I get the point of this. We already have a check above...
>>>>>
>>>>>         current = eth_get_dev();
>>>>>         if (!current) {
>>>>>                 printf("No ethernet found.\n");
>>>>>                 return -ENODEV;
>>>>>         }
>>>>>
>>>>
>>>> But this does not help. Each time eth_get_dev() is called, current can
>>>> be NULL as driver's probe can fail.
>>>
>>> If that's the issue you are hitting it seems like you should attempt
>>> to skip the device instead of printing the message. It doesn't make
>>> sense to me to move to the next device and then print that there is no
>>> Ethernet.
>>
>> Do you mean we should not printf("No ethernet found.\n") and just break here?
>
> I think you shouldn't break, but rather should have an if check around
> the top half of the loop. I.e.:
>
> diff --git a/net/eth.c b/net/eth.c
> index d3ec8d6..78ffb5f 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -343,23 +343,27 @@ int eth_init(void)
>
>         old_current = current;
>         do {
> -               debug("Trying %s\n", current->name);
> -
> -               if (device_active(current)) {
> -                       ret = eth_get_ops(current)->start(current);
> -                       if (ret >= 0) {
> -                               struct eth_device_priv *priv =
> -                                       current->uclass_priv;
> -
> -                               priv->state = ETH_STATE_ACTIVE;
> -                               return 0;
> +               if (current) {
> +                       debug("Trying %s\n", current->name);
> +
> +                       if (device_active(current)) {
> +                               ret = eth_get_ops(current)->start(current);
> +                               if (ret >= 0) {
> +                                       struct eth_device_priv *priv =
> +                                               current->uclass_priv;
> +
> +                                       priv->state = ETH_STATE_ACTIVE;
> +                                       return 0;
> +                               }
> +                       } else {
> +                               ret = eth_errno;
>                         }
> +
> +                       debug("FAIL\n");
>                 } else {
> -                       ret = eth_errno;
> +                       debug("PROBE FAIL\n");
>                 }
>
> -               debug("FAIL\n");
> -
>                 /*
>                  * If ethrotate is enabled, this will change "current",
>                  * otherwise we will drop out of this while loop immediately
> ---
>
> Note that I have not tested this, it's just what I'm thinking is more
> appropriate.
>
>> If it fails, U-Boot just crashes as there is a NULL pointer. I am not
>> sure if test case is able to handle this?
>
> I think it's good to have the a test that hits your scenario. The bug
> fix will prevent the crash, so it's not like we expect it to crash,
> but it will lock down the desired behavior for this condition.
>

I am afraid creating a test case to cover this scenario is not that
easy. Checking function return value does not bring any harm. It makes
our codes safer. In fact, during further debug today, I found another
two places which does not check device_probe() return value. And it is
indeed these two places which causes the subsequent failure here.

>>> Also, this is fundamental to the eth subsystem. You should add a unit
>>> test that fails in your case.
>>>
>>>>>>         } while (old_current != current);
>>>>>>
>>>>>>         return ret;
>>>>>> --
>>>>
>>

Regards,
Bin

[U-Boot] [PATCH] net: eth: Do sanity test on eth dev before eth_get_ops(dev)->start

[Bin Meng]

Hi Joe,

On Wed, Sep 9, 2015 at 11:19 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Joe,
>
> On Wed, Sep 9, 2015 at 1:23 AM, Joe Hershberger
> <joe.hershberger@gmail.com> wrote:
>> Hi Bin,
>>
>> On Tue, Sep 8, 2015 at 11:24 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Joe,
>>>
>>> On Wed, Sep 9, 2015 at 12:01 AM, Joe Hershberger
>>> <joe.hershberger@gmail.com> wrote:
>>>> Hi Bin,
>>>>
>>>> On Tue, Sep 8, 2015 at 10:44 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>> Hi Joe,
>>>>>
>>>>> On Tue, Sep 8, 2015 at 11:32 PM, Joe Hershberger
>>>>> <joe.hershberger@gmail.com> wrote:
>>>>>> Hi Bin,
>>>>>>
>>>>>> On Sat, Sep 5, 2015 at 9:38 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>>>> In eth_init(), eth_get_dev() can return NULL. We should do sanity
>>>>>>> test on eth dev before calling its start function.
>>>>>>>
>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>>  net/eth.c | 4 ++++
>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>>
>>>>>>> diff --git a/net/eth.c b/net/eth.c
>>>>>>> index 26520d3..6ec3a86 100644
>>>>>>> --- a/net/eth.c
>>>>>>> +++ b/net/eth.c
>>>>>>> @@ -370,6 +370,10 @@ int eth_init(void)
>>>>>>>                 eth_try_another(0);
>>>>>>>                 /* This will ensure the new "current" attempted to probe */
>>>>>>>                 current = eth_get_dev();
>>>>>>> +               if (!current) {
>>>>>>> +                       printf("No ethernet found.\n");
>>>>>>> +                       break;
>>>>>>> +               }
>>>>>>
>>>>>> I'm not sure I get the point of this. We already have a check above...
>>>>>>
>>>>>>         current = eth_get_dev();
>>>>>>         if (!current) {
>>>>>>                 printf("No ethernet found.\n");
>>>>>>                 return -ENODEV;
>>>>>>         }
>>>>>>
>>>>>
>>>>> But this does not help. Each time eth_get_dev() is called, current can
>>>>> be NULL as driver's probe can fail.
>>>>
>>>> If that's the issue you are hitting it seems like you should attempt
>>>> to skip the device instead of printing the message. It doesn't make
>>>> sense to me to move to the next device and then print that there is no
>>>> Ethernet.
>>>
>>> Do you mean we should not printf("No ethernet found.\n") and just break here?
>>
>> I think you shouldn't break, but rather should have an if check around
>> the top half of the loop. I.e.:
>>
>> diff --git a/net/eth.c b/net/eth.c
>> index d3ec8d6..78ffb5f 100644
>> --- a/net/eth.c
>> +++ b/net/eth.c
>> @@ -343,23 +343,27 @@ int eth_init(void)
>>
>>         old_current = current;
>>         do {
>> -               debug("Trying %s\n", current->name);
>> -
>> -               if (device_active(current)) {
>> -                       ret = eth_get_ops(current)->start(current);
>> -                       if (ret >= 0) {
>> -                               struct eth_device_priv *priv =
>> -                                       current->uclass_priv;
>> -
>> -                               priv->state = ETH_STATE_ACTIVE;
>> -                               return 0;
>> +               if (current) {
>> +                       debug("Trying %s\n", current->name);
>> +
>> +                       if (device_active(current)) {
>> +                               ret = eth_get_ops(current)->start(current);
>> +                               if (ret >= 0) {
>> +                                       struct eth_device_priv *priv =
>> +                                               current->uclass_priv;
>> +
>> +                                       priv->state = ETH_STATE_ACTIVE;
>> +                                       return 0;
>> +                               }
>> +                       } else {
>> +                               ret = eth_errno;
>>                         }
>> +
>> +                       debug("FAIL\n");
>>                 } else {
>> -                       ret = eth_errno;
>> +                       debug("PROBE FAIL\n");
>>                 }
>>
>> -               debug("FAIL\n");
>> -
>>                 /*
>>                  * If ethrotate is enabled, this will change "current",
>>                  * otherwise we will drop out of this while loop immediately
>> ---
>>
>> Note that I have not tested this, it's just what I'm thinking is more
>> appropriate.
>>
>>> If it fails, U-Boot just crashes as there is a NULL pointer. I am not
>>> sure if test case is able to handle this?
>>
>> I think it's good to have the a test that hits your scenario. The bug
>> fix will prevent the crash, so it's not like we expect it to crash,
>> but it will lock down the desired behavior for this condition.
>>
>
> I am afraid creating a test case to cover this scenario is not that
> easy. Checking function return value does not bring any harm. It makes
> our codes safer. In fact, during further debug today, I found another
> two places which does not check device_probe() return value. And it is
> indeed these two places which causes the subsequent failure here.
>

OK, I've found a way in the DM test codes to trigger this fault, but
unfortunately by creating this test case I've found another potential
issue. I will send a v2 patch for all of these.

Regards,
Bin

[U-Boot] [PATCH] net: eth: Do sanity test on eth dev before eth_get_ops(dev)->start

[Joe Hershberger]

Hi Bin,

On Wed, Sep 9, 2015 at 1:14 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Joe,
>
> On Wed, Sep 9, 2015 at 11:19 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Joe,
>>
>> On Wed, Sep 9, 2015 at 1:23 AM, Joe Hershberger
>> <joe.hershberger@gmail.com> wrote:
>>> Hi Bin,
>>>
>>> On Tue, Sep 8, 2015 at 11:24 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>> Hi Joe,
>>>>
>>>> On Wed, Sep 9, 2015 at 12:01 AM, Joe Hershberger
>>>> <joe.hershberger@gmail.com> wrote:
>>>>> Hi Bin,
>>>>>
>>>>> On Tue, Sep 8, 2015 at 10:44 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>>> Hi Joe,
>>>>>>
>>>>>> On Tue, Sep 8, 2015 at 11:32 PM, Joe Hershberger
>>>>>> <joe.hershberger@gmail.com> wrote:
>>>>>>> Hi Bin,
>>>>>>>
>>>>>>> On Sat, Sep 5, 2015 at 9:38 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>>>>> In eth_init(), eth_get_dev() can return NULL. We should do sanity
>>>>>>>> test on eth dev before calling its start function.
>>>>>>>>
>>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>  net/eth.c | 4 ++++
>>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/net/eth.c b/net/eth.c
>>>>>>>> index 26520d3..6ec3a86 100644
>>>>>>>> --- a/net/eth.c
>>>>>>>> +++ b/net/eth.c
>>>>>>>> @@ -370,6 +370,10 @@ int eth_init(void)
>>>>>>>>                 eth_try_another(0);
>>>>>>>>                 /* This will ensure the new "current" attempted to probe */
>>>>>>>>                 current = eth_get_dev();
>>>>>>>> +               if (!current) {
>>>>>>>> +                       printf("No ethernet found.\n");
>>>>>>>> +                       break;
>>>>>>>> +               }
>>>>>>>
>>>>>>> I'm not sure I get the point of this. We already have a check above...
>>>>>>>
>>>>>>>         current = eth_get_dev();
>>>>>>>         if (!current) {
>>>>>>>                 printf("No ethernet found.\n");
>>>>>>>                 return -ENODEV;
>>>>>>>         }
>>>>>>>
>>>>>>
>>>>>> But this does not help. Each time eth_get_dev() is called, current can
>>>>>> be NULL as driver's probe can fail.
>>>>>
>>>>> If that's the issue you are hitting it seems like you should attempt
>>>>> to skip the device instead of printing the message. It doesn't make
>>>>> sense to me to move to the next device and then print that there is no
>>>>> Ethernet.
>>>>
>>>> Do you mean we should not printf("No ethernet found.\n") and just break here?
>>>
>>> I think you shouldn't break, but rather should have an if check around
>>> the top half of the loop. I.e.:
>>>
>>> diff --git a/net/eth.c b/net/eth.c
>>> index d3ec8d6..78ffb5f 100644
>>> --- a/net/eth.c
>>> +++ b/net/eth.c
>>> @@ -343,23 +343,27 @@ int eth_init(void)
>>>
>>>         old_current = current;
>>>         do {
>>> -               debug("Trying %s\n", current->name);
>>> -
>>> -               if (device_active(current)) {
>>> -                       ret = eth_get_ops(current)->start(current);
>>> -                       if (ret >= 0) {
>>> -                               struct eth_device_priv *priv =
>>> -                                       current->uclass_priv;
>>> -
>>> -                               priv->state = ETH_STATE_ACTIVE;
>>> -                               return 0;
>>> +               if (current) {
>>> +                       debug("Trying %s\n", current->name);
>>> +
>>> +                       if (device_active(current)) {
>>> +                               ret = eth_get_ops(current)->start(current);
>>> +                               if (ret >= 0) {
>>> +                                       struct eth_device_priv *priv =
>>> +                                               current->uclass_priv;
>>> +
>>> +                                       priv->state = ETH_STATE_ACTIVE;
>>> +                                       return 0;
>>> +                               }
>>> +                       } else {
>>> +                               ret = eth_errno;
>>>                         }
>>> +
>>> +                       debug("FAIL\n");
>>>                 } else {
>>> -                       ret = eth_errno;
>>> +                       debug("PROBE FAIL\n");
>>>                 }
>>>
>>> -               debug("FAIL\n");
>>> -
>>>                 /*
>>>                  * If ethrotate is enabled, this will change "current",
>>>                  * otherwise we will drop out of this while loop immediately
>>> ---
>>>
>>> Note that I have not tested this, it's just what I'm thinking is more
>>> appropriate.
>>>
>>>> If it fails, U-Boot just crashes as there is a NULL pointer. I am not
>>>> sure if test case is able to handle this?
>>>
>>> I think it's good to have the a test that hits your scenario. The bug
>>> fix will prevent the crash, so it's not like we expect it to crash,
>>> but it will lock down the desired behavior for this condition.
>>>
>>
>> I am afraid creating a test case to cover this scenario is not that
>> easy. Checking function return value does not bring any harm. It makes
>> our codes safer. In fact, during further debug today, I found another
>> two places which does not check device_probe() return value. And it is
>> indeed these two places which causes the subsequent failure here.
>>
>
> OK, I've found a way in the DM test codes to trigger this fault, but
> unfortunately by creating this test case I've found another potential
> issue. I will send a v2 patch for all of these.

Thanks for looking into this.

Cheers,
-Joe