linux-kernel - Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver

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]

Message-ID: <1dff7a78-1693-45d7-8ee3-357b33848595@quicinc.com>
Date: Tue, 31 Dec 2024 13:00:02 +0800
From: "Aiqun(Maria) Yu" <quic_aiquny@...cinc.com>
To: Pengyu Luo <mitltlatltl@...il.com>
CC: <andersson@...nel.org>, <bryan.odonoghue@...aro.org>,
        <conor+dt@...nel.org>, <devicetree@...r.kernel.org>,
        <dmitry.baryshkov@...aro.org>, <gregkh@...uxfoundation.org>,
        <hdegoede@...hat.com>, <heikki.krogerus@...ux.intel.com>,
        <ilpo.jarvinen@...ux.intel.com>, <konradybcio@...nel.org>,
        <krzk+dt@...nel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-pm@...r.kernel.org>,
        <linux-usb@...r.kernel.org>, <nikita@...n.ru>,
        <platform-driver-x86@...r.kernel.org>, <robh@...nel.org>,
        <sre@...nel.org>
Subject: Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp)
 EC driver

On 12/30/2024 6:44 PM, Pengyu Luo wrote:
> On Mon, Dec 30, 2024 at 5:04 PM Aiqun(Maria) Yu <quic_aiquny@...cinc.com> wrote:
>> On 12/28/2024 1:13 AM, Pengyu Luo wrote:
[...]
>>> +     i2c_transfer(client->adapter, msgs, 2);
>>
>> ARRAY_SIZE(msgs) is suggested instead of pure 2.
>>
> 
> Agree
> 
>>> +     usleep_range(2000, 2500);
>>
>> Why is a sleep needed here? Is this information specified in any datasheet?
>>
> 
> Have a break between 2 transaction. This sleep happens in acpi code, also
> inside a critical region. I rearranged it.
> 
> Local7 = Acquire (\_SB.IC16.MUEC, 0x03E8)
> ...
> write ops
> ...
> Sleep (0x02)
> ...
> read ops
> ...
> Release (\_SB.IC16.MUEC)

Could you please share the exact code snippet that is being referenced?
I'm a bit confused because it doesn't seem to align with the current
logic, which doesn't have read operations within the same mutex lock. I
also want to understand the background and necessity of the sleep function.

> 
>>> +
>>> +     mutex_unlock(&ec->lock);
>>> +
>>> +     return *resp;
>>> +}
>>> +
>>> +/* -------------------------------------------------------------------------- */
>>> +/* Common API */
[...]
>>> +     int i, ret;
>>> +     u8 _resp[RESP_HDR_SIZE + 1];
>>> +     u8 req[REQ_HDR_SIZE + 1] = {0x02, EC_READ, 1, };
>>
>> Could it be made more readable by specifying the macro names for 0x02
>> and 1? This would help in understanding the meaning of these numbers.
>>
> 
> I really don't know the meaning of master command 0x02, 1 is the size for
> the data_seq behind of it. There are many possible sizes. It is not a good
> idea to define a macro name for everyone.
> 

Perhaps you didn't get the "arg..." magic here. A single definition is
sufficient for all sizes.

>> Also, please ensure the actual size of the request buffer is handled
>> properly. In gaokun_ec_request(), the req is passed down directly, and
>> the i2c_msg.len is used dynamically with req[INPUT_SIZE_OFFSET] +
>> REQ_HDR_SIZE. This requires the caller to carefully manage the contents
>> to avoid memory over-read, making the code difficult to read.
>>
>> Creating a defined macro can help you avoid manually defining the size.
>> For example:
>> #define REQ(size, data_0, data_1, args...) \
>> u8 req[REQ_HDR_SIZE + size] = {data_0, data_1, size, args};
>>
> 
> I think wrapping like this is not recommended, see '5)' in [1]
> 
> Best wishes,
> Pengyu
> 
> [1] https://www.kernel.org/doc/html/v4.10/process/coding-style.html#macros-enums-and-rtl

I believe that the consideration of namespace collisions is a valid concern.

Some examples can be like have a naming pattern as well:
/*To have a name pattern to reflect the size like reg0/reg1/reg2*/
#define REQ(variable_name, size, data_0, data_1, args...) \
u8 ##variable_name[REQ_HDR_SIZE + size] = {data_0, data_1, size, args};

/*u8 req1[REQ_HDR_SIZE + 1] = {0x02, EC_READ, 1, };*/
REQ(req, 1, 0x02, EC_READ);

/*u8 req2[REQ_HDR_SIZE + 2] = {0x02, 0x68, 2, 3, 0x5a}; */
REQ(req, 2, 0x02, 0x68, 3, 0x5a);

Please note that this is just an example and a suggestion to avoid the
current manual variable pattern setting. The final decision still
requires the current maintainers' agreement.

-- 
Thx and BRs,
Aiqun(Maria) Yu