linux-kernel - Re: [PATCH 1/5] soundwire: bus_type: add master

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: <626a074b-06a9-01a0-334f-3aaed1f7ed76@linux.intel.com>
Date:   Fri, 20 Mar 2020 13:17:37 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        alsa-devel@...a-project.org
Cc:     linux-kernel@...r.kernel.org, tiwai@...e.de, broonie@...nel.org,
        vkoul@...nel.org, gregkh@...uxfoundation.org, jank@...ence.com,
        slawomir.blauciak@...el.com,
        Bard liao <yung-chuan.liao@...ux.intel.com>,
        Rander Wang <rander.wang@...ux.intel.com>,
        Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
        Hui Wang <hui.wang@...onical.com>,
        Sanyog Kale <sanyog.r.kale@...el.com>
Subject: Re: [PATCH 1/5] soundwire: bus_type: add master_device/driver support

Thanks for the quick review Srinivas,

> This patch in general is missing device tree support for both matching 
> and uevent so this will not clearly work for Qualcomm controller unless 
> we do via platform bus, which does not sound right!

see other email, the platform bus is handled by a platform 
device/driver. There was no intention to change that, it's by design 
rather than an omission/error.
>> +
>> +/**
>> + * sdw_master_device_startup() - startup hardware
>> + * @md: Linux Soundwire master device
>> + *
>> + * This use of this function is optional. It is only needed if the
>> + * hardware cannot be started during a driver probe, e.g. due to power
>> + * rail dependencies. The implementation is platform-specific but the
>> + * bus will typically go through a hardware-reset sequence and devices
>> + * will be enumerated once they report as ATTACHED.
>> + */
>> +int sdw_master_device_startup(struct sdw_master_device *md)
>> +{
>> +    struct sdw_master_driver *mdrv;
>> +    struct device *dev;
>> +    int ret = 0;
>> +
>> +    if (IS_ERR_OR_NULL(md))
>> +        return -EINVAL;
>> +
>> +    dev = &md->dev;
>> +    mdrv = drv_to_sdw_master_driver(dev->driver);
>> +
>> +    if (mdrv && mdrv->startup)
>> +        ret = mdrv->startup(md);
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(sdw_master_device_startup);
> 
> Who would call this function? and How would it get hold of master device 
> instance ?

sdw_master_device_add() returns a struct_master_device *md, so the 
parent has a handle to that device. See the device creation here:

https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L238

This startup() would be called by the parent when all 
requirements/dependencies are met. Put differently, it allows the probe 
to run much earlier and check for platform firmware information (which 
links are enabled, what devices are exposed by platform firmware), while 
the startup is really when the bus clk/data lines will start toggling.

https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L341

and the call from the SOF layer is here:

https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/sound/soc/sof/intel/hda-loader.c#L418

Again, if everything is ready at probe time there's no need to delay the 
actual bus startup. This is not needed for Qualcomm platforms where the 
Master device is part of a codec. It's actually irrelevant since there 
is no driver, so the startup callback does not even exist :-)

> How would soundwire core also ensure that we do not actively use this 
> master if it is not ready. Similar comment for shutdown callback.

That's a fair point, we could add a state variable and a check that the 
probe happened before.

In practice the two cases of device creation and startup are different 
phases so it'd more of a paranoia check.

Thanks,
-Pierre