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: 6439 bytes --]

On Thu, Aug 24, 2017 at 12:23:16PM -0500, Rob Herring 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:
> >>
> >>       intc0: interrupt-controller@0 {
> >>               #interrupt-cells = <3>;
> >>       };
> >>       intc1: interrupt-controller@1 {
> >>               #interrupt-cells = <2>;
> >>       };
> >>
> >>       node {
> >>               interrupts-extended = <&intc0 1 2 3>, <&intc1 4 5>;
> >>       };
> >>
> >> Add checks for properties following this pattern.
> >>
> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> ---
> >> v2:
> >> - Make each property a separate check
> >> - Iterate over raw cells rather than markers
> >> - Fix property length check for 2nd to Nth items
> >> - Improve error messages. If cell sizes are wrong, the next iteration can
> >>   get a bad (but valid) phandle.
> >> - Add a test
> >>
> >>  checks.c                    | 104 ++++++++++++++++++++++++++++++++++++++++++++
> >>  dtc.h                       |   1 +
> >>  livetree.c                  |   6 +++
> >>  tests/bad-phandle-cells.dts |  11 +++++
> >>  tests/run_tests.sh          |   1 +
> >>  5 files changed, 123 insertions(+)
> >>  create mode 100644 tests/bad-phandle-cells.dts
> >>
> >> diff --git a/checks.c b/checks.c
> >> index afabf64337d5..548d7e118c42 100644
> >> --- a/checks.c
> >> +++ b/checks.c
> >> @@ -956,6 +956,93 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
> >>  WARNING(obsolete_chosen_interrupt_controller,
> >>       check_obsolete_chosen_interrupt_controller, NULL);
> >>
> >> +struct provider {
> >> +     const char *prop_name;
> >> +     const char *cell_name;
> >> +     bool optional;
> >
> > AFAICT you don't actually use this optional flag, even in the followup
> > patches; it's always false.
> 
> Yes, it is. Here:
> 
> >> +WARNING_PROPERTY_PHANDLE_CELLS(msi_parent, "msi-parent", "#msi-cells", true);
> 
> Well hidden, isn't it. :)

Little bit, yes.  Objection withdrawn.

> 
> >
> >> +};
> >> +
> >> +static void check_property_phandle_args(struct check *c,
> >> +                                       struct dt_info *dti,
> >> +                                       struct node *node,
> >> +                                       struct property *prop,
> >> +                                       const struct provider *provider)
> >> +{
> >> +     struct node *root = dti->dt;
> >> +     int cell, cellsize = 0;
> >> +
> >> +     for (cell = 0; cell < prop->val.len / sizeof(cell_t); cell += cellsize + 1) {
> >> +             struct node *provider_node;
> >> +             struct property *cellprop;
> >> +             int phandle;
> >> +
> >> +             phandle = propval_cell_n(prop, cell);
> >> +             if (phandle == 0 || phandle == -1) {
> >> +                     cellsize = 0;
> >> +                     continue;
> >
> > I'm not clear what case this is handling.  If the property has an
> > invalid (or unresolved) phandle value, shouldn't that be a FAIL?  As
> > it is we interpret the next cell as a phandle, and since we couldn't
> > resolve the first phandle, we can't be at all sure that it really is a
> > phandle.
> 
> It is valid to have a "blank" phandle when you have optional entries,
> but need to preserve the indexes of the entries. Say an array of gpio
> lines and some may not be hooked up. Not widely used, but it does
> exist in kernel dts files.

Ah ok.  A comment to that effect would be helpful.

> 
> 
> >> +             }
> >> +
> >> +             provider_node = get_node_by_phandle(root, phandle);
> >> +             if (!provider_node) {
> >> +                     FAIL(c, dti, "Could not get phandle node for %s:%s(cell %d)",
> >> +                          node->fullpath, prop->name, cell);
> >> +                     break;
> >> +             }
> >> +
> >> +             cellprop = get_property(provider_node, provider->cell_name);
> >> +             if (cellprop) {
> >> +                     cellsize = propval_cell(cellprop);
> >> +             } else if (provider->optional) {
> >> +                     cellsize = 0;
> >> +             } else {
> >> +                     FAIL(c, dti, "Missing property '%s' in node %s or bad phandle (referred from %s:%s[%d])",
> >> +                          provider->cell_name,
> >> +                          provider_node->fullpath,
> >> +                          node->fullpath, prop->name, cell);
> >> +                     break;
> >> +             }
> >> +
> >> +             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.

Ok.

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

Uh.. I don't really understand what you're getting at here.  We should
be able to determine which of these cases it should be by the
#whatever-cells at &phandle1.

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