Hi Joe,
On Wed, Sep 9, 2015 at 11:19 AM, Bin Meng <bmeng...@gmail.com> wrote:
> Hi Joe,
>
> On Wed, Sep 9, 2015 at 1:23 AM, Joe Hershberger
> <joe.hershber...@gmail.com> wrote:
>> Hi Bin,
>>
>> On Tue, Sep 8, 2015 at 11:24 AM, Bin Meng <bmeng...@gmail.com> wrote:
>>> Hi Joe,
>>>
>>> On Wed, Sep 9, 2015 at 12:01 AM, Joe Hershberger
>>> <joe.hershber...@gmail.com> wrote:
>>>> Hi Bin,
>>>>
>>>> On Tue, Sep 8, 2015 at 10:44 AM, Bin Meng <bmeng...@gmail.com> wrote:
>>>>> Hi Joe,
>>>>>
>>>>> On Tue, Sep 8, 2015 at 11:32 PM, Joe Hershberger
>>>>> <joe.hershber...@gmail.com> wrote:
>>>>>> Hi Bin,
>>>>>>
>>>>>> On Sat, Sep 5, 2015 at 9:38 PM, Bin Meng <bmeng...@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...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
