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] [day] [month] [year] [list]

Message-ID: <DM6PR11MB4074389B63D3CB3B396838DBFF499@DM6PR11MB4074.namprd11.prod.outlook.com>
Date:   Mon, 19 Apr 2021 01:46:46 +0000
From:   "Liao, Bard" <bard.liao@...el.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        Vinod Koul <vkoul@...nel.org>,
        Bard Liao <yung-chuan.liao@...ux.intel.com>
CC:     "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "hui.wang@...onical.com" <hui.wang@...onical.com>,
        "srinivas.kandagatla@...aro.org" <srinivas.kandagatla@...aro.org>,
        "Kale, Sanyog R" <sanyog.r.kale@...el.com>,
        "rander.wang@...ux.intel.com" <rander.wang@...ux.intel.com>
Subject: RE: [PATCH] soundwire: add slave device to linked list after
 device_register()

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
> Sent: Wednesday, March 24, 2021 2:31 AM
> 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; Kale, Sanyog R <sanyog.r.kale@...el.com>;
> rander.wang@...ux.intel.com; Liao, Bard <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.

Hi Vinod,

Do you have any comment or objection after Pierre's explanation? 

Regards,
Bard