Message-ID: <slvow56opklcc2hwz4vzq4t7olazddmvn4lxhoveb43f6mz4p2@6vq3iz7kiuce>
Date: Thu, 17 Apr 2025 11:56:56 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
Cc: "donald.hunter@...il.com" <donald.hunter@...il.com>, 
	"kuba@...nel.org" <kuba@...nel.org>, "davem@...emloft.net" <davem@...emloft.net>, 
	"Dumazet, Eric" <edumazet@...gle.com>, "pabeni@...hat.com" <pabeni@...hat.com>, 
	"horms@...nel.org" <horms@...nel.org>, "vadim.fedorenko@...ux.dev" <vadim.fedorenko@...ux.dev>, 
	"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>, 
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "saeedm@...dia.com" <saeedm@...dia.com>, 
	"leon@...nel.org" <leon@...nel.org>, "tariqt@...dia.com" <tariqt@...dia.com>, 
	"jonathan.lemon@...il.com" <jonathan.lemon@...il.com>, "richardcochran@...il.com" <richardcochran@...il.com>, 
	"Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>, "Olech, Milena" <milena.olech@...el.com>, 
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>, "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>
Subject: Re: [PATCH net-next v2 1/4] dpll: use struct dpll_device_info for
 dpll registration

Thu, Apr 17, 2025 at 11:33:13AM +0200, arkadiusz.kubalewski@...el.com wrote:
>>From: Jiri Pirko <jiri@...nulli.us>
>>Sent: Wednesday, April 16, 2025 2:13 PM
>>
>>Tue, Apr 15, 2025 at 08:15:40PM +0200, arkadiusz.kubalewski@...el.com
>>wrote:
>>>Instead of passing list of properties as arguments to
>>>dpll_device_register(..) use a dedicated struct.
>>>
>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>>>---
>>>v2:
>>>- new commit
>>>---
>>> drivers/dpll/dpll_core.c                      | 34 ++++++++++++-------
>>> drivers/dpll/dpll_core.h                      |  2 +-
>>> drivers/dpll/dpll_netlink.c                   |  7 ++--
>>> drivers/net/ethernet/intel/ice/ice_dpll.c     | 16 +++++----
>>> drivers/net/ethernet/intel/ice/ice_dpll.h     |  1 +
>>> .../net/ethernet/mellanox/mlx5/core/dpll.c    | 10 +++---
>>> drivers/ptp/ptp_ocp.c                         |  7 ++--
>>> include/linux/dpll.h                          | 11 ++++--
>>> 8 files changed, 57 insertions(+), 31 deletions(-)
>>>
>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>index 20bdc52f63a5..af9cda45a89c 100644
>>>--- a/drivers/dpll/dpll_core.c
>>>+++ b/drivers/dpll/dpll_core.c
>>>@@ -34,7 +34,7 @@ static u32 dpll_pin_xa_id;
>>>
>>> struct dpll_device_registration {
>>> 	struct list_head list;
>>>-	const struct dpll_device_ops *ops;
>>>+	const struct dpll_device_info *info;
>>> 	void *priv;
>>> };
>>>
>>>@@ -327,12 +327,12 @@ EXPORT_SYMBOL_GPL(dpll_device_put);
>>>
>>> static struct dpll_device_registration *
>>> dpll_device_registration_find(struct dpll_device *dpll,
>>>-			      const struct dpll_device_ops *ops, void *priv)
>>>+			      const struct dpll_device_info *info, void *priv)
>>> {
>>> 	struct dpll_device_registration *reg;
>>>
>>> 	list_for_each_entry(reg, &dpll->registration_list, list) {
>>>-		if (reg->ops == ops && reg->priv == priv)
>>>+		if (reg->info == info && reg->priv == priv)
>>> 			return reg;
>>> 	}
>>> 	return NULL;
>>>@@ -341,8 +341,7 @@ dpll_device_registration_find(struct dpll_device
>>>*dpll,
>>> /**
>>>  * dpll_device_register - register the dpll device in the subsystem
>>>  * @dpll: pointer to a dpll
>>>- * @type: type of a dpll
>>>- * @ops: ops for a dpll device
>>>+ * @info: dpll device information and operations from registerer
>>>  * @priv: pointer to private information of owner
>>>  *
>>>  * Make dpll device available for user space.
>>>@@ -352,11 +351,13 @@ dpll_device_registration_find(struct dpll_device
>>>*dpll,
>>>  * * 0 on success
>>>  * * negative - error value
>>>  */
>>>-int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
>>>-			 const struct dpll_device_ops *ops, void *priv)
>>>+int dpll_device_register(struct dpll_device *dpll,
>>>+			 const struct dpll_device_info *info, void *priv)
>>
>>I don't like this. If you need some capabilities value, put it into ops
>>struct.
>>
>
>Hmm, this would seems strange, the _ops indicates operations, would
>have to rename the struct..

I don't think so. Happens all the time in kernel for ops to contain
other things.


>
>In theory I could decide on capabilities per ops provided from driver..
>i.e. If phase_input_monitor_feature_set()/phase_input_feature_get() are
>present then capability phase_input_monitor is provided..
>Makes sense?

Yes, that is more or less what I suggested in reply to the other patch
in this set.


>
>>
>>> {
>>>+	const struct dpll_device_ops *ops = info->ops;
>>> 	struct dpll_device_registration *reg;
>>> 	bool first_registration = false;
>>>+	enum dpll_type type = info->type;
>>>
>>> 	if (WARN_ON(!ops))
>>> 		return -EINVAL;
>>>@@ -368,7 +369,7 @@ int dpll_device_register(struct dpll_device *dpll,
>>>enum dpll_type type,
>>> 		return -EINVAL;
>>>
>>> 	mutex_lock(&dpll_lock);
>>>-	reg = dpll_device_registration_find(dpll, ops, priv);
>>>+	reg = dpll_device_registration_find(dpll, info, priv);
>>> 	if (reg) {
>>> 		mutex_unlock(&dpll_lock);
>>> 		return -EEXIST;
>>>@@ -379,9 +380,8 @@ int dpll_device_register(struct dpll_device *dpll,
>>>enum dpll_type type,
>>> 		mutex_unlock(&dpll_lock);
>>> 		return -ENOMEM;
>>> 	}
>>>-	reg->ops = ops;
>>>+	reg->info = info;
>>> 	reg->priv = priv;
>>>-	dpll->type = type;
>>> 	first_registration = list_empty(&dpll->registration_list);
>>> 	list_add_tail(&reg->list, &dpll->registration_list);
>>> 	if (!first_registration) {
>>>@@ -408,14 +408,14 @@ EXPORT_SYMBOL_GPL(dpll_device_register);
>>>  * Context: Acquires a lock (dpll_lock)
>>>  */
>>> void dpll_device_unregister(struct dpll_device *dpll,
>>>-			    const struct dpll_device_ops *ops, void *priv)
>>>+			    const struct dpll_device_info *info, void *priv)
>>> {
>>> 	struct dpll_device_registration *reg;
>>>
>>> 	mutex_lock(&dpll_lock);
>>> 	ASSERT_DPLL_REGISTERED(dpll);
>>> 	dpll_device_delete_ntf(dpll);
>>>-	reg = dpll_device_registration_find(dpll, ops, priv);
>>>+	reg = dpll_device_registration_find(dpll, info, priv);
>>> 	if (WARN_ON(!reg)) {
>>> 		mutex_unlock(&dpll_lock);
>>> 		return;
>>>@@ -807,7 +807,15 @@ const struct dpll_device_ops *dpll_device_ops(struct
>>>dpll_device *dpll)
>>> 	struct dpll_device_registration *reg;
>>>
>>> 	reg = dpll_device_registration_first(dpll);
>>>-	return reg->ops;
>>>+	return reg->info->ops;
>>>+}
>>>+
>>>+const struct dpll_device_info *dpll_device_info(struct dpll_device *dpll)
>>
>>Makes me wonder what you would need this for. I guess "nothing"?
>>
>
>Now using it to get info struct from dpll.. if struct is removed then yeah.
>
>Thank you!
>Arkadiusz
>
>>
>>>+{
>>>+	struct dpll_device_registration *reg;
>>>+
>>>+	reg = dpll_device_registration_first(dpll);
>>>+	return reg->info;
>>> }
>>>
>>> static struct dpll_pin_registration *
>>>diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
>>>index 2b6d8ef1cdf3..baeb10d7dc1e 100644
>>>--- a/drivers/dpll/dpll_core.h
>>>+++ b/drivers/dpll/dpll_core.h
>>>@@ -30,7 +30,6 @@ struct dpll_device {
>>> 	u32 device_idx;
>>> 	u64 clock_id;
>>> 	struct module *module;
>>>-	enum dpll_type type;
>>> 	struct xarray pin_refs;
>>> 	refcount_t refcount;
>>> 	struct list_head registration_list;
>>>@@ -84,6 +83,7 @@ void *dpll_pin_on_pin_priv(struct dpll_pin *parent,
>>>struct dpll_pin *pin);
>>> const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll);
>>> struct dpll_device *dpll_device_get_by_id(int id);
>>> const struct dpll_pin_ops *dpll_pin_ops(struct dpll_pin_ref *ref);
>>>+const struct dpll_device_info *dpll_device_info(struct dpll_device
>>>*dpll);
>>> struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs);
>>> extern struct xarray dpll_device_xa;
>>> extern struct xarray dpll_pin_xa;
>>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>>index c130f87147fa..2de9ec08d551 100644
>>>--- a/drivers/dpll/dpll_netlink.c
>>>+++ b/drivers/dpll/dpll_netlink.c
>>>@@ -564,6 +564,7 @@ static int
>>> dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
>>> 		    struct netlink_ext_ack *extack)
>>> {
>>>+	const struct dpll_device_info *info = dpll_device_info(dpll);
>>> 	int ret;
>>>
>>> 	ret = dpll_msg_add_dev_handle(msg, dpll);
>>>@@ -589,7 +590,7 @@ dpll_device_get_one(struct dpll_device *dpll, struct
>>>sk_buff *msg,
>>> 	ret = dpll_msg_add_mode_supported(msg, dpll, extack);
>>> 	if (ret)
>>> 		return ret;
>>>-	if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
>>>+	if (nla_put_u32(msg, DPLL_A_TYPE, info->type))
>>> 		return -EMSGSIZE;
>>>
>>> 	return 0;
>>>@@ -1415,11 +1416,13 @@ dpll_device_find(u64 clock_id, struct nlattr
>>>*mod_name_attr,
>>> 	unsigned long i;
>>>
>>> 	xa_for_each_marked(&dpll_device_xa, i, dpll, DPLL_REGISTERED) {
>>>+		const struct dpll_device_info *info = dpll_device_info(dpll);
>>>+
>>> 		cid_match = clock_id ? dpll->clock_id == clock_id : true;
>>> 		mod_match = mod_name_attr ? (module_name(dpll->module) ?
>>> 			!nla_strcmp(mod_name_attr,
>>> 				    module_name(dpll->module)) : false) : true;
>>>-		type_match = type ? dpll->type == type : true;
>>>+		type_match = type ? info->type == type : true;
>>> 		if (cid_match && mod_match && type_match) {
>>> 			if (dpll_match) {
>>> 				NL_SET_ERR_MSG(extack, "multiple matches");
>>>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c
>>>b/drivers/net/ethernet/intel/ice/ice_dpll.c
>>>index bce3ad6ca2a6..0f7440a889ac 100644
>>>--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
>>>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
>>>@@ -1977,7 +1977,7 @@ static void
>>> ice_dpll_deinit_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu)
>>> {
>>> 	if (cgu)
>>>-		dpll_device_unregister(d->dpll, &ice_dpll_ops, d);
>>>+		dpll_device_unregister(d->dpll, &d->info, d);
>>> 	dpll_device_put(d->dpll);
>>> }
>>>
>>>@@ -1996,8 +1996,7 @@ ice_dpll_deinit_dpll(struct ice_pf *pf, struct
>>>ice_dpll *d, bool cgu)
>>>  * * negative - initialization failure reason
>>>  */
>>> static int
>>>-ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu,
>>>-		   enum dpll_type type)
>>>+ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu)
>>> {
>>> 	u64 clock_id = pf->dplls.clock_id;
>>> 	int ret;
>>>@@ -2012,7 +2011,7 @@ ice_dpll_init_dpll(struct ice_pf *pf, struct
>>>ice_dpll *d, bool cgu,
>>> 	d->pf = pf;
>>> 	if (cgu) {
>>> 		ice_dpll_update_state(pf, d, true);
>>>-		ret = dpll_device_register(d->dpll, type, &ice_dpll_ops, d);
>>>+		ret = dpll_device_register(d->dpll, &d->info, d);
>>> 		if (ret) {
>>> 			dpll_device_put(d->dpll);
>>> 			return ret;
>>>@@ -2363,7 +2362,12 @@ static int ice_dpll_init_info(struct ice_pf *pf,
>>>bool cgu)
>>> 	if (ret)
>>> 		return ret;
>>> 	de->mode = DPLL_MODE_AUTOMATIC;
>>>+	de->info.type = DPLL_TYPE_EEC;
>>>+	de->info.ops = &ice_dpll_ops;
>>>+
>>> 	dp->mode = DPLL_MODE_AUTOMATIC;
>>>+	dp->info.type = DPLL_TYPE_PPS;
>>>+	dp->info.ops = &ice_dpll_ops;
>>>
>>> 	dev_dbg(ice_pf_to_dev(pf),
>>> 		"%s - success, inputs:%u, outputs:%u rclk-parents:%u\n",
>>>@@ -2426,10 +2430,10 @@ void ice_dpll_init(struct ice_pf *pf)
>>> 	err = ice_dpll_init_info(pf, cgu);
>>> 	if (err)
>>> 		goto err_exit;
>>>-	err = ice_dpll_init_dpll(pf, &pf->dplls.eec, cgu, DPLL_TYPE_EEC);
>>>+	err = ice_dpll_init_dpll(pf, &pf->dplls.eec, cgu);
>>> 	if (err)
>>> 		goto deinit_info;
>>>-	err = ice_dpll_init_dpll(pf, &pf->dplls.pps, cgu, DPLL_TYPE_PPS);
>>>+	err = ice_dpll_init_dpll(pf, &pf->dplls.pps, cgu);
>>> 	if (err)
>>> 		goto deinit_eec;
>>> 	err = ice_dpll_init_pins(pf, cgu);
>>>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h
>>>b/drivers/net/ethernet/intel/ice/ice_dpll.h
>>>index c320f1bf7d6d..9db7463e293a 100644
>>>--- a/drivers/net/ethernet/intel/ice/ice_dpll.h
>>>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
>>>@@ -66,6 +66,7 @@ struct ice_dpll {
>>> 	enum dpll_mode mode;
>>> 	struct dpll_pin *active_input;
>>> 	struct dpll_pin *prev_input;
>>>+	struct dpll_device_info info;
>>> };
>>>
>>> /** ice_dplls - store info required for CCU (clock controlling unit)
>>>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>>>b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>>>index 1e5522a19483..f722b1de0754 100644
>>>--- a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>>>+++ b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>>>@@ -20,6 +20,7 @@ struct mlx5_dpll {
>>> 	} last;
>>> 	struct notifier_block mdev_nb;
>>> 	struct net_device *tracking_netdev;
>>>+	struct dpll_device_info info;
>>> };
>>>
>>> static int mlx5_dpll_clock_id_get(struct mlx5_core_dev *mdev, u64
>>>*clock_id)
>>>@@ -444,8 +445,9 @@ static int mlx5_dpll_probe(struct auxiliary_device
>>>*adev,
>>> 		goto err_free_mdpll;
>>> 	}
>>>
>>>-	err = dpll_device_register(mdpll->dpll, DPLL_TYPE_EEC,
>>>-				   &mlx5_dpll_device_ops, mdpll);
>>>+	mdpll->info.type = DPLL_TYPE_EEC;
>>>+	mdpll->info.ops = &mlx5_dpll_device_ops;
>>>+	err = dpll_device_register(mdpll->dpll, &mdpll->info, mdpll);
>>> 	if (err)
>>> 		goto err_put_dpll_device;
>>>
>>>@@ -481,7 +483,7 @@ static int mlx5_dpll_probe(struct auxiliary_device
>>>*adev,
>>> err_put_dpll_pin:
>>> 	dpll_pin_put(mdpll->dpll_pin);
>>> err_unregister_dpll_device:
>>>-	dpll_device_unregister(mdpll->dpll, &mlx5_dpll_device_ops, mdpll);
>>>+	dpll_device_unregister(mdpll->dpll, &mdpll->info, mdpll);
>>> err_put_dpll_device:
>>> 	dpll_device_put(mdpll->dpll);
>>> err_free_mdpll:
>>>@@ -500,7 +502,7 @@ static void mlx5_dpll_remove(struct auxiliary_device
>>>*adev)
>>> 	dpll_pin_unregister(mdpll->dpll, mdpll->dpll_pin,
>>> 			    &mlx5_dpll_pins_ops, mdpll);
>>> 	dpll_pin_put(mdpll->dpll_pin);
>>>-	dpll_device_unregister(mdpll->dpll, &mlx5_dpll_device_ops, mdpll);
>>>+	dpll_device_unregister(mdpll->dpll, &mdpll->info, mdpll);
>>> 	dpll_device_put(mdpll->dpll);
>>> 	kfree(mdpll);
>>>
>>>diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>>>index 7945c6be1f7c..b3c5d294acb4 100644
>>>--- a/drivers/ptp/ptp_ocp.c
>>>+++ b/drivers/ptp/ptp_ocp.c
>>>@@ -382,6 +382,7 @@ struct ptp_ocp {
>>> 	struct ptp_ocp_sma_connector sma[OCP_SMA_NUM];
>>> 	const struct ocp_sma_op *sma_op;
>>> 	struct dpll_device *dpll;
>>>+	struct dpll_device_info	dpll_info;
>>> };
>>>
>>> #define OCP_REQ_TIMESTAMP	BIT(0)
>>>@@ -4745,7 +4746,9 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct
>>>pci_device_id *id)
>>> 		goto out;
>>> 	}
>>>
>>>-	err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp);
>>>+	bp->dpll_info.type = DPLL_TYPE_PPS;
>>>+	bp->dpll_info.ops = &dpll_ops;
>>>+	err = dpll_device_register(bp->dpll, &bp->dpll_info, bp);
>>> 	if (err)
>>> 		goto out;
>>>
>>>@@ -4796,7 +4799,7 @@ ptp_ocp_remove(struct pci_dev *pdev)
>>> 			dpll_pin_put(bp->sma[i].dpll_pin);
>>> 		}
>>> 	}
>>>-	dpll_device_unregister(bp->dpll, &dpll_ops, bp);
>>>+	dpll_device_unregister(bp->dpll, &bp->dpll_info, bp);
>>> 	dpll_device_put(bp->dpll);
>>> 	devlink_unregister(devlink);
>>> 	ptp_ocp_detach(bp);
>>>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>>>index 5e4f9ab1cf75..0489464af958 100644
>>>--- a/include/linux/dpll.h
>>>+++ b/include/linux/dpll.h
>>>@@ -97,6 +97,11 @@ struct dpll_pin_ops {
>>> 			 struct netlink_ext_ack *extack);
>>> };
>>>
>>>+struct dpll_device_info {
>>>+	enum dpll_type type;
>>>+	const struct dpll_device_ops *ops;
>>>+};
>>>+
>>> struct dpll_pin_frequency {
>>> 	u64 min;
>>> 	u64 max;
>>>@@ -170,11 +175,11 @@ dpll_device_get(u64 clock_id, u32 dev_driver_id,
>>>struct module *module);
>>>
>>> void dpll_device_put(struct dpll_device *dpll);
>>>
>>>-int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
>>>-			 const struct dpll_device_ops *ops, void *priv);
>>>+int dpll_device_register(struct dpll_device *dpll,
>>>+			 const struct dpll_device_info *info, void *priv);
>>>
>>> void dpll_device_unregister(struct dpll_device *dpll,
>>>-			    const struct dpll_device_ops *ops, void *priv);
>>>+			    const struct dpll_device_info *info, void *priv);
>>>
>>> struct dpll_pin *
>>> dpll_pin_get(u64 clock_id, u32 dev_driver_id, struct module *module,
>>>--
>>>2.38.1
>>>

Powered by blists - more mailing lists