Re: [PATCH] soundwire: intel: move to auxiliary bus

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Note that the auxiliary bus API has separate init and add steps, which
requires more attention in the error unwinding paths. The main loop
needs to deal with kfree() and auxiliary_device_uninit() for the
current iteration before jumping to the common label which releases
everything allocated in prior iterations.

The init/add steps can be moved together in the aux bus code if that
makes this usage simpler.  Please do that instead.

IIRC the two steps were separated during the auxbus reviews to allow the
parent to call kfree() on an init failure, and auxiliary_device_uninit()
afterwards.

https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#auxiliary-device

With a single auxbus_register(), the parent wouldn't know whether to use
kfree() or auxiliary_device_uinit() when an error is returned, would it?


It should, you know the difference when you call device_register() vs.
device_initialize()/device_add(), for what to do, right?

Should be no difference here either :)

sorry, not following.

with the regular devices, the errors can only happen on the second "add"
stage.

int device_register(struct device *dev)
{
	device_initialize(dev);
	return device_add(dev);
}

that's not what is currently implemented for the auxiliary bus

the current flow is

ldev = kzalloc(..)
some inits
ret = auxiliary_device_init(&ldev->auxdev)
if (ret < 0) {
     kfree(ldev);
     goto err1;
}

ret = auxiliary_device_add(&ldev->auxdev)
if (ret < 0)
     auxiliary_device_uninit(&ldev->auxdev)
     goto err2;
}
...
err2:
err1:

How would I convert this to

ldev = kzalloc(..)
some inits
ret = auxiliary_device_register()
if (ret) {
    kfree(ldev) or not?
    unit or not?
}

IIRC during reviews there was an ask that the parent and name be checked,
and that's why the code added the two checks below:

int auxiliary_device_init(struct auxiliary_device *auxdev)
{
	struct device *dev = &auxdev->dev;

	if (!dev->parent) {
		pr_err("auxiliary_device has a NULL dev->parent\n");
		return -EINVAL;
	}

	if (!auxdev->name) {
		pr_err("auxiliary_device has a NULL name\n");
		return -EINVAL;
	}

	dev->bus = &auxiliary_bus_type;
	device_initialize(&auxdev->dev);
	return 0;
}

does this clarify the sequence?

Yes, thanks, but I don't know the answer to your question, sorry.  This
feels more complex than it should be, but I do not have the time at the
moment to look into it, sorry.

Try getting the authors of this code to fix it up :)

We can try to check why those two tests were added before initialize(), I don't fully recall these details

If we could move these tests after device_initialize() then we could add a _register function.

Note at this point it would mean an API change and impact the existing Nvidia/Mellanox code, we are using the same sequence as them

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlx5/core/dev.c#L262




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux