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@xxxxxxxxxx>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> ---
> 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.
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?
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++) {
