[PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Witold Szczeponik]

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 devices need 
to have their IRQs disabled in order to support all the devices the 600E has. 

To better understand the motivation, let's look at an excerpt from the 600E's 
DSDT:

    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 one can see, the IRQ line for the last option is empty/disabled.  Also, both 
options share the same priority, meaning they are equal alternatives.  In order 
to be able to use the IRQ 7 for some other device, it is necessary to select 
the second option, which can be done with the patch series applied.

To this end, some preparatory work is done, simplifying the code, and fixing a
potential issue when explicitely assigning resources. 

Here is a brief description of these patches. 

[1/3] - Factor out common some code
[2/3] - Perform the actual setting
[3/3] - Handle IORESOURCE_BITS in resource allocation

The patches are applied against Linux 3.5.x. 

Comments are, as always, welcome.  If the patches should be sent to someone
else, please let me know. 


--- Witold


Changes from previous versions:

V3 -> V2: Added Bjorn Helgaas as "Reviewed by"
          No changes in the code itself
          Based on Linux 3.5.x

V1 -> V2: Split [V1 2/3] into [V2 2/3] and [V2 3/3]
          Removed [V1 3/3], will be submitted separately
          Wrote more comments in response to the previous version
          Sent to a broader audience
          (https://lkml.org/lkml/2012/4/11/442)

V1:       Initial version 
          (https://lkml.org/lkml/2012/3/20/358)

[PATCH V3 1/3] PNP: Simplify setting of resources

[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.5.x.


Signed-off-by: Witold Szczeponik <Witold.Szczeponik@gmx.net>
Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>


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,37 @@ 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;

[PATCH V3 2/3] PNP: Allow resources to be set as disabled

[Witold Szczeponik]

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 made possible.  

The patch achieves this by allowing the strings "disabled" and "<none>" to be 
used as a valid PNP resource value.  The value "disabled" is used because it
also appears when reporting PNP resources, whereas the string "<none>" is
used when reporting PNP options.

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.  

The patch is applied against Linux 3.5.x.


Signed-off-by: Witold Szczeponik <Witold.Szczeponik@gmx.net>
Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>


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);

[PATCH V3 3/3] PNP: Handle IORESOURCE_BITS in resource allocation

[Witold Szczeponik]

The patch copies the flags masked by IORESOURCE_BITS from a resource's 
template.  This 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.)

If the patch is not applied, the resource flags are not initialized properly 
and obscure messages in the kernel log have been seen ("invalid flags").  

The patch is applied against Linux 3.5.x.


Signed-off-by: Witold Szczeponik <Witold.Szczeponik@gmx.net>
Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>


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);

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Rafael J. Wysocki]

Hi,

On Sunday, July 29, 2012, Witold Szczeponik 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 devices need 
> to have their IRQs disabled in order to support all the devices the 600E has. 
> 
> To better understand the motivation, let's look at an excerpt from the 600E's 
> DSDT:
> 
>     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 one can see, the IRQ line for the last option is empty/disabled.  Also, both 
> options share the same priority, meaning they are equal alternatives.  In order 
> to be able to use the IRQ 7 for some other device, it is necessary to select 
> the second option, which can be done with the patch series applied.

Do I understand correctly that you want to disable those things through
sysfs?

Rafael

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Witold Szczeponik]

On 29/07/12 21:22, Rafael J. Wysocki wrote:

> Hi,
> 

[... long snip ...]

> 
> Do I understand correctly that you want to disable those things through
> sysfs?
> 
> Rafael
> 

Hi Rafael, 

the aim is to select a PNP ACPI option where resources can be disabled
(or are not needed).  E.g., the parallel port of the 600E can be used
with and without IRQ lines.  The means to allow for this is to use the
sysfs interface to select disabled resources (just like any other 
resource value).  In https://lkml.org/lkml/2011/7/3/41, I used the 
following example:

  echo disable > /sys/bus/pnp/devices/$device/resources
  echo clear > /sys/bus/pnp/devices/$device/resources
  echo set irq disabled > /sys/bus/pnp/devices/$device/resources
  echo fill > /sys/bus/pnp/devices/$device/resources
  echo activate > /sys/bus/pnp/devices/$device/resources

The third line is made possible by the patch series.  All other
lines are already implemented. 

--- Witold

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Borislav Petkov]

On Sun, Jul 29, 2012 at 09:31:53PM +0200, Witold Szczeponik wrote:
> the aim is to select a PNP ACPI option where resources can be disabled
> (or are not needed).  E.g., the parallel port of the 600E can be used
> with and without IRQ lines.  The means to allow for this is to use the
> sysfs interface to select disabled resources (just like any other 
> resource value).  In https://lkml.org/lkml/2011/7/3/41, I used the 
> following example:
> 
>   echo disable > /sys/bus/pnp/devices/$device/resources
>   echo clear > /sys/bus/pnp/devices/$device/resources
>   echo set irq disabled > /sys/bus/pnp/devices/$device/resources
>   echo fill > /sys/bus/pnp/devices/$device/resources
>   echo activate > /sys/bus/pnp/devices/$device/resources
> 
> The third line is made possible by the patch series.  All other
> lines are already implemented.

Shouldn't this be rather "disable_irq" or something which is a single
word and thus would simplify parsing a lot?

Also, <Documentation/filesystems/sysfs.txt> says

"Attributes should be ASCII text files, preferably with only one value
per file. It is noted that it may not be efficient to contain only one
value per file, so it is socially acceptable to express an array of
values of the same type."

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Witold Szczeponik]

> On Sun, Jul 29, 2012 at 09:31:53PM +0200, Witold Szczeponik wrote:
> > the aim is to select a PNP ACPI option where resources can be disabled
> > (or are not needed).  E.g., the parallel port of the 600E can be used
> > with and without IRQ lines.  The means to allow for this is to use the
> > sysfs interface to select disabled resources (just like any other 
> > resource value).  In https://lkml.org/lkml/2011/7/3/41, I used the 
> > following example:
> > 
> >   echo disable > /sys/bus/pnp/devices/$device/resources
> >   echo clear > /sys/bus/pnp/devices/$device/resources
> >   echo set irq disabled > /sys/bus/pnp/devices/$device/resources
> >   echo fill > /sys/bus/pnp/devices/$device/resources
> >   echo activate > /sys/bus/pnp/devices/$device/resources
> > 
> > The third line is made possible by the patch series.  All other
> > lines are already implemented.
> 
> Shouldn't this be rather "disable_irq" or something which is a single
> word and thus would simplify parsing a lot?
> 
> Also, <Documentation/filesystems/sysfs.txt> says
> 
> "Attributes should be ASCII text files, preferably with only one value
> per file. It is noted that it may not be efficient to contain only one
> value per file, so it is socially acceptable to express an array of
> values of the same type."
> 
> Thanks.
> 
> -- 
> Regards/Gruss,
>     Boris.

Hi Boris, 

the patch series is about adding the term "disabled" (or "<none") to
the list of possible values for resources.  The current state of the
kernel ABI already allows to use the following statement (cf. DSDT
excerpt from https://lkml.org/lkml/2011/7/3/41): 

  echo set irq 7 > /sys/bus/pnp/devices/$device/resources

i.e., the "resources" file can already parse all the values necessary for setting PNP values.  

The patch series is not about adding a new ABI or changing an existing ABI.  It is about extending the existing one to be able to handle the term "disabled" (or "<none>") as a special for a resource value. 

--- Witold

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Witold Szczeponik]

Hi Len and Bjorn, 

is there anything that needs to be done in order for the patch series
to be included in either 3.6 or 3.7?  Except for the (viable) question
as to whether or not a sysfs interface should accept complex inputs
(as it currently does and which is not introduced by this patch series), 
there have been no comments since March, when I made some changes due 
to Bjorn's well made observations. 

The patches extend Linux's PNP capabilities and are required for some
hardware, like the IBM ThinkPad 600E (and similar machines). 

All other users of the ABI should not see any regressions (the ABI
stays the same).  

--- Witold


> 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 devices
> need 
> to have their IRQs disabled in order to support all the devices the 600E
> has. 
> 
> To better understand the motivation, let's look at an excerpt from the
> 600E's 
> DSDT:
> 
>     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 one can see, the IRQ line for the last option is empty/disabled.  Also,
> both 
> options share the same priority, meaning they are equal alternatives.  In
> order 
> to be able to use the IRQ 7 for some other device, it is necessary to
> select 
> the second option, which can be done with the patch series applied.
> 
> To this end, some preparatory work is done, simplifying the code, and
> fixing a
> potential issue when explicitely assigning resources. 
> 
> Here is a brief description of these patches. 
> 
> [1/3] - Factor out common some code
> [2/3] - Perform the actual setting
> [3/3] - Handle IORESOURCE_BITS in resource allocation
> 
> The patches are applied against Linux 3.5.x. 
> 
> Comments are, as always, welcome.  If the patches should be sent to
> someone
> else, please let me know. 
> 
> 
> --- Witold
> 
> 
> Changes from previous versions:
> 
> V3 -> V2: Added Bjorn Helgaas as "Reviewed by"
>           No changes in the code itself
>           Based on Linux 3.5.x
> 
> V1 -> V2: Split [V1 2/3] into [V2 2/3] and [V2 3/3]
>           Removed [V1 3/3], will be submitted separately
>           Wrote more comments in response to the previous version
>           Sent to a broader audience
>           (https://lkml.org/lkml/2012/4/11/442)
> 
> V1:       Initial version 
>           (https://lkml.org/lkml/2012/3/20/358)

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Witold Szczeponik]

Hi all, 

the original mail should have been sent out as a reply to 
https://lkml.org/lkml/2012/7/29/85, but it seems as if I clicked the
wrong button.  

Apologies for any inconvenience. 

--- Witold

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Rafael J. Wysocki]

On Monday, July 30, 2012, Borislav Petkov wrote:
> On Sun, Jul 29, 2012 at 09:31:53PM +0200, Witold Szczeponik wrote:
> > the aim is to select a PNP ACPI option where resources can be disabled
> > (or are not needed).  E.g., the parallel port of the 600E can be used
> > with and without IRQ lines.  The means to allow for this is to use the
> > sysfs interface to select disabled resources (just like any other 
> > resource value).  In https://lkml.org/lkml/2011/7/3/41, I used the 
> > following example:
> > 
> >   echo disable > /sys/bus/pnp/devices/$device/resources
> >   echo clear > /sys/bus/pnp/devices/$device/resources
> >   echo set irq disabled > /sys/bus/pnp/devices/$device/resources
> >   echo fill > /sys/bus/pnp/devices/$device/resources
> >   echo activate > /sys/bus/pnp/devices/$device/resources
> > 
> > The third line is made possible by the patch series.  All other
> > lines are already implemented.
> 
> Shouldn't this be rather "disable_irq" or something which is a single
> word and thus would simplify parsing a lot?

Or just "irq", which isn't going to be confused with anything else it seems.

Thanks,
Rafael

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Witold Szczeponik]

On 02/08/12 22:09, Rafael J. Wysocki wrote:
> On Monday, July 30, 2012, Borislav Petkov wrote:
>> On Sun, Jul 29, 2012 at 09:31:53PM +0200, Witold Szczeponik wrote:
>>> the aim is to select a PNP ACPI option where resources can be disabled
>>> (or are not needed).  E.g., the parallel port of the 600E can be used
>>> with and without IRQ lines.  The means to allow for this is to use the
>>> sysfs interface to select disabled resources (just like any other 
>>> resource value).  In https://lkml.org/lkml/2011/7/3/41, I used the 
>>> following example:
>>>
>>>   echo disable > /sys/bus/pnp/devices/$device/resources
>>>   echo clear > /sys/bus/pnp/devices/$device/resources
>>>   echo set irq disabled > /sys/bus/pnp/devices/$device/resources
>>>   echo fill > /sys/bus/pnp/devices/$device/resources
>>>   echo activate > /sys/bus/pnp/devices/$device/resources
>>>
>>> The third line is made possible by the patch series.  All other
>>> lines are already implemented.
>>
>> Shouldn't this be rather "disable_irq" or something which is a single
>> word and thus would simplify parsing a lot?
> 
> Or just "irq", which isn't going to be confused with anything else it seems.
> 
> Thanks,
> Rafael
> 

Hi Rafael, 

the code in "drivers/pnp/interface.c" implements a (non-trivial) interface
which accepts the keywords "disable", "activate", "fill", "auto", "clear",
and "get" as simple, one word commands.  The remaining "set" command is
more complex, for it determines which resource is to be set ("io", "mem",
"irq", "dma", and "bus"), followed by the actual value(s) of this resource
(e.g., "0x0200-0x021f", or "7"). 

The patch series allows to use the term "disabled" or "<none>" as a 
resource value (c.f. my example above) when needed (c.f. my motivation for
the patch series). 

We could, of course, change the parser in "interface.c", but this would 
change the ABI, I am afraid.  Something that I'd rather not do... 

I hope, this makes the scope of the patch series clear(er).  

--- Witold

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Rafael J. Wysocki]

On Thursday, August 02, 2012, Witold Szczeponik wrote:
> On 02/08/12 22:09, Rafael J. Wysocki wrote:
> > On Monday, July 30, 2012, Borislav Petkov wrote:
> >> On Sun, Jul 29, 2012 at 09:31:53PM +0200, Witold Szczeponik wrote:
> >>> the aim is to select a PNP ACPI option where resources can be disabled
> >>> (or are not needed).  E.g., the parallel port of the 600E can be used
> >>> with and without IRQ lines.  The means to allow for this is to use the
> >>> sysfs interface to select disabled resources (just like any other 
> >>> resource value).  In https://lkml.org/lkml/2011/7/3/41, I used the 
> >>> following example:
> >>>
> >>>   echo disable > /sys/bus/pnp/devices/$device/resources
> >>>   echo clear > /sys/bus/pnp/devices/$device/resources
> >>>   echo set irq disabled > /sys/bus/pnp/devices/$device/resources
> >>>   echo fill > /sys/bus/pnp/devices/$device/resources
> >>>   echo activate > /sys/bus/pnp/devices/$device/resources
> >>>
> >>> The third line is made possible by the patch series.  All other
> >>> lines are already implemented.
> >>
> >> Shouldn't this be rather "disable_irq" or something which is a single
> >> word and thus would simplify parsing a lot?
> > 
> > Or just "irq", which isn't going to be confused with anything else it seems.
> > 
> > Thanks,
> > Rafael
> > 
> 
> Hi Rafael, 
> 
> the code in "drivers/pnp/interface.c" implements a (non-trivial) interface
> which accepts the keywords "disable", "activate", "fill", "auto", "clear",
> and "get" as simple, one word commands.  The remaining "set" command is
> more complex, for it determines which resource is to be set ("io", "mem",
> "irq", "dma", and "bus"), followed by the actual value(s) of this resource
> (e.g., "0x0200-0x021f", or "7"). 
> 
> The patch series allows to use the term "disabled" or "<none>" as a 
> resource value (c.f. my example above) when needed (c.f. my motivation for
> the patch series). 
> 
> We could, of course, change the parser in "interface.c", but this would 
> change the ABI, I am afraid.  Something that I'd rather not do... 

Still, you _are_ doing that by extending the ABI, aren't you?

> I hope, this makes the scope of the patch series clear(er).

Yes, it does, thanks.

My opinion is that the whole interface is wrong and should be changed.  How to
do that is a different matter that would require some consideration.  Perhaps
the least painful way would be to add a new, hopefully better, interface along
with the old one and then deprecate the latter at one point.

Now, since I don't like the existing interface, I'd prefer it not to be
extended.

Thanks,
Rafael

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Witold Szczeponik]

On 02/08/12 23:40, Rafael J. Wysocki wrote:

> On Thursday, August 02, 2012, Witold Szczeponik wrote:
>> On 02/08/12 22:09, Rafael J. Wysocki wrote:
>>> On Monday, July 30, 2012, Borislav Petkov wrote:
>>>> On Sun, Jul 29, 2012 at 09:31:53PM +0200, Witold Szczeponik wrote:

[... snip ...]

>>>>
>>>> Shouldn't this be rather "disable_irq" or something which is a single
>>>> word and thus would simplify parsing a lot?
>>>
>>> Or just "irq", which isn't going to be confused with anything else it seems.
>>>
>>> Thanks,
>>> Rafael
>>>
>>
>> Hi Rafael, 
>>
>> the code in "drivers/pnp/interface.c" implements a (non-trivial) interface
>> which accepts the keywords "disable", "activate", "fill", "auto", "clear",
>> and "get" as simple, one word commands.  The remaining "set" command is
>> more complex, for it determines which resource is to be set ("io", "mem",
>> "irq", "dma", and "bus"), followed by the actual value(s) of this resource
>> (e.g., "0x0200-0x021f", or "7"). 
>>
>> The patch series allows to use the term "disabled" or "<none>" as a 
>> resource value (c.f. my example above) when needed (c.f. my motivation for
>> the patch series). 
>>
>> We could, of course, change the parser in "interface.c", but this would 
>> change the ABI, I am afraid.  Something that I'd rather not do... 
> 
> Still, you _are_ doing that by extending the ABI, aren't you?

As the special value "disabled" is available as of these patches, one could
consider this an extension.  I agree. 

> 
>> I hope, this makes the scope of the patch series clear(er).
> 
> Yes, it does, thanks.
> 
> My opinion is that the whole interface is wrong and should be changed.  How to
> do that is a different matter that would require some consideration.  Perhaps
> the least painful way would be to add a new, hopefully better, interface along
> with the old one and then deprecate the latter at one point.

Personally, I too think that the PNP ABI in sysfs has its rough edges.  However, 
as with the deprecation of any existing ABI, this would require a new ABI first, 
then some time where the old and new ABI live in co-existence, and then to remove 
the currently available ABI. 

> 
> Now, since I don't like the existing interface, I'd prefer it not to be
> extended.

The current ABI does not allow for the kernel to run on my hardware: this is 
a/the problem.  The proposed extension fixes the problem.  

While I agree with your first statement, for the time being I do not see a 
better solution other than to extend the ABI. 

At this point I am repeating my "call for comments" to the community. :-) 

--- Witold

> 
> Thanks,
> Rafael
> 

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Witold Szczeponik]

Hi Rafael, 

what about the patches 1 and 3 which do not make any changes to the ABI?  The first patch simplifies the code, while the third patch fixes a problem in the PNP resource allocation.  Any chances to have them included in 3.7?

--- Witold

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Rafael J. Wysocki]

Hi,

On Sunday, September 16, 2012, Witold Szczeponik wrote:
> Hi Rafael, 
> 
> what about the patches 1 and 3 which do not make any changes to the ABI?
> The first patch simplifies the code, while the third patch fixes a problem in
> the PNP resource allocation.  Any chances to have them included in 3.7?

They would be fine as far as I'm concerned, but I'm not maintaining that part
of the kernel (at least at the moment).

I'm not sure who's the maintainer of it, to be honest.

Thanks,
Rafael

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Witold Szczeponik]

On 18/09/12 23:42, Rafael J. Wysocki wrote:
> Hi,
> 
> On Sunday, September 16, 2012, Witold Szczeponik wrote:
>> Hi Rafael, 
>>
>> what about the patches 1 and 3 which do not make any changes to the ABI?
>> The first patch simplifies the code, while the third patch fixes a problem in
>> the PNP resource allocation.  Any chances to have them included in 3.7?
> 
> They would be fine as far as I'm concerned, but I'm not maintaining that part
> of the kernel (at least at the moment).
> 
> I'm not sure who's the maintainer of it, to be honest.

According to the maintainer list, this would be Len and Bjorn.  
Hence...


Len and Bjorn, 

any chances to include the patches in 3.7?  I checked them 
against 3.6 and they applied without any change.  Or should 
I resend them individually? 

Thanks. 

--- Witold

> 
> Thanks,
> Rafael
> 

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Witold Szczeponik]

On 03/10/12 17:57, Witold Szczeponik wrote:

[...]

> 
> Len and Bjorn, 
> 
> any chances to include the patches in 3.7?  I checked them 
> against 3.6 and they applied without any change.  Or should 
> I resend them individually? 
> 
> Thanks. 
> 
> --- Witold
> 

Hi Len, hello Bjorn, 

the patches submitted in https://lkml.org/lkml/2012/7/29/85 have
not been objected by anyone, make the code simpler, and fix a 
current problem when using the "/sys/bus/pnp/devices/*/resources" 
interface.  I have been running them since at least Linux 3.0, 
with adapted changes in previous versions.  I have not seen any
regressions and I do not expect any regressions from these
patches. 

Bjorn had reviewed them and I am wondering what I can do to have 
them included in (one of) the next Linux kernel(s).

If there is any information that you need before an inclusion is
possible, just let me know. 

--- Witold

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Witold Szczeponik]

On 18/09/12 23:42, Rafael J. Wysocki wrote:

> Hi,
> 
> On Sunday, September 16, 2012, Witold Szczeponik wrote:
>> Hi Rafael, 
>>
>> what about the patches 1 and 3 which do not make any changes to the ABI?
>> The first patch simplifies the code, while the third patch fixes a problem in
>> the PNP resource allocation.  Any chances to have them included in 3.7?
> 
> They would be fine as far as I'm concerned, but I'm not maintaining that part
> of the kernel (at least at the moment).
> 
> I'm not sure who's the maintainer of it, to be honest.

Well, according to https://lkml.org/lkml/2012/10/19/375, that would be you
(together with Bjorn and Len).  :-)  Hence I am resending the request for
inclusion to you as well.  PNP code went, if I am not mistaken, traditionally
through Len's tree.  

--- Witold

> 
> Thanks,
> Rafael
> 

Re: [PATCH V3 0/3] PNP: Allow PNP resources to be disabled (interface)

[Rafael J. Wysocki]

On Friday 19 of October 2012 21:02:20 Witold Szczeponik wrote:
> On 18/09/12 23:42, Rafael J. Wysocki wrote:
> 
> > Hi,
> > 
> > On Sunday, September 16, 2012, Witold Szczeponik wrote:
> >> Hi Rafael, 
> >>
> >> what about the patches 1 and 3 which do not make any changes to the ABI?
> >> The first patch simplifies the code, while the third patch fixes a problem in
> >> the PNP resource allocation.  Any chances to have them included in 3.7?
> > 
> > They would be fine as far as I'm concerned, but I'm not maintaining that part
> > of the kernel (at least at the moment).
> > 
> > I'm not sure who's the maintainer of it, to be honest.
> 
> Well, according to https://lkml.org/lkml/2012/10/19/375, that would be you
> (together with Bjorn and Len).  :-)  Hence I am resending the request for
> inclusion to you as well.  PNP code went, if I am not mistaken, traditionally
> through Len's tree.  

OK, so can you please resend them?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.