Message-ID: <9c1663cb-e40b-4afd-b82c-0ecc939c4485@gmail.com>
Date: Mon, 12 Aug 2024 14:49:20 +0200
From: Denis Benato <benato.denis96@...il.com>
To: Antheas Kapenekakis <lkml@...heas.dev>,
 "Derek J. Clark" <derekjohn.clark@...il.com>
Cc: luke@...nes.dev, bentiss@...nel.org, jikos@...nel.org,
 linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hid-asus-ally: Add full gamepad support

On 12/08/24 10:31, Antheas Kapenekakis wrote:
> Hi Derek and Denis,
> 
> Let us be civil. If I could have bug reported you I would have bug reported
> you. However, for some weird coincidence, I do not have access to the
> ShadowBlip bug tracker or the relevant communities. Nevertheless, this is
> not relevant public discussion. Let us refrain from this discussion further,
> including e.g., name-calling.
Hello,

First and foremost we have been civil, we vigorously stated the truth and
enriched this discussions with information and context you initially left out
(for reasons that at this point are very clear to everyone that has been reading)
and I can assure that we will remain civil as we are not going to make fools
out of ourselves.

We are discussing kernel drivers here, those userspace software are none of your
concerns as those are none of kernel maintainers concerns either. 

> 
> The MCU of the Ally is the embedded microcontroller that runs the RGB and the
> controller of the Ally. Therefore, the discussion of the MCU powersave and
> wake is relevant here. What is not proper is that I should also have replied
> to the original patch. I admitted that much in my original email. However,
> since you are now aware of it, I trust that you can block the patch for
> merging until you review it.
> 

The MCU powersave is a feature driven by the APCI interface and here we are discussing
an USB driver: there is absolutely no reasons to block this patch for a problem you
claim you have found (but never reported) in a totally different driver that even lives
in a totally different subsystem, so if you want those issue solved file the proper bug
report where it is relevant (not here).

> If the patch does not function under normal operation, this is relevant here.
> It means this extended patchset is built on reliance of the broken patch,
> which raises questions. Nevertheless, calling the patchset "experimental"
> is hearsay. Therefore, I will refer to it as ambitious from now on :).
> 

The patch does work under normal operation and it has been tested by many as stated already.

The device firmware performs the wake-up of the device and I don't
see any problem assuming the hardware works to submit a driver:
in fact this assumption is made by every kernel driver.

Again, you claim, without providing evidence, that the hardware is not properly waking up
when a certain ACPI attribute is set to one. Just set it to zero. Over these nine months
you had absolutely no problems is driving the hardware via acpi_call driver and tools
making SMU calls.

If you know a way that I can trigger this problem within my hardware
please, use the relevant mailing list reporting a bug so we can solve
it as we have solved other problems before.

Do not obstruct a fully working kernel driver for reasons unrelated to it.

>> This is 100% an issue with your software. I just completed an exhaustive
>> battery of testing at 8w STAPM/FPPT/SPPT on Quiet power profile with only
>> 2 cores active. The tests included using Steam by itself and the kernel
>> implementation, as well as running InputPlumber (which also has an
>> 80ms delay).
> > Please refrain from name-calling. I was very specific to say that the issue
> here is that under load when in a game, Steam will either let A leak through
> to the game or not respond, sporadically. While in Steam UI the combination
> always works, regardless of TDP. Since you did not test while in a game,
> this renders your test invalid.
> 

I have opened the overlay in game with no problems.

Why should we refrain from name-calling? You started this discussion with the
name of your own software, and in the kernel no anonymous contributions are
allowed either.

Plus, if steam (another userspace software) has a problem with combo keys
recognition and timing we are not entitled to solve that in a kernel driver anyway.

> To save you some additional invalid testing for the other issues: using
> ryzenadj on the Ally causes misbehavior, especially after suspend, where the
> TDP will reset. In addition, modifying SMP and core parking can freeze the
> Ally during suspend. Therefore, for further testing, I expect you to disable
> your "PowerStation" application and instead use the low-power platform
> profile, which is provided by asus-wmi, and is the vendor specific way for
> setting TDP on the Ally. Or use asusctl, which does the same.
> 

All of these information are given to you directly by us nine months ago,
while you were incorrectly driving the hardware via ryzenadj doing exactly
what we told you not to do, so you can expect we know this already.

Here you also make assumptions on how more than four people made their tests,
and while I don't speak for others I can ensure mine were done using the
firmware interface ASUS put in place and is exposed via asus-wmi platform driver:
the driver also in charge of handling the MCU powersave feature you are using as
an unreasonable point of arguments here.

In fact I have been telling you to use that interface for (ance again) more than nine months now,
and you only recently did so in your software because problems we told you were
going to manifest finally manifested, and you have been left with no other choice
but to use the proper way of driving the hardware in question. 

> As for using direct HID commands to program RGB, asusctl does the same,
> including many other userspace apps, and prior to this patchset there was
> no way to do different, so I do not see the problem here.
> 

There is no problem here: your software is driving the reported state inconsistent
with the hardware state and that's a you problem.

Our userspace tooling have been using what the kernel has been exposing, especially
Asusctl that is software written by the same person that contributes to said drivers
and is very well informed about other lkml discussion he partecipates.

> Best,
> Antheas

Best regards,
Denis

Powered by blists - more mailing lists