Re: [EXT] [bug report] net: aquantia: add basic ptp_clock callbacks

[Dan Carpenter <dan.carpenter@oracle.com>]

On Tue, Oct 29, 2019 at 09:14:33AM +0000, Igor Russkikh wrote:
> 
> On 28.10.2019 18:25, Egor Pomozov wrote:
> > Hello Dan,
> > +Igor
> > 
> > Thank you for pointing the issue. We’ll fixed soon.
> 
> 
> >>  1207          clock = ptp_clock_register(&aq_ptp->ptp_info, &aq_nic->ndev->dev);
> >>  1208          if (!clock || IS_ERR(clock)) {
> >>  1209                  netdev_err(aq_nic->ndev, "ptp_clock_register failed\n");
> >>  1210                  err = PTR_ERR(clock);
> >>  1211                  goto err_exit;
> >>
> >> This is a false positive in Smatch but the code is still problematic.
> >>
> >> The issue is that ptp_clock_register() returns error pointers if there
> >> is an error and it returns NULL if the clock is disabled in the config.
> >> If "clock" is NULL then this code returns PTR_ERR(NULL) which is
> >> success but so that's a bug.
> >>
> >> The question is, is it really realistic for people to run this hardware
> >> without a ptp clock?  If so then we should allow it instead of erroring
> >> out here when clock is NULL.  If not then we should enforce that in the
> >> Kconfig instead of waiting until runtime.
> 
> Hi Dan, I'd say thats a false positive.
> There exist HW configuration where PTP is disabled or not available.
> 
> PTR_ERR(NULL) is 0, so eventually driver now functions correctly, allowing to
> proceed but marking ptp functionality as disabled.
> 
> I agree that's a bit counterintuitive but correct.

I have looked at it again and the "!clock" check should be removed
because it is dead code.  It's not possible for it to be NULL there
because aq_ptp_init() is a dummy function if the PTP clock is not
enabled.

regards,
dan carpenter