[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 at gmail.com> wrote:
> Hi Bin,
>
> On Tue, Sep 8, 2015 at 11:24 AM, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Joe,
>>
>> On Wed, Sep 9, 2015 at 12:01 AM, Joe Hershberger
>> <joe.hershberger at gmail.com> wrote:
>>> Hi Bin,
>>>
>>> On Tue, Sep 8, 2015 at 10:44 AM, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>> Hi Joe,
>>>>
>>>> On Tue, Sep 8, 2015 at 11:32 PM, Joe Hershberger
>>>> <joe.hershberger at gmail.com> wrote:
>>>>> Hi Bin,
>>>>>
>>>>> On Sat, Sep 5, 2015 at 9:38 PM, Bin Meng <bmeng.cn at 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 at 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