Received-SPF: pass (google.com: domain of linux-kernel+bounces-23821-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 11 Jan 2024 11:56:48 -0400
From: Jason Gunthorpe <jgg@ziepe.ca>
To: Robin Murphy <robin.murphy@arm.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>, Kevin Tian <kevin.tian@intel.com>,
	Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
	iommu@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] iommufd/selftest: Use right iommu_ops for mock device
Message-ID: <20240111155648.GX50608@ziepe.ca>
References: <20240111073213.180020-1-baolu.lu@linux.intel.com>
 <20240111144840.GW50608@ziepe.ca>
 <016e052e-8a99-4d60-b864-87c9859953f2@arm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <016e052e-8a99-4d60-b864-87c9859953f2@arm.com>

On Thu, Jan 11, 2024 at 03:50:51PM +0000, Robin Murphy wrote:
> On 11/01/2024 2:48 pm, Jason Gunthorpe wrote:
> > On Thu, Jan 11, 2024 at 03:32:13PM +0800, Lu Baolu wrote:
> > > In the iommu probe device path, __iommu_probe_device() gets the iommu_ops
> > > for the device from dev->iommu->fwspec if this field has been initialized
> > > before probing. Otherwise, it will lookup the global iommu device list
> > > and use the iommu_ops of the first iommu device which has no
> > > dev->iommu->fwspec. This causes the wrong iommu_ops to be used for the mock
> > > device on x86 platforms where dev->iommu->fwspec is not used.
> > > 
> > > Preallocate the fwspec for the mock device so that the right iommu ops can
> > > be used.
> > 
> > I really don't like this.
> > 
> > The lifecycle model for fwspec is already a bit confusing. Introducing
> > a new case where a driver pre-allocates the fwspec is making it worse,
> > not better.
> > 
> > eg iommu_init_device() error unwind will free this allocated fwspec
> > leaving the device broken. We don't have the concept of a fwspec that
> > is owned by the device, it is really owned by the probing code.
> 
> As I've tried to explain before, this is in fact the correct use of fwspec
> as originally designed, i.e. being set up by *bus code* before device_add()
> (remember this is not the "IOMMU driver" part of selftest.c).

I understand it was the intention, but it doesn't relaly match how the
code works today..

> Indeed for perfect symmetry the bus code would free the fwspec after the
> corresponding device_del() returns, but there's no harm in that being
> factored into iommu_release_device() since the notifier call occurs
> sufficiently late in device_del() itself as to make no practical difference.

IIRC there were issues with leaking the dev_iommu :(

> I'm working to get things back to that model (wherein the dev_iommu and
> fwspec lifecycles become trivial), just with the slight tweak that these
> days it's going to make more sense to have the initialisation factored into
> device_add() itself (via iommu_probe_device()), rather than beforehand.

I would prefer to simply remove fwspec as I've already shown patches
for. You should give some comment on them.

My main complaint is there is no full vision to remove the 'global
drivers', we will always have some drivers doing FW parsing in probe
and then this different fwspec thing on the side for other drivers.

Jason