linux-kernel - Re: [PATCH] soundwire: add slave device to linked list after device

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]

Date:   Tue, 23 Mar 2021 13:30:38 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Vinod Koul <vkoul@...nel.org>,
        Bard Liao <yung-chuan.liao@...ux.intel.com>
Cc:     alsa-devel@...a-project.org, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org, hui.wang@...onical.com,
        srinivas.kandagatla@...aro.org, sanyog.r.kale@...el.com,
        rander.wang@...ux.intel.com, bard.liao@...el.com
Subject: Re: [PATCH] soundwire: add slave device to linked list after
 device_register()

Hi Vinod,

>> We currently add the slave device to a linked list before
>> device_register(), and then remove it if device_register() fails.
>>
>> It's not clear why this sequence was necessary, this patch moves the
>> linked list management to after the device_register().
> 
> Maybe add a comment :-)
> 
> The reason here is that before calling device_register() we need to be
> ready and completely initialized. device_register is absolutely the last
> step in the flow, always.
> 
> The probe of the device can happen and before you get a chance to
> add to list, many number of things can happen.. So adding after is not a
> very good idea :)

Can you describe that 'many number of things' in the SoundWire context?

While you are correct in general on the use of device_register(), in 
this specific case the device registration and the possible Slave driver 
probe if there's a match doesn't seem to use this linked list.

This sdw_slave_add() routine is called while parsing ACPI/DT tables and 
at this point the bus isn't functional. This sequence only deals with 
device registration and driver probe, the actual activation and 
enumeration happen much later. What does matter is that by the time all 
ACPI/DT devices have been scanned all initialization is complete. The 
last part of the flow is not the device_register() at the individual 
peripheral level.

Even for the Qualcomm case, the steps are different:

	ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode);
	if (ret) {
		dev_err(dev, "Failed to register Soundwire controller (%d)\n",
			ret);
		goto err_clk;
	}

	qcom_swrm_init(ctrl); <<< that's where the bus is functional

Note that we are not going to lay on the tracks for this, this sequence 
was tagged by static analysis tools who don't understand that 
put_device() actually frees memory by way of the .release callback, but 
that led us to ask ourselves whether this sequence was actually necessary.