On Friday, October 24, 2014 03:55:34 PM Rafael J. Wysocki wrote:
> On Friday, October 24, 2014 12:12:30 PM Mika Westerberg wrote:
> > Commit 6ab3430129e2 ("mfd: Add ACPI support") made the MFD subdevices to
> > share the parent MFD ACPI companion device if no _HID/_CID is specified for
> > the subdevice in mfd_cell description. However, since all the subdevices
> > share the ACPI companion, the match and modalias generation logic started
> > to use the ACPI companion as well resulting this:
> >
> > # cat /sys/bus/platform/devices/HID-SENSOR-200041.6.auto/modalias
> > acpi:INT33D1:PNP0C50:
> >
> > instead of the expected one
> >
> > # cat /sys/bus/platform/devices/HID-SENSOR-200041.6.auto/modalias
> > platform:HID-SENSOR-200041
> >
> > In other words the subdevice modalias is overwritten by the one taken from
> > ACPI companion. This causes udev not to load the driver anymore.
> >
> > It is useful to be able to share the ACPI companion so that MFD subdevices
> > (and possibly other devices as well) can access the ACPI resources even if
> > they do not have ACPI representation in the namespace themselves.
> >
> > An example where this is used is Minnowboard LPC driver that creates GPIO
> > as a subdevice among other things. Without the ACPI companion gpiolib is
> > not is not able to lookup the corresponding GPIO controller from ACPI
> > GpioIo resource.
> >
> > To fix this we restrict the match and modalias logic to be limited to the
> > first physical device. The secondary devices will still be able to access
> > the ACPI companion but they will be matched using traditional way.
> >
> > Reported-by: Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>
> > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
>
> So I've applied it, although it didn't make it to the mailing lists for an
> unknown reason, but I modified it slightly. My version is below for
> completness (it's in bleeding-edge for now).
>
> I'm a bit concerned that it will break something obscure I can't recall
> ATM, but since I can't recall it may not be that important (or even non-existent
> at all). In any case, we may need to go back all the way to reverting the
> MFD commit if that happens.
>
> Rafael
>
>
> ---
> From: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Subject: [PATCH] ACPI: Use ACPI companion to match only the first physical device
>
> Commit 6ab3430129e2 ("mfd: Add ACPI support") made the MFD subdevices
> share the parent MFD ACPI companion if no _HID/_CID is specified for
> the subdevice in mfd_cell description. However, since all the subdevices
> share the ACPI companion, the match and modalias generation logic started
> to use the ACPI companion as well resulting this:
>
> # cat /sys/bus/platform/devices/HID-SENSOR-200041.6.auto/modalias
> acpi:INT33D1:PNP0C50:
>
> instead of the expected one
>
> # cat /sys/bus/platform/devices/HID-SENSOR-200041.6.auto/modalias
> platform:HID-SENSOR-200041
>
> In other words the subdevice modalias is overwritten by the one taken from
> ACPI companion. This causes udev not to load the driver anymore.
>
> It is useful to be able to share the ACPI companion so that MFD subdevices
> (and possibly other devices as well) can access the ACPI resources even if
> they do not have ACPI representation in the namespace themselves.
>
> An example where this is used is Minnowboard LPC driver that creates GPIO
> as a subdevice among other things. Without the ACPI companion gpiolib is
> not able to lookup the corresponding GPIO controller from ACPI GpioIo
> resource.
>
> To fix this, restrict the match and modalias logic to be limited to the
> first (primary) physical device associated with the given ACPI comapnion.
> The secondary devices will still be able to access the ACPI companion,
> but they will be matched in a different way.
>
> Fixes: 6ab3430129e2 (mfd: Add ACPI support)
> Reported-by: Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> [ rjw: Change the name of the new function to acpi_companion_match()
> and modify the kerneldoc comment for it, changelog fixups. ]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -181,6 +181,53 @@ static int create_modalias(struct acpi_d
> }
>
> /*
> + * acpi_companion_match() - Can we match via ACPI companion device
> + * @dev: Device in question
> + *
> + * Check if the given device has an ACPI companion and if that companion has
> + * a valid list of PNP IDs, and if the device is the first (primary) physical
> + * device associated with it.
> + *
> + * If multiple physical devices are attached to a single ACPI companion, we need
> + * to be careful. The usage scenario for this kind of relationship is that all
> + * of the physical devices in question use resources provided by the ACPI
> + * companion. A typical case is an MFD device where all the sub-devices share
> + * the parent's ACPI companion. In such cases we can only allow the primary
> + * (first) physical device to be matched with the help of the companion's PNP
> + * IDs.
> + *
> + * Additional physical devices sharing the ACPI companion can still use
> + * resources available from it but they will be matched normally using functions
> + * provided by their bus types (and analogously for their modalias).
> + */
> +static bool acpi_companion_match(const struct device *dev)
> +{
> + struct acpi_device *adev;
> + bool ret;
> +
> + adev = ACPI_COMPANION(dev);
> + if (!adev)
> + return false;
> +
> + if (list_empty(&adev->pnp.ids))
> + return false;
> +
> + ret = true;
On a second thought, should we return true if the list of physical devices is
empty? That surely means ACPI_COMPANION(dev) lied to us?
> + mutex_lock(&adev->physical_node_lock);
> + if (!list_empty(&adev->physical_node_list)) {
> + const struct acpi_device_physical_node *node;
> +
> + node = list_first_entry(&adev->physical_node_list,
> + struct acpi_device_physical_node, node);
> + if (node->dev != dev)
> + ret = false;
And that may be simply
ret = node->dev == dev;
> + }
> + mutex_unlock(&adev->physical_node_lock);
> +
> + return ret;
> +}
> +
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
