Re: [PATCH v4 6/6] firmware: tegra: bpmp: Add support for DRAM MRQ GSCs

[Thierry Reding <thierry.reding@gmail.com>]

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

On Tue, May 16, 2023 at 12:55:03PM +0300, Peter De Schrijver wrote:
> On Tue, May 16, 2023 at 11:35:24AM +0200, Thierry Reding wrote:
> > On Thu, May 11, 2023 at 04:20:51PM +0300, Peter De Schrijver wrote:
> > > Implement support for DRAM MRQ GSCs.
> > > 
> > > Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> > > ---
> > >  drivers/firmware/tegra/bpmp-tegra186.c | 232 ++++++++++++++++++-------
> > >  drivers/firmware/tegra/bpmp.c          |   4 +-
> > >  2 files changed, 168 insertions(+), 68 deletions(-)
> > > 
> > > diff --git a/drivers/firmware/tegra/bpmp-tegra186.c b/drivers/firmware/tegra/bpmp-tegra186.c
> > > index 2e26199041cd..74575c9f0014 100644
> > > --- a/drivers/firmware/tegra/bpmp-tegra186.c
> > > +++ b/drivers/firmware/tegra/bpmp-tegra186.c
> > > @@ -4,7 +4,9 @@
> > >   */
> > >  
> > >  #include <linux/genalloc.h>
> > > +#include <linux/io.h>
> > >  #include <linux/mailbox_client.h>
> > > +#include <linux/of_address.h>
> > >  #include <linux/platform_device.h>
> > >  
> > >  #include <soc/tegra/bpmp.h>
> > > @@ -13,12 +15,21 @@
> > >  
> > >  #include "bpmp-private.h"
> > >  
> > > +enum tegra_bpmp_mem_type { TEGRA_INVALID, TEGRA_SRAM, TEGRA_DRAM };
> > 
> > Still not convinced about this one.
> > 
> > > +
> > >  struct tegra186_bpmp {
> > >  	struct tegra_bpmp *parent;
> > >  
> > >  	struct {
> > > -		struct gen_pool *pool;
> > > -		void __iomem *virt;
> > > +		union {
> > > +			struct {
> > > +				void __iomem *virt;
> > > +				struct gen_pool *pool;
> > > +			} sram;
> > > +			struct {
> > > +				void *virt;
> > > +			} dram;
> > > +		};
> > 
> > The drawback of these unions is that they can lead to ambiguity, so you
> > need the tegra_bpmp_mem_type enum to differentiate between the two.
> > 
> 
> No, on the contrary, now it's clear you can either have void __iomem *
> and struct gen_pool * or void *virt but not both.

No, it's not clear. You can have one part of your driver write the
sram.virt field and another read dram.virt and they'll end up pointing
at the same memory location but with different meaning. That's why you
need to introduce the enumeration in order to specify which one of the
two you want to pick.

And that's exactly where you start introducing the potential for
inconsistency: now you need to be extra careful that the enumeration and
the unions are set correctly. You effectively have two sources of truth
and they don't necessarily match. You can also end up (at least
theoretically) with the invalid value, so you need an extra check for
that too.

You can avoid all of those inconsistencies if you reduce this to one
source of truth, namely the pointers that you're going to use.

Your variant would be slightly better if you omitted the invalid value
because then you could still have an internal inconsistency, but the
likelihood is much reduced.

> > If you change this to something like:
> > 
> > 	struct {
> > 		struct gen_pool *pool;
> > 		void __iomem *sram;
> > 		void *dram;
> > 		dma_addr_t phys;
> > 	} tx, rx;
> > 
> > you eliminate all ambiguity because you can either have pool and sram
> > set, or you can have dram set, and depending on which are set you know
> > which type of memory you're dealing with.
> > 
> 
> No. You just add ambiguity. It's not clear from looking at the data
> structure which fields are valid for which case.

That's easily fixed by adding comments explaining what you use them for.
But the code should make that pretty clear already.

Thierry

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