On Tue, Feb 13, 2024 at 08:53:03AM -0400, Jason Gunthorpe wrote:
> On Tue, Feb 13, 2024 at 12:19:34PM +0000, Will Deacon wrote:
> > > > @@ -875,15 +879,6 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
> > > > mutex_init(&smmu_domain->init_mutex);
> > > > spin_lock_init(&smmu_domain->cb_lock);
> > > >
> > > > - if (dev) {
> > > > - struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
> > > > -
> > > > - if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) {
> > > > - kfree(smmu_domain);
> > > > - return NULL;
> > > > - }
> > > > - }
> > > > -
> > > > return &smmu_domain->domain;
> > > > }
> > >
> > > Everything else is fine, you already tested with that arrangement.
> >
> > Partial reverts are a recipe for confusion, so I'll take this and if you'd
> > like to bring back some of the hunks, please can you send a patch on top
> > that does that?
>
> The typical kernel standard is to fix bugs in patches and only reach
> for a wholesale revert if the community is struggling with bug
> fixing. Dmitry already tested removing that hunk, Robin explained the
> issue, we understand the bug fix is to remove the
> arm_smmu_init_domain_context() call. Nothing justifies a full scale
> revert.
I can't say I'm aware of any consensus for how to handle this, to be
completely honest with you. I just personally see a lot of benefit in
reverting to a known-good state, especially when the revert has been
posted by the bug reporter. Then we can add stuff on top of that known
good state without having to worry about any other problems that we're
yet to uncover. Doesn't really sound controversial...
> I'll send another patch if you want, but it seems like a waste of all
> our time.
It's a bug fix, of course it's a waste of time! We're talking minutes
though, right?
Will
