[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 at gmail.com> wrote:
> Hi Joe,
>
> On Wed, Sep 9, 2015 at 11:19 AM, Bin Meng <bmeng.cn at gmail.com> wrote:
>> 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.
>>
>
> 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