[PATCH net-next 0/5][pull request] 1GbE Intel Wired LAN Driver Updates 2021-07-16

[PATCH net-next 0/5][pull request] 1GbE Intel Wired LAN Driver Updates 2021-07-16

[Tony Nguyen]

Vinicius Costa Gomes says:

Add support for steering traffic to specific RX queues using Flex Filters.

As the name implies, Flex Filters are more flexible than using
Layer-2, VLAN or MAC address filters, one of the reasons is that they
allow "AND" operations more easily, e.g. when the user wants to steer
some traffic based on the source MAC address and the packet ethertype.

Future work include adding support for offloading tc-u32 filters to
the hardware.

The series is divided as follows:

Patch 1/5, add the low level primitives for configuring Flex filters.

Patch 2/5 and 3/5, allow ethtool to manage Flex filters.

Patch 4/5, when specifying filters that have multiple predicates, use
Flex filters.

Patch 5/5, Adds support for exposing the i225 LEDs using the LED subsystem.

The following are changes since commit 919d527956daa3e7ad03a23ba661beb8a46cacf4:
  bnx2x: remove unused variable 'cur_data_offset'
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 1GbE

Kurt Kanzenbach (4):
  igc: Add possibility to add flex filter
  igc: Integrate flex filter into ethtool ops
  igc: Make flex filter more flexible
  igc: Export LEDs

Vinicius Costa Gomes (1):
  igc: Allow for Flex Filters to be installed

 drivers/net/ethernet/intel/Kconfig           |   1 +
 drivers/net/ethernet/intel/igc/igc.h         |  48 +-
 drivers/net/ethernet/intel/igc/igc_defines.h |  62 ++-
 drivers/net/ethernet/intel/igc/igc_ethtool.c |  41 +-
 drivers/net/ethernet/intel/igc/igc_main.c    | 448 ++++++++++++++++++-
 drivers/net/ethernet/intel/igc/igc_regs.h    |  19 +
 6 files changed, 595 insertions(+), 24 deletions(-)

-- 
2.26.2

[PATCH net-next 1/5] igc: Add possibility to add flex filter

[Tony Nguyen]

From: Kurt Kanzenbach <kurt@linutronix.de>

The Intel i225 NIC has the possibility to add flex filters which can
match up to the first 128 byte of a packet. These filters are useful
for all kind of packet matching. One particular use case is Profinet,
as the different traffic classes are distinguished by the frame id
range which cannot be matched by any other means.

Add code to configure and enable flex filters.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Tested-by: Dvora Fuxbrumer <dvorax.fuxbrumer@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h         |  13 ++
 drivers/net/ethernet/intel/igc/igc_defines.h |  48 ++++++-
 drivers/net/ethernet/intel/igc/igc_main.c    | 134 +++++++++++++++++++
 drivers/net/ethernet/intel/igc/igc_regs.h    |  17 +++
 4 files changed, 207 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 5901ed9fb545..6016c132d981 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -33,6 +33,8 @@ void igc_ethtool_set_ops(struct net_device *);
 #define IGC_N_PEROUT	2
 #define IGC_N_SDP	4
 
+#define MAX_FLEX_FILTER			32
+
 enum igc_mac_filter_type {
 	IGC_MAC_FILTER_TYPE_DST = 0,
 	IGC_MAC_FILTER_TYPE_SRC
@@ -502,6 +504,17 @@ struct igc_nfc_rule {
  */
 #define IGC_MAX_RXNFC_RULES		32
 
+struct igc_flex_filter {
+	u8 index;
+	u8 data[128];
+	u8 mask[16];
+	u8 length;
+	u8 rx_queue;
+	u8 prio;
+	u8 immediate_irq;
+	u8 drop;
+};
+
 /* igc_desc_unused - calculate if we have unused descriptors */
 static inline u16 igc_desc_unused(const struct igc_ring *ring)
 {
diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index c3a5a5518790..6d6267d7bf4b 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -17,11 +17,20 @@
 #define IGC_WUC_PME_EN	0x00000002 /* PME Enable */
 
 /* Wake Up Filter Control */
-#define IGC_WUFC_LNKC	0x00000001 /* Link Status Change Wakeup Enable */
-#define IGC_WUFC_MAG	0x00000002 /* Magic Packet Wakeup Enable */
-#define IGC_WUFC_EX	0x00000004 /* Directed Exact Wakeup Enable */
-#define IGC_WUFC_MC	0x00000008 /* Directed Multicast Wakeup Enable */
-#define IGC_WUFC_BC	0x00000010 /* Broadcast Wakeup Enable */
+#define IGC_WUFC_LNKC		0x00000001 /* Link Status Change Wakeup Enable */
+#define IGC_WUFC_MAG		0x00000002 /* Magic Packet Wakeup Enable */
+#define IGC_WUFC_EX		0x00000004 /* Directed Exact Wakeup Enable */
+#define IGC_WUFC_MC		0x00000008 /* Directed Multicast Wakeup Enable */
+#define IGC_WUFC_BC		0x00000010 /* Broadcast Wakeup Enable */
+#define IGC_WUFC_FLEX_HQ	BIT(14)	   /* Flex Filters Host Queuing */
+#define IGC_WUFC_FLX0		BIT(16)	   /* Flexible Filter 0 Enable */
+#define IGC_WUFC_FLX1		BIT(17)	   /* Flexible Filter 1 Enable */
+#define IGC_WUFC_FLX2		BIT(18)	   /* Flexible Filter 2 Enable */
+#define IGC_WUFC_FLX3		BIT(19)	   /* Flexible Filter 3 Enable */
+#define IGC_WUFC_FLX4		BIT(20)	   /* Flexible Filter 4 Enable */
+#define IGC_WUFC_FLX5		BIT(21)	   /* Flexible Filter 5 Enable */
+#define IGC_WUFC_FLX6		BIT(22)	   /* Flexible Filter 6 Enable */
+#define IGC_WUFC_FLX7		BIT(23)	   /* Flexible Filter 7 Enable */
 
 #define IGC_CTRL_ADVD3WUC	0x00100000  /* D3 WUC */
 
@@ -46,6 +55,35 @@
 /* Wake Up Packet Memory stores the first 128 bytes of the wake up packet */
 #define IGC_WUPM_BYTES	128
 
+/* Wakeup Filter Control Extended */
+#define IGC_WUFC_EXT_FLX8	BIT(8)	/* Flexible Filter 8 Enable */
+#define IGC_WUFC_EXT_FLX9	BIT(9)	/* Flexible Filter 9 Enable */
+#define IGC_WUFC_EXT_FLX10	BIT(10)	/* Flexible Filter 10 Enable */
+#define IGC_WUFC_EXT_FLX11	BIT(11)	/* Flexible Filter 11 Enable */
+#define IGC_WUFC_EXT_FLX12	BIT(12)	/* Flexible Filter 12 Enable */
+#define IGC_WUFC_EXT_FLX13	BIT(13)	/* Flexible Filter 13 Enable */
+#define IGC_WUFC_EXT_FLX14	BIT(14)	/* Flexible Filter 14 Enable */
+#define IGC_WUFC_EXT_FLX15	BIT(15)	/* Flexible Filter 15 Enable */
+#define IGC_WUFC_EXT_FLX16	BIT(16)	/* Flexible Filter 16 Enable */
+#define IGC_WUFC_EXT_FLX17	BIT(17)	/* Flexible Filter 17 Enable */
+#define IGC_WUFC_EXT_FLX18	BIT(18)	/* Flexible Filter 18 Enable */
+#define IGC_WUFC_EXT_FLX19	BIT(19)	/* Flexible Filter 19 Enable */
+#define IGC_WUFC_EXT_FLX20	BIT(20)	/* Flexible Filter 20 Enable */
+#define IGC_WUFC_EXT_FLX21	BIT(21)	/* Flexible Filter 21 Enable */
+#define IGC_WUFC_EXT_FLX22	BIT(22)	/* Flexible Filter 22 Enable */
+#define IGC_WUFC_EXT_FLX23	BIT(23)	/* Flexible Filter 23 Enable */
+#define IGC_WUFC_EXT_FLX24	BIT(24)	/* Flexible Filter 24 Enable */
+#define IGC_WUFC_EXT_FLX25	BIT(25)	/* Flexible Filter 25 Enable */
+#define IGC_WUFC_EXT_FLX26	BIT(26)	/* Flexible Filter 26 Enable */
+#define IGC_WUFC_EXT_FLX27	BIT(27)	/* Flexible Filter 27 Enable */
+#define IGC_WUFC_EXT_FLX28	BIT(28)	/* Flexible Filter 28 Enable */
+#define IGC_WUFC_EXT_FLX29	BIT(29)	/* Flexible Filter 29 Enable */
+#define IGC_WUFC_EXT_FLX30	BIT(30)	/* Flexible Filter 30 Enable */
+#define IGC_WUFC_EXT_FLX31	BIT(31)	/* Flexible Filter 31 Enable */
+
+/* Physical Func Reset Done Indication */
+#define IGC_CTRL_EXT_LINK_MODE_MASK	0x00C00000
+
 /* Loop limit on how long we wait for auto-negotiation to complete */
 #define COPPER_LINK_UP_LIMIT		10
 #define PHY_AUTO_NEG_LIMIT		45
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index e29aadbc6744..0f8cd226fd2e 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -3075,6 +3075,140 @@ static void igc_del_etype_filter(struct igc_adapter *adapter, u16 etype)
 		   etype);
 }
 
+static int igc_flex_filter_select(struct igc_adapter *adapter,
+				  struct igc_flex_filter *input,
+				  u32 *fhft)
+{
+	struct igc_hw *hw = &adapter->hw;
+	u8 fhft_index;
+	u32 fhftsl;
+
+	if (input->index >= MAX_FLEX_FILTER) {
+		dev_err(&adapter->pdev->dev, "Wrong Flex Filter index selected!\n");
+		return -EINVAL;
+	}
+
+	/* Indirect table select register */
+	fhftsl = rd32(IGC_FHFTSL);
+	fhftsl &= ~IGC_FHFTSL_FTSL_MASK;
+	switch (input->index) {
+	case 0 ... 7:
+		fhftsl |= 0x00;
+		break;
+	case 8 ... 15:
+		fhftsl |= 0x01;
+		break;
+	case 16 ... 23:
+		fhftsl |= 0x02;
+		break;
+	case 24 ... 31:
+		fhftsl |= 0x03;
+		break;
+	}
+	wr32(IGC_FHFTSL, fhftsl);
+
+	/* Normalize index down to host table register */
+	fhft_index = input->index % 8;
+
+	*fhft = (fhft_index < 4) ? IGC_FHFT(fhft_index) :
+		IGC_FHFT_EXT(fhft_index - 4);
+
+	return 0;
+}
+
+static int __maybe_unused igc_write_flex_filter_ll(struct igc_adapter *adapter,
+						   struct igc_flex_filter *input)
+{
+	struct device *dev = &adapter->pdev->dev;
+	struct igc_hw *hw = &adapter->hw;
+	u8 *data = input->data;
+	u8 *mask = input->mask;
+	u32 queuing;
+	u32 fhft;
+	u32 wufc;
+	int ret;
+	int i;
+
+	/* Length has to be aligned to 8. Otherwise the filter will fail. Bail
+	 * out early to avoid surprises later.
+	 */
+	if (input->length % 8 != 0) {
+		dev_err(dev, "The length of a flex filter has to be 8 byte aligned!\n");
+		return -EINVAL;
+	}
+
+	/* Select corresponding flex filter register and get base for host table. */
+	ret = igc_flex_filter_select(adapter, input, &fhft);
+	if (ret)
+		return ret;
+
+	/* When adding a filter globally disable flex filter feature. That is
+	 * recommended within the datasheet.
+	 */
+	wufc = rd32(IGC_WUFC);
+	wufc &= ~IGC_WUFC_FLEX_HQ;
+	wr32(IGC_WUFC, wufc);
+
+	/* Configure filter */
+	queuing = input->length & IGC_FHFT_LENGTH_MASK;
+	queuing |= (input->rx_queue << IGC_FHFT_QUEUE_SHIFT) & IGC_FHFT_QUEUE_MASK;
+	queuing |= (input->prio << IGC_FHFT_PRIO_SHIFT) & IGC_FHFT_PRIO_MASK;
+
+	if (input->immediate_irq)
+		queuing |= IGC_FHFT_IMM_INT;
+
+	if (input->drop)
+		queuing |= IGC_FHFT_DROP;
+
+	wr32(fhft + 0xFC, queuing);
+
+	/* Write data (128 byte) and mask (128 bit) */
+	for (i = 0; i < 16; ++i) {
+		const size_t data_idx = i * 8;
+		const size_t row_idx = i * 16;
+		u32 dw0 =
+			(data[data_idx + 0] << 0) |
+			(data[data_idx + 1] << 8) |
+			(data[data_idx + 2] << 16) |
+			(data[data_idx + 3] << 24);
+		u32 dw1 =
+			(data[data_idx + 4] << 0) |
+			(data[data_idx + 5] << 8) |
+			(data[data_idx + 6] << 16) |
+			(data[data_idx + 7] << 24);
+		u32 tmp;
+
+		/* Write row: dw0, dw1 and mask */
+		wr32(fhft + row_idx, dw0);
+		wr32(fhft + row_idx + 4, dw1);
+
+		/* mask is only valid for MASK(7, 0) */
+		tmp = rd32(fhft + row_idx + 8);
+		tmp &= ~GENMASK(7, 0);
+		tmp |= mask[i];
+		wr32(fhft + row_idx + 8, tmp);
+	}
+
+	/* Enable filter. */
+	wufc |= IGC_WUFC_FLEX_HQ;
+	if (input->index > 8) {
+		/* Filter 0-7 are enabled via WUFC. The other 24 filters are not. */
+		u32 wufc_ext = rd32(IGC_WUFC_EXT);
+
+		wufc_ext |= (IGC_WUFC_EXT_FLX8 << (input->index - 8));
+
+		wr32(IGC_WUFC_EXT, wufc_ext);
+	} else {
+		wufc |= (IGC_WUFC_FLX0 << input->index);
+	}
+	wr32(IGC_WUFC, wufc);
+
+	dev_dbg(&adapter->pdev->dev, "Added flex filter %u to HW.\n",
+		input->index);
+
+	return 0;
+}
+
 static int igc_enable_nfc_rule(struct igc_adapter *adapter,
 			       const struct igc_nfc_rule *rule)
 {
diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
index 0f82990567d9..828c3501c448 100644
--- a/drivers/net/ethernet/intel/igc/igc_regs.h
+++ b/drivers/net/ethernet/intel/igc/igc_regs.h
@@ -67,6 +67,9 @@
 
 /* Filtering Registers */
 #define IGC_ETQF(_n)		(0x05CB0 + (4 * (_n))) /* EType Queue Fltr */
+#define IGC_FHFT(_n)		(0x09000 + (256 * (_n))) /* Flexible Host Filter */
+#define IGC_FHFT_EXT(_n)	(0x09A00 + (256 * (_n))) /* Flexible Host Filter Extended */
+#define IGC_FHFTSL		0x05804 /* Flex Filter indirect table select */
 
 /* ETQF register bit definitions */
 #define IGC_ETQF_FILTER_ENABLE	BIT(26)
@@ -75,6 +78,19 @@
 #define IGC_ETQF_QUEUE_MASK	0x00070000
 #define IGC_ETQF_ETYPE_MASK	0x0000FFFF
 
+/* FHFT register bit definitions */
+#define IGC_FHFT_LENGTH_MASK	GENMASK(7, 0)
+#define IGC_FHFT_QUEUE_SHIFT	8
+#define IGC_FHFT_QUEUE_MASK	GENMASK(10, 8)
+#define IGC_FHFT_PRIO_SHIFT	16
+#define IGC_FHFT_PRIO_MASK	GENMASK(18, 16)
+#define IGC_FHFT_IMM_INT	BIT(24)
+#define IGC_FHFT_DROP		BIT(25)
+
+/* FHFTSL register bit definitions */
+#define IGC_FHFTSL_FTSL_SHIFT	0
+#define IGC_FHFTSL_FTSL_MASK	GENMASK(1, 0)
+
 /* Redirection Table - RW Array */
 #define IGC_RETA(_i)		(0x05C00 + ((_i) * 4))
 /* RSS Random Key - RW Array */
@@ -240,6 +256,7 @@
 #define IGC_WUFC	0x05808  /* Wakeup Filter Control - RW */
 #define IGC_WUS		0x05810  /* Wakeup Status - R/W1C */
 #define IGC_WUPL	0x05900  /* Wakeup Packet Length - RW */
+#define IGC_WUFC_EXT	0x0580C  /* Wakeup Filter Control Register Extended - RW */
 
 /* Wake Up packet memory */
 #define IGC_WUPM_REG(_i)	(0x05A00 + ((_i) * 4))
-- 
2.26.2

[PATCH net-next 2/5] igc: Integrate flex filter into ethtool ops

[Tony Nguyen]

From: Kurt Kanzenbach <kurt@linutronix.de>

Use the flex filter mechanism to extend the current ethtool filter
operations by intercoperating the user data. This allows to match
eight more bytes within a Ethernet frame in addition to macs, ether
types and vlan.

The matching pattern looks like this:

 * dest_mac [6]
 * src_mac [6]
 * tpid [2]
 * vlan tci [2]
 * ether type [2]
 * user data [8]

This can be used to match Profinet traffic classes by FrameID range.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Tested-by: Dvora Fuxbrumer <dvorax.fuxbrumer@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h         |  24 ++-
 drivers/net/ethernet/intel/igc/igc_defines.h |   4 +
 drivers/net/ethernet/intel/igc/igc_ethtool.c |  20 ++
 drivers/net/ethernet/intel/igc/igc_main.c    | 190 ++++++++++++++++++-
 4 files changed, 228 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 6016c132d981..c21441c8908e 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -478,18 +478,28 @@ struct igc_q_vector {
 };
 
 enum igc_filter_match_flags {
-	IGC_FILTER_FLAG_ETHER_TYPE =	0x1,
-	IGC_FILTER_FLAG_VLAN_TCI   =	0x2,
-	IGC_FILTER_FLAG_SRC_MAC_ADDR =	0x4,
-	IGC_FILTER_FLAG_DST_MAC_ADDR =	0x8,
+	IGC_FILTER_FLAG_ETHER_TYPE =	BIT(0),
+	IGC_FILTER_FLAG_VLAN_TCI   =	BIT(1),
+	IGC_FILTER_FLAG_SRC_MAC_ADDR =	BIT(2),
+	IGC_FILTER_FLAG_DST_MAC_ADDR =	BIT(3),
+	IGC_FILTER_FLAG_USER_DATA =	BIT(4),
+	IGC_FILTER_FLAG_VLAN_ETYPE =	BIT(5),
 };
 
 struct igc_nfc_filter {
 	u8 match_flags;
 	u16 etype;
+	__be16 vlan_etype;
 	u16 vlan_tci;
 	u8 src_addr[ETH_ALEN];
 	u8 dst_addr[ETH_ALEN];
+	u8 user_data[8];
+	u8 user_mask[8];
+	u8 flex_index;
+	u8 rx_queue;
+	u8 prio;
+	u8 immediate_irq;
+	u8 drop;
 };
 
 struct igc_nfc_rule {
@@ -499,10 +509,10 @@ struct igc_nfc_rule {
 	u16 action;
 };
 
-/* IGC supports a total of 32 NFC rules: 16 MAC address based,, 8 VLAN priority
- * based, and 8 ethertype based.
+/* IGC supports a total of 32 NFC rules: 16 MAC address based, 8 VLAN priority
+ * based, 8 ethertype based and 32 Flex filter based rules.
  */
-#define IGC_MAX_RXNFC_RULES		32
+#define IGC_MAX_RXNFC_RULES		64
 
 struct igc_flex_filter {
 	u8 index;
diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 6d6267d7bf4b..c6315690e20f 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -32,6 +32,8 @@
 #define IGC_WUFC_FLX6		BIT(22)	   /* Flexible Filter 6 Enable */
 #define IGC_WUFC_FLX7		BIT(23)	   /* Flexible Filter 7 Enable */
 
+#define IGC_WUFC_FILTER_MASK GENMASK(23, 14)
+
 #define IGC_CTRL_ADVD3WUC	0x00100000  /* D3 WUC */
 
 /* Wake Up Status */
@@ -81,6 +83,8 @@
 #define IGC_WUFC_EXT_FLX30	BIT(30)	/* Flexible Filter 30 Enable */
 #define IGC_WUFC_EXT_FLX31	BIT(31)	/* Flexible Filter 31 Enable */
 
+#define IGC_WUFC_EXT_FILTER_MASK GENMASK(31, 8)
+
 /* Physical Func Reset Done Indication */
 #define IGC_CTRL_EXT_LINK_MODE_MASK	0x00C00000
 
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index fa4171860623..3d46eff87638 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -979,6 +979,12 @@ static int igc_ethtool_get_nfc_rule(struct igc_adapter *adapter,
 		eth_broadcast_addr(fsp->m_u.ether_spec.h_source);
 	}
 
+	if (rule->filter.match_flags & IGC_FILTER_FLAG_USER_DATA) {
+		fsp->flow_type |= FLOW_EXT;
+		memcpy(fsp->h_ext.data, rule->filter.user_data, sizeof(fsp->h_ext.data));
+		memcpy(fsp->m_ext.data, rule->filter.user_mask, sizeof(fsp->m_ext.data));
+	}
+
 	mutex_unlock(&adapter->nfc_rule_lock);
 	return 0;
 
@@ -1215,6 +1221,20 @@ static void igc_ethtool_init_nfc_rule(struct igc_nfc_rule *rule,
 		ether_addr_copy(rule->filter.dst_addr,
 				fsp->h_u.ether_spec.h_dest);
 	}
+
+	/* Check for user defined data */
+	if ((fsp->flow_type & FLOW_EXT) &&
+	    (fsp->h_ext.data[0] || fsp->h_ext.data[1])) {
+		rule->filter.match_flags |= IGC_FILTER_FLAG_USER_DATA;
+		memcpy(rule->filter.user_data, fsp->h_ext.data, sizeof(fsp->h_ext.data));
+		memcpy(rule->filter.user_mask, fsp->m_ext.data, sizeof(fsp->m_ext.data));
+
+		/* VLAN etype matching is only valid using flex filter */
+		if ((fsp->flow_type & FLOW_EXT) && fsp->h_ext.vlan_etype) {
+			rule->filter.vlan_etype = fsp->h_ext.vlan_etype;
+			rule->filter.match_flags |= IGC_FILTER_FLAG_VLAN_ETYPE;
+		}
+	}
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 0f8cd226fd2e..9999d8fc640b 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -3116,8 +3116,8 @@ static int igc_flex_filter_select(struct igc_adapter *adapter,
 	return 0;
 }
 
-static int __maybe_unused igc_write_flex_filter_ll(struct igc_adapter *adapter,
-						   struct igc_flex_filter *input)
+static int igc_write_flex_filter_ll(struct igc_adapter *adapter,
+				    struct igc_flex_filter *input)
 {
 	struct device *dev = &adapter->pdev->dev;
 	struct igc_hw *hw = &adapter->hw;
@@ -3209,11 +3209,192 @@ static int __maybe_unused igc_write_flex_filter_ll(struct igc_adapter *adapter,
 	return 0;
 }
 
+static void igc_flex_filter_add_field(struct igc_flex_filter *flex,
+				      const void *src, unsigned int offset,
+				      size_t len, const void *mask)
+{
+	int i;
+
+	/* data */
+	memcpy(&flex->data[offset], src, len);
+
+	/* mask */
+	for (i = 0; i < len; ++i) {
+		const unsigned int idx = i + offset;
+		const u8 *ptr = mask;
+
+		if (mask) {
+			if (ptr[i] & 0xff)
+				flex->mask[idx / 8] |= BIT(idx % 8);
+
+			continue;
+		}
+
+		flex->mask[idx / 8] |= BIT(idx % 8);
+	}
+}
+
+static int igc_find_avail_flex_filter_slot(struct igc_adapter *adapter)
+{
+	struct igc_hw *hw = &adapter->hw;
+	u32 wufc, wufc_ext;
+	int i;
+
+	wufc = rd32(IGC_WUFC);
+	wufc_ext = rd32(IGC_WUFC_EXT);
+
+	for (i = 0; i < MAX_FLEX_FILTER; i++) {
+		if (i < 8) {
+			if (!(wufc & (IGC_WUFC_FLX0 << i)))
+				return i;
+		} else {
+			if (!(wufc_ext & (IGC_WUFC_EXT_FLX8 << (i - 8))))
+				return i;
+		}
+	}
+
+	return -ENOSPC;
+}
+
+static bool igc_flex_filter_in_use(struct igc_adapter *adapter)
+{
+	struct igc_hw *hw = &adapter->hw;
+	u32 wufc, wufc_ext;
+
+	wufc = rd32(IGC_WUFC);
+	wufc_ext = rd32(IGC_WUFC_EXT);
+
+	if (wufc & IGC_WUFC_FILTER_MASK)
+		return true;
+
+	if (wufc_ext & IGC_WUFC_EXT_FILTER_MASK)
+		return true;
+
+	return false;
+}
+
+static int igc_add_flex_filter(struct igc_adapter *adapter,
+			       struct igc_nfc_rule *rule)
+{
+	struct igc_flex_filter flex = { };
+	struct igc_nfc_filter *filter = &rule->filter;
+	unsigned int eth_offset, user_offset;
+	int ret, index;
+	bool vlan;
+
+	index = igc_find_avail_flex_filter_slot(adapter);
+	if (index < 0)
+		return -ENOSPC;
+
+	/* Construct the flex filter:
+	 *  -> dest_mac [6]
+	 *  -> src_mac [6]
+	 *  -> tpid [2]
+	 *  -> vlan tci [2]
+	 *  -> ether type [2]
+	 *  -> user data [8]
+	 *  -> = 26 bytes => 32 length
+	 */
+	flex.index    = index;
+	flex.length   = 32;
+	flex.rx_queue = rule->action;
+
+	vlan = rule->filter.vlan_tci || rule->filter.vlan_etype;
+	eth_offset = vlan ? 16 : 12;
+	user_offset = vlan ? 18 : 14;
+
+	/* Add destination MAC  */
+	if (rule->filter.match_flags & IGC_FILTER_FLAG_DST_MAC_ADDR)
+		igc_flex_filter_add_field(&flex, &filter->dst_addr, 0,
+					  ETH_ALEN, NULL);
+
+	/* Add source MAC */
+	if (rule->filter.match_flags & IGC_FILTER_FLAG_SRC_MAC_ADDR)
+		igc_flex_filter_add_field(&flex, &filter->src_addr, 6,
+					  ETH_ALEN, NULL);
+
+	/* Add VLAN etype */
+	if (rule->filter.match_flags & IGC_FILTER_FLAG_VLAN_ETYPE)
+		igc_flex_filter_add_field(&flex, &filter->vlan_etype, 12,
+					  sizeof(filter->vlan_etype),
+					  NULL);
+
+	/* Add VLAN TCI */
+	if (rule->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI)
+		igc_flex_filter_add_field(&flex, &filter->vlan_tci, 14,
+					  sizeof(filter->vlan_tci), NULL);
+
+	/* Add Ether type */
+	if (rule->filter.match_flags & IGC_FILTER_FLAG_ETHER_TYPE) {
+		__be16 etype = cpu_to_be16(filter->etype);
+
+		igc_flex_filter_add_field(&flex, &etype, eth_offset,
+					  sizeof(etype), NULL);
+	}
+
+	/* Add user data */
+	if (rule->filter.match_flags & IGC_FILTER_FLAG_USER_DATA)
+		igc_flex_filter_add_field(&flex, &filter->user_data,
+					  user_offset,
+					  sizeof(filter->user_data),
+					  filter->user_mask);
+
+	/* Add it down to the hardware and enable it. */
+	ret = igc_write_flex_filter_ll(adapter, &flex);
+	if (ret)
+		return ret;
+
+	filter->flex_index = index;
+
+	return 0;
+}
+
+static void igc_del_flex_filter(struct igc_adapter *adapter,
+				u16 reg_index)
+{
+	struct igc_hw *hw = &adapter->hw;
+	u32 wufc;
+
+	/* Just disable the filter. The filter table itself is kept
+	 * intact. Another flex_filter_add() should override the "old" data
+	 * then.
+	 */
+	if (reg_index > 8) {
+		u32 wufc_ext = rd32(IGC_WUFC_EXT);
+
+		wufc_ext &= ~(IGC_WUFC_EXT_FLX8 << (reg_index - 8));
+		wr32(IGC_WUFC_EXT, wufc_ext);
+	} else {
+		wufc = rd32(IGC_WUFC);
+
+		wufc &= ~(IGC_WUFC_FLX0 << reg_index);
+		wr32(IGC_WUFC, wufc);
+	}
+
+	if (igc_flex_filter_in_use(adapter))
+		return;
+
+	/* No filters are in use, we may disable flex filters */
+	wufc = rd32(IGC_WUFC);
+	wufc &= ~IGC_WUFC_FLEX_HQ;
+	wr32(IGC_WUFC, wufc);
+}
+
 static int igc_enable_nfc_rule(struct igc_adapter *adapter,
-			       const struct igc_nfc_rule *rule)
+			       struct igc_nfc_rule *rule)
 {
 	int err;
 
+	/* Check for user data first: When user data is set, the only option is
+	 * to use a flex filter. When more options are set (ethertype, vlan tci,
+	 * ...) construct a flex filter matching all of that.
+	 */
+	if (rule->filter.match_flags & IGC_FILTER_FLAG_USER_DATA) {
+		err = igc_add_flex_filter(adapter, rule);
+		if (err)
+			return err;
+	}
+
 	if (rule->filter.match_flags & IGC_FILTER_FLAG_ETHER_TYPE) {
 		err = igc_add_etype_filter(adapter, rule->filter.etype,
 					   rule->action);
@@ -3250,6 +3431,9 @@ static int igc_enable_nfc_rule(struct igc_adapter *adapter,
 static void igc_disable_nfc_rule(struct igc_adapter *adapter,
 				 const struct igc_nfc_rule *rule)
 {
+	if (rule->filter.match_flags & IGC_FILTER_FLAG_USER_DATA)
+		igc_del_flex_filter(adapter, rule->filter.flex_index);
+
 	if (rule->filter.match_flags & IGC_FILTER_FLAG_ETHER_TYPE)
 		igc_del_etype_filter(adapter, rule->filter.etype);
 
-- 
2.26.2

[PATCH net-next 3/5] igc: Allow for Flex Filters to be installed

[Tony Nguyen]

From: Vinicius Costa Gomes <vinicius.gomes@intel.com>

Allows Flex Filters to be installed.

The previous restriction to the types of filters that can be installed
can now be lifted.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Tested-by: Dvora Fuxbrumer <dvorax.fuxbrumer@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 3d46eff87638..5a7b27b2a95c 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1300,12 +1300,6 @@ static int igc_ethtool_add_nfc_rule(struct igc_adapter *adapter,
 		return -EOPNOTSUPP;
 	}
 
-	if ((fsp->flow_type & FLOW_EXT) &&
-	    fsp->m_ext.vlan_tci != htons(VLAN_PRIO_MASK)) {
-		netdev_dbg(netdev, "VLAN mask not supported\n");
-		return -EOPNOTSUPP;
-	}
-
 	if (fsp->ring_cookie >= adapter->num_rx_queues) {
 		netdev_dbg(netdev, "Invalid action\n");
 		return -EINVAL;
-- 
2.26.2

[PATCH net-next 4/5] igc: Make flex filter more flexible

[Tony Nguyen]

From: Kurt Kanzenbach <kurt@linutronix.de>

Currently flex filters are only used for filters containing user data.
However, it makes sense to utilize them also for filters having
multiple conditions, because that's not supported by the driver at the
moment. Add it.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Tested-by: Dvora Fuxbrumer <dvorax.fuxbrumer@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h         |  1 +
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 27 ++++++++++++--------
 drivers/net/ethernet/intel/igc/igc_main.c    | 14 ++++------
 3 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index c21441c8908e..a0ecfe5a4078 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -507,6 +507,7 @@ struct igc_nfc_rule {
 	struct igc_nfc_filter filter;
 	u32 location;
 	u16 action;
+	bool flex;
 };
 
 /* IGC supports a total of 32 NFC rules: 16 MAC address based, 8 VLAN priority
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 5a7b27b2a95c..d3e84416248e 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1222,19 +1222,29 @@ static void igc_ethtool_init_nfc_rule(struct igc_nfc_rule *rule,
 				fsp->h_u.ether_spec.h_dest);
 	}
 
+	/* VLAN etype matching */
+	if ((fsp->flow_type & FLOW_EXT) && fsp->h_ext.vlan_etype) {
+		rule->filter.vlan_etype = fsp->h_ext.vlan_etype;
+		rule->filter.match_flags |= IGC_FILTER_FLAG_VLAN_ETYPE;
+	}
+
 	/* Check for user defined data */
 	if ((fsp->flow_type & FLOW_EXT) &&
 	    (fsp->h_ext.data[0] || fsp->h_ext.data[1])) {
 		rule->filter.match_flags |= IGC_FILTER_FLAG_USER_DATA;
 		memcpy(rule->filter.user_data, fsp->h_ext.data, sizeof(fsp->h_ext.data));
 		memcpy(rule->filter.user_mask, fsp->m_ext.data, sizeof(fsp->m_ext.data));
-
-		/* VLAN etype matching is only valid using flex filter */
-		if ((fsp->flow_type & FLOW_EXT) && fsp->h_ext.vlan_etype) {
-			rule->filter.vlan_etype = fsp->h_ext.vlan_etype;
-			rule->filter.match_flags |= IGC_FILTER_FLAG_VLAN_ETYPE;
-		}
 	}
+
+	/* When multiple filter options or user data or vlan etype is set, use a
+	 * flex filter.
+	 */
+	if ((rule->filter.match_flags & IGC_FILTER_FLAG_USER_DATA) ||
+	    (rule->filter.match_flags & IGC_FILTER_FLAG_VLAN_ETYPE) ||
+	    (rule->filter.match_flags & (rule->filter.match_flags - 1)))
+		rule->flex = true;
+	else
+		rule->flex = false;
 }
 
 /**
@@ -1264,11 +1274,6 @@ static int igc_ethtool_check_nfc_rule(struct igc_adapter *adapter,
 		return -EINVAL;
 	}
 
-	if (flags & (flags - 1)) {
-		netdev_dbg(dev, "Rule with multiple matches not supported\n");
-		return -EOPNOTSUPP;
-	}
-
 	list_for_each_entry(tmp, &adapter->nfc_rule_list, list) {
 		if (!memcmp(&rule->filter, &tmp->filter,
 			    sizeof(rule->filter)) &&
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 9999d8fc640b..11385c380947 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -3385,14 +3385,8 @@ static int igc_enable_nfc_rule(struct igc_adapter *adapter,
 {
 	int err;
 
-	/* Check for user data first: When user data is set, the only option is
-	 * to use a flex filter. When more options are set (ethertype, vlan tci,
-	 * ...) construct a flex filter matching all of that.
-	 */
-	if (rule->filter.match_flags & IGC_FILTER_FLAG_USER_DATA) {
-		err = igc_add_flex_filter(adapter, rule);
-		if (err)
-			return err;
+	if (rule->flex) {
+		return igc_add_flex_filter(adapter, rule);
 	}
 
 	if (rule->filter.match_flags & IGC_FILTER_FLAG_ETHER_TYPE) {
@@ -3431,8 +3425,10 @@ static int igc_enable_nfc_rule(struct igc_adapter *adapter,
 static void igc_disable_nfc_rule(struct igc_adapter *adapter,
 				 const struct igc_nfc_rule *rule)
 {
-	if (rule->filter.match_flags & IGC_FILTER_FLAG_USER_DATA)
+	if (rule->flex) {
 		igc_del_flex_filter(adapter, rule->filter.flex_index);
+		return;
+	}
 
 	if (rule->filter.match_flags & IGC_FILTER_FLAG_ETHER_TYPE)
 		igc_del_etype_filter(adapter, rule->filter.etype);
-- 
2.26.2

[PATCH net-next 5/5] igc: Export LEDs

[Tony Nguyen]

From: Kurt Kanzenbach <kurt@linutronix.de>

Each i225 has three LEDs. Export them via the LED class framework.

Each LED is controllable via sysfs. Example:

$ cd /sys/class/leds/igc_led0
$ cat brightness      # Current Mode
$ cat max_brightness  # 15
$ echo 0 > brightness # Mode 0
$ echo 1 > brightness # Mode 1

The brightness field here reflects the different LED modes ranging
from 0 to 15.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Tested-by: Dvora Fuxbrumer <dvorax.fuxbrumer@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/Kconfig           |   1 +
 drivers/net/ethernet/intel/igc/igc.h         |  10 ++
 drivers/net/ethernet/intel/igc/igc_defines.h |  10 ++
 drivers/net/ethernet/intel/igc/igc_main.c    | 132 +++++++++++++++++++
 drivers/net/ethernet/intel/igc/igc_regs.h    |   2 +
 5 files changed, 155 insertions(+)

diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index 82744a7501c7..3639cf79cfae 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -335,6 +335,7 @@ config IGC
 	tristate "Intel(R) Ethernet Controller I225-LM/I225-V support"
 	default n
 	depends on PCI
+	depends on LEDS_CLASS
 	help
 	  This driver supports Intel(R) Ethernet Controller I225-LM/I225-V
 	  family of adapters.
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index a0ecfe5a4078..2df0fd2b9ecf 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -13,6 +13,7 @@
 #include <linux/ptp_clock_kernel.h>
 #include <linux/timecounter.h>
 #include <linux/net_tstamp.h>
+#include <linux/leds.h>
 
 #include "igc_hw.h"
 
@@ -239,8 +240,17 @@ struct igc_adapter {
 		struct timespec64 start;
 		struct timespec64 period;
 	} perout[IGC_N_PEROUT];
+
+	/* LEDs */
+	struct mutex led_mutex;
+	struct led_classdev led0;
+	struct led_classdev led1;
+	struct led_classdev led2;
 };
 
+#define led_to_igc(ldev, led)                          \
+	container_of(ldev, struct igc_adapter, led)
+
 void igc_up(struct igc_adapter *adapter);
 void igc_down(struct igc_adapter *adapter);
 int igc_open(struct net_device *netdev);
diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index c6315690e20f..156c3ef57c0a 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -144,6 +144,16 @@
 #define IGC_CTRL_SDP0_DIR	0x00400000  /* SDP0 Data direction */
 #define IGC_CTRL_SDP1_DIR	0x00800000  /* SDP1 Data direction */
 
+/* LED Control */
+#define IGC_LEDCTL_LED0_MODE_SHIFT	0
+#define IGC_LEDCTL_LED0_MODE_MASK	GENMASK(3, 0)
+#define IGC_LEDCTL_LED1_MODE_SHIFT	8
+#define IGC_LEDCTL_LED1_MODE_MASK	GENMASK(11, 8)
+#define IGC_LEDCTL_LED2_MODE_SHIFT	16
+#define IGC_LEDCTL_LED2_MODE_MASK	GENMASK(19, 16)
+
+#define IGC_CONNSW_AUTOSENSE_EN		0x1
+
 /* As per the EAS the maximum supported size is 9.5KB (9728 bytes) */
 #define MAX_JUMBO_FRAME_SIZE	0x2600
 
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 11385c380947..100819dcc7dd 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6130,6 +6130,134 @@ int igc_set_spd_dplx(struct igc_adapter *adapter, u32 spd, u8 dplx)
 	return -EINVAL;
 }
 
+static void igc_select_led(struct igc_adapter *adapter, int led,
+			   u32 *mask, u32 *shift)
+{
+	switch (led) {
+	case 0:
+		*mask  = IGC_LEDCTL_LED0_MODE_MASK;
+		*shift = IGC_LEDCTL_LED0_MODE_SHIFT;
+		break;
+	case 1:
+		*mask  = IGC_LEDCTL_LED1_MODE_MASK;
+		*shift = IGC_LEDCTL_LED1_MODE_SHIFT;
+		break;
+	case 2:
+		*mask  = IGC_LEDCTL_LED2_MODE_MASK;
+		*shift = IGC_LEDCTL_LED2_MODE_SHIFT;
+		break;
+	default:
+		*mask = *shift = 0;
+		dev_err(&adapter->pdev->dev, "Unknown led %d selected!", led);
+	}
+}
+
+static void igc_led_set(struct igc_adapter *adapter, int led, u16 brightness)
+{
+	struct igc_hw *hw = &adapter->hw;
+	u32 shift, mask, ledctl;
+
+	igc_select_led(adapter, led, &mask, &shift);
+
+	mutex_lock(&adapter->led_mutex);
+	ledctl = rd32(IGC_LEDCTL);
+	ledctl &= ~mask;
+	ledctl |= brightness << shift;
+	wr32(IGC_LEDCTL, ledctl);
+	mutex_unlock(&adapter->led_mutex);
+}
+
+static enum led_brightness igc_led_get(struct igc_adapter *adapter, int led)
+{
+	struct igc_hw *hw = &adapter->hw;
+	u32 shift, mask, ledctl;
+
+	igc_select_led(adapter, led, &mask, &shift);
+
+	mutex_lock(&adapter->led_mutex);
+	ledctl = rd32(IGC_LEDCTL);
+	mutex_unlock(&adapter->led_mutex);
+
+	return (ledctl & mask) >> shift;
+}
+
+static void igc_led0_set(struct led_classdev *ldev, enum led_brightness b)
+{
+	struct igc_adapter *adapter = led_to_igc(ldev, led0);
+
+	igc_led_set(adapter, 0, b);
+}
+
+static enum led_brightness igc_led0_get(struct led_classdev *ldev)
+{
+	struct igc_adapter *adapter = led_to_igc(ldev, led0);
+
+	return igc_led_get(adapter, 0);
+}
+
+static void igc_led1_set(struct led_classdev *ldev, enum led_brightness b)
+{
+	struct igc_adapter *adapter = led_to_igc(ldev, led1);
+
+	igc_led_set(adapter, 1, b);
+}
+
+static enum led_brightness igc_led1_get(struct led_classdev *ldev)
+{
+	struct igc_adapter *adapter = led_to_igc(ldev, led1);
+
+	return igc_led_get(adapter, 1);
+}
+
+static void igc_led2_set(struct led_classdev *ldev, enum led_brightness b)
+{
+	struct igc_adapter *adapter = led_to_igc(ldev, led2);
+
+	igc_led_set(adapter, 2, b);
+}
+
+static enum led_brightness igc_led2_get(struct led_classdev *ldev)
+{
+	struct igc_adapter *adapter = led_to_igc(ldev, led2);
+
+	return igc_led_get(adapter, 2);
+}
+
+static int igc_led_setup(struct igc_adapter *adapter)
+{
+	/* Setup */
+	mutex_init(&adapter->led_mutex);
+
+	adapter->led0.name	     = "igc_led0";
+	adapter->led0.max_brightness = 15;
+	adapter->led0.brightness_set = igc_led0_set;
+	adapter->led0.brightness_get = igc_led0_get;
+
+	adapter->led1.name	     = "igc_led1";
+	adapter->led1.max_brightness = 15;
+	adapter->led1.brightness_set = igc_led1_set;
+	adapter->led1.brightness_get = igc_led1_get;
+
+	adapter->led2.name	     = "igc_led2";
+	adapter->led2.max_brightness = 15;
+	adapter->led2.brightness_set = igc_led2_set;
+	adapter->led2.brightness_get = igc_led2_get;
+
+	/* Register leds */
+	led_classdev_register(&adapter->pdev->dev, &adapter->led0);
+	led_classdev_register(&adapter->pdev->dev, &adapter->led1);
+	led_classdev_register(&adapter->pdev->dev, &adapter->led2);
+
+	return 0;
+}
+
+static void igc_led_destroy(struct igc_adapter *adapter)
+{
+	led_classdev_unregister(&adapter->led0);
+	led_classdev_unregister(&adapter->led1);
+	led_classdev_unregister(&adapter->led2);
+}
+
 /**
  * igc_probe - Device Initialization Routine
  * @pdev: PCI device information struct
@@ -6357,6 +6485,8 @@ static int igc_probe(struct pci_dev *pdev,
 
 	pm_runtime_put_noidle(&pdev->dev);
 
+	igc_led_setup(adapter);
+
 	return 0;
 
 err_register:
@@ -6398,6 +6528,8 @@ static void igc_remove(struct pci_dev *pdev)
 
 	igc_ptp_stop(adapter);
 
+	igc_led_destroy(adapter);
+
 	set_bit(__IGC_DOWN, &adapter->state);
 
 	del_timer_sync(&adapter->watchdog_timer);
diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
index 828c3501c448..f6247b00c4e3 100644
--- a/drivers/net/ethernet/intel/igc/igc_regs.h
+++ b/drivers/net/ethernet/intel/igc/igc_regs.h
@@ -10,6 +10,8 @@
 #define IGC_EECD		0x00010  /* EEPROM/Flash Control - RW */
 #define IGC_CTRL_EXT		0x00018  /* Extended Device Control - RW */
 #define IGC_MDIC		0x00020  /* MDI Control - RW */
+#define IGC_LEDCTL		0x00E00	 /* LED Control - RW */
+#define IGC_MDICNFG		0x00E04  /* MDC/MDIO Configuration - RW */
 #define IGC_CONNSW		0x00034  /* Copper/Fiber switch control - RW */
 #define IGC_VET			0x00038  /* VLAN Ether Type - RW */
 #define IGC_I225_PHPM		0x00E14  /* I225 PHY Power Management */
-- 
2.26.2

Re: [PATCH net-next 5/5] igc: Export LEDs

[Andrew Lunn]

On Fri, Jul 16, 2021 at 02:24:27PM -0700, Tony Nguyen wrote:
> From: Kurt Kanzenbach <kurt@linutronix.de>
> 
> Each i225 has three LEDs. Export them via the LED class framework.
> 
> Each LED is controllable via sysfs. Example:
> 
> $ cd /sys/class/leds/igc_led0
> $ cat brightness      # Current Mode
> $ cat max_brightness  # 15
> $ echo 0 > brightness # Mode 0
> $ echo 1 > brightness # Mode 1
> 
> The brightness field here reflects the different LED modes ranging
> from 0 to 15.

What do you mean by mode? Do you mean blink mode? Like On means 1G
link, and it blinks for packet TX?

    Andrew

Re: [PATCH net-next 5/5] igc: Export LEDs

[Heiner Kallweit]

On 16.07.2021 23:56, Andrew Lunn wrote:
> On Fri, Jul 16, 2021 at 02:24:27PM -0700, Tony Nguyen wrote:
>> From: Kurt Kanzenbach <kurt@linutronix.de>
>>
>> Each i225 has three LEDs. Export them via the LED class framework.
>>
>> Each LED is controllable via sysfs. Example:
>>
>> $ cd /sys/class/leds/igc_led0
>> $ cat brightness      # Current Mode
>> $ cat max_brightness  # 15
>> $ echo 0 > brightness # Mode 0
>> $ echo 1 > brightness # Mode 1
>>
>> The brightness field here reflects the different LED modes ranging
>> from 0 to 15.
> 
> What do you mean by mode? Do you mean blink mode? Like On means 1G
> link, and it blinks for packet TX?
> 
Supposedly mode refers to a 4-bit bitfield in a LED control register
where each value 0 .. 15 stands for a different blink mode.
So you would need the datasheet to know which value to set.

>     Andrew
> 
Heiner

Re: [PATCH net-next 5/5] igc: Export LEDs

[Andrew Lunn]

On Mon, Jul 19, 2021 at 12:10:52AM +0200, Heiner Kallweit wrote:
> On 16.07.2021 23:56, Andrew Lunn wrote:
> > On Fri, Jul 16, 2021 at 02:24:27PM -0700, Tony Nguyen wrote:
> >> From: Kurt Kanzenbach <kurt@linutronix.de>
> >>
> >> Each i225 has three LEDs. Export them via the LED class framework.
> >>
> >> Each LED is controllable via sysfs. Example:
> >>
> >> $ cd /sys/class/leds/igc_led0
> >> $ cat brightness      # Current Mode
> >> $ cat max_brightness  # 15
> >> $ echo 0 > brightness # Mode 0
> >> $ echo 1 > brightness # Mode 1
> >>
> >> The brightness field here reflects the different LED modes ranging
> >> from 0 to 15.
> > 
> > What do you mean by mode? Do you mean blink mode? Like On means 1G
> > link, and it blinks for packet TX?
> > 
> Supposedly mode refers to a 4-bit bitfield in a LED control register
> where each value 0 .. 15 stands for a different blink mode.
> So you would need the datasheet to know which value to set.

If the brightness is being abused to represent the blink mode, this
patch is going to get my NACK. Unfortunately, i cannot find a
datasheet for this chip to know what the LED control register actually
does. So i'm waiting for a reply to my question.

There is a broad agreement between the LED maintainers and the PHYLIB
maintainers how Ethernet LEDs should be described with the hardware
blinking the LED for different reasons. The LED trigger mechanisms
should be used, one trigger per mode, and the trigger is then
offloaded to the hardware.

	  Andrew

Re: [PATCH net-next 5/5] igc: Export LEDs

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 965 bytes --]

On Mon Jul 19 2021, Andrew Lunn wrote:
>> Supposedly mode refers to a 4-bit bitfield in a LED control register
>> where each value 0 .. 15 stands for a different blink mode.
>> So you would need the datasheet to know which value to set.
>
> If the brightness is being abused to represent the blink mode, this
> patch is going to get my NACK.  Unfortunately, i cannot find a
> datasheet for this chip to know what the LED control register actually
> does. So i'm waiting for a reply to my question.
>
> There is a broad agreement between the LED maintainers and the PHYLIB
> maintainers how Ethernet LEDs should be described with the hardware
> blinking the LED for different reasons. The LED trigger mechanisms
> should be used, one trigger per mode, and the trigger is then
> offloaded to the hardware.

OK. I'll look into it. Can you point me to an example maybe?

Unfortunately this patch seems to be merged already. I guess it should
be reverted.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next 5/5] igc: Export LEDs

[Jakub Kicinski]

On Mon, 19 Jul 2021 08:17:36 +0200, Kurt Kanzenbach wrote:
> > If the brightness is being abused to represent the blink mode, this
> > patch is going to get my NACK.  Unfortunately, i cannot find a
> > datasheet for this chip to know what the LED control register actually
> > does. So i'm waiting for a reply to my question.
> >
> > There is a broad agreement between the LED maintainers and the PHYLIB
> > maintainers how Ethernet LEDs should be described with the hardware
> > blinking the LED for different reasons. The LED trigger mechanisms
> > should be used, one trigger per mode, and the trigger is then
> > offloaded to the hardware.  
> 
> OK. I'll look into it. Can you point me to an example maybe?
> 
> Unfortunately this patch seems to be merged already. I guess it should
> be reverted.

Yes, please send a revert patch saying in the commit message that Andrew
suggest a different interface etc.

Re: [PATCH net-next 5/5] igc: Export LEDs

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]

Hi Andrew,

On Fri Jul 16 2021, Andrew Lunn wrote:
> On Fri, Jul 16, 2021 at 02:24:27PM -0700, Tony Nguyen wrote:
>> From: Kurt Kanzenbach <kurt@linutronix.de>
>> 
>> Each i225 has three LEDs. Export them via the LED class framework.
>> 
>> Each LED is controllable via sysfs. Example:
>> 
>> $ cd /sys/class/leds/igc_led0
>> $ cat brightness      # Current Mode
>> $ cat max_brightness  # 15
>> $ echo 0 > brightness # Mode 0
>> $ echo 1 > brightness # Mode 1
>> 
>> The brightness field here reflects the different LED modes ranging
>> from 0 to 15.
>
> What do you mean by mode? Do you mean blink mode? Like On means 1G
> link, and it blinks for packet TX?

There are different modes such as ON, OFF, LINK established, LINK
activity, PAUSED ... Blinking is controlled by a different register.

Are there better ways to export this?

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next 5/5] igc: Export LEDs

[Andrew Lunn]

On Mon, Jul 19, 2021 at 08:06:41AM +0200, Kurt Kanzenbach wrote:
> Hi Andrew,
> 
> On Fri Jul 16 2021, Andrew Lunn wrote:
> > On Fri, Jul 16, 2021 at 02:24:27PM -0700, Tony Nguyen wrote:
> >> From: Kurt Kanzenbach <kurt@linutronix.de>
> >> 
> >> Each i225 has three LEDs. Export them via the LED class framework.
> >> 
> >> Each LED is controllable via sysfs. Example:
> >> 
> >> $ cd /sys/class/leds/igc_led0
> >> $ cat brightness      # Current Mode
> >> $ cat max_brightness  # 15
> >> $ echo 0 > brightness # Mode 0
> >> $ echo 1 > brightness # Mode 1
> >> 
> >> The brightness field here reflects the different LED modes ranging
> >> from 0 to 15.
> >
> > What do you mean by mode? Do you mean blink mode? Like On means 1G
> > link, and it blinks for packet TX?
> 
> There are different modes such as ON, OFF, LINK established, LINK
> activity, PAUSED ... Blinking is controlled by a different register.
> 
> Are there better ways to export this?

As i said in another email, using LED triggers. For simple link speed
indication, take a look at CONFIG_LED_TRIGGER_PHY. This is purely
software, and probably not what you want. The more complex offload of
to LED control to hardware in the PHY subsystem it still going around
and around. The basic idea is agreed, just not the implementation.
However, most of the implementation is of no help to you, since Intel
drivers ignore the kernel PHY drivers and do their own. But the basic
idea and style of user space API should be kept the same. So take a
look at the work Marek Behún has been doing, e.g.

https://lwn.net/Articles/830947/

and a more recent version

https://lore.kernel.org/netdev/20210602144439.4d20b295@dellmb/T/#m4e97c9016597fb40849c104c7c0e3bc10d5c1ff5

Looking at the basic idea of using LED triggers and offloading
them. Please also try to make us of generic names for these triggers,
so the PHY subsystem might also use the same or similar names when it
eventually gets something merged.

Please also Cc: The LED maintainers and LED list. Doing that from the
start would of avoided this revert, since you would of get earlier
feedback about this.

	 Andrew

Re: [PATCH net-next 5/5] igc: Export LEDs

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 2019 bytes --]

Hi Andrew,

On Mon Jul 19 2021, Andrew Lunn wrote:
> On Mon, Jul 19, 2021 at 08:06:41AM +0200, Kurt Kanzenbach wrote:
>> There are different modes such as ON, OFF, LINK established, LINK
>> activity, PAUSED ... Blinking is controlled by a different register.
>> 
>> Are there better ways to export this?
>
> As i said in another email, using LED triggers. For simple link speed
> indication, take a look at CONFIG_LED_TRIGGER_PHY. This is purely
> software, and probably not what you want.

Here's my use case/reasoning behind this patch: Upon reception of a
certain Ethernet frame, the LEDs should blink for a certain period of
time. Afterwards the default behavior should be restored. The blinking
can be done in hardware, but only for a fixed period. I needed a
different period.

Therefore, I've exported these as regular LEDs to toggle the brightness
from user space directly.

> The more complex offload of to LED control to hardware in the PHY
> subsystem it still going around and around. The basic idea is agreed,
> just not the implementation.  However, most of the implementation is
> of no help to you, since Intel drivers ignore the kernel PHY drivers
> and do their own. But the basic idea and style of user space API
> should be kept the same. So take a look at the work Marek Behún has
> been doing, e.g.
>
> https://lwn.net/Articles/830947/
>
> and a more recent version
>
> https://lore.kernel.org/netdev/20210602144439.4d20b295@dellmb/T/#m4e97c9016597fb40849c104c7c0e3bc10d5c1ff5

Thanks for the pointer.

>
> Looking at the basic idea of using LED triggers and offloading
> them. Please also try to make us of generic names for these triggers,
> so the PHY subsystem might also use the same or similar names when it
> eventually gets something merged.
>
> Please also Cc: The LED maintainers and LED list. Doing that from the
> start would of avoided this revert, since you would of get earlier
> feedback about this.

Yeah, noted.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

On Mon, 19 Jul 2021 15:15:36 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > Are there better ways to export this?  
> 
> As i said in another email, using LED triggers. For simple link speed
> indication, take a look at CONFIG_LED_TRIGGER_PHY.

Please don't use LED_TRIGGER_PHY. The way this driver works is against
the ideas of LED subsystem and we would like to deprecate it.

All network related LED control should be somehow configured via the
"netdev" trigger. I am working on extending "netdev" trigger for this
purpose. But first we need to be able to at least support offloading of
LED triggers. Hopefully I will send another version for that this week.

Marek

Re: [PATCH net-next 5/5] igc: Export LEDs

[Heiner Kallweit]

On 16.07.2021 23:24, Tony Nguyen wrote:
> From: Kurt Kanzenbach <kurt@linutronix.de>
> 
> Each i225 has three LEDs. Export them via the LED class framework.
> 
> Each LED is controllable via sysfs. Example:
> 
> $ cd /sys/class/leds/igc_led0

What if you have multiple igc adapters in a system?
AFAIK the LED subsystem assigns a unique name, but it's
out of your control and most likely not what you want.
Better would be to use the interface name. However then
a challenge is how to deal with interface renaming.

> $ cat brightness      # Current Mode
> $ cat max_brightness  # 15
> $ echo 0 > brightness # Mode 0
> $ echo 1 > brightness # Mode 1
> 

In general I'm not sure using the LED API provides a benefit here.
The brightness attribute is simply misused. Maybe better add
a sysfs attribute like led_mode under the netdev sysfs entry?
Then you also don't have the issue with interface renaming.

Re: [PATCH net-next 5/5] igc: Export LEDs

[Andrew Lunn]

> In general I'm not sure using the LED API provides a benefit here.
> The brightness attribute is simply misused. Maybe better add
> a sysfs attribute like led_mode under the netdev sysfs entry?

I _think_ you can put LED sys files other places than
/sys/class/led. It should be possible to put them into netdev sysfs
directory. However you need to consider what affect network name
spaces have on this and what happens when an interface changes
namespace.

     Andrew

Re: [PATCH net-next 5/5] igc: Export LEDs

[Heiner Kallweit]

On 19.07.2021 02:40, Andrew Lunn wrote:
>> In general I'm not sure using the LED API provides a benefit here.
>> The brightness attribute is simply misused. Maybe better add
>> a sysfs attribute like led_mode under the netdev sysfs entry?
> 
> I _think_ you can put LED sys files other places than
> /sys/class/led. It should be possible to put them into netdev sysfs
> directory. However you need to consider what affect network name
> spaces have on this and what happens when an interface changes
> namespace.
> 

I checked the LED subsystem and didn't find a way to place the LED
sysfs files in a place other than /sys/class/leds. Maybe Pavel can
comment on whether I just missed something.
To avoid the network namespace issues we could use the PCI device
name in the LED name, but this would be quite unfriendly to the
user.

For r8169 I'm facing a similar challenge like Kurt. Most family
members support three LED's:
- Per LED a mode 0 .. 15 can be set that defines which link speed(s)
  and/or activity is indicated.
- Period and duty cycle for blinking can be controlled, but this
  setting applies to all three LED's.

For testing purposes I created sysfs attributes led0, led1, led2,
period, duty and assigned the attribute group to netdev->sysfs_groups[0].
This works fine and all attributes are under /sys/class/net/<ifname>.
Only drawback is that you need to know which trigger mode is set by
values 0..15. However this can be documented in sysfs attribute
documentation under Documentation/ABI/testing.

For using the LED subsystem and triggers two things would have to be
solved:
- How to deal with network device name changes so that the user still
  can identify that a LED belongs to a certain network device.
- How to properly deal with attributes that are shared by a group of
  LED's?

>      Andrew
> .
> 
Heiner

Re: [PATCH net-next 5/5] igc: Export LEDs

[Andrew Lunn]

> I checked the LED subsystem and didn't find a way to place the LED
> sysfs files in a place other than /sys/class/leds. Maybe Pavel can
> comment on whether I just missed something.

https://lwn.net/ml/linux-kernel/20200908000300.6982-1-marek.behun@nic.cz/

It comments the sys files appear under
/sys/class/net/<ifname>/phydev/leds/. phydev is a symbolic link to the
phy devices, provided by the phydev subsystem. So they are actually
attached to the PHY device. And this appears to be due to:

	ret = devm_led_classdev_register_ext(&phydev->mdio.dev, &led->cdev, &init_data);

The LEDs are parented to the phy device. This has the nice side affect
that PHYs are not part of the network name space. You can rename the
interface, /sys/class/net/<ifname> changes, but the symbolic link
still points to the phy device.

When not using phydev, it probably gets trickier. You probably want
the LEDs parented to the PCI device, and you need to follow a
different symbolic link out of /sys/class/net/<ifname>/ to find the
LED.

There was talk of adding an ledtool, which knows about these
links. But i pushed for it to be added to ethtool. Until we get an
implementation actually merged, that is academic.

> For r8169 I'm facing a similar challenge like Kurt. Most family
> members support three LED's:
> - Per LED a mode 0 .. 15 can be set that defines which link speed(s)
>   and/or activity is indicated.
> - Period and duty cycle for blinking can be controlled, but this
>   setting applies to all three LED's.

Cross LED settings is a problem. The Marvell PHYs have a number of
independent modes. Plus they have some shared modes which cross LEDs.
At the moment, there is no good model for these shared modes.

We have to look at the trade offs here. At the moment we have at least
3 different ways of setting PHY LEDs via DT. Because each driver does
it its own way, it probably allows full access to all features. But it
is very unfriendly. Adopting Linux LEDs allows us to have a single
uniform API for all these PHY LEDs, and probably all MAC drivers which
don't use PHY drivers. But at the expense of probably not supporting
all features of the hardware. My opinion is, we should ignore some of
the hardware features in order to get a simple to use uniform
interface for all LEDs, which probably covers the features most people
are interested in anyway.

	Andrew

Re: [PATCH net-next 5/5] igc: Export LEDs

[Heiner Kallweit]

On 20.07.2021 17:42, Andrew Lunn wrote:
>> I checked the LED subsystem and didn't find a way to place the LED
>> sysfs files in a place other than /sys/class/leds. Maybe Pavel can
>> comment on whether I just missed something.
> 
> https://lwn.net/ml/linux-kernel/20200908000300.6982-1-marek.behun@nic.cz/
> 
> It comments the sys files appear under
> /sys/class/net/<ifname>/phydev/leds/. phydev is a symbolic link to the
> phy devices, provided by the phydev subsystem. So they are actually
> attached to the PHY device. And this appears to be due to:
> 
> 	ret = devm_led_classdev_register_ext(&phydev->mdio.dev, &led->cdev, &init_data);
> 
> The LEDs are parented to the phy device. This has the nice side affect
> that PHYs are not part of the network name space. You can rename the
> interface, /sys/class/net/<ifname> changes, but the symbolic link
> still points to the phy device.
> 
> When not using phydev, it probably gets trickier. You probably want
> the LEDs parented to the PCI device, and you need to follow a
> different symbolic link out of /sys/class/net/<ifname>/ to find the
> LED.
> 
> There was talk of adding an ledtool, which knows about these
> links. But i pushed for it to be added to ethtool. Until we get an
> implementation actually merged, that is academic.
> 
>> For r8169 I'm facing a similar challenge like Kurt. Most family
>> members support three LED's:
>> - Per LED a mode 0 .. 15 can be set that defines which link speed(s)
>>   and/or activity is indicated.
>> - Period and duty cycle for blinking can be controlled, but this
>>   setting applies to all three LED's.
> 
> Cross LED settings is a problem. The Marvell PHYs have a number of
> independent modes. Plus they have some shared modes which cross LEDs.
> At the moment, there is no good model for these shared modes.
> 
> We have to look at the trade offs here. At the moment we have at least
> 3 different ways of setting PHY LEDs via DT. Because each driver does
> it its own way, it probably allows full access to all features. But it
> is very unfriendly. Adopting Linux LEDs allows us to have a single
> uniform API for all these PHY LEDs, and probably all MAC drivers which
> don't use PHY drivers. But at the expense of probably not supporting
> all features of the hardware. My opinion is, we should ignore some of
> the hardware features in order to get a simple to use uniform
> interface for all LEDs, which probably covers the features most people
> are interested in anyway.
> 

Thanks for the hint, Andrew. If I make &netdev->dev the parent,
then I get:

ll /sys/class/leds/
total 0
lrwxrwxrwx 1 root root 0 Jul 20 21:37 led0 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led0
lrwxrwxrwx 1 root root 0 Jul 20 21:37 led1 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led1
lrwxrwxrwx 1 root root 0 Jul 20 21:37 led2 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led2

Now the (linked) LED devices are under /sys/class/net/<ifname>, but still
the primary LED devices are under /sys/class/leds and their names have
to be unique therefore. The LED subsystem takes care of unique names,
but in case of a second network interface the LED device name suddenly
would be led0_1 (IIRC). So the names wouldn't be predictable, and I think
that's not what we want.
We could use something like led0_<pci_id>, but then userspace would have
to do echo foo > /sys/class/net/<ifname>/led0*/bar, and that's also not
nice.

> 	Andrew
> 
Heiner

Re: [PATCH net-next 5/5] igc: Export LEDs

[Andrew Lunn]

> Thanks for the hint, Andrew. If I make &netdev->dev the parent,
> then I get:
> 
> ll /sys/class/leds/
> total 0
> lrwxrwxrwx 1 root root 0 Jul 20 21:37 led0 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led0
> lrwxrwxrwx 1 root root 0 Jul 20 21:37 led1 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led1
> lrwxrwxrwx 1 root root 0 Jul 20 21:37 led2 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led2
> 
> Now the (linked) LED devices are under /sys/class/net/<ifname>, but still
> the primary LED devices are under /sys/class/leds and their names have
> to be unique therefore. The LED subsystem takes care of unique names,
> but in case of a second network interface the LED device name suddenly
> would be led0_1 (IIRC). So the names wouldn't be predictable, and I think
> that's not what we want.

We need input from the LED maintainers, but do we actually need the
symbolic links in /sys/class/leds/? For this specific use case, not
generally. Allow an LED to opt out of the /sys/class/leds symlink.

If we could drop those, we can relax the naming requirements so that
the names is unique to a parent device, not globally unique.

    Andrew

Re: [PATCH net-next 5/5] igc: Export LEDs

[Heiner Kallweit]

On 21.07.2021 16:35, Andrew Lunn wrote:
>> Thanks for the hint, Andrew. If I make &netdev->dev the parent,
>> then I get:
>>
>> ll /sys/class/leds/
>> total 0
>> lrwxrwxrwx 1 root root 0 Jul 20 21:37 led0 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led0
>> lrwxrwxrwx 1 root root 0 Jul 20 21:37 led1 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led1
>> lrwxrwxrwx 1 root root 0 Jul 20 21:37 led2 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led2
>>
>> Now the (linked) LED devices are under /sys/class/net/<ifname>, but still
>> the primary LED devices are under /sys/class/leds and their names have
>> to be unique therefore. The LED subsystem takes care of unique names,
>> but in case of a second network interface the LED device name suddenly
>> would be led0_1 (IIRC). So the names wouldn't be predictable, and I think
>> that's not what we want.
> 
> We need input from the LED maintainers, but do we actually need the
> symbolic links in /sys/class/leds/? For this specific use case, not
> generally. Allow an LED to opt out of the /sys/class/leds symlink.
> 

The links are created here:

led_classdev_register_ext()
-> device_create_with_groups()
   -> device_add()
      -> device_add_class_symlinks()

So it seems we'd need an extension to the device core to support
class link opt-out.

> If we could drop those, we can relax the naming requirements so that
> the names is unique to a parent device, not globally unique.
> 
>     Andrew
> 
Heiner

Re: [PATCH net-next 5/5] igc: Export LEDs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

On Wed 2021-07-21 16:35:27, Andrew Lunn wrote:
> > Thanks for the hint, Andrew. If I make &netdev->dev the parent,
> > then I get:
> > 
> > ll /sys/class/leds/
> > total 0
> > lrwxrwxrwx 1 root root 0 Jul 20 21:37 led0 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led0
> > lrwxrwxrwx 1 root root 0 Jul 20 21:37 led1 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led1
> > lrwxrwxrwx 1 root root 0 Jul 20 21:37 led2 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led2
> > 
> > Now the (linked) LED devices are under /sys/class/net/<ifname>, but still
> > the primary LED devices are under /sys/class/leds and their names have
> > to be unique therefore. The LED subsystem takes care of unique names,
> > but in case of a second network interface the LED device name suddenly
> > would be led0_1 (IIRC). So the names wouldn't be predictable, and I think
> > that's not what we want.
> 
> We need input from the LED maintainers, but do we actually need the
> symbolic links in /sys/class/leds/? For this specific use case, not
> generally. Allow an LED to opt out of the /sys/class/leds symlink.
> 
> If we could drop those, we can relax the naming requirements so that
> the names is unique to a parent device, not globally unique.

Well, I believe we already negotiated acceptable naming with
Marek... Is it unsuitable for some reason?



-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH net-next 5/5] igc: Export LEDs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]

Hi!

> > > Now the (linked) LED devices are under /sys/class/net/<ifname>, but still
> > > the primary LED devices are under /sys/class/leds and their names have
> > > to be unique therefore. The LED subsystem takes care of unique names,
> > > but in case of a second network interface the LED device name suddenly
> > > would be led0_1 (IIRC). So the names wouldn't be predictable, and I think
> > > that's not what we want.
> > 
> > We need input from the LED maintainers, but do we actually need the
> > symbolic links in /sys/class/leds/? For this specific use case, not
> > generally. Allow an LED to opt out of the /sys/class/leds symlink.
> > 
> > If we could drop those, we can relax the naming requirements so that
> > the names is unique to a parent device, not globally unique.
> 
> Well, I believe we already negotiated acceptable naming with
> Marek... Is it unsuitable for some reason?

Sorry, hit send too soon.. Marek is now in cc list.

								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

On Tue, 20 Jul 2021 22:29:21 +0200
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> On 20.07.2021 17:42, Andrew Lunn wrote:
> >> I checked the LED subsystem and didn't find a way to place the LED
> >> sysfs files in a place other than /sys/class/leds. Maybe Pavel can
> >> comment on whether I just missed something.  
> > 
> > https://lwn.net/ml/linux-kernel/20200908000300.6982-1-marek.behun@nic.cz/
> > 
> > It comments the sys files appear under
> > /sys/class/net/<ifname>/phydev/leds/. phydev is a symbolic link to the
> > phy devices, provided by the phydev subsystem. So they are actually
> > attached to the PHY device. And this appears to be due to:
> > 
> > 	ret = devm_led_classdev_register_ext(&phydev->mdio.dev, &led->cdev, &init_data);
> > 
> > The LEDs are parented to the phy device. This has the nice side affect
> > that PHYs are not part of the network name space. You can rename the
> > interface, /sys/class/net/<ifname> changes, but the symbolic link
> > still points to the phy device.
> > 
> > When not using phydev, it probably gets trickier. You probably want
> > the LEDs parented to the PCI device, and you need to follow a
> > different symbolic link out of /sys/class/net/<ifname>/ to find the
> > LED.
> > 
> > There was talk of adding an ledtool, which knows about these
> > links. But i pushed for it to be added to ethtool. Until we get an
> > implementation actually merged, that is academic.
> >   
> >> For r8169 I'm facing a similar challenge like Kurt. Most family
> >> members support three LED's:
> >> - Per LED a mode 0 .. 15 can be set that defines which link speed(s)
> >>   and/or activity is indicated.
> >> - Period and duty cycle for blinking can be controlled, but this
> >>   setting applies to all three LED's.  
> > 
> > Cross LED settings is a problem. The Marvell PHYs have a number of
> > independent modes. Plus they have some shared modes which cross LEDs.
> > At the moment, there is no good model for these shared modes.
> > 
> > We have to look at the trade offs here. At the moment we have at least
> > 3 different ways of setting PHY LEDs via DT. Because each driver does
> > it its own way, it probably allows full access to all features. But it
> > is very unfriendly. Adopting Linux LEDs allows us to have a single
> > uniform API for all these PHY LEDs, and probably all MAC drivers which
> > don't use PHY drivers. But at the expense of probably not supporting
> > all features of the hardware. My opinion is, we should ignore some of
> > the hardware features in order to get a simple to use uniform
> > interface for all LEDs, which probably covers the features most people
> > are interested in anyway.
> >   
> 
> Thanks for the hint, Andrew. If I make &netdev->dev the parent,
> then I get:
> 
> ll /sys/class/leds/
> total 0
> lrwxrwxrwx 1 root root 0 Jul 20 21:37 led0 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led0
> lrwxrwxrwx 1 root root 0 Jul 20 21:37 led1 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led1
> lrwxrwxrwx 1 root root 0 Jul 20 21:37 led2 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led2
> 
> Now the (linked) LED devices are under /sys/class/net/<ifname>, but still
> the primary LED devices are under /sys/class/leds and their names have
> to be unique therefore. The LED subsystem takes care of unique names,
> but in case of a second network interface the LED device name suddenly
> would be led0_1 (IIRC). So the names wouldn't be predictable, and I think
> that's not what we want.
> We could use something like led0_<pci_id>, but then userspace would have
> to do echo foo > /sys/class/net/<ifname>/led0*/bar, and that's also not
> nice.

Hi Heiner,

in sysfs, all devices registered under LED class will have symlinks in
/sys/class/leds. This is how device classes work in Linux.

There is a standardized format for LED device names, please look at
Documentation/leds/leds-class.rst.

Basically the LED name is of the format
  devicename:color:function

The list of colors and functions is defined in
  include/dt-bindings/leds/common.h

The function part of the LED is also supposed to (in the future)
specify trigger, i.e. something like:
  if the function is "activity", the LED should blink on network
  activity
(Note that there is not yet a consensus. Jacek, for example, is of the
 opinion that the "activity" function should imply the CPU activity
 trigger. I think that "activity" function together with trigger-source
 defined to be a network device should imply network activity.)

Does your driver register the LEDs based on device-tree?

If so, then LED core will compose the name for the device for you.

If not, then you need to compose the name (in the above format)
yourself.

Are your LEDs controlled by an ethernet PHY, or by the MAC itself?

Marek

Re: [PATCH net-next 5/5] igc: Export LEDs

[Andrew Lunn]

> Hi Heiner,
> 
> in sysfs, all devices registered under LED class will have symlinks in
> /sys/class/leds. This is how device classes work in Linux.
> 
> There is a standardized format for LED device names, please look at
> Documentation/leds/leds-class.rst.
> 
> Basically the LED name is of the format
>   devicename:color:function

The interesting part here is, what does devicename mean, in this
context?

We cannot use the interface name, because it is not unique, and user
space can change it whenever it wants. So we probably need to build
something around the bus ID, e.g. pci_id. Which is not very friendly
:-(

	Andrew

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

On Wed, 21 Jul 2021 21:50:07 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > Hi Heiner,
> > 
> > in sysfs, all devices registered under LED class will have symlinks in
> > /sys/class/leds. This is how device classes work in Linux.
> > 
> > There is a standardized format for LED device names, please look at
> > Documentation/leds/leds-class.rst.
> > 
> > Basically the LED name is of the format
> >   devicename:color:function  
> 
> The interesting part here is, what does devicename mean, in this
> context?
> 
> We cannot use the interface name, because it is not unique, and user
> space can change it whenever it wants. So we probably need to build
> something around the bus ID, e.g. pci_id. Which is not very friendly
> :-(

Unfortunately there isn't consensus about what the devicename should
mean. There are two "schools of thought":

1. device name of the trigger source for the LED, i.e. if the LED
   blinks on activity on mmc0, the devicename should be mmc0. We have
   talked about this in the discussions about ethernet PHYs.
   In the case of the igc driver if the LEDs are controlled by the MAC,
   I guess some PCI identifier would be OK. Or maybe ethernet-mac
   identifier, if we have something like that? (Since we can't use
   interface names due to the possibility of renaming.)

   Pavel and I are supporters of this scheme.

2. device name of the LED controller. For example LEDs controlled by
   the maxim,max77650-led controller (leds-max77650.c) define device
   name as "max77650"

   Jacek supports this scheme.

The complication is that both these schemes are used already in
upstream kernel, and we have to maintain backwards compatibility of
sysfs ABI, so we can't change that.

I have been thinking for some time that maybe we should poll Linux
kernel developers about these two schemes, so that a consensus is
reached. Afterwards we can deprecate the other scheme and add a Kconfig
option (default n for backwards compatibility) to use the new scheme.

What do you think?

Marek

Re: [PATCH net-next 5/5] igc: Export LEDs

[Andrew Lunn]

> > > Basically the LED name is of the format
> > >   devicename:color:function  

> Unfortunately there isn't consensus about what the devicename should
> mean. There are two "schools of thought":
> 
> 1. device name of the trigger source for the LED, i.e. if the LED
>    blinks on activity on mmc0, the devicename should be mmc0. We have
>    talked about this in the discussions about ethernet PHYs.
>    In the case of the igc driver if the LEDs are controlled by the MAC,
>    I guess some PCI identifier would be OK.

I guess this is most likely for Ethernet LEDs, some sort of bus
identifier. But Ethernet makes use of all sorts of busses, so you will
also see USB, memory mapped for SOCs, MDIO, SPI, etc.

> 2. device name of the LED controller. For example LEDs controlled by
>    the maxim,max77650-led controller (leds-max77650.c) define device
>    name as "max77650"

And what happens when the controller is just a tiny bit of silicon in
the corner of something else, not a standalone device? Would this be
'igc', for LEDs controlled by the IGC Ethernet controller? 'mv88e6xxx'
for Marvell Ethernet switches? 

Also, function is totally unclear. The whole reason we want to use
Linux LEDs is triggers, and it is the selected trigger which
determines the function.

Colour is also an issue. The IGC Ethernet controller has no idea what
colour the LEDs are in the RG-45 socket. And this is generic to
Ethernet MAC and PHY chips. The data sheets never mention colour.  You
might know the colour in DT (and maybe ACPI) systems where you have
specific information about the board. But in for PCIe card, USB
dongles, etc, colour is unknown.

So very little of the naming scheme actually makes sense in this
context.

	 Andrew

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

On Wed, 21 Jul 2021 22:54:36 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > > > Basically the LED name is of the format
> > > >   devicename:color:function    
> 
> > Unfortunately there isn't consensus about what the devicename should
> > mean. There are two "schools of thought":
> > 
> > 1. device name of the trigger source for the LED, i.e. if the LED
> >    blinks on activity on mmc0, the devicename should be mmc0. We have
> >    talked about this in the discussions about ethernet PHYs.
> >    In the case of the igc driver if the LEDs are controlled by the MAC,
> >    I guess some PCI identifier would be OK.  
> 
> I guess this is most likely for Ethernet LEDs, some sort of bus
> identifier. But Ethernet makes use of all sorts of busses, so you will
> also see USB, memory mapped for SOCs, MDIO, SPI, etc.

That's why I think we should group them all under a name like ethmac0,
ethmac1, ... We want to do this for PHY controlled LEDs (ethphy0,
ethphy1, ...)

> > 2. device name of the LED controller. For example LEDs controlled by
> >    the maxim,max77650-led controller (leds-max77650.c) define device
> >    name as "max77650"  
> 
> And what happens when the controller is just a tiny bit of silicon in
> the corner of something else, not a standalone device? Would this be
> 'igc', for LEDs controlled by the IGC Ethernet controller? 'mv88e6xxx'
> for Marvell Ethernet switches? 

This is one of the reasons why I prefer the first scheme.

> Also, function is totally unclear. The whole reason we want to use
> Linux LEDs is triggers, and it is the selected trigger which
> determines the function.

As I said there are two "schools of thought" for this as well.
Devicetree deprecated the `linux,default-trigger` DT property and
`function` property should be used instead. Jacek's then defined some
function definition constants in include/dt-bindings/leds/common.h and
sent a proposal for function to trigger mappings
  https://lore.kernel.org/linux-leds/20200920162625.14754-1-jacek.anaszewski@gmail.com/
But this was not implemented, and I together with Pavel do not agree
with this proposal, and I proposed something different:
  https://lore.kernel.org/linux-leds/20200920184422.60c04194@nic.cz/
Since function to trigger mappings is not yet implemented in the code,
we can still decide.

Do you think I should a poll more kernel developers about their
opinions?

> Colour is also an issue. The IGC Ethernet controller has no idea what
> colour the LEDs are in the RG-45 socket. And this is generic to
> Ethernet MAC and PHY chips. The data sheets never mention colour.  You
> might know the colour in DT (and maybe ACPI) systems where you have
> specific information about the board. But in for PCIe card, USB
> dongles, etc, colour is unknown.

The LED core (function led_compose_name in drivers/leds/led-core.c)
skips color and function if they are not present in fwnode, i.e.
  "mmc0::"

I guess in the case of igc, if the color is not known, and if we can
agree on the first scheme for choosing the devicename part, then the
LED names could be, depending on the scheme for function, either
  "ethmac0::lan-0"
  "ethmac0::lan-1"
  "ethmac0::lan-2"
or
  "ethmac0::link"
  "ethmac0::activity"
  "ethmac0::rx"

(If there is color defined in ACPI / DTS though, it should be also
 used.)

So basically we need to decide on these two things:
- scheme for device name
- scheme for function to default trigger mappings

Marek

Re: [PATCH net-next 5/5] igc: Export LEDs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

Hi!

> Also, function is totally unclear. The whole reason we want to use
> Linux LEDs is triggers, and it is the selected trigger which
> determines the function.

Usually, yes. But "function" is what _manufacturer_ wanted LED to
mean, and "trigger" is how user is using it. I have currently disk
activity LED mapped on scrollock.

There are some mini desktops which have skull LED, probably meant to
be user defined LED. In such cases, user will have to select trigger.

> Colour is also an issue. The IGC Ethernet controller has no idea what
> colour the LEDs are in the RG-45 socket. And this is generic to
> Ethernet MAC and PHY chips. The data sheets never mention colour.

Maybe datasheet does not mention color, but the LED _has_ color.

> might know the colour in DT (and maybe ACPI) systems where you have
> specific information about the board. But in for PCIe card, USB
> dongles, etc, colour is unknown.

Not.. really. You don't know from chip specificiations, but you should
know the color as soon as you have USB IDs (because they should tell
you exact hardware). And I believe same is true for PCIe cards. It may
be more tricky if no actual PCIe card exists and it is just a chip
built into generic notebook. (But then, you should have notebook's DMI
id etc...?)

Anyway, you can leave the color field empty if you don't know.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH net-next 5/5] igc: Export LEDs

[Andrew Lunn]

On Thu, Jul 22, 2021 at 12:45:06AM +0200, Pavel Machek wrote:
> Hi!
> 
> > Also, function is totally unclear. The whole reason we want to use
> > Linux LEDs is triggers, and it is the selected trigger which
> > determines the function.
> 
> Usually, yes. But "function" is what _manufacturer_ wanted LED to
> mean

So you are saying the function should be the reset default of the PHY,
or MAC?

> > Colour is also an issue. The IGC Ethernet controller has no idea what
> > colour the LEDs are in the RG-45 socket. And this is generic to
> > Ethernet MAC and PHY chips. The data sheets never mention colour.
> 
> Maybe datasheet does not mention color, but the LED _has_ color.

Sure it does, but the driver is for the LED controller, not the
LED. The controller does not care what colour the LED is. At least for
this application.

> > might know the colour in DT (and maybe ACPI) systems where you have
> > specific information about the board. But in for PCIe card, USB
> > dongles, etc, colour is unknown.
> 
> Not.. really. You don't know from chip specificiations, but you should
> know the color as soon as you have USB IDs (because they should tell
> you exact hardware).

No. All it tells you is what the controller is. The dongle manufacture
can pair any RJ-45 socket with the controller, and it is the RJ-45
socket which provides the LED.

> And I believe same is true for PCIe cards.

Also not true. The PCIe IDs tell you the controller. What RJ-45 socket
is connected is up to the board manufacture.

> Anyway, you can leave the color field empty if you don't know.

That is the more likely case.

     Andrew

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

On Thu, 22 Jul 2021 03:45:21 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Jul 22, 2021 at 12:45:06AM +0200, Pavel Machek wrote:
> > Hi!
> >   
> > > Also, function is totally unclear. The whole reason we want to use
> > > Linux LEDs is triggers, and it is the selected trigger which
> > > determines the function.  
> > 
> > Usually, yes. But "function" is what _manufacturer_ wanted LED to
> > mean  
> 
> So you are saying the function should be the reset default of the PHY,
> or MAC?

Pavel is talking about the manufacturer of the whole device, not just
the controller. For example on Turris Omnia there are icons over the
LEDs indicating their function. There are other devices, for example
ethernet switches, with LEDs dedicated to indicate specific things, and
these do not necessarily have to be the same as the LED controller
default.

Of course the problem is that we are not always able to determine this
from kernel. In the case of ethernet PHYs I would not create LED
classdevs unless they are defined in DTS/ACPI. In the case of igc
I am not exactly sure if the driver should register these LEDs. What if
they the manufacturer did not connected LEDs to all 3 pins, but only 1,
for example? Or used the LED pin for some other funtion, like GPIO (if
igc supports it)? Do we want to create LED classdevs for potentially
nonexistent LEDs? If yes, then I guess the function should be the reset
default.

Marek

Re: [PATCH net-next 5/5] igc: Export LEDs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]

Hi!

> 2. device name of the LED controller. For example LEDs controlled by
>    the maxim,max77650-led controller (leds-max77650.c) define device
>    name as "max77650"

This one is deprecated. Please don't introduce new cases of it.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH net-next 5/5] igc: Export LEDs

[Florian Fainelli]

On 7/21/2021 1:07 PM, Marek Behún wrote:
> On Wed, 21 Jul 2021 21:50:07 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
>>> Hi Heiner,
>>>
>>> in sysfs, all devices registered under LED class will have symlinks in
>>> /sys/class/leds. This is how device classes work in Linux.
>>>
>>> There is a standardized format for LED device names, please look at
>>> Documentation/leds/leds-class.rst.
>>>
>>> Basically the LED name is of the format
>>>    devicename:color:function
>>
>> The interesting part here is, what does devicename mean, in this
>> context?
>>
>> We cannot use the interface name, because it is not unique, and user
>> space can change it whenever it wants. So we probably need to build
>> something around the bus ID, e.g. pci_id. Which is not very friendly
>> :-(
> 
> Unfortunately there isn't consensus about what the devicename should
> mean. There are two "schools of thought":
> 
> 1. device name of the trigger source for the LED, i.e. if the LED
>     blinks on activity on mmc0, the devicename should be mmc0. We have
>     talked about this in the discussions about ethernet PHYs.
>     In the case of the igc driver if the LEDs are controlled by the MAC,
>     I guess some PCI identifier would be OK. Or maybe ethernet-mac
>     identifier, if we have something like that? (Since we can't use
>     interface names due to the possibility of renaming.)
> 
>     Pavel and I are supporters of this scheme.
> 
> 2. device name of the LED controller. For example LEDs controlled by
>     the maxim,max77650-led controller (leds-max77650.c) define device
>     name as "max77650"
> 
>     Jacek supports this scheme.
> 
> The complication is that both these schemes are used already in
> upstream kernel, and we have to maintain backwards compatibility of
> sysfs ABI, so we can't change that.
> 
> I have been thinking for some time that maybe we should poll Linux
> kernel developers about these two schemes, so that a consensus is
> reached. Afterwards we can deprecate the other scheme and add a Kconfig
> option (default n for backwards compatibility) to use the new scheme.
> 
> What do you think?

FWIW, dev_name() should be the "devicename" from what you described 
above. This is foundational property for all devices that Linux 
registers that is used for logging, name spacing within /sys/, uniqe, 
ABI stable, etc. Anything different would be virtually impossible to 
maintain and would lead to ABI breakage down the road, guaranteed.

Yes it can be long (especially with PCI devices), and unfriendly, but 
hey, udev to the rescue then, rename based on user preferences.
-- 
Florian

Re: [PATCH net-next 5/5] igc: Export LEDs

[Jacek Anaszewski]

Hi Marek,

On 7/21/21 10:07 PM, Marek Behún wrote:
> On Wed, 21 Jul 2021 21:50:07 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
>>> Hi Heiner,
>>>
>>> in sysfs, all devices registered under LED class will have symlinks in
>>> /sys/class/leds. This is how device classes work in Linux.
>>>
>>> There is a standardized format for LED device names, please look at
>>> Documentation/leds/leds-class.rst.
>>>
>>> Basically the LED name is of the format
>>>    devicename:color:function
>>
>> The interesting part here is, what does devicename mean, in this
>> context?
>>
>> We cannot use the interface name, because it is not unique, and user
>> space can change it whenever it wants. So we probably need to build
>> something around the bus ID, e.g. pci_id. Which is not very friendly
>> :-(
> 
> Unfortunately there isn't consensus about what the devicename should
> mean. There are two "schools of thought":
> 
> 1. device name of the trigger source for the LED, i.e. if the LED
>     blinks on activity on mmc0, the devicename should be mmc0. We have
>     talked about this in the discussions about ethernet PHYs.
>     In the case of the igc driver if the LEDs are controlled by the MAC,
>     I guess some PCI identifier would be OK. Or maybe ethernet-mac
>     identifier, if we have something like that? (Since we can't use
>     interface names due to the possibility of renaming.)
> 
>     Pavel and I are supporters of this scheme.
> 
> 2. device name of the LED controller. For example LEDs controlled by
>     the maxim,max77650-led controller (leds-max77650.c) define device
>     name as "max77650"
> 
>     Jacek supports this scheme.

I believe you must have misinterpreted some my of my statements.
The whole effort behind LED naming unification was getting rid of
hardware device names in favour of Linux provided device names.
See e.g. the excerpt from Documentation/leds/leds-class.rst:

- devicename:
         it should refer to a unique identifier created by the kernel,
         like e.g. phyN for network devices or inputN for input devices, 
rather
         than to the hardware; the information related to the product 
and the bus
         to which given device is hooked is available in sysfs and can be
         retrieved using get_led_device_info.sh script from tools/leds; 
generally
         this section is expected mostly for LEDs that are somehow 
associated with
         other devices.


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

On Mon, 26 Jul 2021 19:42:04 +0200
Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:

> I believe you must have misinterpreted some my of my statements.
> The whole effort behind LED naming unification was getting rid of
> hardware device names in favour of Linux provided device names.

Hi Jacek,

sorry about that. I don't know how this could have happened :D

Marek

Re: [PATCH net-next 5/5] igc: Export LEDs

[Heiner Kallweit]

On 26.07.2021 19:42, Jacek Anaszewski wrote:
> Hi Marek,
> 
> On 7/21/21 10:07 PM, Marek Behún wrote:
>> On Wed, 21 Jul 2021 21:50:07 +0200
>> Andrew Lunn <andrew@lunn.ch> wrote:
>>
>>>> Hi Heiner,
>>>>
>>>> in sysfs, all devices registered under LED class will have symlinks in
>>>> /sys/class/leds. This is how device classes work in Linux.
>>>>
>>>> There is a standardized format for LED device names, please look at
>>>> Documentation/leds/leds-class.rst.
>>>>
>>>> Basically the LED name is of the format
>>>>    devicename:color:function
>>>
>>> The interesting part here is, what does devicename mean, in this
>>> context?
>>>
>>> We cannot use the interface name, because it is not unique, and user
>>> space can change it whenever it wants. So we probably need to build
>>> something around the bus ID, e.g. pci_id. Which is not very friendly
>>> :-(
>>
>> Unfortunately there isn't consensus about what the devicename should
>> mean. There are two "schools of thought":
>>
>> 1. device name of the trigger source for the LED, i.e. if the LED
>>     blinks on activity on mmc0, the devicename should be mmc0. We have
>>     talked about this in the discussions about ethernet PHYs.
>>     In the case of the igc driver if the LEDs are controlled by the MAC,
>>     I guess some PCI identifier would be OK. Or maybe ethernet-mac
>>     identifier, if we have something like that? (Since we can't use
>>     interface names due to the possibility of renaming.)
>>
>>     Pavel and I are supporters of this scheme.
>>
>> 2. device name of the LED controller. For example LEDs controlled by
>>     the maxim,max77650-led controller (leds-max77650.c) define device
>>     name as "max77650"
>>
>>     Jacek supports this scheme.
> 
> I believe you must have misinterpreted some my of my statements.
> The whole effort behind LED naming unification was getting rid of
> hardware device names in favour of Linux provided device names.
> See e.g. the excerpt from Documentation/leds/leds-class.rst:
> 
> - devicename:
>         it should refer to a unique identifier created by the kernel,
>         like e.g. phyN for network devices or inputN for input devices, rather
>         than to the hardware; the information related to the product and the bus
>         to which given device is hooked is available in sysfs and can be
>         retrieved using get_led_device_info.sh script from tools/leds; generally
>         this section is expected mostly for LEDs that are somehow associated with
>         other devices.
> 
> 
The issue with this is mentioned by Andrew a few lines before. At least in
network subsystem the kernel identifiers can be changed from userspace.
Typical example is the interface renaming from eth0 to e.g. enp1s0.
Then a LED eth0-led1 would have to be automagically renamed to enp1s0-led1.
An option for this could be to make a LED renaming function subscribe to
interface name change notifications. But this looks to me to like using a
quite big hammer for a rather small nail.

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

Hi Heiner (and Pavel and Florian and others),

On Mon, 26 Jul 2021 22:59:05 +0200
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> The issue with this is mentioned by Andrew a few lines before. At least in
> network subsystem the kernel identifiers can be changed from userspace.
> Typical example is the interface renaming from eth0 to e.g. enp1s0.
> Then a LED eth0-led1 would have to be automagically renamed to enp1s0-led1.
> An option for this could be to make a LED renaming function subscribe to
> interface name change notifications. But this looks to me to like using a
> quite big hammer for a rather small nail.

We are not going to be renaming LEDs on inteface rename, that can
introduce a set of problems on it's own.

Yes, if network interface renaming were not possible, the best option
would be to use interface names. It is not possible.

The last time we discussed this (Andrew, Pavel and I), we've decided
that for ethernet PHY controlled LEDs we want the devicename part
should be something like
   phyN  or  ethphyN  or  ethernet-phyN
with N a number unique for every PHY (a simple atomically increased
integer for every ethernet PHY). (This is because the LED pin is
physically connected to the PHY.)

But we can't do this here, because in the case of this igc driver, the
LEDs are controlled by the MAC, not the PHY (as far as I understand).
Which means that the LED is connected to a pin on the SOC or MAC chip.

Florian's suggestion is to use dev_name(), he says:
  FWIW, dev_name() should be the "devicename" from what you described 
  above. This is foundational property for all devices that Linux 
  registers that is used for logging, name spacing within /sys/, uniqe, 
  ABI stable, etc. Anything different would be virtually impossible to 
  maintain and would lead to ABI breakage down the road, guaranteed.

I understand this point of view, since the purpose of dev_name() is
to represent devices in userspace. And for the purpose of LED devicename
part it works beautifully sometimes, for block devices for example,
where the dev_name() is be mmc0, sda1, ...

The problem is that for other kind of devices dev_name() may contain the
':' character (and often it does), which can break parsing LED names,
since the LED name format also contains colons:
  devicename:color:function
So in the case of a block device, it works:
  mmc0::
  sda:red:read
  dm-0::write
But for a PCIe network controller it may contain too many colons:
  0000:02:00.0:yellow:activity

So basically this LED device naming scheme is the reason why we are
trying to make prettier names for LED trigger sources / controllers.

The possible solutions here are the following (the list is not
exhaustive):
- allow using devicenames containing ':' characters, i.e.
    0000:02:00.0:yellow:activity
  This can break existing userspace utilities (there are no official
  ones, though). But IMO it should be a viable solution since we can
  extract the devicename part, because we know that the color and
  function parts do not contain colons.

- substitute ':' characters with a different character in the devicename
  part

- use a prettier name, like we wanted to do with ethernet PHYs.

  The question is what do we want to do for MAC (instead of PHY)
  controlled LEDs, as is the case in this igc driver. We could introduce
  a simple atomically increased integer for every MAC, the same we want
  to do in the PHY case, so the devicename could be something like
  macN (or ethmacN or ethernet-macN)

I confess that I am growing a little frustrated here, because there
seems to be no optimal solution with given constraints and no official
consensus for a suboptimal yet acceptable solution.

Marek

Re: [PATCH net-next 5/5] igc: Export LEDs

[Andrew Lunn]

> The last time we discussed this (Andrew, Pavel and I), we've decided
> that for ethernet PHY controlled LEDs we want the devicename part
> should be something like
>    phyN  or  ethphyN  or  ethernet-phyN
> with N a number unique for every PHY (a simple atomically increased
> integer for every ethernet PHY).

We might want to rethink this. PHYs typically have 2 or 3 LEDs. So we
want a way to indicate which LED of a PHY it is. So i suspect we will
want something like

ethphyN-led0, ethphyN-led1, ethphyN-led2.

I would also suggest N starts at 42, in order to make it clear it is a
made up arbitrary number, it has no meaning other than it is
unique. What we don't want is people thinking ethphy0-led0 has
anything to do with eth0.

> I confess that I am growing a little frustrated here, because there
> seems to be no optimal solution with given constraints and no official
> consensus for a suboptimal yet acceptable solution.

I do think it is clear that the base name is mostly irrelevant and not
going to be used in any meaningful way. You are unlikely to access
these LEDs via /sys/class/leds. You are going to go into
/sys/class/net/<ifname> and then either follow the device symlink, or
the phydev symlink and look for LEDs there. And then only the -ledM
part of the name might be useful. Since the name is mostly
meaningless, we should just decide and move on.

     Andrew

Re: [PATCH net-next 5/5] igc: Export LEDs

[Michael Walle]

>> The last time we discussed this (Andrew, Pavel and I), we've decided
>> that for ethernet PHY controlled LEDs we want the devicename part
>> should be something like
>>    phyN  or  ethphyN  or  ethernet-phyN
>> with N a number unique for every PHY (a simple atomically increased
>> integer for every ethernet PHY).
>
> We might want to rethink this. PHYs typically have 2 or 3 LEDs. So we
> want a way to indicate which LED of a PHY it is. So i suspect we will
> want something like
>
> ethphyN-led0, ethphyN-led1, ethphyN-led2.
>
> I would also suggest N starts at 42, in order to make it clear it is a
> made up arbitrary number, it has no meaning other than it is
> unique. What we don't want is people thinking ethphy0-led0 has
> anything to do with eth0.

Why do we have to distiguish between LEDs connected to the PHY and LEDs
connected to the MAC at all? Why not just name it ethN either if its behind
the PHY or the MAC? Does it really matter from the users POV?

>> I confess that I am growing a little frustrated here, because there
>> seems to be no optimal solution with given constraints and no official
>> consensus for a suboptimal yet acceptable solution.
>
> I do think it is clear that the base name is mostly irrelevant and not
> going to be used in any meaningful way. You are unlikely to access
> these LEDs via /sys/class/leds. You are going to go into
> /sys/class/net/<ifname> and then either follow the device symlink, or
> the phydev symlink and look for LEDs there. And then only the -ledM
> part of the name might be useful. Since the name is mostly
> meaningless, we should just decide and move on.

Even more if it is not relevant ;)

-michael

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

Hi Michael,

On Tue, 27 Jul 2021 10:15:28 +0200
Michael Walle <michael@walle.cc> wrote:

> Why do we have to distiguish between LEDs connected to the PHY and LEDs
> connected to the MAC at all? Why not just name it ethN either if its behind
> the PHY or the MAC? Does it really matter from the users POV?

Because
1. network interfaces can be renamed
2. network interfaces can be moved between network namespaces. The LED
   subsystem is agnostic to network namespaces
So it can for example happen that within a network namespace you
have only one interface, eth0, but in /sys/class/leds you would see
  eth0:green:activity
  eth1:green:activity
So you would know that there are at least 2 network interfaces on the
system, and also with renaming it can happen that the first LED is not
in fact connected to the eth0 interface in your network namespace.

Marek

Re: [PATCH net-next 5/5] igc: Export LEDs

[Michael Walle]

Hi,

Am 2021-07-27 16:56, schrieb Marek Behún:
> On Tue, 27 Jul 2021 10:15:28 +0200
> Michael Walle <michael@walle.cc> wrote:
> 
>> Why do we have to distiguish between LEDs connected to the PHY and 
>> LEDs
>> connected to the MAC at all? Why not just name it ethN either if its 
>> behind
>> the PHY or the MAC? Does it really matter from the users POV?
> 
> Because
> 1. network interfaces can be renamed
> 2. network interfaces can be moved between network namespaces. The LED
>    subsystem is agnostic to network namespaces

I wasn't talking about ethN being same as the network interface name.
For clarity I'll use ethernetN now. My question was why would you
use ethmacN or ethphyN instead if just ethernetN for both. What is
the reason for having two different names? I'm not sure who is using
that name anyway. If it is for an user, I don't think he is interested
in knowing wether that LED is controlled by the PHY or by the MAC.

> So it can for example happen that within a network namespace you
> have only one interface, eth0, but in /sys/class/leds you would see
>   eth0:green:activity
>   eth1:green:activity
> So you would know that there are at least 2 network interfaces on the
> system, and also with renaming it can happen that the first LED is not
> in fact connected to the eth0 interface in your network namespace.

But the first problem persists wether its named ethernetN or ethphyN,
no?

-michael

Re: [PATCH net-next 5/5] igc: Export LEDs

[Andrew Lunn]

> I wasn't talking about ethN being same as the network interface name.
> For clarity I'll use ethernetN now. My question was why would you
> use ethmacN or ethphyN instead if just ethernetN for both. What is
> the reason for having two different names?

We can get into the situation where both the MAC and the PHY have LED
controllers. So we need to ensure the infrastructure we put in place
handles this, we don't get any name clashes.

	Andrew

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

Hi,

On Tue, 27 Jul 2021 17:03:53 +0200
Michael Walle <michael@walle.cc> wrote:

> I wasn't talking about ethN being same as the network interface name.
> For clarity I'll use ethernetN now. My question was why would you
> use ethmacN or ethphyN instead if just ethernetN for both. What is
> the reason for having two different names? I'm not sure who is using
> that name anyway. If it is for an user, I don't think he is interested
> in knowing wether that LED is controlled by the PHY or by the MAC.

Suppose that the system has 2 ethernet MACs, each with an attached PHY.
Each MAC-PHY pair has one LED, but one MAC-PHY pair has the LED
attached to the MAC, and the second pair has the LED attached to the
PHY:
     +------+        +------+
     | macA |        | macB |
     +-+--+-+        +-+----+
       |  |            |
      /   +------+   +-+----+
   ledA   | phyA |   | phyB |
          +------+   +----+-+
                          |
                           \
                            ledB

Now suppose that during system initialization the system enumerates
MACs and PHYs in different order:
   macA -> 0      phyA -> 1
   macB -> 1      phyB -> 0

If we used the devicename as you are suggesting, then for the two LEDs
the devicename part would be the same:
  ledA -> macA -> ethernet0
  ledB -> phyB -> ethernet0
although they are clearly on different MACs.

We could create a simple atomically increasing index only for MACs, and
for a LED connected to a PHY, instead of using the PHY's index, we
would look at the attached MAC and use the MAC index.
The problem is that PHYs and MACs are not always attached, and are not
necessarily mapped 1-to-1. It is possible to have a board where one PHY
can connect to 2 different MACs and you can switch between them, and
also vice versa.

> > So it can for example happen that within a network namespace you
> > have only one interface, eth0, but in /sys/class/leds you would see
> >   eth0:green:activity
> >   eth1:green:activity
> > So you would know that there are at least 2 network interfaces on the
> > system, and also with renaming it can happen that the first LED is not
> > in fact connected to the eth0 interface in your network namespace.  
> 
> But the first problem persists wether its named ethernetN or ethphyN,
> no?

No. The N in the "ethphyN" for etherent PHYs is supposed to be unrelated
to the N in "ethN" for interface names. So if you have eth0 network
interface with attached phy ethphy0, this is a coincidence. (That is
why Andrew is proposing to start the index for PHYs at a different
number, like 42.)

Marek

Re: [PATCH net-next 5/5] igc: Export LEDs

[Michael Walle]

Hi,

Am 2021-07-27 17:28, schrieb Marek Behún:
> On Tue, 27 Jul 2021 17:03:53 +0200
> Michael Walle <michael@walle.cc> wrote:
> 
>> I wasn't talking about ethN being same as the network interface name.
>> For clarity I'll use ethernetN now. My question was why would you
>> use ethmacN or ethphyN instead if just ethernetN for both. What is
>> the reason for having two different names? I'm not sure who is using
>> that name anyway. If it is for an user, I don't think he is interested
>> in knowing wether that LED is controlled by the PHY or by the MAC.
> 
> Suppose that the system has 2 ethernet MACs, each with an attached PHY.
> Each MAC-PHY pair has one LED, but one MAC-PHY pair has the LED
> attached to the MAC, and the second pair has the LED attached to the
> PHY:
>      +------+        +------+
>      | macA |        | macB |
>      +-+--+-+        +-+----+
>        |  |            |
>       /   +------+   +-+----+
>    ledA   | phyA |   | phyB |
>           +------+   +----+-+
>                           |
>                            \
>                             ledB
> 
> Now suppose that during system initialization the system enumerates
> MACs and PHYs in different order:
>    macA -> 0      phyA -> 1
>    macB -> 1      phyB -> 0
> 
> If we used the devicename as you are suggesting, then for the two LEDs
> the devicename part would be the same:
>   ledA -> macA -> ethernet0
>   ledB -> phyB -> ethernet0
> although they are clearly on different MACs.

Why is that the case? Why can't both the MAC and the PHY request a 
unique
name from the same namespace? As Andrew pointed out, the names in
/sys/class/leds don't really matter. Ok, it will still depend on the
probe order which might not be the case if you split it between ethmac
and ethphy.

Sorry, if I may ask stupid questions here. I don't want to cause much
trouble, here. I was just wondering why we have to make up two different
(totally unrelated names to the network interface names) instead of just
one (again totally unrelated to the interface name and index).

> We could create a simple atomically increasing index only for MACs, and
> for a LED connected to a PHY, instead of using the PHY's index, we
> would look at the attached MAC and use the MAC index.

Oh, I see. I was assuming we are talking about "just a number" not
related to anything.

> The problem is that PHYs and MACs are not always attached, and are not
> necessarily mapped 1-to-1. It is possible to have a board where one PHY
> can connect to 2 different MACs and you can switch between them, and
> also vice versa.
> 
>> > So it can for example happen that within a network namespace you
>> > have only one interface, eth0, but in /sys/class/leds you would see
>> >   eth0:green:activity
>> >   eth1:green:activity
>> > So you would know that there are at least 2 network interfaces on the
>> > system, and also with renaming it can happen that the first LED is not
>> > in fact connected to the eth0 interface in your network namespace.
>> 
>> But the first problem persists wether its named ethernetN or ethphyN,
>> no?
> 
> No. The N in the "ethphyN" for etherent PHYs is supposed to be 
> unrelated
> to the N in "ethN" for interface names. So if you have eth0 network
> interface with attached phy ethphy0, this is a coincidence. (That is
> why Andrew is proposing to start the index for PHYs at a different
> number, like 42.)

Yes, in my case ethernet0 has nothing to do with eth0, either.

But I was actually referring to your "you see the leds in /sys/ of all
the network adapters". That problem still persists, right?

-michael

Re: [PATCH net-next 5/5] igc: Export LEDs

[Andrew Lunn]

> But I was actually referring to your "you see the leds in /sys/ of all
> the network adapters". That problem still persists, right?

It is not really a problem. You see all the PHYs in /sys. You see all
the PCI devices in /sys. Because these all have globally unique names,
it is not a problem.

What is not globally unique is interface names. Those are only unique
to a network name space. And /sys/class/net is network name space
aware. It only contains the interfaces in the current network name
space.

	Andrew

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

Hi,

On Tue, 27 Jul 2021 17:53:58 +0200
Michael Walle <michael@walle.cc> wrote:

> > If we used the devicename as you are suggesting, then for the two LEDs
> > the devicename part would be the same:
> >   ledA -> macA -> ethernet0
> >   ledB -> phyB -> ethernet0
> > although they are clearly on different MACs.  
> 
> Why is that the case? Why can't both the MAC and the PHY request a 
> unique name from the same namespace?

So all the network related devices should request a unique network
relate device ID? Should also wireless PHY devices do this? WWAN modems?
And all these should have the same template for devicename part withing
/sys/class/leds? What should be the template for the devicename, if
wireless PHYs and WWAN modems could also be part of this? It cannot be
"ethernet" anymore.

It seems a better idea to me to just some nice identifier for the LED
controller.

> As Andrew pointed out, the names in
> /sys/class/leds don't really matter. Ok, it will still depend on the
> probe order which might not be the case if you split it between ethmac
> and ethphy.

Yes, the LED name does not matter. But the LED subsystem requires names
in a specific format, this is already decided and documented, we are
not going to be changing this. The only reasonable thing we can do now
is to choose a sane devicename.

> Sorry, if I may ask stupid questions here. I don't want to cause much
> trouble, here. I was just wondering why we have to make up two different
> (totally unrelated names to the network interface names) instead of just
> one (again totally unrelated to the interface name and index).

It seems more logical to me from kernel's point of view.

> But I was actually referring to your "you see the leds in /sys/ of all
> the network adapters". That problem still persists, right?

Yes, this still persists. But we really do not want to start
introducing namespaces to the LED subsystem.

Marek

Re: [PATCH net-next 5/5] igc: Export LEDs

[Andrew Lunn]

> Yes, this still persists. But we really do not want to start
> introducing namespaces to the LED subsystem.

Agreed. LED names need to be globally unique, so we don't need to
worry about network name spaces.

      Andrew

Re: [PATCH net-next 5/5] igc: Export LEDs

[Michael Walle]

Am 2021-07-27 18:32, schrieb Marek Behún:
> On Tue, 27 Jul 2021 17:53:58 +0200
> Michael Walle <michael@walle.cc> wrote:
> 
>> > If we used the devicename as you are suggesting, then for the two LEDs
>> > the devicename part would be the same:
>> >   ledA -> macA -> ethernet0
>> >   ledB -> phyB -> ethernet0
>> > although they are clearly on different MACs.
>> 
>> Why is that the case? Why can't both the MAC and the PHY request a
>> unique name from the same namespace?
> 
> So all the network related devices should request a unique network
> relate device ID?  Should also wireless PHY devices do this? WWAN 
> modems?
> And all these should have the same template for devicename part withing
> /sys/class/leds? What should be the template for the devicename, if
> wireless PHYs and WWAN modems could also be part of this? It cannot be
> "ethernet" anymore.

I don't suggest using ethernet for everything. I just questioned
wether the distinction between ethmac and ethphy makes any sense from
a (human) user point of view; if the devicename part is visible to an
user at all.

-michael

Re: [PATCH net-next 5/5] igc: Export LEDs

[Heiner Kallweit]

On 27.07.2021 18:32, Marek Behún wrote:
> Hi,
> 
> On Tue, 27 Jul 2021 17:53:58 +0200
> Michael Walle <michael@walle.cc> wrote:
> 
>>> If we used the devicename as you are suggesting, then for the two LEDs
>>> the devicename part would be the same:
>>>   ledA -> macA -> ethernet0
>>>   ledB -> phyB -> ethernet0
>>> although they are clearly on different MACs.  
>>
>> Why is that the case? Why can't both the MAC and the PHY request a 
>> unique name from the same namespace?
> 
> So all the network related devices should request a unique network
> relate device ID? Should also wireless PHY devices do this? WWAN modems?
> And all these should have the same template for devicename part withing
> /sys/class/leds? What should be the template for the devicename, if
> wireless PHYs and WWAN modems could also be part of this? It cannot be
> "ethernet" anymore.
> 
> It seems a better idea to me to just some nice identifier for the LED
> controller.
> 
>> As Andrew pointed out, the names in
>> /sys/class/leds don't really matter. Ok, it will still depend on the
>> probe order which might not be the case if you split it between ethmac
>> and ethphy.
> 
> Yes, the LED name does not matter. But the LED subsystem requires names
> in a specific format, this is already decided and documented, we are
> not going to be changing this. The only reasonable thing we can do now
> is to choose a sane devicename.
> 
>> Sorry, if I may ask stupid questions here. I don't want to cause much
>> trouble, here. I was just wondering why we have to make up two different
>> (totally unrelated names to the network interface names) instead of just
>> one (again totally unrelated to the interface name and index).
> 
> It seems more logical to me from kernel's point of view.
> 
>> But I was actually referring to your "you see the leds in /sys/ of all
>> the network adapters". That problem still persists, right?
> 
> Yes, this still persists. But we really do not want to start
> introducing namespaces to the LED subsystem.
> 
> Marek
> 

Did we come to any conclusion?

My preliminary r8169 implementation now creates the following LED names:

lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led0-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led0-0300
lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led1-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led1-0300
lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led2-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led2-0300

I understood that LEDs should at least be renamed to r8169-0300::link-0
to link-2 Is this correct? Or do we have to wait with any network LED support
for a name discussion outcome?

For the different LED modes I defined private hw triggers (using trigger_type
to make the triggers usable with r8169 LEDs only). The trigger attribute now
looks like this:

[none] link_10_100 link_1000 link_10_100_1000 link_ACT link_10_100_ACT link_1000_ACT link_10_100_1000_ACT

Nice, or? Issue is just that these trigger names really should be made a
standard for all network LEDs. I don't care about the exact naming, important
is just that trigger names are the same, no matter whether it's about a r8169-
or igc- or whatever network chip controlled LEDs.

And I don't have a good solution for initialization yet. LED mode is whatever
BIOS sets, but initial trigger value is "none". I would have to read the
initial LED control register values, iterate over the triggers to find the
matching one, and call led_trigger_set() to properly set this trigger as
current trigger. Most likely this would need some LED core extensions:
- enable iterating over all triggers with a particular trigger_type
- enable triggers to have private data
Quite some hassle for a small functionality.

Re: [PATCH net-next 5/5] igc: Export LEDs

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 2122 bytes --]

On Wed Jul 28 2021, Heiner Kallweit wrote:
> Did we come to any conclusion?
>
> My preliminary r8169 implementation now creates the following LED
> names:

Is that implementation somewhere available?

>
> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led0-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led0-0300
> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led1-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led1-0300
> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led2-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led2-0300
>
> I understood that LEDs should at least be renamed to r8169-0300::link-0
> to link-2 Is this correct? Or do we have to wait with any network LED support
> for a name discussion outcome?
>
> For the different LED modes I defined private hw triggers (using trigger_type
> to make the triggers usable with r8169 LEDs only). The trigger attribute now
> looks like this:
>
> [none] link_10_100 link_1000 link_10_100_1000 link_ACT link_10_100_ACT link_1000_ACT link_10_100_1000_ACT
>
> Nice, or? Issue is just that these trigger names really should be made a
> standard for all network LEDs. I don't care about the exact naming, important
> is just that trigger names are the same, no matter whether it's about a r8169-
> or igc- or whatever network chip controlled LEDs.

Your above trigger definitions are OK for the igc as well. Except it
supports up to 2500 Mbit/s. For igc there's also more triggers available
such as filter activity, paused and spd mode.

However, what about the cross LED settings which are common to all LEDs?
The igc has one attribute to control the blink rate of all three LEDs.

>
> And I don't have a good solution for initialization yet. LED mode is whatever
> BIOS sets, but initial trigger value is "none". I would have to read the
> initial LED control register values, iterate over the triggers to find the
> matching one, and call led_trigger_set() to properly set this trigger as
> current trigger.

Yes, there needs to be a way to determine the default state.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

Hello Heiner,

On Wed, 28 Jul 2021 22:43:30 +0200
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> Did we come to any conclusion?
> 
> My preliminary r8169 implementation now creates the following LED names:
> 
> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led0-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led0-0300
> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led1-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led1-0300
> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led2-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led2-0300
> 
> I understood that LEDs should at least be renamed to r8169-0300::link-0
> to link-2 Is this correct? Or do we have to wait with any network LED support
> for a name discussion outcome?

I would expect some of the LEDs to, by default, indicate activity.
So maybe look at the settings BIOS left, and if the setting is to
indicate link, use the "link" function, and if activity, use the
"activity" function? 

> For the different LED modes I defined private hw triggers (using trigger_type
> to make the triggers usable with r8169 LEDs only). The trigger attribute now
> looks like this:
> 
> [none] link_10_100 link_1000 link_10_100_1000 link_ACT link_10_100_ACT link_1000_ACT link_10_100_1000_ACT
>
> Nice, or? Issue is just that these trigger names really should be made a
> standard for all network LEDs. I don't care about the exact naming, important
> is just that trigger names are the same, no matter whether it's about a r8169-
> or igc- or whatever network chip controlled LEDs.

This is how I at first proposed doing this, last year. But this is
WRONG!

First, we do not want a different trigger for each possible
configuration. We want one trigger, and then choose configuration via
other sysfs file. I.e. a "hw" trigger, which, when activated, would
create sysfs files "link" and "act", via which you can configure those
options.

Second, we already have a standard LED trigger for network devices,
netdev! So what we should do is use the netdev trigger, and offload
blinking to the LED controller if it supports it. The problems with
this are:
1. not yet implemented in upstream, see my latest version
   https://lore.kernel.org/linux-leds/20210601005155.27997-1-kabel@kernel.org/
2. netdev trigger currently does not support all these different link
   functions. We have these settings:
     device_name: network interface name, i.e. eth0
     link: 0 - do not indicate link
           1 - indicate link (ON when linked)
     tx: 0 - do not blink on transmit
         1 - blink on transmit
     rx: 0 - do not blink on receive
         1 - blink on receive
     interval: duration of LED blink in ms

I would like to extend netdev trigger to support different
configurations. Currently my ideas are as follows:
- a new sysfs file, "advanced", will show up when netdev trigger is
  enabled (and if support is compiled in)
- when advanced is set to 1, for each possible link mode (10base-t,
  100base-t, 1000base-t, ...) a new sysfs directory will show up, and
  in each of these directories the following files:
    rx, tx, link, interval, brightness
    multi_intensity (if the LED is a multi-color LED)
  and possibly even
    pattern
With this, the user can configure more complicated configurations:
- different LED color for different link speeds
- different blink speed for different link speeds
And if some of the configurations are offloadable to the HW, the drivers
can be written to support such offloading. (Maybe even add a read-only
file "offloaded" to indicate if the trigger was offloaded.)

I will work on these ideas in the following weeks and will sent
proposals to linux-leds.

> And I don't have a good solution for initialization yet. LED mode is whatever
> BIOS sets, but initial trigger value is "none". I would have to read the
> initial LED control register values, iterate over the triggers to find the
> matching one, and call led_trigger_set() to properly set this trigger as
> current trigger.

You can set led_cdev->default_trigger prior registering the LED. But
this is moot: we do not want a different trigger for each PHY interface
mode.

Marek

Re: [PATCH net-next 5/5] igc: Export LEDs

[Heiner Kallweit]

On 29.07.2021 10:59, Marek Behún wrote:
> Hello Heiner,
> 
> On Wed, 28 Jul 2021 22:43:30 +0200
> Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 
>> Did we come to any conclusion?
>>
>> My preliminary r8169 implementation now creates the following LED names:
>>
>> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led0-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led0-0300
>> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led1-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led1-0300
>> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led2-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led2-0300
>>
>> I understood that LEDs should at least be renamed to r8169-0300::link-0
>> to link-2 Is this correct? Or do we have to wait with any network LED support
>> for a name discussion outcome?
> 
> I would expect some of the LEDs to, by default, indicate activity.
> So maybe look at the settings BIOS left, and if the setting is to
> indicate link, use the "link" function, and if activity, use the
> "activity" function? 
> 

The function may be changed by the user. Then what? Rename the LED device?
A typical use case is also that one LED indicates both, link and activity.

>> For the different LED modes I defined private hw triggers (using trigger_type
>> to make the triggers usable with r8169 LEDs only). The trigger attribute now
>> looks like this:
>>
>> [none] link_10_100 link_1000 link_10_100_1000 link_ACT link_10_100_ACT link_1000_ACT link_10_100_1000_ACT
>>
>> Nice, or? Issue is just that these trigger names really should be made a
>> standard for all network LEDs. I don't care about the exact naming, important
>> is just that trigger names are the same, no matter whether it's about a r8169-
>> or igc- or whatever network chip controlled LEDs.
> 
> This is how I at first proposed doing this, last year. But this is
> WRONG!
> 
> First, we do not want a different trigger for each possible
> configuration. We want one trigger, and then choose configuration via
> other sysfs file. I.e. a "hw" trigger, which, when activated, would
> create sysfs files "link" and "act", via which you can configure those
> options.
> 
> Second, we already have a standard LED trigger for network devices,
> netdev! So what we should do is use the netdev trigger, and offload
> blinking to the LED controller if it supports it. The problems with
> this are:
> 1. not yet implemented in upstream, see my latest version
>    https://lore.kernel.org/linux-leds/20210601005155.27997-1-kabel@kernel.org/
> 2. netdev trigger currently does not support all these different link
>    functions. We have these settings:
>      device_name: network interface name, i.e. eth0
>      link: 0 - do not indicate link
>            1 - indicate link (ON when linked)
>      tx: 0 - do not blink on transmit
>          1 - blink on transmit
>      rx: 0 - do not blink on receive
>          1 - blink on receive
>      interval: duration of LED blink in ms
> 
> I would like to extend netdev trigger to support different
> configurations. Currently my ideas are as follows:
> - a new sysfs file, "advanced", will show up when netdev trigger is
>   enabled (and if support is compiled in)
> - when advanced is set to 1, for each possible link mode (10base-t,
>   100base-t, 1000base-t, ...) a new sysfs directory will show up, and

This leads to new questions like: How do you know what the possible
link modes are? In a spare minute you could have a look at enum
ethtool_link_mode_bit_indices. Even with standard multi-gig hw
meanwhile you have: 10M, 100M, 1G, 2.5G, 5G, 10G.
Supposedly the information about possible link modes would have to be
stored in led_classdev so that it can generate the appropriate sysfs
directories during registration.

>   in each of these directories the following files:
>     rx, tx, link, interval, brightness
>     multi_intensity (if the LED is a multi-color LED)
>   and possibly even
>     pattern
> With this, the user can configure more complicated configurations:
> - different LED color for different link speeds
> - different blink speed for different link speeds
> And if some of the configurations are offloadable to the HW, the drivers
> can be written to support such offloading. (Maybe even add a read-only
> file "offloaded" to indicate if the trigger was offloaded.)
> 

For a fully hw-offloaded LED like in my case then more or less the only
benefit of led_classdev + netdev trigger is the unified location of
link speed + tx/rx attributes. The brightness attribute has no meaning
because brightness can't be controlled.
Overall quite some overhead for a small functionality. At least in a
simple case like mine I'd use custom attributes under the net_dev like
this if I had to invent something on my own:
led0/speed: where you can say: "echo +100 > led0/speed" to enable 100M link indication
led0/activity: bool

> I will work on these ideas in the following weeks and will sent
> proposals to linux-leds.
> 

I don't want to be the one always saying: Nice framework, but heh:
How about my special case xyz?

I understand that it can be a frustrating job, that needs quite some
patience, to create a framework that you consider to be clean and
that covers the needs of (almost) everybody. I failed with some early
attempts to establish RGB LED support using the HSV color model.

>> And I don't have a good solution for initialization yet. LED mode is whatever
>> BIOS sets, but initial trigger value is "none". I would have to read the
>> initial LED control register values, iterate over the triggers to find the
>> matching one, and call led_trigger_set() to properly set this trigger as
>> current trigger.
> 
> You can set led_cdev->default_trigger prior registering the LED. But
> this is moot: we do not want a different trigger for each PHY interface
> mode.
> 
> Marek
> 

Re: [PATCH net-next 5/5] igc: Export LEDs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

Hi!

> > Yes, this still persists. But we really do not want to start
> > introducing namespaces to the LED subsystem.
> 
> Did we come to any conclusion?
> 
> My preliminary r8169 implementation now creates the following LED names:
> 
> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led0-0300 ->
> > ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led0-0300

So "r8159-0300:green:activity" would be closer to the naming we want,
but lets not do that, we really want this to be similar to what others
are doing, and that probably means "ethphy3:green:activity" AFAICT.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

On Tue, 10 Aug 2021 19:29:27 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> So "r8159-0300:green:activity" would be closer to the naming we want,
> but lets not do that, we really want this to be similar to what others
> are doing, and that probably means "ethphy3:green:activity" AFAICT.

Pavel, one point of the discussion is that in this case the LED is
controlled by MAC, not PHY. So the question is whether we want to do
"ethmacN" (in addition to "ethphyN").

Marek

Re: [PATCH net-next 5/5] igc: Export LEDs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

Hi!

> > So "r8159-0300:green:activity" would be closer to the naming we want,
> > but lets not do that, we really want this to be similar to what others
> > are doing, and that probably means "ethphy3:green:activity" AFAICT.
> 
> Pavel, one point of the discussion is that in this case the LED is
> controlled by MAC, not PHY. So the question is whether we want to do
> "ethmacN" (in addition to "ethphyN").

Sorry, I missed that. I guess that yes, ethmacX is okay, too.

Even better would be to find common term that could be used for both
ethmacN and ethphyN and just use that. (Except that we want to avoid
ethX). Maybe "ethportX" would be suitable?

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

On Tue, 10 Aug 2021 21:53:35 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> > Pavel, one point of the discussion is that in this case the LED is
> > controlled by MAC, not PHY. So the question is whether we want to do
> > "ethmacN" (in addition to "ethphyN").  
> 
> Sorry, I missed that. I guess that yes, ethmacX is okay, too.
> 
> Even better would be to find common term that could be used for both
> ethmacN and ethphyN and just use that. (Except that we want to avoid
> ethX). Maybe "ethportX" would be suitable?

See
  https://lore.kernel.org/linux-leds/YQAlPrF2uu3Gr+0d@lunn.ch/
and
  https://lore.kernel.org/linux-leds/20210727172828.1529c764@thinkpad/

Marek

Re: [PATCH net-next 5/5] igc: Export LEDs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

On Tue 2021-08-10 22:53:53, Marek Behún wrote:
> On Tue, 10 Aug 2021 21:53:35 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > > Pavel, one point of the discussion is that in this case the LED is
> > > controlled by MAC, not PHY. So the question is whether we want to do
> > > "ethmacN" (in addition to "ethphyN").  
> > 
> > Sorry, I missed that. I guess that yes, ethmacX is okay, too.
> > 
> > Even better would be to find common term that could be used for both
> > ethmacN and ethphyN and just use that. (Except that we want to avoid
> > ethX). Maybe "ethportX" would be suitable?
> 
> See
>   https://lore.kernel.org/linux-leds/YQAlPrF2uu3Gr+0d@lunn.ch/
> and
>   https://lore.kernel.org/linux-leds/20210727172828.1529c764@thinkpad/

Ok, I guess I'd preffer all LEDs corresponding to one port to be
grouped, but that may be hard to do.

Best regards,
							Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

On Tue, 17 Aug 2021 21:02:42 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> On Tue 2021-08-10 22:53:53, Marek Behún wrote:
> > On Tue, 10 Aug 2021 21:53:35 +0200
> > Pavel Machek <pavel@ucw.cz> wrote:
> >   
> > > > Pavel, one point of the discussion is that in this case the LED
> > > > is controlled by MAC, not PHY. So the question is whether we
> > > > want to do "ethmacN" (in addition to "ethphyN").    
> > > 
> > > Sorry, I missed that. I guess that yes, ethmacX is okay, too.
> > > 
> > > Even better would be to find common term that could be used for
> > > both ethmacN and ethphyN and just use that. (Except that we want
> > > to avoid ethX). Maybe "ethportX" would be suitable?  
> > 
> > See
> >   https://lore.kernel.org/linux-leds/YQAlPrF2uu3Gr+0d@lunn.ch/
> > and
> >   https://lore.kernel.org/linux-leds/20210727172828.1529c764@thinkpad/
> >  
> 
> Ok, I guess I'd preffer all LEDs corresponding to one port to be
> grouped, but that may be hard to do.

Hi Pavel (and Andrew),

The thing is that normally we are creating LED classdevs when the
parent device is probed. In this case when the PHY is probed. But we
will know the corresponding MAC only once the PHY is attached to it's
network interface.

Also, a PHY may be theoretically attached to multiple different
interfaces during it's lifetime. I guess there isn't many boards
currently that have such a configuration wired (maybe none), but
kernel's API is prepared for this.

So we really can't group them under one parent device, unless:

- we create LED classdevs only after PHY is attached (which will make
  us unable to blink the LEDs when the PHY is not attached yet) and
  destroy them when PHY is detached. I find this solution wrong - the
  LEDs will be unavailable for example if the MAC driver fails to probe
  for some reason

- or we add a mechanism to reprobe LEDs upon request (which also seems
  a rather unsatisfactory solution to me...)

- maybe some other solution?

Marek

Re: [PATCH net-next 5/5] igc: Export LEDs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

Hi!

> > > > > Pavel, one point of the discussion is that in this case the LED
> > > > > is controlled by MAC, not PHY. So the question is whether we
> > > > > want to do "ethmacN" (in addition to "ethphyN").    
> > > > 
> > > > Sorry, I missed that. I guess that yes, ethmacX is okay, too.
> > > > 
> > > > Even better would be to find common term that could be used for
> > > > both ethmacN and ethphyN and just use that. (Except that we want
> > > > to avoid ethX). Maybe "ethportX" would be suitable?  
> > > 
> > > See
> > >   https://lore.kernel.org/linux-leds/YQAlPrF2uu3Gr+0d@lunn.ch/
> > > and
> > >   https://lore.kernel.org/linux-leds/20210727172828.1529c764@thinkpad/
> > >  
> > 
> > Ok, I guess I'd preffer all LEDs corresponding to one port to be
> > grouped, but that may be hard to do.
> 
> Hi Pavel (and Andrew),
> 
> The thing is that normally we are creating LED classdevs when the
> parent device is probed. In this case when the PHY is probed. But we
> will know the corresponding MAC only once the PHY is attached to it's
> network interface.
> 
> Also, a PHY may be theoretically attached to multiple different
> interfaces during it's lifetime. I guess there isn't many boards
> currently that have such a configuration wired (maybe none), but
> kernel's API is prepared for this.
> 
> So we really can't group them under one parent device, unless:

Ok, I guess my proposal is just too complex to implement. Let's go
with "ethmacN" + "ethphyN".

Best regards,

								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH net-next 5/5] igc: Export LEDs

[Heiner Kallweit]

On 10.08.2021 19:29, Pavel Machek wrote:
> Hi!
> 
>>> Yes, this still persists. But we really do not want to start
>>> introducing namespaces to the LED subsystem.
>>
>> Did we come to any conclusion?
>>
>> My preliminary r8169 implementation now creates the following LED names:
>>
>> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led0-0300 ->
>>> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led0-0300
> 
> So "r8159-0300:green:activity" would be closer to the naming we want,

The LED ports in the network chip are multi-function. You can select activity
or link or both. Supposedly renaming the device on function switch is not an
attractive option. Any proposal on how to handle this?

Also the NIC has no clue about the color of the LEDs in the RJ45 port.
Realtek network chips driven by r8169 can be found on basically every
consumer mainboard. Determining LED color would need hundreds of DMI
match entries and would be a maintenance nightmare.
So color would need to be empty?

> but lets not do that, we really want this to be similar to what others
> are doing, and that probably means "ethphy3:green:activity" AFAICT.
> 
A challenge here would be unique numbering if there are multiple
network interfaces with LED support (especially if the interfaces
use different drivers). So the numbering service would have to be
in LED subsystem core or network subsystem core.

> Best regards,
> 								Pavel
> 

Re: [PATCH net-next 5/5] igc: Export LEDs

[Andrew Lunn]

> A challenge here would be unique numbering if there are multiple
> network interfaces with LED support (especially if the interfaces
> use different drivers). So the numbering service would have to be
> in LED subsystem core or network subsystem core.

Yes, it needs to be somewhere common.

We also need to document that the number is meaningless and
arbitrary. It can change from boot to boot. Also, LEDs from the same
PHY or MAC are not guaranteed to be contiguous, since multiple
PHYs/MACs can be enumerating their LEDs at the same time.

     Andrew

Re: [PATCH net-next 5/5] igc: Export LEDs

[Marek Behún]

Hi Andrew,

On Tue, 27 Jul 2021 03:57:13 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > The last time we discussed this (Andrew, Pavel and I), we've decided
> > that for ethernet PHY controlled LEDs we want the devicename part
> > should be something like
> >    phyN  or  ethphyN  or  ethernet-phyN
> > with N a number unique for every PHY (a simple atomically increased
> > integer for every ethernet PHY).  
> 
> We might want to rethink this. PHYs typically have 2 or 3 LEDs. So we
> want a way to indicate which LED of a PHY it is. So i suspect we will
> want something like
> 
> ethphyN-led0, ethphyN-led1, ethphyN-led2.

But... there is still color and function and possibly function-numerator
to differentiate them. I was talking only about the devicename part. So
for three LEDs you can have, for example:
  ethphyN:green:link
  ethphyN:yellow:activity
Even if you don't have information about color, the default function
(on chip reset) should be different. And even if it is not, the
function enumerator would fix this:
  ethphyN::link-1
  ethphyN::link-2

Marek

Documentation for naming LEDs was Re: [PATCH net-next 5/5] igc: Export LEDs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3383 bytes --]

Hi!

> > > The last time we discussed this (Andrew, Pavel and I), we've decided
> > > that for ethernet PHY controlled LEDs we want the devicename part
> > > should be something like
> > >    phyN  or  ethphyN  or  ethernet-phyN
> > > with N a number unique for every PHY (a simple atomically increased
> > > integer for every ethernet PHY).  
> > 
> > We might want to rethink this. PHYs typically have 2 or 3 LEDs. So we
> > want a way to indicate which LED of a PHY it is. So i suspect we will
> > want something like
> > 
> > ethphyN-led0, ethphyN-led1, ethphyN-led2.
> 
> But... there is still color and function and possibly function-numerator
> to differentiate them. I was talking only about the devicename part. So
> for three LEDs you can have, for example:
>   ethphyN:green:link
>   ethphyN:yellow:activity

For the record, this is the solution I'd like to see. Plus, we do want
it consistent between drivers, and I believe we need more than
list of functions provided by dt-bindings/leds/common.h -- we want
people to set something reasonable for "device" part, too.

Thus, I'm proposing this:

Rules will be simple -- when new type of LED is added to the
system, it will need to come with documentation explaining what the
LED does, and people will be expected to use existing names when
possible.

We'll also want to list "known bad" names in the file, so that there's
central place to search for aliases.

Thoughts?

Best regards,
								Pavel

--- /dev/null	2021-08-08 09:30:15.272028621 +0200
+++ Documentation/leds/well-known-leds.txt	2021-07-03 14:33:45.573718655 +0200
@@ -0,0 +1,57 @@
+-*- org -*-
+
+It is somehow important to provide consistent interface to the
+userland. LED devices have one problem there, and that is naming of
+directories in /sys/class/leds. It would be nice if userland would
+just know right "name" for given LED function, but situation got more
+complex.
+
+Anyway, if backwards compatibility is not an issue, new code should
+use one of the "good" names from this list, and you should extend the
+list where applicable.
+
+Bad names are listed, too; in case you are writing application that
+wants to use particular feature, you should probe for good name, first,
+but then try the bad ones, too.
+
+* Keyboards
+  
+Good: "input*:*:capslock"
+Good: "input*:*:scrolllock"
+Good: "input*:*:numlock"
+Bad: "shift-key-light" (Motorola Droid 4, capslock)
+
+Set of common keyboard LEDs, going back to PC AT or so.
+
+Good: "platform::kbd_backlight"
+Bad: "tpacpi::thinklight" (IBM/Lenovo Thinkpads)
+Bad: "lp5523:kb{1,2,3,4,5,6}" (Nokia N900)
+
+Frontlight/backlight of main keyboard.
+
+Bad: "button-backlight" (Motorola Droid 4)
+
+Some phones have touch buttons below screen; it is different from main
+keyboard. And this is their backlight.
+
+* Sound subsystem
+
+Good: "platform:*:mute"
+Good: "platform:*:micmute"
+
+LEDs on notebook body, indicating that sound input / output is muted.
+
+* System notification
+
+Good: "status-led:{red,green,blue}" (Motorola Droid 4)
+Bad: "lp5523:{r,g,b}" (Nokia N900)
+
+Phones usually have multi-color status LED.
+
+* Power management
+
+Good: "platform:*:charging" (allwinner sun50i)
+
+* Screen
+
+Good: ":backlight" (Motorola Droid 4)


-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH net-next 5/5] igc: Export LEDs

[Jesse Brandeburg]

On 7/16/2021 2:24 PM, Tony Nguyen wrote:
> From: Kurt Kanzenbach <kurt@linutronix.de>
>
> Each i225 has three LEDs. Export them via the LED class framework.
>
> Each LED is controllable via sysfs. Example:
>
> $ cd /sys/class/leds/igc_led0
> $ cat brightness      # Current Mode
> $ cat max_brightness  # 15
> $ echo 0 > brightness # Mode 0
> $ echo 1 > brightness # Mode 1
>
> The brightness field here reflects the different LED modes ranging
> from 0 to 15.
>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Tested-by: Dvora Fuxbrumer <dvorax.fuxbrumer@linux.intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

Just a few hours ago, Kurt sent a revert for this patch, we should just 
drop it from this series.

Re: [PATCH net-next 5/5] igc: Export LEDs

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

On Mon Jul 19 2021, Jesse Brandeburg wrote:
> On 7/16/2021 2:24 PM, Tony Nguyen wrote:
>> From: Kurt Kanzenbach <kurt@linutronix.de>
>>
>> Each i225 has three LEDs. Export them via the LED class framework.
>>
>> Each LED is controllable via sysfs. Example:
>>
>> $ cd /sys/class/leds/igc_led0
>> $ cat brightness      # Current Mode
>> $ cat max_brightness  # 15
>> $ echo 0 > brightness # Mode 0
>> $ echo 1 > brightness # Mode 1
>>
>> The brightness field here reflects the different LED modes ranging
>> from 0 to 15.
>>
>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> Tested-by: Dvora Fuxbrumer <dvorax.fuxbrumer@linux.intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>
> Just a few hours ago, Kurt sent a revert for this patch, we should just 
> drop it from this series.

Yes, I'll revisit this patch with the feedback received. Drop it for now.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next 0/5][pull request] 1GbE Intel Wired LAN Driver Updates 2021-07-16

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Fri, 16 Jul 2021 14:24:22 -0700 you wrote:
> Vinicius Costa Gomes says:
> 
> Add support for steering traffic to specific RX queues using Flex Filters.
> 
> As the name implies, Flex Filters are more flexible than using
> Layer-2, VLAN or MAC address filters, one of the reasons is that they
> allow "AND" operations more easily, e.g. when the user wants to steer
> some traffic based on the source MAC address and the packet ethertype.
> 
> [...]

Here is the summary with links:
  - [net-next,1/5] igc: Add possibility to add flex filter
    https://git.kernel.org/netdev/net-next/c/6574631b50ed
  - [net-next,2/5] igc: Integrate flex filter into ethtool ops
    https://git.kernel.org/netdev/net-next/c/2b477d057e33
  - [net-next,3/5] igc: Allow for Flex Filters to be installed
    https://git.kernel.org/netdev/net-next/c/7991487ecb2d
  - [net-next,4/5] igc: Make flex filter more flexible
    https://git.kernel.org/netdev/net-next/c/73744262210c
  - [net-next,5/5] igc: Export LEDs
    https://git.kernel.org/netdev/net-next/c/cf8331825a8d

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 0/5][pull request] 1GbE Intel Wired LAN Driver Updates 2021-07-16

[Andrew Lunn]

On Sat, Jul 17, 2021 at 12:30:05AM +0000, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
> 
> This series was applied to netdev/net-next.git (refs/heads/master):

FYI: Depending on the reply to my question, i could be going to NACK
the last patch.

Please could we slow down the merging patches where there are
outstanding questions.

	    Andrew