Re: [PATCH net-next v10 02/13] net: pse-pd: Add support for reporting events

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 06 May 2025 11:38:34 +0200 Kory Maincent wrote:
> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
> index c650cd3dcb80..fbfd293987c1 100644
> --- a/Documentation/netlink/specs/ethtool.yaml
> +++ b/Documentation/netlink/specs/ethtool.yaml
> @@ -98,6 +98,12 @@ definitions:
>      name: tcp-data-split
>      type: enum
>      entries: [ unknown, disabled, enabled ]
> +  -
> +    name: pse-events
> +    type: flags
> +    name-prefix: ethtool-pse-event-
> +    header: linux/ethtool.h
> +    entries: [ over-current, over-temp ]

please change this enum similarly to what I suggested on the hwts
source patch

>  attribute-sets:
>    -
> @@ -1528,6 +1534,18 @@ attribute-sets:
>          name: hwtstamp-flags
>          type: nest
>          nested-attributes: bitset
> +  -
> +    name: pse-ntf
> +    attr-cnt-name: __ethtool-a-pse-ntf-cnt

please use -- instead of underscores

> +    attributes:
> +      -
> +        name: header
> +        type: nest
> +        nested-attributes: header
> +      -
> +        name: events
> +        type: uint
> +        enum: pse-events

A tiny "doc:" here or better explanation in the Documentation/ 
may be nice. I thought it was a counter when I first looked...


> +  ===============================  ======  ========================
> +  ``ETHTOOL_A_PSE_HEADER``         nested  request header
> +  ``ETHTOOL_A_PSE_EVENTS``         bitset  PSE events
> +  ===============================  ======  ========================
> +
> +When set, the optional ``ETHTOOL_A_PSE_EVENTS`` attribute identifies the
> +PSE events.
> +
> +.. kernel-doc:: include/uapi/linux/ethtool.h
> +    :identifiers: ethtool_pse_events

I guess in HTML the enum will get rendered here so it will be clearer.

>  static DEFINE_MUTEX(pse_list_mutex);
>  static LIST_HEAD(pse_controller_list);
> @@ -23,6 +27,7 @@ static LIST_HEAD(pse_controller_list);
>   * @list: list entry for the pcdev's PSE controller list
>   * @id: ID of the PSE line in the PSE controller device
>   * @refcnt: Number of gets of this pse_control
> + * @attached_phydev: PHY device pointer attached by the PSE control
>   */
>  struct pse_control {
>  	struct pse_controller_dev *pcdev;
> @@ -30,6 +35,7 @@ struct pse_control {
>  	struct list_head list;
>  	unsigned int id;
>  	struct kref refcnt;
> +	struct phy_device *attached_phydev;
>  };

Adding the attached_phydev should be a separate patch, I think

>  static int of_load_single_pse_pi_pairset(struct device_node *node,
> @@ -208,6 +214,52 @@ static int of_load_pse_pis(struct pse_controller_dev *pcdev)
>  	return ret;
>  }
>  
> +/**
> + * pse_control_find_net_by_id - Find net attached to the pse control id
> + * @pcdev: a pointer to the PSE
> + * @id: index of the PSE control
> + * @tracker: refcount tracker used by netdev
> + *
> + * Return: net device pointer or NULL. The device returned has had a
> + *	   reference added and the pointer is safe until the user calls
> + *	   netdev_put() to indicate they have finished with it.
> + */
> +static struct net_device *
> +pse_control_find_net_by_id(struct pse_controller_dev *pcdev, int id,
> +			   netdevice_tracker *tracker)
> +{
> +	struct pse_control *psec, *next;
> +
> +	mutex_lock(&pse_list_mutex);
> +	list_for_each_entry_safe(psec, next, &pcdev->pse_control_head, list) {
> +		if (psec->id == id) {
> +			struct net_device *netdev = NULL;
> +			struct phy_device *phydev;
> +
> +			kref_get(&psec->refcnt);
> +			/* Release the mutex before taking the rtnl lock
> +			 * to avoid deadlock in case of a pse_control_put
> +			 * call with the rtnl lock held.
> +			 */
> +			mutex_unlock(&pse_list_mutex);
> +			/* Acquire rtnl to protect the net device
> +			 * reference get.
> +			 */
> +			rtnl_lock();
> +			phydev = psec->attached_phydev;
> +			if (phydev->attached_dev) {
> +				netdev = phydev->attached_dev;
> +				netdev_hold(netdev, tracker, GFP_ATOMIC);

GFP_KERNEL ?

> +			}
> +			rtnl_unlock();
> +			pse_control_put(psec);
> +			return netdev;
> +		}
> +	}
> +	mutex_unlock(&pse_list_mutex);
> +	return NULL;
> +}
> +
>  static int pse_pi_is_enabled(struct regulator_dev *rdev)
>  {
>  	struct pse_controller_dev *pcdev = rdev_get_drvdata(rdev);

> +/**
> + * devm_pse_irq_helper - Register IRQ based PSE event notifier
> + *

Why the empty line here but not in the other kdocs?

> + * @pcdev: a pointer to the PSE
> + * @irq: the irq value to be passed to request_irq
> + * @irq_flags: the flags to be passed to request_irq
> + * @d: PSE interrupt description
> + *
> + * Return: 0 on success and failure value on error

... and errno on failure ?

> + */

> diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
> index 4f6b99eab2a6..1234bce46413 100644
> --- a/net/ethtool/pse-pd.c
> +++ b/net/ethtool/pse-pd.c
> @@ -12,6 +12,7 @@
>  #include <linux/ethtool_netlink.h>
>  #include <linux/ethtool.h>
>  #include <linux/phy.h>
> +#include "bitset.h"

Unused include.

>  struct pse_req_info {
>  	struct ethnl_req_info base;
> @@ -315,3 +316,46 @@ const struct ethnl_request_ops ethnl_pse_request_ops = {
>  	.set			= ethnl_set_pse,
>  	/* PSE has no notification */
>  };
> +
> +void ethnl_pse_send_ntf(struct net_device *netdev, unsigned long notifs,
> +			struct netlink_ext_ack *extack)
> +{
> +	struct genl_info info;
> +	void *reply_payload;
> +	struct sk_buff *skb;
> +	int reply_len;
> +	int ret;
> +
> +	if (!netdev || !notifs)
> +		return;
> +
> +	ethnl_info_init_ntf(&info, ETHTOOL_MSG_PSE_NTF);
> +	info.extack = extack;

You don't seem to be using this info for anything.

> +	reply_len = ethnl_reply_header_size() +
> +		    nla_total_size(sizeof(u32)); /* _PSE_NTF_EVENTS */
> +
> +	skb = genlmsg_new(reply_len, GFP_KERNEL);
> +	if (!skb)
> +		return;
> +
> +	reply_payload = ethnl_bcastmsg_put(skb, ETHTOOL_MSG_PSE_NTF);
> +	if (!reply_payload)
> +		goto err_skb;
> +
> +	ret = ethnl_fill_reply_header(skb, netdev,
> +				      ETHTOOL_A_PSE_NTF_HEADER);

first on a single line

> +	if (ret < 0)
> +		goto err_skb;
> +
> +	if (nla_put_u32(skb, ETHTOOL_A_PSE_NTF_EVENTS, notifs))

put_uint? Or make notifs argument u32, either way is fine, since we only
have 2 bits defined now. The mixing is a bit surprising.

> +		goto err_skb;
> +
> +	genlmsg_end(skb, reply_payload);
> +	ethnl_multicast(skb, netdev);