Re: [alsa-devel] [RFC PATCH 5/7] soundwire: add debugfs support
On Mon, May 06, 2019 at 10:08:10PM +0530, Vinod Koul wrote: > Yes, but then device exit routine is supposed to do debugfs cleanup as > well, so that would ensure these references are dropped at that point of > time. Greg should that not take care of it or we *should* always do > refcounting. Always do refcounting. How else can you "guarantee" that it is safe?
Re: [alsa-devel] [RFC PATCH 5/7] soundwire: add debugfs support
On 5/6/19 11:38 AM, Vinod Koul wrote:
On 06-05-19, 09:48, Pierre-Louis Bossart wrote:
+struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
+{
+ if (d)
+ return d->fs;
+ return NULL;
+}
+EXPORT_SYMBOL(sdw_bus_debugfs_get_root);
_GPL()?
Oops, that's a big miss. will fix, thanks for spotting this.
Not really. The Soundwire code is dual licensed. Many of the soundwire
symbols are indeed exported as EXPORT_SYMBOL. But I agree this one is
'linux' specific so can be made _GPL.
Pierre, does Intel still care about this being dual licensed or not?
Debugfs was never in scope for the dual-licensed parts, we've already
agreed for SOF to move to _GPL.
But why is this exported at all? No one calls this function.
I will have to check.
It is used by codec driver which are not upstream yet. So my suggestion
would be NOT to export this and only do so when we have users for it
That would be true for other APIs exported out as well.
It'll just make the first codec driver patchset more complicated but fine.
Re: [alsa-devel] [RFC PATCH 5/7] soundwire: add debugfs support
On 06-05-19, 09:48, Pierre-Louis Bossart wrote:
> > > +struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
> > > +{
> > > + if (d)
> > > + return d->fs;
> > > + return NULL;
> > > +}
> > > +EXPORT_SYMBOL(sdw_bus_debugfs_get_root);
> >
> > _GPL()?
>
> Oops, that's a big miss. will fix, thanks for spotting this.
Not really. The Soundwire code is dual licensed. Many of the soundwire
symbols are indeed exported as EXPORT_SYMBOL. But I agree this one is
'linux' specific so can be made _GPL.
Pierre, does Intel still care about this being dual licensed or not?
>
> >
> > But why is this exported at all? No one calls this function.
>
> I will have to check.
It is used by codec driver which are not upstream yet. So my suggestion
would be NOT to export this and only do so when we have users for it
That would be true for other APIs exported out as well.
> >
> > > +struct sdw_slave_debugfs {
> > > + struct sdw_slave *slave;
> >
> > Same question as above, why do you need this pointer?
>
> will check.
The deubugfs code does hold a ref to slave object to read the data and
dump, this particular instance might be able to get rid of (should be
doable)
> > And meta-comment, if you _EVER_ save off a pointer to a reference
> > counted object (like this and the above one), you HAVE to grab a
> > reference to it, otherwise it can go away at any point in time as that
> > is the point of reference counted objects.
> >
> > So even if you do need/want this, you have to properly handle the
> > reference count by incrementing/decrementing it as needed.
Yes, but then device exit routine is supposed to do debugfs cleanup as
well, so that would ensure these references are dropped at that point of
time. Greg should that not take care of it or we *should* always do
refcounting.
Thanks
--
~Vinod
Re: [alsa-devel] [RFC PATCH 5/7] soundwire: add debugfs support
@@ -136,6 +139,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
void sdw_delete_bus_master(struct sdw_bus *bus)
{
sdw_sysfs_bus_exit(bus);
+ if (bus->debugfs)
+ sdw_bus_debugfs_exit(bus->debugfs);
No need to check, just call it.
That was on my todo list, will remove.
+struct sdw_bus_debugfs {
+ struct sdw_bus *bus;
Why do you need to save this pointer?
+ struct dentry *fs;
This really is all you need to have around, right?
will check.
+struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
+{
+ if (d)
+ return d->fs;
+ return NULL;
+}
+EXPORT_SYMBOL(sdw_bus_debugfs_get_root);
_GPL()?
Oops, that's a big miss. will fix, thanks for spotting this.
But why is this exported at all? No one calls this function.
I will have to check.
+struct sdw_slave_debugfs {
+ struct sdw_slave *slave;
Same question as above, why do you need this pointer?
will check.
And meta-comment, if you _EVER_ save off a pointer to a reference
counted object (like this and the above one), you HAVE to grab a
reference to it, otherwise it can go away at any point in time as that
is the point of reference counted objects.
So even if you do need/want this, you have to properly handle the
reference count by incrementing/decrementing it as needed.
good comment, thank you for the guidance.
+struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave)
+{
+ struct sdw_bus_debugfs *master;
+ struct sdw_slave_debugfs *d;
+ char name[32];
+
+ master = slave->bus->debugfs;
+ if (!master)
+ return NULL;
+
+ d = kzalloc(sizeof(*d), GFP_KERNEL);
+ if (!d)
+ return NULL;
+
+ /* create the debugfs slave-name */
+ snprintf(name, sizeof(name), "%s", dev_name(&slave->dev));
+ d->fs = debugfs_create_dir(name, master->fs);
+ if (IS_ERR_OR_NULL(d->fs)) {
+ dev_err(&slave->dev, "slave debugfs root creation failed\n");
+ goto err;
+ }
You never care about the return value of a debugfs call. I have a 100+
patch series stripping all of this out of the kernel, please don't force
me to add another one to it :)
Just call debugfs and move on, you can always put the return value of
one call into another one just fine, and your function logic should
never change if debugfs returns an error or not, you do not care.
Yes, it's agreed that we should not depend on debugfs or fail here. will
fix, no worries.
+void sdw_debugfs_init(void)
+{
+ sdw_debugfs_root = debugfs_create_dir("soundwire", NULL);
+ if (IS_ERR_OR_NULL(sdw_debugfs_root)) {
+ pr_warn("SoundWire: Failed to create debugfs directory\n");
+ sdw_debugfs_root = NULL;
+ return;
Same here, just call the function and return.
yep, will do.
Re: [RFC PATCH 5/7] soundwire: add debugfs support
On Fri, May 03, 2019 at 08:00:28PM -0500, Pierre-Louis Bossart wrote:
> Add base debugfs mechanism for SoundWire bus by creating soundwire
> root and master-N and slave-x hierarchy.
>
> Also add SDW Slave SCP, DP0 and DP-N register debug file.
>
> Registers not implemented will print as "XX"
>
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
> is the use of scnprintf to avoid known issues with snprintf.
>
> Signed-off-by: Pierre-Louis Bossart
> ---
> drivers/soundwire/Makefile| 3 +-
> drivers/soundwire/bus.c | 5 +
> drivers/soundwire/bus.h | 28 +
> drivers/soundwire/bus_type.c | 3 +
> drivers/soundwire/debugfs.c | 214 ++
> drivers/soundwire/slave.c | 1 +
> include/linux/soundwire/sdw.h | 9 ++
> 7 files changed, 262 insertions(+), 1 deletion(-)
> create mode 100644 drivers/soundwire/debugfs.c
>
> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> index a72a29731a28..f2c425fa15bd 100644
> --- a/drivers/soundwire/Makefile
> +++ b/drivers/soundwire/Makefile
> @@ -4,7 +4,8 @@
>
> #Bus Objs
> soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
> - sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o
> + sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o \
> + debugfs.o
> obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
>
> #Cadence Objs
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index dd9181693554..b79eba321b71 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -53,6 +53,8 @@ int sdw_add_bus_master(struct sdw_bus *bus)
> if (ret < 0)
> dev_warn(bus->dev, "Bus sysfs init failed:%d\n", ret);
>
> + bus->debugfs = sdw_bus_debugfs_init(bus);
> +
> /*
>* Device numbers in SoundWire are 0 through 15. Enumeration device
>* number (0), Broadcast device number (15), Group numbers (12 and
> @@ -114,6 +116,7 @@ static int sdw_delete_slave(struct device *dev, void
> *data)
> struct sdw_bus *bus = slave->bus;
>
> sdw_sysfs_slave_exit(slave);
> + sdw_slave_debugfs_exit(slave->debugfs);
>
> mutex_lock(&bus->bus_lock);
>
> @@ -136,6 +139,8 @@ static int sdw_delete_slave(struct device *dev, void
> *data)
> void sdw_delete_bus_master(struct sdw_bus *bus)
> {
> sdw_sysfs_bus_exit(bus);
> + if (bus->debugfs)
> + sdw_bus_debugfs_exit(bus->debugfs);
No need to check, just call it.
>
> device_for_each_child(bus->dev, NULL, sdw_delete_slave);
> }
> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> index 0707e68a8d21..f0b1f4f9d7b2 100644
> --- a/drivers/soundwire/bus.h
> +++ b/drivers/soundwire/bus.h
> @@ -20,6 +20,34 @@ void sdw_extract_slave_id(struct sdw_bus *bus,
>
> extern const struct attribute_group *sdw_slave_dev_attr_groups[];
>
> +#ifdef CONFIG_DEBUG_FS
> +struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus);
> +void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d);
> +struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d);
> +struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave);
> +void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d);
> +void sdw_debugfs_init(void);
> +void sdw_debugfs_exit(void);
> +#else
> +struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus)
> +{ return NULL; }
> +
> +void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d) {}
> +
> +struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
> +{ return NULL; }
> +
> +struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave)
> +{ return NULL; }
> +
> +void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d) {}
> +
> +void sdw_debugfs_init(void) {}
> +
> +void sdw_debugfs_exit(void) {}
> +
> +#endif
> +
> enum {
> SDW_MSG_FLAG_READ = 0,
> SDW_MSG_FLAG_WRITE,
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index f68fe45c1037..f28c1a2446af 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -6,6 +6,7 @@
> #include
> #include
> #include
> +#include "bus.h"
>
> /**
> * sdw_get_device_id - find the matching SoundWire device id
> @@ -182,11 +183,13 @@ EXPORT_SYMBOL_GPL(sdw_unregister_driver);
>
> static int __init sdw_bus_init(void)
> {
> + sdw_debugfs_init();
> return bus_register(&sdw_bus_type);
> }
>
> static void __exit sdw_bus_exit(void)
> {
> + sdw_debugfs_exit();
> bus_unregister(&sdw_bus_type);
> }
>
> diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
> new file mode 100644
> index ..8a8b4df95c46
> --- /dev/null
> +++ b/drivers/soundwire/debugfs.c
> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2017-19 Intel Corporation.
> +
> +#include
>
[RFC PATCH 5/7] soundwire: add debugfs support
Add base debugfs mechanism for SoundWire bus by creating soundwire
root and master-N and slave-x hierarchy.
Also add SDW Slave SCP, DP0 and DP-N register debug file.
Registers not implemented will print as "XX"
Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
is the use of scnprintf to avoid known issues with snprintf.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/Makefile| 3 +-
drivers/soundwire/bus.c | 5 +
drivers/soundwire/bus.h | 28 +
drivers/soundwire/bus_type.c | 3 +
drivers/soundwire/debugfs.c | 214 ++
drivers/soundwire/slave.c | 1 +
include/linux/soundwire/sdw.h | 9 ++
7 files changed, 262 insertions(+), 1 deletion(-)
create mode 100644 drivers/soundwire/debugfs.c
diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index a72a29731a28..f2c425fa15bd 100644
--- a/drivers/soundwire/Makefile
+++ b/drivers/soundwire/Makefile
@@ -4,7 +4,8 @@
#Bus Objs
soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
- sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o
+ sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o \
+ debugfs.o
obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
#Cadence Objs
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index dd9181693554..b79eba321b71 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -53,6 +53,8 @@ int sdw_add_bus_master(struct sdw_bus *bus)
if (ret < 0)
dev_warn(bus->dev, "Bus sysfs init failed:%d\n", ret);
+ bus->debugfs = sdw_bus_debugfs_init(bus);
+
/*
* Device numbers in SoundWire are 0 through 15. Enumeration device
* number (0), Broadcast device number (15), Group numbers (12 and
@@ -114,6 +116,7 @@ static int sdw_delete_slave(struct device *dev, void *data)
struct sdw_bus *bus = slave->bus;
sdw_sysfs_slave_exit(slave);
+ sdw_slave_debugfs_exit(slave->debugfs);
mutex_lock(&bus->bus_lock);
@@ -136,6 +139,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
void sdw_delete_bus_master(struct sdw_bus *bus)
{
sdw_sysfs_bus_exit(bus);
+ if (bus->debugfs)
+ sdw_bus_debugfs_exit(bus->debugfs);
device_for_each_child(bus->dev, NULL, sdw_delete_slave);
}
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 0707e68a8d21..f0b1f4f9d7b2 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -20,6 +20,34 @@ void sdw_extract_slave_id(struct sdw_bus *bus,
extern const struct attribute_group *sdw_slave_dev_attr_groups[];
+#ifdef CONFIG_DEBUG_FS
+struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus);
+void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d);
+struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d);
+struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave);
+void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d);
+void sdw_debugfs_init(void);
+void sdw_debugfs_exit(void);
+#else
+struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus)
+{ return NULL; }
+
+void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d) {}
+
+struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
+{ return NULL; }
+
+struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave)
+{ return NULL; }
+
+void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d) {}
+
+void sdw_debugfs_init(void) {}
+
+void sdw_debugfs_exit(void) {}
+
+#endif
+
enum {
SDW_MSG_FLAG_READ = 0,
SDW_MSG_FLAG_WRITE,
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index f68fe45c1037..f28c1a2446af 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include "bus.h"
/**
* sdw_get_device_id - find the matching SoundWire device id
@@ -182,11 +183,13 @@ EXPORT_SYMBOL_GPL(sdw_unregister_driver);
static int __init sdw_bus_init(void)
{
+ sdw_debugfs_init();
return bus_register(&sdw_bus_type);
}
static void __exit sdw_bus_exit(void)
{
+ sdw_debugfs_exit();
bus_unregister(&sdw_bus_type);
}
diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
new file mode 100644
index ..8a8b4df95c46
--- /dev/null
+++ b/drivers/soundwire/debugfs.c
@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2017-19 Intel Corporation.
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include "bus.h"
+
+#ifdef CONFIG_DEBUG_FS
+struct dentry *sdw_debugfs_root;
+#endif
+
+struct sdw_bus_debugfs {
+ struct sdw_bus *bus;
+
+ struct dentry *fs;
+};
+
+struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus)
+{
+ struct sdw_bus_debugfs *d;
+ cha
