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

[net-next PATCH v2 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.

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 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 ++++++++--
 3 files changed, 70 insertions(+), 8 deletions(-)

-- 
2.40.1

[net-next PATCH v2 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

[net-next PATCH v2 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 ++++++++++++
 1 file changed, 12 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.
-- 
2.40.1

[net-next PATCH v2 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..2e41d10e323c 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);
 
+	free(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:
+	free(speeds);
 	phy->phy_num_led_triggers = 0;
 	return err;
 }
-- 
2.40.1

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

[kernel test robot]

Hi Christian,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/net-phy-refactor-and-better-document-phy_speeds-function/20231215-064112
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231214154906.29436-3-ansuelsmth%40gmail.com
patch subject: [net-next PATCH v2 2/3] net: phy: add simple helper to return count of supported speeds
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20231215/202312152053.bbqS93v4-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231215/202312152053.bbqS93v4-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312152053.bbqS93v4-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/phy/phy.c:239:14: warning: no previous prototype for 'phy_supported_speeds_num' [-Wmissing-prototypes]
     239 | unsigned int phy_supported_speeds_num(struct phy_device *phy)
         |              ^~~~~~~~~~~~~~~~~~~~~~~~


vim +/phy_supported_speeds_num +239 drivers/net/phy/phy.c

   231	
   232	/**
   233	 * phy_supported_speeds_num - return the number of all speeds currently
   234	 *			      supported by a phy device
   235	 * @phy: The phy device to return supported speeds of.
   236	 *
   237	 * Description: Returns the number of supported speeds.
   238	 */
 > 239	unsigned int phy_supported_speeds_num(struct phy_device *phy)
   240	{
   241		return phy_speeds(NULL, 0, phy->supported);
   242	}
   243	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Christian,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/net-phy-refactor-and-better-document-phy_speeds-function/20231215-064112
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231214154906.29436-4-ansuelsmth%40gmail.com
patch subject: [net-next PATCH v2 3/3] net: phy: led: dynamically allocate speed modes array
config: arm-randconfig-002-20231215 (https://download.01.org/0day-ci/archive/20231215/202312152038.v9NZyBxd-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231215/202312152038.v9NZyBxd-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312152038.v9NZyBxd-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/net/phy/phy_led_triggers.c:89:30: error: call to undeclared function 'phy_supported_speeds_num'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      89 |         phy->phy_num_led_triggers = phy_supported_speeds_num(phy);
         |                                     ^
   drivers/net/phy/phy_led_triggers.c:89:30: note: did you mean 'phy_supported_speeds'?
   include/linux/phy.h:208:14: note: 'phy_supported_speeds' declared here
     208 | unsigned int phy_supported_speeds(struct phy_device *phy,
         |              ^
>> drivers/net/phy/phy_led_triggers.c:133:2: error: call to undeclared library function 'free' with type 'void (void *)'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     133 |         free(speeds);
         |         ^
   drivers/net/phy/phy_led_triggers.c:133:2: note: include the header <stdlib.h> or explicitly provide a declaration for 'free'
   2 errors generated.


vim +/phy_supported_speeds_num +89 drivers/net/phy/phy_led_triggers.c

    83	
    84	int phy_led_triggers_register(struct phy_device *phy)
    85	{
    86		unsigned int *speeds;
    87		int i, err;
    88	
  > 89		phy->phy_num_led_triggers = phy_supported_speeds_num(phy);
    90		if (!phy->phy_num_led_triggers)
    91			return 0;
    92	
    93		speeds = kmalloc_array(phy->phy_num_led_triggers, sizeof(*speeds),
    94				       GFP_KERNEL);
    95		if (!speeds)
    96			return -ENOMEM;
    97	
    98		/* Presence of speed modes already checked up */
    99		phy_supported_speeds(phy, speeds, phy->phy_num_led_triggers);
   100	
   101		phy->led_link_trigger = devm_kzalloc(&phy->mdio.dev,
   102						     sizeof(*phy->led_link_trigger),
   103						     GFP_KERNEL);
   104		if (!phy->led_link_trigger) {
   105			err = -ENOMEM;
   106			goto out_clear;
   107		}
   108	
   109		err = phy_led_trigger_register(phy, phy->led_link_trigger, 0, "link");
   110		if (err)
   111			goto out_free_link;
   112	
   113		phy->phy_led_triggers = devm_kcalloc(&phy->mdio.dev,
   114						    phy->phy_num_led_triggers,
   115						    sizeof(struct phy_led_trigger),
   116						    GFP_KERNEL);
   117		if (!phy->phy_led_triggers) {
   118			err = -ENOMEM;
   119			goto out_unreg_link;
   120		}
   121	
   122		for (i = 0; i < phy->phy_num_led_triggers; i++) {
   123			err = phy_led_trigger_register(phy, &phy->phy_led_triggers[i],
   124						       speeds[i],
   125						       phy_speed_to_str(speeds[i]));
   126			if (err)
   127				goto out_unreg;
   128		}
   129	
   130		phy->last_triggered = NULL;
   131		phy_led_trigger_change_speed(phy);
   132	
 > 133		free(speeds);
   134	
   135		return 0;
   136	out_unreg:
   137		while (i--)
   138			phy_led_trigger_unregister(&phy->phy_led_triggers[i]);
   139		devm_kfree(&phy->mdio.dev, phy->phy_led_triggers);
   140	out_unreg_link:
   141		phy_led_trigger_unregister(phy->led_link_trigger);
   142	out_free_link:
   143		devm_kfree(&phy->mdio.dev, phy->led_link_trigger);
   144		phy->led_link_trigger = NULL;
   145	out_clear:
   146		free(speeds);
   147		phy->phy_num_led_triggers = 0;
   148		return err;
   149	}
   150	EXPORT_SYMBOL_GPL(phy_led_triggers_register);
   151	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Christian,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/net-phy-refactor-and-better-document-phy_speeds-function/20231215-064112
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231214154906.29436-4-ansuelsmth%40gmail.com
patch subject: [net-next PATCH v2 3/3] net: phy: led: dynamically allocate speed modes array
config: arm-randconfig-003-20231215 (https://download.01.org/0day-ci/archive/20231215/202312152044.iRWfyQXJ-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231215/202312152044.iRWfyQXJ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312152044.iRWfyQXJ-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/net/phy/phy_led_triggers.c:89:30: error: implicit declaration of function 'phy_supported_speeds_num' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           phy->phy_num_led_triggers = phy_supported_speeds_num(phy);
                                       ^
   drivers/net/phy/phy_led_triggers.c:89:30: note: did you mean 'phy_supported_speeds'?
   include/linux/phy.h:208:14: note: 'phy_supported_speeds' declared here
   unsigned int phy_supported_speeds(struct phy_device *phy,
                ^
>> drivers/net/phy/phy_led_triggers.c:133:2: error: implicitly declaring library function 'free' with type 'void (void *)' [-Werror,-Wimplicit-function-declaration]
           free(speeds);
           ^
   drivers/net/phy/phy_led_triggers.c:133:2: note: include the header <stdlib.h> or explicitly provide a declaration for 'free'
   2 errors generated.


vim +/phy_supported_speeds_num +89 drivers/net/phy/phy_led_triggers.c

    83	
    84	int phy_led_triggers_register(struct phy_device *phy)
    85	{
    86		unsigned int *speeds;
    87		int i, err;
    88	
  > 89		phy->phy_num_led_triggers = phy_supported_speeds_num(phy);
    90		if (!phy->phy_num_led_triggers)
    91			return 0;
    92	
    93		speeds = kmalloc_array(phy->phy_num_led_triggers, sizeof(*speeds),
    94				       GFP_KERNEL);
    95		if (!speeds)
    96			return -ENOMEM;
    97	
    98		/* Presence of speed modes already checked up */
    99		phy_supported_speeds(phy, speeds, phy->phy_num_led_triggers);
   100	
   101		phy->led_link_trigger = devm_kzalloc(&phy->mdio.dev,
   102						     sizeof(*phy->led_link_trigger),
   103						     GFP_KERNEL);
   104		if (!phy->led_link_trigger) {
   105			err = -ENOMEM;
   106			goto out_clear;
   107		}
   108	
   109		err = phy_led_trigger_register(phy, phy->led_link_trigger, 0, "link");
   110		if (err)
   111			goto out_free_link;
   112	
   113		phy->phy_led_triggers = devm_kcalloc(&phy->mdio.dev,
   114						    phy->phy_num_led_triggers,
   115						    sizeof(struct phy_led_trigger),
   116						    GFP_KERNEL);
   117		if (!phy->phy_led_triggers) {
   118			err = -ENOMEM;
   119			goto out_unreg_link;
   120		}
   121	
   122		for (i = 0; i < phy->phy_num_led_triggers; i++) {
   123			err = phy_led_trigger_register(phy, &phy->phy_led_triggers[i],
   124						       speeds[i],
   125						       phy_speed_to_str(speeds[i]));
   126			if (err)
   127				goto out_unreg;
   128		}
   129	
   130		phy->last_triggered = NULL;
   131		phy_led_trigger_change_speed(phy);
   132	
 > 133		free(speeds);
   134	
   135		return 0;
   136	out_unreg:
   137		while (i--)
   138			phy_led_trigger_unregister(&phy->phy_led_triggers[i]);
   139		devm_kfree(&phy->mdio.dev, phy->phy_led_triggers);
   140	out_unreg_link:
   141		phy_led_trigger_unregister(phy->led_link_trigger);
   142	out_free_link:
   143		devm_kfree(&phy->mdio.dev, phy->led_link_trigger);
   144		phy->led_link_trigger = NULL;
   145	out_clear:
   146		free(speeds);
   147		phy->phy_num_led_triggers = 0;
   148		return err;
   149	}
   150	EXPORT_SYMBOL_GPL(phy_led_triggers_register);
   151	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Christian Marangi]

On Fri, Dec 15, 2023 at 08:50:28PM +0800, kernel test robot wrote:
> Hi Christian,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on net-next/main]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/net-phy-refactor-and-better-document-phy_speeds-function/20231215-064112
> base:   net-next/main
> patch link:    https://lore.kernel.org/r/20231214154906.29436-4-ansuelsmth%40gmail.com
> patch subject: [net-next PATCH v2 3/3] net: phy: led: dynamically allocate speed modes array
> config: arm-randconfig-002-20231215 (https://download.01.org/0day-ci/archive/20231215/202312152038.v9NZyBxd-lkp@intel.com/config)
> compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231215/202312152038.v9NZyBxd-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202312152038.v9NZyBxd-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
> >> drivers/net/phy/phy_led_triggers.c:89:30: error: call to undeclared function 'phy_supported_speeds_num'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>       89 |         phy->phy_num_led_triggers = phy_supported_speeds_num(phy);
>          |                                     ^
>    drivers/net/phy/phy_led_triggers.c:89:30: note: did you mean 'phy_supported_speeds'?
>    include/linux/phy.h:208:14: note: 'phy_supported_speeds' declared here
>      208 | unsigned int phy_supported_speeds(struct phy_device *phy,
>          |              ^
> >> drivers/net/phy/phy_led_triggers.c:133:2: error: call to undeclared library function 'free' with type 'void (void *)'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>      133 |         free(speeds);
>          |         ^
>    drivers/net/phy/phy_led_triggers.c:133:2: note: include the header <stdlib.h> or explicitly provide a declaration for 'free'
>    2 errors generated.
> 
> 
> vim +/phy_supported_speeds_num +89 drivers/net/phy/phy_led_triggers.c
> 
>     83	
>     84	int phy_led_triggers_register(struct phy_device *phy)
>     85	{
>     86		unsigned int *speeds;
>     87		int i, err;
>     88	
>   > 89		phy->phy_num_led_triggers = phy_supported_speeds_num(phy);
>     90		if (!phy->phy_num_led_triggers)
>     91			return 0;
>     92	
>     93		speeds = kmalloc_array(phy->phy_num_led_triggers, sizeof(*speeds),
>     94				       GFP_KERNEL);
>     95		if (!speeds)
>     96			return -ENOMEM;
>     97	
>     98		/* Presence of speed modes already checked up */
>     99		phy_supported_speeds(phy, speeds, phy->phy_num_led_triggers);
>    100	
>    101		phy->led_link_trigger = devm_kzalloc(&phy->mdio.dev,
>    102						     sizeof(*phy->led_link_trigger),
>    103						     GFP_KERNEL);
>    104		if (!phy->led_link_trigger) {
>    105			err = -ENOMEM;
>    106			goto out_clear;
>    107		}
>    108	
>    109		err = phy_led_trigger_register(phy, phy->led_link_trigger, 0, "link");
>    110		if (err)
>    111			goto out_free_link;
>    112	
>    113		phy->phy_led_triggers = devm_kcalloc(&phy->mdio.dev,
>    114						    phy->phy_num_led_triggers,
>    115						    sizeof(struct phy_led_trigger),
>    116						    GFP_KERNEL);
>    117		if (!phy->phy_led_triggers) {
>    118			err = -ENOMEM;
>    119			goto out_unreg_link;
>    120		}
>    121	
>    122		for (i = 0; i < phy->phy_num_led_triggers; i++) {
>    123			err = phy_led_trigger_register(phy, &phy->phy_led_triggers[i],
>    124						       speeds[i],
>    125						       phy_speed_to_str(speeds[i]));
>    126			if (err)
>    127				goto out_unreg;
>    128		}
>    129	
>    130		phy->last_triggered = NULL;
>    131		phy_led_trigger_change_speed(phy);
>    132	
>  > 133		free(speeds);
>    134	
>    135		return 0;
>    136	out_unreg:
>    137		while (i--)
>    138			phy_led_trigger_unregister(&phy->phy_led_triggers[i]);
>    139		devm_kfree(&phy->mdio.dev, phy->phy_led_triggers);
>    140	out_unreg_link:
>    141		phy_led_trigger_unregister(phy->led_link_trigger);
>    142	out_free_link:
>    143		devm_kfree(&phy->mdio.dev, phy->led_link_trigger);
>    144		phy->led_link_trigger = NULL;
>    145	out_clear:
>    146		free(speeds);
>    147		phy->phy_num_led_triggers = 0;
>    148		return err;
>    149	}
>    150	EXPORT_SYMBOL_GPL(phy_led_triggers_register);
>    151	
>

Ugh didn't think that LEDs netdev trigger doesn't have a dependency on
PHY...

Andrew any idea about this?

I can see 2 solution (or maybe 3???):

- Add the dependency for PHY
- Move phy_speeds net_utils.c (with the settings table moved there)
- Implement a custom function in ledtrig-netdev.c

It's sad since the phy_speed was just what we needed to implement this
ins a ""clean way"".

-- 
	Ansuel

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

[Russell King (Oracle)]

On Sun, Dec 17, 2023 at 02:12:58AM +0100, Christian Marangi wrote:
> On Fri, Dec 15, 2023 at 08:50:28PM +0800, kernel test robot wrote:
> > Hi Christian,
> > 
> > kernel test robot noticed the following build errors:
> > 
> > [auto build test ERROR on net-next/main]
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/net-phy-refactor-and-better-document-phy_speeds-function/20231215-064112
> > base:   net-next/main
> > patch link:    https://lore.kernel.org/r/20231214154906.29436-4-ansuelsmth%40gmail.com
> > patch subject: [net-next PATCH v2 3/3] net: phy: led: dynamically allocate speed modes array
> > config: arm-randconfig-002-20231215 (https://download.01.org/0day-ci/archive/20231215/202312152038.v9NZyBxd-lkp@intel.com/config)
> > compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231215/202312152038.v9NZyBxd-lkp@intel.com/reproduce)
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202312152038.v9NZyBxd-lkp@intel.com/
> > 
> > All errors (new ones prefixed by >>):
> > 
> > >> drivers/net/phy/phy_led_triggers.c:89:30: error: call to undeclared function 'phy_supported_speeds_num'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
> >       89 |         phy->phy_num_led_triggers = phy_supported_speeds_num(phy);
> >          |                                     ^
> >    drivers/net/phy/phy_led_triggers.c:89:30: note: did you mean 'phy_supported_speeds'?
> >    include/linux/phy.h:208:14: note: 'phy_supported_speeds' declared here
> >      208 | unsigned int phy_supported_speeds(struct phy_device *phy,
> >          |              ^
> > >> drivers/net/phy/phy_led_triggers.c:133:2: error: call to undeclared library function 'free' with type 'void (void *)'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
> >      133 |         free(speeds);
> >          |         ^
> >    drivers/net/phy/phy_led_triggers.c:133:2: note: include the header <stdlib.h> or explicitly provide a declaration for 'free'
> >    2 errors generated.
> > 
> > 
> > vim +/phy_supported_speeds_num +89 drivers/net/phy/phy_led_triggers.c
> > 
> >     83	
> >     84	int phy_led_triggers_register(struct phy_device *phy)
> >     85	{
> >     86		unsigned int *speeds;
> >     87		int i, err;
> >     88	
> >   > 89		phy->phy_num_led_triggers = phy_supported_speeds_num(phy);
> >     90		if (!phy->phy_num_led_triggers)
> >     91			return 0;
> >     92	
> >     93		speeds = kmalloc_array(phy->phy_num_led_triggers, sizeof(*speeds),
> >     94				       GFP_KERNEL);
> >     95		if (!speeds)
> >     96			return -ENOMEM;
> >     97	
> >     98		/* Presence of speed modes already checked up */
> >     99		phy_supported_speeds(phy, speeds, phy->phy_num_led_triggers);
> >    100	
> >    101		phy->led_link_trigger = devm_kzalloc(&phy->mdio.dev,
> >    102						     sizeof(*phy->led_link_trigger),
> >    103						     GFP_KERNEL);
> >    104		if (!phy->led_link_trigger) {
> >    105			err = -ENOMEM;
> >    106			goto out_clear;
> >    107		}
> >    108	
> >    109		err = phy_led_trigger_register(phy, phy->led_link_trigger, 0, "link");
> >    110		if (err)
> >    111			goto out_free_link;
> >    112	
> >    113		phy->phy_led_triggers = devm_kcalloc(&phy->mdio.dev,
> >    114						    phy->phy_num_led_triggers,
> >    115						    sizeof(struct phy_led_trigger),
> >    116						    GFP_KERNEL);
> >    117		if (!phy->phy_led_triggers) {
> >    118			err = -ENOMEM;
> >    119			goto out_unreg_link;
> >    120		}
> >    121	
> >    122		for (i = 0; i < phy->phy_num_led_triggers; i++) {
> >    123			err = phy_led_trigger_register(phy, &phy->phy_led_triggers[i],
> >    124						       speeds[i],
> >    125						       phy_speed_to_str(speeds[i]));
> >    126			if (err)
> >    127				goto out_unreg;
> >    128		}
> >    129	
> >    130		phy->last_triggered = NULL;
> >    131		phy_led_trigger_change_speed(phy);
> >    132	
> >  > 133		free(speeds);
> >    134	
> >    135		return 0;
> >    136	out_unreg:
> >    137		while (i--)
> >    138			phy_led_trigger_unregister(&phy->phy_led_triggers[i]);
> >    139		devm_kfree(&phy->mdio.dev, phy->phy_led_triggers);
> >    140	out_unreg_link:
> >    141		phy_led_trigger_unregister(phy->led_link_trigger);
> >    142	out_free_link:
> >    143		devm_kfree(&phy->mdio.dev, phy->led_link_trigger);
> >    144		phy->led_link_trigger = NULL;
> >    145	out_clear:
> >    146		free(speeds);
> >    147		phy->phy_num_led_triggers = 0;
> >    148		return err;
> >    149	}
> >    150	EXPORT_SYMBOL_GPL(phy_led_triggers_register);
> >    151	
> >
> 
> Ugh didn't think that LEDs netdev trigger doesn't have a dependency on
> PHY...

I don't think you've read and comprehended the kernel build bot
message.

It's complaining that:

1) phy_supported_speeds_num() is not declared in a header file. We can
   see this plainly in patch 2, where you introduce this new function,
   but don't add a function prototype to *any* header file.

2) the function "free" doesn't exist (which is absolutely correct,
   this isn't userspace code).

Obviously, this could not have been build-tested prior to sending it out
either of these would cause a build error. Maybe you built a kernel with
a config that had LEDs support disabled?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Christian Marangi]

On Sun, Dec 17, 2023 at 10:38:41AM +0000, Russell King (Oracle) wrote:
> On Sun, Dec 17, 2023 at 02:12:58AM +0100, Christian Marangi wrote:
> > On Fri, Dec 15, 2023 at 08:50:28PM +0800, kernel test robot wrote:
> > > Hi Christian,
> > > 
> > > kernel test robot noticed the following build errors:
> > > 
> > > [auto build test ERROR on net-next/main]
> > > 
> > > url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/net-phy-refactor-and-better-document-phy_speeds-function/20231215-064112
> > > base:   net-next/main
> > > patch link:    https://lore.kernel.org/r/20231214154906.29436-4-ansuelsmth%40gmail.com
> > > patch subject: [net-next PATCH v2 3/3] net: phy: led: dynamically allocate speed modes array
> > > config: arm-randconfig-002-20231215 (https://download.01.org/0day-ci/archive/20231215/202312152038.v9NZyBxd-lkp@intel.com/config)
> > > compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
> > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231215/202312152038.v9NZyBxd-lkp@intel.com/reproduce)
> > > 
> > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > the same patch/commit), kindly add following tags
> > > | Reported-by: kernel test robot <lkp@intel.com>
> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202312152038.v9NZyBxd-lkp@intel.com/
> > > 
> > > All errors (new ones prefixed by >>):
> > > 
> > > >> drivers/net/phy/phy_led_triggers.c:89:30: error: call to undeclared function 'phy_supported_speeds_num'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
> > >       89 |         phy->phy_num_led_triggers = phy_supported_speeds_num(phy);
> > >          |                                     ^
> > >    drivers/net/phy/phy_led_triggers.c:89:30: note: did you mean 'phy_supported_speeds'?
> > >    include/linux/phy.h:208:14: note: 'phy_supported_speeds' declared here
> > >      208 | unsigned int phy_supported_speeds(struct phy_device *phy,
> > >          |              ^
> > > >> drivers/net/phy/phy_led_triggers.c:133:2: error: call to undeclared library function 'free' with type 'void (void *)'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
> > >      133 |         free(speeds);
> > >          |         ^
> > >    drivers/net/phy/phy_led_triggers.c:133:2: note: include the header <stdlib.h> or explicitly provide a declaration for 'free'
> > >    2 errors generated.
> > > 
> > > 
> > > vim +/phy_supported_speeds_num +89 drivers/net/phy/phy_led_triggers.c
> > > 
> > >     83	
> > >     84	int phy_led_triggers_register(struct phy_device *phy)
> > >     85	{
> > >     86		unsigned int *speeds;
> > >     87		int i, err;
> > >     88	
> > >   > 89		phy->phy_num_led_triggers = phy_supported_speeds_num(phy);
> > >     90		if (!phy->phy_num_led_triggers)
> > >     91			return 0;
> > >     92	
> > >     93		speeds = kmalloc_array(phy->phy_num_led_triggers, sizeof(*speeds),
> > >     94				       GFP_KERNEL);
> > >     95		if (!speeds)
> > >     96			return -ENOMEM;
> > >     97	
> > >     98		/* Presence of speed modes already checked up */
> > >     99		phy_supported_speeds(phy, speeds, phy->phy_num_led_triggers);
> > >    100	
> > >    101		phy->led_link_trigger = devm_kzalloc(&phy->mdio.dev,
> > >    102						     sizeof(*phy->led_link_trigger),
> > >    103						     GFP_KERNEL);
> > >    104		if (!phy->led_link_trigger) {
> > >    105			err = -ENOMEM;
> > >    106			goto out_clear;
> > >    107		}
> > >    108	
> > >    109		err = phy_led_trigger_register(phy, phy->led_link_trigger, 0, "link");
> > >    110		if (err)
> > >    111			goto out_free_link;
> > >    112	
> > >    113		phy->phy_led_triggers = devm_kcalloc(&phy->mdio.dev,
> > >    114						    phy->phy_num_led_triggers,
> > >    115						    sizeof(struct phy_led_trigger),
> > >    116						    GFP_KERNEL);
> > >    117		if (!phy->phy_led_triggers) {
> > >    118			err = -ENOMEM;
> > >    119			goto out_unreg_link;
> > >    120		}
> > >    121	
> > >    122		for (i = 0; i < phy->phy_num_led_triggers; i++) {
> > >    123			err = phy_led_trigger_register(phy, &phy->phy_led_triggers[i],
> > >    124						       speeds[i],
> > >    125						       phy_speed_to_str(speeds[i]));
> > >    126			if (err)
> > >    127				goto out_unreg;
> > >    128		}
> > >    129	
> > >    130		phy->last_triggered = NULL;
> > >    131		phy_led_trigger_change_speed(phy);
> > >    132	
> > >  > 133		free(speeds);
> > >    134	
> > >    135		return 0;
> > >    136	out_unreg:
> > >    137		while (i--)
> > >    138			phy_led_trigger_unregister(&phy->phy_led_triggers[i]);
> > >    139		devm_kfree(&phy->mdio.dev, phy->phy_led_triggers);
> > >    140	out_unreg_link:
> > >    141		phy_led_trigger_unregister(phy->led_link_trigger);
> > >    142	out_free_link:
> > >    143		devm_kfree(&phy->mdio.dev, phy->led_link_trigger);
> > >    144		phy->led_link_trigger = NULL;
> > >    145	out_clear:
> > >    146		free(speeds);
> > >    147		phy->phy_num_led_triggers = 0;
> > >    148		return err;
> > >    149	}
> > >    150	EXPORT_SYMBOL_GPL(phy_led_triggers_register);
> > >    151	
> > >
> > 
> > Ugh didn't think that LEDs netdev trigger doesn't have a dependency on
> > PHY...
> 
> I don't think you've read and comprehended the kernel build bot
> message.
> 
> It's complaining that:
> 
> 1) phy_supported_speeds_num() is not declared in a header file. We can
>    see this plainly in patch 2, where you introduce this new function,
>    but don't add a function prototype to *any* header file.
> 
> 2) the function "free" doesn't exist (which is absolutely correct,
>    this isn't userspace code).
> 
> Obviously, this could not have been build-tested prior to sending it out
> either of these would cause a build error. Maybe you built a kernel with
> a config that had LEDs support disabled?
>

Yes you are correct and I have sent this revision by mistake. (already
sent other revision that have this corrected)

Anyway I have sent the question to the wrong series... Resending it to
the correct one, sorry for the noise. (anyway thanks for answering. Yes
the problem in that series is that with a kernel randconfig PHY kconfig
is not built and LED trigger doesn't depend on it, so it's a matter of
decide it worth to add an additional dependency or move functions)

-- 
	Ansuel