[RFC] Use Ancillary bus implementation in SOF #2265

fredoh9 · 2020-07-09T23:21:41Z

This is updated PR from #1841.

Renamed from Virtual bus to Ancillary bus.

fredoh9 · 2020-07-09T23:22:20Z

I took renaming of source codes but documentation on first commit(516836b) will be updated from Dave. That's why you can still find "virtual bus" in Documentation/.

This is the initial implementation of the Ancillary Bus, ancillary_device and ancillary_driver. The ancillary bus is a software based bus intended to support registering ancillary_devices and ancillary_drivers and provide matching between them and probing of the registered drivers. The bus will support probe/remove shutdown and suspend/resume callbacks. Signed-off-by: Dave Ertman <david.m.ertman@intel.com> Signed-off-by: Kiran Patil <kiran.patil@intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Tested-by: Andrew Bowers <andrewx.bowers@intel.com> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

A client in the SOF (Sound Open Firmware) context is a device that needs to communicate with the DSP via IPC messages. The SOF core is responsible for serializing the IPC messages to the DSP from the different clients. One example of an SOF client would be an IPC test client that floods the DSP with test IPC messages to validate if the serialization works as expected. Multi-client support will also add the ability to split the existing audio cards into multiple ones, so as to e.g. to deal with HDMI with a dedicated client instead of adding HDMI to all cards. This patch introduces descriptors for SOF client driver and SOF client device along with APIs for registering and unregistering a SOF client driver, sending IPCs from a client device and accessing the SOF core debugfs root entry. Along with this, add a couple of new members to struct snd_sof_dev that will be used for maintaining the list of clients. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Fred Oh <fred.oh@linux.intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Create an SOF client driver for IPC flood test. This driver is used to set up the debugfs entries and the read/write ops for initiating the IPC flood test that would be used to measure the min/max/avg response times for sending IPCs to the DSP. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Fred Oh <fred.oh@linux.intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add a new op for registering clients. The clients to be registered depend on the DSP capabilities and the ACPI/DT information. For now, we only add 2 IPC test clients that will be used for run tandem IPC flood tests for all Intel platforms. For ACPI platforms, change the Kconfig to select SND_SOC_SOF_PROBE_WORK_QUEUE to allow the ancillary driver to probe when the client is registered. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Fred Oh <fred.oh@linux.intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Remove the IPC flood test support in the SOF core as it is now added in the IPC flood test client. Signed-off-by: Fred Oh <fred.oh@linux.intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

fredoh9 · 2020-07-14T00:41:53Z

I merged ancillary bus documentation from #2264 to first commit and rebased all again.

paravmellanox · 2020-07-14T17:09:02Z

Documentation/driver-api/ancillary_bus.rst

+.. SPDX-License-Identifier: GPL-2.0-only
+
+===========
+Ancillary Bus


@ranj063 is name changed discussed with Greg KH? Did I miss the discussion?

@paravmellanox the name change was based on our discussions with kernel developers at Intel. This version is based on the feedback provided and is still under review.

but to be clear Greg KH was informed in the email thread for the previous series.

@plbossart can you please point me to the discussion thread?

Was the requirement of [1] dropped out? if so, can you point me to the thread? Need to ask what changed? OR it is just the name change?

[1] https://patchwork.kernel.org/patch/11280547/#23056771

I don't understand your question based on a reference that's more than 6 months old. What requirement are you referring to?

then what solution you are suggesting?

We cannot have both a generic light-weight mechanism and burden it with unrelated stuff. The odds of an audio solution sharing any sysfs attributes with rdma are nil

I am familiar with device groups, but that should be handled with your own drivers.

I don't think we can have default sysfs attributes added by default in the ancillary_device code, that was my point.

In other words, you can use existing kernel capabilities to add your own attributes.

The only point that maybe would be problematic for userspace is that you would need a device 'type' to know what attributes to expect. That maybe something that needs to be added as a default attribute, with an enum that avoid collisions between device types exposes to userspace. e.g if /sys/bus/ancillary/ancillary.1/type stays 'AUDIO' then userspace should not expect any net/rdma capabiltiies.

@plbossart
Also we certainly don't want to expose every ancillary device as subfunction device.

there will be new API such as
ancillary_device_sf_register(ancillary_device, &sf_sysfs_group);

This will do current work + new sysfs.
So base ancillary device is unmodified.

who calls this API and when?

then what solution you are suggesting?

We cannot have both a generic light-weight mechanism and burden it with unrelated stuff. The odds of an audio solution sharing any sysfs attributes with rdma are nil

@plbossart we just crossed each other. :-)

whether a device is audio or rdma, it is decided on the bus driver side. when 'struct device' is created, it only has base sysfs attributes which I described previously and here below.

Below attributes are created optionally.

sfnum

irqs

resources

numa node info

link to parent pci device

This is done through by having additional APIs than what is already present in PR.

who calls this API and when?

who calls this API and when?

@plbossart This API is called by the mlx5_core driver when

user issues devlink port add command.
An example:
$ devlink port add pci/0000.06.00.0/100 flavour pcisf pfnum 1 sfnum 10

content of [2] is more example oriented for subfunction.
Please refer to diagram and it will likely give you more insights.

This is explained in
[1] https://lore.kernel.org/netdev/20200519092258.GF4655@nanopsycho/
and
[2] https://marc.info/?l=linux-netdev&m=158555928517777&w=2

paravmellanox · 2020-07-14T17:11:05Z

Documentation/driver-api/ancillary_bus.rst

+just the matching up of multiple registering drivers, and sacrifices common
+generality to extend the bus functionality into subsystem specific functions,
+then the ancillary bus should not be used.  A custom bus solution, or some other
+method should be considered in this use case.


@ranj063 we discussed with Greg KH that virtbus will be used beyond just the matching service instead of creating one.
It is beyond just the matching service, did you discuss this over email to limit the usage when Greg KH agreed last time?

@paravmellanox could you please elaborate on what you mean by "beyond just the matching service". Put differently, what would be missing from the current implementation of the ancillary bus for your needs?

@ranj063
Please follow through the thread and discussion [1] with Greg KH.
We must converge before posting this series and have good enough answer to prove Greg KH suggestions wrong. I am not sure you want to go that path at this point.

[1] https://patchwork.kernel.org/patch/11280547/#23056771

I looked through the linked thread and I didn't see any mention of any functionality beyond the matching. The primary focus of ancillary bus is to provide a way for multiple kernel objects to be matched and have access to a shared object. Emphasis on generic interface and commonality.

If I missed something could you please point me to it?

Thanks

in [2] refer to this line.
cat /sys/bus/virtbus/devices/mlx5_sf.1/sfnum

Ignore the incorrect device name mlx5_sf.1, it is virtdev.1.
But the part to focus is additional sysfs attributes that will be used by systemd to rename the netdevices of this virtdev (sub function).

At beginning there is only handful attribute like sfnum. But as it becomes mature, it will have more attributes like irqs, numa hierarchy of device, because device should be self-contained for such device specific properties.

It has functionality beyond matching, that is: a sub-function device is anchored on the virtbus. There will be netdevice and rdma device attached to this device.
So when someone sees the sysfs files as below there are actual net and rdma devices attached to it.

/sys/bus/virtbus/devices/virtdev.1/net/eth0
/sys/bus/virtbus/devices/virtdev.1/infiniband/mlx5_2

When someone uses the bus as matching service bus, there won't be class devices underneath like above.

This subtle difference and information is capture and acked in thread by Greg KH in [1].

[1] https://patchwork.kernel.org/patch/11280547/#23056985
[2] https://lore.kernel.org/netdev/20200519092258.GF4655@nanopsycho/

paravmellanox · 2020-07-14T17:14:15Z

drivers/bus/ancillary_bus.c

@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Lightweight software based bus for Ancillary devices


@ranj063 I think lightweight prefix is misleading here.

lyakh · 2020-07-15T07:45:40Z

Checked (visually through diffs, so, some error probability remains), that these patches are the same as the last version of #1841 so I could ack this one too as I've acked the original one. There's just one nitpick from me, that I noted in #2264

dmertman · 2020-07-15T18:05:21Z

On the request of @ranj063 (offline discussion) I am going to pull this PR into my Documentation PR, since there is some thrash happening in the Documentation, but this PR looks to be fairly static. This will provide a single location for feedback on the ancillary bus implementation.

paravmellanox · 2020-07-15T18:27:21Z

drivers/bus/ancillary_bus.c

+		goto err;
+	}
+
+	dev_dbg(&adev->dev, "Registering ancillary device '%s'\n",


now new coding standard is 100 characters in line.
Please review whole code to see follow new alignment. Less lines are easier to merge.

paravmellanox · 2020-07-15T18:28:14Z

drivers/bus/ancillary_bus.c

+
+	ret = dev_set_name(&adev->dev, "%s.%d", adev->match_name, adev->id);
+	if (ret) {
+		dev_err(&adev->dev, "dev_set_name failed for device\n");


Can you please check (if not yet checked), if this is legit. Because we fail to set the device name and dev_err will try to print uninitialized device name.

paravmellanox · 2020-07-15T18:28:58Z

drivers/bus/ancillary_bus.c

+
+	ret = adrv->remove(adev);
+	dev_pm_domain_detach(_dev, true);
+


no need for empty line when code before is straight forward.

fredoh9 · 2020-07-15T20:26:43Z

We have decided to consolidate to one PR, #2264. Please move all comments/input to 2264.

dmertman · 2020-07-16T20:23:27Z

-----Original Message----- From: Parav Pandit ***@***.***> Sent: Wednesday, July 15, 2020 11:41 AM To: thesofproject/linux ***@***.***> Cc: Ertman, David M ***@***.***>; Mention ***@***.***> Subject: Re: [thesofproject/linux] [RFC] Use Ancillary bus implementation in SOF (#2265) @paravmellanox commented on this pull request. ________________________________ In Documentation/driver-api/ancillary_bus.rst <#2265 (comment)> : > @@ -0,0 +1,200 @@ +.. SPDX-License-Identifier: GPL-2.0-only + +=========== +Ancillary Bus @parav <https://github.com/parav> - I think what we are trying to avoid here is pollute the ancillary bus code from any subsystem specific semantics. As long as your SF stuff doesn't do that I think we are ok. Hi Shiraz, I am not fully following your comment. Given that sf will have more attributes in sysfs. (a) irq details (b) sfnum (sf number, set by the user during device creation time) (c) resources directory (like pci to tell about what register resources are used) (d) link to parent pci device -> to tell systemd/udev to name class (rdma, net) devices properly (e) if needed iommu support in future.

[DME] These are things that can be stored in the shared object though. There is no reason that the bus has to be involved in keeping track of these things.

…

Not sure if you consider these addition as pollution or enhancement. Initially I wanted separate bus and probed Greg KH for it. But his response in [2] indicates that its ok for this bus to serve dual purpose. [2] https://patchwork.kernel.org/patch/11280547/#23056985 If you think otherwise, we should discuss on mailing list first reviving old thread. But I am afraid of shaking GKH again on this as we asked him many times and he settle on this bus... so... :-) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2265 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe- auth/AH72R5JSAXWM5VNQGLOJ7TLR3XZ5TANCNFSM4OWB76BA> . <https://github.com/notifications/beacon/AH72R5NVZY4EB4ZSHIDERW3R3X Z5TA5CNFSM4OWB76BKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2 FEZLWNFSXPKTDN5WW2ZLOORPWSZGODLDL5GY.gif>

fredoh9 requested review from bardliao, dbaluta, juimonen, keyonjie, kv2019i, lgirdwood, libinyang, lyakh, plbossart, ranj063 and xiulipan as code owners July 9, 2020 23:21

fredoh9 force-pushed the fix/0708_ancillary_bus branch from 1b48a87 to 2efca26 Compare July 9, 2020 23:29

plbossart mentioned this pull request Jul 10, 2020

implementation and usage of ancillary bus #2264

Closed

dmertman and others added 5 commits July 13, 2020 17:36

ASoC: SOF: debug: Remove IPC flood test support in SOF core

8e8e887

Remove the IPC flood test support in the SOF core as it is now added in the IPC flood test client. Signed-off-by: Fred Oh <fred.oh@linux.intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

fredoh9 force-pushed the fix/0708_ancillary_bus branch from 2efca26 to 8e8e887 Compare July 14, 2020 00:38

paravmellanox reviewed Jul 14, 2020

View reviewed changes

paravmellanox reviewed Jul 15, 2020

View reviewed changes

fredoh9 closed this Jul 15, 2020

[RFC] Use Ancillary bus implementation in SOF #2265

[RFC] Use Ancillary bus implementation in SOF #2265

Uh oh!

Conversation

fredoh9 commented Jul 9, 2020

Uh oh!

fredoh9 commented Jul 9, 2020

Uh oh!

fredoh9 commented Jul 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paravmellanox Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh commented Jul 15, 2020

Uh oh!

dmertman commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredoh9 commented Jul 15, 2020

Uh oh!

dmertman commented Jul 16, 2020 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

paravmellanox Jul 14, 2020 •

edited

Loading

ranj063 Jul 14, 2020 •

edited

Loading

dmertman commented Jul 15, 2020 •

edited

Loading