Received-SPF: pass (google.com: domain of linux-kernel+bounces-194496-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240507-cros_ec-hwmon-v2-0-1222c5fca0f7@weissschuh.net>
 <20240507-cros_ec-hwmon-v2-1-1222c5fca0f7@weissschuh.net> <SY4P282MB30635BA1D4087113E79921B5C5F52@SY4P282MB3063.AUSP282.PROD.OUTLOOK.COM>
 <9cf224dd-51eb-4608-abcf-06f337d08178@t-8ch.de> <SY4P282MB306325BB023A95198F25A21DC5F12@SY4P282MB3063.AUSP282.PROD.OUTLOOK.COM>
 <c9b110eb-ff0e-41f2-9492-8a5d8c3c01d0@roeck-us.net> <b8072b36-688f-41b8-8b32-40fc4fa4d148@t-8ch.de>
 <6824f030-92da-4439-af3b-8c2498f4382e@roeck-us.net> <SY4P282MB30638301303268093B6D1ABFC5F22@SY4P282MB3063.AUSP282.PROD.OUTLOOK.COM>
 <22a16af6-93c4-454c-853b-5959a5c018d3@t-8ch.de> <SY4P282MB30634D9D9873C9C8DC41D4EEC5F22@SY4P282MB3063.AUSP282.PROD.OUTLOOK.COM>
In-Reply-To: <SY4P282MB30634D9D9873C9C8DC41D4EEC5F22@SY4P282MB3063.AUSP282.PROD.OUTLOOK.COM>
From: Guenter Roeck <groeck@google.com>
Date: Wed, 29 May 2024 10:00:27 -0700
Message-ID: <CABXOdTcyuR-YJYoMrAh11ksYcL-6LZPFERw94Z8-mTgMUfLP3g@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver
To: Stephen Horvath <s.horvath@outlook.com.au>
Cc: =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= <linux@weissschuh.net>, 
	Guenter Roeck <linux@roeck-us.net>, Jean Delvare <jdelvare@suse.com>, 
	Benson Leung <bleung@chromium.org>, Lee Jones <lee@kernel.org>, 
	Guenter Roeck <groeck@chromium.org>, linux-kernel@vger.kernel.org, 
	linux-hwmon@vger.kernel.org, chrome-platform@lists.linux.dev, 
	Dustin Howett <dustin@howett.net>, Mario Limonciello <mario.limonciello@amd.com>, 
	Moritz Fischer <mdf@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, May 29, 2024 at 12:40=E2=80=AFAM Stephen Horvath
<s.horvath@outlook.com.au> wrote:
>
> Hi Thomas,
>
> On 29/5/24 16:23, Thomas Wei=C3=9Fschuh wrote:
> > On 2024-05-29 10:58:23+0000, Stephen Horvath wrote:
> >> On 29/5/24 09:29, Guenter Roeck wrote:
> >>> On 5/28/24 09:15, Thomas Wei=C3=9Fschuh wrote:
> >>>> On 2024-05-28 08:50:49+0000, Guenter Roeck wrote:
> >>>>> On 5/27/24 17:15, Stephen Horvath wrote:
> >>>>>> On 28/5/24 05:24, Thomas Wei=C3=9Fschuh wrote:
> >>>>>>> On 2024-05-25 09:13:09+0000, Stephen Horvath wrote:
> >>>>>>>> Don't forget it can also return `EC_FAN_SPEED_STALLED`.
> >
> > <snip>
> >
> >>>>>>>
> >>>>>>> Thanks for the hint. I'll need to think about how to
> >>>>>>> handle this better.
> >>>>>>>
> >>>>>>>> Like Guenter, I also don't like returning `-ENODEV`,
> >>>>>>>> but I don't have a
> >>>>>>>> problem with checking for `EC_FAN_SPEED_NOT_PRESENT`
> >>>>>>>> in case it was removed
> >>>>>>>> since init or something.
> >>>>>>>
> >>>>>
> >>>>> That won't happen. Chromebooks are not servers, where one might
> >>>>> be able to
> >>>>> replace a fan tray while the system is running.
> >>>>
> >>>> In one of my testruns this actually happened.
> >>>> When running on battery, one specific of the CPU sensors sporadicall=
y
> >>>> returned EC_FAN_SPEED_NOT_PRESENT.
> >>>>
> >>>
> >>> What Chromebook was that ? I can't see the code path in the EC source
> >>> that would get me there.
> >>>
> >>
> >> I believe Thomas and I both have the Framework 13 AMD, the source code=
 is
> >> here:
> >> https://github.com/FrameworkComputer/EmbeddedController/tree/lotus-zep=
hyr
> >
> > Correct.
> >
> >> The organisation confuses me a little, but Dustin has previous said on=
 the
> >> framework forums (https://community.frame.work/t/what-ec-is-used/38574=
/2):
> >>
> >> "This one is based on the Zephyr port of the ChromeOS EC, and tracks
> >> mainline more closely. It is in the branch lotus-zephyr.
> >> All of the model-specific code lives in zephyr/program/lotus.
> >> The 13"-specific code lives in a few subdirectories off the main tree =
named
> >> azalea."
> >
> > The EC code is at [0]:
> >
> > $ ectool version
> > RO version:    azalea_v3.4.113353-ec:b4c1fb,os
> > RW version:    azalea_v3.4.113353-ec:b4c1fb,os
> > Firmware copy: RO
> > Build info:    azalea_v3.4.113353-ec:b4c1fb,os:7b88e1,cmsis:4aa3ff 2024=
-03-26 07:10:22 lotus@ip-172-26-3-226
> > Tool version:  0.0.1-isolate May  6 2024 none
>
> I can confirm mine is the same build too.
>
> >  From the build info I gather it should be commit b4c1fb, which is the
> > current HEAD of the lotus-zephyr branch.
> > Lotus is the Framework 16 AMD, which is very similar to Azalea, the
> > Framework 13 AMD, which I tested this against.
> > Both share the same codebase.
> >
> >> Also I just unplugged my fan and you are definitely correct, the EC on=
ly
> >> generates EC_FAN_SPEED_NOT_PRESENT for fans it does not have the capab=
ility
> >> to support. Even after a reboot it just returns 0 RPM for an unplugged=
 fan.
> >> I thought about simulating a stall too, but I was mildly scared I was =
going
> >> to break one of the tiny blades.
> >
> > I get the error when unplugging *the charger*.
> >
> > To be more precise:
> >
> > It does not happen always.
> > It does not happen instantly on unplugging.
> > It goes away after a few seconds/minutes.
> > During the issue, one specific sensor reads 0xffff.
> >
>
> Oh I see, I haven't played around with the temp sensors until now, but I
> can confirm the last temp sensor (cpu@4c / temp4) will randomly (every
> ~2-15 seconds) return EC_TEMP_SENSOR_ERROR (0xfe).
> Unplugging the charger doesn't seem to have any impact for me.
> The related ACPI sensor also says 180.8=C2=B0C.
> I'll probably create an issue or something shortly.
>
> I was mildly confused by 'CPU sensors' and 'EC_FAN_SPEED_NOT_PRESENT' in
> the same sentence, but I'm now assuming you mean the temp sensor?
>

Same here. it might not matter as much if the values were the same,
but EC_FAN_SPEED_NOT_PRESENT =3D=3D 0xffff,  and
EC_TEMP_SENSOR_NOT_PRESENT=3D=3D0xff, so they must not be confused with
each other. EC_TEMP_SENSOR_NOT_PRESENT should be static as well,
though, and not be returned randomly.

Guenter

> >>>>>>> Ok.
> >>>>>>>
> >>>>>>>> My approach was to return the speed as `0`, since
> >>>>>>>> the fan probably isn't
> >>>>>>>> spinning, but set HWMON_F_FAULT for `EC_FAN_SPEED_NOT_PRESENT` a=
nd
> >>>>>>>> HWMON_F_ALARM for `EC_FAN_SPEED_STALLED`.
> >>>>>>>> No idea if this is correct though.
> >>>>>>>
> >>>>>>> I'm not a fan of returning a speed of 0 in case of errors.
> >>>>>>> Rather -EIO which can't be mistaken.
> >>>>>>> Maybe -EIO for both EC_FAN_SPEED_NOT_PRESENT (which
> >>>>>>> should never happen)
> >>>>>>> and also for EC_FAN_SPEED_STALLED.
> >>>>>>
> >>>>>> Yeah, that's pretty reasonable.
> >>>>>>
> >>>>>
> >>>>> -EIO is an i/o error. I have trouble reconciling that with
> >>>>> EC_FAN_SPEED_NOT_PRESENT or EC_FAN_SPEED_STALLED.
> >>>>>
> >>>>> Looking into the EC source code [1], I see:
> >>>>>
> >>>>> EC_FAN_SPEED_NOT_PRESENT means that the fan is not present.
> >>>>> That should return -ENODEV in the above code, but only for
> >>>>> the purpose of making the attribute invisible.
> >>>>>
> >>>>> EC_FAN_SPEED_STALLED means exactly that, i.e., that the fan
> >>>>> is present but not turning. The EC code does not expect that
> >>>>> to happen and generates a thermal event in case it does.
> >>>>> Given that, it does make sense to set the fault flag.
> >>>>> The actual fan speed value should then be reported as 0 or
> >>>>> possibly -ENODATA. It should _not_ generate any other error
> >>>>> because that would trip up the "sensors" command for no
> >>>>> good reason.
> >>>>
> >>>> Ack.
> >>>>
> >>>> Currently I have the following logic (for both fans and temp):
> >>>>
> >>>> if NOT_PRESENT during probing:
> >>>>     make the attribute invisible.
> >>>>
> >>>> if any error during runtime (including NOT_PRESENT):
> >>>>     return -ENODATA and a FAULT
> >>>>
> >>>> This should also handle the sporadic NOT_PRESENT failures.
> >>>>
> >>>> What do you think?
> >>>>
> >>>> Is there any other feedback to this revision or should I send the ne=
xt?
> >>>>
> >>>
> >>> No, except I'd really like to know which Chromebook randomly generate=
s
> >>> a EC_FAN_SPEED_NOT_PRESENT response because that really looks like a =
bug.
> >>> Also, can you reproduce the problem with the ectool command ?
> >
> > Yes, the ectool command reports the same issue at the same time.
> >
> > The fan affected was always the sensor cpu@4c, which is
> > compatible =3D "amd,sb-tsi".
> >
> >> I have a feeling it was related to the concurrency problems between AC=
PI and
> >> the CrOS code that are being fixed in another patch by Ben Walsh, I wa=
s also
> >> seeing some weird behaviour sometimes but I *believe* it was fixed by =
that.
> >
> > I don't think it's this issue.
> > Ben's series at [1], is for MEC ECs which are the older Intel
> > Frameworks, not the Framework 13 AMD.
>
> Yeah sorry, I saw it mentioned AMD and threw it into my kernel, I also
> thought it stopped the 'packet too long' messages (for
> EC_CMD_CONSOLE_SNAPSHOT) but it did not.
>
> Thanks,
> Steve