linux-kernel - RE: [PATCH 4/9] mailbox: create opened message type

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, 21 Dec 2012 10:28:47 +0000
From:	"Bedia, Vaibhav" <vaibhav.bedia@...com>
To:	Loic Pallardy <loic.pallardy-ext@...ricsson.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:	Ohad Ben-Cohen <ohad@...ery.com>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	Russell King <linux@....linux.org.uk>,
	Loic PALLARDY <loic.pallardy@...com>,
	Arnd Bergmann <arnd@...db.de>,
	Janusz Krzysztofik <jkrzyszt@....icnet.pl>,
	Tony Lindgren <tony@...mide.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	"Gutierrez, Juan" <jgutierrez@...com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Felipe Contreras <felipe.contreras@...ia.com>,
	Dom Cobley <popcornmix@...il.com>,
	Wim Van Sebroeck <wim@...ana.be>,
	Omar Ramirez Luna <omar.ramirez@...itl.com>,
	Tejun Heo <tj@...nel.org>, "Anna, Suman" <s-anna@...com>,
	STEricsson_nomadik_linux <STEricsson_nomadik_linux@...t.st.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH 4/9] mailbox: create opened message type

On Tue, Dec 18, 2012 at 18:40:07, Loic Pallardy wrote:
> Current message type is a u32 to fit HW fifo format.
> This should be extended to support any message exchanges
> and type of mailbox.
> Propose structure owns the original u32 and an optional
> pointer on additional data.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@...com>
> ---
>  drivers/mailbox/Kconfig         |  9 ++++++
>  drivers/mailbox/mailbox-omap1.c | 18 ++++++------
>  drivers/mailbox/mailbox-omap2.c | 10 ++++---
>  drivers/mailbox/mailbox.c       | 64 +++++++++++++++++++++++++++++------------
>  drivers/mailbox/mailbox.h       |  6 ++--
>  include/linux/mailbox.h         | 17 ++++++++++-
>  6 files changed, 89 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index d1e7d74..efb766f 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -33,4 +33,13 @@ config MBOX_KFIFO_SIZE
>           This can also be changed at runtime (via the mbox_kfifo_size
>           module parameter).
>  
> +config MBOX_DATA_SIZE
> +	int "Mailbox associated data max size (bytes)"
> +	default 64
> +	help
> +	  Specify the default size of mailbox's associated data buffer
> +	  (bytes)
> +          This can also be changed at runtime (via the mbox_kfifo_size
> +          module parameter).
> +
>  endif
> diff --git a/drivers/mailbox/mailbox-omap1.c b/drivers/mailbox/mailbox-omap1.c
> index 31cb70a..94e90af 100644
> --- a/drivers/mailbox/mailbox-omap1.c
> +++ b/drivers/mailbox/mailbox-omap1.c
> @@ -50,26 +50,26 @@ static inline void mbox_write_reg(u32 val, size_t ofs)
>  }
>  
>  /* msg */
> -static mbox_msg_t omap1_mbox_fifo_read(struct mailbox *mbox)
> +static int omap1_mbox_fifo_read(struct mailbox *mbox, struct mailbox_msg *msg)
>  {
>  	struct omap_mbox1_fifo *fifo =
>  		&((struct omap_mbox1_priv *)mbox->priv)->rx_fifo;
> -	mbox_msg_t msg;
>  
> -	msg = mbox_read_reg(fifo->data);
> -	msg |= ((mbox_msg_t) mbox_read_reg(fifo->cmd)) << 16;
> +	msg->header = mbox_read_reg(fifo->data);
> +	msg->header |= ((mbox_msg_t) mbox_read_reg(fifo->cmd)) << 16;

Now that struct mailbox_msg encapsulates the data, you can
get rid of the mbox_msg_t typedef completely. Having the data
as part of the mailbox_msg along with the functions with mbox_msg_t
as the return type just creates confusion IMHO.
 
>  
> -	return msg;
> +	return 0;
>  }

Convert all return 0 functions to void?

[...]

>  
> diff --git a/include/linux/mailbox.h b/include/linux/mailbox.h
> index 18c9502..d256e1a 100644
> --- a/include/linux/mailbox.h
> +++ b/include/linux/mailbox.h
> @@ -7,7 +7,22 @@ typedef int __bitwise mailbox_irq_t;
>  #define IRQ_TX ((__force mailbox_irq_t) 1)
>  #define IRQ_RX ((__force mailbox_irq_t) 2)
>  
> -int mailbox_msg_send(struct mailbox *, mbox_msg_t msg);
> +struct mailbox_msg {
> +	int		size;
> +	u32		header;
> +	void		*pdata;
> +};
> +
> +#define MAILBOX_FILL_MSG(_msg, _header, _pdata, _size) { \
> +	_msg.header = _header; \
> +	_msg.pdata = (void *)_pdata; \
> +	_msg.size = _size; \
> +}
> +
> +#define MAILBOX_FILL_HEADER_MSG(_msg, _header) \
> +	MAILBOX_FILL_MSG(_msg, _header, NULL, 0);
> +

I used these patches as part of the suspend-resume support for AM335x
which has the same mailbox IP as OMAP4. I used the MAILBOX_FILL_HEADER_MSG
helper and things work as expected.

However, I found the 'header' part to be very confusing. Why not treat the
OMAP case as a special case of the new MAILBOX_FILL_MSG where the data size
is set to 1?

Regards,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/