diff mbox

[v5,2/8] acpi: fix enumeration (visited) flags for bus rescans

Message ID 1467404352-27101-3-git-send-email-octavian.purdila@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Octavian Purdila July 1, 2016, 8:19 p.m. UTC
If the ACPI tables changes as a result of a dinamically loaded table and
a bus rescan is required the enumeration/visited flag are not
consistent.

I2C/SPI are not directly enumerated in acpi_bus_attach(), however the
visited flag is set. This makes it impossible to check if an ACPI device
has already been enumerated by the I2C and SPI subsystems. To fix this
issue we only set the visited flags if the device is not I2C or SPI.

With this change we also need to remove setting visited to false from
acpi_bus_attach(), otherwise if we rescan already enumerated I2C/SPI
devices we try to re-enumerate them.

Note that I2C/SPI devices can be enumerated either via a scan handler
(when using PRP0001) or via regular device_attach(). In either case
the flow goes through acpi_default_enumeration() which makes it the
ideal place to mark the ACPI device as enumerated.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/acpi/scan.c  | 13 +++++++------
 include/linux/acpi.h | 18 ++++++++++++++++++
 2 files changed, 25 insertions(+), 6 deletions(-)

Comments

Mika Westerberg July 6, 2016, 3:03 p.m. UTC | #1
On Fri, Jul 01, 2016 at 11:19:06PM +0300, Octavian Purdila wrote:
> If the ACPI tables changes as a result of a dinamically loaded table and
> a bus rescan is required the enumeration/visited flag are not
> consistent.
> 
> I2C/SPI are not directly enumerated in acpi_bus_attach(), however the
> visited flag is set. This makes it impossible to check if an ACPI device
> has already been enumerated by the I2C and SPI subsystems. To fix this
> issue we only set the visited flags if the device is not I2C or SPI.
> 
> With this change we also need to remove setting visited to false from
> acpi_bus_attach(), otherwise if we rescan already enumerated I2C/SPI
> devices we try to re-enumerate them.
> 
> Note that I2C/SPI devices can be enumerated either via a scan handler
> (when using PRP0001) or via regular device_attach(). In either case
> the flow goes through acpi_default_enumeration() which makes it the
> ideal place to mark the ACPI device as enumerated.

Hmm, this somehow fails to enumerate PRP0001 devices.

I'm adding following SSDT to my initrd and the resulting device is never
enumerated on SPI bus (it exists in /sys/bus/acpi/devices). If I use
normal ACPI IDs (non-PRP0001) then it works fine.

/*
 * Minnowboard MAX
 *
 * http://wiki.minnowboard.org/MinnowBoard_MAX
 *
 * This adds Atmel AT25 compatible serial EEPROM to the SPI host controller
 * available on Minnowboard MAX low speed connector.
 */
DefinitionBlock ("at25.aml", "SSDT", 5, "INTEL", "AT25", 1)
{
    External (_SB_.SPI1, DeviceObj)

    Scope (\_SB.SPI1)
    {
        Device (EEP0) {
            Name (_HID, "PRP0001")
            Name (_DDN, "Atmel AT25 compatible EEPROM")
            Name (_CRS, ResourceTemplate () {
                SpiSerialBus (
                    1,                      // Chip select
                    PolarityLow,            // Chip select is active low
                    FourWireMode,           // Full duplex
                    8,                      // Bits per word is 8 (byte)
                    ControllerInitiated,    // Don't care
                    1000000,                // Initial bus speed is 1MHz
                    ClockPolarityLow,       // SPI mode 0
                    ClockPhaseFirst,        // SPI mode 0
                    "\\_SB.SPI1",           // SPI host controller
                    0                       // Must be 0
                )
            })

            /*
             * See Documentation/devicetree/bindings/eeprom/at25.txt for
             * more information about these bindings.
             */
            Name (_DSD, Package () {
                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                Package () {
                    Package () {"compatible", Package () {"atmel,at25"}},
                    Package () {"size", 1024},
                    Package () {"pagesize", 32},
                    Package () {"address-width", 16},
                }
            })
        }
    }
}
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Octavian Purdila July 6, 2016, 3:37 p.m. UTC | #2
On Wed, Jul 6, 2016 at 6:03 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Fri, Jul 01, 2016 at 11:19:06PM +0300, Octavian Purdila wrote:
>> If the ACPI tables changes as a result of a dinamically loaded table and
>> a bus rescan is required the enumeration/visited flag are not
>> consistent.
>>
>> I2C/SPI are not directly enumerated in acpi_bus_attach(), however the
>> visited flag is set. This makes it impossible to check if an ACPI device
>> has already been enumerated by the I2C and SPI subsystems. To fix this
>> issue we only set the visited flags if the device is not I2C or SPI.
>>
>> With this change we also need to remove setting visited to false from
>> acpi_bus_attach(), otherwise if we rescan already enumerated I2C/SPI
>> devices we try to re-enumerate them.
>>
>> Note that I2C/SPI devices can be enumerated either via a scan handler
>> (when using PRP0001) or via regular device_attach(). In either case
>> the flow goes through acpi_default_enumeration() which makes it the
>> ideal place to mark the ACPI device as enumerated.
>
> Hmm, this somehow fails to enumerate PRP0001 devices.
>
> I'm adding following SSDT to my initrd and the resulting device is never
> enumerated on SPI bus (it exists in /sys/bus/acpi/devices). If I use
> normal ACPI IDs (non-PRP0001) then it works fine.
>

I specifically tested with PRP0001 via initrd with I2C, I am not sure
why it does not work on SPI. I'll give it a try tomorrow when I have
access to my setup.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki July 6, 2016, 9:09 p.m. UTC | #3
On Wednesday, July 06, 2016 06:37:46 PM Octavian Purdila wrote:
> On Wed, Jul 6, 2016 at 6:03 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Fri, Jul 01, 2016 at 11:19:06PM +0300, Octavian Purdila wrote:
> >> If the ACPI tables changes as a result of a dinamically loaded table and
> >> a bus rescan is required the enumeration/visited flag are not
> >> consistent.
> >>
> >> I2C/SPI are not directly enumerated in acpi_bus_attach(), however the
> >> visited flag is set. This makes it impossible to check if an ACPI device
> >> has already been enumerated by the I2C and SPI subsystems. To fix this
> >> issue we only set the visited flags if the device is not I2C or SPI.
> >>
> >> With this change we also need to remove setting visited to false from
> >> acpi_bus_attach(), otherwise if we rescan already enumerated I2C/SPI
> >> devices we try to re-enumerate them.
> >>
> >> Note that I2C/SPI devices can be enumerated either via a scan handler
> >> (when using PRP0001) or via regular device_attach(). In either case
> >> the flow goes through acpi_default_enumeration() which makes it the
> >> ideal place to mark the ACPI device as enumerated.
> >
> > Hmm, this somehow fails to enumerate PRP0001 devices.
> >
> > I'm adding following SSDT to my initrd and the resulting device is never
> > enumerated on SPI bus (it exists in /sys/bus/acpi/devices). If I use
> > normal ACPI IDs (non-PRP0001) then it works fine.
> >
> 
> I specifically tested with PRP0001 via initrd with I2C, I am not sure
> why it does not work on SPI. I'll give it a try tomorrow when I have
> access to my setup.

OK, I'll wait for this test to conclude before applying the series, then. :-)

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mika Westerberg July 7, 2016, 9 a.m. UTC | #4
On Wed, Jul 06, 2016 at 11:09:44PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 06, 2016 06:37:46 PM Octavian Purdila wrote:
> > On Wed, Jul 6, 2016 at 6:03 PM, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > On Fri, Jul 01, 2016 at 11:19:06PM +0300, Octavian Purdila wrote:
> > >> If the ACPI tables changes as a result of a dinamically loaded table and
> > >> a bus rescan is required the enumeration/visited flag are not
> > >> consistent.
> > >>
> > >> I2C/SPI are not directly enumerated in acpi_bus_attach(), however the
> > >> visited flag is set. This makes it impossible to check if an ACPI device
> > >> has already been enumerated by the I2C and SPI subsystems. To fix this
> > >> issue we only set the visited flags if the device is not I2C or SPI.
> > >>
> > >> With this change we also need to remove setting visited to false from
> > >> acpi_bus_attach(), otherwise if we rescan already enumerated I2C/SPI
> > >> devices we try to re-enumerate them.
> > >>
> > >> Note that I2C/SPI devices can be enumerated either via a scan handler
> > >> (when using PRP0001) or via regular device_attach(). In either case
> > >> the flow goes through acpi_default_enumeration() which makes it the
> > >> ideal place to mark the ACPI device as enumerated.
> > >
> > > Hmm, this somehow fails to enumerate PRP0001 devices.
> > >
> > > I'm adding following SSDT to my initrd and the resulting device is never
> > > enumerated on SPI bus (it exists in /sys/bus/acpi/devices). If I use
> > > normal ACPI IDs (non-PRP0001) then it works fine.
> > >
> > 
> > I specifically tested with PRP0001 via initrd with I2C, I am not sure
> > why it does not work on SPI. I'll give it a try tomorrow when I have
> > access to my setup.
> 
> OK, I'll wait for this test to conclude before applying the series, then. :-)

I tested again and it works fine. I probably had an older version of the
series applied or something like that. Now, I'm able to see my AT25 SPI
eeprom successfully probed:

[    7.264705] at25 spi-PRP0001:00: 1 KByte at25 eeprom, pagesize 32

Sorry about the confusion.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mika Westerberg July 7, 2016, 9:28 a.m. UTC | #5
On Fri, Jul 01, 2016 at 11:19:06PM +0300, Octavian Purdila wrote:
> If the ACPI tables changes as a result of a dinamically loaded table and
> a bus rescan is required the enumeration/visited flag are not
> consistent.
> 
> I2C/SPI are not directly enumerated in acpi_bus_attach(), however the
> visited flag is set. This makes it impossible to check if an ACPI device
> has already been enumerated by the I2C and SPI subsystems. To fix this
> issue we only set the visited flags if the device is not I2C or SPI.
> 
> With this change we also need to remove setting visited to false from
> acpi_bus_attach(), otherwise if we rescan already enumerated I2C/SPI
> devices we try to re-enumerate them.
> 
> Note that I2C/SPI devices can be enumerated either via a scan handler
> (when using PRP0001) or via regular device_attach(). In either case
> the flow goes through acpi_default_enumeration() which makes it the
> ideal place to mark the ACPI device as enumerated.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 5f28cf7..f80f8a7 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1406,7 +1406,7 @@  void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 	acpi_bus_get_flags(device);
 	device->flags.match_driver = false;
 	device->flags.initialized = true;
-	device->flags.visited = false;
+	acpi_device_clear_enumerated(device);
 	device_initialize(&device->dev);
 	dev_set_uevent_suppress(&device->dev, true);
 	acpi_init_coherency(device);
@@ -1683,8 +1683,10 @@  static void acpi_default_enumeration(struct acpi_device *device)
 	acpi_dev_get_resources(device, &resource_list, acpi_check_spi_i2c_slave,
 			       &is_spi_i2c_slave);
 	acpi_dev_free_resource_list(&resource_list);
-	if (!is_spi_i2c_slave)
+	if (!is_spi_i2c_slave) {
 		acpi_create_platform_device(device);
+		acpi_device_set_enumerated(device);
+	}
 }
 
 static const struct acpi_device_id generic_device_ids[] = {
@@ -1751,7 +1753,7 @@  static void acpi_bus_attach(struct acpi_device *device)
 	acpi_bus_get_status(device);
 	/* Skip devices that are not present. */
 	if (!acpi_device_is_present(device)) {
-		device->flags.visited = false;
+		acpi_device_clear_enumerated(device);
 		device->flags.power_manageable = 0;
 		return;
 	}
@@ -1766,7 +1768,7 @@  static void acpi_bus_attach(struct acpi_device *device)
 
 		device->flags.initialized = true;
 	}
-	device->flags.visited = false;
+
 	ret = acpi_scan_attach_handler(device);
 	if (ret < 0)
 		return;
@@ -1780,7 +1782,6 @@  static void acpi_bus_attach(struct acpi_device *device)
 		if (!ret && device->pnp.type.platform_id)
 			acpi_default_enumeration(device);
 	}
-	device->flags.visited = true;
 
  ok:
 	list_for_each_entry(child, &device->children, node)
@@ -1872,7 +1873,7 @@  void acpi_bus_trim(struct acpi_device *adev)
 	 */
 	acpi_device_set_power(adev, ACPI_STATE_D3_COLD);
 	adev->flags.initialized = false;
-	adev->flags.visited = false;
+	acpi_device_clear_enumerated(adev);
 }
 EXPORT_SYMBOL_GPL(acpi_bus_trim);
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 288fac5..7089e99 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -532,6 +532,16 @@  void acpi_walk_dep_device_list(acpi_handle handle);
 struct platform_device *acpi_create_platform_device(struct acpi_device *);
 #define ACPI_PTR(_ptr)	(_ptr)
 
+static inline void acpi_device_set_enumerated(struct acpi_device *adev)
+{
+	adev->flags.visited = true;
+}
+
+static inline void acpi_device_clear_enumerated(struct acpi_device *adev)
+{
+	adev->flags.visited = false;
+}
+
 #else	/* !CONFIG_ACPI */
 
 #define acpi_disabled 1
@@ -678,6 +688,14 @@  static inline enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev)
 
 #define ACPI_PTR(_ptr)	(NULL)
 
+static inline void acpi_device_set_enumerated(struct acpi_device *adev)
+{
+}
+
+static inline void acpi_device_clear_enumerated(struct acpi_device *adev)
+{
+}
+
 #endif	/* !CONFIG_ACPI */
 
 #ifdef CONFIG_ACPI