Re: [Linux-graphics-maintainer] [PATCH 3/6] Input: Update vmmouse.c to use the common VMW_PORT macros

[Thomas Hellstrom <thellstrom@vmware.com>]

On 12/02/2015 01:04 AM, Greg Kroah-Hartman wrote:
> On Tue, Dec 01, 2015 at 02:54:20PM -0800, Sinclair Yeh wrote:
>> Hi,
>>
>> On Tue, Dec 01, 2015 at 02:45:27PM -0800, Dmitry Torokhov wrote:
>>> On Tue, Dec 1, 2015 at 2:32 PM, Sinclair Yeh <syeh@vmware.com> wrote:
>>>> Hi,
>>>>
>> <snip>
>>
>>>>>>   */
>>>>>> -#define VMMOUSE_CMD(cmd, in1, out1, out2, out3, out4)      \
>>>>>> -({                                                 \
>>>>>> -   unsigned long __dummy1, __dummy2;               \
>>>>>> -   __asm__ __volatile__ ("inl %%dx" :              \
>>>>>> -           "=a"(out1),                             \
>>>>>> -           "=b"(out2),                             \
>>>>>> -           "=c"(out3),                             \
>>>>>> -           "=d"(out4),                             \
>>>>>> -           "=S"(__dummy1),                         \
>>>>>> -           "=D"(__dummy2) :                        \
>>>>>> -           "a"(VMMOUSE_PROTO_MAGIC),               \
>>>>>> -           "b"(in1),                               \
>>>>>> -           "c"(VMMOUSE_PROTO_CMD_##cmd),           \
>>>>>> -           "d"(VMMOUSE_PROTO_PORT) :               \
>>>>>> -           "memory");                              \
>>>>>> +#define VMMOUSE_CMD(cmd, in1, out1, out2, out3, out4)                 \
>>>>>> +({                                                            \
>>>>>> +   unsigned long __dummy1 = 0, __dummy2 = 0;                  \
>>>>> Why do we need to initialize dummies?
>>>> Because for some commands those parameters to VMW_PORT() can be both
>>>> input and outout.
>>> The vmmouse commands do not use them as input though, so it seems we
>>> are simply wasting CPU cycles setting them to 0 just because we are
>>> using the new VMW_PORT here. Why do we need to switch? What is the
>>> benefit of doing this?
>> There are two reasons.  One is to make the code more readable and
>> maintainable.  Rather than having mostly similar inline assembly
>> code sprinkled across multiple modules, we can just use the macros
>> and document that.
> But the macro is only used here, and the variables aren't used at all,
> so it makes no sense in this file.
>

IMO, this makes a lot of sense because we now get a single definition of
VMW_PORT in the platform code that a developer can refer to to
understand things like 32-64 bit compatibilty, and usage conditions and
it also forces the developer to adopt the good practice of clearing
currently unused input variables rather than to leave them undefined. In
addition, if something needs to be changed we have one single place to
change rather than a lot of places scattered all over various kernel
modules.

Things that we (I) previously, for example, didn't get quite right in
the vmmouse module despite spending a considerable amount of time on the
subject.

Thanks,
Thomas