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

[Peter De Schrijver]

On Wed, Jun 07, 2023 at 05:57:39PM +0200, Thierry Reding wrote:
> 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@xxxxxxxxxx>
> > > > ---
> > > >  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

No. You can't the union in combination with the discriminating enum
tells you you should only either sram or dram.

> 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.
> 

I don't think pointers should be used as a discriminator.

> 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.

No. The code should follow the data structures, that's why unions exist.

Peter.