* [PATCH 0/3] PNP: Allow PNP resources to be disabled (interface)
@ 2012-03-20 18:49 Witold Szczeponik
2012-03-20 19:57 ` [PATCH 1/3] PNP: Simplify setting of resources Witold Szczeponik
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Witold Szczeponik @ 2012-03-20 18:49 UTC (permalink / raw)
To: bhelgaas, abelay; +Cc: linux-kernel
Hello everybody,
this simple patch series continues the work begun in commit
18fd470a48396c8795ba7256c5973e92ffa25cb3 where ACPI PNP resource templates
with empty/disabled resources are handled.
The aim of this patch series is to allow to set resources as "disabled" using
the "/sys/bus/pnp/devices/*/resources" interface. Such "disabled" resources
are needed by some vintage IBM ThinkPads like the 600E where some resources
need to have their IRQs disabled in order to support all the devices the 600E
has.
To this end, some preparatory work is done, simplifying the code and allowing
for more flexibility in the definition of these resources.
Here's a brief description of these patches.
[1/3] - Factor out common some code
[2/3] - Perform the actual setting
[3/3] - Generalize handling of resource flags
The patches are applied against Linux 3.3.x.
Comments are, as always, welcome. :-)
--- Witold
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] PNP: Simplify setting of resources
2012-03-20 18:49 [PATCH 0/3] PNP: Allow PNP resources to be disabled (interface) Witold Szczeponik
@ 2012-03-20 19:57 ` Witold Szczeponik
2012-03-27 20:32 ` Bjorn Helgaas
2012-03-20 20:00 ` [PATCH 2/3] PNP: Allow resources to be set as disabled Witold Szczeponik
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Witold Szczeponik @ 2012-03-20 19:57 UTC (permalink / raw)
To: bhelgaas; +Cc: linux-kernel
This patch factors out the setting of PNP resources into one function which is
then reused for all PNP resource types. This makes the code more concise and
avoids duplication. The parameters "type" and "flags" are not used at the
moment but will be used by follow-up patches. Placeholders for these patches
can be found in the comment lines that contain the "TBD" marker.
As the code does not make any changes to the ABI, no regressions are expected.
NB: While at it, support for bus type resources is added.
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
@@ -298,6 +298,39 @@ static ssize_t pnp_show_current_resource
return ret;
}
+static char *pnp_get_resource_value(char *buf,
+ unsigned long type,
+ resource_size_t *start,
+ resource_size_t *end,
+ unsigned long *flags)
+{
+ if (start)
+ *start = 0;
+ if (end)
+ *end = 0;
+ if (flags)
+ *flags = 0;
+
+ /* TBD: allow for disabled resources */
+
+ buf = skip_spaces(buf);
+ if (start) {
+ *start = simple_strtoull(buf, &buf, 0);
+ if (end) {
+ buf = skip_spaces(buf);
+ if (*buf == '-') {
+ buf = skip_spaces(buf + 1);
+ *end = simple_strtoull(buf, &buf, 0);
+ } else
+ *end = *start;
+ }
+ }
+
+ /* TBD: allow for additional flags, e.g., IORESOURCE_WINDOW */
+
+ return buf;
+}
+
static ssize_t pnp_set_current_resources(struct device *dmdev,
struct device_attribute *attr,
const char *ubuf, size_t count)
@@ -305,7 +338,6 @@ static ssize_t pnp_set_current_resources
struct pnp_dev *dev = to_pnp_dev(dmdev);
char *buf = (void *)ubuf;
int retval = 0;
- resource_size_t start, end;
if (dev->status & PNP_ATTACHED) {
retval = -EBUSY;
@@ -349,6 +381,10 @@ static ssize_t pnp_set_current_resources
goto done;
}
if (!strnicmp(buf, "set", 3)) {
+ resource_size_t start;
+ resource_size_t end;
+ unsigned long flags;
+
if (dev->active)
goto done;
buf += 3;
@@ -357,42 +393,42 @@ static ssize_t pnp_set_current_resources
while (1) {
buf = skip_spaces(buf);
if (!strnicmp(buf, "io", 2)) {
- buf = skip_spaces(buf + 2);
- start = simple_strtoul(buf, &buf, 0);
- buf = skip_spaces(buf);
- if (*buf == '-') {
- buf = skip_spaces(buf + 1);
- end = simple_strtoul(buf, &buf, 0);
- } else
- end = start;
- pnp_add_io_resource(dev, start, end, 0);
- continue;
- }
- if (!strnicmp(buf, "mem", 3)) {
- buf = skip_spaces(buf + 3);
- start = simple_strtoul(buf, &buf, 0);
- buf = skip_spaces(buf);
- if (*buf == '-') {
- buf = skip_spaces(buf + 1);
- end = simple_strtoul(buf, &buf, 0);
- } else
- end = start;
- pnp_add_mem_resource(dev, start, end, 0);
- continue;
- }
- if (!strnicmp(buf, "irq", 3)) {
- buf = skip_spaces(buf + 3);
- start = simple_strtoul(buf, &buf, 0);
- pnp_add_irq_resource(dev, start, 0);
- continue;
- }
- if (!strnicmp(buf, "dma", 3)) {
- buf = skip_spaces(buf + 3);
- start = simple_strtoul(buf, &buf, 0);
- pnp_add_dma_resource(dev, start, 0);
- continue;
- }
- break;
+ buf = pnp_get_resource_value(buf + 2,
+ IORESOURCE_IO,
+ &start,
+ &end,
+ &flags);
+ pnp_add_io_resource(dev, start, end, flags);
+ } else if (!strnicmp(buf, "mem", 3)) {
+ buf = pnp_get_resource_value(buf + 3,
+ IORESOURCE_MEM,
+ &start,
+ &end,
+ &flags);
+ pnp_add_mem_resource(dev, start, end, flags);
+ } else if (!strnicmp(buf, "irq", 3)) {
+ buf = pnp_get_resource_value(buf + 3,
+ IORESOURCE_IRQ,
+ &start,
+ NULL,
+ &flags);
+ pnp_add_irq_resource(dev, start, flags);
+ } else if (!strnicmp(buf, "dma", 3)) {
+ buf = pnp_get_resource_value(buf + 3,
+ IORESOURCE_DMA,
+ &start,
+ NULL,
+ &flags);
+ pnp_add_dma_resource(dev, start, flags);
+ } else if (!strnicmp(buf, "bus", 3)) {
+ buf = pnp_get_resource_value(buf + 3,
+ IORESOURCE_BUS,
+ &start,
+ &end,
+ NULL);
+ pnp_add_bus_resource(dev, start, end);
+ } else
+ break;
}
mutex_unlock(&pnp_res_mutex);
goto done;
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] PNP: Allow resources to be set as disabled
2012-03-20 18:49 [PATCH 0/3] PNP: Allow PNP resources to be disabled (interface) Witold Szczeponik
2012-03-20 19:57 ` [PATCH 1/3] PNP: Simplify setting of resources Witold Szczeponik
@ 2012-03-20 20:00 ` Witold Szczeponik
2012-03-27 20:38 ` Bjorn Helgaas
2012-03-20 20:05 ` [PATCH 3/3] PNP: Allow resource flags to be set explicitly Witold Szczeponik
2012-03-27 20:57 ` [PATCH 0/3] PNP: Allow PNP resources to be disabled (interface) Bjorn Helgaas
3 siblings, 1 reply; 12+ messages in thread
From: Witold Szczeponik @ 2012-03-20 20:00 UTC (permalink / raw)
To: bhelgaas; +Cc: linux-kernel
This patch allows to set PNP resources as "disabled". As such, the patch is
a follow-up to commit 18fd470a48396c8795ba7256c5973e92ffa25cb3 where parsing
of ACPI PNP resources that can be disabled was enabled.
The patch achieves this by doing two things: (1) it allows the strings
"disabled" and "<none>" to be used as a valid PNP resource value, and (2) when
assigning PNP resources, it copies the flags masked by IORESOURCE_BITS from the
resource's templates. The latter is necessary because the resource settings
require proper IORESOURCE_BITS which are not known during the definition of
these resources using the "/sys/bus/pnp/*/*/resources" interface. (In fact,
they should not be set by the user as the resource templates define the
proper settings.)
The patch is required in order to support the setting of "disabled" IRQs like
described in the commit 29df8d8f8702f0f53c1375015f09f04bc8d023c1, i.e., with
this patch applied, some vintage IBM ThinkPads like the 600E can allocate the
resources such that all devices can be used simultaneously.
If the second part of the patch is not applied, the resource flags are not
initialized properly and obscure messages in the kernel log have be seen
("invalid flags").
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
@@ -311,10 +311,14 @@ static char *pnp_get_resource_value(char
if (flags)
*flags = 0;
- /* TBD: allow for disabled resources */
-
buf = skip_spaces(buf);
- if (start) {
+ if (flags && !strnicmp(buf, "disabled", 8)) {
+ buf += 8;
+ *flags |= IORESOURCE_DISABLED;
+ } else if (flags && !strnicmp(buf, "<none>", 6)) {
+ buf += 6;
+ *flags |= IORESOURCE_DISABLED;
+ } else if (start) {
*start = simple_strtoull(buf, &buf, 0);
if (end) {
buf = skip_spaces(buf);
Index: linux/drivers/pnp/manager.c
===================================================================
--- linux.orig/drivers/pnp/manager.c
+++ linux/drivers/pnp/manager.c
@@ -18,11 +18,27 @@
DEFINE_MUTEX(pnp_res_mutex);
+static struct resource *pnp_find_resource(struct pnp_dev *dev,
+ unsigned char rule,
+ unsigned long type,
+ unsigned int bar)
+{
+ struct resource *res = pnp_get_resource(dev, type, bar);
+
+ /* when the resource already exists, set its resource bits from rule */
+ if (res) {
+ res->flags &= ~IORESOURCE_BITS;
+ res->flags |= rule & IORESOURCE_BITS;
+ }
+
+ return res;
+}
+
static int pnp_assign_port(struct pnp_dev *dev, struct pnp_port *rule, int idx)
{
struct resource *res, local_res;
- res = pnp_get_resource(dev, IORESOURCE_IO, idx);
+ res = pnp_find_resource(dev, rule->flags, IORESOURCE_IO, idx);
if (res) {
pnp_dbg(&dev->dev, " io %d already set to %#llx-%#llx "
"flags %#lx\n", idx, (unsigned long long) res->start,
@@ -65,7 +81,7 @@ static int pnp_assign_mem(struct pnp_dev
{
struct resource *res, local_res;
- res = pnp_get_resource(dev, IORESOURCE_MEM, idx);
+ res = pnp_find_resource(dev, rule->flags, IORESOURCE_MEM, idx);
if (res) {
pnp_dbg(&dev->dev, " mem %d already set to %#llx-%#llx "
"flags %#lx\n", idx, (unsigned long long) res->start,
@@ -78,6 +94,7 @@ static int pnp_assign_mem(struct pnp_dev
res->start = 0;
res->end = 0;
+ /* ??? rule->flags restricted to 8 bits, all tests bogus ??? */
if (!(rule->flags & IORESOURCE_MEM_WRITEABLE))
res->flags |= IORESOURCE_READONLY;
if (rule->flags & IORESOURCE_MEM_CACHEABLE)
@@ -123,7 +140,7 @@ static int pnp_assign_irq(struct pnp_dev
5, 10, 11, 12, 9, 14, 15, 7, 3, 4, 13, 0, 1, 6, 8, 2
};
- res = pnp_get_resource(dev, IORESOURCE_IRQ, idx);
+ res = pnp_find_resource(dev, rule->flags, IORESOURCE_IRQ, idx);
if (res) {
pnp_dbg(&dev->dev, " irq %d already set to %d flags %#lx\n",
idx, (int) res->start, res->flags);
@@ -182,7 +199,7 @@ static int pnp_assign_dma(struct pnp_dev
1, 3, 5, 6, 7, 0, 2, 4
};
- res = pnp_get_resource(dev, IORESOURCE_DMA, idx);
+ res = pnp_find_resource(dev, rule->flags, IORESOURCE_DMA, idx);
if (res) {
pnp_dbg(&dev->dev, " dma %d already set to %d flags %#lx\n",
idx, (int) res->start, res->flags);
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] PNP: Allow resource flags to be set explicitly
2012-03-20 18:49 [PATCH 0/3] PNP: Allow PNP resources to be disabled (interface) Witold Szczeponik
2012-03-20 19:57 ` [PATCH 1/3] PNP: Simplify setting of resources Witold Szczeponik
2012-03-20 20:00 ` [PATCH 2/3] PNP: Allow resources to be set as disabled Witold Szczeponik
@ 2012-03-20 20:05 ` Witold Szczeponik
2012-03-27 20:52 ` Bjorn Helgaas
2012-03-27 20:57 ` [PATCH 0/3] PNP: Allow PNP resources to be disabled (interface) Bjorn Helgaas
3 siblings, 1 reply; 12+ messages in thread
From: Witold Szczeponik @ 2012-03-20 20:05 UTC (permalink / raw)
To: bhelgaas; +Cc: linux-kernel
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
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>"),
+ 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);
+ 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);
+ }
return buf;
}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] PNP: Simplify setting of resources
2012-03-20 19:57 ` [PATCH 1/3] PNP: Simplify setting of resources Witold Szczeponik
@ 2012-03-27 20:32 ` Bjorn Helgaas
2012-04-01 16:05 ` Witold Szczeponik
0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Helgaas @ 2012-03-27 20:32 UTC (permalink / raw)
To: Witold Szczeponik; +Cc: linux-kernel
On Tue, Mar 20, 2012 at 1:57 PM, Witold Szczeponik
<Witold.Szczeponik@gmx.net> wrote:
> This patch factors out the setting of PNP resources into one function which is
> then reused for all PNP resource types. This makes the code more concise and
> avoids duplication. The parameters "type" and "flags" are not used at the
> moment but will be used by follow-up patches. Placeholders for these patches
> can be found in the comment lines that contain the "TBD" marker.
>
> As the code does not make any changes to the ABI, no regressions are expected.
>
> NB: While at it, support for bus type resources is added.
>
> The patch is applied against Linux 3.3.x.
A few minor comments below, but I think this looks good. In the past,
I've sent PNP patches through linux-acpi and Len's ACPI tree.
Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
> 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
> @@ -298,6 +298,39 @@ static ssize_t pnp_show_current_resource
> return ret;
> }
>
> +static char *pnp_get_resource_value(char *buf,
> + unsigned long type,
> + resource_size_t *start,
> + resource_size_t *end,
> + unsigned long *flags)
> +{
> + if (start)
> + *start = 0;
> + if (end)
> + *end = 0;
> + if (flags)
> + *flags = 0;
> +
> + /* TBD: allow for disabled resources */
> +
> + buf = skip_spaces(buf);
> + if (start) {
> + *start = simple_strtoull(buf, &buf, 0);
> + if (end) {
> + buf = skip_spaces(buf);
> + if (*buf == '-') {
> + buf = skip_spaces(buf + 1);
> + *end = simple_strtoull(buf, &buf, 0);
> + } else
> + *end = *start;
> + }
> + }
> +
> + /* TBD: allow for additional flags, e.g., IORESOURCE_WINDOW */
> +
> + return buf;
> +}
> +
> static ssize_t pnp_set_current_resources(struct device *dmdev,
> struct device_attribute *attr,
> const char *ubuf, size_t count)
> @@ -305,7 +338,6 @@ static ssize_t pnp_set_current_resources
> struct pnp_dev *dev = to_pnp_dev(dmdev);
> char *buf = (void *)ubuf;
> int retval = 0;
> - resource_size_t start, end;
>
> if (dev->status & PNP_ATTACHED) {
> retval = -EBUSY;
> @@ -349,6 +381,10 @@ static ssize_t pnp_set_current_resources
> goto done;
> }
> if (!strnicmp(buf, "set", 3)) {
> + resource_size_t start;
> + resource_size_t end;
> + unsigned long flags;
> +
> if (dev->active)
> goto done;
> buf += 3;
> @@ -357,42 +393,42 @@ static ssize_t pnp_set_current_resources
> while (1) {
> buf = skip_spaces(buf);
> if (!strnicmp(buf, "io", 2)) {
> - buf = skip_spaces(buf + 2);
> - start = simple_strtoul(buf, &buf, 0);
> - buf = skip_spaces(buf);
> - if (*buf == '-') {
> - buf = skip_spaces(buf + 1);
> - end = simple_strtoul(buf, &buf, 0);
> - } else
> - end = start;
> - pnp_add_io_resource(dev, start, end, 0);
> - continue;
> - }
> - if (!strnicmp(buf, "mem", 3)) {
> - buf = skip_spaces(buf + 3);
> - start = simple_strtoul(buf, &buf, 0);
> - buf = skip_spaces(buf);
> - if (*buf == '-') {
> - buf = skip_spaces(buf + 1);
> - end = simple_strtoul(buf, &buf, 0);
> - } else
> - end = start;
> - pnp_add_mem_resource(dev, start, end, 0);
> - continue;
> - }
> - if (!strnicmp(buf, "irq", 3)) {
> - buf = skip_spaces(buf + 3);
> - start = simple_strtoul(buf, &buf, 0);
> - pnp_add_irq_resource(dev, start, 0);
> - continue;
> - }
> - if (!strnicmp(buf, "dma", 3)) {
> - buf = skip_spaces(buf + 3);
> - start = simple_strtoul(buf, &buf, 0);
> - pnp_add_dma_resource(dev, start, 0);
> - continue;
> - }
> - break;
> + buf = pnp_get_resource_value(buf + 2,
> + IORESOURCE_IO,
> + &start,
> + &end,
> + &flags);
Looks like at least "&end" will fit on the same line as "&start".
> + pnp_add_io_resource(dev, start, end, flags);
> + } else if (!strnicmp(buf, "mem", 3)) {
> + buf = pnp_get_resource_value(buf + 3,
> + IORESOURCE_MEM,
> + &start,
> + &end,
> + &flags);
> + pnp_add_mem_resource(dev, start, end, flags);
> + } else if (!strnicmp(buf, "irq", 3)) {
> + buf = pnp_get_resource_value(buf + 3,
> + IORESOURCE_IRQ,
> + &start,
> + NULL,
> + &flags);
> + pnp_add_irq_resource(dev, start, flags);
> + } else if (!strnicmp(buf, "dma", 3)) {
> + buf = pnp_get_resource_value(buf + 3,
> + IORESOURCE_DMA,
> + &start,
> + NULL,
> + &flags);
> + pnp_add_dma_resource(dev, start, flags);
> + } else if (!strnicmp(buf, "bus", 3)) {
> + buf = pnp_get_resource_value(buf + 3,
> + IORESOURCE_BUS,
> + &start,
> + &end,
> + NULL);
> + pnp_add_bus_resource(dev, start, end);
It makes sense to allow a user to set bus number resources just like
any other resources, but I don't think the rest of PNP supports this
yet. I think only PNPACPI supports bus number resources at all, so
when you "activate" the device after setting its resources, we'll call
pnpacpi_set_resources() and pnpacpi_encode_resources(), but
pnpacpi_encode_resources() doesn't know how to encode the address16
descriptor that will probably be used for bus numbers.
I guess that's all right, though. We already have the same situation
for all the address space descriptors when used for IO or MEM.
> + } else
> + break;
> }
> mutex_unlock(&pnp_res_mutex);
> goto done;
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] PNP: Allow resources to be set as disabled
2012-03-20 20:00 ` [PATCH 2/3] PNP: Allow resources to be set as disabled Witold Szczeponik
@ 2012-03-27 20:38 ` Bjorn Helgaas
2012-04-01 16:06 ` Witold Szczeponik
0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Helgaas @ 2012-03-27 20:38 UTC (permalink / raw)
To: Witold Szczeponik; +Cc: linux-kernel
On Tue, Mar 20, 2012 at 2:00 PM, Witold Szczeponik
<Witold.Szczeponik@gmx.net> wrote:
> This patch allows to set PNP resources as "disabled". As such, the patch is
> a follow-up to commit 18fd470a48396c8795ba7256c5973e92ffa25cb3 where parsing
> of ACPI PNP resources that can be disabled was enabled.
>
> The patch achieves this by doing two things: (1) it allows the strings
> "disabled" and "<none>" to be used as a valid PNP resource value, and (2) when
> assigning PNP resources, it copies the flags masked by IORESOURCE_BITS from the
> resource's templates.
These look like reasonable things to do, but (2) doesn't seem to
depend on (1), so you might just split them into two patches.
> The latter is necessary because the resource settings
> require proper IORESOURCE_BITS which are not known during the definition of
> these resources using the "/sys/bus/pnp/*/*/resources" interface. (In fact,
> they should not be set by the user as the resource templates define the
> proper settings.)
>
> The patch is required in order to support the setting of "disabled" IRQs like
> described in the commit 29df8d8f8702f0f53c1375015f09f04bc8d023c1, i.e., with
> this patch applied, some vintage IBM ThinkPads like the 600E can allocate the
> resources such that all devices can be used simultaneously.
>
> If the second part of the patch is not applied, the resource flags are not
> initialized properly and obscure messages in the kernel log have be seen
s/be/been/
> ("invalid flags").
>
> 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
> @@ -311,10 +311,14 @@ static char *pnp_get_resource_value(char
> if (flags)
> *flags = 0;
>
> - /* TBD: allow for disabled resources */
> -
> buf = skip_spaces(buf);
> - if (start) {
> + if (flags && !strnicmp(buf, "disabled", 8)) {
> + buf += 8;
> + *flags |= IORESOURCE_DISABLED;
> + } else if (flags && !strnicmp(buf, "<none>", 6)) {
> + buf += 6;
> + *flags |= IORESOURCE_DISABLED;
What's the value in supporting both "disabled" and "<none>"? Having
both suggests that they do different things, but it looks like they
have the same effect.
> + } else if (start) {
> *start = simple_strtoull(buf, &buf, 0);
> if (end) {
> buf = skip_spaces(buf);
> Index: linux/drivers/pnp/manager.c
> ===================================================================
> --- linux.orig/drivers/pnp/manager.c
> +++ linux/drivers/pnp/manager.c
> @@ -18,11 +18,27 @@
>
> DEFINE_MUTEX(pnp_res_mutex);
>
> +static struct resource *pnp_find_resource(struct pnp_dev *dev,
> + unsigned char rule,
> + unsigned long type,
> + unsigned int bar)
> +{
> + struct resource *res = pnp_get_resource(dev, type, bar);
> +
> + /* when the resource already exists, set its resource bits from rule */
> + if (res) {
> + res->flags &= ~IORESOURCE_BITS;
> + res->flags |= rule & IORESOURCE_BITS;
> + }
> +
> + return res;
> +}
> +
> static int pnp_assign_port(struct pnp_dev *dev, struct pnp_port *rule, int idx)
> {
> struct resource *res, local_res;
>
> - res = pnp_get_resource(dev, IORESOURCE_IO, idx);
> + res = pnp_find_resource(dev, rule->flags, IORESOURCE_IO, idx);
> if (res) {
> pnp_dbg(&dev->dev, " io %d already set to %#llx-%#llx "
> "flags %#lx\n", idx, (unsigned long long) res->start,
> @@ -65,7 +81,7 @@ static int pnp_assign_mem(struct pnp_dev
> {
> struct resource *res, local_res;
>
> - res = pnp_get_resource(dev, IORESOURCE_MEM, idx);
> + res = pnp_find_resource(dev, rule->flags, IORESOURCE_MEM, idx);
> if (res) {
> pnp_dbg(&dev->dev, " mem %d already set to %#llx-%#llx "
> "flags %#lx\n", idx, (unsigned long long) res->start,
> @@ -78,6 +94,7 @@ static int pnp_assign_mem(struct pnp_dev
> res->start = 0;
> res->end = 0;
>
> + /* ??? rule->flags restricted to 8 bits, all tests bogus ??? */
> if (!(rule->flags & IORESOURCE_MEM_WRITEABLE))
> res->flags |= IORESOURCE_READONLY;
> if (rule->flags & IORESOURCE_MEM_CACHEABLE)
> @@ -123,7 +140,7 @@ static int pnp_assign_irq(struct pnp_dev
> 5, 10, 11, 12, 9, 14, 15, 7, 3, 4, 13, 0, 1, 6, 8, 2
> };
>
> - res = pnp_get_resource(dev, IORESOURCE_IRQ, idx);
> + res = pnp_find_resource(dev, rule->flags, IORESOURCE_IRQ, idx);
> if (res) {
> pnp_dbg(&dev->dev, " irq %d already set to %d flags %#lx\n",
> idx, (int) res->start, res->flags);
> @@ -182,7 +199,7 @@ static int pnp_assign_dma(struct pnp_dev
> 1, 3, 5, 6, 7, 0, 2, 4
> };
>
> - res = pnp_get_resource(dev, IORESOURCE_DMA, idx);
> + res = pnp_find_resource(dev, rule->flags, IORESOURCE_DMA, idx);
> if (res) {
> pnp_dbg(&dev->dev, " dma %d already set to %d flags %#lx\n",
> idx, (int) res->start, res->flags);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] PNP: Allow resource flags to be set explicitly
2012-03-20 20:05 ` [PATCH 3/3] PNP: Allow resource flags to be set explicitly Witold Szczeponik
@ 2012-03-27 20:52 ` Bjorn Helgaas
2012-04-01 16:07 ` Witold Szczeponik
0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Helgaas @ 2012-03-27 20:52 UTC (permalink / raw)
To: Witold Szczeponik; +Cc: linux-kernel
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;
> }
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] PNP: Allow PNP resources to be disabled (interface)
2012-03-20 18:49 [PATCH 0/3] PNP: Allow PNP resources to be disabled (interface) Witold Szczeponik
` (2 preceding siblings ...)
2012-03-20 20:05 ` [PATCH 3/3] PNP: Allow resource flags to be set explicitly Witold Szczeponik
@ 2012-03-27 20:57 ` Bjorn Helgaas
2012-04-01 16:04 ` Witold Szczeponik
3 siblings, 1 reply; 12+ messages in thread
From: Bjorn Helgaas @ 2012-03-27 20:57 UTC (permalink / raw)
To: Witold Szczeponik; +Cc: abelay, linux-kernel
On Tue, Mar 20, 2012 at 12:49 PM, Witold Szczeponik
<Witold.Szczeponik@gmx.net> wrote:
> Hello everybody,
>
> this simple patch series continues the work begun in commit
> 18fd470a48396c8795ba7256c5973e92ffa25cb3 where ACPI PNP resource templates
> with empty/disabled resources are handled.
>
> The aim of this patch series is to allow to set resources as "disabled" using
> the "/sys/bus/pnp/devices/*/resources" interface. Such "disabled" resources
> are needed by some vintage IBM ThinkPads like the 600E where some resources
> need to have their IRQs disabled in order to support all the devices the 600E
> has.
If I understand this correctly, even with this patches, you still have
to manually muck around with "/sys/bus/pnp/devices/*/resources" to get
these ThinkPads to work? If that's the case, there must be more PNP
work we could do to make them Just Work. And that's fine; I'm just
trying to understand the current situation better. Is there a
bugzilla or other URL with more details?
> To this end, some preparatory work is done, simplifying the code and allowing
> for more flexibility in the definition of these resources.
>
> Here's a brief description of these patches.
>
> [1/3] - Factor out common some code
> [2/3] - Perform the actual setting
> [3/3] - Generalize handling of resource flags
>
> The patches are applied against Linux 3.3.x.
>
> Comments are, as always, welcome. :-)
>
>
> --- Witold
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] PNP: Allow PNP resources to be disabled (interface)
2012-03-27 20:57 ` [PATCH 0/3] PNP: Allow PNP resources to be disabled (interface) Bjorn Helgaas
@ 2012-04-01 16:04 ` Witold Szczeponik
0 siblings, 0 replies; 12+ messages in thread
From: Witold Szczeponik @ 2012-04-01 16:04 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-kernel
On 27/03/12 22:57, Bjorn Helgaas wrote:
[...]
>
> If I understand this correctly, even with this patches, you still have
> to manually muck around with "/sys/bus/pnp/devices/*/resources" to get
> these ThinkPads to work? If that's the case, there must be more PNP
> work we could do to make them Just Work. And that's fine; I'm just
> trying to understand the current situation better. Is there a
> bugzilla or other URL with more details?
>
Yes, this is true: even with this patch applied it is necessary to
explicitly set the IRQ link to disabled. Let me briefly explain it,
using an excerpt from the 600E's DSDT (printer port):
Name (PLPT, ResourceTemplate ()
{
StartDependentFnNoPri ()
{
IO (Decode16, 0x03BC, 0x03BC, 0x01, 0x04)
IRQNoFlags () {7}
}
/* Some entries deleted */
StartDependentFnNoPri ()
{
IO (Decode16, 0x03BC, 0x03BC, 0x01, 0x04)
IRQNoFlags () {}
}
EndDependentFn ()
})
As you can see, the IRQ line for the second option is empty/disabled.
Also, both options share the same priority, there is no priority of
the first option over the second option.
In order to be able to use the IRQ 7 for some other device, I need
to be able to select the second option, which can be done with the
patch series applied.
As for a bugzilla entry: I did not find any. Frankly, I don't
think there is one, for until recently, disabled resources were
not handled at all (at least in the PNPACPI case). Though, it is
(remotely?) possible the handling of optional IRQ lines
(IORESOURCE_IRQ_OPTIONAL) is obsolete these days (because the
quirks that introduce that flag seem to fix a symptom that could
have been fixed by commit 18fd470a48396c8795ba7256c5973e92ffa25cb3).
--- Witold
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] PNP: Simplify setting of resources
2012-03-27 20:32 ` Bjorn Helgaas
@ 2012-04-01 16:05 ` Witold Szczeponik
0 siblings, 0 replies; 12+ messages in thread
From: Witold Szczeponik @ 2012-04-01 16:05 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-kernel
On 27/03/12 22:32, Bjorn Helgaas wrote:
[...]
>
> A few minor comments below, but I think this looks good. In the past,
> I've sent PNP patches through linux-acpi and Len's ACPI tree.
>
> Reviewed-by: Bjorn Helgaas<bhelgaas@google.com>
>
I will send an updated version of the patch series also to Len and
linux-acpi. Thanks for pointing this out to me!
>> 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
>> @@ -298,6 +298,39 @@ static ssize_t pnp_show_current_resource
>> return ret;
>> }
>>
>> +static char *pnp_get_resource_value(char *buf,
>> + unsigned long type,
>> + resource_size_t *start,
>> + resource_size_t *end,
>> + unsigned long *flags)
>> +{
>> + if (start)
>> + *start = 0;
>> + if (end)
>> + *end = 0;
>> + if (flags)
>> + *flags = 0;
>> +
>> + /* TBD: allow for disabled resources */
>> +
>> + buf = skip_spaces(buf);
>> + if (start) {
>> + *start = simple_strtoull(buf,&buf, 0);
>> + if (end) {
>> + buf = skip_spaces(buf);
>> + if (*buf == '-') {
>> + buf = skip_spaces(buf + 1);
>> + *end = simple_strtoull(buf,&buf, 0);
>> + } else
>> + *end = *start;
>> + }
>> + }
>> +
>> + /* TBD: allow for additional flags, e.g., IORESOURCE_WINDOW */
>> +
>> + return buf;
>> +}
>> +
>> static ssize_t pnp_set_current_resources(struct device *dmdev,
>> struct device_attribute *attr,
>> const char *ubuf, size_t count)
>> @@ -305,7 +338,6 @@ static ssize_t pnp_set_current_resources
>> struct pnp_dev *dev = to_pnp_dev(dmdev);
>> char *buf = (void *)ubuf;
>> int retval = 0;
>> - resource_size_t start, end;
>>
>> if (dev->status& PNP_ATTACHED) {
>> retval = -EBUSY;
>> @@ -349,6 +381,10 @@ static ssize_t pnp_set_current_resources
>> goto done;
>> }
>> if (!strnicmp(buf, "set", 3)) {
>> + resource_size_t start;
>> + resource_size_t end;
>> + unsigned long flags;
>> +
>> if (dev->active)
>> goto done;
>> buf += 3;
>> @@ -357,42 +393,42 @@ static ssize_t pnp_set_current_resources
>> while (1) {
>> buf = skip_spaces(buf);
>> if (!strnicmp(buf, "io", 2)) {
>> - buf = skip_spaces(buf + 2);
>> - start = simple_strtoul(buf,&buf, 0);
>> - buf = skip_spaces(buf);
>> - if (*buf == '-') {
>> - buf = skip_spaces(buf + 1);
>> - end = simple_strtoul(buf,&buf, 0);
>> - } else
>> - end = start;
>> - pnp_add_io_resource(dev, start, end, 0);
>> - continue;
>> - }
>> - if (!strnicmp(buf, "mem", 3)) {
>> - buf = skip_spaces(buf + 3);
>> - start = simple_strtoul(buf,&buf, 0);
>> - buf = skip_spaces(buf);
>> - if (*buf == '-') {
>> - buf = skip_spaces(buf + 1);
>> - end = simple_strtoul(buf,&buf, 0);
>> - } else
>> - end = start;
>> - pnp_add_mem_resource(dev, start, end, 0);
>> - continue;
>> - }
>> - if (!strnicmp(buf, "irq", 3)) {
>> - buf = skip_spaces(buf + 3);
>> - start = simple_strtoul(buf,&buf, 0);
>> - pnp_add_irq_resource(dev, start, 0);
>> - continue;
>> - }
>> - if (!strnicmp(buf, "dma", 3)) {
>> - buf = skip_spaces(buf + 3);
>> - start = simple_strtoul(buf,&buf, 0);
>> - pnp_add_dma_resource(dev, start, 0);
>> - continue;
>> - }
>> - break;
>> + buf = pnp_get_resource_value(buf + 2,
>> + IORESOURCE_IO,
>> +&start,
>> +&end,
>> +&flags);
>
> Looks like at least "&end" will fit on the same line as "&start".
Indeed, they do: will change this.
>
>> + pnp_add_io_resource(dev, start, end, flags);
>> + } else if (!strnicmp(buf, "mem", 3)) {
>> + buf = pnp_get_resource_value(buf + 3,
>> + IORESOURCE_MEM,
>> +&start,
>> +&end,
>> +&flags);
>> + pnp_add_mem_resource(dev, start, end, flags);
>> + } else if (!strnicmp(buf, "irq", 3)) {
>> + buf = pnp_get_resource_value(buf + 3,
>> + IORESOURCE_IRQ,
>> +&start,
>> + NULL,
>> +&flags);
>> + pnp_add_irq_resource(dev, start, flags);
>> + } else if (!strnicmp(buf, "dma", 3)) {
>> + buf = pnp_get_resource_value(buf + 3,
>> + IORESOURCE_DMA,
>> +&start,
>> + NULL,
>> +&flags);
>> + pnp_add_dma_resource(dev, start, flags);
>> + } else if (!strnicmp(buf, "bus", 3)) {
>> + buf = pnp_get_resource_value(buf + 3,
>> + IORESOURCE_BUS,
>> +&start,
>> +&end,
>> + NULL);
>> + pnp_add_bus_resource(dev, start, end);
>
> It makes sense to allow a user to set bus number resources just like
> any other resources, but I don't think the rest of PNP supports this
> yet. I think only PNPACPI supports bus number resources at all, so
> when you "activate" the device after setting its resources, we'll call
> pnpacpi_set_resources() and pnpacpi_encode_resources(), but
> pnpacpi_encode_resources() doesn't know how to encode the address16
> descriptor that will probably be used for bus numbers.
>
> I guess that's all right, though. We already have the same situation
> for all the address space descriptors when used for IO or MEM.
>
>> + } else
>> + break;
>> }
>> mutex_unlock(&pnp_res_mutex);
>> goto done;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] PNP: Allow resources to be set as disabled
2012-03-27 20:38 ` Bjorn Helgaas
@ 2012-04-01 16:06 ` Witold Szczeponik
0 siblings, 0 replies; 12+ messages in thread
From: Witold Szczeponik @ 2012-04-01 16:06 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-kernel
On 27/03/12 22:38, Bjorn Helgaas wrote:
[...]
>> The patch achieves this by doing two things: (1) it allows the strings
>> "disabled" and"<none>" to be used as a valid PNP resource value, and (2) when
>> assigning PNP resources, it copies the flags masked by IORESOURCE_BITS from the
>> resource's templates.
>
> These look like reasonable things to do, but (2) doesn't seem to
> depend on (1), so you might just split them into two patches.
Will do. Actually, (2) solves a problem that apparently had not been observed
yet: When *any* PNP resource was set using the "/sys/bus/pnp/*/*/resources"
interface, the IORESOURCE_BITS were always cleared, which might have been, by
coincidence, a meaningful value. However, once I started disabling IRQ lines,
I've seen error messages in the kernel logs (an IRQ's IORESOURCE_BITS are never
cleared, if I am not mistaken).
[...]
>>
>> If the second part of the patch is not applied, the resource flags are not
>> initialized properly and obscure messages in the kernel log have be seen
>
> s/be/been/
ACK. And will be moved to some other patch.
>
>> ("invalid flags").
>>
>> 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
>> @@ -311,10 +311,14 @@ static char *pnp_get_resource_value(char
>> if (flags)
>> *flags = 0;
>>
>> - /* TBD: allow for disabled resources */
>> -
>> buf = skip_spaces(buf);
>> - if (start) {
>> + if (flags&& !strnicmp(buf, "disabled", 8)) {
>> + buf += 8;
>> + *flags |= IORESOURCE_DISABLED;
>> + } else if (flags&& !strnicmp(buf, "<none>", 6)) {
>> + buf += 6;
>> + *flags |= IORESOURCE_DISABLED;
>
> What's the value in supporting both "disabled" and "<none>"? Having
> both suggests that they do different things, but it looks like they
> have the same effect.
These two values correspond to the two different ways to report "disabled"
resources by the kernel: drivers/pnp/interface.c uses "disabled" when
displaying PNP resources and "<none>" when displaying PNP options. (Maybe
the latter should be changed to "disabled", too, but this would be a
change in the ABI.)
>
>> + } else if (start) {
>> *start = simple_strtoull(buf,&buf, 0);
>> if (end) {
>> buf = skip_spaces(buf);
>> Index: linux/drivers/pnp/manager.c
>> ===================================================================
>> --- linux.orig/drivers/pnp/manager.c
>> +++ linux/drivers/pnp/manager.c
>> @@ -18,11 +18,27 @@
>>
>> DEFINE_MUTEX(pnp_res_mutex);
>>
>> +static struct resource *pnp_find_resource(struct pnp_dev *dev,
>> + unsigned char rule,
>> + unsigned long type,
>> + unsigned int bar)
>> +{
>> + struct resource *res = pnp_get_resource(dev, type, bar);
>> +
>> + /* when the resource already exists, set its resource bits from rule */
>> + if (res) {
>> + res->flags&= ~IORESOURCE_BITS;
>> + res->flags |= rule& IORESOURCE_BITS;
>> + }
>> +
>> + return res;
>> +}
>> +
>> static int pnp_assign_port(struct pnp_dev *dev, struct pnp_port *rule, int idx)
>> {
>> struct resource *res, local_res;
>>
>> - res = pnp_get_resource(dev, IORESOURCE_IO, idx);
>> + res = pnp_find_resource(dev, rule->flags, IORESOURCE_IO, idx);
>> if (res) {
>> pnp_dbg(&dev->dev, " io %d already set to %#llx-%#llx "
>> "flags %#lx\n", idx, (unsigned long long) res->start,
>> @@ -65,7 +81,7 @@ static int pnp_assign_mem(struct pnp_dev
>> {
>> struct resource *res, local_res;
>>
>> - res = pnp_get_resource(dev, IORESOURCE_MEM, idx);
>> + res = pnp_find_resource(dev, rule->flags, IORESOURCE_MEM, idx);
>> if (res) {
>> pnp_dbg(&dev->dev, " mem %d already set to %#llx-%#llx "
>> "flags %#lx\n", idx, (unsigned long long) res->start,
>> @@ -78,6 +94,7 @@ static int pnp_assign_mem(struct pnp_dev
>> res->start = 0;
>> res->end = 0;
>>
>> + /* ??? rule->flags restricted to 8 bits, all tests bogus ??? */
>> if (!(rule->flags& IORESOURCE_MEM_WRITEABLE))
>> res->flags |= IORESOURCE_READONLY;
>> if (rule->flags& IORESOURCE_MEM_CACHEABLE)
>> @@ -123,7 +140,7 @@ static int pnp_assign_irq(struct pnp_dev
>> 5, 10, 11, 12, 9, 14, 15, 7, 3, 4, 13, 0, 1, 6, 8, 2
>> };
>>
>> - res = pnp_get_resource(dev, IORESOURCE_IRQ, idx);
>> + res = pnp_find_resource(dev, rule->flags, IORESOURCE_IRQ, idx);
>> if (res) {
>> pnp_dbg(&dev->dev, " irq %d already set to %d flags %#lx\n",
>> idx, (int) res->start, res->flags);
>> @@ -182,7 +199,7 @@ static int pnp_assign_dma(struct pnp_dev
>> 1, 3, 5, 6, 7, 0, 2, 4
>> };
>>
>> - res = pnp_get_resource(dev, IORESOURCE_DMA, idx);
>> + res = pnp_find_resource(dev, rule->flags, IORESOURCE_DMA, idx);
>> if (res) {
>> pnp_dbg(&dev->dev, " dma %d already set to %d flags %#lx\n",
>> idx, (int) res->start, res->flags);
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] PNP: Allow resource flags to be set explicitly
2012-03-27 20:52 ` Bjorn Helgaas
@ 2012-04-01 16:07 ` Witold Szczeponik
0 siblings, 0 replies; 12+ messages in thread
From: Witold Szczeponik @ 2012-04-01 16:07 UTC (permalink / raw)
To: Bjorn Helgaas, linux-kernel
On 27/03/12 22:52, Bjorn Helgaas wrote:
> On Tue, Mar 20, 2012 at 2:05 PM, Witold Szczeponik
[...]
>
> I don't quite understand the illustration. Maybe it would help if you
> showed before& after output.
I will describe the patch in more details in a new version. Also, I will
submit this patch separately, for it does not fix a problem but rather
"just" makes the kernel more maintainable.
>
>> 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>"?
As for using the both, see my answer to your comments on [1/3].
Actually, only the first would be used for printing (the second can
be used for reading). The patch maintains a mask of flags already
printed (variable "mask"): once a flag has been printed, is is masked
out from further processing.
Using this logic, one can have more than one value when reading the
flags, but the first to appear in the table will be used for printing.
>
>> + 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.
This is on purpose: I wanted to have a fallback if anything else fails.
When printing the flags (cf. above), all flags are printed,
too ("res->flags" vs. "mask").
But you are right, if I were to print the unhandled flags only, I
could OR them here and restore the symmetry.
Any preferences?
>
>> + }
>>
>> return buf;
>> }
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-04-01 16:08 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-20 18:49 [PATCH 0/3] PNP: Allow PNP resources to be disabled (interface) Witold Szczeponik
2012-03-20 19:57 ` [PATCH 1/3] PNP: Simplify setting of resources Witold Szczeponik
2012-03-27 20:32 ` Bjorn Helgaas
2012-04-01 16:05 ` Witold Szczeponik
2012-03-20 20:00 ` [PATCH 2/3] PNP: Allow resources to be set as disabled Witold Szczeponik
2012-03-27 20:38 ` Bjorn Helgaas
2012-04-01 16:06 ` Witold Szczeponik
2012-03-20 20:05 ` [PATCH 3/3] PNP: Allow resource flags to be set explicitly Witold Szczeponik
2012-03-27 20:52 ` Bjorn Helgaas
2012-04-01 16:07 ` Witold Szczeponik
2012-03-27 20:57 ` [PATCH 0/3] PNP: Allow PNP resources to be disabled (interface) Bjorn Helgaas
2012-04-01 16:04 ` Witold Szczeponik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).