Re: [net-next PATCH v4 04/14] net: phy: Add a binding for PHY LEDs

[Michal Kubiak <michal.kubiak@intel.com>]

On Fri, Mar 17, 2023 at 03:03:32PM +0100, Andrew Lunn wrote:

> > Hi Andrew,
> > 
> > Personally, I see no good reason to provide a dummy implementation
> > of "phy_led_set_brightness", especially if you implement it in the next
> > patch. You only use that function only the function pointer in
> > "led_classdev". I think you can just skip it in this patch.
> 
> Hi Michal
> 
> The basic code for this patch has been sitting in my tree for a long
> time. It used to be, if you did not have a set_brightness method in
> cdev, the registration failed. That made it hard to test this patch on
> its own during development work, did i have the link list correct, can
> i unload the PHY driver without it exploding etc. I need to check if
> it is still mandatory.
> 

Thank you for the explanation. I was not aware of failing registration
in case of undefined "cdev->brightness_set_blocking". I think it is
a good reason of defining the dummy function. (The only alternative
would be to squash two commits, but I think it is easier to review
smaller chunks of code).


> > > +static int of_phy_led(struct phy_device *phydev,
> > > +		      struct device_node *led)
> > > +{
> > > +	struct device *dev = &phydev->mdio.dev;
> > > +	struct led_init_data init_data = {};
> > > +	struct led_classdev *cdev;
> > > +	struct phy_led *phyled;
> > > +	int err;
> > > +
> > > +	phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL);
> > > +	if (!phyled)
> > > +		return -ENOMEM;
> > > +
> > > +	cdev = &phyled->led_cdev;
> > > +
> > > +	err = of_property_read_u32(led, "reg", &phyled->index);
> > > +	if (err)
> > > +		return err;
> > 
> > Memory leak. 'phyled' is not freed in case of error.
> 
> devm_ API, so it gets freed when the probe fails.
> 
> > > +
> > > +	cdev->brightness_set_blocking = phy_led_set_brightness;
> > 
> > Please move this initialization to the patch where you are actually
> > implementing this callback.
> > 
> > > +	cdev->max_brightness = 1;
> > > +	init_data.devicename = dev_name(&phydev->mdio.dev);
> > > +	init_data.fwnode = of_fwnode_handle(led);
> > > +
> > > +	err = devm_led_classdev_register_ext(dev, cdev, &init_data);
> > > +	if (err)
> > > +		return err;
> > 
> > Another memory leak.
> 
> Ah, maybe you don't know about devm_ ? devm_ allocations and actions
> register an action to be taken when the device is removed, either
> because the probe failed, or when the device is unregistered. For
> memory allocation, the memory is freed automagically. For actions like
> registering an LED, requesting an interrupt etc, an unregister/release
> is performed. This makes cleanup less buggy since the core does it.
> 
>    Andrew


Yeah, it is my fault, I apologize for that.
I didn't consider neither the probe() context, nor the lifetime of the
list. You are right - I had no experience with using this devm_ API,
so I looked at it as a standard memory allocation.
Thank you for your patience and this piece of knowledge.

Thanks,
Michal