On 01/09/16 13:31, Will Deacon wrote:
> On Thu, Sep 01, 2016 at 01:07:19PM +0100, Robin Murphy wrote:
>> On 31/08/16 18:28, Will Deacon wrote:
>>> On Tue, Aug 23, 2016 at 08:05:15PM +0100, Robin Murphy wrote:
>>>> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
>>>> +{
>>>> + struct iommu_fwspec *fwspec = dev->archdata.iommu;
>>>> + size_t size;
>>>> +
>>>> + if (!fwspec)
>>>> + return -EINVAL;
>>>> +
>>>> + size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]);
>>>> + fwspec = krealloc(dev->archdata.iommu, size, GFP_KERNEL);
>>>> + if (!fwspec)
>>>> + return -ENOMEM;
>>>> +
>>>> + while (num_ids--)
>>>> + fwspec->ids[fwspec->num_ids++] = *ids++;
>>>> +
>>>> + dev->archdata.iommu = fwspec;
>>>
>>> It might just be me, but I find this really fiddly to read. The fact
>>> that you realloc the whole fwspec, rather than just the array isn't
>>> helping, but I also think that while loop would be much better off as
>>> a for loop, using the index as, well, an index into the ids array and
>>> fwspec->ids array.
>>
>> Sure - copying one array into the tail end of another is always going to
>> be boring, ugly code, which I feel compelled to make as compact as
>> possible so as not to distract from the more interesting code, but I
>> guess that's self-defeating if it then no longer looks like something
>> simple and boring to skip over. I'll expend a few more precious lines on
>> turning it back into something staid and sensible ;)
>
> Why do you need to make the copy explicit?
Er, because we're filling the *new* ID(s) from the caller-provided
pointer into the now-enlarged array - I don't see how one could do that
implicitly. Tell you what, I'll also rename the variables to be less
confusing while I'm cleaning up the loop.
Robin.
> If you ensure that the array
> is NULL in a freshly initialised fwspec, then you can either kmalloc
> it when adding the IDs, or krealloc it if you need to append to an
> array that's already been initialised. It's pretty much the same as you
> have already, just operating on the array as opposed to the containing
> structure.
>
> Will
>
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
