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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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