On Sat 15 Feb 2025 at 07:53, Greg Kroah-Hartman [off-list ref] wrote:

[...]

quoted

quoted

quoted

+							int id)
+{
+	struct auxiliary_device *auxdev;
+	int ret;
+
+	auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL);
+	if (!auxdev)
+		return ERR_PTR(-ENOMEM);

Ick, who cares what the error value really is?  Why not just do NULL or
a valid pointer?  That makes the caller much simpler to handle, right?

Sure why not

I have tried the 'NULL or valid' approach. In the consumers,
which mostly return an integer from their various init function, I got
this weird to come up with one from NULL. EINVAL, ENOMEM, etc ... can't
really pick one.

It is actually easier to pass something along.

quoted

quoted

quoted

+
+	auxdev->id = id;
+	auxdev->name = devname;
+	auxdev->dev.parent = dev;
+	auxdev->dev.platform_data = platform_data;
+	auxdev->dev.release = auxiliary_device_release;
+	device_set_of_node_from_dev(&auxdev->dev, dev);
+
+	ret = auxiliary_device_init(auxdev);

Only way this will fail is if you forgot to set parent or a valid name.
So why not check for devname being non-NULL above this?

If auxiliary_device_init() ever changes it would be easy to forget to
update that and lead to something nasty to debug, don't you think ?

Yes, this is being more defensive, I take back my objection, thanks.

quoted

quoted

quoted

+	if (ret) {
+		kfree(auxdev);
+		return ERR_PTR(ret);
+	}
+
+	ret = __auxiliary_device_add(auxdev, modname);
+	if (ret) {
+		/*
+		 * NOTE: It may look odd but auxdev should not be freed
+		 * here. auxiliary_device_uninit() calls device_put()
+		 * which call the device release function, freeing auxdev.
+		 */
+		auxiliary_device_uninit(auxdev);

Yes it is odd, are you SURE you should be calling device_del() on the
device if this fails?  auxiliary_device_uninit(), makes sense so why not
just call that here?

I'm confused ... I am call auxiliary_device_uninit() here. What do you
mean ? 

Oh wow, I got this wrong, sorry, I was thinking you were calling
auxiliary_device_destroy().  Nevermind, ugh, it was a long day...

No worries

thanks,

greg k-h

-- 
Jerome