I'd like to make netdevsim offload tc-taprio, but currently, this Qdisc
emits a ETHTOOL_GET_TS_INFO call to the driver to make sure that it has
a PTP clock, so that it is reasonably capable of offloading the schedule.
Needless to say, netdevsim doesn't have a PTP clock, so that's something
to think about.
It wouldn't be that hard to create an object which emulates PTP clock
operations on top of the unadjustable CLOCK_MONOTONIC_RAW plus a
software-controlled time domain via a timecounter/cyclecounter and then
link that PHC to the netdevsim device, so this is what is done here.
The scope of the mock-up PHC driver seems to be in drivers/ptp/, since
it is in principle reusable by other virtual network devices as well,
such as veth, and those could even take packet timestamps when passing
skbs between peers (the same timestamp is given as TX timestamp to one
peer, and as RX timestamp to the other, resulting in a zero-delay link).
Nonetheless, netdevsim doesn't support packet RX/TX (and taprio doesn't
need packet timestamping), so for now, the mock-up PHC driver doesn't
support packet timestamping either.
The driver is fully functional for its intended purpose, and it
successfully passes the PTP selftests.
$ echo "1 1 8" > /sys/bus/netdevsim/new_device
$ ethtool -T eni1np1
Time stamping parameters for eni1np1:
Capabilities:
PTP Hardware Clock: 2
Hardware Transmit Timestamp Modes: none
Hardware Receive Filter Modes: none
$ cd tools/testing/selftests/ptp/
$ ./phc.sh /dev/ptp2
TEST: settime [ OK ]
TEST: adjtime [ OK ]
TEST: adjfreq [ OK ]
Signed-off-by: Vladimir Oltean <[email protected]>
---
v1->v2: patch is new
drivers/net/Kconfig | 1 +
drivers/net/netdevsim/ethtool.c | 11 ++
drivers/net/netdevsim/netdev.c | 11 +-
drivers/net/netdevsim/netdevsim.h | 2 +
drivers/ptp/Kconfig | 11 ++
drivers/ptp/Makefile | 1 +
drivers/ptp/ptp_mock.c | 175 ++++++++++++++++++++++++++++++
include/linux/ptp_mock.h | 38 +++++++
8 files changed, 249 insertions(+), 1 deletion(-)
create mode 100644 drivers/ptp/ptp_mock.c
create mode 100644 include/linux/ptp_mock.h
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 368c6f5b327e..4953c1494723 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -592,6 +592,7 @@ config NETDEVSIM
depends on INET
depends on IPV6 || IPV6=n
depends on PSAMPLE || PSAMPLE=n
+ depends on PTP_1588_CLOCK_MOCK || PTP_1588_CLOCK_MOCK=n
select NET_DEVLINK
help
This driver is a developer testing tool and software model that can
diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index ffd9f84b6644..bd546d4d26c6 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -140,6 +140,16 @@ nsim_set_fecparam(struct net_device *dev, struct ethtool_fecparam *fecparam)
return 0;
}
+static int nsim_get_ts_info(struct net_device *dev,
+ struct ethtool_ts_info *info)
+{
+ struct netdevsim *ns = netdev_priv(dev);
+
+ info->phc_index = mock_phc_index(ns->phc);
+
+ return 0;
+}
+
static const struct ethtool_ops nsim_ethtool_ops = {
.supported_coalesce_params = ETHTOOL_COALESCE_ALL_PARAMS,
.get_pause_stats = nsim_get_pause_stats,
@@ -153,6 +163,7 @@ static const struct ethtool_ops nsim_ethtool_ops = {
.set_channels = nsim_set_channels,
.get_fecparam = nsim_get_fecparam,
.set_fecparam = nsim_set_fecparam,
+ .get_ts_info = nsim_get_ts_info,
};
static void nsim_ethtool_ring_init(struct netdevsim *ns)
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);
@@ -318,6 +324,8 @@ static int nsim_init_netdevsim(struct netdevsim *ns)
err_utn_destroy:
rtnl_unlock();
nsim_udp_tunnels_info_destroy(ns->netdev);
+err_phc_destroy:
+ mock_phc_destroy(ns->phc);
return err;
}
@@ -380,6 +388,7 @@ void nsim_destroy(struct netdevsim *ns)
rtnl_unlock();
if (nsim_dev_port_is_pf(ns->nsim_dev_port))
nsim_udp_tunnels_info_destroy(dev);
+ mock_phc_destroy(ns->phc);
free_netdev(dev);
}
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 7d8ed8d8df5c..59526420c78e 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -19,6 +19,7 @@
#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/netdevice.h>
+#include <linux/ptp_mock.h>
#include <linux/u64_stats_sync.h>
#include <net/devlink.h>
#include <net/udp_tunnel.h>
@@ -73,6 +74,7 @@ struct netdevsim {
struct net_device *netdev;
struct nsim_dev *nsim_dev;
struct nsim_dev_port *nsim_dev_port;
+ struct mock_phc *phc;
u64 tx_packets;
u64 tx_bytes;
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 32dff1b4f891..ed9d97a032f1 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -155,6 +155,17 @@ config PTP_1588_CLOCK_IDTCM
To compile this driver as a module, choose M here: the module
will be called ptp_clockmatrix.
+config PTP_1588_CLOCK_MOCK
+ tristate "Mock-up PTP clock"
+ depends on PTP_1588_CLOCK
+ help
+ This driver offers a set of PTP clock manipulation operations over
+ the system monotonic time. It can be used by virtual network device
+ drivers to emulate PTP capabilities.
+
+ To compile this driver as a module, choose M here: the module
+ will be called ptp_mock.
+
config PTP_1588_CLOCK_VMW
tristate "VMware virtual PTP clock"
depends on ACPI && HYPERVISOR_GUEST && X86
diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index 553f18bf3c83..dea0cebd2303 100644
--- a/drivers/ptp/Makefile
+++ b/drivers/ptp/Makefile
@@ -16,6 +16,7 @@ ptp-qoriq-y += ptp_qoriq.o
ptp-qoriq-$(CONFIG_DEBUG_FS) += ptp_qoriq_debugfs.o
obj-$(CONFIG_PTP_1588_CLOCK_IDTCM) += ptp_clockmatrix.o
obj-$(CONFIG_PTP_1588_CLOCK_IDT82P33) += ptp_idt82p33.o
+obj-$(CONFIG_PTP_1588_CLOCK_MOCK) += ptp_mock.o
obj-$(CONFIG_PTP_1588_CLOCK_VMW) += ptp_vmw.o
obj-$(CONFIG_PTP_1588_CLOCK_OCP) += ptp_ocp.o
obj-$(CONFIG_PTP_DFL_TOD) += ptp_dfl_tod.o
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)
+#define MOCK_PHC_FADJ_SHIFT 9
+#define MOCK_PHC_FADJ_DENOMINATOR 15625ULL
+
+/* The largest cycle_delta that timecounter_read_delta() can handle without a
+ * 64-bit overflow during the multiplication with cc->mult, given the max "adj"
+ * we permit, is ~8.3 seconds. Make sure readouts are more frequent than that.
+ */
+#define MOCK_PHC_REFRESH_INTERVAL (HZ * 5)
+
+#define info_to_phc(d) container_of((d), struct mock_phc, info)
+
+struct mock_phc {
+ struct ptp_clock_info info;
+ struct ptp_clock *clock;
+ struct timecounter tc;
+ struct cyclecounter cc;
+ spinlock_t lock;
+};
+
+static u64 mock_phc_cc_read(const struct cyclecounter *cc)
+{
+ return ktime_to_ns(ktime_get_raw());
+}
+
+static int mock_phc_adjfine(struct ptp_clock_info *info, long scaled_ppm)
+{
+ struct mock_phc *phc = info_to_phc(info);
+ s64 adj;
+
+ adj = (s64)scaled_ppm << MOCK_PHC_FADJ_SHIFT;
+ adj = div_s64(adj, MOCK_PHC_FADJ_DENOMINATOR);
+
+ spin_lock(&phc->lock);
+ timecounter_read(&phc->tc);
+ phc->cc.mult = MOCK_PHC_CC_MULT + adj;
+ spin_unlock(&phc->lock);
+
+ return 0;
+}
+
+static int mock_phc_adjtime(struct ptp_clock_info *info, s64 delta)
+{
+ struct mock_phc *phc = info_to_phc(info);
+
+ spin_lock(&phc->lock);
+ timecounter_adjtime(&phc->tc, delta);
+ spin_unlock(&phc->lock);
+
+ return 0;
+}
+
+static int mock_phc_settime64(struct ptp_clock_info *info,
+ const struct timespec64 *ts)
+{
+ struct mock_phc *phc = info_to_phc(info);
+ u64 ns = timespec64_to_ns(ts);
+
+ spin_lock(&phc->lock);
+ timecounter_init(&phc->tc, &phc->cc, ns);
+ spin_unlock(&phc->lock);
+
+ return 0;
+}
+
+static int mock_phc_gettime64(struct ptp_clock_info *info, struct timespec64 *ts)
+{
+ struct mock_phc *phc = info_to_phc(info);
+ u64 ns;
+
+ spin_lock(&phc->lock);
+ ns = timecounter_read(&phc->tc);
+ spin_unlock(&phc->lock);
+
+ *ts = ns_to_timespec64(ns);
+
+ return 0;
+}
+
+static long mock_phc_refresh(struct ptp_clock_info *info)
+{
+ struct timespec64 ts;
+
+ mock_phc_gettime64(info, &ts);
+
+ return MOCK_PHC_REFRESH_INTERVAL;
+}
+
+int mock_phc_index(struct mock_phc *phc)
+{
+ if (!phc)
+ return -1;
+
+ return ptp_clock_index(phc->clock);
+}
+
+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);
+}
+
+void mock_phc_destroy(struct mock_phc *phc)
+{
+ if (!phc)
+ return;
+
+ ptp_clock_unregister(phc->clock);
+ kfree(phc);
+}
diff --git a/include/linux/ptp_mock.h b/include/linux/ptp_mock.h
new file mode 100644
index 000000000000..72eb401034d9
--- /dev/null
+++ b/include/linux/ptp_mock.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Mock-up PTP Hardware Clock driver for virtual network devices
+ *
+ * Copyright 2023 NXP
+ */
+
+#ifndef _PTP_MOCK_H_
+#define _PTP_MOCK_H_
+
+struct device;
+struct mock_phc;
+
+#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_MOCK)
+
+struct mock_phc *mock_phc_create(struct device *dev);
+void mock_phc_destroy(struct mock_phc *phc);
+int mock_phc_index(struct mock_phc *phc);
+
+#else
+
+static inline struct mock_phc *mock_phc_create(struct device *dev)
+{
+ return NULL;
+}
+
+static inline void mock_phc_destroy(struct mock_phc *phc)
+{
+}
+
+static inline int mock_phc_index(struct mock_phc *phc)
+{
+ return -1;
+}
+
+#endif
+
+#endif /* _PTP_MOCK_H_ */
--
2.34.1
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.
...
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).
+ Dan Carpenter
On Thu, Jun 15, 2023 at 01:17:18AM +0300, Vladimir Oltean wrote:
> 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.
Yes, sorry about that.
> > > +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.
Thanks. It's unclear to me if ERR_PTR(0) is actually valid or not.
By itself it does seem harmless to me. But, OTOH, it is sometimes
indicative of some other problem. Fortunately that seems not to
be the case here.
>
> > 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).
Thanks, got it.
On Wed, Jun 14, 2023 at 12:54:37AM +0300, Vladimir Oltean wrote:
> +
> + 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);
> +}
Simon added me to the CC list because this code generates a Smatch
warning about passing zero to ERR_PTR() which is NULL. I have written
a blog about this kind of warning.
https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/
Returning NULL can be perfectly fine, but in this code here it will lead
to a crash. The caller checks for error pointers but after that it
assumes that "phc" is a non-NULL pointer.
The IS_ERR_OR_NULL() check is not correct. It should just be if
if (IS_ERR()).
However, the question is can this driver work without a phc->clock?
Depending on the answer you have to do one of two things.
If yes, then the correct thing is to add NULL checks throughout the
driver to prevent a NULL dereference.
If no, then the correct thing is to ensure that CONFIG_PTP_1588_CLOCK is
enabled using Kconfig. We should never have a driver where we compile
it and then it can't probe.
regards,
dan carpenter