From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qv1-f50.google.com (mail-qv1-f50.google.com [209.85.219.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 25ECC1C8606 for ; Fri, 14 Feb 2025 20:14:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.219.50 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1739564081; cv=none; b=JrBzZsR1KIJVreO5/MV5BrA0vBucjXzEaF58w2w3LAPjgjoKZ7mfiw+VMdGJMmcf2LEIhG3LdIPFktc5nQYBm3cVW4L+89GMBEJYGxKnGITfRXWS4eHisIGGKW/jHDrm1Pxs55VhlsL3nS4aAguXtthA+/6qmi7NdZW1XYh8OoU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1739564081; c=relaxed/simple; bh=YNsBtJJ9+cy2UshunM7rNUNI3Qb2RchH6uhbFDj9n5E=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=D+mfto67bEfSSHgiPA3UZVjmcLIkOc5Zg8Yk6jGqe0jIuHxGEjOdWNkKoK27q3qmbR/M/pVfFAOn5qxy6djs12L618tQj1NAs6VK+C8Jzj/QTzLweozxNTplInx1/Bdyy6uPRyTjhCY8m9r/pESz/GThztdK0vlHjCQurLST/TY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ziepe.ca; spf=pass smtp.mailfrom=ziepe.ca; dkim=pass (2048-bit key) header.d=ziepe.ca header.i=@ziepe.ca header.b=lDdgc3y7; arc=none smtp.client-ip=209.85.219.50 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ziepe.ca Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ziepe.ca Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=ziepe.ca header.i=@ziepe.ca header.b="lDdgc3y7" Received: by mail-qv1-f50.google.com with SMTP id 6a1803df08f44-6e66d4f3be2so1173416d6.3 for ; Fri, 14 Feb 2025 12:14:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ziepe.ca; s=google; t=1739564077; x=1740168877; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=aitcMe7WROI3inmWPb5j5yTibRGZT3eFja/8dsNPCSo=; b=lDdgc3y71bo8L0V8hpCdEtVgcsYESprTduXIi+kFt0zRyUBJTudrWt7COhlV/BBLji 5ZkMyQmRRCnytLs+RviwSVnWQOUoM5cP3ErSy21ZoSWjTaurrbfAeOLlOz47ThtY1fmC FUlgo2rNZt2vfHG+MFiY/pf6R6M/Ppt/VKABJ4J0S5yCZTzfNBiG/G8XHBO7ymhTkzq4 20ubMy9bEoplUkIsMCUNIc3rJPoNAuW8WmZphPHnp6uSlSve6YYVLYFopk6107RDJFBN 7obB/RiGZMO7NKpkJS8XE23hj8SP8OqV5dlORQbhZ8Cw+0rpdqFpPn+NC5hiIEfe8k0v yuTw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1739564077; x=1740168877; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=aitcMe7WROI3inmWPb5j5yTibRGZT3eFja/8dsNPCSo=; b=hfCCa/FmtMrlxi5mGZ4zWF73gTaeabhgxrqOuUylm31VVXHGbmoZTGG7IW7Yvo5lc4 sc+oKoMDXF7y7mE1vtU4z//cj7rFnz5YI4VUzkQWuvE5o5jznidD2CEtWjnX4ThZXFOd Ep3TCtI7JkrGaLqc2ojhMaYW3m6CtvFpgWHzJFjUJ/l8k7JWnpIrsWRzSvSSGjut9Pgg KBlhV2NACNAwJYefkNJbSmVmbK0JyAstVXy751hihg9dUpORg+Um2Vsv+/cozWfaDUw6 Eo0nkxweXm1eOgEcMXu8bfWRrO8cS+LralCFrAwl22wjCfJ2uHFWD9SUmdfiwjaA1ZzP ZLsQ== X-Forwarded-Encrypted: i=1; AJvYcCVLcglJTKi/7eEupomp6TlOL2uToxYBlK4wGxAPe5+ABeWz9jnFvmI1l2OlcqepmES0RkTaxgaVy+0ba2Q=@vger.kernel.org X-Gm-Message-State: AOJu0YyPk6QvM7MP/zZ3z17+RMpwk4JjTBPwgLtioR6as7luSsJPFDha ECIPX1RcKhdmSlWZhKn3ttODpLfX9TVCbCt6NVUw4eOASIJM+D5khl0qRigImrw= X-Gm-Gg: ASbGncv2D7Y2QvNOjq0EZiwTWp6W/9+UOSrlL33JNxmdHTqVVKTbzwNDPEr1DGGahbp ZMMxfmdqo80lFjqs2QlzGiOuRCPMmEcEVJMRbM4I/Tp67+XaigYokwzt20VXAhAJw3kqmhFOLjI Q2cw8xgA9c5CCMOlbeJDh0ig+q4BzPpk8ShT4NBIr9Z4BSY+D5XiwNU9CX6AkRceNRuNfXpUZGb 8bcSMBwu1N2hPoa96tiGTH8RtnjCxFCdnfRB1yHfk/DWF7wt/RH1jP/tq2r7TAUuSpUN6TKWZoO cFKL2b4uaG18enkliI9qJCriZkD7MoLcf4RBl89OYX0R70ddK4qp0UqM0VGEyzb3 X-Google-Smtp-Source: AGHT+IGnMpUVAQSaoCinCLx2FBIU+4N7ViIjMJqQfxMTZ4fEWvtSzvHCiXUgAsqSsNDrtdOvrjsIMg== X-Received: by 2002:ad4:4eed:0:b0:6d4:e0a:230e with SMTP id 6a1803df08f44-6e66ccc114cmr11243186d6.16.1739564077030; Fri, 14 Feb 2025 12:14:37 -0800 (PST) Received: from ziepe.ca (hlfxns017vw-142-68-128-5.dhcp-dynamic.fibreop.ns.bellaliant.net. [142.68.128.5]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-6e65daffdbbsm24455776d6.99.2025.02.14.12.14.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 14 Feb 2025 12:14:36 -0800 (PST) Received: from jgg by wakko with local (Exim 4.97) (envelope-from ) id 1tj255-0000000GmgQ-3jtW; Fri, 14 Feb 2025 16:14:35 -0400 Date: Fri, 14 Feb 2025 16:14:35 -0400 From: Jason Gunthorpe To: Robin Murphy Cc: Lorenzo Pieralisi , Hanjun Guo , Sudeep Holla , "Rafael J. Wysocki" , Len Brown , Russell King , Greg Kroah-Hartman , Danilo Krummrich , Stuart Yoder , Laurentiu Tudor , Nipun Gupta , Nikhil Agarwal , Joerg Roedel , Will Deacon , Rob Herring , Saravana Kannan , Bjorn Helgaas , linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, iommu@lists.linux.dev, devicetree@vger.kernel.org, linux-pci@vger.kernel.org, Charan Teja Kalla Subject: Re: [PATCH 2/2] iommu: Get DT/ACPI parsing into the proper probe path Message-ID: <20250214201435.GF3696814@ziepe.ca> References: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Thu, Feb 13, 2025 at 11:49:00PM +0000, Robin Murphy wrote: > much just calling the same path twice. At client driver probe time, > dev->driver is obviously set; conversely at device_add(), or a > subsequent bus_iommu_probe(), any device waiting for an IOMMU really Could you put the dev->driver test into iommu_device_use_default_domain()? It looks like many of the cases are just guarding that call. > should *not* have a driver already, so we can use that as a condition to > disambiguate the two cases, and avoid recursing back into the IOMMU core > at the wrong times. Which sounds like this: > + mutex_unlock(&iommu_probe_device_lock); > + dev->bus->dma_configure(dev); > + mutex_lock(&iommu_probe_device_lock); > + } Shouldn't call iommu_device_use_default_domain() ? But... I couldn't guess what the problem with calling it is? In the not-probed case it will see dev->iommu_group is NULL and succeed. The probed case could be prevented by checking dev->iommu_group sooner in __iommu_probe_device()? Anyhow, the approach seems OK > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 9f4efa8f75a6..42b8f1833c3c 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -1619,6 +1619,9 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in) > { > int err; > > + if (device_iommu_mapped(dev)) > + return 0; This is unlocked and outside a driver context, it should have a comment explaining why races with probe can't happen? > + /* > + * For FDT-based systems and ACPI IORT/VIOT, the common firmware parsing > + * is buried in the bus dma_configure path. Properly unpicking that is > + * still a fairly big job, so for now just invoke the whole thing. Our > + * bus_iommu_probe() walk may see devices with drivers already bound, > + * but that must mean they're already configured - either probed by > + * another IOMMU, or there was no IOMMU for iommu_fwspec_init() to wait > + * for - so either way we can safely skip this and avoid worrying about > + * those recursing back here thinking they need a replay call. > + */ > + if (!dev->driver && dev->bus->dma_configure) { > + mutex_unlock(&iommu_probe_device_lock); > + dev->bus->dma_configure(dev); > + mutex_lock(&iommu_probe_device_lock); > + } > + > + /* > + * At this point, either valid devices now have a fwspec, or we can > + * assume that only one of Intel, AMD, s390, PAMU or legacy SMMUv2 can > + * be present, and that any of their registered instances has suitable > + * ops for probing, and thus cheekily co-opt the same mechanism. > + */ > + ops = iommu_fwspec_ops(dev_iommu_fwspec_get(dev)); > + if (!ops) > + return -ENODEV; > + > /* Device is probed already if in a group */ > if (dev->iommu_group) > return 0; This is the test I mean, if iommu_group is set then dev->iommu->iommu_dev->ops is supposed to be valid too. It seems like it should be done earlier.. > + /* > + * And if we do now see any replay calls, they would indicate someone > + * misusing the dma_configure path outside bus code. > + */ > + if (dev_iommu_fwspec_get(dev) && dev->driver) > + dev_WARN(dev, "late IOMMU probe at driver bind, something fishy here!\n"); WARN_ON_ONCE or dump_stack() to get the stack trace out? > @@ -121,6 +121,9 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np, > if (!master_np) > return -ENODEV; > > + if (device_iommu_mapped(dev)) > + return 0; Same note > @@ -151,7 +154,12 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np, > iommu_fwspec_free(dev); > mutex_unlock(&iommu_probe_device_lock); > > - if (!err && dev->bus) > + /* > + * If we have reason to believe the IOMMU driver missed the initial > + * iommu_probe_device() call for dev, try to fix it up. This should > + * no longer happen unless of_dma_configure() is being misused. > + */ > + if (!err && dev->driver) > err = iommu_probe_device(dev); This is being conservative? After some time of nobody complaining it can be removed? Jason