Date:   Fri, 17 Mar 2023 16:12:07 +0100
From:   Michal Kubiak <michal.kubiak@intel.com>
To:     Andrew Lunn <andrew@lunn.ch>
CC:     Christian Marangi <ansuelsmth@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        "Jakub Kicinski" <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Gregory Clement <gregory.clement@bootlin.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>,
        Andy Gross <agross@kernel.org>,
        "Bjorn Andersson" <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        John Crispin <john@phrozen.org>, <netdev@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-arm-msm@vger.kernel.org>, "Lee Jones" <lee@kernel.org>,
        <linux-leds@vger.kernel.org>
Subject: Re: [net-next PATCH v4 04/14] net: phy: Add a binding for PHY LEDs
Message-ID: <ZBSDRw5MF431wxz1@localhost.localdomain>
References: <20230317023125.486-1-ansuelsmth@gmail.com>
 <20230317023125.486-5-ansuelsmth@gmail.com>
 <ZBRtRw8pg0mcRxbZ@localhost.localdomain>
 <f292505c-ab74-47f4-be7f-18dd4a7e2903@lunn.ch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <f292505c-ab74-47f4-be7f-18dd4a7e2903@lunn.ch>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?zv+0U5xmlDTL62z/vwuvNuxwT1Sy4V+qjBaKqLqfdc/t3Suan8+sNf6QMDvy?=
 =?us-ascii?Q?vuHbkGOVcrjAApisAvuRX2kdZYKbzZnmUWy2DLBgMNQEnFZMgTW/yiG1hRpo?=
 =?us-ascii?Q?odcmlPVAdMKeefaIqXtfIy7eQtXkSsdB9nS4AbzKGSP+hhW8h5HuMlp/y4Dd?=
 =?us-ascii?Q?mAWIi+w3lNU2xfHpWNXKPawTkuzK4Puj0Srq33yuWqwpODNTA4LY63Rwgn2J?=
 =?us-ascii?Q?fyPWBrao4hx10ebLI8ejiDE5X0bptRSmqCYBJj5jmqSnLVbJGOb7u9HDk4SJ?=
 =?us-ascii?Q?V/k052EWrnaIdUzB9AzwQvTzAh25AFsOjCs2/f9WZW2j0gfy6C749/sJzj1S?=
 =?us-ascii?Q?MrJxftrcodEwG/7FJyi24aBTRzNdJzIfMIvQHJPK5TztTASy2GqrqdRD8shX?=
 =?us-ascii?Q?6ziZbCGl2UMuiAPwvgriLyImMnYfsQfL6iA4cjiCXFZ/ZzwYUf17ZAIxsABf?=
 =?us-ascii?Q?rkKFJ1D22Jhs4kvyZOMUjKSIh0+HpMMMonADV/B2F4xhU5FEAh7P6h4Uvv4i?=
 =?us-ascii?Q?v90PbVsH9PVCVkk8rbBlPOy8YmUasrGY65D5ooQ9pvE02GfaJznZSc83cDHA?=
 =?us-ascii?Q?HkKAcXdAEd9ORu1oPnzx+irc7vYLnxF+2v5q+61K2Vq/sN36TnoAdEoyDjLQ?=
 =?us-ascii?Q?T1orEtcji7gWC2tTW6EeE8s+LjHvW673WDId3W0QRxm3HnJ+mltXpchvgTqR?=
 =?us-ascii?Q?1OG+Lq1Pl7mNaUV69UndccgIdE4eng/OW3gEqOm/Yi5Hk7F5p/Qu6yA/qxyq?=
 =?us-ascii?Q?knr02cT5KV5CLogFX9AUFVC3f8ybnGXiVQWEwfVJPP+WbkMjv9F3OeGyHBj0?=
 =?us-ascii?Q?JIrOhpYJ/xx7VUFFGxk8BmMjMwBKNY9Y+2Rszq8LF/X486klsPXl6WNs95cJ?=
 =?us-ascii?Q?ASzUaubblRcN1BSL+MgIuAwQ4e/ugWwYSwV+YCKaYXYfM802Vs/M+U3IAzqQ?=
 =?us-ascii?Q?3B+U0Cppz58kAycwtAC0vC4l33ZX7g51/p7CIT3J1ZcO14mBXf86uWTjpwlk?=
 =?us-ascii?Q?Zt1NFUuDZWbvtcRiXBBuMLGun9A4MtswhkhVgmy+QeR+OuEuKglAerT7GPZ6?=
 =?us-ascii?Q?ixPB9GLaxkXY6/iodMT0RGGMPWCoZFm0wsNWE1xIGMhTPigEh0rbezSCF+Aa?=
 =?us-ascii?Q?L/5iEieybScS7QZAzN+F/0UsoQ2Mav6sQa4L35b3kTyg0MQHkIkKK/olfHs7?=
 =?us-ascii?Q?X+oaLnoWGejMRRqpBgRsGXuu/m8XV3X0IN2oq0oyD80miqUdGgL1qMllMW40?=
 =?us-ascii?Q?ys8gzD6Txh2YvRaJirl20tO0zi87rfUINLaDOlw4EVpjKfBO0hVcAv4+usZB?=
 =?us-ascii?Q?oEf9W48G0H28aYtZejNLXnbQ+StGzyn+rLpSta6ZnrXd78EO/J+QgNew1QJb?=
 =?us-ascii?Q?uuAZsjh7FloCwAm13IAyPgyXiOC+5XPrSt9v9TCujhvYS4ztGf7vh8WlXar1?=
 =?us-ascii?Q?eM2gMMPL3sTItMSIbOenb4uOv71bAtIKBP4DtPnVPVlR0iVg58K3qWJYBW7B?=
 =?us-ascii?Q?+OVH4fcCvwKMwdqKnQv2Jo7Z5UsVBnDf+ZnR0XXSTmzrGuV3sOobtKXamYCK?=
 =?us-ascii?Q?mh0mYIGXlKEFojPOSQ13d43bUNrjFxxO998/8a8RurxJVurKbGdgU3Vh3Rb+?=
 =?us-ascii?Q?Hg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 588d2904-56f7-472e-ac96-08db26fa01e3
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB2937.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Mar 2023 15:12:22.0465
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /JCB8qGONY1iAMawpw1KEYhi0phDOKtOD1SawrBv6AaAn/rwgujmxrn5wel+MJUuHcU5nlCDc2kf18/C+S2uiQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ1PR11MB6153
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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