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@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> 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:
signature.asc
Description: PGP signature
