| 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
| ||
|
Message-ID: <aUAIhNMTPQVl3b4W@google.com>
Date: Mon, 15 Dec 2025 13:09:24 +0000
From: Fabio Baltieri <fabiobaltieri@...omium.org>
To: Simon Glass <sjg@...omium.org>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Benson Leung <bleung@...omium.org>,
Guenter Roeck <groeck@...omium.org>,
Tzung-Bi Shih <tzungbi@...nel.org>, linux-input@...r.kernel.org,
devicetree@...r.kernel.org, chrome-platform@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/3] Input: cros_ec_keyb: add function key support
Hey Simon,
On Thu, Dec 11, 2025 at 06:29:01AM -0700, Simon Glass wrote:
> > @@ -44,6 +52,13 @@
> > * @bs_idev: The input device for non-matrix buttons and switches (or NULL).
> > * @notifier: interrupt event notifier for transport devices
> > * @vdata: vivaldi function row data
> > + * @fn_key: coordinate of the function key
> > + * @fn_keymap: array of coordinate and codes for the function keys
> > + * @fn_keymap_len: number of entries in the fn_keymap array
> > + * @fn_key_status: active function keys bitmap
> > + * @normal_key_status: active normal keys bitmap
> > + * @fn_key_pressed: tracks the function key status
> > + * @fn_key_triggered: tracks where any function key fired
> > */
> > struct cros_ec_keyb {
> > unsigned int rows;
> > @@ -61,6 +76,14 @@ struct cros_ec_keyb {
> > struct notifier_block notifier;
> >
> > struct vivaldi_data vdata;
> > +
> > + uint32_t fn_key;
>
> Normally we use u32/u8 these days
Okay, I did notice the file was a bit of a mix, I'll change them in v2.
>
> > + uint32_t *fn_keymap;
> > + int fn_keymap_len;
> > + uint32_t fn_key_status;
> > + uint8_t normal_key_status[CROS_EC_KEYBOARD_COLS_MAX];
> > + bool fn_key_pressed;
> > + bool fn_key_triggered;
> > };
> >
> > /**
> > @@ -166,16 +189,108 @@ static bool cros_ec_keyb_has_ghosting(struct cros_ec_keyb *ckdev, uint8_t *buf)
> > return false;
> > }
> >
> > +static bool cros_ec_key_is(int row, int col, uint32_t key)
> > +{
> > + if (row == KEY_ROW(key) && col == KEY_COL(key))
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +static void cros_ec_keyb_process_one(struct cros_ec_keyb *ckdev,
> > + int row, int col, bool state)
> > +{
> > + struct input_dev *idev = ckdev->idev;
> > + const unsigned short *keycodes = idev->keycode;
> > + int pos = MATRIX_SCAN_CODE(row, col, ckdev->row_shift);
> > + unsigned int code = keycodes[pos];
> > +
> > + dev_dbg(ckdev->dev, "changed: [r%d c%d]: byte %02x\n", row, col, state);
> > +
> > + if (ckdev->fn_keymap) {
> > + if (cros_ec_key_is(row, col, ckdev->fn_key)) {
> > + ckdev->fn_key_pressed = state;
> > +
> > + if (state) {
> > + ckdev->fn_key_triggered = false;
> > + } else if (!ckdev->fn_key_triggered) {
> > + /*
> > + * Send the original code if nothing else has
> > + * been pressed together with Fn.
> > + */
> > + input_event(idev, EV_MSC, MSC_SCAN, pos);
> > + input_report_key(idev, code, true);
> > + input_sync(ckdev->idev);
>
> What is this function? I might be missing a patch?
input_sync? it sends an EV_SYN, been there from the start, though I
noticed I miss one two lines below, was relying on the rest of the
function to send it but I changed the logic at some point and broke that
path, will fix that.
>
> > +
> > + input_event(idev, EV_MSC, MSC_SCAN, pos);
> > + input_report_key(idev, code, false);
> > + }
> > +
> > + return;
> > + }
> > +
> > + if (!state) {
> > + /* Key release, may need to release the Fn code */
> > + for (int i = 0; i < ckdev->fn_keymap_len; i++) {
> > + if (!cros_ec_key_is(row, col,
> > + ckdev->fn_keymap[i]))
> > + continue;
> > +
> > + if ((ckdev->fn_key_status & BIT(i)) == 0)
> > + continue;
> > +
> > + code = KEY_VAL(ckdev->fn_keymap[i]);
> > + ckdev->fn_key_status &= ~BIT(i);
> > +
> > + input_event(idev, EV_MSC, MSC_SCAN, pos);
> > + input_report_key(idev, code, state);
> > +
> > + return;
> > + }
> > +
> > + if ((ckdev->normal_key_status[col] & BIT(row)) == 0)
> > + /* Discard, key press code was not sent */
> > + return;
> > + } else if (ckdev->fn_key_pressed) {
> > + /* Key press while holding Fn */
> > + ckdev->fn_key_triggered = true;
> > +
> > + for (int i = 0; i < ckdev->fn_keymap_len; i++) {
> > + if (!cros_ec_key_is(row, col,
> > + ckdev->fn_keymap[i]))
> > + continue;
> > +
> > + code = KEY_VAL(ckdev->fn_keymap[i]);
> > + ckdev->fn_key_status |= BIT(i);
> > +
> > + input_event(idev, EV_MSC, MSC_SCAN, pos);
> > + input_report_key(idev, code, state);
> > +
> > + return;
> > + }
> > +
> > + /* Do not emit a code if the key is not mapped */
> > + return;
> > + }
> > + }
>
> I think this function could do with splitting a bit
Yeah, I don't love it either but there's a lot of logic intertwined in
there, tried to split it myself and ended up breaking stuff, the logic
for the fn key itsel though can go that's a good 16 lines, I'll start
with that, send a v2 and then go from there.
> Can the sandbox driver support this too?
Not sure what you are referring to, can you give me a pointer?
Hey thanks for the review, good to hear from you. :-)
Cheers,
Fabio
--
Fabio Baltieri
Powered by blists - more mailing lists