Hi Akihiko, thanks for reading it that closely.
Akihiko Odaki <[email protected]> wrote: > With no response, __ps2_command() in libps2 returns -EIO. With this > patch, two 0xfe responses instead make it return -EPROTO. The two cases > therefore do not return the same error. Correct, and the sentence claiming otherwise was wrong. ps2_do_sendbyte() stops at -EAGAIN once the resends have used up max_attempts, and __ps2_command() maps that to -EPROTO on its way out so the resend does not leak to callers. The traces show the same split from the device side: without the patch each unknown command is written once -- nothing comes back, ps2dev->nak stays 1, -EIO -- and with it twice, because 0xFE makes libps2 retry once before it gives up. v2 says -EIO -> -EPROTO and drops the "same error" claim. > Commit 06b3611fc2a3 ("ps2: reject unknown commands, instead of > blindly accepting them") changed unknown keyboard commands from ACK > to Resend in 2016, so this behavior is ten years old, not twenty. Corrected, and v2 cites the commit instead of counting decades. > I suggest dropping the final sentence. > > Additionally, it would be better to ensure the inline rationale is > documented consistently across both the mouse and keyboard paths. Dropped -- and the sentence before it as well. It described the silence this patch removes, which reads oddly right next to the code that no longer produces it, and the commit message makes that point already. What is left is the sentence that says why a resend rather than an ACK, which is the part a reader cannot derive from the line below it. The keyboard path gets that same comment as patch 2 of v2, which keeps patch 1 a pure fix for stable. > Also, let's add Cc: [email protected] Added. One thing the re-measuring turned up: Linux runs into two unknown commands during mouse detection, not one. The ALPS probe sends 0xEC, which ps2_write_mouse() only answers while the mouse is in wrap mode, and the TrackPoint probe sends 0xE1. Each costs a 200 ms reply timeout, which is what the ~430 ms consist of. v2 says so. Christian
