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
