Re: [alsa-devel] [RFC PATCH 5/7] soundwire: add debugfs support

[Pierre-Louis Bossart]

> > @@ -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.