From: thierry.reding@gmail•com (Thierry Reding)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v3 04/12] firmware: tegra: Add IVC library
Date: Mon, 22 Aug 2016 14:40:34 +0200 [thread overview]
Message-ID: <20160822124034.GA17367@ulmo.ba.sec> (raw)
In-Reply-To: <90222c3a-7c69-6fa3-d161-4ed0c5759f34@nvidia.com>
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@nvidia•com>
> >
> > 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@nvidia•com> and Joseph Lo
> > <josephl@nvidia•com>.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia•com>
> > ---
> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160822/663fe482/attachment-0001.sig>
next prev parent reply other threads:[~2016-08-22 12:40 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-19 17:32 [PATCH v3 00/12] Initial Tegra186 support Thierry Reding
2016-08-19 17:32 ` [PATCH v3 01/12] dt-bindings: mailbox: Add Tegra HSP binding Thierry Reding
2016-08-19 17:32 ` [PATCH v3 02/12] mailbox: Add Tegra HSP driver Thierry Reding
2016-08-22 13:43 ` Arnd Bergmann
2016-08-22 14:17 ` Thierry Reding
2016-08-22 16:42 ` Stephen Warren
2016-08-22 16:53 ` Stephen Warren
2016-08-23 0:06 ` Sivaram Nair
2016-08-23 0:12 ` Sivaram Nair
2016-08-19 17:32 ` [PATCH v3 03/12] dt-bindings: firmware: Add bindings for Tegra BPMP Thierry Reding
2016-08-19 17:32 ` [PATCH v3 04/12] firmware: tegra: Add IVC library Thierry Reding
2016-08-22 10:46 ` Jon Hunter
2016-08-22 12:40 ` Thierry Reding [this message]
2016-08-22 18:49 ` Stephen Warren
2016-08-24 15:13 ` Jon Hunter
2016-08-19 17:32 ` [PATCH v3 05/12] firmware: tegra: Add BPMP support Thierry Reding
2016-08-22 9:26 ` Jon Hunter
2016-08-22 12:54 ` Thierry Reding
2016-08-22 14:24 ` Jon Hunter
2016-08-22 15:00 ` Thierry Reding
2016-08-22 18:51 ` Stephen Warren
2016-08-22 13:34 ` Arnd Bergmann
2016-08-22 14:02 ` Thierry Reding
2016-08-22 14:42 ` Arnd Bergmann
2016-08-22 15:32 ` Thierry Reding
2016-08-22 15:43 ` Arnd Bergmann
2016-08-22 18:56 ` Stephen Warren
2016-08-23 14:58 ` Arnd Bergmann
2016-08-22 22:23 ` Stephen Warren
2016-08-23 23:26 ` Sivaram Nair
2016-08-19 17:32 ` [PATCH v3 06/12] soc/tegra: Add Tegra186 support Thierry Reding
2016-08-22 19:01 ` Stephen Warren
2016-08-23 13:44 ` Jon Hunter
2016-08-19 17:32 ` [PATCH v3 07/12] arm64: defconfig: Enable Tegra186 SoC Thierry Reding
2016-08-22 19:01 ` Stephen Warren
2016-08-19 17:32 ` [PATCH v3 08/12] arm64: dts: tegra: Add Tegra186 support Thierry Reding
2016-08-22 17:11 ` Stephen Warren
2016-08-22 19:07 ` Stephen Warren
2016-08-19 17:32 ` [PATCH v3 09/12] arm64: dts: tegra: Add NVIDIA P3310 main board support Thierry Reding
2016-08-22 19:08 ` Stephen Warren
2016-08-23 17:35 ` Jon Hunter
2016-08-19 17:32 ` [PATCH v3 10/12] arm64: dts: tegra: Add NVIDIA P2771 " Thierry Reding
2016-08-22 19:11 ` Stephen Warren
2016-08-19 17:32 ` [PATCH v3 11/12] clk: tegra: Add BPMP clock driver Thierry Reding
2016-08-22 10:11 ` Jon Hunter
2016-08-22 13:28 ` Thierry Reding
2016-08-23 13:49 ` Jon Hunter
2016-08-22 19:47 ` Stephen Warren
2016-08-19 17:32 ` [PATCH v3 12/12] reset: Add Tegra BPMP reset driver Thierry Reding
2016-08-22 19:56 ` Stephen Warren
2016-11-26 13:39 ` [PATCH v3 00/12] Initial Tegra186 support Pavel Machek
2016-11-28 7:33 ` Thierry Reding
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160822124034.GA17367@ulmo.ba.sec \
--to=thierry.reding@gmail$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox