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

Pierre-Louis Bossart
2019-05-06T14:52:02Z

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