linux-kernel - Re: [PATCH 1/2] of: Implement iterator for phandles

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 22 Mar 2016 18:55:49 +0100
From:	Joerg Roedel <jroedel@...e.de>
To:	Rob Herring <robh+dt@...nel.org>
Cc:	Joerg Roedel <joro@...tes.org>,
	Grant Likely <grant.likely@...aro.org>,
	Will Deacon <will.deacon@....com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Linux IOMMU <iommu@...ts.linux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] of: Implement iterator for phandles

Hi Rob,

On Fri, Mar 18, 2016 at 10:54:57AM -0500, Rob Herring wrote:
> This mostly looks fine to me, but it is kind of a lot of functions
> just for this one thing. For example, I think the caller can track the
> index themselves if they care about it. I'd also like to see a more
> standard style for_each type iterator define rather than open coded
> while loops.

Thanks for your feedback. I think I worked everything into the patches
and am about to send a new version.

> > +struct of_phandle_iterator {
> > +       const struct device_node *np;
> > +       const __be32 *list;
> > +       const __be32 *list_end;
> > +       const __be32 *phandle_end;
> > +       phandle phandle;
> > +       struct device_node *node;
> 
> np and node? If you need both, name them based on what they point to.

Yes, 'np' is the parent node, while 'node' is the current node the
iterator points to. The parent node is needed for the error messages
later.

> > +static inline struct device_node *of_phandle_iterator_node(struct of_phandle_iterator *it)
> > +{
> > +       if (!it->node)
> > +               it->node = of_find_node_by_phandle(it->phandle);
> > +
> > +       if (it->node)
> > +               of_node_get(it->node);
> 
> The above function may have already done the get. Not sure offhand.

Yes, it does. But when the iterator moves on the node will be put again.
So when it is returned here we get an extra reference.

But it doesn't matter anymore as I changed the code to unconditionally
get the node in of_phandle_iterator_next now.



	Joerg