lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 30 Nov 2023 16:55:37 +0000
From: "Michalik, Michal" <michal.michalik@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"vadim.fedorenko@...ux.dev" <vadim.fedorenko@...ux.dev>, "Kubalewski,
 Arkadiusz" <arkadiusz.kubalewski@...el.com>, "jonathan.lemon@...il.com"
	<jonathan.lemon@...il.com>, "pabeni@...hat.com" <pabeni@...hat.com>, poros
	<poros@...hat.com>, "Olech, Milena" <milena.olech@...el.com>, mschmidt
	<mschmidt@...hat.com>, "linux-clk@...r.kernel.org"
	<linux-clk@...r.kernel.org>, "bvanassche@....org" <bvanassche@....org>,
	"kuba@...nel.org" <kuba@...nel.org>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>
Subject: RE: [PATCH RFC net-next v4 1/2] netdevsim: implement DPLL for
 subsystem selftests

On 23 November 2023 1:24 PM CET, Jiri Pirko wrote:
> 
> Thu, Nov 23, 2023 at 11:52:42AM CET, michal.michalik@...el.com wrote:
>>DPLL subsystem integration tests require a module which mimics the
>>behavior of real driver which supports DPLL hardware. To fully test the
>>subsystem the netdevsim is amended with DPLL implementation.
>>
>>Signed-off-by: Michal Michalik <michal.michalik@...el.com>
>>---
>> drivers/net/Kconfig               |   1 +
>> drivers/net/netdevsim/Makefile    |   2 +-
>> drivers/net/netdevsim/dev.c       |  21 +-
>> drivers/net/netdevsim/dpll.c      | 489 ++++++++++++++++++++++++++++++
>> drivers/net/netdevsim/netdev.c    |  10 +
>> drivers/net/netdevsim/netdevsim.h |  44 +++
>> 6 files changed, 565 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/net/netdevsim/dpll.c
>>
>>diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>>index af0da4bb429b..633ec89881ef 100644
>>--- a/drivers/net/Kconfig
>>+++ b/drivers/net/Kconfig
>>@@ -626,6 +626,7 @@ config NETDEVSIM
>> 	depends on PSAMPLE || PSAMPLE=n
>> 	depends on PTP_1588_CLOCK_MOCK || PTP_1588_CLOCK_MOCK=n
>> 	select NET_DEVLINK
>>+	select DPLL
>> 	help
>> 	  This driver is a developer testing tool and software model that can
>> 	  be used to test various control path networking APIs, especially
>>diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
>>index f8de93bc5f5b..f338ffb34f16 100644
>>--- a/drivers/net/netdevsim/Makefile
>>+++ b/drivers/net/netdevsim/Makefile
>>@@ -3,7 +3,7 @@
>> obj-$(CONFIG_NETDEVSIM) += netdevsim.o
>> 
>> netdevsim-objs := \
>>-	netdev.o dev.o ethtool.o fib.o bus.o health.o hwstats.o udp_tunnels.o
>>+	netdev.o dev.o ethtool.o fib.o bus.o health.o hwstats.o udp_tunnels.o dpll.o
>> 
>> ifeq ($(CONFIG_BPF_SYSCALL),y)
>> netdevsim-objs += \
>>diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>>index b4d3b9cde8bd..76da4e8aa9af 100644
>>--- a/drivers/net/netdevsim/dev.c
>>+++ b/drivers/net/netdevsim/dev.c
>>@@ -342,6 +342,17 @@ static int nsim_dev_debugfs_init(struct nsim_dev *nsim_dev)
>> 	debugfs_create_file("max_vfs", 0600, nsim_dev->ddir,
>> 			    nsim_dev, &nsim_dev_max_vfs_fops);
>> 
>>+	debugfs_create_u64("dpll_clock_id", 0600,
> 
> Does not make sense to me to make this writeable. RO please.
> Maybe, this is not needed at all, since the clock id is exposed over
> dpll subsystem. Why do you need it?
> 

I'll make it read only - that is a good catch.

I assume I'm testing mostly the DPLL netlink interface, so I need to know
exactly what I should expect for particular netdevsim device. In other words,
for example - if I was testing if thermometer is giving good temperature
readings I would need a good reference to compare, possibly other thermometer.
It would make not much sense to compare the readings to itself.
Does it make sense?

>>+			   nsim_dev->ddir, &nsim_dev->dpll.dpll_e_pd.clock_id);
>>+	debugfs_create_u32("dpll_e_status", 0600, nsim_dev->ddir,
>>+			   &nsim_dev->dpll.dpll_e_pd.status);
>>+	debugfs_create_u32("dpll_p_status", 0600, nsim_dev->ddir,
>>+			   &nsim_dev->dpll.dpll_p_pd.status);
>>+	debugfs_create_u32("dpll_e_temp", 0600, nsim_dev->ddir,
>>+			   &nsim_dev->dpll.dpll_e_pd.temperature);
>>+	debugfs_create_u32("dpll_p_temp", 0600, nsim_dev->ddir,
>>+			   &nsim_dev->dpll.dpll_p_pd.temperature);
>>+
>> 	nsim_dev->nodes_ddir = debugfs_create_dir("rate_nodes", nsim_dev->ddir);
>> 	if (IS_ERR(nsim_dev->nodes_ddir)) {
>> 		err = PTR_ERR(nsim_dev->nodes_ddir);
>>@@ -1601,14 +1612,21 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>> 	if (err)
>> 		goto err_psample_exit;
>> 
>>-	err = nsim_dev_port_add_all(nsim_dev, nsim_bus_dev->port_count);
>>+	err = nsim_dpll_init_owner(&nsim_dev->dpll, nsim_bus_dev->port_count);
> 
> "owner" Why? Sounds silly.
> 

Removing "owner".

> 
>> 	if (err)
>> 		goto err_hwstats_exit;
>> 
>>+	err = nsim_dev_port_add_all(nsim_dev, nsim_bus_dev->port_count);
>>+	if (err)
>>+		goto err_teardown_dpll;
>>+
>> 	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
>> 	devl_unlock(devlink);
>>+
>> 	return 0;
>> 
>>+err_teardown_dpll:
>>+	nsim_dpll_free_owner(&nsim_dev->dpll);
>> err_hwstats_exit:
>> 	nsim_dev_hwstats_exit(nsim_dev);
>> err_psample_exit:
>>@@ -1656,6 +1674,7 @@ static void nsim_dev_reload_destroy(struct nsim_dev *nsim_dev)
>> 	}
>> 
>> 	nsim_dev_port_del_all(nsim_dev);
>>+	nsim_dpll_free_owner(&nsim_dev->dpll);
>> 	nsim_dev_hwstats_exit(nsim_dev);
>> 	nsim_dev_psample_exit(nsim_dev);
>> 	nsim_dev_health_exit(nsim_dev);
>>diff --git a/drivers/net/netdevsim/dpll.c b/drivers/net/netdevsim/dpll.c
>>new file mode 100644
>>index 000000000000..26a8b0f3be16
>>--- /dev/null
>>+++ b/drivers/net/netdevsim/dpll.c
>>@@ -0,0 +1,489 @@
>>+// SPDX-License-Identifier: GPL-2.0
>>+/*
>>+ * Copyright (c) 2023, Intel Corporation.
>>+ * Author: Michal Michalik <michal.michalik@...el.com>
>>+ */
>>+#include "netdevsim.h"
>>+
>>+#define EEC_DPLL_DEV 0
>>+#define EEC_DPLL_TEMPERATURE 20
>>+#define PPS_DPLL_DEV 1
>>+#define PPS_DPLL_TEMPERATURE 30
>>+
>>+#define PIN_GNSS 0
>>+#define PIN_GNSS_CAPABILITIES DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE
>>+#define PIN_GNSS_PRIORITY 5
>>+
>>+#define PIN_PPS 1
>>+#define PIN_PPS_CAPABILITIES                          \
>>+	(DPLL_PIN_CAPABILITIES_DIRECTION_CAN_CHANGE | \
>>+	 DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE |  \
>>+	 DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE)
>>+#define PIN_PPS_PRIORITY 6
>>+
>>+#define PIN_RCLK 2
>>+#define PIN_RCLK_CAPABILITIES                        \
>>+	(DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE | \
>>+	 DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE)
>>+#define PIN_RCLK_PRIORITY 7
>>+
>>+#define EEC_PINS_NUMBER 3
>>+#define PPS_PINS_NUMBER 2
> 
> Please maintain proper prefix for defines as well.
> 

Ok - makes sense.

> 
> Also, for functions and struct, make sure you have "nsim_dpll_" prefix.
> 

Ok.

> 
>>+
>>+static int nsim_fill_pin_properties(struct dpll_pin_properties *pp,
>>+				    const char *label, enum dpll_pin_type type,
>>+				    unsigned long caps, u32 freq_supp_num,
>>+				    u64 fmin, u64 fmax)
>>+{
>>+	struct dpll_pin_frequency *freq_supp;
>>+
>>+	freq_supp = kzalloc(sizeof(*freq_supp), GFP_KERNEL);
>>+	if (!freq_supp)
>>+		goto freq_supp;
>>+	freq_supp->min = fmin;
>>+	freq_supp->max = fmax;
>>+
>>+	pp->board_label = kasprintf(GFP_KERNEL, "%s_brd", label);
>>+	if (!pp->board_label)
>>+		goto board_label;
>>+	pp->panel_label = kasprintf(GFP_KERNEL, "%s_pnl", label);
>>+	if (!pp->panel_label)
>>+		goto panel_label;
>>+	pp->package_label = kasprintf(GFP_KERNEL, "%s_pcg", label);
>>+	if (!pp->package_label)
>>+		goto package_label;
>>+	pp->freq_supported_num = freq_supp_num;
>>+	pp->freq_supported = freq_supp;
>>+	pp->capabilities = caps;
>>+	pp->type = type;
>>+
>>+	return 0;
>>+
>>+package_label:
>>+	kfree(pp->panel_label);
>>+panel_label:
>>+	kfree(pp->board_label);
>>+board_label:
>>+	kfree(freq_supp);
>>+freq_supp:
>>+	return -ENOMEM;
>>+}
>>+
>>+static void nsim_fill_pin_pd(struct nsim_pin_priv_data *pd, u64 frequency,
>>+			     u32 prio, enum dpll_pin_direction direction)
>>+{
>>+	pd->state_dpll = DPLL_PIN_STATE_DISCONNECTED;
>>+	pd->state_pin = DPLL_PIN_STATE_DISCONNECTED;
>>+	pd->frequency = frequency;
>>+	pd->direction = direction;
>>+	pd->prio = prio;
>>+}
>>+
>>+static int nsim_dds_ops_mode_get(const struct dpll_device *dpll,
>>+				 void *dpll_priv, enum dpll_mode *mode,
>>+				 struct netlink_ext_ack *extack)
>>+{
>>+	struct nsim_dpll_priv_data *pd = dpll_priv;
>>+	*mode = pd->mode;
>>+	return 0;
>>+};
>>+
>>+static bool nsim_dds_ops_mode_supported(const struct dpll_device *dpll,
>>+					void *dpll_priv,
>>+					const enum dpll_mode mode,
>>+					struct netlink_ext_ack *extack)
>>+{
>>+	return true;
> 
> This should return true only for what is returned in mode_get.
> This op is a leftover, I will try to remove it soon.
>

I assumed all modes are supported - leaving it as is till removing by you.
 
>
>>+};
>>+
>>+static int nsim_dds_ops_lock_status_get(const struct dpll_device *dpll,
>>+					void *dpll_priv,
>>+					enum dpll_lock_status *status,
>>+					struct netlink_ext_ack *extack)
>>+{
>>+	struct nsim_dpll_priv_data *pd = dpll_priv;
>>+
>>+	*status = pd->status;
>>+	return 0;
>>+};
>>+
>>+static int nsim_dds_ops_temp_get(const struct dpll_device *dpll,
>>+				 void *dpll_priv, s32 *temp,
>>+				 struct netlink_ext_ack *extack)
>>+{
>>+	struct nsim_dpll_priv_data *pd = dpll_priv;
>>+
>>+	*temp = pd->temperature;
>>+	return 0;
>>+};
>>+
>>+static int nsim_pin_frequency_set(const struct dpll_pin *pin, void *pin_priv,
>>+				  const struct dpll_device *dpll,
>>+				  void *dpll_priv, const u64 frequency,
>>+				  struct netlink_ext_ack *extack)
>>+{
>>+	struct nsim_pin_priv_data *pd = pin_priv;
>>+
>>+	pd->frequency = frequency;
>>+	return 0;
>>+};
>>+
>>+static int nsim_pin_frequency_get(const struct dpll_pin *pin, void *pin_priv,
>>+				  const struct dpll_device *dpll,
>>+				  void *dpll_priv, u64 *frequency,
>>+				  struct netlink_ext_ack *extack)
>>+{
>>+	struct nsim_pin_priv_data *pd = pin_priv;
>>+
>>+	*frequency = pd->frequency;
>>+	return 0;
>>+};
>>+
>>+static int nsim_pin_direction_set(const struct dpll_pin *pin, void *pin_priv,
>>+				  const struct dpll_device *dpll,
>>+				  void *dpll_priv,
>>+				  const enum dpll_pin_direction direction,
>>+				  struct netlink_ext_ack *extack)
>>+{
>>+	struct nsim_pin_priv_data *pd = pin_priv;
>>+
>>+	pd->direction = direction;
>>+	return 0;
>>+};
>>+
>>+static int nsim_pin_direction_get(const struct dpll_pin *pin, void *pin_priv,
>>+				  const struct dpll_device *dpll,
>>+				  void *dpll_priv,
>>+				  enum dpll_pin_direction *direction,
>>+				  struct netlink_ext_ack *extack)
>>+{
>>+	struct nsim_pin_priv_data *pd = pin_priv;
>>+
>>+	*direction = pd->direction;
>>+	return 0;
>>+};
>>+
>>+static int nsim_pin_state_on_pin_get(const struct dpll_pin *pin, void *pin_priv,
>>+				     const struct dpll_pin *parent_pin,
>>+				     void *parent_priv,
>>+				     enum dpll_pin_state *state,
>>+				     struct netlink_ext_ack *extack)
>>+{
>>+	struct nsim_pin_priv_data *pd = pin_priv;
>>+
>>+	*state = pd->state_pin;
>>+	return 0;
>>+};
>>+
>>+static int nsim_pin_state_on_dpll_get(const struct dpll_pin *pin,
>>+				      void *pin_priv,
>>+				      const struct dpll_device *dpll,
>>+				      void *dpll_priv,
>>+				      enum dpll_pin_state *state,
>>+				      struct netlink_ext_ack *extack)
>>+{
>>+	struct nsim_pin_priv_data *pd = pin_priv;
>>+
>>+	*state = pd->state_dpll;
>>+	return 0;
>>+};
>>+
>>+static int nsim_pin_state_on_pin_set(const struct dpll_pin *pin, void *pin_priv,
>>+				     const struct dpll_pin *parent_pin,
>>+				     void *parent_priv,
>>+				     const enum dpll_pin_state state,
>>+				     struct netlink_ext_ack *extack)
>>+{
>>+	struct nsim_pin_priv_data *pd = pin_priv;
>>+
>>+	pd->state_pin = state;
>>+	return 0;
>>+};
>>+
>>+static int nsim_pin_state_on_dpll_set(const struct dpll_pin *pin,
>>+				      void *pin_priv,
>>+				      const struct dpll_device *dpll,
>>+				      void *dpll_priv,
>>+				      const enum dpll_pin_state state,
>>+				      struct netlink_ext_ack *extack)
>>+{
>>+	struct nsim_pin_priv_data *pd = pin_priv;
>>+
>>+	pd->state_dpll = state;
>>+	return 0;
>>+};
>>+
>>+static int nsim_pin_prio_get(const struct dpll_pin *pin, void *pin_priv,
>>+			     const struct dpll_device *dpll, void *dpll_priv,
>>+			     u32 *prio, struct netlink_ext_ack *extack)
>>+{
>>+	struct nsim_pin_priv_data *pd = pin_priv;
>>+
>>+	*prio = pd->prio;
>>+	return 0;
>>+};
>>+
>>+static int nsim_pin_prio_set(const struct dpll_pin *pin, void *pin_priv,
>>+			     const struct dpll_device *dpll, void *dpll_priv,
>>+			     const u32 prio, struct netlink_ext_ack *extack)
>>+{
>>+	struct nsim_pin_priv_data *pd = pin_priv;
>>+
>>+	pd->prio = prio;
>>+	return 0;
>>+};
>>+
>>+static void nsim_free_pin_properties(struct dpll_pin_properties *pp)
>>+{
>>+	kfree(pp->board_label);
>>+	kfree(pp->panel_label);
>>+	kfree(pp->package_label);
>>+	kfree(pp->freq_supported);
>>+}
>>+
>>+static struct dpll_device_ops nsim_dds_ops = {
> 
> const
> 

Adding, thanks.

> 
>>+	.mode_get = nsim_dds_ops_mode_get,
>>+	.mode_supported = nsim_dds_ops_mode_supported,
>>+	.lock_status_get = nsim_dds_ops_lock_status_get,
>>+	.temp_get = nsim_dds_ops_temp_get,
>>+};
>>+
>>+static struct dpll_pin_ops nsim_pin_ops = {
> 
> const
> 

Adding, thanks.

> 
>>+	.frequency_set = nsim_pin_frequency_set,
>>+	.frequency_get = nsim_pin_frequency_get,
>>+	.direction_set = nsim_pin_direction_set,
>>+	.direction_get = nsim_pin_direction_get,
>>+	.state_on_pin_get = nsim_pin_state_on_pin_get,
>>+	.state_on_dpll_get = nsim_pin_state_on_dpll_get,
>>+	.state_on_pin_set = nsim_pin_state_on_pin_set,
>>+	.state_on_dpll_set = nsim_pin_state_on_dpll_set,
>>+	.prio_get = nsim_pin_prio_get,
>>+	.prio_set = nsim_pin_prio_set,
>>+};
>>+
>>+int nsim_dpll_init_owner(struct nsim_dpll *dpll, unsigned int ports_count)
>>+{
>>+	u64 clock_id;
>>+	int err;
>>+
>>+	get_random_bytes(&clock_id, sizeof(clock_id));
>>+
>>+	/* Create EEC DPLL */
>>+	dpll->dpll_e = dpll_device_get(clock_id, EEC_DPLL_DEV, THIS_MODULE);
>>+	if (IS_ERR(dpll->dpll_e))
>>+		return -EFAULT;
>>+
>>+	dpll->dpll_e_pd.temperature = EEC_DPLL_TEMPERATURE;
>>+	dpll->dpll_e_pd.mode = DPLL_MODE_AUTOMATIC;
>>+	dpll->dpll_e_pd.clock_id = clock_id;
>>+	dpll->dpll_e_pd.status = DPLL_LOCK_STATUS_UNLOCKED;
>>+
>>+	err = dpll_device_register(dpll->dpll_e, DPLL_TYPE_EEC, &nsim_dds_ops,
>>+				   &dpll->dpll_e_pd);
>>+	if (err)
>>+		goto e_reg;
>>+
>>+	/* Create PPS DPLL */
>>+	dpll->dpll_p = dpll_device_get(clock_id, PPS_DPLL_DEV, THIS_MODULE);
>>+	if (IS_ERR(dpll->dpll_p))
>>+		goto dpll_p;
>>+
>>+	dpll->dpll_p_pd.temperature = PPS_DPLL_TEMPERATURE;
>>+	dpll->dpll_p_pd.mode = DPLL_MODE_MANUAL;
>>+	dpll->dpll_p_pd.clock_id = clock_id;
>>+	dpll->dpll_p_pd.status = DPLL_LOCK_STATUS_UNLOCKED;
>>+
>>+	err = dpll_device_register(dpll->dpll_p, DPLL_TYPE_PPS, &nsim_dds_ops,
>>+				   &dpll->dpll_p_pd);
>>+	if (err)
>>+		goto p_reg;
>>+
>>+	/* Create first pin (GNSS) */
>>+	err = nsim_fill_pin_properties(&dpll->pp_gnss, "GNSS",
> 
> It's of type GNSS. No need to provide redundant label. Please remove.
>

To be frank, I don't see anything bad with using the label. Removed package and
panel labels, though. Left only board label for testing purposes.

> 
>>+				       DPLL_PIN_TYPE_GNSS,
>>+				       PIN_GNSS_CAPABILITIES, 1,
>>+				       DPLL_PIN_FREQUENCY_1_HZ,
>>+				       DPLL_PIN_FREQUENCY_1_HZ);
>>+	if (err)
>>+		goto pp_gnss;
>>+	dpll->p_gnss =
>>+		dpll_pin_get(clock_id, PIN_GNSS, THIS_MODULE, &dpll->pp_gnss);
>>+	if (IS_ERR(dpll->p_gnss))
>>+		goto p_gnss;
>>+	nsim_fill_pin_pd(&dpll->p_gnss_pd, DPLL_PIN_FREQUENCY_1_HZ,
>>+			 PIN_GNSS_PRIORITY, DPLL_PIN_DIRECTION_INPUT);
>>+	err = dpll_pin_register(dpll->dpll_e, dpll->p_gnss, &nsim_pin_ops,
>>+				&dpll->p_gnss_pd);
>>+	if (err)
>>+		goto e_gnss_reg;
>>+
>>+	/* Create second pin (PPS) */
>>+	err = nsim_fill_pin_properties(&dpll->pp_pps, "PPS", DPLL_PIN_TYPE_EXT,
> 
> Label "pps"? That does not sound correct. Please fix.
>

Why do you think it does not sound correct? For me it's perfectly fine. External
pulse per second pin set with only DPLL_PIN_FREQUENCY_1_HZ.

> 
>>+				       PIN_PPS_CAPABILITIES, 1,
>>+				       DPLL_PIN_FREQUENCY_1_HZ,
>>+				       DPLL_PIN_FREQUENCY_1_HZ);
>>+	if (err)
>>+		goto pp_pps;
>>+	dpll->p_pps =
>>+		dpll_pin_get(clock_id, PIN_PPS, THIS_MODULE, &dpll->pp_pps);
>>+	if (IS_ERR(dpll->p_pps)) {
>>+		err = -EFAULT;
>>+		goto p_pps;
>>+	}
>>+	nsim_fill_pin_pd(&dpll->p_pps_pd, DPLL_PIN_FREQUENCY_1_HZ,
>>+			 PIN_PPS_PRIORITY, DPLL_PIN_DIRECTION_INPUT);
>>+	err = dpll_pin_register(dpll->dpll_e, dpll->p_pps, &nsim_pin_ops,
>>+				&dpll->p_pps_pd);
>>+	if (err)
>>+		goto e_pps_reg;
>>+	err = dpll_pin_register(dpll->dpll_p, dpll->p_pps, &nsim_pin_ops,
>>+				&dpll->p_pps_pd);
>>+	if (err)
>>+		goto p_pps_reg;
>>+
>>+	dpll->pp_rclk =
>>+		kcalloc(ports_count, sizeof(*dpll->pp_rclk), GFP_KERNEL);
>>+	dpll->p_rclk = kcalloc(ports_count, sizeof(*dpll->p_rclk), GFP_KERNEL);
>>+	dpll->p_rclk_pd =
>>+		kcalloc(ports_count, sizeof(*dpll->p_rclk_pd), GFP_KERNEL);
>>+
>>+	return 0;
>>+
>>+p_pps_reg:
>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_pps, &nsim_pin_ops,
>>+			    &dpll->p_pps_pd);
>>+e_pps_reg:
>>+	dpll_pin_put(dpll->p_pps);
>>+p_pps:
>>+	nsim_free_pin_properties(&dpll->pp_pps);
>>+pp_pps:
>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_gnss, &nsim_pin_ops,
>>+			    &dpll->p_gnss_pd);
>>+e_gnss_reg:
>>+	dpll_pin_put(dpll->p_gnss);
>>+p_gnss:
>>+	nsim_free_pin_properties(&dpll->pp_gnss);
>>+pp_gnss:
>>+	dpll_device_unregister(dpll->dpll_p, &nsim_dds_ops, &dpll->dpll_p_pd);
>>+p_reg:
>>+	dpll_device_put(dpll->dpll_p);
>>+dpll_p:
>>+	dpll_device_unregister(dpll->dpll_e, &nsim_dds_ops, &dpll->dpll_e_pd);
>>+e_reg:
> 
> Please have error labels named properly. See:
> git grep goto drivers/net/netdevsim/
> 

Got it - will try to align to it more.

> 
>>+	dpll_device_put(dpll->dpll_e);
>>+	return err;
>>+}
>>+
>>+void nsim_dpll_free_owner(struct nsim_dpll *dpll)
>>+{
>>+	/* Free GNSS pin */
>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_gnss, &nsim_pin_ops,
>>+			    &dpll->p_gnss_pd);
>>+	dpll_pin_put(dpll->p_gnss);
>>+	nsim_free_pin_properties(&dpll->pp_gnss);
>>+
>>+	/* Free PPS pin */
>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_pps, &nsim_pin_ops,
>>+			    &dpll->p_pps_pd);
>>+	dpll_pin_unregister(dpll->dpll_p, dpll->p_pps, &nsim_pin_ops,
>>+			    &dpll->p_pps_pd);
>>+	dpll_pin_put(dpll->p_pps);
>>+	nsim_free_pin_properties(&dpll->pp_pps);
>>+
>>+	/* Free DPLL EEC */
>>+	dpll_device_unregister(dpll->dpll_e, &nsim_dds_ops, &dpll->dpll_e_pd);
>>+	dpll_device_put(dpll->dpll_e);
>>+
>>+	/* Free DPLL PPS */
>>+	dpll_device_unregister(dpll->dpll_p, &nsim_dds_ops, &dpll->dpll_p_pd);
>>+	dpll_device_put(dpll->dpll_p);
>>+
>>+	kfree(dpll->pp_rclk);
>>+	kfree(dpll->p_rclk);
>>+	kfree(dpll->p_rclk_pd);
>>+}
>>+
>>+int nsim_rclk_init(struct netdevsim *ns)
>>+{
>>+	struct nsim_dpll *dpll;
>>+	unsigned int index;
>>+	char *name;
>>+	int err;
>>+
>>+	index = ns->nsim_dev_port->port_index;
>>+	dpll = &ns->nsim_dev->dpll;
>>+	err = -ENOMEM;
>>+
>>+	name = kasprintf(GFP_KERNEL, "RCLK_%i", index);
> 
> Not good for anything. It is not really a label. The link from netdevice
> to this pin. Please remove this label entirely.
>

Not true, my intention is to test the DPLL netlink interface including filtering
by label. Therefore I need it.

> 
>>+	if (!name)
>>+		goto err;
>>+
>>+	/* Get EEC DPLL */
>>+	if (IS_ERR(dpll->dpll_e))
>>+		goto dpll;
>>+
>>+	/* Get PPS DPLL */
>>+	if (IS_ERR(dpll->dpll_p))
>>+		goto dpll;
>>+
>>+	/* Create Recovered clock pin (RCLK) */
>>+	nsim_fill_pin_properties(&dpll->pp_rclk[index], name,
>>+				 DPLL_PIN_TYPE_SYNCE_ETH_PORT,
>>+				 PIN_RCLK_CAPABILITIES, 1, 1e6, 125e6);
>>+	dpll->p_rclk[index] = dpll_pin_get(dpll->dpll_e_pd.clock_id,
>>+					   PIN_RCLK + index, THIS_MODULE,
>>+					   &dpll->pp_rclk[index]);
>>+	if (IS_ERR(dpll->p_rclk[index]))
>>+		goto p_rclk;
>>+	nsim_fill_pin_pd(&dpll->p_rclk_pd[index], DPLL_PIN_FREQUENCY_10_MHZ,
>>+			 PIN_RCLK_PRIORITY, DPLL_PIN_DIRECTION_INPUT);
>>+	err = dpll_pin_register(dpll->dpll_e, dpll->p_rclk[index],
>>+				&nsim_pin_ops, &dpll->p_rclk_pd[index]);
>>+	if (err)
>>+		goto dpll_e_reg;
>>+	err = dpll_pin_register(dpll->dpll_p, dpll->p_rclk[index],
>>+				&nsim_pin_ops, &dpll->p_rclk_pd[index]);
>>+	if (err)
>>+		goto dpll_p_reg;
>>+
>>+	netdev_dpll_pin_set(ns->netdev, dpll->p_rclk[index]);
>>+
>>+	kfree(name);
>>+	return 0;
>>+
>>+dpll_p_reg:
>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_rclk[index], &nsim_pin_ops,
>>+			    &dpll->p_rclk_pd[index]);
>>+dpll_e_reg:
>>+	dpll_pin_put(dpll->p_rclk[index]);
>>+p_rclk:
>>+	nsim_free_pin_properties(&dpll->pp_rclk[index]);
>>+dpll:
>>+	kfree(name);
>>+err:
>>+	return err;
>>+}
>>+
>>+void nsim_rclk_free(struct netdevsim *ns)
>>+{
>>+	struct nsim_dpll *dpll;
>>+	unsigned int index;
>>+
>>+	index = ns->nsim_dev_port->port_index;
>>+	dpll = &ns->nsim_dev->dpll;
>>+
>>+	if (IS_ERR(dpll->dpll_e))
>>+		return;
>>+
>>+	if (IS_ERR(dpll->dpll_p))
>>+		return;
>>+
>>+	/* Free RCLK pin */
>>+	netdev_dpll_pin_clear(ns->netdev);
>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_rclk[index], &nsim_pin_ops,
>>+			    &dpll->p_rclk_pd[index]);
>>+	dpll_pin_unregister(dpll->dpll_p, dpll->p_rclk[index], &nsim_pin_ops,
>>+			    &dpll->p_rclk_pd[index]);
>>+	dpll_pin_put(dpll->p_rclk[index]);
>>+	nsim_free_pin_properties(&dpll->pp_rclk[index]);
>>+}
>>diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>>index aecaf5f44374..3c604d8608a3 100644
>>--- a/drivers/net/netdevsim/netdev.c
>>+++ b/drivers/net/netdevsim/netdev.c
>>@@ -344,8 +344,15 @@ static int nsim_init_netdevsim(struct netdevsim *ns)
>> 	if (err)
>> 		goto err_ipsec_teardown;
>> 	rtnl_unlock();
>>+
>>+	err = nsim_rclk_init(ns);
>>+	if (err)
>>+		goto err_netdevice_unregister;
>>+
>> 	return 0;
>> 
>>+err_netdevice_unregister:
>>+	unregister_netdevice(ns->netdev);
>> err_ipsec_teardown:
>> 	nsim_ipsec_teardown(ns);
>> 	nsim_macsec_teardown(ns);
>>@@ -419,6 +426,9 @@ void nsim_destroy(struct netdevsim *ns)
>> 	if (nsim_dev_port_is_pf(ns->nsim_dev_port))
>> 		nsim_udp_tunnels_info_destroy(dev);
>> 	mock_phc_destroy(ns->phc);
>>+
>>+	nsim_rclk_free(ns);
>>+
>> 	free_netdev(dev);
>> }
>> 
>>diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>>index 028c825b86db..bd798a4cf49f 100644
>>--- a/drivers/net/netdevsim/netdevsim.h
>>+++ b/drivers/net/netdevsim/netdevsim.h
>>@@ -25,6 +25,8 @@
>> #include <net/udp_tunnel.h>
>> #include <net/xdp.h>
>> #include <net/macsec.h>
>>+#include <linux/dpll.h>
>>+#include <linux/random.h>
>> 
>> #define DRV_NAME	"netdevsim"
>> 
>>@@ -90,6 +92,42 @@ struct nsim_ethtool {
>> 	struct ethtool_fecparam fec;
>> };
>> 
>>+struct nsim_dpll_priv_data {
>>+	enum dpll_mode mode;
>>+	int temperature;
>>+	u64 clock_id;
>>+	enum dpll_lock_status status;
>>+};
>>+
>>+struct nsim_pin_priv_data {
> 
> You are missing "dpll" here. Also the "priv_data" suffix looks silly.
> Please just have:
> struct nsim_dpll
> struct nsim_dpll_pin
> struct nsim_dpll_device
> 

Seems logical, simplifying.

> 
>>+	u64 frequency;
>>+	enum dpll_pin_direction direction;
>>+	enum dpll_pin_state state_pin;
>>+	enum dpll_pin_state state_dpll;
>>+	u32 prio;
>>+};
>>+
>>+struct nsim_dpll {
>>+	bool owner;
> 
> Never used.
> 

Removing.

> 
>>+
>>+	struct dpll_device *dpll_e;
>>+	struct nsim_dpll_priv_data dpll_e_pd;
>>+	struct dpll_device *dpll_p;
>>+	struct nsim_dpll_priv_data dpll_p_pd;
>>+
>>+	struct dpll_pin_properties pp_gnss;
>>+	struct dpll_pin *p_gnss;
>>+	struct nsim_pin_priv_data p_gnss_pd;
>>+
>>+	struct dpll_pin_properties pp_pps;
>>+	struct dpll_pin *p_pps;
>>+	struct nsim_pin_priv_data p_pps_pd;
>>+
>>+	struct dpll_pin_properties *pp_rclk;
>>+	struct dpll_pin **p_rclk;
>>+	struct nsim_pin_priv_data *p_rclk_pd;
>>+};
>>+
>> struct netdevsim {
>> 	struct net_device *netdev;
>> 	struct nsim_dev *nsim_dev;
>>@@ -323,6 +361,7 @@ struct nsim_dev {
>> 	} udp_ports;
>> 	struct nsim_dev_psample *psample;
>> 	u16 esw_mode;
>>+	struct nsim_dpll dpll;
>> };
>> 
>> static inline bool nsim_esw_mode_is_legacy(struct nsim_dev *nsim_dev)
>>@@ -415,5 +454,10 @@ struct nsim_bus_dev {
>> 	bool init;
>> };
>> 
>>+int nsim_dpll_init_owner(struct nsim_dpll *dpll, unsigned int ports_count);
>>+void nsim_dpll_free_owner(struct nsim_dpll *dpll);
>>+int nsim_rclk_init(struct netdevsim *ns);
>>+void nsim_rclk_free(struct netdevsim *ns);
> 
> _dpll_, please make the naming consitent for all dpll
> function/struct/define names
> 

Added _dpll_ everywhere (I hope).

> 
>>+
>> int nsim_bus_init(void);
>> void nsim_bus_exit(void);
>>-- 
>>2.39.3
>>
>>

Powered by blists - more mailing lists