On Fri, Aug 18, 2017 at 11:02:01AM -0500, Rob Herring wrote:
> On Thu, Aug 17, 2017 at 11:35 PM, David Gibson
> <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
> >
> > Yay! Someone actually implementing checks.
>
> Not like it is my first. I'll celebrate when someone else does.
>
>
> >> 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. Add a checks for properties
> >> following this pattern.
> >
> > I think this description would be easier to follow if you led with an example.
> >
> >>
> >> Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> >
> > Looks pretty good, though I have some suggestions.
> >
> >> ---
> >> checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 117 insertions(+)
> >>
> >> diff --git a/checks.c b/checks.c
> >> index afabf64337d5..c0450e118043 100644
> >> --- a/checks.c
> >> +++ b/checks.c
> >> @@ -956,6 +956,120 @@ 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;
> >> +};
> >> +
> >> +static void check_property_phandle_args(struct check *c,
> >> + struct dt_info *dti,
> >> + struct node *node,
> >> + struct property *prop,
> >> + const struct provider *provider)
> >> +{
> >> + struct marker *m = prop->val.markers;
> >> +
> >> + if (!m) {
> >> + FAIL(c, dti, "Missing phandles in %s:%s",
> >> + node->fullpath, prop->name);
> >> + return;
> >> + }
> >> + for_each_marker_of_type(m, REF_PHANDLE) {
> >
> > So going through the markers I think is not the best approach.
> > That'll work if the source contains a reference here, which it usually
> > will, but there are some circumstances where it could contain a "raw"
> > phandle value (the most obvious being when you're decompiling an
> > existing dtb).
> >
> > But I don't think you really need to. Instead you should be able to
> > read the actual value, look it up with get_node_by_phandle(). You can
> > make this check dependent on fixup_phandle_references to make sure
> > it's executed after the references are resolved.
>
> That's how I implemented it initially...
Ok.. why did you change?
> > That should also let you more strictly verify the property in the
> > client node, including checking for misaligned entries of extraneous bits.
> >
> >> + int cellsize;
> >> + struct node *root = dti->dt;
> >> + struct node *provider_node;
> >> + struct property *cellprop;
> >> +
> >> + provider_node = get_node_by_ref(root, m->ref);
> >> + if (!provider_node) {
> >> + FAIL(c, dti, "Could not get provider for %s:%s",
> >> + node->fullpath, prop->name);
> >> + break;
> >> + }
> >
> > AFAIK the "provider" terminology isn't standardized somewhere. It's a
> > reasonable term internally, but if can rephrase to it in the displayed
> > messages that would be a bonus.
>
> The kernel uses that at least. I guess maybe "phandle node" will work.
Hm, if the kernel uses the term, that's probably enough, actually.
> >> + 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 %s in provider %s for %s",
> >> + provider->cell_name,
> >> + provider_node->fullpath,
> >> + node->fullpath);
> >> + break;
> >> + }
> >> +
> >> + if (prop->val.len < ((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);
> >> + }
> >> + }
> >> +}
> >> +
> >> +static const struct provider providers[] = {
> >> + { "clocks", "#clock-cells" },
> >> + { "phys", "#phy-cells" },
> >> + { "interrupts-extended", "#interrupt-cells" },
> >> + { "mboxes", "#mbox-cells" },
> >> + { "pwms", "#pwm-cells" },
> >> + { "dmas", "#dma-cells" },
> >> + { "resets", "#reset-cells" },
> >> + { "hwlocks", "#hwlock-cells" },
> >> + { "power-domains", "#power-domain-cells" },
> >> + { "io-channels", "#io-channel-cells" },
> >> + { "iommus", "#iommu-cells" },
> >> + { "mux-controls", "#mux-control-cells" },
> >> + { "cooling-device", "#cooling-cells" },
> >> + { "thermal-sensors", "#thermal-sensor-cells" },
> >> + { "sound-dais", "#sound-dai-cells" },
> >> + { "msi-parent", "#msi-cells", true },
> >> + { NULL },
> >> +};
> >
> > How hard would it be to use macros to make each of these providers
> > different check structures. I think the output would be a bit more
> > useful if errors in different types of these things was reported as a
> > different check failure rather than one big generic one.
>
> Should be doable.
Ok. Not essential, but I think it would improve the output.
> > It does mean listing everything in the check_table which is a pain. I
> > would really like to change things so that a single macro can both
> > declare the check and add it to the master list, but I haven't thought
> > of a portable way to do that so far.
>
> This is done in the kernel frequently using linker sections. Each
> check entry would get put into a specific section, then you just
> iterate through the entries. Would that work? I could imagine that
> linker magic may not be all the portable.
Right. Linker sections are the usual way to do this, but that
requires lower level knowledge of how the toolchain works than I
really want to put into dtc. At heart dtc is really a very
straightforward standard C program, so I don't want to introduce
dependencies on a specific compiler.
>
> > If you do need the providers array, ARRAY_SIZE() is preferred over a
> > terminator NULL.
>
> Okay.
>
> >> +static void check_provider_cells_property(struct check *c,
> >> + struct dt_info *dti,
> >> + struct node *node)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0; providers[i].prop_name; i++) {
> >> + struct property *prop = get_property(node, providers[i].prop_name);
> >> + if (!prop)
> >> + continue;
> >> + check_property_phandle_args(c, dti, node, prop, &providers[i]);
> >> + }
> >> +}
> >> +WARNING(provider_cells_property, check_provider_cells_property, NULL);
> >> +
> >> +static void check_gpio_cells_property(struct check *c,
> >> + struct dt_info *dti,
> >> + struct node *node)
> >> +{
> >> + struct property *prop;
> >> +
> >> + for_each_property(node, prop) {
> >> + char *str;
> >> + struct provider provider;
> >> +
> >> + /* Skip over false matches */
> >> + if (strstr(prop->name, "nr-gpio"))
> >> + continue;
> >> +
> >> + str = strrchr(prop->name, '-');
> >> + if (!str || !(streq(str, "-gpio") || streq(str, "-gpios")))
> >> + continue;
> >> +
> >> + provider.prop_name = prop->name;
> >> + provider.cell_name = "#gpio-cells";
> >> + provider.optional = false;
> >> + check_property_phandle_args(c, dti, node, prop, &provider);
> >> + }
> >> +
> >> +}
> >> +WARNING(gpio_cells_property, check_gpio_cells_property, NULL);
> >
> > Since the gpio stuff is a bit tweaker, it would be good to go into a
> > separate patch. Then the commit message can explain what the logic
> > here is for (I'm having a little trouble following it).
>
> Okay.
>
> Rob
--
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
