From: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> To: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> Cc: Timo Alho <talho-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, Peter De Schrijver <pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, Sivaram Nair <sivaramn-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: Re: [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-DDmLM1+adcrQT0dZR+AlfA@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 --]
WARNING: multiple messages have this Message-ID (diff)
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: 104+ 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 ` Thierry Reding [not found] ` <20160819173233.13260-1-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-08-19 17:32 ` [PATCH v3 01/12] dt-bindings: mailbox: Add Tegra HSP binding Thierry Reding 2016-08-19 17:32 ` Thierry Reding 2016-08-19 17:32 ` [PATCH v3 02/12] mailbox: Add Tegra HSP driver Thierry Reding 2016-08-19 17:32 ` Thierry Reding 2016-08-22 13:43 ` Arnd Bergmann 2016-08-22 13:43 ` Arnd Bergmann 2016-08-22 14:17 ` Thierry Reding 2016-08-22 14:17 ` Thierry Reding [not found] ` <20160822141728.GF17367-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org> 2016-08-22 16:42 ` Stephen Warren 2016-08-22 16:42 ` Stephen Warren [not found] ` <20160819173233.13260-3-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-08-22 16:53 ` Stephen Warren 2016-08-22 16:53 ` Stephen Warren 2016-08-23 0:06 ` Sivaram Nair 2016-08-23 0:06 ` Sivaram Nair 2016-08-23 0:12 ` 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 ` Thierry Reding 2016-08-19 17:32 ` [PATCH v3 04/12] firmware: tegra: Add IVC library Thierry Reding 2016-08-19 17:32 ` Thierry Reding [not found] ` <20160819173233.13260-5-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-08-22 10:46 ` Jon Hunter 2016-08-22 10:46 ` Jon Hunter [not found] ` <90222c3a-7c69-6fa3-d161-4ed0c5759f34-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2016-08-22 12:40 ` Thierry Reding [this message] 2016-08-22 12:40 ` Thierry Reding 2016-08-22 18:49 ` Stephen Warren 2016-08-22 18:49 ` Stephen Warren 2016-08-24 15:13 ` Jon Hunter 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-19 17:32 ` Thierry Reding [not found] ` <20160819173233.13260-6-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-08-22 9:26 ` Jon Hunter 2016-08-22 9:26 ` Jon Hunter [not found] ` <94227d94-1d60-fda7-731b-26656633d585-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2016-08-22 12:54 ` Thierry Reding 2016-08-22 12:54 ` Thierry Reding [not found] ` <20160822125458.GC17367-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org> 2016-08-22 14:24 ` Jon Hunter 2016-08-22 14:24 ` Jon Hunter [not found] ` <6bb4d32f-4f13-285e-430e-672f375a9a46-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2016-08-22 15:00 ` Thierry Reding 2016-08-22 15:00 ` Thierry Reding 2016-08-22 18:51 ` Stephen Warren 2016-08-22 18:51 ` Stephen Warren 2016-08-22 13:34 ` Arnd Bergmann 2016-08-22 13:34 ` Arnd Bergmann 2016-08-22 14:02 ` Thierry Reding 2016-08-22 14:02 ` Thierry Reding [not found] ` <20160822140211.GE17367-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org> 2016-08-22 14:42 ` Arnd Bergmann 2016-08-22 14:42 ` Arnd Bergmann 2016-08-22 15:32 ` Thierry Reding 2016-08-22 15:32 ` Thierry Reding [not found] ` <20160822153258.GB21012-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org> 2016-08-22 15:43 ` Arnd Bergmann 2016-08-22 15:43 ` Arnd Bergmann 2016-08-22 18:56 ` Stephen Warren 2016-08-22 18:56 ` Stephen Warren 2016-08-23 14:58 ` Arnd Bergmann 2016-08-23 14:58 ` Arnd Bergmann 2016-08-23 23:26 ` Sivaram Nair 2016-08-23 23:26 ` Sivaram Nair 2016-08-22 22:23 ` Stephen Warren 2016-08-22 22:23 ` Stephen Warren 2016-08-19 17:32 ` [PATCH v3 06/12] soc/tegra: Add Tegra186 support Thierry Reding 2016-08-19 17:32 ` Thierry Reding [not found] ` <20160819173233.13260-7-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-08-22 19:01 ` Stephen Warren 2016-08-22 19:01 ` Stephen Warren 2016-08-23 13:44 ` Jon Hunter 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-19 17:32 ` Thierry Reding [not found] ` <20160819173233.13260-8-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-08-22 19:01 ` Stephen Warren 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-19 17:32 ` Thierry Reding [not found] ` <20160819173233.13260-9-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-08-22 17:11 ` Stephen Warren 2016-08-22 17:11 ` Stephen Warren 2016-08-22 19:07 ` 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-19 17:32 ` Thierry Reding [not found] ` <20160819173233.13260-10-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-08-22 19:08 ` Stephen Warren 2016-08-22 19:08 ` Stephen Warren 2016-08-23 17:35 ` Jon Hunter 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-19 17:32 ` Thierry Reding [not found] ` <20160819173233.13260-11-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-08-22 19:11 ` Stephen Warren 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-19 17:32 ` Thierry Reding [not found] ` <20160819173233.13260-12-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-08-22 10:11 ` Jon Hunter 2016-08-22 10:11 ` Jon Hunter [not found] ` <0d7080bc-9e82-75dd-7169-0a5b7429801e-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2016-08-22 13:28 ` Thierry Reding 2016-08-22 13:28 ` Thierry Reding [not found] ` <20160822132833.GD17367-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org> 2016-08-23 13:49 ` Jon Hunter 2016-08-23 13:49 ` Jon Hunter 2016-08-22 19:47 ` Stephen Warren 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-19 17:32 ` Thierry Reding [not found] ` <20160819173233.13260-13-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-08-22 19:56 ` Stephen Warren 2016-08-22 19:56 ` Stephen Warren 2016-11-26 13:39 ` [PATCH v3 00/12] Initial Tegra186 support Pavel Machek 2016-11-26 13:39 ` Pavel Machek [not found] ` <20161126133927.GE20568-5NIqAleC692hcjWhqY66xCZi+YwRKgec@public.gmane.org> 2016-11-28 7:33 ` Thierry Reding 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-re5jqeeqqe8avxtiumwx3w@public.gmane.org \ --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org \ --cc=josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org \ --cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \ --cc=linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org \ --cc=sivaramn-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org \ --cc=talho-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.