diff mbox series

[RFC] spi/acpi: enumerate all SPI slaves in the namespace

Message ID 20190528164040.6781-1-ard.biesheuvel@linaro.org
State New
Headers show
Series [RFC] spi/acpi: enumerate all SPI slaves in the namespace | expand

Commit Message

Ard Biesheuvel May 28, 2019, 4:40 p.m. UTC 
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.

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.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 drivers/spi/spi.c | 75 ++++++++++++++++++--
 1 file changed, 69 insertions(+), 6 deletions(-)

-- 
2.20.1

Comments

Mika Westerberg May 29, 2019, 9:55 a.m. UTC | #1 
+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@linaro.org>

> ---

>  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
diff mbox series

Patch

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) {}