[PATCH 1/2] PNP: Add a release method to system resource driver

[PATCH 1/2] PNP: Add a release method to system resource driver

[Matthew Garrett]

This could conceivably be hotpluggable, and we may want to displace it
from devices under certain circustances, so add a release method to hand
back the resources.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/pnp/system.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/pnp/system.c b/drivers/pnp/system.c
index 49c1720..b0a50f2 100644
--- a/drivers/pnp/system.c
+++ b/drivers/pnp/system.c
@@ -52,7 +52,17 @@ static void reserve_range(struct pnp_dev *dev, struct resource *r, int port)
 		 res ? "has been" : "could not be");
 }
 
-static void reserve_resources_of_dev(struct pnp_dev *dev)
+static void free_range(struct pnp_dev *dev, struct resource *r, int port)
+{
+	resource_size_t start = r->start, end = r->end;
+
+	if (port)
+		release_region(start, end - start + 1);
+	else
+		release_mem_region(start, end - start + 1);
+}
+
+static void handle_resources_of_dev(struct pnp_dev *dev, bool free)
 {
 	struct resource *res;
 	int i;
@@ -75,29 +85,41 @@ static void reserve_resources_of_dev(struct pnp_dev *dev)
 		if (res->end < res->start)
 			continue;	/* invalid */
 
-		reserve_range(dev, res, 1);
+		if (free)
+			free_range(dev, res, 1);
+		else
+			reserve_range(dev, res, 1);
 	}
 
 	for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_MEM, i)); i++) {
 		if (res->flags & IORESOURCE_DISABLED)
 			continue;
 
-		reserve_range(dev, res, 0);
+		if (free)
+			free_range(dev, res, 0);
+		else
+			reserve_range(dev, res, 0);
 	}
 }
 
 static int system_pnp_probe(struct pnp_dev *dev,
 			    const struct pnp_device_id *dev_id)
 {
-	reserve_resources_of_dev(dev);
+	handle_resources_of_dev(dev, false);
 	return 0;
 }
 
+static void system_pnp_remove(struct pnp_dev *dev)
+{
+	handle_resources_of_dev(dev, true);
+}
+
 static struct pnp_driver system_pnp_driver = {
 	.name     = "system",
 	.id_table = pnp_dev_table,
 	.flags    = PNP_DRIVER_RES_DO_NOT_CHANGE,
 	.probe    = system_pnp_probe,
+	.remove   = system_pnp_remove,
 };
 
 static int __init pnp_system_init(void)
-- 
1.7.11.4

[PATCH 2/2] PNP: Unbind drivers if the new driver matches _HID rather than _CID

[Matthew Garrett]

ACPIPNP devices may have two levels of ID - the HID (a single string
defining the hardware) and the CIDs (zero or more strings defining
interfaces compatible with the hardware). If a driver matching a CID is
bound first, it will be impossible to bind a driver that matches the HID
despite it being more specific. This has been seen in the wild with
platforms that define an IPMI device as:

                Device (NIPM)
                {
                    Name (_HID, EisaId ("IPI0001"))
                    Name (_CID, EisaId ("PNP0C01"))

resulting in the device being claimed by the generic motherboard resource
driver binding to the PNP0C01 CID. The IPMI driver attempts to bind later
and fails, preventing the IPMI device from being made available to the ACPI
layer.

This can be avoided at driver probe time by detaching the existing driver
if the new driver matches the device HID. Since each device can only have
a single HID this will only permit more specific drivers to dislodge more
generic drivers.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/pnp/driver.c | 42 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index 00e9403..8d22836 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -25,6 +25,14 @@ static int compare_func(const char *ida, const char *idb)
 	return 1;
 }
 
+static int compare_single_pnp_id(const char *ida, const char *idb)
+{
+	if (memcmp(ida, idb, 3) == 0)
+		if (compare_func(ida, idb) == 1)
+			return 1;
+	return 0;
+}
+
 int compare_pnp_id(struct pnp_id *pos, const char *id)
 {
 	if (!pos || !id || (strlen(id) != 7))
@@ -32,9 +40,8 @@ int compare_pnp_id(struct pnp_id *pos, const char *id)
 	if (memcmp(id, "ANYDEVS", 7) == 0)
 		return 1;
 	while (pos) {
-		if (memcmp(pos->id, id, 3) == 0)
-			if (compare_func(pos->id, id) == 1)
-				return 1;
+		if (compare_single_pnp_id(pos->id, id) == 1)
+			return 1;
 		pos = pos->next;
 	}
 	return 0;
@@ -56,6 +63,22 @@ static const struct pnp_device_id *match_device(struct pnp_driver *drv,
 	return NULL;
 }
 
+static int match_hid(struct pnp_driver *drv, struct pnp_dev *dev)
+{
+	const struct pnp_device_id *drv_id = drv->id_table;
+
+	if (!drv_id)
+		return 0;
+
+	while (&drv_id->id) {
+		/* The first ID is _HID */
+		if (compare_single_pnp_id(dev->id->id, drv_id->id))
+			return 1;
+		drv_id++;
+	}
+	return 0;
+}
+
 int pnp_device_attach(struct pnp_dev *pnp_dev)
 {
 	spin_lock(&pnp_lock);
@@ -151,6 +174,19 @@ static int pnp_bus_match(struct device *dev, struct device_driver *drv)
 
 	if (match_device(pnp_drv, pnp_dev) == NULL)
 		return 0;
+
+	/*
+	 * ACPIPNP offers two levels of device ID - HID and CID. HID defines
+	 * the specific device ID while CID represents the device
+	 * compatibility IDs. If a device is matched by a compatibility ID
+	 * first, it will be impossible for a hardware-specific driver to
+	 * bind since there will already be a driver. We can handle this case
+	 * by unbinding the original driver if the device has one bound and
+	 * if the new driver matches the HID rather than a compatibility ID.
+	 */
+	if (dev->driver && match_hid(pnp_drv, pnp_dev))
+		device_release_driver(dev);
+
 	return 1;
 }
 
-- 
1.7.11.4

Re: [PATCH 1/2] PNP: Add a release method to system resource driver

[Bjorn Helgaas]

On Tue, Sep 25, 2012 at 7:25 AM, Matthew Garrett <mjg@redhat.com> wrote:
> This could conceivably be hotpluggable, and we may want to displace it
> from devices under certain circustances, so add a release method to hand

s/circustances/circumstances/

> back the resources.
>
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>

I've never directly applied PNP patches; I've always sent them through
Len's ACPI tree (cc'd), so let me know if you want me to do more than
ack this.

> ---
>  drivers/pnp/system.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pnp/system.c b/drivers/pnp/system.c
> index 49c1720..b0a50f2 100644
> --- a/drivers/pnp/system.c
> +++ b/drivers/pnp/system.c
> @@ -52,7 +52,17 @@ static void reserve_range(struct pnp_dev *dev, struct resource *r, int port)
>                  res ? "has been" : "could not be");
>  }
>
> -static void reserve_resources_of_dev(struct pnp_dev *dev)
> +static void free_range(struct pnp_dev *dev, struct resource *r, int port)
> +{
> +       resource_size_t start = r->start, end = r->end;
> +
> +       if (port)
> +               release_region(start, end - start + 1);
> +       else
> +               release_mem_region(start, end - start + 1);
> +}
> +
> +static void handle_resources_of_dev(struct pnp_dev *dev, bool free)
>  {
>         struct resource *res;
>         int i;
> @@ -75,29 +85,41 @@ static void reserve_resources_of_dev(struct pnp_dev *dev)
>                 if (res->end < res->start)
>                         continue;       /* invalid */
>
> -               reserve_range(dev, res, 1);
> +               if (free)
> +                       free_range(dev, res, 1);
> +               else
> +                       reserve_range(dev, res, 1);
>         }
>
>         for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_MEM, i)); i++) {
>                 if (res->flags & IORESOURCE_DISABLED)
>                         continue;
>
> -               reserve_range(dev, res, 0);
> +               if (free)
> +                       free_range(dev, res, 0);
> +               else
> +                       reserve_range(dev, res, 0);
>         }
>  }
>
>  static int system_pnp_probe(struct pnp_dev *dev,
>                             const struct pnp_device_id *dev_id)
>  {
> -       reserve_resources_of_dev(dev);
> +       handle_resources_of_dev(dev, false);
>         return 0;
>  }
>
> +static void system_pnp_remove(struct pnp_dev *dev)
> +{
> +       handle_resources_of_dev(dev, true);
> +}
> +
>  static struct pnp_driver system_pnp_driver = {
>         .name     = "system",
>         .id_table = pnp_dev_table,
>         .flags    = PNP_DRIVER_RES_DO_NOT_CHANGE,
>         .probe    = system_pnp_probe,
> +       .remove   = system_pnp_remove,
>  };
>
>  static int __init pnp_system_init(void)
> --
> 1.7.11.4
>

Re: [PATCH 2/2] PNP: Unbind drivers if the new driver matches _HID rather than _CID

[Bjorn Helgaas]

On Tue, Sep 25, 2012 at 7:25 AM, Matthew Garrett <mjg@redhat.com> wrote:
> ACPIPNP devices may have two levels of ID - the HID (a single string
> defining the hardware) and the CIDs (zero or more strings defining
> interfaces compatible with the hardware). If a driver matching a CID is
> bound first, it will be impossible to bind a driver that matches the HID
> despite it being more specific. This has been seen in the wild with
> platforms that define an IPMI device as:
>
>                 Device (NIPM)
>                 {
>                     Name (_HID, EisaId ("IPI0001"))
>                     Name (_CID, EisaId ("PNP0C01"))
>
> resulting in the device being claimed by the generic motherboard resource
> driver binding to the PNP0C01 CID. The IPMI driver attempts to bind later
> and fails, preventing the IPMI device from being made available to the ACPI
> layer.
>
> This can be avoided at driver probe time by detaching the existing driver
> if the new driver matches the device HID. Since each device can only have
> a single HID this will only permit more specific drivers to dislodge more
> generic drivers.
>
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

Seems reasonable to me.  The HID/CID idea is not new to ACPI; both
isapnp and pnpbios seem to support it as well.

Do you know of any scenarios besides this IPMI one where there's the
possibility of two drivers matching the same device?  If so, does the
detach/attach process work reasonably?  My worry is that drivers don't
normally give up devices, so the detach path is not well exercised.
And I don't know what happens to any users of the device during the
switch.  For example, if something was using a TPM and we replaced the
driver, what does that look like to the user?

> ---
>  drivers/pnp/driver.c | 42 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
> index 00e9403..8d22836 100644
> --- a/drivers/pnp/driver.c
> +++ b/drivers/pnp/driver.c
> @@ -25,6 +25,14 @@ static int compare_func(const char *ida, const char *idb)
>         return 1;
>  }
>
> +static int compare_single_pnp_id(const char *ida, const char *idb)
> +{
> +       if (memcmp(ida, idb, 3) == 0)
> +               if (compare_func(ida, idb) == 1)
> +                       return 1;
> +       return 0;
> +}
> +
>  int compare_pnp_id(struct pnp_id *pos, const char *id)
>  {
>         if (!pos || !id || (strlen(id) != 7))
> @@ -32,9 +40,8 @@ int compare_pnp_id(struct pnp_id *pos, const char *id)
>         if (memcmp(id, "ANYDEVS", 7) == 0)
>                 return 1;
>         while (pos) {
> -               if (memcmp(pos->id, id, 3) == 0)
> -                       if (compare_func(pos->id, id) == 1)
> -                               return 1;
> +               if (compare_single_pnp_id(pos->id, id) == 1)
> +                       return 1;
>                 pos = pos->next;
>         }
>         return 0;
> @@ -56,6 +63,22 @@ static const struct pnp_device_id *match_device(struct pnp_driver *drv,
>         return NULL;
>  }
>
> +static int match_hid(struct pnp_driver *drv, struct pnp_dev *dev)
> +{
> +       const struct pnp_device_id *drv_id = drv->id_table;
> +
> +       if (!drv_id)
> +               return 0;
> +
> +       while (&drv_id->id) {
> +               /* The first ID is _HID */
> +               if (compare_single_pnp_id(dev->id->id, drv_id->id))
> +                       return 1;
> +               drv_id++;
> +       }
> +       return 0;
> +}
> +
>  int pnp_device_attach(struct pnp_dev *pnp_dev)
>  {
>         spin_lock(&pnp_lock);
> @@ -151,6 +174,19 @@ static int pnp_bus_match(struct device *dev, struct device_driver *drv)
>
>         if (match_device(pnp_drv, pnp_dev) == NULL)
>                 return 0;
> +
> +       /*
> +        * ACPIPNP offers two levels of device ID - HID and CID. HID defines
> +        * the specific device ID while CID represents the device
> +        * compatibility IDs. If a device is matched by a compatibility ID
> +        * first, it will be impossible for a hardware-specific driver to
> +        * bind since there will already be a driver. We can handle this case
> +        * by unbinding the original driver if the device has one bound and
> +        * if the new driver matches the HID rather than a compatibility ID.
> +        */
> +       if (dev->driver && match_hid(pnp_drv, pnp_dev))
> +               device_release_driver(dev);
> +
>         return 1;
>  }
>
> --
> 1.7.11.4
>

Re: [PATCH 2/2] PNP: Unbind drivers if the new driver matches _HID rather than _CID

[Matthew Garrett]

On Tue, Sep 25, 2012 at 01:04:25PM -0600, Bjorn Helgaas wrote:
> On Tue, Sep 25, 2012 at 7:25 AM, Matthew Garrett <mjg@redhat.com> wrote:

> Do you know of any scenarios besides this IPMI one where there's the
> possibility of two drivers matching the same device?  If so, does the
> detach/attach process work reasonably?  My worry is that drivers don't
> normally give up devices, so the detach path is not well exercised.
> And I don't know what happens to any users of the device during the
> switch.  For example, if something was using a TPM and we replaced the
> driver, what does that look like to the user?

Yeah, this could definitely happen with TPM - tpm_infinion could 
displace tpm_tis. This actually flags up something kind of obviously 
broken in the TPM code, since tpm_infineon comes *after* tpm_tis in the 
link order despite being more specific. Winning. It looks like there's a 
valid tpm_release function, but I'll find an infineon machine and figure 
out whether it actually works or not.

-- 
Matthew Garrett | mjg59@srcf.ucam.org