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

quoted

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).

quoted

quoted

+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.

quoted

quoted

+
+	cdev->brightness_set_blocking = phy_led_set_brightness;

Please move this initialization to the patch where you are actually
implementing this callback.

quoted

+	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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel