linux-kernel - Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead

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:   Mon, 6 Jan 2020 08:51:59 -0600
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Vinod Koul <vkoul@...nel.org>
Cc:     alsa-devel@...a-project.org, tiwai@...e.de,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
        broonie@...nel.org, srinivas.kandagatla@...aro.org,
        jank@...ence.com, slawomir.blauciak@...el.com,
        Sanyog Kale <sanyog.r.kale@...el.com>,
        Bard liao <yung-chuan.liao@...ux.intel.com>,
        Rander Wang <rander.wang@...ux.intel.com>
Subject: Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform
 devices and use 'Master Devices' instead


>>>> +		/* let the SoundWire master driver to its probe */
>>>> +		md->driver->probe(md, link);
>>>
>>> So you are invoking driver probe here.. That is typically role of driver
>>> core to do that.. If we need that, make driver core do that for you!
>>>
>>> That reminds me I am missing match code for master driver...
>>
>> There is no match for the master because it doesn't have an existence in
>> ACPI. There are no _ADR or HID that can be used, the only thing that exists
>> is the Controller which has 4 sublinks. Each master must be added  by hand.
>>
>> Also the SoundWire master cannot be enumerated or matched against a
>> SoundWire bus, since it controls the bus itself (that would be a chicken and
>> egg problem). The SoundWire master would need to be matched on a parent bus
>> (which does not exist for Intel) since the hardware is embedded in a larger
>> audio cluster that's visible on PCI only.
>>
>> Currently for Intel platforms, the SoundWire master device is created by the
>> SOF driver (via the abstraction in intel_init.c).
> 
> That is okay for me, the thing that is bit confusing is having a probe
> etc and no match.. (more below)..
> 
>>> So we seem to be somewhere is middle wrt driver probing here! IIUC this
>>> is not a full master driver, thats okay, but then it is not
>>> completely transparent either...
>>>
>>> I was somehow thinking that the driver will continue to be
>>> 'platform/acpi/of' driver and master device abstraction will be
>>> handled in the core (for example see how the busses like i2c handle
>>> this). The master device is created and used to represent but driver
>>> probing etc is not done
>>
>> I2C controllers are typically PCI devices or have some sort of ACPI
>> description. This is not the case for SoundWire masters on Intel platforms,
> 
> Well the world is not PCI/ACPI... We have controllers which are DT
> described and work in same manner as a PCI device.
Both DT and PCI would use a DIFFERENT matching on the parent bus, not a 
matching provided by the SoundWire subsystem itself.

> 
>> so even if I wanted to I would have no ability to implement any matching or
>> parent bus registration.
>>
>> Also the notion of 'probe' does not necessarily mean that the device is
>> attached to a bus, we use DAI 'drivers' in ASoC and still have probe/remove
>> callbacks.
> 
> The "big" difference is that probe is called by core (asoc) and not by
> driver onto themselves.. IMO that needs to go away.

What I did is not different from what existed already with platform 
devices. They were manually created, weren't they?

> 
>> And if you look at the definitions, we added additional callbacks since
>> probe/remove are not enough to deal with hardware restrictions:
>>
>> For Intel platforms, we have a startup() callback which is only invoked once
>> the DSP is powered and the rails stable. Likewise we added an
>> 'autonomous_clock_stop()' callback which will be needed when the Linux
>> driver hands-over control of the hardware to the DSP firmware, e.g. to deal
>> with in-band wakes in D0i3.
>>
>> FWIW, the implementation here follows what was suggested for Greybus 'Host
>> Devices' [1] [2], so it's not like I am creating any sort of dangerous
>> precedent.
>>
>> [1]
>> https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275
>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124
> 
> And if you look closely all this work is done by core not by drivers!
> Drivers _should_ never do all this, it is the job of core to do that for
> you.

Please look at the code again, you have a USB probe that will manually 
call the GreyBus device creation.

static int ap_probe(struct usb_interface *interface,
		    const struct usb_device_id *id)
{
	hd = gb_hd_create(&es2_driver, &udev->dev, 	


static struct usb_driver es2_ap_driver = {
	.name =		"es2_ap_driver",
	.probe =	ap_probe, <<< code above
	.disconnect =	ap_disconnect,
	.id_table =	id_table,
	.soft_unbind =	1,
};

The master device probe suggested here is also called as part of the 
parent SOF PCI device probe, same as this USB example. I really don't 
see what your objection is, given that there is no way to deal with the 
SoundWire controller as a independent entity for Intel platforms.