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

[Thierry Reding]

On Thu, Jun 08, 2023 at 12:06:58PM +0300, Peter De Schrijver wrote:
> 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.

That's precisely my point. This only works in conjunction with the
additional enum and it unnecessarily complicates things.

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

I don't think we should extra data to discriminate when we can already
discriminate using the existing data.

Thierry

Attachment: signature.asc
Description: PGP signature