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  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 1 Nov 2017 20:27:28 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Jassi Brar <jassisinghbrar@...il.com>
Cc:     Sudeep Holla <sudeep.holla@....com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH] mailbox: add support for doorbell/signal mode controllers

On Wed 01 Nov 20:02 PDT 2017, Jassi Brar wrote:

> On Thu, Nov 2, 2017 at 3:47 AM, Bjorn Andersson
> <bjorn.andersson@...aro.org> wrote:
> > On Wed 01 Nov 11:15 PDT 2017, Sudeep Holla wrote:
> >>
> >>  80         writel_relaxed(msg->cmd, mb->mbox_base +
> >> MAILBOX_A2B_CMD(chans->idx));
> >>  81         writel_relaxed(msg->rx_size, mb->mbox_base +
> >>
> >>  82                        MAILBOX_A2B_DAT(chans->idx));
> >>
> >>  83
> >
> > This is just terrible, using the void *mssg to pass a struct which is
> > interpreted by the controller removes any form of abstraction provided
> > by the framework.
> >
> > In my view the void *mssg should point to the data to be written in the
> > mailbox register, and hence might be of different size - but only of
> > native type.
> >
> Please note the terrible 'rx_size' is not a software option - the
> hardware has a DAT register so the driver rightfully allows a client
> to write a value in that as well.

Okay, so the interface exposed by the mailbox driver is not { cmd,
rx_size } but rather __le32 data[2], or perhaps a generic { cmd, data }.
That sounds more generic.


I think it would be good to replace the totally opaque void * with some
sort of structured data type and having the framework ensure that
clients and controllers are compatible. That would, by design, allow for
reuse between controllers and clients.

Perhaps something like:

enum mbox_msg_type {
	MBOX_MSG_TYPE_NULL,
	MBOX_MSG_TYPE_U32,
	MBOX_MSG_TYPE_CMD_DATA,
};

struct mbox_msg {
	enum mbox_msg_type type;

	union {
		u32 u;
		struct {
			u32 cmd;
			u32 data;
		} cd;
	};
};

And have the controller register for a specific "type", so the framework
could validate that the type of data being passed matches the hardware.

Have you had any thoughts around this before?

Regards,
Bjorn

Powered by blists - more mailing lists