[PATCH 1/4] iwlwifi: mvm: Explicitly use %pd1 in debugfs entry

[PATCH 1/4] iwlwifi: mvm: Explicitly use %pd1 in debugfs entry

[Jia He]

'%pd'(no digit following) will mean to print last 4 components of file
dentry. Hence explicitly use %pd1 instead.

Signed-off-by: Jia He <justin.he@arm.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
index 38d0bfb649cc..41ccbb4286c2 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
@@ -752,7 +752,7 @@ void iwl_mvm_vif_dbgfs_register(struct iwl_mvm *mvm, struct ieee80211_vif *vif)
 	 * find
 	 * netdev:wlan0 -> ../../../ieee80211/phy0/netdev:wlan0/iwlmvm/
 	 */
-	snprintf(buf, 100, "../../../%pd3/%pd",
+	snprintf(buf, 100, "../../../%pd3/%pd1",
 		 dbgfs_dir,
 		 mvmvif->dbgfs_dir);
 
-- 
2.17.1

[PATCH 2/4] lib/vsprintf.c: Make %p{D,d} mean as much components as possible

[Jia He]

From: Linus Torvalds <torvalds@linux-foundation.org>

We have '%pD'(no digit following) for printing a filename. It may not be
perfect (by default it only prints one component.

%pD4 should be more than good enough, but we should make plain "%pD" mean
"as much of the path that is reasonable" rather than "as few components as
possible" (ie 1).

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jia He <justin.he@arm.com>
---
 Documentation/core-api/printk-formats.rst | 3 ++-
 lib/vsprintf.c                            | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 9be6de402cb9..aa76cbec0dae 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -413,7 +413,8 @@ dentry names
 For printing dentry name; if we race with :c:func:`d_move`, the name might
 be a mix of old and new ones, but it won't oops.  %pd dentry is a safer
 equivalent of %s dentry->d_name.name we used to use, %pd<n> prints ``n``
-last components.  %pD does the same thing for struct file.
+last components.  %pD does the same thing for struct file. By default, %p{D,d}
+is equal to %p{D,d}4.
 
 Passed by reference.
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 6c56c62fd9a5..5b563953f970 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -880,11 +880,11 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 	int i, n;
 
 	switch (fmt[1]) {
-		case '2': case '3': case '4':
+		case '1': case '2': case '3': case '4':
 			depth = fmt[1] - '0';
 			break;
 		default:
-			depth = 1;
+			depth = 4;
 	}
 
 	rcu_read_lock();
-- 
2.17.1

Re: [PATCH 2/4] lib/vsprintf.c: Make %p{D,d} mean as much components as possible

[Andy Shevchenko]

On Wed, Apr 28, 2021 at 5:56 PM Jia He <justin.he@arm.com> wrote:
>
> From: Linus Torvalds <torvalds@linux-foundation.org>

Hmm... Okay.

> We have '%pD'(no digit following) for printing a filename. It may not be
> perfect (by default it only prints one component.
>
> %pD4 should be more than good enough, but we should make plain "%pD" mean
> "as much of the path that is reasonable" rather than "as few components as
> possible" (ie 1).

Sorry, but from above I didn't get why.

The commit message tells only about %pD, but patch changes behaviour
of the ~100 or so users of "%pd" without any explanation.

Besides that the patch is prepended only by one change (which is also
not related to %pD), while we have ~30 users which behaviour got
changed.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] lib/vsprintf.c: Make %p{D,d} mean as much components as possible

[Petr Mladek]

On Wed 2021-04-28 21:59:27, Jia He wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> We have '%pD'(no digit following) for printing a filename. It may not be
> perfect (by default it only prints one component.
> 
> %pD4 should be more than good enough, but we should make plain "%pD" mean
> "as much of the path that is reasonable" rather than "as few components as
> possible" (ie 1).

Could you please provide link to the discussion where this idea was
came from?

It would be great to add and example into the commit message how
it improved the output.

Also please explain why it is useful/safe to change the behavior
for all existing users. It seems that you checked them and prevented
any regression by the other patches in this patchset.

Anyway, some regressions are fixed by the followup patches.
It would break bisection.

We either need to prevent the regression before this patch.
Or the changes have to be done in this patch. For example,
it would be perfectly fine to update test_printf.c in
this patch.

> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

If you want to keep Linus as the author and do more changes, you might
describe here changes done by you, for example:

[justin.he@arm.com: update documentation and test_printf]
Signed-off-by: Jia He <justin.he@arm.com>

Or you might make you the author and add

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>

> ---
>  Documentation/core-api/printk-formats.rst | 3 ++-
>  lib/vsprintf.c                            | 4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index 9be6de402cb9..aa76cbec0dae 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst

Plese, update also the pattern:

-	%pd{,2,3,4}
-	%pD{,2,3,4}
+	%pd{1,2,3,4}
+	%pD{1,2,3,4}

> @@ -413,7 +413,8 @@ dentry names
>  For printing dentry name; if we race with :c:func:`d_move`, the name might
>  be a mix of old and new ones, but it won't oops.  %pd dentry is a safer
>  equivalent of %s dentry->d_name.name we used to use, %pd<n> prints ``n``
> -last components.  %pD does the same thing for struct file.
> +last components.  %pD does the same thing for struct file. By default, %p{D,d}
> +is equal to %p{D,d}4.
>  
>  Passed by reference.

Best Regards,
Petr

Re: [PATCH 2/4] lib/vsprintf.c: Make %p{D,d} mean as much components as possible

[Andy Shevchenko]

On Thu, Apr 29, 2021 at 11:47 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Wed 2021-04-28 21:59:27, Jia He wrote:
> > From: Linus Torvalds <torvalds@linux-foundation.org>
> >
> > We have '%pD'(no digit following) for printing a filename. It may not be
> > perfect (by default it only prints one component.
> >
> > %pD4 should be more than good enough, but we should make plain "%pD" mean
> > "as much of the path that is reasonable" rather than "as few components as
> > possible" (ie 1).
>
> Could you please provide link to the discussion where this idea was
> came from?

https://lore.kernel.org/lkml/20210427025805.GD3122264@magnolia/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] lib/vsprintf.c: Make %p{D,d} mean as much components as possible

[Petr Mladek]

On Thu 2021-04-29 11:52:49, Andy Shevchenko wrote:
> On Thu, Apr 29, 2021 at 11:47 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Wed 2021-04-28 21:59:27, Jia He wrote:
> > > From: Linus Torvalds <torvalds@linux-foundation.org>
> > >
> > > We have '%pD'(no digit following) for printing a filename. It may not be
> > > perfect (by default it only prints one component.
> > >
> > > %pD4 should be more than good enough, but we should make plain "%pD" mean
> > > "as much of the path that is reasonable" rather than "as few components as
> > > possible" (ie 1).
> >
> > Could you please provide link to the discussion where this idea was
> > came from?
> 
> https://lore.kernel.org/lkml/20210427025805.GD3122264@magnolia/

Thanks for the link. I see that it was not clear whether the patch
was good for %pd behavior.

Linus actually suggests to keep %pd behavior as it was before, see
https://lore.kernel.org/lkml/CAHk-=wimsMqGdzik187YWLb-ru+iktb4MYbMQG1rnZ81dXYFVg@mail.gmail.com/

Well, I think that this is up to the file system developers to decide.
I am not sure if the path would do more harm than good,
or vice versa, for dentry names.

Best Regards,
Petr

RE: [PATCH 2/4] lib/vsprintf.c: Make %p{D,d} mean as much components as possible

[Justin He]

Hi

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Thursday, April 29, 2021 5:25 PM
> To: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Justin He <Justin.He@arm.com>; Linus Torvalds <torvalds@linux-
> foundation.org>; Steven Rostedt <rostedt@goodmis.org>; Sergey Senozhatsky
> <senozhatsky@chromium.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Rasmus Villemoes
> <linux@rasmusvillemoes.dk>; Jonathan Corbet <corbet@lwn.net>; Luca Coelho
> <luciano.coelho@intel.com>; Kalle Valo <kvalo@codeaurora.org>; David S.
> Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Johannes Berg
> <johannes.berg@intel.com>; Linux Documentation List <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; open list:TI WILINK WIRELES... <linux-
> wireless@vger.kernel.org>; netdev <netdev@vger.kernel.org>; linux-
> s390@vger.kernel.org
> Subject: Re: [PATCH 2/4] lib/vsprintf.c: Make %p{D,d} mean as much
> components as possible
>
> On Thu 2021-04-29 11:52:49, Andy Shevchenko wrote:
> > On Thu, Apr 29, 2021 at 11:47 AM Petr Mladek <pmladek@suse.com> wrote:
> > >
> > > On Wed 2021-04-28 21:59:27, Jia He wrote:
> > > > From: Linus Torvalds <torvalds@linux-foundation.org>
> > > >
> > > > We have '%pD'(no digit following) for printing a filename. It may not
> be
> > > > perfect (by default it only prints one component.
> > > >
> > > > %pD4 should be more than good enough, but we should make plain "%pD"
> mean
> > > > "as much of the path that is reasonable" rather than "as few
> components as
> > > > possible" (ie 1).
> > >
> > > Could you please provide link to the discussion where this idea was
> > > came from?
> >
> > https://lore.kernel.org/lkml/20210427025805.GD3122264@magnolia/
>
> Thanks for the link. I see that it was not clear whether the patch
> was good for %pd behavior.
>
> Linus actually suggests to keep %pd behavior as it was before, see
> https://lore.kernel.org/lkml/CAHk-=wimsMqGdzik187YWLb-
> ru+iktb4MYbMQG1rnZ81dXYFVg@mail.gmail.com/

Okay, let me keep the default %pd behavior as before('%pd' is '%pd1') and
change the behavior of %pD ('%pD' is '%pD4')

--
Cheers,
Justin (Jia He)
>
> Well, I think that this is up to the file system developers to decide.
> I am not sure if the path would do more harm than good,
> or vice versa, for dentry names.
>
> Best Regards,
> Petr
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH 3/4] s390/hmcdrv: Remove the redundant directory path in debug message

[Jia He]

It would be better to use full file path with '%pD' instead of hard coding.

Signed-off-by: Jia He <justin.he@arm.com>
---
 drivers/s390/char/hmcdrv_dev.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/char/hmcdrv_dev.c b/drivers/s390/char/hmcdrv_dev.c
index 20e9cd542e03..cdde75508c8a 100644
--- a/drivers/s390/char/hmcdrv_dev.c
+++ b/drivers/s390/char/hmcdrv_dev.c
@@ -137,7 +137,7 @@ static int hmcdrv_dev_open(struct inode *inode, struct file *fp)
 	if (rc)
 		module_put(THIS_MODULE);
 
-	pr_debug("open file '/dev/%pD' with return code %d\n", fp, rc);
+	pr_debug("open file '%pD' with return code %d\n", fp, rc);
 	return rc;
 }
 
@@ -146,7 +146,7 @@ static int hmcdrv_dev_open(struct inode *inode, struct file *fp)
  */
 static int hmcdrv_dev_release(struct inode *inode, struct file *fp)
 {
-	pr_debug("closing file '/dev/%pD'\n", fp);
+	pr_debug("closing file '%pD'\n", fp);
 	kfree(fp->private_data);
 	fp->private_data = NULL;
 	hmcdrv_ftp_shutdown();
@@ -231,7 +231,7 @@ static ssize_t hmcdrv_dev_read(struct file *fp, char __user *ubuf,
 	retlen = hmcdrv_dev_transfer((char *) fp->private_data,
 				     *pos, ubuf, len);
 
-	pr_debug("read from file '/dev/%pD' at %lld returns %zd/%zu\n",
+	pr_debug("read from file '%pD' at %lld returns %zd/%zu\n",
 		 fp, (long long) *pos, retlen, len);
 
 	if (retlen > 0)
@@ -248,7 +248,7 @@ static ssize_t hmcdrv_dev_write(struct file *fp, const char __user *ubuf,
 {
 	ssize_t retlen;
 
-	pr_debug("writing file '/dev/%pD' at pos. %lld with length %zd\n",
+	pr_debug("writing file '%pD' at pos. %lld with length %zd\n",
 		 fp, (long long) *pos, len);
 
 	if (!fp->private_data) { /* first expect a cmd write */
@@ -272,7 +272,7 @@ static ssize_t hmcdrv_dev_write(struct file *fp, const char __user *ubuf,
 	if (retlen > 0)
 		*pos += retlen;
 
-	pr_debug("write to file '/dev/%pD' returned %zd\n", fp, retlen);
+	pr_debug("write to file '%pD' returned %zd\n", fp, retlen);
 
 	return retlen;
 }
-- 
2.17.1

[PATCH 4/4] lib/test_printf: Explicitly add components number to %pD and %pd

[Jia He]

After changing the default components number from 1 to 4 for %pD
and %pd, it would be better to explicitly add the number in test_printf
cases.

Add a test case of %pd5 to verify if it can be capped by 4 components.

Signed-off-by: Jia He <justin.he@arm.com>
---
 lib/test_printf.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 27b964ec723d..899cd55d1c90 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -478,18 +478,20 @@ static struct dentry test_dentry[4] __initdata = {
 static void __init
 dentry(void)
 {
-	test("foo", "%pd", &test_dentry[0]);
+	test("foo", "%pd1", &test_dentry[0]);
 	test("foo", "%pd2", &test_dentry[0]);
 
-	test("(null)", "%pd", NULL);
-	test("(efault)", "%pd", PTR_INVALID);
-	test("(null)", "%pD", NULL);
-	test("(efault)", "%pD", PTR_INVALID);
+	test("(null)", "%pd1", NULL);
+	test("(efault)", "%pd1", PTR_INVALID);
+	test("(null)", "%pD1", NULL);
+	test("(efault)", "%pD1", PTR_INVALID);
 
-	test("romeo", "%pd", &test_dentry[3]);
+	test("romeo", "%pd1", &test_dentry[3]);
 	test("alfa/romeo", "%pd2", &test_dentry[3]);
 	test("bravo/alfa/romeo", "%pd3", &test_dentry[3]);
 	test("/bravo/alfa/romeo", "%pd4", &test_dentry[3]);
+	test("/bravo/alfa/romeo", "%pd", &test_dentry[3]);
+	test("/bravo/alfa/romeo", "%pd5", &test_dentry[3]);
 	test("/bravo/alfa", "%pd4", &test_dentry[2]);
 
 	test("bravo/alfa  |bravo/alfa  ", "%-12pd2|%*pd2", &test_dentry[2], -12, &test_dentry[2]);
-- 
2.17.1