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: <20241229155551.92070-1-mitltlatltl@gmail.com>
Date: Sun, 29 Dec 2024 23:55:51 +0800
From: Pengyu Luo <mitltlatltl@...il.com>
To: ilpo.jarvinen@...ux.intel.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,
	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,
	mitltlatltl@...il.com,
	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 Sun, Dec 29, 2024 at 11:33 PM Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com> wrote:
> On Sat, 28 Dec 2024, Pengyu Luo wrote:
> > On Sat, Dec 28, 2024 at 8:33 PM Bryan O'Donoghue <bryan.odonoghue@...aro.org> wrote:
> > > On 27/12/2024 17:13, Pengyu Luo wrote:
> > > > There are 3 variants, Huawei released first 2 at the same time.
> > >
> > > There are three variants of which Huawei released the first two
> > > simultaneously.

[skipped]

> > > > +/* Thermal Zone */
> > > > +/* Range from 0 to 0x2C, partial valid */
> > > > +static const u8 temp_reg[] = {0x05, 0x07, 0x08, 0x0E, 0x0F, 0x12, 0x15, 0x1E,
> > > > +                           0x1F, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26,
> > > > +                           0x27, 0x28, 0x29, 0x2A};
> > > > +
> > > > +int gaokun_ec_wmi_get_temp(struct gaokun_ec *ec, s16 temp[GAOKUN_TZ_REG_NUM])
> > >
> > > int gaokun_ec_wmi_get_temp(struct gaokun_ec *ec, s16 *temp, size_t
> > > temp_reg_num)
> > >
> > >
> > > > +{
> > > > +     /* GTMP */
> > > > +     u8 req[REQ_HDR_SIZE] = {0x02, 0x61, 1,};
> > > > +     u8 resp[RESP_HDR_SIZE + sizeof(s16)];
> > > > +     int ret, i = 0;
> > > > +
> > > > +     while (i < GAOKUN_TZ_REG_NUM) {
> > > while (i < temp_reg_num)
> > >
> >
> > It is a constant. But later, as Krzysztof suggested, I will use interfaces
> > from hwmon, then reading one at a time.
> >
> > > > +             req[INPUT_DATA_OFFSET] = temp_reg[i];
> > > > +             ret = gaokun_ec_read(ec, req, sizeof(resp), resp);
> > > > +             if (ret)
> > > > +                     return -EIO;
> > > > +             temp[i++] = *(s16 *)(resp + DATA_OFFSET);
> > >
> > > What's the point of the casting here ?
> > >
> > > memcpy(temp, resp, sizeof(s16));
> > > temp++;
> >
> > A 2Bytes symbol number in little endian, ec return it like this, so
> > casting.
>
> You should use __le16 and proper endianess conversion function then.
>

Agree

> It's bit confusing that in the declaration you used RESP_HDR_SIZE and here
> you do it with DATA_OFFSET instead. It feels DATA_OFFSET is unnecessary
> duplicate of RESP_HDR_SIZE and will easily lead confusing variation such
> as above.
>

I totally agree with you, it is duplicated.
In declaration, u8 resp[RESP_HDR_SIZE]; RESP_HDR_SIZE indicates the size.
When assigning, val = resp[DATA_OFFSET]; let us know we are extracting a
data from a response without thinking, so I added an alias. Removing it
is also fine for me.

Best wishes,
Pengyu