linux-kernel - Re: [PATCH 2/3] platform: surface: Add surface xbl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 29 Oct 2021 15:32:01 +0300
From:   Felipe Balbi <balbi@...nel.org>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Jarrett Schultz <jaschultzms@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <mgross@...ux.intel.com>,
        Maximilian Luz <luzmaximilian@...il.com>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jarrett Schultz <jaschultz@...rosoft.com>
Subject: Re: [PATCH 2/3] platform: surface: Add surface xbl


Hi,

Andy Shevchenko <andy.shevchenko@...il.com> writes:

> On Fri, Oct 29, 2021 at 7:48 AM Felipe Balbi <balbi@...nel.org> wrote:
>> Andy Shevchenko <andy.shevchenko@...il.com> writes:
>
> ...
>
>> > Capital L will be better to read and understand the
>> > abbreviation. Actually usually we do something like this:
>> >
>> > Extensible Boot Loader (EBL)
>>
>> nah, this is silly Andy. It's just capitalized as eXtensible Boot
>> Loader, very much akin to eXtensible Host Controller Interface.
>
> My point here is to have a full name followed by the abbreviation. and
> n(O)t in (F)ancy st(Y)le.

too bad my patch removing acronyms from the kernel got rejects :-p

Seriously, this is pretty pointless. You're vouching for something that
will just cause confusion. Every piece of internal documentation refers
to xbl and you want this to be renamed to ebl because it looks nicer for
you. Thanks, but no thanks.

>> >  +static const struct attribute_group inputs_attr_group = {
>> >  +       .attrs = inputs_attrs,
>> >  +};
>> >  +
>> >  +static u8 surface_xbl_readb(void __iomem *base, u32 offset)
>> >  +{
>> >  +       return readb(base + offset);
>> >  +}
>> >  +
>> >  +static u16 surface_xbl_readw(void __iomem *base, u32 offset)
>> >  +{
>> >  +       return readw(base + offset);
>> >  +}
>> >
>> > Either use corresponding io accessors in-line, or make first parameter
>> > to be sirface_xbl pointer. Otherwise these helpers useless.
>>
>> I agree with passing surface_xbl point as first parameter, but calling
>> the accessors pointless is a bit much. At a minimum, they make it easier
>> to ftrace the entire driver by simply ftracing surface_xbl_*
>
> My point is that the above seems half-baked. It's pointless to have a
> func(a,b) { return readl(a + b); }. It doesn't add value.

sure it does. echo surface_xbl_* > ftrace_filter_function (or whatever
the filename was) it reason enough IMHO. Not to mention that these
little accessors will likely be optimized by the compiler.

-- 
balbi