Received-SPF: pass (google.com: domain of linux-kernel+bounces-2425-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <657e4b1d.df0a0220.897b4.394e@mx.google.com>
Date: Sun, 17 Dec 2023 02:12:58 +0100
From: Christian Marangi <ansuelsmth@gmail.com>
To: kernel test robot <lkp@intel.com>
Cc: Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	linux-kernel@vger.kernel.org, llvm@lists.linux.dev,
	oe-kbuild-all@lists.linux.dev, netdev@vger.kernel.org
Subject: Re: [net-next PATCH v2 3/3] net: phy: led: dynamically allocate
 speed modes array
References: <20231214154906.29436-4-ansuelsmth@gmail.com>
 <202312152038.v9NZyBxd-lkp@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202312152038.v9NZyBxd-lkp@intel.com>

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