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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZBSDRw5MF431wxz1@localhost.localdomain>
Date:   Fri, 17 Mar 2023 16:12:07 +0100
From:   Michal Kubiak <michal.kubiak@...el.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     Christian Marangi <ansuelsmth@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        "Jakub Kicinski" <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Gregory Clement <gregory.clement@...tlin.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Andy Gross <agross@...nel.org>,
        "Bjorn Andersson" <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        John Crispin <john@...ozen.org>, <netdev@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-arm-msm@...r.kernel.org>, "Lee Jones" <lee@...nel.org>,
        <linux-leds@...r.kernel.org>
Subject: Re: [net-next PATCH v4 04/14] net: phy: Add a binding for PHY LEDs

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