Re: [PATCH] ACPI: Use ACPI companion to match only the first physical device

[Rafael J. Wysocki]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/