linux-kernel - RE: [PATCH v3 3/4] soc: aspeed: Add eSPI driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 27 Aug 2021 08:49:13 +0000
From:   ChiaWei Wang <chiawei_wang@...eedtech.com>
To:     Jeremy Kerr <jk@...econstruct.com.au>,
        "joel@....id.au" <joel@....id.au>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "andrew@...id.au" <andrew@...id.au>,
        "linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
        "openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v3 3/4] soc: aspeed: Add eSPI driver

Hi Jeremy

> From: Jeremy Kerr <jk@...econstruct.com.au>
> Sent: Friday, August 27, 2021 12:37 PM
> 
> Hi Chiawei,
> 
> > The eSPI slave device comprises four channels, where each of them has
> > individual functionality.  Putting the four channels driver code into
> > a single file makes it hard to maintain and trace.
> 
> Yep, understood.
> 
> > We did consider to make them standard .c files.
> > But it requires to export channel functions into kernel space although
> > they are dedicated only to this eSPI driver.
> 
> What do you mean by "export into kernel space" here? The function prototypes

The channel functions will be visible to all kernel driver files.

> need to be available to your main (-ctrl.c) file, regardless of whether you're
> putting the entire functions in a header file, or just the prototype. There's
> doesn't need to be any difference in visibility outside of your own module if
> you were to do this the usual way.

Maybe I was trying to make channels function visible only to espi-ctrl.c too far.
I will revise the driver to present in the usual .c way.

> 
> > As espi-ctrl needs to invoke corresponding channel functions when it
> > is interrupted by eSPI events.
> >
> > To avoid polluting kernel space, we decided to put driver code in
> > header files and make the channel functions 'static'.
> >
> > BTW, I once encountered .c file inclusion in other projects. Is it
> > proper for Linux driver development?
> 
> It can be, just that in this case it's a bit unusual, and I can't see a good reason
> for doing so. This could just be a standard multiple-source-file module.
> 
> > eSPI communication is based on the its cycle packet format.
> > We intended to let userspace decided how to interpret and compose
> > TX/RX packets including header, tag, length (encoded), and data.
> > IOCTL comes to our first mind as it also works in the 'packet' like
> > paradigm.
> 
> But you're not always exposing a packet-like interface for this. For example,
> your virtual-wire interface just has a get/set interface for bits in a register
> (plus some PCH event handling, which may not be applicable to all
> platforms...).
> 
> The other channels do look like more of a packet interface though, but in that
> case I'm not convinced that an ioctl interface is the best way to go for that.
> You're essentially sending a (length, pointer) pair over the ioctls there, which
> sounds more like a write() than an ioctl().

In most cases, yes. 
Currently only the peripheral channel has more than the 2 (put tx/get rx) IOCTL code.
We think it might be a good idea to make the user interfaces of all channels consistent using IOCTL.

> 
> Regardless of the choice of interface though, this will definitely need some
> documentation or description of the API, and the ioc header to be somewhere
> useful for userspace to consume.
> 
> With that documented, we'd have a better idea of how the new ABI is
> supposed to work.

Sure. more comments will be added in aspeed-espi-ioc.h to describe the usage and the purpose.

Thanks for your feedback.

Chiawei