Re: [PATCH v2 16/21] ipmi: kcs_bmc: Add a "raw" character device interface

Andrew Jeffery
2021-04-12T01:33:52Z

On Fri, 9 Apr 2021, at 17:25, Arnd Bergmann wrote:
> On Fri, Mar 19, 2021 at 7:31 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > The existing IPMI chardev encodes IPMI behaviours as the name suggests.
> > However, KCS devices are useful beyond IPMI (or keyboards), as they
> > provide a means to generate IRQs and exchange arbitrary data between a
> > BMC and its host system.
>
> I only noticed the series after Joel asked about the DT changes on the arm
> side. One question though:
>
> How does this related to the drivers/input/serio/ framework that also talks
> to the keyboard controller for things that are not keyboards?

I've taken a brief look and I feel they're somewhat closely related.

It's plausible that we could wrangle the code so the Aspeed and Nuvoton
KCS drivers move under drivers/input/serio. If you squint, the i8042
serio device driver has similarities with what the Aspeed and Nuvoton
device drivers are providing to the KCS IPMI stack.

Both the KCS IPMI and raw chardev I've implemented in this patch need
both read and write access to the status register (STR). serio could
potentially expose its value through serio_interrupt() using the
SERIO_OOB_DATA flag, but I haven't put any thought into it beyond this
sentence. We'd need some extra support for writing STR via the serio
API. I'm not sure that fits into the abstraction (unless we make
serio_write() take a flags argument?).

In that vein, the serio_raw interface is close to the functionality
that the raw chardev provides in this patch, though again serio_raw
lacks userspace access to STR. Flags are ignored in the ->interrupt()
callback so all values received via ->interrupt() are exposed as data.
The result is there's no way to take care of SERIO_OOB_DATA in the
read() path. Given that, I think we'd have to expose an ioctl() to
access the STR value after taking care of SERIO_OOB_DATA in
->interrupt().

I'm not sure where that lands us.

Dmitry, any thoughts here?

> Are these
> separate communication channels on adjacent I/O ports, or does there
> need to be some arbitration?

As it stands there's no arbitration.

Cheers,

Andrew