lkml.org 
[lkml]   [2014]   [Jan]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] Input: ims-pcu - remove unneeded get_unaligned_xxx
From
On Fri, Jan 3, 2014 at 10:16 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.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@gmail.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


\
 
 \ /
  Last update: 2014-01-04 08:01    [W:2.163 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site