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

[Thierry Reding]

On Thu, Jun 08, 2023 at 02:22:23PM +0300, Peter De Schrijver wrote:
> On Wed, Jun 07, 2023 at 05:57:39PM +0200, Thierry Reding wrote:
> > > 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.
> > 
> 
> There are 4 possible states for these pointers:
> both NULL
> both non-NULL
> sram pointer NULL, dram pointer non-NULL
> dram pointer NULL, sram pointer non-NULL
> 
> So how is this one source of truth?

If you add a tristate enum you turn this into 6 possible states, how is
that any better?

My point is that the pointer contents are enough to determine which mode
we use. In either case we have to make sure that the state is consistent
so we can't end up with both non-NULL. The difference is that without
the enum we only have to make sure that the pointers are correct. With
the additional enum we also need to make sure that that value is
consistent with the values that we store in the pointers.

Anyway, time is running out, so I'll just apply the series and "fix"
this up myself.

Thierry

Attachment: signature.asc
Description: PGP signature