Re: [PATCH v3 04/12] firmware: tegra: Add IVC library

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5188 bytes --]

On Mon, Aug 22, 2016 at 11:46:49AM +0100, Jon Hunter wrote:
> 
> On 19/08/16 18:32, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > The Inter-VM communication (IVC) is a communication protocol which is
> > designed for interprocessor communication (IPC) or the communication
> > between the hypervisor and the virtual machine with a guest OS.
> > 
> > Message channels are used to communicate between processors. They are
> > backed by DRAM or SRAM, so care must be taken to maintain coherence of
> > data.
> > 
> > The IVC library maintains memory-based descriptors for the transmission
> > and reception channels as well as the data coherence of the counter and
> > payload. Clients, such as the driver for the BPMP firmware, can use the
> > library to exchange messages with remote processors.
> > 
> > Based on work by Peter Newman <pnewman-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> and Joseph Lo
> > <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>.
> > 
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> > Changes in v3:
> > - use a more object oriented design
> > 
> >  drivers/firmware/Kconfig        |   1 +
> >  drivers/firmware/Makefile       |   1 +
> >  drivers/firmware/tegra/Kconfig  |  13 +
> >  drivers/firmware/tegra/Makefile |   1 +
> >  drivers/firmware/tegra/ivc.c    | 683 ++++++++++++++++++++++++++++++++++++++++
> >  include/soc/tegra/ivc.h         | 109 +++++++
> >  6 files changed, 808 insertions(+)
> >  create mode 100644 drivers/firmware/tegra/Kconfig
> >  create mode 100644 drivers/firmware/tegra/Makefile
> >  create mode 100644 drivers/firmware/tegra/ivc.c
> >  create mode 100644 include/soc/tegra/ivc.h
> 
> [snip]
> 
> > +static void *tegra_ivc_frame_virt(struct tegra_ivc *ivc,
> > +				  struct tegra_ivc_header *header,
> > +				  unsigned int frame)
> > +{
> > +	BUG_ON(frame >= ivc->num_frames);
> 
> WARN_ON and return an error pointer?

I think I'll actually drop these. Or move them one layer up. The only
parameters passed into as frame are ivc->{rx,tx}.position, and all the
code that modifies these will properly wrap them at ivc->num_frames. I
think the only way that this condition could become true is if someone
were to directly access the structure and modify the position. That's
technically possible, so I guess the checks could stay in for extra
paranoia.

> > +
> > +	return (void *)(header + 1) + ivc->frame_size * frame;
> > +}
> > +
> > +static inline dma_addr_t tegra_ivc_frame_phys(struct tegra_ivc *ivc,
> > +					      dma_addr_t phys,
> > +					      unsigned int frame)
> > +{
> > +	unsigned long offset;
> > +
> > +	BUG_ON(!ivc->peer);
> > +	BUG_ON(frame >= ivc->num_frames);
> 
> WARN_ON?

I've moved this up one layer since it's a little cumbersome to return an
error via dma_addr_t and the !ivc->peer check is present in all callers
of this function anyway.

> > +	offset = sizeof(struct tegra_ivc_header) + ivc->frame_size * frame;
> > +
> > +	return phys + offset;
> > +}
> 
> [snip]
> 
> > +static int check_ivc_params(unsigned long base1, unsigned long base2,
> > +			    unsigned int num_frames, size_t frame_size)
> > +{
> > +	BUG_ON(offsetof(struct tegra_ivc_header, tx.count) & (TEGRA_IVC_ALIGN - 1));
> > +	BUG_ON(offsetof(struct tegra_ivc_header, rx.count) & (TEGRA_IVC_ALIGN - 1));
> > +	BUG_ON(sizeof(struct tegra_ivc_header) & (TEGRA_IVC_ALIGN - 1));
> 
> WARN_ON?

I've turned all of these into BUILD_BUG_ON() because the parameters are
all statically known at build time. I've also switched to the IS_ALIGNED
macro here and the checks below because it's easier to read.

> > +int tegra_ivc_init(struct tegra_ivc *ivc, struct device *peer,
> > +		   void __iomem *rx_virt, dma_addr_t rx_phys,
> > +		   void __iomem *tx_virt, dma_addr_t tx_phys,
> > +		   unsigned int num_frames, size_t frame_size,
> > +		   void (*notify)(struct tegra_ivc *ivc, void *data),
> > +		   void *data)
> > +{
> > +	size_t queue_size;
> > +	int err;
> > +
> > +	err = check_ivc_params((unsigned long)rx_virt, (unsigned long)tx_virt,
> > +			       num_frames, frame_size);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	BUG_ON(!ivc);
> > +	BUG_ON(!notify);
> 
> We should check this first and just return -EINVAL.

Yes, done. I've wrapped these in a single WARN_ON() with a -EINVAL
return.

> > +/**
> > + * tegra_ivc_read_get_next_frame - Peek at the next frame to receive
> > + * @ivc		pointer of the IVC channel
> > + *
> > + * Peek at the next frame to be received, without removing it from
> > + * the queue.
> > + *
> > + * Returns a pointer to the frame, or an error encoded pointer.
> > + */
> > +void *tegra_ivc_read_get_next_frame(struct tegra_ivc *ivc);
> 
> Is it odd to return a void * pointer here and not a pointer to a
> specific structure type?

I think that's by design. IVC is a generic library to implement an IPC
mechanism on top. There is no specific structure to return a pointer to
here. The caller determines what type it wants to put into frames.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]