Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
From: Robin Murphy <robin.murphy@arm.com>
To: Will Deacon <will@kernel.org>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc: linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-acpi@vger.kernel.org, devicetree@vger.kernel.org,
	iommu@lists.linux-foundation.org, joro@8bytes.org,
	robh+dt@kernel.org, mark.rutland@arm.com,
	lorenzo.pieralisi@arm.com, guohanjun@huawei.com,
	sudeep.holla@arm.com, rjw@rjwysocki.net, lenb@kernel.org,
	bhelgaas@google.com, eric.auger@redhat.com,
	jonathan.cameron@huawei.com, zhangfei.gao@linaro.org
Subject: Re: [PATCH v4 11/13] iommu/arm-smmu-v3: Improve add_device() error handling
Date: Wed, 15 Jan 2020 17:44:57 +0000
Message-ID: <5287c59f-0331-4d2e-e8a0-292bf27683fb@arm.com> (raw)
In-Reply-To: <20200114152538.GB2579@willie-the-truck>

On 14/01/2020 3:25 pm, Will Deacon wrote:
> On Thu, Dec 19, 2019 at 05:30:31PM +0100, Jean-Philippe Brucker wrote:
>> Let add_device() clean up after itself. The iommu_bus_init() function
>> does call remove_device() on error, but other sites (e.g. of_iommu) do
>> not.
>>
>> Don't free level-2 stream tables because we'd have to track if we
>> allocated each of them or if they are used by other endpoints. It's not
>> worth the hassle since they are managed resources.
>>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>> ---
>>   drivers/iommu/arm-smmu-v3.c | 28 +++++++++++++++++++++-------
>>   1 file changed, 21 insertions(+), 7 deletions(-)
> 
> I think this is alright, with one caveat relating to:
> 
> 
> 	/*
> 	 * We _can_ actually withstand dodgy bus code re-calling add_device()
> 	 * without an intervening remove_device()/of_xlate() sequence, but
> 	 * we're not going to do so quietly...
> 	 */
> 	if (WARN_ON_ONCE(fwspec->iommu_priv)) {
> 		master = fwspec->iommu_priv;
> 		smmu = master->smmu;
> 	} ...
> 
> 
> which may be on shakey ground if the subsequent add_device() call can fail
> and free stuff that the first one allocated. At least, I don't know what
> we're trying to support with this, so it's hard to tell whether or not it
> still works as intended after your change.

Hmm, if add_device() ever did fail it should really be expected to 
return the device back to an un-added state, so I don't believe that 
particular concern should be significant regardless...
> How is this supposed to work? I don't recall ever seeing that WARN fire,
> so can we just remove this and bail instead? Robin?

However, I am inclined to agree that it's probably better to make it all 
moot. Although it indeed should never happen, ISTR at the time there 
appeared to be some possible path somewhere by which the notifier may 
have been triggered a second time - possibly if some other device failed 
or deferred after the first call, triggering the bus code to start all 
over again. Since then, though, we've made a lot of changes to how 
->add_device usually gets called, plus stuff like the 
iommu_device_link() call has snuck in that might not stand up to a 
replay anyway, so I don't see any problem with making this condition a 
hard failure. It's certainly much easier to reason about.

In fact, there will already be a WARN from iommu_probe_device() now 
(because the first call will have set the group), so I don't think we 
need any additional diagnostic in the driver any more.

Robin.

> Something like below before your changes...
> 
> Will
> 
> --->8
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index effe72eb89e7..6ae3df2f3495 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2534,28 +2534,23 @@ static int arm_smmu_add_device(struct device *dev)
>   
>   	if (!fwspec || fwspec->ops != &arm_smmu_ops)
>   		return -ENODEV;
> -	/*
> -	 * We _can_ actually withstand dodgy bus code re-calling add_device()
> -	 * without an intervening remove_device()/of_xlate() sequence, but
> -	 * we're not going to do so quietly...
> -	 */
> -	if (WARN_ON_ONCE(fwspec->iommu_priv)) {
> -		master = fwspec->iommu_priv;
> -		smmu = master->smmu;
> -	} else {
> -		smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> -		if (!smmu)
> -			return -ENODEV;
> -		master = kzalloc(sizeof(*master), GFP_KERNEL);
> -		if (!master)
> -			return -ENOMEM;
>   
> -		master->dev = dev;
> -		master->smmu = smmu;
> -		master->sids = fwspec->ids;
> -		master->num_sids = fwspec->num_ids;
> -		fwspec->iommu_priv = master;
> -	}
> +	if (WARN_ON_ONCE(fwspec->iommu_priv))
> +		return -EBUSY;
> +
> +	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> +	if (!smmu)
> +		return -ENODEV;
> +
> +	master = kzalloc(sizeof(*master), GFP_KERNEL);
> +	if (!master)
> +		return -ENOMEM;
> +
> +	master->dev = dev;
> +	master->smmu = smmu;
> +	master->sids = fwspec->ids;
> +	master->num_sids = fwspec->num_ids;
> +	fwspec->iommu_priv = master;
>   
>   	/* Check the SIDs are in range of the SMMU and our stream table */
>   	for (i = 0; i < master->num_sids; i++) {
> 

  parent reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-19 16:30 [PATCH v4 00/13] iommu: Add PASID support to Arm SMMUv3 Jean-Philippe Brucker
2019-12-19 16:30 ` [PATCH v4 01/13] iommu/arm-smmu-v3: Drop __GFP_ZERO flag from DMA allocation Jean-Philippe Brucker
2019-12-19 16:30 ` [PATCH v4 02/13] dt-bindings: document PASID property for IOMMU masters Jean-Philippe Brucker
2019-12-19 16:30 ` [PATCH v4 03/13] iommu/arm-smmu-v3: Parse PASID devicetree property of platform devices Jean-Philippe Brucker
2019-12-19 16:30 ` [PATCH v4 04/13] ACPI/IORT: Parse SSID property of named component node Jean-Philippe Brucker
2019-12-19 16:30 ` [PATCH v4 05/13] iommu/arm-smmu-v3: Prepare arm_smmu_s1_cfg for SSID support Jean-Philippe Brucker
2019-12-19 16:30 ` [PATCH v4 06/13] iommu/arm-smmu-v3: Add context descriptor tables allocators Jean-Philippe Brucker
2020-01-14 11:06   ` Will Deacon
2020-01-14 11:52     ` Jean-Philippe Brucker
2020-01-14 11:56       ` Will Deacon
2019-12-19 16:30 ` [PATCH v4 07/13] iommu/arm-smmu-v3: Add support for Substream IDs Jean-Philippe Brucker
2020-01-14 12:38   ` Will Deacon
2020-01-14 16:30     ` Jean-Philippe Brucker
2019-12-19 16:30 ` [PATCH v4 08/13] iommu/arm-smmu-v3: Propagate ssid_bits Jean-Philippe Brucker
2019-12-19 16:30 ` [PATCH v4 09/13] iommu/arm-smmu-v3: Prepare for handling arm_smmu_write_ctx_desc() failure Jean-Philippe Brucker
2020-01-14 12:42   ` Will Deacon
2020-01-14 16:31     ` Jean-Philippe Brucker
2019-12-19 16:30 ` [PATCH v4 10/13] iommu/arm-smmu-v3: Add second level of context descriptor table Jean-Philippe Brucker
2019-12-20  7:37   ` Auger Eric
2020-01-14 15:04   ` Will Deacon
2020-01-15  9:45     ` Jean-Philippe Brucker
2019-12-19 16:30 ` [PATCH v4 11/13] iommu/arm-smmu-v3: Improve add_device() error handling Jean-Philippe Brucker
2020-01-14 15:25   ` Will Deacon
2020-01-15 16:17     ` Will Deacon
2020-01-15 17:44     ` Robin Murphy [this message]
2019-12-19 16:30 ` [PATCH v4 12/13] PCI/ATS: Add PASID stubs Jean-Philippe Brucker
2019-12-19 16:30 ` [PATCH v4 13/13] iommu/arm-smmu-v3: Add support for PCI PASID Jean-Philippe Brucker
2019-12-20  7:37   ` Auger Eric
2020-01-14 12:45   ` Will Deacon
2020-01-15  7:57     ` Jean-Philippe Brucker
2020-01-09 14:36 ` [PATCH v4 00/13] iommu: Add PASID support to Arm SMMUv3 Jean-Philippe Brucker
2020-01-09 14:41   ` Will Deacon
2020-01-10  7:15     ` Jean-Philippe Brucker
2020-01-14 15:40 ` Will Deacon

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5287c59f-0331-4d2e-e8a0-292bf27683fb@arm.com \
    --to=robin.murphy@arm.com \
    --cc=bhelgaas@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=eric.auger@redhat.com \
    --cc=guohanjun@huawei.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jean-philippe@linaro.org \
    --cc=jonathan.cameron@huawei.com \
    --cc=joro@8bytes.org \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=rjw@rjwysocki.net \
    --cc=robh+dt@kernel.org \
    --cc=sudeep.holla@arm.com \
    --cc=will@kernel.org \
    --cc=zhangfei.gao@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git