Hi Bin, On Wed, Sep 9, 2015 at 1:14 AM, Bin Meng <bmeng...@gmail.com> wrote: > 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.
Thanks for looking into this. Cheers, -Joe _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot