From: Pierre-Louis Bossart <[email protected]>
Now that the auxiliary_bus exists, there's no reason to use platform
devices as children of a PCI device any longer.
This patch refactors the code by extending a basic auxiliary device
with Intel link-specific structures that need to be passed between
controller and link levels. This refactoring is much cleaner with no
need for cross-pointers between device and link structures.
Note that the auxiliary bus API has separate init and add steps, which
requires more attention in the error unwinding paths. The main loop
needs to deal with kfree() and auxiliary_device_uninit() for the
current iteration before jumping to the common label which releases
everything allocated in prior iterations.
Signed-off-by: Pierre-Louis Bossart <[email protected]>
Reviewed-by: Guennadi Liakhovetski <[email protected]>
Reviewed-by: Ranjani Sridharan <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
v2:
- add link_dev_register for all kzalloc, device_init, and device_add.
v3:
- Modify the function description of sdw_intel_probe() which was
missing in previous version.
v4:
- Move intel_link_dev_unregister(ldev) before pm_runtime_put_noidle(
ldev->link_res.dev) to fix use-after-free reported by KASAN.
---
drivers/soundwire/Kconfig | 1 +
drivers/soundwire/intel.c | 56 ++++---
drivers/soundwire/intel.h | 14 +-
drivers/soundwire/intel_init.c | 232 +++++++++++++++++++---------
include/linux/soundwire/sdw_intel.h | 6 +-
5 files changed, 202 insertions(+), 107 deletions(-)
diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
index 016e74230bb7..2b7795233282 100644
--- a/drivers/soundwire/Kconfig
+++ b/drivers/soundwire/Kconfig
@@ -25,6 +25,7 @@ config SOUNDWIRE_INTEL
tristate "Intel SoundWire Master driver"
select SOUNDWIRE_CADENCE
select SOUNDWIRE_GENERIC_ALLOCATION
+ select AUXILIARY_BUS
depends on ACPI && SND_SOC
help
SoundWire Intel Master driver.
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index fd95f94630b1..c11e3d8cd308 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -11,7 +11,7 @@
#include <linux/module.h>
#include <linux/interrupt.h>
#include <linux/io.h>
-#include <linux/platform_device.h>
+#include <linux/auxiliary_bus.h>
#include <sound/pcm_params.h>
#include <linux/pm_runtime.h>
#include <sound/soc.h>
@@ -1327,11 +1327,14 @@ static int intel_init(struct sdw_intel *sdw)
}
/*
- * probe and init
+ * probe and init (aux_dev_id argument is required by function prototype but not used)
*/
-static int intel_master_probe(struct platform_device *pdev)
+static int intel_link_probe(struct auxiliary_device *auxdev,
+ const struct auxiliary_device_id *aux_dev_id)
+
{
- struct device *dev = &pdev->dev;
+ struct device *dev = &auxdev->dev;
+ struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
struct sdw_intel *sdw;
struct sdw_cdns *cdns;
struct sdw_bus *bus;
@@ -1344,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev)
cdns = &sdw->cdns;
bus = &cdns->bus;
- sdw->instance = pdev->id;
- sdw->link_res = dev_get_platdata(dev);
+ sdw->instance = auxdev->id;
+ sdw->link_res = &ldev->link_res;
cdns->dev = dev;
cdns->registers = sdw->link_res->registers;
cdns->instance = sdw->instance;
cdns->msg_count = 0;
- bus->link_id = pdev->id;
+ bus->link_id = auxdev->id;
sdw_cdns_probe(cdns);
@@ -1384,10 +1387,10 @@ static int intel_master_probe(struct platform_device *pdev)
return 0;
}
-int intel_master_startup(struct platform_device *pdev)
+int intel_link_startup(struct auxiliary_device *auxdev)
{
struct sdw_cdns_stream_config config;
- struct device *dev = &pdev->dev;
+ struct device *dev = &auxdev->dev;
struct sdw_cdns *cdns = dev_get_drvdata(dev);
struct sdw_intel *sdw = cdns_to_intel(cdns);
struct sdw_bus *bus = &cdns->bus;
@@ -1524,9 +1527,9 @@ int intel_master_startup(struct platform_device *pdev)
return ret;
}
-static int intel_master_remove(struct platform_device *pdev)
+static void intel_link_remove(struct auxiliary_device *auxdev)
{
- struct device *dev = &pdev->dev;
+ struct device *dev = &auxdev->dev;
struct sdw_cdns *cdns = dev_get_drvdata(dev);
struct sdw_intel *sdw = cdns_to_intel(cdns);
struct sdw_bus *bus = &cdns->bus;
@@ -1542,19 +1545,17 @@ static int intel_master_remove(struct platform_device *pdev)
snd_soc_unregister_component(dev);
}
sdw_bus_master_delete(bus);
-
- return 0;
}
-int intel_master_process_wakeen_event(struct platform_device *pdev)
+int intel_link_process_wakeen_event(struct auxiliary_device *auxdev)
{
- struct device *dev = &pdev->dev;
+ struct device *dev = &auxdev->dev;
struct sdw_intel *sdw;
struct sdw_bus *bus;
void __iomem *shim;
u16 wake_sts;
- sdw = platform_get_drvdata(pdev);
+ sdw = dev_get_drvdata(dev);
bus = &sdw->cdns.bus;
if (bus->prop.hw_disabled) {
@@ -1976,17 +1977,22 @@ static const struct dev_pm_ops intel_pm = {
SET_RUNTIME_PM_OPS(intel_suspend_runtime, intel_resume_runtime, NULL)
};
-static struct platform_driver sdw_intel_drv = {
- .probe = intel_master_probe,
- .remove = intel_master_remove,
+static const struct auxiliary_device_id intel_link_id_table[] = {
+ { .name = "soundwire_intel.link" },
+ {},
+};
+MODULE_DEVICE_TABLE(auxiliary, intel_link_id_table);
+
+static struct auxiliary_driver sdw_intel_drv = {
+ .probe = intel_link_probe,
+ .remove = intel_link_remove,
.driver = {
- .name = "intel-sdw",
+ /* auxiliary_driver_register() sets .name to be the modname */
.pm = &intel_pm,
- }
+ },
+ .id_table = intel_link_id_table
};
-
-module_platform_driver(sdw_intel_drv);
+module_auxiliary_driver(sdw_intel_drv);
MODULE_LICENSE("Dual BSD/GPL");
-MODULE_ALIAS("platform:intel-sdw");
-MODULE_DESCRIPTION("Intel Soundwire Master Driver");
+MODULE_DESCRIPTION("Intel Soundwire Link Driver");
diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index 06bac8ba14e9..0b47b148da3f 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -7,7 +7,6 @@
/**
* struct sdw_intel_link_res - Soundwire Intel link resource structure,
* typically populated by the controller driver.
- * @pdev: platform_device
* @mmio_base: mmio base of SoundWire registers
* @registers: Link IO registers base
* @shim: Audio shim pointer
@@ -23,7 +22,6 @@
* @list: used to walk-through all masters exposed by the same controller
*/
struct sdw_intel_link_res {
- struct platform_device *pdev;
void __iomem *mmio_base; /* not strictly needed, useful for debug */
void __iomem *registers;
void __iomem *shim;
@@ -48,7 +46,15 @@ struct sdw_intel {
#endif
};
-int intel_master_startup(struct platform_device *pdev);
-int intel_master_process_wakeen_event(struct platform_device *pdev);
+int intel_link_startup(struct auxiliary_device *auxdev);
+int intel_link_process_wakeen_event(struct auxiliary_device *auxdev);
+
+struct sdw_intel_link_dev {
+ struct auxiliary_device auxdev;
+ struct sdw_intel_link_res link_res;
+};
+
+#define auxiliary_dev_to_sdw_intel_link_dev(auxiliary_dev) \
+ container_of(auxiliary_dev, struct sdw_intel_link_dev, auxdev)
#endif /* __SDW_INTEL_LOCAL_H */
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index 30ce95ec2d70..9e283bef53d2 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -12,7 +12,7 @@
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/module.h>
-#include <linux/platform_device.h>
+#include <linux/auxiliary_bus.h>
#include <linux/pm_runtime.h>
#include <linux/soundwire/sdw_intel.h>
#include "cadence_master.h"
@@ -24,28 +24,108 @@
#define SDW_LINK_BASE 0x30000
#define SDW_LINK_SIZE 0x10000
+static void intel_link_dev_release(struct device *dev)
+{
+ struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+ struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
+
+ kfree(ldev);
+}
+
+/* alloc, init and add link devices */
+static struct sdw_intel_link_dev *intel_link_dev_register(struct sdw_intel_res *res,
+ struct sdw_intel_ctx *ctx,
+ struct fwnode_handle *fwnode,
+ const char *name,
+ int link_id)
+{
+ struct sdw_intel_link_dev *ldev;
+ struct sdw_intel_link_res *link;
+ struct auxiliary_device *auxdev;
+ int ret;
+
+ ldev = kzalloc(sizeof(*ldev), GFP_KERNEL);
+ if (!ldev)
+ return ERR_PTR(-ENOMEM);
+
+ auxdev = &ldev->auxdev;
+ auxdev->name = name;
+ auxdev->dev.parent = res->parent;
+ auxdev->dev.fwnode = fwnode;
+ auxdev->dev.release = intel_link_dev_release;
+
+ /* we don't use an IDA since we already have a link ID */
+ auxdev->id = link_id;
+
+ /*
+ * keep a handle on the allocated memory, to be used in all other functions.
+ * Since the same pattern is used to skip links that are not enabled, there is
+ * no need to check if ctx->ldev[i] is NULL later on.
+ */
+ ctx->ldev[link_id] = ldev;
+
+ /* Add link information used in the driver probe */
+ link = &ldev->link_res;
+ link->mmio_base = res->mmio_base;
+ link->registers = res->mmio_base + SDW_LINK_BASE
+ + (SDW_LINK_SIZE * link_id);
+ link->shim = res->mmio_base + SDW_SHIM_BASE;
+ link->alh = res->mmio_base + SDW_ALH_BASE;
+
+ link->ops = res->ops;
+ link->dev = res->dev;
+
+ link->clock_stop_quirks = res->clock_stop_quirks;
+ link->shim_lock = &ctx->shim_lock;
+ link->shim_mask = &ctx->shim_mask;
+ link->link_mask = ctx->link_mask;
+
+ /* now follow the two-step init/add sequence */
+ ret = auxiliary_device_init(auxdev);
+ if (ret < 0) {
+ dev_err(res->parent, "failed to initialize link dev %s link_id %d\n",
+ name, link_id);
+ kfree(ldev);
+ return ERR_PTR(ret);
+ }
+
+ ret = auxiliary_device_add(&ldev->auxdev);
+ if (ret < 0) {
+ dev_err(res->parent, "failed to add link dev %s link_id %d\n",
+ ldev->auxdev.name, link_id);
+ /* ldev will be freed with the put_device() and .release sequence */
+ auxiliary_device_uninit(&ldev->auxdev);
+ return ERR_PTR(ret);
+ }
+
+ return ldev;
+}
+
+static void intel_link_dev_unregister(struct sdw_intel_link_dev *ldev)
+{
+ auxiliary_device_delete(&ldev->auxdev);
+ auxiliary_device_uninit(&ldev->auxdev);
+}
+
static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx)
{
- struct sdw_intel_link_res *link = ctx->links;
+ struct sdw_intel_link_dev *ldev;
u32 link_mask;
int i;
- if (!link)
- return 0;
-
link_mask = ctx->link_mask;
- for (i = 0; i < ctx->count; i++, link++) {
+ for (i = 0; i < ctx->count; i++) {
if (!(link_mask & BIT(i)))
continue;
- if (link->pdev) {
- pm_runtime_disable(&link->pdev->dev);
- platform_device_unregister(link->pdev);
- }
+ ldev = ctx->ldev[i];
- if (!link->clock_stop_quirks)
- pm_runtime_put_noidle(link->dev);
+ pm_runtime_disable(&ldev->auxdev.dev);
+ if (!ldev->link_res.clock_stop_quirks)
+ pm_runtime_put_noidle(ldev->link_res.dev);
+
+ intel_link_dev_unregister(ldev);
}
return 0;
@@ -91,9 +171,8 @@ EXPORT_SYMBOL_NS(sdw_intel_thread, SOUNDWIRE_INTEL_INIT);
static struct sdw_intel_ctx
*sdw_intel_probe_controller(struct sdw_intel_res *res)
{
- struct platform_device_info pdevinfo;
- struct platform_device *pdev;
struct sdw_intel_link_res *link;
+ struct sdw_intel_link_dev *ldev;
struct sdw_intel_ctx *ctx;
struct acpi_device *adev;
struct sdw_slave *slave;
@@ -116,67 +195,60 @@ static struct sdw_intel_ctx
count = res->count;
dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);
- ctx = devm_kzalloc(&adev->dev, sizeof(*ctx), GFP_KERNEL);
+ /*
+ * we need to alloc/free memory manually and can't use devm:
+ * this routine may be called from a workqueue, and not from
+ * the parent .probe.
+ * If devm_ was used, the memory might never be freed on errors.
+ */
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
return NULL;
ctx->count = count;
- ctx->links = devm_kcalloc(&adev->dev, ctx->count,
- sizeof(*ctx->links), GFP_KERNEL);
- if (!ctx->links)
+
+ /*
+ * allocate the array of pointers. The link-specific data is allocated
+ * as part of the first loop below and released with the auxiliary_device_uninit().
+ * If some links are disabled, the link pointer will remain NULL. Given that the
+ * number of links is small, this is simpler than using a list to keep track of links.
+ */
+ ctx->ldev = kcalloc(ctx->count, sizeof(*ctx->ldev), GFP_KERNEL);
+ if (!ctx->ldev) {
+ kfree(ctx);
return NULL;
+ }
- ctx->count = count;
ctx->mmio_base = res->mmio_base;
ctx->link_mask = res->link_mask;
ctx->handle = res->handle;
mutex_init(&ctx->shim_lock);
- link = ctx->links;
link_mask = ctx->link_mask;
INIT_LIST_HEAD(&ctx->link_list);
- /* Create SDW Master devices */
- for (i = 0; i < count; i++, link++) {
- if (!(link_mask & BIT(i))) {
- dev_dbg(&adev->dev,
- "Link %d masked, will not be enabled\n", i);
+ for (i = 0; i < count; i++) {
+ if (!(link_mask & BIT(i)))
continue;
- }
- link->mmio_base = res->mmio_base;
- link->registers = res->mmio_base + SDW_LINK_BASE
- + (SDW_LINK_SIZE * i);
- link->shim = res->mmio_base + SDW_SHIM_BASE;
- link->alh = res->mmio_base + SDW_ALH_BASE;
-
- link->ops = res->ops;
- link->dev = res->dev;
-
- link->clock_stop_quirks = res->clock_stop_quirks;
- link->shim_lock = &ctx->shim_lock;
- link->shim_mask = &ctx->shim_mask;
- link->link_mask = link_mask;
-
- memset(&pdevinfo, 0, sizeof(pdevinfo));
-
- pdevinfo.parent = res->parent;
- pdevinfo.name = "intel-sdw";
- pdevinfo.id = i;
- pdevinfo.fwnode = acpi_fwnode_handle(adev);
- pdevinfo.data = link;
- pdevinfo.size_data = sizeof(*link);
-
- pdev = platform_device_register_full(&pdevinfo);
- if (IS_ERR(pdev)) {
- dev_err(&adev->dev,
- "platform device creation failed: %ld\n",
- PTR_ERR(pdev));
+ /*
+ * init and add a device for each link
+ *
+ * The name of the device will be soundwire_intel.link.[i],
+ * with the "soundwire_intel" module prefix automatically added
+ * by the auxiliary bus core.
+ */
+ ldev = intel_link_dev_register(res,
+ ctx,
+ acpi_fwnode_handle(adev),
+ "link",
+ i);
+ if (IS_ERR(ldev))
goto err;
- }
- link->pdev = pdev;
- link->cdns = platform_get_drvdata(pdev);
+
+ link = &ldev->link_res;
+ link->cdns = dev_get_drvdata(&ldev->auxdev.dev);
if (!link->cdns) {
dev_err(&adev->dev, "failed to get link->cdns\n");
@@ -194,8 +266,7 @@ static struct sdw_intel_ctx
num_slaves++;
}
- ctx->ids = devm_kcalloc(&adev->dev, num_slaves,
- sizeof(*ctx->ids), GFP_KERNEL);
+ ctx->ids = kcalloc(num_slaves, sizeof(*ctx->ids), GFP_KERNEL);
if (!ctx->ids)
goto err;
@@ -213,8 +284,14 @@ static struct sdw_intel_ctx
return ctx;
err:
- ctx->count = i;
- sdw_intel_cleanup(ctx);
+ while (i--) {
+ if (!(link_mask & BIT(i)))
+ continue;
+ ldev = ctx->ldev[i];
+ intel_link_dev_unregister(ldev);
+ }
+ kfree(ctx->ldev);
+ kfree(ctx);
return NULL;
}
@@ -222,7 +299,7 @@ static int
sdw_intel_startup_controller(struct sdw_intel_ctx *ctx)
{
struct acpi_device *adev;
- struct sdw_intel_link_res *link;
+ struct sdw_intel_link_dev *ldev;
u32 caps;
u32 link_mask;
int i;
@@ -241,27 +318,28 @@ sdw_intel_startup_controller(struct sdw_intel_ctx *ctx)
return -EINVAL;
}
- if (!ctx->links)
+ if (!ctx->ldev)
return -EINVAL;
- link = ctx->links;
link_mask = ctx->link_mask;
/* Startup SDW Master devices */
- for (i = 0; i < ctx->count; i++, link++) {
+ for (i = 0; i < ctx->count; i++) {
if (!(link_mask & BIT(i)))
continue;
- intel_master_startup(link->pdev);
+ ldev = ctx->ldev[i];
- if (!link->clock_stop_quirks) {
+ intel_link_startup(&ldev->auxdev);
+
+ if (!ldev->link_res.clock_stop_quirks) {
/*
* we need to prevent the parent PCI device
* from entering pm_runtime suspend, so that
* power rails to the SoundWire IP are not
* turned off.
*/
- pm_runtime_get_noresume(link->dev);
+ pm_runtime_get_noresume(ldev->link_res.dev);
}
}
@@ -272,8 +350,8 @@ sdw_intel_startup_controller(struct sdw_intel_ctx *ctx)
* sdw_intel_probe() - SoundWire Intel probe routine
* @res: resource data
*
- * This registers a platform device for each Master handled by the controller,
- * and SoundWire Master and Slave devices will be created by the platform
+ * This registers an auxiliary device for each Master handled by the controller,
+ * and SoundWire Master and Slave devices will be created by the auxiliary
* device probe. All the information necessary is stored in the context, and
* the res argument pointer can be freed after this step.
* This function will be called after sdw_intel_acpi_scan() by SOF probe.
@@ -306,27 +384,31 @@ EXPORT_SYMBOL_NS(sdw_intel_startup, SOUNDWIRE_INTEL_INIT);
void sdw_intel_exit(struct sdw_intel_ctx *ctx)
{
sdw_intel_cleanup(ctx);
+ kfree(ctx->ids);
+ kfree(ctx->ldev);
+ kfree(ctx);
}
EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT);
void sdw_intel_process_wakeen_event(struct sdw_intel_ctx *ctx)
{
- struct sdw_intel_link_res *link;
+ struct sdw_intel_link_dev *ldev;
u32 link_mask;
int i;
- if (!ctx->links)
+ if (!ctx->ldev)
return;
- link = ctx->links;
link_mask = ctx->link_mask;
/* Startup SDW Master devices */
- for (i = 0; i < ctx->count; i++, link++) {
+ for (i = 0; i < ctx->count; i++) {
if (!(link_mask & BIT(i)))
continue;
- intel_master_process_wakeen_event(link->pdev);
+ ldev = ctx->ldev[i];
+
+ intel_link_process_wakeen_event(&ldev->auxdev);
}
}
EXPORT_SYMBOL_NS(sdw_intel_process_wakeen_event, SOUNDWIRE_INTEL_INIT);
diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h
index 3a5446ac014a..1ebea7764011 100644
--- a/include/linux/soundwire/sdw_intel.h
+++ b/include/linux/soundwire/sdw_intel.h
@@ -58,7 +58,7 @@ struct sdw_intel_acpi_info {
u32 link_mask;
};
-struct sdw_intel_link_res;
+struct sdw_intel_link_dev;
/* Intel clock-stop/pm_runtime quirk definitions */
@@ -109,7 +109,7 @@ struct sdw_intel_slave_id {
* Controller
* @num_slaves: total number of devices exposed across all enabled links
* @handle: ACPI parent handle
- * @links: information for each link (controller-specific and kept
+ * @ldev: information for each link (controller-specific and kept
* opaque here)
* @ids: array of slave_id, representing Slaves exposed across all enabled
* links
@@ -123,7 +123,7 @@ struct sdw_intel_ctx {
u32 link_mask;
int num_slaves;
acpi_handle handle;
- struct sdw_intel_link_res *links;
+ struct sdw_intel_link_dev **ldev;
struct sdw_intel_slave_id *ids;
struct list_head link_list;
struct mutex shim_lock; /* lock for access to shared SHIM registers */
--
2.17.1
On 5/11/21 12:21 AM, Bard Liao wrote:
> From: Pierre-Louis Bossart <[email protected]>
>
> Now that the auxiliary_bus exists, there's no reason to use platform
> devices as children of a PCI device any longer.
>
> This patch refactors the code by extending a basic auxiliary device
> with Intel link-specific structures that need to be passed between
> controller and link levels. This refactoring is much cleaner with no
> need for cross-pointers between device and link structures.
>
> Note that the auxiliary bus API has separate init and add steps, which
> requires more attention in the error unwinding paths. The main loop
> needs to deal with kfree() and auxiliary_device_uninit() for the
> current iteration before jumping to the common label which releases
> everything allocated in prior iterations.
>
> Signed-off-by: Pierre-Louis Bossart <[email protected]>
> Reviewed-by: Guennadi Liakhovetski <[email protected]>
> Reviewed-by: Ranjani Sridharan <[email protected]>
> Signed-off-by: Bard Liao <[email protected]>
> ---
> v2:
> - add link_dev_register for all kzalloc, device_init, and device_add.
> v3:
> - Modify the function description of sdw_intel_probe() which was
> missing in previous version.
> v4:
> - Move intel_link_dev_unregister(ldev) before pm_runtime_put_noidle(
> ldev->link_res.dev) to fix use-after-free reported by KASAN.
Two years ago, GregKH stated in
https://lore.kernel.org/lkml/[email protected]/
"So soundwire is creating platform devices? Ick, no! Don't do that"
Fast forward two years, this patch provides a solution to remove the use
of the platform devices with the auxiliary bus. This move does not add
any new functionality, it's just a replacement of one type of device by
another.
The reviews have been rather limited since the first version shared on
March 22.
a) I updated the code to follow the model of the Mellanox driver in
https://elixir.bootlin.com/linux/latest/source/include/linux/mlx5/driver.h#L545
I hope this addresses GregKH's feedback on the need for a 'register'
function to combined the two init and add steps. I didn't see a path to
add a generic register function in the auxiliary bus code since between
init and add there is a need to setup domain-specific structures. Other
contributors to the auxiliary bus (CC:ed) also didn't have a burning
desire to add this capability.
b) Vinod commented:
"What I would like to see the end result is that sdw driver for Intel
controller here is a simple auxdev device and no additional custom setup
layer required... which implies that this handling should be moved into
auxdev or Intel code setting up auxdev..."
I was unable to figure out what this comment hinted at: the auxbus is
already handled in the intel_init.c and intel.c files and the auxbus is
used to model a set of links/managers below the PCI device, not the
controller itself. There is also no such thing as a simple auxdev device
used in the kernel today, the base layer is meant to be extended with
domain-specific structures. There is really no point in creating a
simple auxbus device without extensions.
My reply from March 24 which included technical details on how the
auxiliary bus is designed was not followed by any replies/comments from
Vinod, so I don't know if there is agreement or still a disconnect.
I understand everyone is busy, but I could use more feedback on this
patch. Vinod and Greg can you please chime in on this v4?
Thank you.
> ---
> drivers/soundwire/Kconfig | 1 +
> drivers/soundwire/intel.c | 56 ++++---
> drivers/soundwire/intel.h | 14 +-
> drivers/soundwire/intel_init.c | 232 +++++++++++++++++++---------
> include/linux/soundwire/sdw_intel.h | 6 +-
> 5 files changed, 202 insertions(+), 107 deletions(-)
>
> diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
> index 016e74230bb7..2b7795233282 100644
> --- a/drivers/soundwire/Kconfig
> +++ b/drivers/soundwire/Kconfig
> @@ -25,6 +25,7 @@ config SOUNDWIRE_INTEL
> tristate "Intel SoundWire Master driver"
> select SOUNDWIRE_CADENCE
> select SOUNDWIRE_GENERIC_ALLOCATION
> + select AUXILIARY_BUS
> depends on ACPI && SND_SOC
> help
> SoundWire Intel Master driver.
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index fd95f94630b1..c11e3d8cd308 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -11,7 +11,7 @@
> #include <linux/module.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> -#include <linux/platform_device.h>
> +#include <linux/auxiliary_bus.h>
> #include <sound/pcm_params.h>
> #include <linux/pm_runtime.h>
> #include <sound/soc.h>
> @@ -1327,11 +1327,14 @@ static int intel_init(struct sdw_intel *sdw)
> }
>
> /*
> - * probe and init
> + * probe and init (aux_dev_id argument is required by function prototype but not used)
> */
> -static int intel_master_probe(struct platform_device *pdev)
> +static int intel_link_probe(struct auxiliary_device *auxdev,
> + const struct auxiliary_device_id *aux_dev_id)
> +
> {
> - struct device *dev = &pdev->dev;
> + struct device *dev = &auxdev->dev;
> + struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
> struct sdw_intel *sdw;
> struct sdw_cdns *cdns;
> struct sdw_bus *bus;
> @@ -1344,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev)
> cdns = &sdw->cdns;
> bus = &cdns->bus;
>
> - sdw->instance = pdev->id;
> - sdw->link_res = dev_get_platdata(dev);
> + sdw->instance = auxdev->id;
> + sdw->link_res = &ldev->link_res;
> cdns->dev = dev;
> cdns->registers = sdw->link_res->registers;
> cdns->instance = sdw->instance;
> cdns->msg_count = 0;
>
> - bus->link_id = pdev->id;
> + bus->link_id = auxdev->id;
>
> sdw_cdns_probe(cdns);
>
> @@ -1384,10 +1387,10 @@ static int intel_master_probe(struct platform_device *pdev)
> return 0;
> }
>
> -int intel_master_startup(struct platform_device *pdev)
> +int intel_link_startup(struct auxiliary_device *auxdev)
> {
> struct sdw_cdns_stream_config config;
> - struct device *dev = &pdev->dev;
> + struct device *dev = &auxdev->dev;
> struct sdw_cdns *cdns = dev_get_drvdata(dev);
> struct sdw_intel *sdw = cdns_to_intel(cdns);
> struct sdw_bus *bus = &cdns->bus;
> @@ -1524,9 +1527,9 @@ int intel_master_startup(struct platform_device *pdev)
> return ret;
> }
>
> -static int intel_master_remove(struct platform_device *pdev)
> +static void intel_link_remove(struct auxiliary_device *auxdev)
> {
> - struct device *dev = &pdev->dev;
> + struct device *dev = &auxdev->dev;
> struct sdw_cdns *cdns = dev_get_drvdata(dev);
> struct sdw_intel *sdw = cdns_to_intel(cdns);
> struct sdw_bus *bus = &cdns->bus;
> @@ -1542,19 +1545,17 @@ static int intel_master_remove(struct platform_device *pdev)
> snd_soc_unregister_component(dev);
> }
> sdw_bus_master_delete(bus);
> -
> - return 0;
> }
>
> -int intel_master_process_wakeen_event(struct platform_device *pdev)
> +int intel_link_process_wakeen_event(struct auxiliary_device *auxdev)
> {
> - struct device *dev = &pdev->dev;
> + struct device *dev = &auxdev->dev;
> struct sdw_intel *sdw;
> struct sdw_bus *bus;
> void __iomem *shim;
> u16 wake_sts;
>
> - sdw = platform_get_drvdata(pdev);
> + sdw = dev_get_drvdata(dev);
> bus = &sdw->cdns.bus;
>
> if (bus->prop.hw_disabled) {
> @@ -1976,17 +1977,22 @@ static const struct dev_pm_ops intel_pm = {
> SET_RUNTIME_PM_OPS(intel_suspend_runtime, intel_resume_runtime, NULL)
> };
>
> -static struct platform_driver sdw_intel_drv = {
> - .probe = intel_master_probe,
> - .remove = intel_master_remove,
> +static const struct auxiliary_device_id intel_link_id_table[] = {
> + { .name = "soundwire_intel.link" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(auxiliary, intel_link_id_table);
> +
> +static struct auxiliary_driver sdw_intel_drv = {
> + .probe = intel_link_probe,
> + .remove = intel_link_remove,
> .driver = {
> - .name = "intel-sdw",
> + /* auxiliary_driver_register() sets .name to be the modname */
> .pm = &intel_pm,
> - }
> + },
> + .id_table = intel_link_id_table
> };
> -
> -module_platform_driver(sdw_intel_drv);
> +module_auxiliary_driver(sdw_intel_drv);
>
> MODULE_LICENSE("Dual BSD/GPL");
> -MODULE_ALIAS("platform:intel-sdw");
> -MODULE_DESCRIPTION("Intel Soundwire Master Driver");
> +MODULE_DESCRIPTION("Intel Soundwire Link Driver");
> diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
> index 06bac8ba14e9..0b47b148da3f 100644
> --- a/drivers/soundwire/intel.h
> +++ b/drivers/soundwire/intel.h
> @@ -7,7 +7,6 @@
> /**
> * struct sdw_intel_link_res - Soundwire Intel link resource structure,
> * typically populated by the controller driver.
> - * @pdev: platform_device
> * @mmio_base: mmio base of SoundWire registers
> * @registers: Link IO registers base
> * @shim: Audio shim pointer
> @@ -23,7 +22,6 @@
> * @list: used to walk-through all masters exposed by the same controller
> */
> struct sdw_intel_link_res {
> - struct platform_device *pdev;
> void __iomem *mmio_base; /* not strictly needed, useful for debug */
> void __iomem *registers;
> void __iomem *shim;
> @@ -48,7 +46,15 @@ struct sdw_intel {
> #endif
> };
>
> -int intel_master_startup(struct platform_device *pdev);
> -int intel_master_process_wakeen_event(struct platform_device *pdev);
> +int intel_link_startup(struct auxiliary_device *auxdev);
> +int intel_link_process_wakeen_event(struct auxiliary_device *auxdev);
> +
> +struct sdw_intel_link_dev {
> + struct auxiliary_device auxdev;
> + struct sdw_intel_link_res link_res;
> +};
> +
> +#define auxiliary_dev_to_sdw_intel_link_dev(auxiliary_dev) \
> + container_of(auxiliary_dev, struct sdw_intel_link_dev, auxdev)
>
> #endif /* __SDW_INTEL_LOCAL_H */
> diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
> index 30ce95ec2d70..9e283bef53d2 100644
> --- a/drivers/soundwire/intel_init.c
> +++ b/drivers/soundwire/intel_init.c
> @@ -12,7 +12,7 @@
> #include <linux/interrupt.h>
> #include <linux/io.h>
> #include <linux/module.h>
> -#include <linux/platform_device.h>
> +#include <linux/auxiliary_bus.h>
> #include <linux/pm_runtime.h>
> #include <linux/soundwire/sdw_intel.h>
> #include "cadence_master.h"
> @@ -24,28 +24,108 @@
> #define SDW_LINK_BASE 0x30000
> #define SDW_LINK_SIZE 0x10000
>
> +static void intel_link_dev_release(struct device *dev)
> +{
> + struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> + struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
> +
> + kfree(ldev);
> +}
> +
> +/* alloc, init and add link devices */
> +static struct sdw_intel_link_dev *intel_link_dev_register(struct sdw_intel_res *res,
> + struct sdw_intel_ctx *ctx,
> + struct fwnode_handle *fwnode,
> + const char *name,
> + int link_id)
> +{
> + struct sdw_intel_link_dev *ldev;
> + struct sdw_intel_link_res *link;
> + struct auxiliary_device *auxdev;
> + int ret;
> +
> + ldev = kzalloc(sizeof(*ldev), GFP_KERNEL);
> + if (!ldev)
> + return ERR_PTR(-ENOMEM);
> +
> + auxdev = &ldev->auxdev;
> + auxdev->name = name;
> + auxdev->dev.parent = res->parent;
> + auxdev->dev.fwnode = fwnode;
> + auxdev->dev.release = intel_link_dev_release;
> +
> + /* we don't use an IDA since we already have a link ID */
> + auxdev->id = link_id;
> +
> + /*
> + * keep a handle on the allocated memory, to be used in all other functions.
> + * Since the same pattern is used to skip links that are not enabled, there is
> + * no need to check if ctx->ldev[i] is NULL later on.
> + */
> + ctx->ldev[link_id] = ldev;
> +
> + /* Add link information used in the driver probe */
> + link = &ldev->link_res;
> + link->mmio_base = res->mmio_base;
> + link->registers = res->mmio_base + SDW_LINK_BASE
> + + (SDW_LINK_SIZE * link_id);
> + link->shim = res->mmio_base + SDW_SHIM_BASE;
> + link->alh = res->mmio_base + SDW_ALH_BASE;
> +
> + link->ops = res->ops;
> + link->dev = res->dev;
> +
> + link->clock_stop_quirks = res->clock_stop_quirks;
> + link->shim_lock = &ctx->shim_lock;
> + link->shim_mask = &ctx->shim_mask;
> + link->link_mask = ctx->link_mask;
> +
> + /* now follow the two-step init/add sequence */
> + ret = auxiliary_device_init(auxdev);
> + if (ret < 0) {
> + dev_err(res->parent, "failed to initialize link dev %s link_id %d\n",
> + name, link_id);
> + kfree(ldev);
> + return ERR_PTR(ret);
> + }
> +
> + ret = auxiliary_device_add(&ldev->auxdev);
> + if (ret < 0) {
> + dev_err(res->parent, "failed to add link dev %s link_id %d\n",
> + ldev->auxdev.name, link_id);
> + /* ldev will be freed with the put_device() and .release sequence */
> + auxiliary_device_uninit(&ldev->auxdev);
> + return ERR_PTR(ret);
> + }
> +
> + return ldev;
> +}
> +
> +static void intel_link_dev_unregister(struct sdw_intel_link_dev *ldev)
> +{
> + auxiliary_device_delete(&ldev->auxdev);
> + auxiliary_device_uninit(&ldev->auxdev);
> +}
> +
> static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx)
> {
> - struct sdw_intel_link_res *link = ctx->links;
> + struct sdw_intel_link_dev *ldev;
> u32 link_mask;
> int i;
>
> - if (!link)
> - return 0;
> -
> link_mask = ctx->link_mask;
>
> - for (i = 0; i < ctx->count; i++, link++) {
> + for (i = 0; i < ctx->count; i++) {
> if (!(link_mask & BIT(i)))
> continue;
>
> - if (link->pdev) {
> - pm_runtime_disable(&link->pdev->dev);
> - platform_device_unregister(link->pdev);
> - }
> + ldev = ctx->ldev[i];
>
> - if (!link->clock_stop_quirks)
> - pm_runtime_put_noidle(link->dev);
> + pm_runtime_disable(&ldev->auxdev.dev);
> + if (!ldev->link_res.clock_stop_quirks)
> + pm_runtime_put_noidle(ldev->link_res.dev);
> +
> + intel_link_dev_unregister(ldev);
> }
>
> return 0;
> @@ -91,9 +171,8 @@ EXPORT_SYMBOL_NS(sdw_intel_thread, SOUNDWIRE_INTEL_INIT);
> static struct sdw_intel_ctx
> *sdw_intel_probe_controller(struct sdw_intel_res *res)
> {
> - struct platform_device_info pdevinfo;
> - struct platform_device *pdev;
> struct sdw_intel_link_res *link;
> + struct sdw_intel_link_dev *ldev;
> struct sdw_intel_ctx *ctx;
> struct acpi_device *adev;
> struct sdw_slave *slave;
> @@ -116,67 +195,60 @@ static struct sdw_intel_ctx
> count = res->count;
> dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);
>
> - ctx = devm_kzalloc(&adev->dev, sizeof(*ctx), GFP_KERNEL);
> + /*
> + * we need to alloc/free memory manually and can't use devm:
> + * this routine may be called from a workqueue, and not from
> + * the parent .probe.
> + * If devm_ was used, the memory might never be freed on errors.
> + */
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> if (!ctx)
> return NULL;
>
> ctx->count = count;
> - ctx->links = devm_kcalloc(&adev->dev, ctx->count,
> - sizeof(*ctx->links), GFP_KERNEL);
> - if (!ctx->links)
> +
> + /*
> + * allocate the array of pointers. The link-specific data is allocated
> + * as part of the first loop below and released with the auxiliary_device_uninit().
> + * If some links are disabled, the link pointer will remain NULL. Given that the
> + * number of links is small, this is simpler than using a list to keep track of links.
> + */
> + ctx->ldev = kcalloc(ctx->count, sizeof(*ctx->ldev), GFP_KERNEL);
> + if (!ctx->ldev) {
> + kfree(ctx);
> return NULL;
> + }
>
> - ctx->count = count;
> ctx->mmio_base = res->mmio_base;
> ctx->link_mask = res->link_mask;
> ctx->handle = res->handle;
> mutex_init(&ctx->shim_lock);
>
> - link = ctx->links;
> link_mask = ctx->link_mask;
>
> INIT_LIST_HEAD(&ctx->link_list);
>
> - /* Create SDW Master devices */
> - for (i = 0; i < count; i++, link++) {
> - if (!(link_mask & BIT(i))) {
> - dev_dbg(&adev->dev,
> - "Link %d masked, will not be enabled\n", i);
> + for (i = 0; i < count; i++) {
> + if (!(link_mask & BIT(i)))
> continue;
> - }
>
> - link->mmio_base = res->mmio_base;
> - link->registers = res->mmio_base + SDW_LINK_BASE
> - + (SDW_LINK_SIZE * i);
> - link->shim = res->mmio_base + SDW_SHIM_BASE;
> - link->alh = res->mmio_base + SDW_ALH_BASE;
> -
> - link->ops = res->ops;
> - link->dev = res->dev;
> -
> - link->clock_stop_quirks = res->clock_stop_quirks;
> - link->shim_lock = &ctx->shim_lock;
> - link->shim_mask = &ctx->shim_mask;
> - link->link_mask = link_mask;
> -
> - memset(&pdevinfo, 0, sizeof(pdevinfo));
> -
> - pdevinfo.parent = res->parent;
> - pdevinfo.name = "intel-sdw";
> - pdevinfo.id = i;
> - pdevinfo.fwnode = acpi_fwnode_handle(adev);
> - pdevinfo.data = link;
> - pdevinfo.size_data = sizeof(*link);
> -
> - pdev = platform_device_register_full(&pdevinfo);
> - if (IS_ERR(pdev)) {
> - dev_err(&adev->dev,
> - "platform device creation failed: %ld\n",
> - PTR_ERR(pdev));
> + /*
> + * init and add a device for each link
> + *
> + * The name of the device will be soundwire_intel.link.[i],
> + * with the "soundwire_intel" module prefix automatically added
> + * by the auxiliary bus core.
> + */
> + ldev = intel_link_dev_register(res,
> + ctx,
> + acpi_fwnode_handle(adev),
> + "link",
> + i);
> + if (IS_ERR(ldev))
> goto err;
> - }
> - link->pdev = pdev;
> - link->cdns = platform_get_drvdata(pdev);
> +
> + link = &ldev->link_res;
> + link->cdns = dev_get_drvdata(&ldev->auxdev.dev);
>
> if (!link->cdns) {
> dev_err(&adev->dev, "failed to get link->cdns\n");
> @@ -194,8 +266,7 @@ static struct sdw_intel_ctx
> num_slaves++;
> }
>
> - ctx->ids = devm_kcalloc(&adev->dev, num_slaves,
> - sizeof(*ctx->ids), GFP_KERNEL);
> + ctx->ids = kcalloc(num_slaves, sizeof(*ctx->ids), GFP_KERNEL);
> if (!ctx->ids)
> goto err;
>
> @@ -213,8 +284,14 @@ static struct sdw_intel_ctx
> return ctx;
>
> err:
> - ctx->count = i;
> - sdw_intel_cleanup(ctx);
> + while (i--) {
> + if (!(link_mask & BIT(i)))
> + continue;
> + ldev = ctx->ldev[i];
> + intel_link_dev_unregister(ldev);
> + }
> + kfree(ctx->ldev);
> + kfree(ctx);
> return NULL;
> }
>
> @@ -222,7 +299,7 @@ static int
> sdw_intel_startup_controller(struct sdw_intel_ctx *ctx)
> {
> struct acpi_device *adev;
> - struct sdw_intel_link_res *link;
> + struct sdw_intel_link_dev *ldev;
> u32 caps;
> u32 link_mask;
> int i;
> @@ -241,27 +318,28 @@ sdw_intel_startup_controller(struct sdw_intel_ctx *ctx)
> return -EINVAL;
> }
>
> - if (!ctx->links)
> + if (!ctx->ldev)
> return -EINVAL;
>
> - link = ctx->links;
> link_mask = ctx->link_mask;
>
> /* Startup SDW Master devices */
> - for (i = 0; i < ctx->count; i++, link++) {
> + for (i = 0; i < ctx->count; i++) {
> if (!(link_mask & BIT(i)))
> continue;
>
> - intel_master_startup(link->pdev);
> + ldev = ctx->ldev[i];
>
> - if (!link->clock_stop_quirks) {
> + intel_link_startup(&ldev->auxdev);
> +
> + if (!ldev->link_res.clock_stop_quirks) {
> /*
> * we need to prevent the parent PCI device
> * from entering pm_runtime suspend, so that
> * power rails to the SoundWire IP are not
> * turned off.
> */
> - pm_runtime_get_noresume(link->dev);
> + pm_runtime_get_noresume(ldev->link_res.dev);
> }
> }
>
> @@ -272,8 +350,8 @@ sdw_intel_startup_controller(struct sdw_intel_ctx *ctx)
> * sdw_intel_probe() - SoundWire Intel probe routine
> * @res: resource data
> *
> - * This registers a platform device for each Master handled by the controller,
> - * and SoundWire Master and Slave devices will be created by the platform
> + * This registers an auxiliary device for each Master handled by the controller,
> + * and SoundWire Master and Slave devices will be created by the auxiliary
> * device probe. All the information necessary is stored in the context, and
> * the res argument pointer can be freed after this step.
> * This function will be called after sdw_intel_acpi_scan() by SOF probe.
> @@ -306,27 +384,31 @@ EXPORT_SYMBOL_NS(sdw_intel_startup, SOUNDWIRE_INTEL_INIT);
> void sdw_intel_exit(struct sdw_intel_ctx *ctx)
> {
> sdw_intel_cleanup(ctx);
> + kfree(ctx->ids);
> + kfree(ctx->ldev);
> + kfree(ctx);
> }
> EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT);
>
> void sdw_intel_process_wakeen_event(struct sdw_intel_ctx *ctx)
> {
> - struct sdw_intel_link_res *link;
> + struct sdw_intel_link_dev *ldev;
> u32 link_mask;
> int i;
>
> - if (!ctx->links)
> + if (!ctx->ldev)
> return;
>
> - link = ctx->links;
> link_mask = ctx->link_mask;
>
> /* Startup SDW Master devices */
> - for (i = 0; i < ctx->count; i++, link++) {
> + for (i = 0; i < ctx->count; i++) {
> if (!(link_mask & BIT(i)))
> continue;
>
> - intel_master_process_wakeen_event(link->pdev);
> + ldev = ctx->ldev[i];
> +
> + intel_link_process_wakeen_event(&ldev->auxdev);
> }
> }
> EXPORT_SYMBOL_NS(sdw_intel_process_wakeen_event, SOUNDWIRE_INTEL_INIT);
> diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h
> index 3a5446ac014a..1ebea7764011 100644
> --- a/include/linux/soundwire/sdw_intel.h
> +++ b/include/linux/soundwire/sdw_intel.h
> @@ -58,7 +58,7 @@ struct sdw_intel_acpi_info {
> u32 link_mask;
> };
>
> -struct sdw_intel_link_res;
> +struct sdw_intel_link_dev;
>
> /* Intel clock-stop/pm_runtime quirk definitions */
>
> @@ -109,7 +109,7 @@ struct sdw_intel_slave_id {
> * Controller
> * @num_slaves: total number of devices exposed across all enabled links
> * @handle: ACPI parent handle
> - * @links: information for each link (controller-specific and kept
> + * @ldev: information for each link (controller-specific and kept
> * opaque here)
> * @ids: array of slave_id, representing Slaves exposed across all enabled
> * links
> @@ -123,7 +123,7 @@ struct sdw_intel_ctx {
> u32 link_mask;
> int num_slaves;
> acpi_handle handle;
> - struct sdw_intel_link_res *links;
> + struct sdw_intel_link_dev **ldev;
> struct sdw_intel_slave_id *ids;
> struct list_head link_list;
> struct mutex shim_lock; /* lock for access to shared SHIM registers */
>
On 25-05-21, 13:30, Pierre-Louis Bossart wrote:
> On 5/11/21 12:21 AM, Bard Liao wrote:
> > From: Pierre-Louis Bossart <[email protected]>
> >
> > Now that the auxiliary_bus exists, there's no reason to use platform
> > devices as children of a PCI device any longer.
> >
> > This patch refactors the code by extending a basic auxiliary device
> > with Intel link-specific structures that need to be passed between
> > controller and link levels. This refactoring is much cleaner with no
> > need for cross-pointers between device and link structures.
> >
> > Note that the auxiliary bus API has separate init and add steps, which
> > requires more attention in the error unwinding paths. The main loop
> > needs to deal with kfree() and auxiliary_device_uninit() for the
> > current iteration before jumping to the common label which releases
> > everything allocated in prior iterations.
> >
> > Signed-off-by: Pierre-Louis Bossart <[email protected]>
> > Reviewed-by: Guennadi Liakhovetski <[email protected]>
> > Reviewed-by: Ranjani Sridharan <[email protected]>
> > Signed-off-by: Bard Liao <[email protected]>
> > ---
> > v2:
> > - add link_dev_register for all kzalloc, device_init, and device_add.
> > v3:
> > - Modify the function description of sdw_intel_probe() which was
> > missing in previous version.
> > v4:
> > - Move intel_link_dev_unregister(ldev) before pm_runtime_put_noidle(
> > ldev->link_res.dev) to fix use-after-free reported by KASAN.
>
> Two years ago, GregKH stated in
> https://lore.kernel.org/lkml/[email protected]/
>
> "So soundwire is creating platform devices? Ick, no! Don't do that"
>
> Fast forward two years, this patch provides a solution to remove the use of
> the platform devices with the auxiliary bus. This move does not add any new
> functionality, it's just a replacement of one type of device by another.
>
> The reviews have been rather limited since the first version shared on March
> 22.
>
> a) I updated the code to follow the model of the Mellanox driver in
>
> https://elixir.bootlin.com/linux/latest/source/include/linux/mlx5/driver.h#L545
>
> I hope this addresses GregKH's feedback on the need for a 'register'
> function to combined the two init and add steps. I didn't see a path to add
> a generic register function in the auxiliary bus code since between init and
> add there is a need to setup domain-specific structures. Other contributors
> to the auxiliary bus (CC:ed) also didn't have a burning desire to add this
> capability.
>
> b) Vinod commented:
>
> "What I would like to see the end result is that sdw driver for Intel
> controller here is a simple auxdev device and no additional custom setup
> layer required... which implies that this handling should be moved into
> auxdev or Intel code setting up auxdev..."
>
> I was unable to figure out what this comment hinted at: the auxbus is
> already handled in the intel_init.c and intel.c files and the auxbus is used
> to model a set of links/managers below the PCI device, not the controller
> itself. There is also no such thing as a simple auxdev device used in the
> kernel today, the base layer is meant to be extended with domain-specific
> structures. There is really no point in creating a simple auxbus device
> without extensions.
<back from vacations>
I would like to see that the init_init.c removed completely, that is my
ask here
This layer was created by me to aid in creating the platform devices.
Also the mistake was not to use platform resources and instead pass a
custom structure for resources (device iomem address, irq etc)
I would like to see is the PCI/SOF parent driver create the sdw aux
device and that should be all needed to be done. The aux device would be
probed by sdw driver. No custom resource structs for resources please.
If that is not possible, I would like to understand technical details of
why that would be that case. If required necessary changes should be
made to aux bus to handle and not have sequencing issue which you had
trouble with platform approach.
Thanks
--
~Vinod
>> b) Vinod commented:
>>
>> "What I would like to see the end result is that sdw driver for Intel
>> controller here is a simple auxdev device and no additional custom setup
>> layer required... which implies that this handling should be moved into
>> auxdev or Intel code setting up auxdev..."
>>
>> I was unable to figure out what this comment hinted at: the auxbus is
>> already handled in the intel_init.c and intel.c files and the auxbus is used
>> to model a set of links/managers below the PCI device, not the controller
>> itself. There is also no such thing as a simple auxdev device used in the
>> kernel today, the base layer is meant to be extended with domain-specific
>> structures. There is really no point in creating a simple auxbus device
>> without extensions.
>
> <back from vacations>
same here :-)
> I would like to see that the init_init.c removed completely, that is my
> ask here
>
> This layer was created by me to aid in creating the platform devices.
> Also the mistake was not to use platform resources and instead pass a
> custom structure for resources (device iomem address, irq etc)
We are 100% aligned on the ask to remove intel_init.c, this layer is
unnecessary and adds more work for developers/maintainers. We will move
all this in the SOF driver.
> I would like to see is the PCI/SOF parent driver create the sdw aux
> device and that should be all needed to be done. The aux device would be
> probed by sdw driver. No custom resource structs for resources please.
I was following the previous paragraph but got stuck on the last
sentence 'no custom structs for resources', see below.
> If that is not possible, I would like to understand technical details of
> why that would be that case. If required necessary changes should be
> made to aux bus to handle and not have sequencing issue which you had
> trouble with platform approach.
I don't know what you are referring to with the 'sequencing issue which
you had trouble with platform approach'. We never had any technical
issues with platform devices, the solution works and has been
productized. We are only doing this iso-functionality transition because
GregKH asked us to do only use platform devices IF there is a real
platform device (controlled by DT/ACPI).
I think we are also having language/specification issues here. I don't
understand what you describe as a 'resource' - there is no interaction
with firmware - nor how we can avoid being domain-specific for something
that is Intel-specific.
Let's go back to the code to help the discussion: the auxiliary driver
which manages a SoundWire link needs to be provided with a 'custom'
structure that describes basic information provided by the PCI parent
(link masks, quirks, IO register bases) and contains internal fields
needed for the link management (mutex, ops, list, etc). This is the
structure we use:
struct sdw_intel_link_res {
void __iomem *mmio_base; /* not strictly needed, useful for debug */
void __iomem *registers;
void __iomem *shim;
void __iomem *alh;
int irq;
const struct sdw_intel_ops *ops;
struct device *dev;
struct mutex *shim_lock; /* protect shared registers */
u32 *shim_mask;
u32 clock_stop_quirks;
u32 link_mask;
struct sdw_cdns *cdns;
struct list_head list;
};
We could if it was desired for architectural clarity split this
structure in what is provided by the parent and what is used inside of
the auxiliary driver as an internal context that the parent doesn't
touch, but these definitions are again Intel-specific.
Then both types of information are included in the 'link_dev' extension
of the auxiliary device.
struct sdw_intel_link_dev {
struct auxiliary_device auxdev;
struct sdw_intel_link_res link_res;
};
That's the basic design of the auxiliary bus, domain-specific data
structures are not added inside of the auxiliary_device but are part of
an extension accessed with container_of(). That's what everyone using
the auxiliary bus is doing.
Vinod, if you can elaborate on what 'resources' refer to in your reply
that would help. We've been using the same approach as others relying on
the auxiliary bus and I am struggling to see what is wrong with the
solution we suggested, or what changes to the auxiliary bus core would
be needed. I don't mind doing something different but I just don't
understand what the suggestion is.
Thanks!
Hi Pierre,
You might want to check your setting, this and some other mail (not all
though) sent by you seem to have landed up in my spam folder, dont know
why gmail is doing that...
On 01-06-21, 08:56, Pierre-Louis Bossart wrote:
>
> > > b) Vinod commented:
> > >
> > > "What I would like to see the end result is that sdw driver for Intel
> > > controller here is a simple auxdev device and no additional custom setup
> > > layer required... which implies that this handling should be moved into
> > > auxdev or Intel code setting up auxdev..."
> > >
> > > I was unable to figure out what this comment hinted at: the auxbus is
> > > already handled in the intel_init.c and intel.c files and the auxbus is used
> > > to model a set of links/managers below the PCI device, not the controller
> > > itself. There is also no such thing as a simple auxdev device used in the
> > > kernel today, the base layer is meant to be extended with domain-specific
> > > structures. There is really no point in creating a simple auxbus device
> > > without extensions.
> >
> > <back from vacations>
>
> same here :-)
>
> > I would like to see that the init_init.c removed completely, that is my
> > ask here
> >
> > This layer was created by me to aid in creating the platform devices.
> > Also the mistake was not to use platform resources and instead pass a
> > custom structure for resources (device iomem address, irq etc)
>
> We are 100% aligned on the ask to remove intel_init.c, this layer is
> unnecessary and adds more work for developers/maintainers. We will move all
> this in the SOF driver.
>
> > I would like to see is the PCI/SOF parent driver create the sdw aux
> > device and that should be all needed to be done. The aux device would be
> > probed by sdw driver. No custom resource structs for resources please.
> I was following the previous paragraph but got stuck on the last sentence
> 'no custom structs for resources', see below.
>
> > If that is not possible, I would like to understand technical details of
> > why that would be that case. If required necessary changes should be
> > made to aux bus to handle and not have sequencing issue which you had
> > trouble with platform approach.
>
> I don't know what you are referring to with the 'sequencing issue which you
> had trouble with platform approach'. We never had any technical issues with
> platform devices, the solution works and has been productized. We are only
> doing this iso-functionality transition because GregKH asked us to do only
> use platform devices IF there is a real platform device (controlled by
> DT/ACPI).
>
> I think we are also having language/specification issues here. I don't
> understand what you describe as a 'resource' - there is no interaction with
> firmware - nor how we can avoid being domain-specific for something that is
> Intel-specific.
>
> Let's go back to the code to help the discussion: the auxiliary driver which
> manages a SoundWire link needs to be provided with a 'custom' structure that
> describes basic information provided by the PCI parent (link masks, quirks,
> IO register bases) and contains internal fields needed for the link
> management (mutex, ops, list, etc). This is the structure we use:
>
> struct sdw_intel_link_res {
> void __iomem *mmio_base; /* not strictly needed, useful for debug */
> void __iomem *registers;
> void __iomem *shim;
> void __iomem *alh;
These are resources and any auxiliary_device should add this. That way
while creating you can set up. Hint look at how platform_device sets up
resources
> int irq;
irq is a generic field and should be again moved into auxiliary_device
> const struct sdw_intel_ops *ops;
This is for callbacks right? Why cant the sdw aux driver call APIs
exported by SOF driver?
> struct device *dev;
Why do you need a dev pointer here? Is this parent or something else?
> struct mutex *shim_lock; /* protect shared registers */
Okay so you serialize the access to shim across sdw and sof right?
export an api from sof driver and get rid of lock here
> u32 *shim_mask;
> u32 clock_stop_quirks;
> u32 link_mask;
> struct sdw_cdns *cdns;
> struct list_head list;
these sound as internal data to sdw instance, move into intel
driver instances
> };
>
> We could if it was desired for architectural clarity split this structure in
> what is provided by the parent and what is used inside of the auxiliary
> driver as an internal context that the parent doesn't touch, but these
> definitions are again Intel-specific.
So rather than think Intel specfic, I would suggest you think in generic
terms. You have a child auxiliary_device (think like PCI etc), add
the generic resources like iomem regions, irq etc and call into SOF
driver. That would make sdw driver look neat and help you get rid of
this bits
>
> Then both types of information are included in the 'link_dev' extension of
> the auxiliary device.
>
> struct sdw_intel_link_dev {
> struct auxiliary_device auxdev;
> struct sdw_intel_link_res link_res;
> };
>
> That's the basic design of the auxiliary bus, domain-specific data
> structures are not added inside of the auxiliary_device but are part of an
> extension accessed with container_of(). That's what everyone using the
> auxiliary bus is doing.
I would say resources (as illustrated above) are not domain-specific
data but a generic stuff which any type of device object should contain
> Vinod, if you can elaborate on what 'resources' refer to in your reply that
> would help. We've been using the same approach as others relying on the
> auxiliary bus and I am struggling to see what is wrong with the solution we
> suggested, or what changes to the auxiliary bus core would be needed. I
> don't mind doing something different but I just don't understand what the
> suggestion is.
I think auxiliary_device needs to look more like a real device rather
than a simple wrapper as it is now and put heavy onus on implementers.
Device drivers should be simple and boring. The details should be
handled in bus
Thanks
--
~Vinod
On Wed, Jun 09, 2021 at 09:44:08AM -0500, Pierre-Louis Bossart wrote:
> The consensus for the auxiliary_device model was hard to reach, and the
> agreement was to align on a minimal model. If you disagree with the
> directions, you will have to convince Nvidia/Mellanox and Intel networking
> folks who contributed the solution to do something different.
The purpose of the aux devices was primarily to bind a *software*
interface between two parts of the kernel.
If there is a strong defined HW boundary and no software interface
then the mfd subsytem may be a better choice.
For a software layer I expect to see some 'handle' and then a set of
APIs to work within that. It is OK if that 'handle' refers to some HW
resources that the API needs to work, the purpose of this is to
control HW after all.
You might help Vinod by explaining what the SW API is here.
Jason
>> The consensus for the auxiliary_device model was hard to reach, and the
>> agreement was to align on a minimal model. If you disagree with the
>> directions, you will have to convince Nvidia/Mellanox and Intel networking
>> folks who contributed the solution to do something different.
>
> The purpose of the aux devices was primarily to bind a *software*
> interface between two parts of the kernel.
The auxiliary bus documentation states clearly that we wanted to
partition the functionality of a specific hardware into separate parts
that are not exposed through ACPI/DT.
See excerpts from
https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html
"In some subsystems, the functionality of the core device
(PCI/ACPI/other) is too complex for a single device to be managed by a
monolithic driver (e.g. Sound Open Firmware)" <- that's us.
This is the case for our audio controller, which exposes 4 SoundWire
links. Since the links can be individually power-managed, creating 4
subdevices below the PCI parent is a natural design.
"An example for this kind of requirement is the audio subsystem where a
single IP is handling multiple entities such as HDMI, Soundwire, local
devices such as mics/speakers etc:" <- that's also us
Péter Ujfalusi is working on this part to split the DSP capabilities and
let different 'clients' control them. That's a different case where we
partition 'firmware' capabilities, not hardware as in the case of SoundWire.
> If there is a strong defined HW boundary and no software interface
> then the mfd subsytem may be a better choice.
That objection has been made before, there were lengthy threads on this
between GregKH, Mark Brown and others. Clearly if we go back to this
kind of debates I will respectfully stick to platform devices until
maintainers agree.
This is beyond my area of expertise, outside of my control, and I've
spent enough time trying to move away from platform devices - we've been
at it for 2 years.
The auxiliary bus as suggested in this patch works fine. We don't have
any needs that are not handled by the auxiliary bus code as of today,
and we are not planning any future extensions.
> For a software layer I expect to see some 'handle' and then a set of
> APIs to work within that. It is OK if that 'handle' refers to some HW
> resources that the API needs to work, the purpose of this is to
> control HW after all.
>
> You might help Vinod by explaining what the SW API is here.
There is no suggested change in API, what we use today for the platform
devices is exactly the same as what we need for auxiliary bus devices.
We are not creating something new for SoundWire, just substituting one
type of devices for another.
On 6/8/21 11:46 PM, Vinod Koul wrote:
> Hi Pierre,
>
> You might want to check your setting, this and some other mail (not all
> though) sent by you seem to have landed up in my spam folder, dont know
> why gmail is doing that...
I haven't changed any of my configurations, not sure what happens?
> On 01-06-21, 08:56, Pierre-Louis Bossart wrote:
>>
>>>> b) Vinod commented:
>>>>
>>>> "What I would like to see the end result is that sdw driver for Intel
>>>> controller here is a simple auxdev device and no additional custom setup
>>>> layer required... which implies that this handling should be moved into
>>>> auxdev or Intel code setting up auxdev..."
>>>>
>>>> I was unable to figure out what this comment hinted at: the auxbus is
>>>> already handled in the intel_init.c and intel.c files and the auxbus is used
>>>> to model a set of links/managers below the PCI device, not the controller
>>>> itself. There is also no such thing as a simple auxdev device used in the
>>>> kernel today, the base layer is meant to be extended with domain-specific
>>>> structures. There is really no point in creating a simple auxbus device
>>>> without extensions.
>>>
>>> <back from vacations>
>>
>> same here :-)
>>
>>> I would like to see that the init_init.c removed completely, that is my
>>> ask here
>>>
>>> This layer was created by me to aid in creating the platform devices.
>>> Also the mistake was not to use platform resources and instead pass a
>>> custom structure for resources (device iomem address, irq etc)
>>
>> We are 100% aligned on the ask to remove intel_init.c, this layer is
>> unnecessary and adds more work for developers/maintainers. We will move all
>> this in the SOF driver.
>>
>>> I would like to see is the PCI/SOF parent driver create the sdw aux
>>> device and that should be all needed to be done. The aux device would be
>>> probed by sdw driver. No custom resource structs for resources please.
>> I was following the previous paragraph but got stuck on the last sentence
>> 'no custom structs for resources', see below.
>>
>>> If that is not possible, I would like to understand technical details of
>>> why that would be that case. If required necessary changes should be
>>> made to aux bus to handle and not have sequencing issue which you had
>>> trouble with platform approach.
>>
>> I don't know what you are referring to with the 'sequencing issue which you
>> had trouble with platform approach'. We never had any technical issues with
>> platform devices, the solution works and has been productized. We are only
>> doing this iso-functionality transition because GregKH asked us to do only
>> use platform devices IF there is a real platform device (controlled by
>> DT/ACPI).
>>
>> I think we are also having language/specification issues here. I don't
>> understand what you describe as a 'resource' - there is no interaction with
>> firmware - nor how we can avoid being domain-specific for something that is
>> Intel-specific.
>>
>> Let's go back to the code to help the discussion: the auxiliary driver which
>> manages a SoundWire link needs to be provided with a 'custom' structure that
>> describes basic information provided by the PCI parent (link masks, quirks,
>> IO register bases) and contains internal fields needed for the link
>> management (mutex, ops, list, etc). This is the structure we use:
>>
>> struct sdw_intel_link_res {
>> void __iomem *mmio_base; /* not strictly needed, useful for debug */
>> void __iomem *registers;
>> void __iomem *shim;
>> void __iomem *alh;
>
> These are resources and any auxiliary_device should add this. That way
> while creating you can set up. Hint look at how platform_device sets up
> resources
If you look at the *existing* code, we don't handle any "resources" with
the platform devices, we use the platform_device_info.data to pass the
link information. It's a void pointer. We do not touch the resource
field in the platform_device_into at all.
https://elixir.bootlin.com/linux/latest/source/drivers/soundwire/intel_init.c#L168
>> int irq;
>
> irq is a generic field and should be again moved into auxiliary_device
It's information passed by the parent so that all links use the same
irq. We added this maybe 1.5 years ago after spending months chasing
race conditions that we could not root cause. there's nothing generic
about this field.
>> const struct sdw_intel_ops *ops;
>
> This is for callbacks right? Why cant the sdw aux driver call APIs
> exported by SOF driver?
this is part of the context, this could be moved to a different structure.
>> struct device *dev;
>
> Why do you need a dev pointer here? Is this parent or something else?
for convenience for runtime_pm, there are cases where the link can
suspend but the parent has to remain active due to power rail
dependencies, so we need to handle pm_runtime_get_noresume() and
pm_runtime_put_noidle().
https://elixir.bootlin.com/linux/latest/source/drivers/soundwire/intel.h#L25
We already use this field *today*, this isn't new. I guess we could use
dev->parent but that'd be a different patch.
>> struct mutex *shim_lock; /* protect shared registers */
>
> Okay so you serialize the access to shim across sdw and sof right?
> export an api from sof driver and get rid of lock here
this is again something we do today. This is not a new field.
see the description here:
https://elixir.bootlin.com/linux/latest/source/drivers/soundwire/intel.h#L25
This is not about serialization between SOF and SDW, only SDW drivers
access the shim. It's about serialization between the different SDW
driver instances accessing common hardware registers. Nothing new.
>> u32 *shim_mask;
>> u32 clock_stop_quirks;
>> u32 link_mask;
>> struct sdw_cdns *cdns;
>> struct list_head list;
>
>
> these sound as internal data to sdw instance, move into intel
> driver instances
what intel driver?
We have a PCI Intel driver for the parent (SOF) and a driver instance
for each SoundWire link - probed when the parent creates the different
SoundWire devices.
we need to have an Intel link driver which is different from the SOF
driver used for the parent. This is information needed at the child level.
>> };
>>
>> We could if it was desired for architectural clarity split this structure in
>> what is provided by the parent and what is used inside of the auxiliary
>> driver as an internal context that the parent doesn't touch, but these
>> definitions are again Intel-specific.
>
> So rather than think Intel specfic, I would suggest you think in generic
> terms. You have a child auxiliary_device (think like PCI etc), add
> the generic resources like iomem regions, irq etc and call into SOF
> driver. That would make sdw driver look neat and help you get rid of
> this bits
Not able to get what this means, sorry. the child device should not
'call into the SOF driver', mixing parent and child layers leads to
disaster in general.
The model is exactly the same as what we have today with the platform
devices. We did not add ANY new fields or information, what is passed in
that structure is exactly the same as what we do upstream today with the
platform devices.
To make my point, here is the structure in intel.h as of v5.13-rc1
struct sdw_intel_link_res {
struct platform_device *pdev;
void __iomem *mmio_base; /* not strictly needed, useful for debug */
void __iomem *registers;
void __iomem *shim;
void __iomem *alh;
int irq;
const struct sdw_intel_ops *ops;
struct device *dev;
struct mutex *shim_lock; /* protect shared registers */
u32 *shim_mask;
u32 clock_stop_quirks;
u32 link_mask;
struct sdw_cdns *cdns;
struct list_head list;
};
and here's what we suggested in this patch:
struct sdw_intel_link_res {
void __iomem *mmio_base; /* not strictly needed, useful for debug */
void __iomem *registers;
void __iomem *shim;
void __iomem *alh;
int irq;
const struct sdw_intel_ops *ops;
struct device *dev;
struct mutex *shim_lock; /* protect shared registers */
u32 *shim_mask;
u32 clock_stop_quirks;
u32 link_mask;
struct sdw_cdns *cdns;
struct list_head list;
};
You will notice that we removed the platform_device *pdev, but embedded
this structure into a larger one to make use of container_of()
struct sdw_intel_link_dev {
struct auxiliary_device auxdev;
struct sdw_intel_link_res link_res;
};
That's it. We did not change anything else, all the other fields are
identical. We are only changing the TYPE of device and the interfaces
for probe/remove but using the same information and the same device
hierarchy.
>> Then both types of information are included in the 'link_dev' extension of
>> the auxiliary device.
>>
>> struct sdw_intel_link_dev {
>> struct auxiliary_device auxdev;
>> struct sdw_intel_link_res link_res;
>> };
>>
>> That's the basic design of the auxiliary bus, domain-specific data
>> structures are not added inside of the auxiliary_device but are part of an
>> extension accessed with container_of(). That's what everyone using the
>> auxiliary bus is doing.
>
> I would say resources (as illustrated above) are not domain-specific
> data but a generic stuff which any type of device object should contain
??
>> Vinod, if you can elaborate on what 'resources' refer to in your reply that
>> would help. We've been using the same approach as others relying on the
>> auxiliary bus and I am struggling to see what is wrong with the solution we
>> suggested, or what changes to the auxiliary bus core would be needed. I
>> don't mind doing something different but I just don't understand what the
>> suggestion is.
>
> I think auxiliary_device needs to look more like a real device rather
> than a simple wrapper as it is now and put heavy onus on implementers.
The consensus for the auxiliary_device model was hard to reach, and the
agreement was to align on a minimal model. If you disagree with the
directions, you will have to convince Nvidia/Mellanox and Intel
networking folks who contributed the solution to do something different.
I also don't see what's heavy, we are not adding new complexity compared
to the use of the platform devices. It's the same code that implementers
need to provide, there is no additional cost.
> Device drivers should be simple and boring. The details should be
> handled in bus
The auxiliary bus is minimal on purpose and cannot contain details if it
used in areas as diverse as networking, SOF clients and SoundWire child
devices. The 'details' need to be handled as domain-specific extensions.
This patch only suggests a modification from platform devices to
auxiliary devices. That's it. Iso functionality. No new features or
concepts. No new fields. No performance/footprint/cost change.
I did not ask to do this work and I don't have have any emotional
attachment to this work. I was trying to make GregKH happy after he
mentioned more than 2 years ago that plaform devices should not be used
when there isn't an ACPI/DT description.
If you don't agree with the directions, we will withdraw this patch and
stay with the platform devices. There are no negative impacts from a
performance perspective, but it's not what GregKH wanted. I try to make
both of you happy, if this doesn't happen then there's no solution, is
there?
On Wed, Jun 09, 2021 at 09:44:08AM -0500, Pierre-Louis Bossart wrote:
> The model is exactly the same as what we have today with the platform
> devices. We did not add ANY new fields or information, what is passed in
> that structure is exactly the same as what we do upstream today with the
> platform devices.
>
> To make my point, here is the structure in intel.h as of v5.13-rc1
>
> struct sdw_intel_link_res {
> struct platform_device *pdev;
> void __iomem *mmio_base; /* not strictly needed, useful for debug */
> void __iomem *registers;
> void __iomem *shim;
> void __iomem *alh;
> int irq;
> const struct sdw_intel_ops *ops;
> struct device *dev;
> struct mutex *shim_lock; /* protect shared registers */
> u32 *shim_mask;
> u32 clock_stop_quirks;
> u32 link_mask;
> struct sdw_cdns *cdns;
> struct list_head list;
> };
>
> and here's what we suggested in this patch:
>
> struct sdw_intel_link_res {
> void __iomem *mmio_base; /* not strictly needed, useful for debug */
> void __iomem *registers;
> void __iomem *shim;
> void __iomem *alh;
> int irq;
> const struct sdw_intel_ops *ops;
> struct device *dev;
> struct mutex *shim_lock; /* protect shared registers */
> u32 *shim_mask;
> u32 clock_stop_quirks;
> u32 link_mask;
> struct sdw_cdns *cdns;
> struct list_head list;
> };
>
> You will notice that we removed the platform_device *pdev, but embedded this
> structure into a larger one to make use of container_of()
>
> struct sdw_intel_link_dev {
> struct auxiliary_device auxdev;
> struct sdw_intel_link_res link_res;
> };
>
> That's it. We did not change anything else, all the other fields are
> identical. We are only changing the TYPE of device and the interfaces for
> probe/remove but using the same information and the same device hierarchy.
And this is the correct thing to do, you have done it properly here,
nice work.
greg k-h
On 09-06-21, 12:10, Jason Gunthorpe wrote:
> On Wed, Jun 09, 2021 at 09:44:08AM -0500, Pierre-Louis Bossart wrote:
>
> > The consensus for the auxiliary_device model was hard to reach, and the
> > agreement was to align on a minimal model. If you disagree with the
> > directions, you will have to convince Nvidia/Mellanox and Intel networking
> > folks who contributed the solution to do something different.
>
> The purpose of the aux devices was primarily to bind a *software*
> interface between two parts of the kernel.
Then I dont think this example is valid... This example has a PCI device,
which represents a DSP, HDA controller, DMICs, Soundwire
links... So at least here it is hardware.
> If there is a strong defined HW boundary and no software interface
> then the mfd subsytem may be a better choice.
More I think that might be better choice for this example, but then MFD
is a 'platform device' and Greg already nacked that
> For a software layer I expect to see some 'handle' and then a set of
> APIs to work within that. It is OK if that 'handle' refers to some HW
> resources that the API needs to work, the purpose of this is to
> control HW after all.
>
> You might help Vinod by explaining what the SW API is here.
>
> Jason
--
~Vinod
On 09-06-21, 09:44, Pierre-Louis Bossart wrote:
>
>
> On 6/8/21 11:46 PM, Vinod Koul wrote:
> > Hi Pierre,
> >
> > You might want to check your setting, this and some other mail (not all
> > though) sent by you seem to have landed up in my spam folder, dont know
> > why gmail is doing that...
>
> I haven't changed any of my configurations, not sure what happens?
>
> > On 01-06-21, 08:56, Pierre-Louis Bossart wrote:
> > >
> > > > > b) Vinod commented:
> > > > >
> > > > > "What I would like to see the end result is that sdw driver for Intel
> > > > > controller here is a simple auxdev device and no additional custom setup
> > > > > layer required... which implies that this handling should be moved into
> > > > > auxdev or Intel code setting up auxdev..."
> > > > >
> > > > > I was unable to figure out what this comment hinted at: the auxbus is
> > > > > already handled in the intel_init.c and intel.c files and the auxbus is used
> > > > > to model a set of links/managers below the PCI device, not the controller
> > > > > itself. There is also no such thing as a simple auxdev device used in the
> > > > > kernel today, the base layer is meant to be extended with domain-specific
> > > > > structures. There is really no point in creating a simple auxbus device
> > > > > without extensions.
> > > >
> > > > <back from vacations>
> > >
> > > same here :-)
> > >
> > > > I would like to see that the init_init.c removed completely, that is my
> > > > ask here
> > > >
> > > > This layer was created by me to aid in creating the platform devices.
> > > > Also the mistake was not to use platform resources and instead pass a
> > > > custom structure for resources (device iomem address, irq etc)
> > >
> > > We are 100% aligned on the ask to remove intel_init.c, this layer is
> > > unnecessary and adds more work for developers/maintainers. We will move all
> > > this in the SOF driver.
> > >
> > > > I would like to see is the PCI/SOF parent driver create the sdw aux
> > > > device and that should be all needed to be done. The aux device would be
> > > > probed by sdw driver. No custom resource structs for resources please.
> > > I was following the previous paragraph but got stuck on the last sentence
> > > 'no custom structs for resources', see below.
> > >
> > > > If that is not possible, I would like to understand technical details of
> > > > why that would be that case. If required necessary changes should be
> > > > made to aux bus to handle and not have sequencing issue which you had
> > > > trouble with platform approach.
> > >
> > > I don't know what you are referring to with the 'sequencing issue which you
> > > had trouble with platform approach'. We never had any technical issues with
> > > platform devices, the solution works and has been productized. We are only
> > > doing this iso-functionality transition because GregKH asked us to do only
> > > use platform devices IF there is a real platform device (controlled by
> > > DT/ACPI).
> > >
> > > I think we are also having language/specification issues here. I don't
> > > understand what you describe as a 'resource' - there is no interaction with
> > > firmware - nor how we can avoid being domain-specific for something that is
> > > Intel-specific.
> > >
> > > Let's go back to the code to help the discussion: the auxiliary driver which
> > > manages a SoundWire link needs to be provided with a 'custom' structure that
> > > describes basic information provided by the PCI parent (link masks, quirks,
> > > IO register bases) and contains internal fields needed for the link
> > > management (mutex, ops, list, etc). This is the structure we use:
> > >
> > > struct sdw_intel_link_res {
> > > void __iomem *mmio_base; /* not strictly needed, useful for debug */
> > > void __iomem *registers;
> > > void __iomem *shim;
> > > void __iomem *alh;
> >
> > These are resources and any auxiliary_device should add this. That way
> > while creating you can set up. Hint look at how platform_device sets up
> > resources
>
> If you look at the *existing* code, we don't handle any "resources" with the
> platform devices, we use the platform_device_info.data to pass the link
> information. It's a void pointer. We do not touch the resource field in the
> platform_device_into at all.
Yes that is true I dont disagree on that part. My ask here is to make it
better, it can be followed up after this but I would at least like to
agree on the direction.
> https://elixir.bootlin.com/linux/latest/source/drivers/soundwire/intel_init.c#L168
>
> > > int irq;
> >
> > irq is a generic field and should be again moved into auxiliary_device
>
> It's information passed by the parent so that all links use the same irq. We
> added this maybe 1.5 years ago after spending months chasing race conditions
> that we could not root cause. there's nothing generic about this field.
>
> > > const struct sdw_intel_ops *ops;
> >
> > This is for callbacks right? Why cant the sdw aux driver call APIs
> > exported by SOF driver?
>
> this is part of the context, this could be moved to a different structure.
ok
> > > struct device *dev;
> >
> > Why do you need a dev pointer here? Is this parent or something else?
>
> for convenience for runtime_pm, there are cases where the link can suspend
> but the parent has to remain active due to power rail dependencies, so we
> need to handle pm_runtime_get_noresume() and pm_runtime_put_noidle().
>
> https://elixir.bootlin.com/linux/latest/source/drivers/soundwire/intel.h#L25
>
> We already use this field *today*, this isn't new. I guess we could use
> dev->parent but that'd be a different patch.
Absolutely, that should be a different patch.
>
> > > struct mutex *shim_lock; /* protect shared registers */
> >
> > Okay so you serialize the access to shim across sdw and sof right?
> > export an api from sof driver and get rid of lock here
>
> this is again something we do today. This is not a new field.
>
> see the description here:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/soundwire/intel.h#L25
>
> This is not about serialization between SOF and SDW, only SDW drivers access
> the shim. It's about serialization between the different SDW driver
> instances accessing common hardware registers. Nothing new.
Yes this is existing and can be improved upon. I guess can be moved to
SOF driver?
>
> > > u32 *shim_mask;
> > > u32 clock_stop_quirks;
> > > u32 link_mask;
> > > struct sdw_cdns *cdns;
> > > struct list_head list;
> >
> >
> > these sound as internal data to sdw instance, move into intel
> > driver instances
>
> what intel driver?
Should have been clear, sdw intel driver
>
> We have a PCI Intel driver for the parent (SOF) and a driver instance for
> each SoundWire link - probed when the parent creates the different SoundWire
> devices.
>
> we need to have an Intel link driver which is different from the SOF driver
> used for the parent. This is information needed at the child level.
>
> > > };
> > >
> > > We could if it was desired for architectural clarity split this structure in
> > > what is provided by the parent and what is used inside of the auxiliary
> > > driver as an internal context that the parent doesn't touch, but these
> > > definitions are again Intel-specific.
> >
> > So rather than think Intel specfic, I would suggest you think in generic
> > terms. You have a child auxiliary_device (think like PCI etc), add
> > the generic resources like iomem regions, irq etc and call into SOF
> > driver. That would make sdw driver look neat and help you get rid of
> > this bits
>
> Not able to get what this means, sorry. the child device should not 'call
> into the SOF driver', mixing parent and child layers leads to disaster in
> general.
>
> The model is exactly the same as what we have today with the platform
> devices. We did not add ANY new fields or information, what is passed in
> that structure is exactly the same as what we do upstream today with the
> platform devices.
Yes that is the correct thing to do from conversion point of view. But
as part of conversion, as a follow up patches I would like to see things
improved. My ask here again is to remove the init layer. I would have
liked the resources like irq and iomem ones moved into aux device, but
looks like consensus is that aux device will not support that!
> To make my point, here is the structure in intel.h as of v5.13-rc1
>
> struct sdw_intel_link_res {
> struct platform_device *pdev;
> void __iomem *mmio_base; /* not strictly needed, useful for debug */
> void __iomem *registers;
> void __iomem *shim;
> void __iomem *alh;
> int irq;
> const struct sdw_intel_ops *ops;
> struct device *dev;
> struct mutex *shim_lock; /* protect shared registers */
> u32 *shim_mask;
> u32 clock_stop_quirks;
> u32 link_mask;
> struct sdw_cdns *cdns;
> struct list_head list;
> };
>
> and here's what we suggested in this patch:
>
> struct sdw_intel_link_res {
> void __iomem *mmio_base; /* not strictly needed, useful for debug */
> void __iomem *registers;
> void __iomem *shim;
> void __iomem *alh;
> int irq;
> const struct sdw_intel_ops *ops;
> struct device *dev;
> struct mutex *shim_lock; /* protect shared registers */
> u32 *shim_mask;
> u32 clock_stop_quirks;
> u32 link_mask;
> struct sdw_cdns *cdns;
> struct list_head list;
> };
>
> You will notice that we removed the platform_device *pdev, but embedded this
> structure into a larger one to make use of container_of()
>
> struct sdw_intel_link_dev {
> struct auxiliary_device auxdev;
> struct sdw_intel_link_res link_res;
> };
>
> That's it. We did not change anything else, all the other fields are
> identical. We are only changing the TYPE of device and the interfaces for
> probe/remove but using the same information and the same device hierarchy.
The move in itself is okay but I dont think that should be the end goal.
--
~Vinod
On Fri, Jun 11, 2021 at 04:56:52PM +0530, Vinod Koul wrote:
> On 09-06-21, 12:10, Jason Gunthorpe wrote:
> > On Wed, Jun 09, 2021 at 09:44:08AM -0500, Pierre-Louis Bossart wrote:
> >
> > > The consensus for the auxiliary_device model was hard to reach, and the
> > > agreement was to align on a minimal model. If you disagree with the
> > > directions, you will have to convince Nvidia/Mellanox and Intel networking
> > > folks who contributed the solution to do something different.
> >
> > The purpose of the aux devices was primarily to bind a *software*
> > interface between two parts of the kernel.
>
> Then I dont think this example is valid... This example has a PCI device,
> which represents a DSP, HDA controller, DMICs, Soundwire
> links... So at least here it is hardware.
Yes, and that's fine, and exactly what aux devices were created for.
You divide up a single logically addressable device into differently
handled portions.
Not all hardware specs were as "smart" as USB was in allowing multiple
drivers to bind to the same physical USB device and talk to it at the
same time :)
Luckily the USB spec authors learned from the mistakes of PCI...
thanks,
greg k-h
Thanks Vinod for your feedback,
>> If you look at the *existing* code, we don't handle any "resources" with the
>> platform devices, we use the platform_device_info.data to pass the link
>> information. It's a void pointer. We do not touch the resource field in the
>> platform_device_into at all.
>
> Yes that is true I dont disagree on that part. My ask here is to make it
> better, it can be followed up after this but I would at least like to
> agree on the direction.
[...]
>> That's it. We did not change anything else, all the other fields are
>> identical. We are only changing the TYPE of device and the interfaces for
>> probe/remove but using the same information and the same device hierarchy.
>
> The move in itself is okay but I dont think that should be the end goal.
What we suggested in this patch is only an iso-functionality change. I
believe from Greg's and your feedback that there is no objection on that
small step.
This is not the end-goal indeed. The second step would be to remove the
intel_init.c file. I fully agree with you Vinod that this can be moved
into the SOF driver, and we could do this in a follow-up step. We can
also improve the partition between 'context' used by the child driver
and information passed by the parent on SHIM registers and bases.
I think we'd need to agree on the details of the second step, Bard and I
can work on a proposal, but I don't see a disconnect on the direction to
simplify the interface. That's the right thing to do.
On 11-05-21, 13:21, Bard Liao wrote:
> From: Pierre-Louis Bossart <[email protected]>
>
> Now that the auxiliary_bus exists, there's no reason to use platform
> devices as children of a PCI device any longer.
>
> This patch refactors the code by extending a basic auxiliary device
> with Intel link-specific structures that need to be passed between
> controller and link levels. This refactoring is much cleaner with no
> need for cross-pointers between device and link structures.
>
> Note that the auxiliary bus API has separate init and add steps, which
> requires more attention in the error unwinding paths. The main loop
> needs to deal with kfree() and auxiliary_device_uninit() for the
> current iteration before jumping to the common label which releases
> everything allocated in prior iterations.
Applied, thanks
--
~Vinod
On 11-06-21, 09:51, Pierre-Louis Bossart wrote:
> Thanks Vinod for your feedback,
>
> > > If you look at the *existing* code, we don't handle any "resources" with the
> > > platform devices, we use the platform_device_info.data to pass the link
> > > information. It's a void pointer. We do not touch the resource field in the
> > > platform_device_into at all.
> >
> > Yes that is true I dont disagree on that part. My ask here is to make it
> > better, it can be followed up after this but I would at least like to
> > agree on the direction.
>
> [...]
>
> > > That's it. We did not change anything else, all the other fields are
> > > identical. We are only changing the TYPE of device and the interfaces for
> > > probe/remove but using the same information and the same device hierarchy.
> >
> > The move in itself is okay but I dont think that should be the end goal.
>
> What we suggested in this patch is only an iso-functionality change. I
> believe from Greg's and your feedback that there is no objection on that
> small step.
>
> This is not the end-goal indeed. The second step would be to remove the
> intel_init.c file. I fully agree with you Vinod that this can be moved into
> the SOF driver, and we could do this in a follow-up step. We can also
> improve the partition between 'context' used by the child driver and
> information passed by the parent on SHIM registers and bases.
>
> I think we'd need to agree on the details of the second step, Bard and I can
> work on a proposal, but I don't see a disconnect on the direction to
> simplify the interface. That's the right thing to do.
That is agreeable to me. Looking forward to updates to clean these bits
up
Thanks
--
~Vinod