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
