Re: [patch-4.16] arm/smmuv1,v2: Protect smmu master list with a lock

[Julien Grall <julien@xxxxxxx>]

On 04/11/2021 09:18, Michal Orzel wrote:
> Hello,
Hi Michal,

> On 01.11.2021 21:51, Stefano Stabellini wrote:
> > On Mon, 1 Nov 2021, Ian Jackson wrote:
> > > Stefano Stabellini writes ("Re: [patch-4.16] arm/smmuv1,v2: Protect smmu master list 
> > > with a lock"):
> > > > In regards to this specific patch and also the conversation about 4.16
> > > > or 4.17: I think it would be fine to take this patch in 4.16 in its
> > > > current form. Although it is not required because PCI passthrough is
> > > > not going to be complete in 4.16 anyway, I like that this patch makes
> > > > the code consistent in terms of protection of rbtree accesses.  With
> > > > this patch the arm_smmu_master rbtree is consistently protected from
> > > > concurrent accesses. Without this patch, it is sometimes protected and
> > > > sometimes not, which is not great.
> > > It sounds like this is a possible latent bug, or at least a bad state
> > > of the code that might lead to the introduction of bad bugs later.
> > >
> > > So I think I understand the upside.
> > >
> > > > So I think that is something that could be good to have in 4.16. But
> > > > like you said, the patch is not strictly required so it is fine either
> > > > way.
> > > Can you set out the downside for me too ?  What are the risks ?  How
> > > are the affected code paths used in 4.16 ?
> > >
> > > A good way to think about this is: if taking this patch for 4.16
> > > causes problems, what would that look like ?
> > The patch affects the SMMU code paths that are currently in-use for
> > non-PCI devices which are currently supported. A bug in this patch could
> > cause a failure to setup the SMMU for one or more devices. I would
> > imagine that it would manifest probably as either an error or an hang
> > (given that it is adding spin locks) early at boot when the SMMU is
> > configured.
> >
> > The validation of this patch would mostly happen by review: it is the
> > kind of patch that changes some "return -1" into "goto err".
> In order not to leave this patch high and dry:
> I can see that Stefano and Bertrand are in favor of this patch and
> Julien is rather against. Therefore I wanted to ask what are we doing with this 
> patch.
My main objection is on the process. We should not merge patch that 
doesn't fix a real issue at this stage of this release.
That said, the patch is low risk and both Stefano/Bertrand are for it. So...

> Do we want it for 4.16?
... Ian can we get your release-acked-by?

Cheers,

--
Julien Grall