On Thu, 28 Oct 2021, 00:14 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> On Wed, 27 Oct 2021, Julien Grall wrote:
> > > > > > +    return ret;
> > > > > >    }
> > > > > >    static int register_smmu_master(struct arm_smmu_device *smmu,
> > > > > > @@ -2056,7 +2066,10 @@ static int arm_smmu_add_device(struct
> device
> > > > > > *dev)
> > > > > >        } else {
> > > > > >            struct arm_smmu_master *master;
> > > > > > +        spin_lock(&arm_smmu_devices_lock);
> > > > > >            master = find_smmu_master(smmu, dev->of_node);
> > > > > > +        spin_unlock(&arm_smmu_devices_lock);
> > > > >
> > > > > At the moment, unlocking here is fine because we don't remove the
> > > > > device. However, there are a series to supporting removing a
> device (see
> > > > > [1]). So I think it would be preferable to unlock after the last
> use of
> > > > > 'cfg'.
> > > > >
> > > Ok. I will move unlocking to the end of this else {} block. I was not
> aware
> > > of the patch you are referring to.
> >
> > I think the end of the else is still too early. This needs to at least
> be past
> > iommu_group_set_iommudata() because we store cfg.
> >
> > Potentially, the lock wants to also englobe
> arm_smmu_master_alloc_smes(). So I
> > am wondering whether it would be simpler to hold the lock for the whole
> > duration of arm_smmu_add_device() (I can see value when we will want to
> > interlock with the remove code).
>
> The patch was to protect the smmu master list. From that point of view
> the unlock right after find_smmu_master would be sufficient, right?
>

Yes. However this is not fixing all the problems (see below).


> We only need to protect cfg if we are worried that the same device is
> added in two different ways at the same time. Did I get it right? If so,
> I would say that that case should not be possible? Am I missing another
> potential conflict?
>

It should not be possible to add the same device twice. The problem is more
when we are going to remove the device. In this case, "master" may
disappear at any point.

The support for removing device is not yet implemented in the tree. But
there is already a patch on the ML. So I think it would be shortsighted to
only move the lock to just solve concurrent access to the list.


>
> I am pointing this out for two reasons:
>
> Protecting the list is different from protecting each element from
> concurrent modification of the element itself. If the latter is a
> potential problem, I wonder if arm_smmu_add_device is the only function
> affected?
>

I had a brief looked at the code and couldn't find any other places where
this may be an issue.


> The second reason is that extending the lock past
> arm_smmu_master_alloc_smes is a bit worrying because it causes
> &arm_smmu_devices_lock and smmu->stream_map_lock to nest, which wasn't
> the case before.
>

Nested locks are common. I don't believe there would be a problem here with
this one.


> I am not saying that it is a bad idea to extend the lock past
> arm_smmu_master_alloc_smes -- it might be the right thing to do.


I don't usually suggest locking changes blindly ;).

But I

am merely saying that it might be best to think twice about it.

and/or do
> that after 4.16.


To be honest, this patch is not useful the callback to register a device in
the IOMMU subsystem. So if you are concerned with the my suggested locking
then, I am afraid the current patch is a no-go on my side for 4.16.

That said we can work towards a new locking approach for 4.17. However, I
would want to have a proposal from your side or at least some details on
why the suggested locking is not suitable.

Cheers,