linux-kernel - Re: [PATCH] soundwire: intel: move to auxiliary bus

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 for Android: free password hash cracker in your pocket

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

Message-ID: <35cc8d35-a778-d8b2-bee3-bb53f8a6c51e@linux.intel.com>
Date:   Tue, 23 Mar 2021 14:14:18 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     alsa-devel@...a-project.org, vinod.koul@...aro.org,
        linux-kernel@...r.kernel.org, hui.wang@...onical.com,
        vkoul@...nel.org, srinivas.kandagatla@...aro.org,
        sanyog.r.kale@...el.com,
        Bard Liao <yung-chuan.liao@...ux.intel.com>,
        rander.wang@...ux.intel.com, bard.liao@...el.com
Subject: Re: [PATCH] soundwire: intel: move to auxiliary bus



On 3/23/21 1:32 PM, Greg KH wrote:
> On Tue, Mar 23, 2021 at 01:04:49PM -0500, Pierre-Louis Bossart wrote:
>>
>>>> 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?