Messages in this thread | | | From | Bjorn Helgaas <> | Date | Tue, 27 Mar 2012 14:52:28 -0600 | Subject | Re: [PATCH 3/3] PNP: Allow resource flags to be set explicitly |
| |
On Tue, Mar 20, 2012 at 2:05 PM, Witold Szczeponik <Witold.Szczeponik@gmx.net> wrote: > The patch introduces a new means to display and to read a PNP resource's > flags. > > In the current implementation, this is done by explicitly checking for a flag > and then displaying the corresponding value. At the same time, there is no > support to read these flags when setting PNP resources using the > "/sys/bus/pnp/*/*/resources" interfaces. > > In order to uniformly display and read the flags, a table of all flags and the > corresponding clear text is introduced. This table contains all the > information that is needed to display and to set the flags. > > If a resource contains flags that are not handled by this table, they are > displayed explicitly and also can be read explicitly (c.f. example below). > > To better understand the implications of this patch, the following output from > a devices resource table illustrates the patch. If a device contains the > resources, > > io disabled > io 0xd00-0xffff window > > then the patch ensures that all values are displayed and read uniformly. If > new flags need to be interpreted, then only extending the flag table is > required. However, even without such extensions, the routines would work > properly, because then the "missing" flags would be displayed explicitly, e.g., > > io disabled > io 0xd00-0xffff flags 0x05001
I don't quite understand the illustration. Maybe it would help if you showed before & after output.
> The patch is applied against Linux 3.3.x. > > > Signed-off-by: Witold Szczeponik <Witold.Szczeponik@gmx.net> > > > Index: linux/drivers/pnp/interface.c > =================================================================== > --- linux.orig/drivers/pnp/interface.c > +++ linux/drivers/pnp/interface.c > @@ -242,6 +242,29 @@ static ssize_t pnp_show_options(struct d > return ret; > } > > +#define PNP_RES_VAL(typ, flg, val) \ > + { \ > + .type = (typ), \ > + .mask = IORESOURCE_ ## flg, \ > + .flags = IORESOURCE_ ## flg, \ > + .value = (val) \ > + } > +static struct pnp_resource_value { > + unsigned long type; > + unsigned long mask; > + unsigned long flags; > + const char *value; > +} pnp_resource_values[] = { > + PNP_RES_VAL(IORESOURCE_MEM, PREFETCH, "pref"), > + PNP_RES_VAL(IORESOURCE_IO | IORESOURCE_MEM, MEM_64, "64bit"), > + PNP_RES_VAL(IORESOURCE_IO | IORESOURCE_MEM, WINDOW, "window"), > + PNP_RES_VAL(IORESOURCE_TYPE_BITS, DISABLED, "disabled"), > + PNP_RES_VAL(IORESOURCE_TYPE_BITS, DISABLED, "<none>"),
I'm not clear on the usefulness of having both "disabled" and "<none>" here (same question as on patch [1/3]). From reading the code, it looks like we would always print them together, e.g., "disabled <none>"?
> + PNP_RES_VAL(IORESOURCE_TYPE_BITS, AUTO, "auto"), > + { .type = 0 } > +}; > +#undef PNP_RES_VAL > + > static ssize_t pnp_show_current_resources(struct device *dmdev, > struct device_attribute *attr, > char *buf) > @@ -249,7 +272,6 @@ static ssize_t pnp_show_current_resource > struct pnp_dev *dev = to_pnp_dev(dmdev); > pnp_info_buffer_t *buffer; > struct pnp_resource *pnp_res; > - struct resource *res; > int ret; > > if (!dev) > @@ -266,31 +288,53 @@ static ssize_t pnp_show_current_resource > pnp_printf(buffer, "state = %s\n", dev->active ? "active" : "disabled"); > > list_for_each_entry(pnp_res, &dev->resources, list) { > - res = &pnp_res->res; > + struct resource *res = &pnp_res->res; > + unsigned long mask = res->flags; > + struct pnp_resource_value *val; > > pnp_printf(buffer, pnp_resource_type_name(res)); > > - if (res->flags & IORESOURCE_DISABLED) { > - pnp_printf(buffer, " disabled\n"); > - continue; > + if (mask & IORESOURCE_DISABLED) { > + pnp_printf(buffer, " disabled"); > + mask &= ~IORESOURCE_DISABLED; > + } else { > + switch (pnp_resource_type(res)) { > + case IORESOURCE_IO: > + case IORESOURCE_MEM: > + case IORESOURCE_BUS: > + pnp_printf(buffer, " %#llx-%#llx", > + (unsigned long long) res->start, > + (unsigned long long) res->end); > + break; > + case IORESOURCE_IRQ: > + case IORESOURCE_DMA: > + pnp_printf(buffer, " %lld", > + (unsigned long long) res->start); > + break; > + } > } > > - switch (pnp_resource_type(res)) { > - case IORESOURCE_IO: > - case IORESOURCE_MEM: > - case IORESOURCE_BUS: > - pnp_printf(buffer, " %#llx-%#llx%s\n", > - (unsigned long long) res->start, > - (unsigned long long) res->end, > - res->flags & IORESOURCE_WINDOW ? > - " window" : ""); > - break; > - case IORESOURCE_IRQ: > - case IORESOURCE_DMA: > - pnp_printf(buffer, " %lld\n", > - (unsigned long long) res->start); > - break; > + /* ignore the "automatically assigned" flag */ > + mask &= ~IORESOURCE_AUTO; > + /* ignore all bus-specific flags */ > + mask &= ~IORESOURCE_BITS; > + > + for (val = pnp_resource_values; val->type; val++) { > + if ((pnp_resource_type(res) & val->type) > + && (mask & val->mask) > + && (mask & val->mask == val->flags)) { > + if (val->value && val->value[0]) > + pnp_printf(buffer, " %s", val->value); > + mask &= ~val->mask; > + } > } > + > + /* fallback when there are still some unhandled flags */ > + if (mask) > + pnp_printf(buffer, " flags %#llx", > + (unsigned long long) res->flags); > + > + pnp_printf(buffer, "\n"); > } > > ret = (buffer->curr - buf); > @@ -330,7 +374,32 @@ static char *pnp_get_resource_value(char > } > } > > - /* TBD: allow for additional flags, e.g., IORESOURCE_WINDOW */ > + /* allow for additional flags, e.g., "window" (IORESOURCE_WINDOW) */ > + buf = skip_spaces(buf);
You could just do this here and unindent the rest of the function:
if (!flags) return buf;
> + if (flags) { > + struct pnp_resource_value *val = pnp_resource_values; > + > + while (val->type) { > + size_t len = 0; > + > + if ((val->type & type) && val->value) > + len = strlen(val->value); > + > + if (len && !strnicmp(buf, val->value, len)) { > + *flags = (*flags & ~val->mask) | val->flags; > + > + buf = skip_spaces(buf + len); > + val = pnp_resource_values; /* restart loop */ > + } else > + val += 1; /* check next value */ > + } > + } > + > + /* fallback: allow for direct flag setting */ > + if (flags && !strnicmp(buf, "flags", 5)) { > + buf += 5; > + *flags = simple_strtoul(buf, &buf, 0);
Is there a minor asymmetry here? When printing, I think you could print "pref flags 0x..." (so you have both decoded bits and undecoded bits), but if you pasted that same "pref flags 0x..." text into a "set," you would lose the PREFETCH bit because you overwrite *flags instead of ORing in the extra bits.
> + } > > return buf; > } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |