* [PATCH] x86: squash lines for simple wrapper functions
@ 2016-09-06 11:24 Masahiro Yamada
2016-09-08 6:33 ` Ingo Molnar
0 siblings, 1 reply; 7+ messages in thread
From: Masahiro Yamada @ 2016-09-06 11:24 UTC (permalink / raw)
To: x86, Ingo Molnar
Cc: Masahiro Yamada, Toshi Kani, Denys Vlasenko, Borislav Petkov,
Paul Gortmaker, H. Peter Anvin, Nathan Zimmer, Thomas Gleixner,
linux-kernel, Mike Travis, Daniel J Blueman, Dimitri Sivanich,
Matt Fleming, Hedi Berriche, Steffen Persvold, Alex Thorlton,
Wei Jiangang
Remove unneeded variables and assignments. I am also removing
unnecessary parentheses while I am here.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
arch/x86/kernel/apic/apic_flat_64.c | 16 +++-------------
arch/x86/kernel/apic/apic_numachip.c | 5 +----
arch/x86/kernel/apic/x2apic_uv_x.c | 5 +----
arch/x86/mm/pat_rbtree.c | 4 +---
arch/x86/platform/uv/bios_uv.c | 7 ++-----
arch/x86/platform/uv/tlb_uv.c | 6 +-----
6 files changed, 9 insertions(+), 34 deletions(-)
diff --git a/arch/x86/kernel/apic/apic_flat_64.c b/arch/x86/kernel/apic/apic_flat_64.c
index 5b2ae10..c7228f9 100644
--- a/arch/x86/kernel/apic/apic_flat_64.c
+++ b/arch/x86/kernel/apic/apic_flat_64.c
@@ -116,27 +116,17 @@ static void flat_send_IPI_all(int vector)
static unsigned int flat_get_apic_id(unsigned long x)
{
- unsigned int id;
-
- id = (((x)>>24) & 0xFFu);
-
- return id;
+ return ((x) >> 24) & 0xFFu;
}
static unsigned long set_apic_id(unsigned int id)
{
- unsigned long x;
-
- x = ((id & 0xFFu)<<24);
- return x;
+ return (id & 0xFFu) << 24;
}
static unsigned int read_xapic_id(void)
{
- unsigned int id;
-
- id = flat_get_apic_id(apic_read(APIC_ID));
- return id;
+ return flat_get_apic_id(apic_read(APIC_ID));
}
static int flat_apic_id_registered(void)
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index 714d4fd..fce51bf 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -40,10 +40,7 @@ static unsigned int numachip1_get_apic_id(unsigned long x)
static unsigned long numachip1_set_apic_id(unsigned int id)
{
- unsigned long x;
-
- x = ((id & 0xffU) << 24);
- return x;
+ return (id & 0xffU) << 24;
}
static unsigned int numachip2_get_apic_id(unsigned long x)
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index cb0673c..6a4d4b9 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -533,11 +533,8 @@ static unsigned int x2apic_get_apic_id(unsigned long x)
static unsigned long set_apic_id(unsigned int id)
{
- unsigned long x;
-
/* maskout x2apic_extra_bits ? */
- x = id;
- return x;
+ return id;
}
static unsigned int uv_read_apic_id(void)
diff --git a/arch/x86/mm/pat_rbtree.c b/arch/x86/mm/pat_rbtree.c
index de391b7..159b52c 100644
--- a/arch/x86/mm/pat_rbtree.c
+++ b/arch/x86/mm/pat_rbtree.c
@@ -254,9 +254,7 @@ struct memtype *rbt_memtype_erase(u64 start, u64 end)
struct memtype *rbt_memtype_lookup(u64 addr)
{
- struct memtype *data;
- data = memtype_rb_lowest_match(&memtype_rbroot, addr, addr + PAGE_SIZE);
- return data;
+ return memtype_rb_lowest_match(&memtype_rbroot, addr, addr + PAGE_SIZE);
}
#if defined(CONFIG_DEBUG_FS)
diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
index 23f2f3e..b4d5e95 100644
--- a/arch/x86/platform/uv/bios_uv.c
+++ b/arch/x86/platform/uv/bios_uv.c
@@ -149,11 +149,8 @@ EXPORT_SYMBOL_GPL(uv_bios_change_memprotect);
s64
uv_bios_reserved_page_pa(u64 buf, u64 *cookie, u64 *addr, u64 *len)
{
- s64 ret;
-
- ret = uv_bios_call_irqsave(UV_BIOS_GET_PARTITION_ADDR, (u64)cookie,
- (u64)addr, buf, (u64)len, 0);
- return ret;
+ return uv_bios_call_irqsave(UV_BIOS_GET_PARTITION_ADDR, (u64)cookie,
+ (u64)addr, buf, (u64)len, 0);
}
EXPORT_SYMBOL_GPL(uv_bios_reserved_page_pa);
diff --git a/arch/x86/platform/uv/tlb_uv.c b/arch/x86/platform/uv/tlb_uv.c
index fdb4d42..276e1b7 100644
--- a/arch/x86/platform/uv/tlb_uv.c
+++ b/arch/x86/platform/uv/tlb_uv.c
@@ -580,11 +580,7 @@ static int uv1_wait_completion(struct bau_desc *bau_desc,
*/
static unsigned long uv2_3_read_status(unsigned long offset, int rshft, int desc)
{
- unsigned long descriptor_status;
-
- descriptor_status =
- ((read_lmmr(offset) >> rshft) & UV_ACT_STATUS_MASK) << 1;
- return descriptor_status;
+ return ((read_lmmr(offset) >> rshft) & UV_ACT_STATUS_MASK) << 1;
}
/*
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: squash lines for simple wrapper functions
2016-09-06 11:24 [PATCH] x86: squash lines for simple wrapper functions Masahiro Yamada
@ 2016-09-08 6:33 ` Ingo Molnar
2016-09-09 14:40 ` Thomas Gleixner
2016-09-10 9:30 ` Masahiro Yamada
0 siblings, 2 replies; 7+ messages in thread
From: Ingo Molnar @ 2016-09-08 6:33 UTC (permalink / raw)
To: Masahiro Yamada
Cc: x86, Ingo Molnar, Toshi Kani, Denys Vlasenko, Borislav Petkov,
Paul Gortmaker, H. Peter Anvin, Nathan Zimmer, Thomas Gleixner,
linux-kernel, Mike Travis, Daniel J Blueman, Dimitri Sivanich,
Matt Fleming, Hedi Berriche, Steffen Persvold, Alex Thorlton,
Wei Jiangang
* Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Remove unneeded variables and assignments. I am also removing
> unnecessary parentheses while I am here.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> arch/x86/kernel/apic/apic_flat_64.c | 16 +++-------------
> arch/x86/kernel/apic/apic_numachip.c | 5 +----
> arch/x86/kernel/apic/x2apic_uv_x.c | 5 +----
> arch/x86/mm/pat_rbtree.c | 4 +---
> arch/x86/platform/uv/bios_uv.c | 7 ++-----
> arch/x86/platform/uv/tlb_uv.c | 6 +-----
> 6 files changed, 9 insertions(+), 34 deletions(-)
>
> diff --git a/arch/x86/kernel/apic/apic_flat_64.c b/arch/x86/kernel/apic/apic_flat_64.c
> index 5b2ae10..c7228f9 100644
> --- a/arch/x86/kernel/apic/apic_flat_64.c
> +++ b/arch/x86/kernel/apic/apic_flat_64.c
> @@ -116,27 +116,17 @@ static void flat_send_IPI_all(int vector)
>
> static unsigned int flat_get_apic_id(unsigned long x)
> {
> - unsigned int id;
> -
> - id = (((x)>>24) & 0xFFu);
> -
> - return id;
> + return ((x) >> 24) & 0xFFu;
So while we are removing unnecessary things, exactly why does the 'x' need
parentheses?
> static unsigned long set_apic_id(unsigned int id)
> {
> - unsigned long x;
> -
> - x = ((id & 0xFFu)<<24);
> - return x;
> + return (id & 0xFFu) << 24;
'id' is already unsigned, why does the 'u' have to be stressed in the literal?
(Ditto for other places as well)
> static unsigned long numachip1_set_apic_id(unsigned int id)
> {
> - unsigned long x;
> -
> - x = ((id & 0xffU) << 24);
> - return x;
> + return (id & 0xffU) << 24;
> }
Why is the spelling of the literal inconsistent here with the other patterns?
> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
> @@ -533,11 +533,8 @@ static unsigned int x2apic_get_apic_id(unsigned long x)
>
> static unsigned long set_apic_id(unsigned int id)
> {
> - unsigned long x;
> -
> /* maskout x2apic_extra_bits ? */
> - x = id;
> - return x;
> + return id;
> }
This was clearly left there to document a quirk and as a placeholder for future
changes.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: squash lines for simple wrapper functions
2016-09-08 6:33 ` Ingo Molnar
@ 2016-09-09 14:40 ` Thomas Gleixner
2016-09-10 4:35 ` Ingo Molnar
2016-09-10 9:30 ` Masahiro Yamada
1 sibling, 1 reply; 7+ messages in thread
From: Thomas Gleixner @ 2016-09-09 14:40 UTC (permalink / raw)
To: Ingo Molnar
Cc: Masahiro Yamada, x86, Ingo Molnar, Toshi Kani, Denys Vlasenko,
Borislav Petkov, Paul Gortmaker, H. Peter Anvin, Nathan Zimmer,
linux-kernel, Mike Travis, Daniel J Blueman, Dimitri Sivanich,
Matt Fleming, Hedi Berriche, Steffen Persvold, Alex Thorlton,
Wei Jiangang
On Thu, 8 Sep 2016, Ingo Molnar wrote:
> * Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> > static unsigned long set_apic_id(unsigned int id)
> > {
> > - unsigned long x;
> > -
> > /* maskout x2apic_extra_bits ? */
> > - x = id;
> > - return x;
> > + return id;
> > }
>
> This was clearly left there to document a quirk and as a placeholder for future
> changes.
Keeping the comment and rewording it to:
/* CHECKME: Do we need to mask out the xapic extra bits */
should be good enough. The variable dance is not really giving any value.
Thanks,
tglx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: squash lines for simple wrapper functions
2016-09-09 14:40 ` Thomas Gleixner
@ 2016-09-10 4:35 ` Ingo Molnar
2016-09-10 5:53 ` Thomas Gleixner
0 siblings, 1 reply; 7+ messages in thread
From: Ingo Molnar @ 2016-09-10 4:35 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Masahiro Yamada, x86, Ingo Molnar, Toshi Kani, Denys Vlasenko,
Borislav Petkov, Paul Gortmaker, H. Peter Anvin, Nathan Zimmer,
linux-kernel, Mike Travis, Daniel J Blueman, Dimitri Sivanich,
Matt Fleming, Hedi Berriche, Steffen Persvold, Alex Thorlton,
Wei Jiangang
* Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 8 Sep 2016, Ingo Molnar wrote:
> > * Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> > > static unsigned long set_apic_id(unsigned int id)
> > > {
> > > - unsigned long x;
> > > -
> > > /* maskout x2apic_extra_bits ? */
> > > - x = id;
> > > - return x;
> > > + return id;
> > > }
> >
> > This was clearly left there to document a quirk and as a placeholder for future
> > changes.
>
> Keeping the comment and rewording it to:
>
> /* CHECKME: Do we need to mask out the xapic extra bits */
>
> should be good enough. The variable dance is not really giving any value.
Yeah, sure - my point was that the mindless removal is wrong.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: squash lines for simple wrapper functions
2016-09-10 4:35 ` Ingo Molnar
@ 2016-09-10 5:53 ` Thomas Gleixner
2016-09-10 9:09 ` Ingo Molnar
0 siblings, 1 reply; 7+ messages in thread
From: Thomas Gleixner @ 2016-09-10 5:53 UTC (permalink / raw)
To: Ingo Molnar
Cc: Masahiro Yamada, x86, Ingo Molnar, Toshi Kani, Denys Vlasenko,
Borislav Petkov, Paul Gortmaker, H. Peter Anvin, Nathan Zimmer,
linux-kernel, Mike Travis, Daniel J Blueman, Dimitri Sivanich,
Matt Fleming, Hedi Berriche, Steffen Persvold, Alex Thorlton,
Wei Jiangang
On Sat, 10 Sep 2016, Ingo Molnar wrote:
> * Thomas Gleixner <tglx@linutronix.de> wrote:
>
> > On Thu, 8 Sep 2016, Ingo Molnar wrote:
> > > * Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> > > > static unsigned long set_apic_id(unsigned int id)
> > > > {
> > > > - unsigned long x;
> > > > -
> > > > /* maskout x2apic_extra_bits ? */
> > > > - x = id;
> > > > - return x;
> > > > + return id;
> > > > }
> > >
> > > This was clearly left there to document a quirk and as a placeholder for future
> > > changes.
> >
> > Keeping the comment and rewording it to:
> >
> > /* CHECKME: Do we need to mask out the xapic extra bits */
> >
> > should be good enough. The variable dance is not really giving any value.
>
> Yeah, sure - my point was that the mindless removal is wrong.
He kept the comment. The rewording is a bonus.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: squash lines for simple wrapper functions
2016-09-10 5:53 ` Thomas Gleixner
@ 2016-09-10 9:09 ` Ingo Molnar
0 siblings, 0 replies; 7+ messages in thread
From: Ingo Molnar @ 2016-09-10 9:09 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Masahiro Yamada, x86, Ingo Molnar, Toshi Kani, Denys Vlasenko,
Borislav Petkov, Paul Gortmaker, H. Peter Anvin, Nathan Zimmer,
linux-kernel, Mike Travis, Daniel J Blueman, Dimitri Sivanich,
Matt Fleming, Hedi Berriche, Steffen Persvold, Alex Thorlton,
Wei Jiangang
* Thomas Gleixner <tglx@linutronix.de> wrote:
> On Sat, 10 Sep 2016, Ingo Molnar wrote:
> > * Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > > On Thu, 8 Sep 2016, Ingo Molnar wrote:
> > > > * Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> > > > > static unsigned long set_apic_id(unsigned int id)
> > > > > {
> > > > > - unsigned long x;
> > > > > -
> > > > > /* maskout x2apic_extra_bits ? */
> > > > > - x = id;
> > > > > - return x;
> > > > > + return id;
> > > > > }
> > > >
> > > > This was clearly left there to document a quirk and as a placeholder for future
> > > > changes.
> > >
> > > Keeping the comment and rewording it to:
> > >
> > > /* CHECKME: Do we need to mask out the xapic extra bits */
> > >
> > > should be good enough. The variable dance is not really giving any value.
> >
> > Yeah, sure - my point was that the mindless removal is wrong.
>
> He kept the comment. The rewording is a bonus.
Ok, fair enough.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: squash lines for simple wrapper functions
2016-09-08 6:33 ` Ingo Molnar
2016-09-09 14:40 ` Thomas Gleixner
@ 2016-09-10 9:30 ` Masahiro Yamada
1 sibling, 0 replies; 7+ messages in thread
From: Masahiro Yamada @ 2016-09-10 9:30 UTC (permalink / raw)
To: Ingo Molnar, Thomas Gleixner
Cc: X86 ML, Ingo Molnar, Toshi Kani, Denys Vlasenko, Borislav Petkov,
Paul Gortmaker, H. Peter Anvin, Nathan Zimmer,
Linux Kernel Mailing List, Mike Travis, Daniel J Blueman,
Dimitri Sivanich, Matt Fleming, Hedi Berriche, Steffen Persvold,
Alex Thorlton, Wei Jiangang
Hi Ingo, Thomas,
Thanks for your review!
2016-09-08 15:33 GMT+09:00 Ingo Molnar <mingo@kernel.org>:
>> static unsigned int flat_get_apic_id(unsigned long x)
>> {
>> - unsigned int id;
>> -
>> - id = (((x)>>24) & 0xFFu);
>> -
>> - return id;
>> + return ((x) >> 24) & 0xFFu;
>
> So while we are removing unnecessary things, exactly why does the 'x' need
> parentheses?
I will change it to:
return (x >> 24) & 0xFF;
>> static unsigned long set_apic_id(unsigned int id)
>> {
>> - unsigned long x;
>> -
>> - x = ((id & 0xFFu)<<24);
>> - return x;
>> + return (id & 0xFFu) << 24;
>
> 'id' is already unsigned, why does the 'u' have to be stressed in the literal?
> (Ditto for other places as well)
I will change it to:
return (id & 0xFF) << 24;
>> static unsigned long numachip1_set_apic_id(unsigned int id)
>> {
>> - unsigned long x;
>> -
>> - x = ((id & 0xffU) << 24);
>> - return x;
>> + return (id & 0xffU) << 24;
>> }
>
> Why is the spelling of the literal inconsistent here with the other patterns?
I think 0xff is more consistent than 0xFF
in arch/x86/kernel/apic/apic_numachip.c
Making the constant literals consistent across files
is a too much churn, I think.
>> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
>> @@ -533,11 +533,8 @@ static unsigned int x2apic_get_apic_id(unsigned long x)
>>
>> static unsigned long set_apic_id(unsigned int id)
>> {
>> - unsigned long x;
>> -
>> /* maskout x2apic_extra_bits ? */
>> - x = id;
>> - return x;
>> + return id;
>> }
>
> This was clearly left there to document a quirk and as a placeholder for future
> changes.
>
As suggested by Thomas, I will change it to:
{
/* CHECKME: Do we need to mask out the xapic extra bits? */
return id;
}
(I am adding '?' at the comment line.)
If there is no more comment, I will send v2.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-09-10 9:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-06 11:24 [PATCH] x86: squash lines for simple wrapper functions Masahiro Yamada
2016-09-08 6:33 ` Ingo Molnar
2016-09-09 14:40 ` Thomas Gleixner
2016-09-10 4:35 ` Ingo Molnar
2016-09-10 5:53 ` Thomas Gleixner
2016-09-10 9:09 ` Ingo Molnar
2016-09-10 9:30 ` Masahiro Yamada
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).