* [PATCH] atm: ambassador: remove h from printk format specifier
@ 2020-12-15 14:22 trix
2020-12-17 0:45 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: trix @ 2020-12-15 14:22 UTC (permalink / raw)
To: 3chas3; +Cc: linux-atm-general, netdev, linux-kernel, Tom Rix
From: Tom Rix <trix@redhat.com>
See Documentation/core-api/printk-formats.rst.
h should no longer be used in the format specifier for printk.
Signed-off-by: Tom Rix <trix@redhat.com>
---
drivers/atm/ambassador.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/atm/ambassador.c b/drivers/atm/ambassador.c
index c039b8a4fefe..6b0fff8c0141 100644
--- a/drivers/atm/ambassador.c
+++ b/drivers/atm/ambassador.c
@@ -2169,7 +2169,7 @@ static void setup_pci_dev(struct pci_dev *pci_dev)
pci_lat = (lat < MIN_PCI_LATENCY) ? MIN_PCI_LATENCY : lat;
if (lat != pci_lat) {
- PRINTK (KERN_INFO, "Changing PCI latency timer from %hu to %hu",
+ PRINTK (KERN_INFO, "Changing PCI latency timer from %u to %u",
lat, pci_lat);
pci_write_config_byte(pci_dev, PCI_LATENCY_TIMER, pci_lat);
}
@@ -2300,7 +2300,7 @@ static void __init amb_check_args (void) {
unsigned int max_rx_size;
#ifdef DEBUG_AMBASSADOR
- PRINTK (KERN_NOTICE, "debug bitmap is %hx", debug &= DBG_MASK);
+ PRINTK (KERN_NOTICE, "debug bitmap is %x", debug &= DBG_MASK);
#else
if (debug)
PRINTK (KERN_NOTICE, "no debugging support");
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] atm: ambassador: remove h from printk format specifier
2020-12-15 14:22 [PATCH] atm: ambassador: remove h from printk format specifier trix
@ 2020-12-17 0:45 ` Jakub Kicinski
2020-12-17 13:17 ` Tom Rix
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2020-12-17 0:45 UTC (permalink / raw)
To: trix; +Cc: 3chas3, linux-atm-general, netdev, linux-kernel
On Tue, 15 Dec 2020 06:22:28 -0800 trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
>
> See Documentation/core-api/printk-formats.rst.
> h should no longer be used in the format specifier for printk.
>
> Signed-off-by: Tom Rix <trix@redhat.com>
That's for new code I assume?
What's the harm in leaving this ancient code be?
> diff --git a/drivers/atm/ambassador.c b/drivers/atm/ambassador.c
> index c039b8a4fefe..6b0fff8c0141 100644
> --- a/drivers/atm/ambassador.c
> +++ b/drivers/atm/ambassador.c
> @@ -2169,7 +2169,7 @@ static void setup_pci_dev(struct pci_dev *pci_dev)
> pci_lat = (lat < MIN_PCI_LATENCY) ? MIN_PCI_LATENCY : lat;
>
> if (lat != pci_lat) {
> - PRINTK (KERN_INFO, "Changing PCI latency timer from %hu to %hu",
> + PRINTK (KERN_INFO, "Changing PCI latency timer from %u to %u",
> lat, pci_lat);
> pci_write_config_byte(pci_dev, PCI_LATENCY_TIMER, pci_lat);
> }
> @@ -2300,7 +2300,7 @@ static void __init amb_check_args (void) {
> unsigned int max_rx_size;
>
> #ifdef DEBUG_AMBASSADOR
> - PRINTK (KERN_NOTICE, "debug bitmap is %hx", debug &= DBG_MASK);
> + PRINTK (KERN_NOTICE, "debug bitmap is %x", debug &= DBG_MASK);
> #else
> if (debug)
> PRINTK (KERN_NOTICE, "no debugging support");
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] atm: ambassador: remove h from printk format specifier
2020-12-17 0:45 ` Jakub Kicinski
@ 2020-12-17 13:17 ` Tom Rix
2020-12-17 17:28 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Tom Rix @ 2020-12-17 13:17 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: 3chas3, linux-atm-general, netdev, linux-kernel
On 12/16/20 4:45 PM, Jakub Kicinski wrote:
> On Tue, 15 Dec 2020 06:22:28 -0800 trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> See Documentation/core-api/printk-formats.rst.
>> h should no longer be used in the format specifier for printk.
>>
>> Signed-off-by: Tom Rix <trix@redhat.com>
> That's for new code I assume?
>
> What's the harm in leaving this ancient code be?
This change is part of a tree wide cleanup.
drivers/atm status is listed as Maintained in MAINTAINERS so changes like this should be ok.
Should drivers/atm status be changed?
Tom
>
>> diff --git a/drivers/atm/ambassador.c b/drivers/atm/ambassador.c
>> index c039b8a4fefe..6b0fff8c0141 100644
>> --- a/drivers/atm/ambassador.c
>> +++ b/drivers/atm/ambassador.c
>> @@ -2169,7 +2169,7 @@ static void setup_pci_dev(struct pci_dev *pci_dev)
>> pci_lat = (lat < MIN_PCI_LATENCY) ? MIN_PCI_LATENCY : lat;
>>
>> if (lat != pci_lat) {
>> - PRINTK (KERN_INFO, "Changing PCI latency timer from %hu to %hu",
>> + PRINTK (KERN_INFO, "Changing PCI latency timer from %u to %u",
>> lat, pci_lat);
>> pci_write_config_byte(pci_dev, PCI_LATENCY_TIMER, pci_lat);
>> }
>> @@ -2300,7 +2300,7 @@ static void __init amb_check_args (void) {
>> unsigned int max_rx_size;
>>
>> #ifdef DEBUG_AMBASSADOR
>> - PRINTK (KERN_NOTICE, "debug bitmap is %hx", debug &= DBG_MASK);
>> + PRINTK (KERN_NOTICE, "debug bitmap is %x", debug &= DBG_MASK);
>> #else
>> if (debug)
>> PRINTK (KERN_NOTICE, "no debugging support");
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] atm: ambassador: remove h from printk format specifier
2020-12-17 13:17 ` Tom Rix
@ 2020-12-17 17:28 ` Jakub Kicinski
2020-12-17 22:03 ` Tom Rix
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2020-12-17 17:28 UTC (permalink / raw)
To: Tom Rix; +Cc: 3chas3, linux-atm-general, netdev, linux-kernel
On Thu, 17 Dec 2020 05:17:24 -0800 Tom Rix wrote:
> On 12/16/20 4:45 PM, Jakub Kicinski wrote:
> > On Tue, 15 Dec 2020 06:22:28 -0800 trix@redhat.com wrote:
> >> From: Tom Rix <trix@redhat.com>
> >>
> >> See Documentation/core-api/printk-formats.rst.
> >> h should no longer be used in the format specifier for printk.
> >>
> >> Signed-off-by: Tom Rix <trix@redhat.com>
> > That's for new code I assume?
> >
> > What's the harm in leaving this ancient code be?
>
> This change is part of a tree wide cleanup.
What's the purpose of the "clean up"? Why is it making the code better?
This is a quote from your change:
- PRINTK (KERN_NOTICE, "debug bitmap is %hx", debug &= DBG_MASK);
+ PRINTK (KERN_NOTICE, "debug bitmap is %x", debug &= DBG_MASK);
Are you sure that the use of %hx is the worst part of that line?
> drivers/atm status is listed as Maintained in MAINTAINERS so changes
> like this should be ok.
>
> Should drivers/atm status be changed?
Up to Chas, but AFAIU we're probably only a few years away from ATM as
a whole walking into the light. So IMHO "Obsolete" would be justified.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] atm: ambassador: remove h from printk format specifier
2020-12-17 17:28 ` Jakub Kicinski
@ 2020-12-17 22:03 ` Tom Rix
2020-12-18 18:02 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Tom Rix @ 2020-12-17 22:03 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: 3chas3, linux-atm-general, netdev, linux-kernel
On 12/17/20 9:28 AM, Jakub Kicinski wrote:
> On Thu, 17 Dec 2020 05:17:24 -0800 Tom Rix wrote:
>> On 12/16/20 4:45 PM, Jakub Kicinski wrote:
>>> On Tue, 15 Dec 2020 06:22:28 -0800 trix@redhat.com wrote:
>>>> From: Tom Rix <trix@redhat.com>
>>>>
>>>> See Documentation/core-api/printk-formats.rst.
>>>> h should no longer be used in the format specifier for printk.
>>>>
>>>> Signed-off-by: Tom Rix <trix@redhat.com>
>>> That's for new code I assume?
>>>
>>> What's the harm in leaving this ancient code be?
>> This change is part of a tree wide cleanup.
> What's the purpose of the "clean up"? Why is it making the code better?
>
> This is a quote from your change:
>
> - PRINTK (KERN_NOTICE, "debug bitmap is %hx", debug &= DBG_MASK);
> + PRINTK (KERN_NOTICE, "debug bitmap is %x", debug &= DBG_MASK);
>
> Are you sure that the use of %hx is the worst part of that line?
In this case, it means this bit of code is compliant with the %h checker in checkpatch.
why you are seeing this change for %hx and not the horrible debug &= or the old PRINTK macro is because the change was mechanical.
leveraging the clang build and a special fixit for %h, an allyesconfig for x86_64 cleans this problem from most of the tree in about an hour. atm/ was just one of the places it hit, there are about 100 more.
If you want the debug &= fixed, i can do that.
The macro is a treewide problem and i can add that to the treewide cleanups i am planning.
Tom
>
>> drivers/atm status is listed as Maintained in MAINTAINERS so changes
>> like this should be ok.
>>
>> Should drivers/atm status be changed?
> Up to Chas, but AFAIU we're probably only a few years away from ATM as
> a whole walking into the light. So IMHO "Obsolete" would be justified.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] atm: ambassador: remove h from printk format specifier
2020-12-17 22:03 ` Tom Rix
@ 2020-12-18 18:02 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2020-12-18 18:02 UTC (permalink / raw)
To: Tom Rix; +Cc: 3chas3, linux-atm-general, netdev, linux-kernel
On Thu, 17 Dec 2020 14:03:07 -0800 Tom Rix wrote:
> On 12/17/20 9:28 AM, Jakub Kicinski wrote:
> > On Thu, 17 Dec 2020 05:17:24 -0800 Tom Rix wrote:
> >> On 12/16/20 4:45 PM, Jakub Kicinski wrote:
> >>> On Tue, 15 Dec 2020 06:22:28 -0800 trix@redhat.com wrote:
> >>>> From: Tom Rix <trix@redhat.com>
> >>>>
> >>>> See Documentation/core-api/printk-formats.rst.
> >>>> h should no longer be used in the format specifier for printk.
> >>>>
> >>>> Signed-off-by: Tom Rix <trix@redhat.com>
> >>> That's for new code I assume?
> >>>
> >>> What's the harm in leaving this ancient code be?
> >> This change is part of a tree wide cleanup.
> > What's the purpose of the "clean up"? Why is it making the code better?
> >
> > This is a quote from your change:
> >
> > - PRINTK (KERN_NOTICE, "debug bitmap is %hx", debug &= DBG_MASK);
> > + PRINTK (KERN_NOTICE, "debug bitmap is %x", debug &= DBG_MASK);
> >
> > Are you sure that the use of %hx is the worst part of that line?
>
> In this case, it means this bit of code is compliant with the %h checker in checkpatch.
>
> why you are seeing this change for %hx and not the horrible debug &= or the old PRINTK macro is because the change was mechanical.
>
> leveraging the clang build and a special fixit for %h, an allyesconfig for x86_64 cleans this problem from most of the tree in about an hour. atm/ was just one of the places it hit, there are about 100 more.
>
> If you want the debug &= fixed, i can do that.
No, the opposite of that. I would like to see fewer patches touching
prehistoric code for little to no gain :(
> The macro is a treewide problem and i can add that to the treewide cleanups i am planning.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-18 18:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-15 14:22 [PATCH] atm: ambassador: remove h from printk format specifier trix
2020-12-17 0:45 ` Jakub Kicinski
2020-12-17 13:17 ` Tom Rix
2020-12-17 17:28 ` Jakub Kicinski
2020-12-17 22:03 ` Tom Rix
2020-12-18 18:02 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).