From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robin Murphy Subject: Re: [PATCH v5 04/19] iommu/of: Introduce iommu_fwspec Date: Thu, 1 Sep 2016 14:25:12 +0100 Message-ID: References: <3e8eaf4fd65833fecc62828214aee81f6ca6c190.1471975357.git.robin.murphy@arm.com> <20160831172856.GI29505@arm.com> <900f3dcb-217c-4fb3-2f7d-15572f31a0c0@arm.com> <20160901123158.GE6721@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20160901123158.GE6721-5wv7dgnIgG8@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Will Deacon Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, punit.agrawal-5wv7dgnIgG8@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org List-Id: devicetree@vger.kernel.org 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 >