PNP: Allow PNP resources to be disabled (interface)
cover PNP: Allow PNP resources to be disabled (interface)
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
PATCH 1/3 PNP: Simplify setting of resources
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.
drivers/pnp/interface.c +73 -37
@@ -298,6 +298,39 @@ static ssize_t pnp_show_current_resourcereturn 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_resourcesstruct 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_resourceswhile (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;
Re: [PATCH 1/3] PNP: Simplify setting of resources
Quoted 11 lines · Witold Szczeponik
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.
Quoted 101 lines
Signed-off-by Witold Szczeponik <[email protected]>Index:
linux/drivers/pnp/interface.c
===================================================================drivers/pnp/interface.c +37
-32@@ -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);
drivers/pnp/interface.c static ssize_t pnp_set_current_resources · line 400
Quoted 10 lines
drivers/pnp/interface.c +5
-5- 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".
Quoted 19 lines
drivers/pnp/interface.c +19
-0+ 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,
drivers/pnp/interface.c static ssize_t pnp_set_current_resources · line 429
Quoted 10 lines
drivers/pnp/interface.c +10
-0+ 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.
drivers/pnp/interface.c static ssize_t pnp_set_current_resources · line 434
Quoted 5 lines
drivers/pnp/interface.c +2
-0+ } else + break; } mutex_unlock(&pnp_res_mutex); goto done;
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ¥Šwÿº{.nÇ+‰·¥Š{±þG«�éÿŠ{ayº ʇڙë,j¢f£¢·hš�ï�êÿ‘êçz_è®(階ŠÝ¢j"�ú¶m§ÿÿ¾«þG«�éÿ¢¸?™¨èÚ&£ø§~�á¶iO•æ¬z·švØ^¶m§ÿÿà ÿ¶ìÿ¢¸?–I¥
Re: [PATCH 1/3] PNP: Simplify setting of resources
On 27/03/12 22:32, Bjorn Helgaas wrote:
[...]
Quoted 6 lines
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<[email protected]>
I will send an updated version of the patch series also to Len and linux-acpi. Thanks for pointing this out to me!
Quoted 105 lines
Quoted 105 lines
Signed-off-by Witold Szczeponik<[email protected]>Index:
linux/drivers/pnp/interface.c
===================================================================drivers/pnp/interface.c +37
-36@@ -298,6 +298,39 @@ static ssize_t pnp_show_current_resourcereturn 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_resourcesstruct 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_resourceswhile (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 ; - }
Quoted 8 lines
drivers/pnp/interface.c static ssize_t pnp_set_current_resources · line 400
Quoted 6 lines
drivers/pnp/interface.c +5
-1-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.
Quoted 47 lines
Quoted 20 lines
drivers/pnp/interface.c +19
-0+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,drivers/pnp/interface.c static ssize_t pnp_set_current_resources · line 429
Quoted 10 lines
drivers/pnp/interface.c +10
-0+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()andpnpacpi_encode_resources(), butpnpacpi_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.
Quoted 5 lines
drivers/pnp/interface.c +2
-0+ }else +break ; }mutex_unlock (&pnp_res_mutex); goto done;
PATCH 2/3 PNP: Allow resources to be set as disabled
In reply to Witold SzczeponikThis 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.
drivers/pnp/interface.c +7 -3
@@ -311,10 +311,14 @@ static char *pnp_get_resource_value(charif (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);
drivers/pnp/manager.c +21 -4
@@ -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_dev5 ,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);
Re: [PATCH 2/3] PNP: Allow resources to be set as disabled
Quoted 8 lines · Witold Szczeponik
This patch allows to set PNP resources as "disabled". As such, the patch is a follow-up to commit
18fd470a48396c8795ba7256c5973e92ffa25cb3where 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_BITSfrom 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.
Quoted 6 lines
The latter is necessary because the resource settings
require properIORESOURCE_BITSwhich 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.)
Quoted 7 lines
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/
Quoted 16 lines
("invalid flags").
The patch is applied against Linux 3.3.x.
Signed-off-by Witold Szczeponik <[email protected]>Index:
linux/drivers/pnp/interface.c
===================================================================drivers/pnp/interface.c +0
-0@@ -311,10 +311,14 @@ static char *pnp_get_resource_value(char if (flags) *flags = 0;
drivers/pnp/interface.c static char *pnp_get_resource_value(char · line 320
Quoted 10 lines
drivers/pnp/interface.c +6
-3- /* 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.
Quoted 73 lines
drivers/pnp/interface.c +1
-0+ } else if (start) { *start = simple_strtoull(buf, &buf, 0); if (end) { buf = skip_spaces(buf);Index:
linux/drivers/pnp/manager.c
===================================================================drivers/pnp/manager.c +17
-1@@ -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,drivers/pnp/manager.c +1
-1@@ -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,drivers/pnp/manager.c +1
-0@@ -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_dev5, 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);drivers/pnp/manager.c +0
-0@@ -182,7 +199,7 @@ static int pnp_assign_dma(struct pnp_dev1, 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);
Re: [PATCH 2/3] PNP: Allow resources to be set as disabled
On 27/03/12 22:38, Bjorn Helgaas wrote:
[...]
Quoted 7 lines
Quoted 4 lines
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_BITSfrom 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).
[...]
Quoted 5 lines
Quoted 3 lines
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.
Quoted 17 lines
Quoted 17 lines
("invalid flags").
The patch is applied against Linux 3.3.x.
Signed-off-by Witold Szczeponik<[email protected]>Index:
linux/drivers/pnp/interface.c
===================================================================drivers/pnp/interface.c +0
-0@@ -311,10 +311,14 @@ static char *pnp_get_resource_value(charif (flags) *flags =0 ;
Quoted 14 lines
drivers/pnp/interface.c static char *pnp_get_resource_value(char · line 320
Quoted 10 lines
drivers/pnp/interface.c +6
-3-/* 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.)
Quoted 75 lines
Quoted 74 lines
drivers/pnp/interface.c +1
-0+ }else if (start) { *start =simple_strtoull (buf,&buf,0 );if (end) { buf =skip_spaces (buf);Index:
linux/drivers/pnp/manager.c
===================================================================drivers/pnp/manager.c +21
-4@@ -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_dev5 ,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);
PATCH 3/3 PNP: Allow resource flags to be set explicitly
In reply to Witold SzczeponikThe 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.
drivers/pnp/interface.c +90 -21
@@ -242,6 +242,29 @@ static ssize_t pnp_show_options(struct dreturn 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_resourcestruct 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_resourcepnp_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; }
Re: [PATCH 3/3] PNP: Allow resource flags to be set explicitly
Quoted 22 lines · Witold Szczeponik
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
Quoted 7 lines
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 flags0x05001
I don't quite understand the illustration. Maybe it would help if you showed before & after output.
Quoted 22 lines
The patch is applied against Linux 3.3.x.
Signed-off-by Witold Szczeponik <[email protected]>Index:
linux/drivers/pnp/interface.c
===================================================================drivers/pnp/interface.c +8
-0@@ -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 {
drivers/pnp/interface.c static ssize_t pnp_show_options(struct d · line 262
Quoted 10 lines
drivers/pnp/interface.c +10
-0+ 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>"?
Quoted 90 lines
drivers/pnp/interface.c +46
-20+ 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);
drivers/pnp/interface.c static char *pnp_get_resource_value(char · line 378
Quoted 6 lines
drivers/pnp/interface.c +2
-1@@ -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;Quoted 13 lines
drivers/pnp/interface.c +13
-0+ 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);
drivers/pnp/interface.c static char *pnp_get_resource_value(char · line 401
Quoted 10 lines
drivers/pnp/interface.c +10
-0+ 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.
Quoted 4 lines
drivers/pnp/interface.c +1
-0+ } return buf; }
Re: [PATCH 3/3] PNP: Allow resource flags to be set explicitly
Quoted 1 line · Bjorn Helgaas
On Tue, Mar 20, 2012 at 2:05 PM, Witold Szczeponik
[...]
Quoted 3 lines
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.
Quoted 30 lines
Quoted 30 lines
The patch is applied against Linux 3.3.x.
Signed-off-by Witold Szczeponik<[email protected]>Index:
linux/drivers/pnp/interface.c
===================================================================drivers/pnp/interface.c +15
-0@@ -242,6 +242,29 @@ static ssize_t pnp_show_options(struct dreturn 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" ),
Quoted 8 lines
drivers/pnp/interface.c static ssize_t pnp_show_options(struct d · line 262
Quoted 3 lines
drivers/pnp/interface.c +3
-0+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.
Quoted 124 lines
Quoted 91 lines
drivers/pnp/interface.c +46
-20+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_resourcestruct 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_resourcepnp_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);drivers/pnp/interface.c static char *pnp_get_resource_value(char · line 378
Quoted 6 lines
drivers/pnp/interface.c +2
-1@@ -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;Quoted 21 lines
drivers/pnp/interface.c +21
-0+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 )) {
Quoted 8 lines
drivers/pnp/interface.c static char *pnp_get_resource_value(char · line 401
Quoted 2 lines
drivers/pnp/interface.c +2
-0+ 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?
Quoted 6 lines
Quoted 5 lines
drivers/pnp/interface.c +1
-0+ }return buf; }
Quoted 11 lines · Witold Szczeponik
Hello everybody,
this simple patch series continues the work begun in commit
18fd470a48396c8795ba7256c5973e92ffa25cb3where 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?
Quoted 16 lines
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 flagsThe patches are applied against Linux 3.3.x.
Comments are, as always, welcome. :-)
--- Witold
On 27/03/12 22:57, Bjorn Helgaas wrote:
[...]
Quoted 8 lines
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