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);
