+Rafael, Jarkko and linux-acpi.
On Tue, May 28, 2019 at 06:40:40PM +0200, Ard Biesheuvel wrote:
> Currently, the ACPI enumeration that takes place when registering a
> SPI master only considers immediate child devices in the ACPI namespace,
> rather than checking the ResourceSource field in the SpiSerialBus()
> resource descriptor.
>
> This is incorrect: SPI slaves could reside anywhere in the ACPI
> namespace, and so we should enumerate the entire namespace and look for
> any device that refers to the newly registered SPI master in its
> resource descriptor.
Yes, that's right.
> In order to prevent potential regressions, retain the old code and run
> it first. Only then, enumerate the entire namespace. Note that this
> could result in child devices wrongly being associated with a SPI master
> but this can only occur if a SPI master has a child device representing
> a SPI slave that is connected to another master.
I don't think we need to retain the old behavior here. We did the same
already for I2C and I don't remember seeing any regressions. Also I have
not seen too many real SPI slaves in ACPI based systems so most probably
nobody even notices this change ;-)
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> ---
> drivers/spi/spi.c | 75 ++++++++++++++++++--
> 1 file changed, 69 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 5e75944ad5d1..d2f4332d9cc3 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1882,10 +1882,6 @@ static acpi_status acpi_register_spi_device(struct spi_controller *ctlr,
> struct spi_device *spi;
> int ret;
>
> - if (acpi_bus_get_status(adev) || !adev->status.present ||
> - acpi_device_enumerated(adev))
> - return AE_OK;
> -
> spi = spi_alloc_device(ctlr);
> if (!spi) {
> dev_err(&ctlr->dev, "failed to allocate SPI device for %s\n",
> @@ -1927,18 +1923,64 @@ static acpi_status acpi_register_spi_device(struct spi_controller *ctlr,
> return AE_OK;
> }
>
> +static acpi_status acpi_spi_add_child_device(acpi_handle handle, u32 level,
> + void *data, void **return_value)
> +{
> + struct spi_controller *ctlr = data;
> + struct acpi_device *adev;
> +
> + if (acpi_bus_get_device(handle, &adev) ||
> + acpi_bus_get_status(adev) ||
> + !adev->status.present ||
> + acpi_device_enumerated(adev))
> + return AE_OK;
> +
> + return acpi_register_spi_device(ctlr, adev);
> +}
> +
> +static int acpi_spi_check_parent(struct acpi_resource *ares, void *data)
> +{
> + struct acpi_resource_spi_serialbus *sb;
> +
> + if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> + return 1;
> +
> + sb = &ares->data.spi_serial_bus;
> +
> + /* check whether this resource refers our controller */
> + acpi_get_handle(NULL, sb->resource_source.string_ptr, data);
> + return 1;
> +}
> +
> static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
> void *data, void **return_value)
> {
> struct spi_controller *ctlr = data;
> + acpi_handle parent_handle = NULL;
> + struct list_head resource_list;
> struct acpi_device *adev;
> + int ret;
>
> - if (acpi_bus_get_device(handle, &adev))
> + if (acpi_bus_get_device(handle, &adev) ||
> + acpi_bus_get_status(adev) ||
> + !adev->status.present ||
> + acpi_device_enumerated(adev))
> + return AE_OK;
> +
> + INIT_LIST_HEAD(&resource_list);
> + ret = acpi_dev_get_resources(adev, &resource_list,
> + acpi_spi_check_parent,
> + &parent_handle);
> + acpi_dev_free_resource_list(&resource_list);
> +
> + if (ret < 0 || ACPI_HANDLE(ctlr->dev.parent) != parent_handle)
> return AE_OK;
>
> return acpi_register_spi_device(ctlr, adev);
> }
>
> +#define SPI_ACPI_ENUMERATE_MAX_DEPTH 32
> +
> static void acpi_register_spi_devices(struct spi_controller *ctlr)
> {
> acpi_status status;
> @@ -1948,10 +1990,31 @@ static void acpi_register_spi_devices(struct spi_controller *ctlr)
> if (!handle)
> return;
>
> + /*
> + * Enumerate child nodes of this controller. This logic is retained to
> + * prevent potential regressions on systems where the ResourceSource of
> + * a device's SpiSerialBus() resource does not refer to the parent SPI
> + * controller device.
> + */
> status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> + acpi_spi_add_child_device, NULL, ctlr,
> + NULL);
> + if (ACPI_FAILURE(status)) {
> + dev_warn(&ctlr->dev, "failed to enumerate SPI children\n");
> + return;
> + }
> +
> + /*
> + * Walk the entire namespace and enumerate all devices containing a
> + * SpiSerialBus() resource whose ResourceSource argument refers to this
> + * SPI controller device.
> + */
> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> + SPI_ACPI_ENUMERATE_MAX_DEPTH,
> acpi_spi_add_device, NULL, ctlr, NULL);
> if (ACPI_FAILURE(status))
> - dev_warn(&ctlr->dev, "failed to enumerate SPI slaves\n");
> + dev_warn(&ctlr->dev,
> + "failed to enumerate SPI slaves in the ACPI namespace\n");
> }
> #else
> static inline void acpi_register_spi_devices(struct spi_controller *ctlr) {}
> --
> 2.20.1
