[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 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.

Regards,
Bin