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

[Simon Horman <simon.horman@corigine.com>]

On Wed, Jun 14, 2023 at 12:54:37AM +0300, Vladimir Oltean wrote:

Hi Vladimir,

some minor feedback from my side.

...

> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 35fa1ca98671..58cd51de5b79 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -291,13 +291,19 @@ static void nsim_setup(struct net_device *dev)
>  
>  static int nsim_init_netdevsim(struct netdevsim *ns)
>  {
> +	struct mock_phc *phc;
>  	int err;
>  
> +	phc = mock_phc_create(&ns->nsim_bus_dev->dev);
> +	if (IS_ERR(phc))
> +		return PTR_ERR(phc);
> +
> +	ns->phc = phc;
>  	ns->netdev->netdev_ops = &nsim_netdev_ops;
>  
>  	err = nsim_udp_tunnels_info_create(ns->nsim_dev, ns->netdev);
>  	if (err)
> -		return err;
> +		goto err_phc_destroy;
>  
>  	rtnl_lock();
>  	err = nsim_bpf_init(ns);

...

> diff --git a/drivers/ptp/ptp_mock.c b/drivers/ptp/ptp_mock.c
> new file mode 100644
> index 000000000000..e09e6009c4f7
> --- /dev/null
> +++ b/drivers/ptp/ptp_mock.c
> @@ -0,0 +1,175 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2023 NXP
> + *
> + * Mock-up PTP Hardware Clock driver for virtual network devices
> + *
> + * Create a PTP clock which offers PTP time manipulation operations
> + * using a timecounter/cyclecounter on top of CLOCK_MONOTONIC_RAW.
> + */
> +
> +#include <linux/ptp_clock_kernel.h>
> +#include <linux/ptp_mock.h>
> +#include <linux/timecounter.h>
> +
> +/* Clamp scaled_ppm between -2,097,152,000 and 2,097,152,000,
> + * and thus "adj" between -68,719,476 and 68,719,476
> + */
> +#define MOCK_PHC_MAX_ADJ_PPB		32000000
> +/* Timestamps from ktime_get_raw() have 1 ns resolution, so the scale factor
> + * (MULT >> SHIFT) needs to be 1. Pick SHIFT as 31 bits, which translates
> + * MULT(freq 0) into 0x80000000.
> + */
> +#define MOCK_PHC_CC_SHIFT		31
> +#define MOCK_PHC_CC_MULT		(1 << MOCK_PHC_CC_SHIFT)

Maybe BIT()?

...

> +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.

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

...
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan