[net-next PATCH v3 0/3] net: add define to describe link speed modes

[net-next PATCH v3 0/3] net: add define to describe link speed modes

[Christian Marangi]

This is a simple series to add a way to describe link speed modes number.

An additional helper is added and the phy_speeds is better documented
and expanded to return just the modes number.
Documentation on the additional helper is not added to the phy.h as
suggested from another patch where in double documentation define .c is
preferred.

This is also needed in the upcoming changes in the netdev trigger for LEDs
where phy_speeds functions is used to declare a more compact array instead
of using a "big enough" approach.

Changes v3:
- Fix various compilation error (wrong revision pushed)
Changes v2:
- Drop stupid enum-define hack
- Introduce helper function
- Document phy_speeds function
- Extent phy_speeds function

Christian Marangi (3):
  net: phy: refactor and better document phy_speeds function
  net: phy: add simple helper to return count of supported speeds
  net: phy: led: dynamically allocate speed modes array

 drivers/net/phy/phy-core.c         | 50 +++++++++++++++++++++++++++---
 drivers/net/phy/phy.c              | 12 +++++++
 drivers/net/phy/phy_led_triggers.c | 16 ++++++++--
 include/linux/phy.h                |  2 ++
 4 files changed, 72 insertions(+), 8 deletions(-)

-- 
2.40.1

[net-next PATCH v3 1/3] net: phy: refactor and better document phy_speeds function

[Christian Marangi]

Refactor the phy_speeds function to be more readable and understandable
and add some documentation on it.

While on it extend it to take NULL speeds values to make it return only
the count of speed modes in the passed mask.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phy-core.c | 50 ++++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 966c93cbe616..9618d89458d1 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -317,17 +317,57 @@ phy_lookup_setting(int speed, int duplex, const unsigned long *mask, bool exact)
 }
 EXPORT_SYMBOL_GPL(phy_lookup_setting);
 
+/**
+ * phy_speeds - return all speeds in mask
+ * @speeds: pointer to array where to put the speed modes
+ * @size: size of array where to put the speed modes
+ * @mask: mask of speed modes to compare with
+ *
+ * Take mask, test bit in mask with the settings table and compose the
+ * speeds array based on that as many as size permits.
+ *
+ * With speeds NULL, only the number of detected modes is returned and
+ * no array is composed. (size value is ignored)
+ *
+ * Return the number of detected modes in mask.
+ */
 size_t phy_speeds(unsigned int *speeds, size_t size,
 		  unsigned long *mask)
 {
+	unsigned int curr_speed;
 	size_t count;
 	int i;
 
-	for (i = 0, count = 0; i < ARRAY_SIZE(settings) && count < size; i++)
-		if (settings[i].bit < __ETHTOOL_LINK_MODE_MASK_NBITS &&
-		    test_bit(settings[i].bit, mask) &&
-		    (count == 0 || speeds[count - 1] != settings[i].speed))
-			speeds[count++] = settings[i].speed;
+	for (i = 0, count = 0; i < ARRAY_SIZE(settings); i++) {
+		/* Inconsistent mapping with ethtool modes? */
+		if (unlikely(settings[i].bit >= __ETHTOOL_LINK_MODE_MASK_NBITS))
+			return count;
+
+		/* Skip. Speed not in provided mask */
+		if (!test_bit(settings[i].bit, mask))
+			continue;
+
+		/* settings struct is set in descending order with
+		 * ordered speed modes. Detect a new speed mode by
+		 * checking if it's different than the current one.
+		 */
+		if (count == 0 || curr_speed != settings[i].speed) {
+			curr_speed = settings[i].speed;
+
+			/* With speeds not declared, we return only
+			 * the number of detected speed mode in the mask.
+			 */
+			if (speeds) {
+				/* No more space to put new modes */
+				if (count > size)
+					return count;
+
+				speeds[count] = curr_speed;
+			}
+
+			count++;
+		}
+	}
 
 	return count;
 }
-- 
2.40.1

Re: [net-next PATCH v3 1/3] net: phy: refactor and better document phy_speeds function

[Randy Dunlap]

Hi--

On 12/15/23 05:29, Christian Marangi wrote:
> Refactor the phy_speeds function to be more readable and understandable
> and add some documentation on it.
> 
> While on it extend it to take NULL speeds values to make it return only
> the count of speed modes in the passed mask.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/phy/phy-core.c | 50 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> index 966c93cbe616..9618d89458d1 100644
> --- a/drivers/net/phy/phy-core.c
> +++ b/drivers/net/phy/phy-core.c
> @@ -317,17 +317,57 @@ phy_lookup_setting(int speed, int duplex, const unsigned long *mask, bool exact)
>  }
>  EXPORT_SYMBOL_GPL(phy_lookup_setting);
>  
> +/**
> + * phy_speeds - return all speeds in mask
> + * @speeds: pointer to array where to put the speed modes
> + * @size: size of array where to put the speed modes
> + * @mask: mask of speed modes to compare with
> + *
> + * Take mask, test bit in mask with the settings table and compose the
> + * speeds array based on that as many as size permits.
> + *
> + * With speeds NULL, only the number of detected modes is returned and
> + * no array is composed. (size value is ignored)
> + *
> + * Return the number of detected modes in mask.

    * Return: the number of detected modes in mask.


> + */


-- 
#Randy
https://people.kernel.org/tglx/notes-about-netiquette
https://subspace.kernel.org/etiquette.html

[net-next PATCH v3 2/3] net: phy: add simple helper to return count of supported speeds

[Christian Marangi]

Add simple helper to return count of supported speeds for the passed PHY
device.

This can be useful to know the number of speed modes to dynamically
allocate a speed array for it.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phy.c | 12 ++++++++++++
 include/linux/phy.h   |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index a5fa077650e8..311560e72126 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -229,6 +229,18 @@ phy_find_valid(int speed, int duplex, unsigned long *supported)
 	return phy_lookup_setting(speed, duplex, supported, false);
 }
 
+/**
+ * phy_supported_speeds_num - return the number of all speeds currently
+ *			      supported by a phy device
+ * @phy: The phy device to return supported speeds of.
+ *
+ * Description: Returns the number of supported speeds.
+ */
+unsigned int phy_supported_speeds_num(struct phy_device *phy)
+{
+	return phy_speeds(NULL, 0, phy->supported);
+}
+
 /**
  * phy_supported_speeds - return all speeds currently supported by a phy device
  * @phy: The phy device to return supported speeds of.
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3cc52826f18e..52aa415fab0f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -202,6 +202,8 @@ static inline void phy_interface_set_rgmii(unsigned long *intf)
 	__set_bit(PHY_INTERFACE_MODE_RGMII_TXID, intf);
 }
 
+unsigned int phy_supported_speeds_num(struct phy_device *phy);
+
 /*
  * phy_supported_speeds - return all speeds currently supported by a PHY device
  */
-- 
2.40.1

Re: [net-next PATCH v3 2/3] net: phy: add simple helper to return count of supported speeds

[Randy Dunlap]

On 12/15/23 05:29, Christian Marangi wrote:
> Add simple helper to return count of supported speeds for the passed PHY
> device.
> 
> This can be useful to know the number of speed modes to dynamically
> allocate a speed array for it.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/phy/phy.c | 12 ++++++++++++
>  include/linux/phy.h   |  2 ++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index a5fa077650e8..311560e72126 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -229,6 +229,18 @@ phy_find_valid(int speed, int duplex, unsigned long *supported)
>  	return phy_lookup_setting(speed, duplex, supported, false);
>  }
>  
> +/**
> + * phy_supported_speeds_num - return the number of all speeds currently
> + *			      supported by a phy device
> + * @phy: The phy device to return supported speeds of.
> + *
> + * Description: Returns the number of supported speeds.

For kernel-doc, better to have that line as:

 * Returns: the number of supported speeds.



> + */
> +unsigned int phy_supported_speeds_num(struct phy_device *phy)
> +{
> +	return phy_speeds(NULL, 0, phy->supported);
> +}
> +
>  /**
>   * phy_supported_speeds - return all speeds currently supported by a phy device
>   * @phy: The phy device to return supported speeds of.
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 3cc52826f18e..52aa415fab0f 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -202,6 +202,8 @@ static inline void phy_interface_set_rgmii(unsigned long *intf)
>  	__set_bit(PHY_INTERFACE_MODE_RGMII_TXID, intf);
>  }
>  
> +unsigned int phy_supported_speeds_num(struct phy_device *phy);
> +
>  /*
>   * phy_supported_speeds - return all speeds currently supported by a PHY device
>   */

-- 
#Randy
https://people.kernel.org/tglx/notes-about-netiquette
https://subspace.kernel.org/etiquette.html

[net-next PATCH v3 3/3] net: phy: led: dynamically allocate speed modes array

[Christian Marangi]

Instead of defining a big enough array for speed modes use the newly
introduced API to get the actual number of supported speed modes and
dynamically allocate them.

Allocated space is freed at the end of the LED register loop.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phy_led_triggers.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy_led_triggers.c b/drivers/net/phy/phy_led_triggers.c
index f550576eb9da..3b962ec13d90 100644
--- a/drivers/net/phy/phy_led_triggers.c
+++ b/drivers/net/phy/phy_led_triggers.c
@@ -83,14 +83,21 @@ static void phy_led_trigger_unregister(struct phy_led_trigger *plt)
 
 int phy_led_triggers_register(struct phy_device *phy)
 {
+	unsigned int *speeds;
 	int i, err;
-	unsigned int speeds[50];
 
-	phy->phy_num_led_triggers = phy_supported_speeds(phy, speeds,
-							 ARRAY_SIZE(speeds));
+	phy->phy_num_led_triggers = phy_supported_speeds_num(phy);
 	if (!phy->phy_num_led_triggers)
 		return 0;
 
+	speeds = kmalloc_array(phy->phy_num_led_triggers, sizeof(*speeds),
+			       GFP_KERNEL);
+	if (!speeds)
+		return -ENOMEM;
+
+	/* Presence of speed modes already checked up */
+	phy_supported_speeds(phy, speeds, phy->phy_num_led_triggers);
+
 	phy->led_link_trigger = devm_kzalloc(&phy->mdio.dev,
 					     sizeof(*phy->led_link_trigger),
 					     GFP_KERNEL);
@@ -123,6 +130,8 @@ int phy_led_triggers_register(struct phy_device *phy)
 	phy->last_triggered = NULL;
 	phy_led_trigger_change_speed(phy);
 
+	kfree(speeds);
+
 	return 0;
 out_unreg:
 	while (i--)
@@ -134,6 +143,7 @@ int phy_led_triggers_register(struct phy_device *phy)
 	devm_kfree(&phy->mdio.dev, phy->led_link_trigger);
 	phy->led_link_trigger = NULL;
 out_clear:
+	kfree(speeds);
 	phy->phy_num_led_triggers = 0;
 	return err;
 }
-- 
2.40.1