On Wed, Feb 10, 2021 at 04:36:00AM -0800, Joe Perches wrote:

On Wed, 2021-02-10 at 12:18 +0200, Andy Shevchenko wrote:

quoted

On Wed, Feb 10, 2021 at 10:30 AM Joe Perches [off-list ref] wrote:

quoted

On Tue, 2021-02-09 at 23:49 -0800, Drew Fustini wrote:

quoted

quoted

-     debugfs_create_file("pinctrl-devices", S_IFREG | S_IRUGO,
+     debugfs_create_file("pinctrl-devices", 0400,
                          debugfs_root, NULL, &pinctrl_devices_fops);

NAK.  You've changed the permission levels.

NAK is usually given when the whole idea is broken. Here is not the
case and you may have helped to amend the patch.

NAK IMO just means the patch should not be applied, not that the
concept is broken.

quoted

...

quoted

And you have to keep the S_IFREG or'd along with the octal.

Perhaps time to read the code?
https://elixir.bootlin.com/linux/latest/source/fs/debugfs/inode.c#L387

Then the commit message is also broken.

quoted

quoted

checkpatch does this conversion using this command line:

$ ./scripts/checkpatch.pl -f --show-types --terse drivers/pinctrl/*.[ch] --types=SYMBOLIC_PERMS --fix-inplace

NAK! See above.

The command line above is for octal conversion of the symbolic permissions.

Any other conversion would be for a different purpose and that purpose and
should be described in the commit message.

Thanks for review comments from all.

I will change from the incorrect 0400 to 0444.

As for S_IFREG, it does seem like leaving off S_IFREG is the most common
case when using octal permissions with debugfs_create_*():

$ git grep debugfs_create drivers/ |grep 0444 |grep -v S_IFREG | wc -l
302
$ git grep debugfs_create drivers/ |grep 0444 |grep S_IFREG | wc -l
9

As noted by Andy, this is okay as the S_IFREG flag is added to the mode
__debugfs_create_file() inside fs/debugfs/inode.c. I will note this in
the commit message.

Thank you,
Drew