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: <c90316f5-a5a9-fe22-ec11-a30a54ff0a9d@linux.intel.com>
Date:   Wed, 7 Oct 2020 15:59:28 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     "Ertman, David M" <david.m.ertman@...el.com>,
        Parav Pandit <parav@...dia.com>,
        Leon Romanovsky <leon@...nel.org>
Cc:     "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



>> 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.
>> 	 */
>> }
> 
> I like this flow +1
> 
> But ... since the init() function is performing both device_init and
> device_add - it should probably be called ancillary_device_register,
> and we are back to a single exported API for both register and
> unregister.

Kind reminder that we introduced the two functions to allow the caller 
to know if it needed to free memory when initialize() fails, and it 
didn't need to free memory when add() failed since put_device() takes 
care of it. If you have a single init() function it's impossible to know 
which behavior to select on error.

I also have a case with SoundWire where it's nice to first initialize, 
then set some data and then add.

> 
> At that point, do we need wrappers on the primitives init, add, del,
> and put?
> 
> -DaveE
>