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

[Christian Marangi <ansuelsmth@gmail.com>]

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