diff mbox series

[v3,1/2] ACPI: use %pe for better readability of errors while printing

Message ID 20240213074416.2169929-1-onkarnath.1@samsung.com
State New
Headers show
Series [v3,1/2] ACPI: use %pe for better readability of errors while printing | expand

Commit Message

Onkarnath Feb. 13, 2024, 7:44 a.m. UTC 
From: Onkarnath <onkarnath.1@samsung.com>

As %pe is already introduced, it's better to use it in place of (%ld) for
printing errors in logs. It would enhance readability of logs.

Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
v1 -> v2: Updated subject line as per file history & corrected spellings
in description.
v2 -> v3: Updated Reviewed-by tag.

 drivers/acpi/acpi_processor.c | 2 +-
 drivers/acpi/acpi_watchdog.c  | 2 +-
 drivers/acpi/pci_slot.c       | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Comments

Bjorn Helgaas Feb. 13, 2024, 10:14 p.m. UTC | #1 
On Tue, Feb 13, 2024 at 01:14:15PM +0530, Onkarnarth wrote:
> From: Onkarnath <onkarnath.1@samsung.com>
> 
> As %pe is already introduced, it's better to use it in place of (%ld) for
> printing errors in logs. It would enhance readability of logs.

Here are some more candidates that I see regularly:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/dd.c?id=v6.7#n590

Something like:

  git grep "\(_info(\|_warn(\).*%d"

finds a ton of them (plus a lot of unrelated hits, of course).  If you
were to do this for drivers/pci/, I would want them all for the whole
directory in a single patch, and I would take the opportunity to make
minor changes so the style is more consistent, e.g.,
"... failed (%pe)" or something.

Bjorn
Rafael J. Wysocki Feb. 15, 2024, 7:56 p.m. UTC | #2 
On Tue, Feb 13, 2024 at 9:20 AM Onkarnarth <onkarnath.1@samsung.com> wrote:
>
> From: Onkarnath <onkarnath.1@samsung.com>
>
> As %pe is already introduced, it's better to use it in place of (%ld) for
> printing errors in logs. It would enhance readability of logs.
>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>

What exactly is the role of this S-o-b?  Has the person helped you to
develop the patch or something else?

> Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
> Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
> v1 -> v2: Updated subject line as per file history & corrected spellings
> in description.
> v2 -> v3: Updated Reviewed-by tag.
>
>  drivers/acpi/acpi_processor.c | 2 +-
>  drivers/acpi/acpi_watchdog.c  | 2 +-
>  drivers/acpi/pci_slot.c       | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 4fe2ef54088c..2ddd36a21850 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -161,7 +161,7 @@ static void cpufreq_add_device(const char *name)
>
>         pdev = platform_device_register_simple(name, PLATFORM_DEVID_NONE, NULL, 0);
>         if (IS_ERR(pdev))
> -               pr_info("%s device creation failed: %ld\n", name, PTR_ERR(pdev));
> +               pr_info("%s device creation failed: %pe\n", name, pdev);
>  }
>
>  #ifdef CONFIG_X86
> diff --git a/drivers/acpi/acpi_watchdog.c b/drivers/acpi/acpi_watchdog.c
> index 8e9e001da38f..14b24157799c 100644
> --- a/drivers/acpi/acpi_watchdog.c
> +++ b/drivers/acpi/acpi_watchdog.c
> @@ -179,7 +179,7 @@ void __init acpi_watchdog_init(void)
>         pdev = platform_device_register_simple("wdat_wdt", PLATFORM_DEVID_NONE,
>                                                resources, nresources);
>         if (IS_ERR(pdev))
> -               pr_err("Device creation failed: %ld\n", PTR_ERR(pdev));
> +               pr_err("Device creation failed: %pe\n", pdev);
>
>         kfree(resources);
>
> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> index d6cb2c27a23b..741bcc9d6d6a 100644
> --- a/drivers/acpi/pci_slot.c
> +++ b/drivers/acpi/pci_slot.c
> @@ -111,7 +111,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>         snprintf(name, sizeof(name), "%llu", sun);
>         pci_slot = pci_create_slot(pci_bus, device, name, NULL);
>         if (IS_ERR(pci_slot)) {
> -               pr_err("pci_create_slot returned %ld\n", PTR_ERR(pci_slot));
> +               pr_err("pci_create_slot returned %pe\n", pci_slot);
>                 kfree(slot);
>                 return AE_OK;
>         }
> --
Onkarnath Feb. 16, 2024, 5:45 a.m. UTC | #3 
>On Tue, Feb 13, 2024 at 9:20 AM Onkarnarth <onkarnath.1@samsung.com> wrote:
>>
>> From: Onkarnath <onkarnath.1@samsung.com>
>>
>> As %pe is already introduced, it's better to use it in place of (%ld) for
>> printing errors in logs. It would enhance readability of logs.
>>
>> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
>
>What exactly is the role of this S-o-b?  Has the person helped you to
>develop the patch or something else?
>

Yes It was meant for Co-developed tag, Because We are working collectively for making errors more readable for our product kernel.(5.4)
And some part of this patch was made by him.

Then we checked that it is also suggested by open source to have %pe for printing errors:
https://lore.kernel.org/all/92972476-0b1f-4d0a-9951-af3fc8bc6e65@suswa.mountain/

So I prepared same changes for open source kernel, and because of smaller patch I kept it as normal signed-off tag only.
If it is needed I can resend with Co-developed tag.

Thanks,
Onkarnath
Rafael J. Wysocki Feb. 16, 2024, 6:29 p.m. UTC | #4 
On Fri, Feb 16, 2024 at 6:54 AM Onkarnath <onkarnath.1@samsung.com> wrote:
>
> >On Tue, Feb 13, 2024 at 9:20 AM Onkarnarth <onkarnath.1@samsung.com> wrote:
> >>
> >> From: Onkarnath <onkarnath.1@samsung.com>
> >>
> >> As %pe is already introduced, it's better to use it in place of (%ld) for
> >> printing errors in logs. It would enhance readability of logs.
> >>
> >> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> >
> >What exactly is the role of this S-o-b?  Has the person helped you to
> >develop the patch or something else?
> >
>
> Yes It was meant for Co-developed tag, Because We are working collectively for making errors more readable for our product kernel.(5.4)
> And some part of this patch was made by him.
>
> Then we checked that it is also suggested by open source to have %pe for printing errors:
> https://lore.kernel.org/all/92972476-0b1f-4d0a-9951-af3fc8bc6e65@suswa.mountain/
>
> So I prepared same changes for open source kernel, and because of smaller patch I kept it as normal signed-off tag only.
> If it is needed I can resend with Co-developed tag.

No need, I can add it for you.  Thanks for the explanation!
Rafael J. Wysocki Feb. 16, 2024, 6:32 p.m. UTC | #5 
On Tue, Feb 13, 2024 at 9:20 AM Onkarnarth <onkarnath.1@samsung.com> wrote:
>
> From: Onkarnath <onkarnath.1@samsung.com>
>
> As %pe is already introduced, it's better to use it in place of (%ld) for
> printing errors in logs. It would enhance readability of logs.
>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
> Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
> v1 -> v2: Updated subject line as per file history & corrected spellings
> in description.
> v2 -> v3: Updated Reviewed-by tag.
>
>  drivers/acpi/acpi_processor.c | 2 +-
>  drivers/acpi/acpi_watchdog.c  | 2 +-
>  drivers/acpi/pci_slot.c       | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 4fe2ef54088c..2ddd36a21850 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -161,7 +161,7 @@ static void cpufreq_add_device(const char *name)
>
>         pdev = platform_device_register_simple(name, PLATFORM_DEVID_NONE, NULL, 0);
>         if (IS_ERR(pdev))
> -               pr_info("%s device creation failed: %ld\n", name, PTR_ERR(pdev));
> +               pr_info("%s device creation failed: %pe\n", name, pdev);
>  }
>
>  #ifdef CONFIG_X86
> diff --git a/drivers/acpi/acpi_watchdog.c b/drivers/acpi/acpi_watchdog.c
> index 8e9e001da38f..14b24157799c 100644
> --- a/drivers/acpi/acpi_watchdog.c
> +++ b/drivers/acpi/acpi_watchdog.c
> @@ -179,7 +179,7 @@ void __init acpi_watchdog_init(void)
>         pdev = platform_device_register_simple("wdat_wdt", PLATFORM_DEVID_NONE,
>                                                resources, nresources);
>         if (IS_ERR(pdev))
> -               pr_err("Device creation failed: %ld\n", PTR_ERR(pdev));
> +               pr_err("Device creation failed: %pe\n", pdev);
>
>         kfree(resources);
>
> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> index d6cb2c27a23b..741bcc9d6d6a 100644
> --- a/drivers/acpi/pci_slot.c
> +++ b/drivers/acpi/pci_slot.c
> @@ -111,7 +111,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>         snprintf(name, sizeof(name), "%llu", sun);
>         pci_slot = pci_create_slot(pci_bus, device, name, NULL);
>         if (IS_ERR(pci_slot)) {
> -               pr_err("pci_create_slot returned %ld\n", PTR_ERR(pci_slot));
> +               pr_err("pci_create_slot returned %pe\n", pci_slot);
>                 kfree(slot);
>                 return AE_OK;
>         }
> --

Applied as 6.9 material, thanks!
diff mbox series

Patch

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 4fe2ef54088c..2ddd36a21850 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -161,7 +161,7 @@  static void cpufreq_add_device(const char *name)
 
 	pdev = platform_device_register_simple(name, PLATFORM_DEVID_NONE, NULL, 0);
 	if (IS_ERR(pdev))
-		pr_info("%s device creation failed: %ld\n", name, PTR_ERR(pdev));
+		pr_info("%s device creation failed: %pe\n", name, pdev);
 }
 
 #ifdef CONFIG_X86
diff --git a/drivers/acpi/acpi_watchdog.c b/drivers/acpi/acpi_watchdog.c
index 8e9e001da38f..14b24157799c 100644
--- a/drivers/acpi/acpi_watchdog.c
+++ b/drivers/acpi/acpi_watchdog.c
@@ -179,7 +179,7 @@  void __init acpi_watchdog_init(void)
 	pdev = platform_device_register_simple("wdat_wdt", PLATFORM_DEVID_NONE,
 					       resources, nresources);
 	if (IS_ERR(pdev))
-		pr_err("Device creation failed: %ld\n", PTR_ERR(pdev));
+		pr_err("Device creation failed: %pe\n", pdev);
 
 	kfree(resources);
 
diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
index d6cb2c27a23b..741bcc9d6d6a 100644
--- a/drivers/acpi/pci_slot.c
+++ b/drivers/acpi/pci_slot.c
@@ -111,7 +111,7 @@  register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	snprintf(name, sizeof(name), "%llu", sun);
 	pci_slot = pci_create_slot(pci_bus, device, name, NULL);
 	if (IS_ERR(pci_slot)) {
-		pr_err("pci_create_slot returned %ld\n", PTR_ERR(pci_slot));
+		pr_err("pci_create_slot returned %pe\n", pci_slot);
 		kfree(slot);
 		return AE_OK;
 	}