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. >>> 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot