From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1755278AbcIJJar (ORCPT <rfc822;w@1wt.eu>);
        Sat, 10 Sep 2016 05:30:47 -0400
Received: from conssluserg-04.nifty.com ([210.131.2.83]:62142 "EHLO
        conssluserg-04.nifty.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1755161AbcIJJaq (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Sat, 10 Sep 2016 05:30:46 -0400
DKIM-Filter: OpenDKIM Filter v2.10.3 conssluserg-04.nifty.com u8A9UXSm021901
X-Nifty-SrcIP: [209.85.213.169]
MIME-Version: 1.0
In-Reply-To: <20160908063359.GB24253@gmail.com>
References: <1473161053-10068-1-git-send-email-yamada.masahiro@socionext.com> <20160908063359.GB24253@gmail.com>
From: Masahiro Yamada <yamada.masahiro@socionext.com>
Date: Sat, 10 Sep 2016 18:30:32 +0900
X-Gmail-Original-Message-ID: <CAK7LNASxcum=9PLomcZ+=jOxhaPSzhKZuSXX7i6K21YR=qAamw@mail.gmail.com>
Message-ID: <CAK7LNASxcum=9PLomcZ+=jOxhaPSzhKZuSXX7i6K21YR=qAamw@mail.gmail.com>
Subject: Re: [PATCH] x86: squash lines for simple wrapper functions
To: Ingo Molnar <mingo@kernel.org>, Thomas Gleixner <tglx@linutronix.de>
Cc: X86 ML <x86@kernel.org>, Ingo Molnar <mingo@redhat.com>,
        Toshi Kani <toshi.kani@hpe.com>, Denys Vlasenko <dvlasenk@redhat.com>,
        Borislav Petkov <bp@suse.de>,
        Paul Gortmaker <paul.gortmaker@windriver.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Nathan Zimmer <nzimmer@sgi.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Mike Travis <travis@sgi.com>, Daniel J Blueman <daniel@numascale.com>,
        Dimitri Sivanich <sivanich@sgi.com>,
        Matt Fleming <matt@codeblueprint.co.uk>, Hedi Berriche <hedi@sgi.com>,
        Steffen Persvold <sp@numascale.com>, Alex Thorlton <athorlton@sgi.com>,
        Wei Jiangang <weijg.fnst@cn.fujitsu.com>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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