Received-SPF: pass (google.com: domain of linux-kernel+bounces-23729-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 11 Jan 2024 10:48:40 -0400
From: Jason Gunthorpe <jgg@ziepe.ca>
To: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Joerg Roedel <joro@8bytes.org>,
	Will Deacon <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>,
	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: <20240111144840.GW50608@ziepe.ca>
References: <20240111073213.180020-1-baolu.lu@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240111073213.180020-1-baolu.lu@linux.intel.com>

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.

The fundamental issue is we now have a special kind of driver:

	fwspec = dev_iommu_fwspec_get(dev);
	if (fwspec && fwspec->ops)
		ops = fwspec->ops;
	else
		ops = iommu_ops_from_fwnode(NULL);
                                           ^^^^^^^^

Which represents a "global" non-fwspec using driver that will only
bind to devices that didn't parse into a fwspec.

The code above supports only one of these drivers at time, but allows
more than one to be registered - it is inconsistent.

I think the right/easy answer is to iterate over all the "global"
drivers and call their probe instead of just the first one.

Especially since my approach over here migrates the whole thing to work
by iterating:

https://lore.kernel.org/all/0-v2-f82a05539a64+5109-iommu_fwspec_p2_jgg@nvidia.com/

And this patch:

https://lore.kernel.org/all/28-v2-f82a05539a64+5109-iommu_fwspec_p2_jgg@nvidia.com/

Is how I made the iterating logic, it could be pulled out and tidied a
bit.

Jason