Re: [PATCH v2 1/3] checks: add phandle with arg property checks

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3275 bytes --]

On Fri, Aug 25, 2017 at 10:27:09AM -0500, Rob Herring wrote:
> On Fri, Aug 25, 2017 at 8:17 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Thu, Aug 24, 2017 at 02:19:27PM -0500, Rob Herring wrote:
> > > On Thu, Aug 24, 2017 at 12:23 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > > > On Wed, Aug 23, 2017 at 9:03 PM, David Gibson
> > > > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > >> On Tue, Aug 22, 2017 at 06:02:06PM -0500, Rob Herring wrote:
> > > >>> Many common bindings follow the same pattern of client properties
> > > >>> containing a phandle and N arg cells where N is defined in the provider
> > > >>> with a '#<specifier>-cells' property such as:
> > >
> > > [...]
> > >
> > > >>> +             if (prop->val.len < ((cell + cellsize + 1) * sizeof(cell_t))) {
> > > >>> +                     FAIL(c, dti, "%s property size (%d) too small for cell size %d in %s",
> > > >>> +                          prop->name, prop->val.len, cellsize, node->fullpath);
> > > >>> +             }
> > > >>
> > > >> How will this cope if the property is really badly formed - e.g. a 3
> > > >> byte property, so you can't even grab a whole first phandle?  I think
> > > >> it will trip the assert() in propval_cell_n() which isn't great.
> > > >
> > > > At least for your example, we'd exit the loop (cell < 3/4). But I need
> > > > to a check for that because it would be silent currently. I'll add a
> > > > check that the size is a multiple of 4 and greater than 0.
> > > >
> > > > However, the check here is not perfect because we could have
> > > > "<&phandle1 1 2>" when really it should be "<&phandle1 &phandle2
> > > > &phandle3>". We don't fail until we're checking the 2nd phandle.
> > > > That's why I added the "or bad phandle" and the cell # in the message
> > > > above. In the opposite case, we'd be silent. One thing that could be
> > > > done is double check things against the markers if they are present.
> > >
> > > Here's what that looks like:
> > >
> > > /* If we have markers, verify the current cell is a phandle */
> > > if (prop->val.markers) {
> > >   struct marker *m = prop->val.markers;
> > >   for_each_marker_of_type(m, REF_PHANDLE) {
> > >     if (m->offset == (cell * sizeof(cell_t)))
> > >       break;
> > >   }
> > >   if (!m)
> > >   FAIL(c, dti, "Property '%s', cell %d is not a valid phandle in %s",
> > >     prop->name, cell, node->fullpath);
> >
> > The logic seems sound, but I don't like the message.  An integer
> > literal is no less a phandle than a reference, just usually not the
> > best way of entering one.
> 
> Then what do you propose? There's not really any way I can distinguish
> a mixture. If #whatever-cells was wrong and I'm pointing to an integer
> literal that's not a phandle, it looks no different than if
> #whatever-cells is correct and I'm pointing to an integer literal that
> is a phandle.

Simply s/valid phandle/phandle reference/ would be sufficient.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]