Re: [PATCH v2 net-next 6/9] net: netdevsim: create a mock-up PTP Hardware Clock driver

[Vladimir Oltean <vladimir.oltean@nxp.com>]

Hi Simon,

On Wed, Jun 14, 2023 at 03:11:44PM +0200, Simon Horman wrote:
> > +#define MOCK_PHC_CC_SHIFT		31
> > +#define MOCK_PHC_CC_MULT		(1 << MOCK_PHC_CC_SHIFT)
> 
> Maybe BIT()?

Sorry, not everything that is 1 << something has BIT() semantics.
This in particular is quite clearly just a multiplier factor
expressed as a power of 2.

> > +struct mock_phc *mock_phc_create(struct device *dev)
> > +{
> > +	struct mock_phc *phc;
> > +	int err;
> > +
> > +	phc = kzalloc(sizeof(*phc), GFP_KERNEL);
> > +	if (!phc) {
> > +		err = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	phc->info = (struct ptp_clock_info) {
> > +		.owner		= THIS_MODULE,
> > +		.name		= "Mock-up PTP clock",
> > +		.max_adj	= MOCK_PHC_MAX_ADJ_PPB,
> > +		.adjfine	= mock_phc_adjfine,
> > +		.adjtime	= mock_phc_adjtime,
> > +		.gettime64	= mock_phc_gettime64,
> > +		.settime64	= mock_phc_settime64,
> > +		.do_aux_work	= mock_phc_refresh,
> > +	};
> > +
> > +	phc->cc = (struct cyclecounter) {
> > +		.read	= mock_phc_cc_read,
> > +		.mask	= CYCLECOUNTER_MASK(64),
> > +		.mult	= MOCK_PHC_CC_MULT,
> > +		.shift	= MOCK_PHC_CC_SHIFT,
> > +	};
> > +
> > +	spin_lock_init(&phc->lock);
> > +	timecounter_init(&phc->tc, &phc->cc, 0);
> > +
> > +	phc->clock = ptp_clock_register(&phc->info, dev);
> > +	if (IS_ERR_OR_NULL(phc->clock)) {
> > +		err = PTR_ERR_OR_ZERO(phc->clock);
> > +		goto out_free_phc;
> > +	}
> > +
> > +	ptp_schedule_worker(phc->clock, MOCK_PHC_REFRESH_INTERVAL);
> > +
> > +	return phc;
> > +
> > +out_free_phc:
> > +	kfree(phc);
> > +out:
> > +	return ERR_PTR(err);
> > +}
> 
> Smatch complains that ERR_PTR may be passed zero.
> Looking at the IS_ERR_OR_NULL block above, this does indeed seem to be the
> case.

The intention here had something to do with PTP being optional for the
caller (netdevsim). Not sure whether the implementation is the best -
and in particular whether ERR_PTR(0) is NULL or not. I guess this is
what the smatch warning (which I haven't looked at) is saying.

> Keeping Smatch happy is one thing - your call - but I do wonder if the
> caller of mock_phc_create() handles the NULL case correctly.

mock_phc_create() returns a pointer to an opaque data structure -
struct mock_phc - and the caller just carries that pointer around to the
other API calls exported by the mock_phc module. It doesn't need to care
whether the pointer is NULL or not, just the mock_phc module does (and
it does handle that part well, at least assuming that the pointer is NULL).