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

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