minor debugfs-related issues Greg pointed out during 11/7/2016 review

[selltc]

KanBoard-27433

Greg's comment on 11/7 indicated: Minor review comments, you can fix these up in future patches.

The comments were in response to a patch David submitted on 11/3, which has since been committed to Greg's tree in commit 8217becc1b416e348d1bc8fcb54c09967e0f4987:

[PATCH 02/15] staging: unisys: visorbus: convert client_bus_info sysfs to debugfs

Greg's comments below.

diff --git a/drivers/staging/unisys/include/visorbus.h b/drivers/staging/unisys/include/visorbus.h
index 677627c..03d56f8 100644
--- a/drivers/staging/unisys/include/visorbus.h
+++ b/drivers/staging/unisys/include/visorbus.h
@@ -166,6 +166,8 @@ struct visor_device {
    struct controlvm_message_header *pending_msg_hdr;
    void *vbus_hdr_info;
    uuid_le partition_uuid;
+   struct dentry *debugfs_dir;
+   struct dentry *debugfs_client_bus_info;

No need to keep the dentry for the bus info, right?

@@ -151,6 +153,8 @@ struct bus_type visorbus_type = {
 {
    struct visor_device *dev = dev_get_drvdata(xdev);

+   debugfs_remove(dev->debugfs_client_bus_info);
+   debugfs_remove_recursive(dev->debugfs_dir);

Yup, removing the directory will remove the file, so no need to keep it around.

+   dev->debugfs_dir = debugfs_create_dir(dev_name(&dev->device),
+                         visorbus_debugfs_dir);
+   if (!dev->debugfs_dir) {
+       err = -ENOMEM;
+       goto err_hdr_info;
+   }

There's never any need to check the return value of a debugfs call, unless you really want to for some really odd reason. It shouldn't keep your main logic from doing anything different, as you should not be relying on it in any way.

So just drop the check here.

+   dev->debugfs_client_bus_info =
+       debugfs_create_file("client_bus_info", S_IRUSR | S_IRGRP,
+                   dev->debugfs_dir, dev,
+                   &client_bus_info_debugfs_fops);
+   if (!dev->debugfs_client_bus_info) {
+       err = -ENOMEM;
+       goto err_debugfs_dir;
+   }

Same here. Also, no need to keep the dentry at all.

+   visorbus_debugfs_dir = debugfs_create_dir("visorbus", NULL);
+   if (!visorbus_debugfs_dir)
+       return -ENOMEM;

And here, no need to check.

[michael-didomenico]

I was a little perplexed to find out that 95% of the debugfs_create_dir() and debugfs_create_file() calls in the kernel are doing the exact same error-checking I am. So I need to do some more homework to actually convince myself that what Greg says is even true.

[selltc]

DAG... I was accidentally logged in as Mike again. Above comment was mine.

[davidker]

Tim/MikeD :)

I would trust what Greg says even if 95% do it differently. He wrote it ;)

David

[davidker]

Can you make sure you have a Suggested-by: or Reported-by: with Greg's name on it for this patch?