Re: [PATCH v13 07/10] iommu/arm-smmu-v3: Maintain a SID->device structure

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

On Thu, Mar 25, 2021 at 05:48:07PM +0000, Will Deacon wrote:
> > +/* smmu->streams_mutex must be held */
> 
> Can you add a lockdep assertion for that?

Sure

> > +__maybe_unused
> > +static struct arm_smmu_master *
> > +arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
> > +{
> > +	struct rb_node *node;
> > +	struct arm_smmu_stream *stream;
> > +
> > +	node = smmu->streams.rb_node;
> > +	while (node) {
> > +		stream = rb_entry(node, struct arm_smmu_stream, node);
> > +		if (stream->id < sid)
> > +			node = node->rb_right;
> > +		else if (stream->id > sid)
> > +			node = node->rb_left;
> > +		else
> > +			return stream->master;
> > +	}
> > +
> > +	return NULL;
> > +}
> 
> [...]
> 
> > +static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
> > +				  struct arm_smmu_master *master)
> > +{
> > +	int i;
> > +	int ret = 0;
> > +	struct arm_smmu_stream *new_stream, *cur_stream;
> > +	struct rb_node **new_node, *parent_node = NULL;
> > +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(master->dev);
> > +
> > +	master->streams = kcalloc(fwspec->num_ids, sizeof(*master->streams),
> > +				  GFP_KERNEL);
> > +	if (!master->streams)
> > +		return -ENOMEM;
> > +	master->num_streams = fwspec->num_ids;
> > +
> > +	mutex_lock(&smmu->streams_mutex);
> > +	for (i = 0; i < fwspec->num_ids; i++) {
> > +		u32 sid = fwspec->ids[i];
> > +
> > +		new_stream = &master->streams[i];
> > +		new_stream->id = sid;
> > +		new_stream->master = master;
> > +
> > +		/*
> > +		 * Check the SIDs are in range of the SMMU and our stream table
> > +		 */
> > +		if (!arm_smmu_sid_in_range(smmu, sid)) {
> > +			ret = -ERANGE;
> > +			break;
> > +		}
> > +
> > +		/* Ensure l2 strtab is initialised */
> > +		if (smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB) {
> > +			ret = arm_smmu_init_l2_strtab(smmu, sid);
> > +			if (ret)
> > +				break;
> > +		}
> > +
> > +		/* Insert into SID tree */
> > +		new_node = &(smmu->streams.rb_node);
> > +		while (*new_node) {
> > +			cur_stream = rb_entry(*new_node, struct arm_smmu_stream,
> > +					      node);
> > +			parent_node = *new_node;
> > +			if (cur_stream->id > new_stream->id) {
> > +				new_node = &((*new_node)->rb_left);
> > +			} else if (cur_stream->id < new_stream->id) {
> > +				new_node = &((*new_node)->rb_right);
> > +			} else {
> > +				dev_warn(master->dev,
> > +					 "stream %u already in tree\n",
> > +					 cur_stream->id);
> > +				ret = -EINVAL;
> > +				break;
> > +			}
> > +		}
> > +		if (ret)
> > +			break;
> > +
> > +		rb_link_node(&new_stream->node, parent_node, new_node);
> > +		rb_insert_color(&new_stream->node, &smmu->streams);
> > +	}
> > +
> > +	if (ret) {
> > +		for (i--; i >= 0; i--)
> 
> Is 'i--' really what you want for the initial value? Doesn't that correspond
> to the ID you *didn't* add to the tree?

In case of error we break out of the loop, with i corresponding to the
stream that caused a fault but wasn't yet added to the tree. So i-- is
the last stream that was successfully added, or -1 in which case we don't
enter this for loop.

> > +			rb_erase(&master->streams[i].node, &smmu->streams);
> > +		kfree(master->streams);
> 
> Do you need to NULLify master->streams and/or reset master->num_streams
> after this? Seems like they're left dangling.

master is freed by arm_smmu_probe_device() when we return an error. Since
this function is unlikely to ever have another caller I didn't bother
cleaning up here

Thanks,
Jean