Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 19 Aug 2022 15:26:45 +0100
From:   Jean-Philippe Brucker <jpb@kernel.org>
To:     Saravana Kannan <saravanak@google.com>
Cc:     Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        eric.auger@redhat.com, linux-kernel@vger.kernel.org,
        iommu@lists.linux-foundation.org, kernel-team@android.com
Subject: Re: [PATCH v2 8/9] iommu/of: Delete usage of
 driver_deferred_probe_check_state()
Message-ID: <Yv+dpeIPvde7oDHi@myrica>
References: <20220601070707.3946847-1-saravanak@google.com>
 <20220601070707.3946847-9-saravanak@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220601070707.3946847-9-saravanak@google.com>
Precedence: bulk

Hi Saravana,

On Wed, Jun 01, 2022 at 12:07:04AM -0700, Saravana Kannan via iommu wrote:
> Now that fw_devlink=on and fw_devlink.strict=1 by default and fw_devlink
> supports iommu DT properties, the execution will never get to the point
> where driver_deferred_probe_check_state() is called before the supplier
> has probed successfully or before deferred probe timeout has expired.
> 
> So, delete the call and replace it with -ENODEV.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/iommu/of_iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 5696314ae69e..41f4eb005219 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -40,7 +40,7 @@ static int of_iommu_xlate(struct device *dev,
>  	 * a proper probe-ordering dependency mechanism in future.
>  	 */
>  	if (!ops)
> -		return driver_deferred_probe_check_state(dev);
> +		return -ENODEV;

Unfortunately this breaks virtio-iommu on v6.0-rc1. Could you add it to
your revert series [1]?

Eventually I'd really like to use fw_devlink in order to enable module
support, so I'm looking for a proper fix. Our DT description is peculiar
because the virtio-iommu, when it uses the virtio-pci transport, can
appear on the same PCI bus than the endpoints it manages. So the PCI node
looks like this:

	pcie@10000000 {
		compatible = "pci-host-ecam-generic";
		...
		iommu-map = <0x00 &viommu 0x00 0x20>
		            <0x21 &viommu 0x21 0xffdf>;

		viommu: virtio_iommu@32 {
			#iommu-cells = <0x01>;
			reg = <0x2000 0x00 0x00 0x00 0x00>;
			compatible = "virtio,pci-iommu";
		};
	};

The iommu-map property points to the IOMMU sub-node for all endpoints
except the IOMMU itself (device 0x20 - 00:04.0). fw_devlink currently
rejects this:

  [    0.270150] OF: Not linking pcie@10000000 to virtio_iommu@32 - is descendant
  [    0.270324] OF: Not linking pcie@10000000 to virtio_iommu@32 - is descendant

I don't know yet if this kind of topology could ever work with fw_devlink,
but I thought I'd notify you before digging further.

Thanks,
Jean

[1] https://lore.kernel.org/all/20220727185012.3255200-1-saravanak@google.com/#t

>  
>  	if (!try_module_get(ops->owner))
>  		return -ENODEV;
> -- 
> 2.36.1.255.ge46751e96f-goog
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu