RE: [PATCH v2 1/6] Add ancillary bus support

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

 



> From: Leon Romanovsky <leon@xxxxxxxxxx>
> Sent: Thursday, October 8, 2020 12:56 AM
> 
> > > This API is partially obscures low level driver-core code and needs
> > > to provide clear and proper abstractions without need to remember
> > > about put_device. There is already _add() interface why don't you do
> > > put_device() in it?
> > >
> >
> > The pushback Pierre is referring to was during our mid-tier internal
> > review.  It was primarily a concern of Parav as I recall, so he can speak to his
> reasoning.
> >
> > What we originally had was a single API call
> > (ancillary_device_register) that started with a call to
> > device_initialize(), and every error path out of the function performed a
> put_device().
> >
> > Is this the model you have in mind?
> 
> I don't like this flow:
> ancillary_device_initialize()
> if (ancillary_ancillary_device_add()) {
>   put_device(....)
>   ancillary_device_unregister()
Calling device_unregister() is incorrect, because add() wasn't successful.
Only put_device() or a wrapper ancillary_device_put() is necessary.

>   return err;
> }
> 
> And prefer this flow:
> ancillary_device_initialize()
> if (ancillary_device_add()) {
>   ancillary_device_unregister()
This is incorrect and a clear deviation from the current core APIs that adds the confusion.

>   return err;
> }
> 
> In this way, the ancillary users won't need to do non-intuitive put_device();

Below is most simple, intuitive and matching with core APIs for name and design pattern wise.
init()
{
	err = ancillary_device_initialize();
	if (err)
		return ret;

	err = ancillary_device_add();
	if (ret)
		goto err_unwind;

	err = some_foo();
	if (err)
		goto err_foo;
	return 0;

err_foo:
	ancillary_device_del(adev);
err_unwind:
	ancillary_device_put(adev->dev);
	return err;
}

cleanup()
{
	ancillary_device_de(adev);
	ancillary_device_put(adev);
	/* It is common to have a one wrapper for this as ancillary_device_unregister().
	 * This will match with core device_unregister() that has precise documentation.
	 * but given fact that init() code need proper error unwinding, like above,
	 * it make sense to have two APIs, and no need to export another symbol for unregister().
	 * This pattern is very easy to audit and code.
	 */
}




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux