Re: [PATCH v5 04/19] iommu/of: Introduce iommu_fwspec

[Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>]

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? 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