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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BY5PR12MB43221A308CE750FACEB0A806DC0A0@BY5PR12MB4322.namprd12.prod.outlook.com>
Date:   Wed, 7 Oct 2020 20:17:15 +0000
From:   Parav Pandit <parav@...dia.com>
To:     Leon Romanovsky <leon@...nel.org>,
        "Ertman, David M" <david.m.ertman@...el.com>
CC:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "parav@...lanox.com" <parav@...lanox.com>,
        "tiwai@...e.de" <tiwai@...e.de>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ranjani.sridharan@...ux.intel.com" 
        <ranjani.sridharan@...ux.intel.com>,
        "fred.oh@...ux.intel.com" <fred.oh@...ux.intel.com>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "dledford@...hat.com" <dledford@...hat.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        Jason Gunthorpe <jgg@...dia.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "Williams, Dan J" <dan.j.williams@...el.com>,
        "Saleem, Shiraz" <shiraz.saleem@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "Patil, Kiran" <kiran.patil@...el.com>
Subject: RE: [PATCH v2 1/6] Add ancillary bus support


> From: Leon Romanovsky <leon@...nel.org>
> 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.
	 */
}