linux-kernel - Re: [PATCH] Input: ims-pcu - remove unneeded get_unaligned

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, 3 Jan 2014 22:49:07 -0800
From:	Andrey Smirnov <andrew.smirnov@...il.com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Input: ims-pcu - remove unneeded get_unaligned_xxx

On Fri, Jan 3, 2014 at 10:16 PM, Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
> On Fri, Jan 03, 2014 at 09:52:25PM -0800, Andrey Smirnov wrote:
>> On Fri, Jan 3, 2014 at 9:28 PM, Dmitry Torokhov
>> <dmitry.torokhov@...il.com> wrote:
>> > pcu->cmd_buf[IMS_PCU_DATA_OFFSET] is word aligned so we do not need to use
>> > get_unaligned_le16 to access it.
>> >
>> > Also let's add build time check to make sure it stays aligned.
>>
>> - AFAIK, there's no guarantee the "pcu" itself is aligned
>
> Yes. kmalloc returns aligned pointer, otherwise every access to members
> other than u8 would risk unaligned exception.

OK, fair point.

>
>> - This change assumes that aligning data on the 2-byte boundary is
>> sufficient for all architectures that do not allow unaligned data
>> access, which I don't think is a good assumption to make
>
> What arches require word access be double-word aligned?

I don't know and neither do I care. Even if there aren't any in Linux
today do you expect it to never add a support to one that would impose
such a restriction?

The whole point of get_unaligned_* "framework" is to provide drivers
with unified interface that would allow you not to care about
alignment. "Framework" in which architectures that support unaligned
access can fallback to good old functions like *_to_cpup and
architectures that do would provide the code to handle access in
whatever manner is best suited for them.

>
>> - On x86 or any other architecture that allows unaligned access
>> get_unaligned_le16() is actually results to call to le16_to_cpup(), so
>> this change doesn't really save anything while imposing restrictions
>> on the arrangement of the fields in struct ims_pcu and causing
>> unnecessary build errors.
>
> Unless somebody changes the layout there won't be any new build errors,
> will there?

So do you expect that code to never change from now on? I most likely
will be working on changes to support accelerometer data aggregation
and implementing associated with it input devices and this may or may
not require me to add fields to that structure, so what am I supposed
to do in that case? Juggle fields around until I find the right
combination that does not trigger build error?.

I honestly don't understand the purpose of this change, it doesn't
really save any performance, IMHO decreases potability of the driver,
so what is the gain. What does adding all the restrictions that this
change imposes buy us?

>
> Thanks.
>
> --
> Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/