[RFC PATCH 0/8] ACPI: change the way of enumerating PNPACPI/Platform devices

[RFC PATCH 0/8] ACPI: change the way of enumerating PNPACPI/Platform devices

[Zhang Rui]

Hi, all,

Currently, PNP bus is used as the default bus for for enumerating ACPI
devices with _HID/_CID.
For a device that needs to be enumerated to platform bus, we need to add
its _HID/_CID to the platform scan handler white list explicitly.

This becomes a problem as more and more _HID devices need to be
enumerated to platform bus nowadays, thus the list is continuously growing.

So, a solution that uses platform bus for _HID enumeration by default
is preferred.

In order to do this, this patch set
first, changes the way of enumerating PNP devices. We introduce a white list
to create PNP devices instead. The white list contains all the pnp_device_id
strings in all the pnp drivers, thus this change is transparent to PNP core
and all the PNP drivers.
second, changes the code to enumerate ACPI devices with _HID/_CID to platform
bus by default, unless the device already has a scan handler attached.

The patches of this patch set has been tested on my desktop machine,
I can see that there are four PNP devices disappeared from PNP bus
/sys/bus/pnp/devices/00:01/id:PNP0200
/sys/bus/pnp/devices/00:02/id:INT0800
/sys/bus/pnp/devices/00:03/id:PNP0103
/sys/bus/pnp/devices/00:0a/id:PNP0c04
and all of these devices has no driver attached.

At the same time there are 21 more platform devices created,
/sys/bus/platform/devices/INT0800:00  
/sys/bus/platform/devices/INT3F0D:00  
/sys/bus/platform/devices/LNXSYSTM:00 (ACPI system bus/root node)
/sys/bus/platform/devices/LNXTHERM:00 (ACPI thermal zone)
/sys/bus/platform/devices/LNXTHERM:01 (ACPI thermal zone)
/sys/bus/platform/devices/PNP0000:00  (PIC)
/sys/bus/platform/devices/PNP0100:00  (system timer?)
/sys/bus/platform/devices/PNP0103:00  (HPET)
/sys/bus/platform/devices/PNP0B00:00  (CMOS RTC)
/sys/bus/platform/devices/PNP0C01:00  (system board)
/sys/bus/platform/devices/PNP0C01:00  (system board)
/sys/bus/platform/devices/PNP0C02:00  (General ID for reserving resources)
/sys/bus/platform/devices/PNP0C02:03  (General ID for reserving resources)
/sys/bus/platform/devices/PNP0C04:00  (Numeric data processor?)
/sys/bus/platform/devices/PNP0C0B:00  (ACPI fan)
/sys/bus/platform/devices/PNP0C0B:01  (ACPI fan)
/sys/bus/platform/devices/PNP0C0B:02  (ACPI fan)
/sys/bus/platform/devices/PNP0C0B:03  (ACPI fan)
/sys/bus/platform/devices/PNP0C0B:04  (ACPI fan)
/sys/bus/platform/devices/PNP0C0D:00  (ACPI lid)
/sys/bus/platform/devices/PNP0C14:00  (ACPI wmi)

Clarification:
Although it seems that we are introducing a much bigger white list to replace
a small one, the benefit is that
1. without the patch, the acpi_platform scan handler white list is
   continuously growing.
2. with the patch set, the PNPACPI scan handler white list will become
   smaller and smaller by
   a) remove the ids from the PNPACPI white list, for the devices
      that are never enumerated via ACPI
   b) remove the ids from the PNPACPI whilte list and convert the drivers to
      platform bus drivers, for the devices that are not PNP devices in nature.
   which will be done later.

Known Issue:
PNP bus does not check the device resources when adding a new PNP device,
while Platform bus does by invoking insert_resource() in platform_device_add().
This results in failure when creating platform device node for some ACPI
device objects in case there is resource conflict.
In my desktop, I can see that
Creating PNP0200:00 (DMA controller) fails because its resource (IO: 0x81 ~ 0x91)
conflicts with "0080-008f : dma page reg"
and
Creating PNP0C02:02 fails because its resource (IO: 0x72 ~ 0x7f)
conflicts with "0070-0077 : PNP0B00:00" (CMOS RTC).

But anyway, IMO, this specific problem does not mean the proposal of this patch
is wrong, thus I send this patch out for early review to see if I'm in the right
direction.
Any comments are really welcome.

thanks,
rui

----------------------------------------------------------------
Zhang Rui (8):
      ACPI: introduce .match() callback for ACPI scan handler
      PNPACPI: use whilte list for pnpacpi device enumeration
      PNPACPI: remove ids that does not comply with the ACPI PNP id rule
      PNPACPI: remove unsupported serial PNP ids from PNPACPI id list
      PNPACPI: check and enumerate CMOS RTC devices explicitly
      ACPI: use platform bus as the default bus for _HID enumeration
      Revert "ACPI / PNP: skip ACPI device nodes associated with physical nodes already"
      PNPACPI: create both PNP and Platform device nodes for PNP0C01/PNP0C02

 drivers/acpi/acpi_platform.c |   28 ---
 drivers/acpi/scan.c          |   31 ++--
 drivers/pnp/pnpacpi/core.c   |  417 ++++++++++++++++++++++++++++++++++++++----
 include/acpi/acpi_bus.h      |    1 +
 include/linux/pnp.h          |    3 +
 5 files changed, 402 insertions(+), 78 deletions(-)

[RFC PATCH 1/8] ACPI: introduce .match() callback for ACPI scan handler

[Zhang Rui]

Currently, acpi scan handler uses strcmp() to match device ids
and scan handler ids.

When converting PNPACPI enumeration into a scan handler, which I will do
later in this patch set, the current code becomes not flexible enough
because PNP acpi scan handler requires wildcase and case insensitive support.

Thus a per scan handler .match() callback is introduced in this patch,
so that specified scan handler can have more flexible matching mechanism
by introduce its own .match() callback.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/scan.c     |   17 +++++++++++------
 include/acpi/acpi_bus.h |    1 +
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 57b053f..dca22eb 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1907,14 +1907,19 @@ static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
 	const struct acpi_device_id *devid;
 
 	for (devid = handler->ids; devid->id[0]; devid++)
-		if (!strcmp((char *)devid->id, idstr)) {
-			if (matchid)
-				*matchid = devid;
-
-			return true;
-		}
+		if (handler->match) {
+			if (handler->match(idstr, (char *)devid->id))
+				goto success;
+		} else
+			if (!strcmp((char *)devid->id, idstr))
+				goto success;
 
 	return false;
+
+success:
+	if (matchid)
+		*matchid = devid;
+	return true;
 }
 
 static struct acpi_scan_handler *acpi_scan_match_handler(char *idstr,
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 8256eb4..8c5e235 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -131,6 +131,7 @@ static inline struct acpi_hotplug_profile *to_acpi_hotplug_profile(
 struct acpi_scan_handler {
 	const struct acpi_device_id *ids;
 	struct list_head list_node;
+	int (*match)(char *devid, char *handler_id);
 	int (*attach)(struct acpi_device *dev, const struct acpi_device_id *id);
 	void (*detach)(struct acpi_device *dev);
 	struct acpi_hotplug_profile hotplug;
-- 
1.7.9.5

[RFC PATCH 2/8] PNPACPI: use whilte list for pnpacpi device enumeration

[Zhang Rui]

ACPI can be used to enumerate PNP devices, but the code does not
handle this in a good manner.

Currently, if an ACPI device
1. has _CRS method,
2. has an identifications of
   "three capital charactors followed by four hex numbers",
3. is not in the exclude list,
it is enumerated to PNP bus.

So actually, PNP bus is used as the default bus for enumerating _HID devices.

But, nowadays, more and more _HID devices are needed to be enumerate to
platform bus instead. And a white list is used for those devices to avoid
overlapping with PNP bus.
The problem is that this list is continuously growing.

So, a solution that uses platform bus as the default bus for _HID enumeration
is preferred.
In order to do this, this patch changes the way of enumerating PNP devices.
As the first step, we use a white list to create PNP devices instead.
This white list contains all the pnp_device_id strings in all the pnp drivers,
thus this change is transparent to PNP core and all the PNP drivers.

Note: I just grep all the id strings in all pnp_device_id instances and
      copy them to the new white list, with a few changes to the comments
      only, to follow the format of:

      /* driver name, or file name if not a PNP driver */
      {"id-string"}, /* optional comments for the id-string */
      ...

The next steps would be reduce this PNPACPI white list by
1. remove the ids for the devices that are never enumerated via ACPI
2. remove the ids and convert the drivers to platform bus drivers
   for the devices that are not PNP devices in nature.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/scan.c        |    2 +
 drivers/pnp/pnpacpi/core.c |  395 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/pnp.h        |    3 +
 3 files changed, 366 insertions(+), 34 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index dca22eb..5967338 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -11,6 +11,7 @@
 #include <linux/kthread.h>
 #include <linux/dmi.h>
 #include <linux/nls.h>
+#include <linux/pnp.h>
 
 #include <asm/pgtable.h>
 
@@ -2190,6 +2191,7 @@ int __init acpi_scan_init(void)
 	acpi_container_init();
 	acpi_memory_hotplug_init();
 	acpi_dock_init();
+	acpi_pnp_init();
 
 	mutex_lock(&acpi_scan_lock);
 	/*
diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 9f611cb..76df7fc 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -30,26 +30,6 @@
 
 static int num;
 
-/* We need only to blacklist devices that have already an acpi driver that
- * can't use pnp layer. We don't need to blacklist device that are directly
- * used by the kernel (PCI root, ...), as it is harmless and there were
- * already present in pnpbios. But there is an exception for devices that
- * have irqs (PIC, Timer) because we call acpi_register_gsi.
- * Finally, only devices that have a CRS method need to be in this list.
- */
-static struct acpi_device_id excluded_id_list[] __initdata = {
-	{"PNP0C09", 0},		/* EC */
-	{"PNP0C0F", 0},		/* Link device */
-	{"PNP0000", 0},		/* PIC */
-	{"PNP0100", 0},		/* Timer */
-	{"", 0},
-};
-
-static inline int __init is_exclusive_device(struct acpi_device *dev)
-{
-	return (!acpi_match_device_ids(dev, excluded_id_list));
-}
-
 /*
  * Compatible Device IDs
  */
@@ -73,6 +53,317 @@ static int __init ispnpidacpi(const char *id)
 	return 1;
 }
 
+static const struct acpi_device_id acpi_pnp_device_ids[]= {
+	/* pata_isapnp */
+	{"PNP0600"}, /* Generic ESDI/IDE/ATA compatible hard disk controller */
+	/* floppy */
+	{"PNP0700"},
+	/* ipmi_si */
+	{"IPI0001"},
+	/* tpm_inf_pnp */
+	{"IFX0101"}, /* Infineon TPMs */
+	{"IFX0102"}, /* Infineon TPMs */
+	/*tpm_tis */
+	{"PNP0C31"}, /* TPM */
+	{"ATM1200"}, /* Atmel */
+	{"IFX0102"}, /* Infineon */
+	{"BCM0101"}, /* Broadcom */
+	{"BCM0102"}, /* Broadcom */
+	{"NSC1200"}, /* National */
+	{"ICO0102"}, /* Intel */
+	/* ide	 */
+	{"PNP0600"}, /* Generic ESDI/IDE/ATA compatible hard disk controller */
+	/* ns558 */
+	{"@P@0001"}, /* ALS 100 */
+	{"@P@0020"}, /* ALS 200 */
+	{"@P@1001"}, /* ALS 100+ */
+	{"@P@2001"}, /* ALS 120 */
+	{"ASB16fd"}, /* AdLib NSC16 */
+	{"AZT3001"}, /* AZT1008 */
+	{"CDC0001"}, /* Opl3-SAx */
+	{"CSC0001"}, /* CS4232 */
+	{"CSC000f"}, /* CS4236 */
+	{"CSC0101"}, /* CS4327 */
+	{"CTL7001"}, /* SB16 */
+	{"CTL7002"}, /* AWE64 */
+	{"CTL7005"}, /* Vibra16 */
+	{"ENS2020"}, /* SoundscapeVIVO */
+	{"ESS0001"}, /* ES1869 */
+	{"ESS0005"}, /* ES1878 */
+	{"ESS6880"}, /* ES688 */
+	{"IBM0012"}, /* CS4232 */
+	{"OPT0001"}, /* OPTi Audio16 */
+	{"YMH0006"}, /* Opl3-SA */
+	{"YMH0022"}, /* Opl3-SAx */
+	{"PNPb02f"}, /* Generic */
+	/* i8042 kbd */
+	{"PNP0300"},
+	{"PNP0301"},
+	{"PNP0302"},
+	{"PNP0303"},
+	{"PNP0304"},
+	{"PNP0305"},
+	{"PNP0306"},
+	{"PNP0309"},
+	{"PNP030a"},
+	{"PNP030b"},
+	{"PNP0320"},
+	{"PNP0343"},
+	{"PNP0344"},
+	{"PNP0345"},
+	{"CPQA0D7"},
+	/* i8042 aux */
+	{"AUI0200"},
+	{"FJC6000"},
+	{"FJC6001"},
+	{"PNP0f03"},
+	{"PNP0f0b"},
+	{"PNP0f0e"},
+	{"PNP0f12"},
+	{"PNP0f13"},
+	{"PNP0f19"},
+	{"PNP0f1c"},
+	{"SYN0801"},
+	/* fcpnp */
+	{"AVM0900"},
+	/* radio-cadet */
+	{"MSM0c24"}, /* ADS Cadet AM/FM Radio Card */
+	/* radio-gemtek */
+	{"ADS7183"}, /* AOpen FX-3D/Pro Radio */
+	/* radio-sf16fmr2 */
+	{"MFRad13"}, /* tuner subdevice of SF16-FMD2 */
+	/* ene_ir */
+	{"ENE0100"},
+	{"ENE0200"},
+	{"ENE0201"},
+	{"ENE0202"},
+	/* fintek-cir */
+	{"FIT0002"}, /* CIR */
+	/* ite-cir */
+	{"ITE8704"}, /* Default model */
+	{"ITE8713"}, /* CIR found in EEEBox 1501U */
+	{"ITE8708"}, /* Bridged IT8512 */
+	{"ITE8709"}, /* SRAM-Bridged IT8512 */
+	/* nuvoton-cir */
+	{"WEC0530"}, /* CIR */
+	{"NTN0530"}, /* CIR for new chip's pnp id*/
+	/* Winbond CIR */
+	{"WEC1022"},
+	/* wbsd */
+	{"WEC0517"},
+	{"WEC0518"},
+	/* Winbond CIR */
+	{"TCM5090"}, /* 3Com Etherlink III (TP) */
+	{"TCM5091"}, /* 3Com Etherlink III */
+	{"TCM5094"}, /* 3Com Etherlink III (combo) */
+	{"TCM5095"}, /* 3Com Etherlink III (TPO) */
+	{"TCM5098"}, /* 3Com Etherlink III (TPC) */
+	{"PNP80f7"}, /* 3Com Etherlink III compatible */
+	{"PNP80f8"}, /* 3Com Etherlink III compatible */
+	/* nsc-ircc */
+	{"NSC6001"},
+	{"HWPC224"},
+	{"IBM0071"},
+	/* smsc-ircc2 */
+	{"SMCf010"},
+	/* sb1000 */
+	{"GIC1000"},
+	/* parport_pc */
+	{"PNP0400"}, /* Standard LPT Printer Port */
+	{"PNP0401"}, /* ECP Printer Port */
+	/* apple-gmux */
+	{"APP000B"},
+	/* fujitsu-laptop.c */
+	{"FUJ02bf"},
+	{"FUJ02B1"},
+	{"FUJ02E3"},
+	/* system */
+	{"PNP0c02"}, /* General ID for reserving resources */
+	{"PNP0c01"}, /* memory controller */
+	/* rtc_cmos */
+	{"PNP0b00"},
+	{"PNP0b01"},
+	{"PNP0b02"},
+	/* c6xdigio */
+        {"PNP0400"}, /* Standard LPT Printer Port */
+        {"PNP0401"}, /* ECP Printer Port */
+	/* ni_atmio.c */
+	{"NIC1900"},
+	{"NIC2400"},
+	{"NIC2500"},
+	{"NIC2600"},
+	{"NIC2700"},
+	/* serial */
+	{"AAC000F"}, /* Archtek America Corp. Archtek SmartLink Modem 3334BT Plug & Play */
+	{"ADC0001"}, /* Anchor Datacomm BV. SXPro 144 External Data Fax Modem Plug & Play */
+	{"ADC0002"}, /* SXPro 288 External Data Fax Modem Plug & Play */
+	{"AEI0250"}, /* PROLiNK 1456VH ISA PnP K56flex Fax Modem */
+	{"AEI1240"}, /* Actiontec ISA PNP 56K X2 Fax Modem */
+	{"AKY1021"}, /* Rockwell 56K ACF II Fax+Data+Voice Modem */
+	{"AZT4001"}, /* AZT3005 PnP SOUND DEVICE */
+	{"BDP3336"}, /* Best Data Products Inc. Smart One 336F PnP Modem */
+	{"BRI0A49"}, /* Boca Complete Ofc Communicator 14.4 Data-FAX */
+	{"BRI1400"}, /* Boca Research 33,600 ACF Modem */
+	{"BRI3400"}, /* Boca 33.6 Kbps Internal FD34FSVD */
+	{"BRI0A49"}, /* Boca 33.6 Kbps Internal FD34FSVD */
+	{"BDP3336"}, /* Best Data Products Inc. Smart One 336F PnP Modem */
+	{"CPI4050"}, /* Computer Peripherals Inc. EuroViVa CommCenter-33.6 SP PnP */
+	{"CTL3001"}, /* Creative Labs Phone Blaster 28.8 DSVD PnP Voice */
+	{"CTL3011"}, /* Creative Labs Modem Blaster 28.8 DSVD PnP Voice */
+	{"DAV0336"}, /* Davicom ISA 33.6K Modem */
+	{"DMB1032"}, /* Creative Modem Blaster Flash56 DI5601-1 */
+	{"DMB2001"}, /* Creative Modem Blaster V.90 DI5660 */
+	{"ETT0002"}, /* E-Tech CyberBULLET PC56RVP */
+	{"FUJ0202"}, /* Fujitsu 33600 PnP-I2 R Plug & Play */
+	{"FUJ0205"}, /* Fujitsu FMV-FX431 Plug & Play */
+	{"FUJ0206"}, /* Fujitsu 33600 PnP-I4 R Plug & Play */
+	{"FUJ0209"}, /* Fujitsu Fax Voice 33600 PNP-I5 R Plug & Play */
+	{"GVC000F"}, /* Archtek SmartLink Modem 3334BT Plug & Play */
+	{"GVC0303"}, /* Archtek SmartLink Modem 3334BRV 33.6K Data Fax Voice */
+	{"HAY0001"}, /* Hayes Optima 288 V.34-V.FC + FAX + Voice Plug & Play */
+	{"HAY000C"}, /* Hayes Optima 336 V.34 + FAX + Voice PnP */
+	{"HAY000D"}, /* Hayes Optima 336B V.34 + FAX + Voice PnP */
+	{"HAY5670"}, /* Hayes Accura 56K Ext Fax Modem PnP */
+	{"HAY5674"}, /* Hayes Accura 56K Ext Fax Modem PnP */
+	{"HAY5675"}, /* Hayes Accura 56K Fax Modem PnP */
+	{"HAYF000"}, /* Hayes 288, V.34 + FAX */
+	{"HAYF001"}, /* Hayes Optima 288 V.34 + FAX + Voice, Plug & Play */
+	{"IBM0033"}, /* IBM Thinkpad 701 Internal Modem Voice */
+	{"PNP4972"}, /* Intermec CV60 touchscreen port */
+	{"IXDC801"}, /* Intertex 28k8 33k6 Voice EXT PnP */
+	{"IXDC901"}, /* Intertex 33k6 56k Voice EXT PnP */
+	{"IXDD801"}, /* Intertex 28k8 33k6 Voice SP EXT PnP */
+	{"IXDD901"}, /* Intertex 33k6 56k Voice SP EXT PnP */
+	{"IXDF401"}, /* Intertex 28k8 33k6 Voice SP INT PnP */
+	{"IXDF801"}, /* Intertex 28k8 33k6 Voice SP EXT PnP */
+	{"IXDF901"}, /* Intertex 33k6 56k Voice SP EXT PnP */
+	{"KOR4522"}, /* KORTEX 28800 Externe PnP */
+	{"KORF661"}, /* KXPro 33.6 Vocal ASVD PnP */
+	{"LAS4040"}, /* LASAT Internet 33600 PnP */
+	{"LAS4540"}, /* Lasat Safire 560 PnP */
+	{"LAS5440"}, /* Lasat Safire 336  PnP */
+	{"MNP0281"}, /* Microcom TravelPorte FAST V.34 Plug & Play */
+	{"MNP0336"}, /* Microcom DeskPorte V.34 FAST or FAST+ Plug & Play */
+	{"MNP0339"}, /* Microcom DeskPorte FAST EP 28.8 Plug & Play */
+	{"MNP0342"}, /* Microcom DeskPorte 28.8P Plug & Play */
+	{"MNP0500"}, /* Microcom DeskPorte FAST ES 28.8 Plug & Play */
+	{"MNP0501"}, /* Microcom DeskPorte FAST ES 28.8 Plug & Play */
+	{"MNP0502"}, /* Microcom DeskPorte 28.8S Internal Plug & Play */
+	{"MOT1105"}, /* Motorola BitSURFR Plug & Play */
+	{"MOT1111"}, /* Motorola TA210 Plug & Play */
+	{"MOT1114"}, /* Motorola HMTA 200 (ISDN) Plug & Play */
+	{"MOT1115"}, /* Motorola BitSURFR Plug & Play */
+	{"MOT1190"}, /* Motorola Lifestyle 28.8 Internal */
+	{"MOT1501"}, /* Motorola V.3400 Plug & Play */
+	{"MOT1502"}, /* Motorola Lifestyle 28.8 V.34 Plug & Play */
+	{"MOT1505"}, /* Motorola Power 28.8 V.34 Plug & Play */
+	{"MOT1509"}, /* Motorola ModemSURFR External 28.8 Plug & Play */
+	{"MOT150A"}, /* Motorola Premier 33.6 Desktop Plug & Play */
+	{"MOT150F"}, /* Motorola VoiceSURFR 56K External PnP */
+	{"MOT1510"}, /* Motorola ModemSURFR 56K External PnP */
+	{"MOT1550"}, /* Motorola ModemSURFR 56K Internal PnP */
+	{"MOT1560"}, /* Motorola ModemSURFR Internal 28.8 Plug & Play */
+	{"MOT1580"}, /* Motorola Premier 33.6 Internal Plug & Play */
+	{"MOT15B0"}, /* Motorola OnlineSURFR 28.8 Internal Plug & Play */
+	{"MOT15F0"}, /* Motorola VoiceSURFR 56K Internal PnP */
+	{"MVX00A1"}, /*  Deskline K56 Phone System PnP */
+	{"MVX00F2"}, /* PC Rider K56 Phone System PnP */
+	{"nEC8241"}, /* NEC 98NOTE SPEAKER PHONE FAX MODEM(33600bps) */
+	{"PMC2430"}, /* Pace 56 Voice Internal Plug & Play Modem */
+	{"PNP0500"}, /* Generic standard PC COM port	 */
+	{"PNP0501"}, /* Generic 16550A-compatible COM port */
+	{"PNPC000"}, /* Compaq 14400 Modem */
+	{"PNPC001"}, /* Compaq 2400/9600 Modem */
+	{"PNPC031"}, /* Dial-Up Networking Serial Cable between 2 PCs */
+	{"PNPC032"}, /* Dial-Up Networking Parallel Cable between 2 PCs */
+	{"PNPC100"}, /* Standard 9600 bps Modem */
+	{"PNPC101"}, /* Standard 14400 bps Modem */
+	{"PNPC102"}, /*  Standard 28800 bps Modem*/
+	{"PNPC103"}, /*  Standard Modem*/
+	{"PNPC104"}, /*  Standard 9600 bps Modem*/
+	{"PNPC105"}, /*  Standard 14400 bps Modem*/
+	{"PNPC106"}, /*  Standard 28800 bps Modem*/
+	{"PNPC107"}, /*  Standard Modem */
+	{"PNPC108"}, /* Standard 9600 bps Modem */
+	{"PNPC109"}, /* Standard 14400 bps Modem */
+	{"PNPC10A"}, /* Standard 28800 bps Modem */
+	{"PNPC10B"}, /* Standard Modem */
+	{"PNPC10C"}, /* Standard 9600 bps Modem */
+	{"PNPC10D"}, /* Standard 14400 bps Modem */
+	{"PNPC10E"}, /* Standard 28800 bps Modem */
+	{"PNPC10F"}, /* Standard Modem */
+	{"PNP2000"}, /* Standard PCMCIA Card Modem */
+	{"ROK0030"}, /* Rockwell 33.6 DPF Internal PnP, Modular Technology 33.6 Internal PnP */
+	{"ROK0100"}, /* KORTEX 14400 Externe PnP */
+	{"ROK4120"}, /* Rockwell 28.8 */
+	{"ROK4920"}, /* Viking 28.8 INTERNAL Fax+Data+Voice PnP */
+	{"RSS00A0"}, /* Rockwell 33.6 DPF External PnP, BT Prologue 33.6 External PnP, Modular Technology 33.6 External PnP */
+	{"RSS0262"}, /* Viking 56K FAX INT */
+	{"RSS0250"}, /* K56 par,VV,Voice,Speakphone,AudioSpan,PnP */
+	{"SUP1310"}, /* SupraExpress 28.8 Data/Fax PnP modem */
+	{"SUP1381"}, /* SupraExpress 336i PnP Voice Modem */
+	{"SUP1421"}, /* SupraExpress 33.6 Data/Fax PnP modem */
+	{"SUP1590"}, /* SupraExpress 33.6 Data/Fax PnP modem */
+	{"SUP1620"}, /* SupraExpress 336i Sp ASVD */
+	{"SUP1760"}, /* SupraExpress 33.6 Data/Fax PnP modem */
+	{"SUP2171"}, /* SupraExpress 56i Sp Intl */
+	{"TEX0011"}, /* Phoebe Micro 33.6 Data Fax 1433VQH Plug & Play */
+	{"UAC000F"}, /* Archtek SmartLink Modem 3334BT Plug & Play */
+	{"USR0000"}, /* 3Com Corp. Gateway Telepath IIvi 33.6 */
+	{"USR0002"}, /* U.S. Robotics Sporster 33.6K Fax INT PnP */
+	{"USR0004"}, /*  Sportster Vi 14.4 PnP FAX Voicemail */
+	{"USR0006"}, /* U.S. Robotics 33.6K Voice INT PnP */
+	{"USR0007"}, /* U.S. Robotics 33.6K Voice EXT PnP */
+	{"USR0009"}, /* U.S. Robotics Courier V.Everything INT PnP */
+	{"USR2002"}, /* U.S. Robotics 33.6K Voice INT PnP */
+	{"USR2070"}, /* U.S. Robotics 56K Voice INT PnP */
+	{"USR2080"}, /* U.S. Robotics 56K Voice EXT PnP */
+	{"USR3031"}, /* U.S. Robotics 56K FAX INT */
+	{"USR3050"}, /* U.S. Robotics 56K FAX INT */
+	{"USR3070"}, /* U.S. Robotics 56K Voice INT PnP */
+	{"USR3080"}, /* U.S. Robotics 56K Voice EXT PnP */
+	{"USR3090"}, /* U.S. Robotics 56K Voice INT PnP */
+	{"USR9100"}, /* U.S. Robotics 56K Message  */
+	{"USR9160"}, /* U.S. Robotics 56K FAX EXT PnP*/
+	{"USR9170"}, /* U.S. Robotics 56K FAX INT PnP*/
+	{"USR9180"}, /* U.S. Robotics 56K Voice EXT PnP*/
+	{"USR9190"}, /* U.S. Robotics 56K Voice INT PnP*/
+	{"WACFXXX"}, /* Wacom tablets */
+	{"FPI2002"}, /* Compaq touchscreen */
+	{"FUJ02B2"}, /* Fujitsu Stylistic touchscreens */
+	{"FUJ02B3"},
+	{"FUJ02B4"}, /* Fujitsu Stylistic LT touchscreens */
+	{"FUJ02B6"}, /* Passive Fujitsu Stylistic touchscreens */
+	{"FUJ02B7"},
+	{"FUJ02B8"},
+	{"FUJ02B9"},
+	{"FUJ02BC"},
+	{"FUJ02E5"}, /* Fujitsu Wacom Tablet PC device */
+	{"FUJ02E6"}, /* Fujitsu P-series tablet PC device */
+	{"FUJ02E7"}, /* Fujitsu Wacom 2FGT Tablet PC device */
+	{"FUJ02E9"}, /* Fujitsu Wacom 1FGT Tablet PC device */
+	{"LTS0001"}, /* LG C1 EXPRESS DUAL (C1-PB11A3) touch screen (actually a FUJ02E6 in disguise) */
+	{"WCI0003"}, /* Rockwell's (PORALiNK) 33600 INT PNP */
+	{"WEC1022"}, /* Winbond CIR port, should not be probed. We should keep track of it to prevent the legacy serial driver from probing it */
+	{"PNPCXXX"}, /* Unknown PnP modems */
+	{"PNPDXXX"}, /* More unknown PnP modems */
+	/* scl200wdt */
+	{"NSC0800"}, /* National Semiconductor PC87307/PC97307 watchdog component */
+	/* mpu401 */
+	{"PNPb006"},
+	/* cs423x-pnpbios */
+	{"CSC0100"},
+	{"CSC0000"},
+	{"GIM0100"}, /* Guillemot Turtlebeach something appears to be cs4232 compatible */
+	/* es18xx-pnpbios */
+	{"ESS1869"},
+	{"ESS1879"},
+	/* snd-opl3sa2-pnpbios */
+	{"YMH0021"},
+	{"NMX2210"}, /* Gateway Solo 2500 */
+	{""},
+};
+
 static int pnpacpi_get_resources(struct pnp_dev *dev)
 {
 	pnp_dbg(&dev->dev, "get resources\n");
@@ -258,7 +549,7 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	if (!pnpid)
 		return 0;
 
-	if (is_exclusive_device(device) || !device->status.present)
+	if (!device->status.present)
 		return 0;
 
 	dev = pnp_alloc_dev(&pnpacpi_protocol, num, pnpid);
@@ -312,19 +603,6 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	return 0;
 }
 
-static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
-						     u32 lvl, void *context,
-						     void **rv)
-{
-	struct acpi_device *device;
-
-	if (!acpi_bus_get_device(handle, &device))
-		pnpacpi_add_device(device);
-	else
-		return AE_CTRL_DEPTH;
-	return AE_OK;
-}
-
 static int __init acpi_pnp_match(struct device *dev, void *_pnp)
 {
 	struct acpi_device *acpi = to_acpi_device(dev);
@@ -361,6 +639,55 @@ static struct acpi_bus_type __initdata acpi_pnp_bus = {
 };
 
 int pnpacpi_disabled __initdata;
+
+static int __init acpi_pnp_scan_handler_attach(struct acpi_device *adev,
+                                   const struct acpi_device_id *id)
+{
+	return 1;
+}
+
+static int __init acpi_pnp_scan_handler_match(char *devid, char *handlerid)
+{
+	int i;
+
+	if (!ispnpidacpi(devid))
+		return 0;
+
+	if (memcmp(devid, handlerid, 3))
+		return 0;
+
+        for (i = 3; i < 7; i++) {
+                if (handlerid[i] != 'X' &&
+		    toupper(devid[i]) != toupper(handlerid[i]))
+                        return 0;
+        }
+	return 1;
+}
+
+static struct acpi_scan_handler pnpacpi_handler __initdata = {
+	.ids = acpi_pnp_device_ids,
+	.match = acpi_pnp_scan_handler_match,
+	.attach = acpi_pnp_scan_handler_attach,
+};
+
+void __init acpi_pnp_init(void)
+{
+	acpi_scan_add_handler(&pnpacpi_handler);
+}
+
+static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
+						     u32 lvl, void *context,
+						     void **rv)
+{
+	struct acpi_device *device;
+
+	if (acpi_bus_get_device(handle, &device))
+		return AE_CTRL_DEPTH;
+	if (device->handler == &pnpacpi_handler)
+		pnpacpi_add_device(device);
+	return AE_OK;
+}
+
 static int __init pnpacpi_init(void)
 {
 	if (acpi_disabled || pnpacpi_disabled) {
diff --git a/include/linux/pnp.h b/include/linux/pnp.h
index 195aafc..b329699 100644
--- a/include/linux/pnp.h
+++ b/include/linux/pnp.h
@@ -343,8 +343,11 @@ static inline struct acpi_device *pnp_acpi_device(struct pnp_dev *dev)
 		return dev->data;
 	return NULL;
 }
+
+void acpi_pnp_init(void);
 #else
 #define pnp_acpi_device(dev) 0
+static inline void acpi_pnp_init(void) {}
 #endif
 
 /* status */
-- 
1.7.9.5

[RFC PATCH 3/8] PNPACPI: remove ids that does not comply with the ACPI PNP id rule

[Zhang Rui]

The PNPACPI white list just copies all the ids from all the
struct pnp_device_id instances, but some of them do not
comply with the ACPI PNP id rule (3 Alpha Charactors + 4 Hex numbers).

For those ids, the coressponding devices will never be enumerated
via ACPI, so it is safe to remove those ids from the PNPACPI white list.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/pnp/pnpacpi/core.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 76df7fc..d47fbdf 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -74,10 +74,6 @@ static const struct acpi_device_id acpi_pnp_device_ids[]= {
 	/* ide	 */
 	{"PNP0600"}, /* Generic ESDI/IDE/ATA compatible hard disk controller */
 	/* ns558 */
-	{"@P@0001"}, /* ALS 100 */
-	{"@P@0020"}, /* ALS 200 */
-	{"@P@1001"}, /* ALS 100+ */
-	{"@P@2001"}, /* ALS 120 */
 	{"ASB16fd"}, /* AdLib NSC16 */
 	{"AZT3001"}, /* AZT1008 */
 	{"CDC0001"}, /* Opl3-SAx */
-- 
1.7.9.5

[RFC PATCH 4/8] PNPACPI: remove unsupported serial PNP ids from PNPACPI id list

[Zhang Rui]

The "serial" pnp driver supports some unknown PNP modems (PNPCXXX/PNPDXXX)
by matching magic strings in the pnp device name or the pnp device card name.

ACPI enumerated PNP device neither supports pnp card,
nor supports those magic strings in its device name,
which means this mechamism never works for ACPI enumerated
PNPCXXX/PNPDXXX devices.
So it is safe to remove those two ids from the PNPACPI id list.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/pnp/pnpacpi/core.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index d47fbdf..36dda39 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -341,8 +341,6 @@ static const struct acpi_device_id acpi_pnp_device_ids[]= {
 	{"LTS0001"}, /* LG C1 EXPRESS DUAL (C1-PB11A3) touch screen (actually a FUJ02E6 in disguise) */
 	{"WCI0003"}, /* Rockwell's (PORALiNK) 33600 INT PNP */
 	{"WEC1022"}, /* Winbond CIR port, should not be probed. We should keep track of it to prevent the legacy serial driver from probing it */
-	{"PNPCXXX"}, /* Unknown PnP modems */
-	{"PNPDXXX"}, /* More unknown PnP modems */
 	/* scl200wdt */
 	{"NSC0800"}, /* National Semiconductor PC87307/PC97307 watchdog component */
 	/* mpu401 */
-- 
1.7.9.5

[RFC PATCH 5/8] PNPACPI: check and enumerate CMOS RTC devices explicitly

[Zhang Rui]

For CMOS RTC devices, the pnpacpi scan handler does not work because
there is already a cmos rtc scan handler installed, thus we need to
check those devices and enumerate them to PNP bus explicitly.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/pnp/pnpacpi/core.c |   22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 36dda39..1ee7eb7 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -176,10 +176,6 @@ static const struct acpi_device_id acpi_pnp_device_ids[]= {
 	/* system */
 	{"PNP0c02"}, /* General ID for reserving resources */
 	{"PNP0c01"}, /* memory controller */
-	/* rtc_cmos */
-	{"PNP0b00"},
-	{"PNP0b01"},
-	{"PNP0b02"},
 	/* c6xdigio */
         {"PNP0400"}, /* Standard LPT Printer Port */
         {"PNP0401"}, /* ECP Printer Port */
@@ -669,6 +665,22 @@ void __init acpi_pnp_init(void)
 	acpi_scan_add_handler(&pnpacpi_handler);
 }
 
+/*
+ * For CMOS RTC devices, the pnpacpi scan handler does not work because
+ * there is already a cmos rtc scan handler installed, thus we need to
+ * check those devices and enumerate them to PNP bus explicitly.
+ */
+static int is_cmos_rtc_device(struct acpi_device *adev)
+{
+	struct acpi_device_id ids[] = {
+		{ "PNP0B00" },
+		{ "PNP0B01" },
+		{ "PNP0B02" },
+		{""},
+	};
+	return !acpi_match_device_ids(adev, ids);
+}
+
 static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
 						     u32 lvl, void *context,
 						     void **rv)
@@ -677,7 +689,7 @@ static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
 
 	if (acpi_bus_get_device(handle, &device))
 		return AE_CTRL_DEPTH;
-	if (device->handler == &pnpacpi_handler)
+	if (device->handler == &pnpacpi_handler || is_cmos_rtc_device(device))
 		pnpacpi_add_device(device);
 	return AE_OK;
 }
-- 
1.7.9.5

[RFC PATCH 6/8] ACPI: use platform bus as the default bus for _HID enumeration

[Zhang Rui]

Because of the growing demand for enumerating ACPI devices to platform bus,
this patch changes the code to enumerate ACPI devices with _HID/_CID to
platform bus by default, unless the device already has a scan handler attached.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/acpi_platform.c |   28 ----------------------------
 drivers/acpi/scan.c          |   12 ++++++------
 2 files changed, 6 insertions(+), 34 deletions(-)

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index dbfe49e..33376a9 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -22,24 +22,6 @@
 
 ACPI_MODULE_NAME("platform");
 
-/*
- * The following ACPI IDs are known to be suitable for representing as
- * platform devices.
- */
-static const struct acpi_device_id acpi_platform_device_ids[] = {
-
-	{ "PNP0D40" },
-	{ "ACPI0003" },
-	{ "VPC2004" },
-	{ "BCM4752" },
-
-	/* Intel Smart Sound Technology */
-	{ "INT33C8" },
-	{ "80860F28" },
-
-	{ }
-};
-
 /**
  * acpi_create_platform_device - Create platform device for ACPI device node
  * @adev: ACPI device node to create a platform device for.
@@ -125,13 +107,3 @@ int acpi_create_platform_device(struct acpi_device *adev,
 	kfree(resources);
 	return 1;
 }
-
-static struct acpi_scan_handler platform_handler = {
-	.ids = acpi_platform_device_ids,
-	.attach = acpi_create_platform_device,
-};
-
-void __init acpi_platform_init(void)
-{
-	acpi_scan_add_handler(&platform_handler);
-}
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 5967338..61af32e 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2022,14 +2022,15 @@ static int acpi_scan_attach_handler(struct acpi_device *device)
 		handler = acpi_scan_match_handler(hwid->id, &devid);
 		if (handler) {
 			ret = handler->attach(device, devid);
-			if (ret > 0) {
+			if (ret > 0)
 				device->handler = handler;
-				break;
-			} else if (ret < 0) {
-				break;
-			}
+			if (ret)
+				goto end;
 		}
 	}
+end:
+	if (!list_empty(&device->pnp.ids) && !device->handler)
+		acpi_create_platform_device(device, NULL);
 	return ret;
 }
 
@@ -2185,7 +2186,6 @@ int __init acpi_scan_init(void)
 	acpi_pci_root_init();
 	acpi_pci_link_init();
 	acpi_processor_init();
-	acpi_platform_init();
 	acpi_lpss_init();
 	acpi_cmos_rtc_init();
 	acpi_container_init();
-- 
1.7.9.5

[RFC PATCH 7/8] Revert "ACPI / PNP: skip ACPI device nodes associated with physical nodes already"

[Zhang Rui]

This reverts commit 2905875344f977acd188a2b0f1d163491e91459b.

commit 2905875344f977acd188a2b0f1d163491e91459b was introduced to prevent
PNP device objects from being created for ACPI device nodes already associated
with platform devices.

This is not needed any more because the platform device node won't be created
if a device has already been attached to the PNPACPI scan handler.

Plus, in some cases, we may need both PNP node and platform node for the
same ACPI device object, on purpose, like what I will do in next patch.

Thus reverting this commit.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/pnp/pnpacpi/core.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 1ee7eb7..739fa24 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -524,10 +524,6 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	struct acpi_hardware_id *id;
 	int error;
 
-	/* Skip devices that are already bound */
-	if (device->physical_node_count)
-		return 0;
-
 	/*
 	 * If a PnPacpi device is not present , the device
 	 * driver should not be loaded.
-- 
1.7.9.5

[RFC PATCH 8/8] PNPACPI: create both PNP and Platform device nodes for PNP0C01/PNP0C02

[Zhang Rui]

ACPI devices with id "PNP0C01/PNP0C02" means that we need to
protect their resources from being allocated by others.

Currently, this is done in drivers/pnp/system.c.

But the problem is that, there are some devices with extra ids besides
PNP0C01/PNP0C02, and for these devices,
1) PNP0C01/PNP0C02 suggest that resource reservation is still needed.
2) the other ids suggest that we should enumerate them to platform bus

To reserve resources for those devices, we should either use the current code
by exporting the device to PNP bus, or introduce resource reservation support
in platform bus/ACPI.

This patch follows the first way by enumerating an ACPI device to platform bus
AND pnp bus at the same time.
Its PNP device node will be probed by drivers/pnp/system.c and do
everything as we do today.
Its platform device node will also be created so that a platform bus
driver can be probed.

The advantage is that, it brings little change to the current code,
the patch itself looks safe and clear.
The disadvantage is that
1) we create two physical device nodes for the same ACPI node,
   this is against our effort that has been doing recently.
2) we still depend on PNP bus to do this (resouce reservation) for us,
   which is still a problem we need to fix sooner or later.

An alternative proposal is to remove the depedency of PNP bus and
do resource management in ACPI for all PNP0C01/PNP0C02 devices instead,
no matter what bus they are enumerated to.
To do this, we need to
1) introduce a fs_initcall() in ACPI, to reserve all PNP0C01/PNP0C02 resources
in ACPI, something like we did via drivers/acpi/motherboard.c before
(but the code needs to follow drivers/pnp/quirks.c and system.c strictly).
This initcall will be run after PCI claiming BARs and before PCI assigning
resources for uninitialized devices.
2) skip drivers/pnp/quirks.c and drivers/pnp/system.c for ACPI
   enumerted PNP devices, by checking pnp_device->protocal.
3) remove PNP0C01/PNP0C02 from PNPACPI white list.

By doing this, we can remove the depedency of PNP bus, but this requires
a lot of code duplication(need to copy quirks.c and system.c logic into ACPI),
which does not look good neither.

Any comments will be appreciated.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/pnp/pnpacpi/core.c |   20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 739fa24..5b13600 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -173,9 +173,6 @@ static const struct acpi_device_id acpi_pnp_device_ids[]= {
 	{"FUJ02bf"},
 	{"FUJ02B1"},
 	{"FUJ02E3"},
-	/* system */
-	{"PNP0c02"}, /* General ID for reserving resources */
-	{"PNP0c01"}, /* memory controller */
 	/* c6xdigio */
         {"PNP0400"}, /* Standard LPT Printer Port */
         {"PNP0401"}, /* ECP Printer Port */
@@ -677,6 +674,20 @@ static int is_cmos_rtc_device(struct acpi_device *adev)
 	return !acpi_match_device_ids(adev, ids);
 }
 
+/*
+ * For devices with id "PNP0C01"/"PNP0C02", they will be enumerated
+ * to PNP bus anyway to do resource reservation.
+ */
+static int is_system_device(struct acpi_device *adev)
+{
+	struct acpi_device_id ids[] = {
+		{"PNP0C02"},
+		{"PNP0C01"},
+		{""},
+	};
+	return !acpi_match_device_ids(adev, ids);
+}
+
 static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
 						     u32 lvl, void *context,
 						     void **rv)
@@ -685,7 +696,8 @@ static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
 
 	if (acpi_bus_get_device(handle, &device))
 		return AE_CTRL_DEPTH;
-	if (device->handler == &pnpacpi_handler || is_cmos_rtc_device(device))
+	if (device->handler == &pnpacpi_handler || is_system_device(device) ||
+	    is_cmos_rtc_device(device))
 		pnpacpi_add_device(device);
 	return AE_OK;
 }
-- 
1.7.9.5

Re: [RFC PATCH 0/8] ACPI: change the way of enumerating PNPACPI/Platform devices

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1198 bytes --]

On Wed, 2014-02-26 at 17:11 +0800, Zhang Rui wrote:

> 1. without the patch, the acpi_platform scan handler white list is
>    continuously growing.
> 2. with the patch set, the PNPACPI scan handler white list will become
>    smaller and smaller by
>    a) remove the ids from the PNPACPI white list, for the devices
>       that are never enumerated via ACPI
>    b) remove the ids from the PNPACPI whilte list and convert the drivers to
>       platform bus drivers, for the devices that are not PNP devices in nature.
>    which will be done later.

I agree that this is the right direction to go in. The number of acpipnp
devices that are also pnpbios or (even worse) isapnp is (a) unlikely to
increase, and (b) *very* small, and pretty much limited to superio and a
small number of sound cards on old laptops. We may come up with a better
solution for them later, but right now shifting away from requiring that
every new ACPI device driver also bump a whitelist is an improvement.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus for _HID enumeration

[Rafael J. Wysocki]

On Wednesday, February 26, 2014 05:11:12 PM Zhang Rui wrote:
> Because of the growing demand for enumerating ACPI devices to platform bus,
> this patch changes the code to enumerate ACPI devices with _HID/_CID to
> platform bus by default, unless the device already has a scan handler attached.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/acpi_platform.c |   28 ----------------------------
>  drivers/acpi/scan.c          |   12 ++++++------
>  2 files changed, 6 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> index dbfe49e..33376a9 100644
> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -22,24 +22,6 @@
>  
>  ACPI_MODULE_NAME("platform");
>  
> -/*
> - * The following ACPI IDs are known to be suitable for representing as
> - * platform devices.
> - */
> -static const struct acpi_device_id acpi_platform_device_ids[] = {
> -
> -	{ "PNP0D40" },
> -	{ "ACPI0003" },
> -	{ "VPC2004" },
> -	{ "BCM4752" },
> -
> -	/* Intel Smart Sound Technology */
> -	{ "INT33C8" },
> -	{ "80860F28" },
> -
> -	{ }
> -};
> -
>  /**
>   * acpi_create_platform_device - Create platform device for ACPI device node
>   * @adev: ACPI device node to create a platform device for.
> @@ -125,13 +107,3 @@ int acpi_create_platform_device(struct acpi_device *adev,
>  	kfree(resources);
>  	return 1;
>  }
> -
> -static struct acpi_scan_handler platform_handler = {
> -	.ids = acpi_platform_device_ids,
> -	.attach = acpi_create_platform_device,
> -};
> -
> -void __init acpi_platform_init(void)
> -{
> -	acpi_scan_add_handler(&platform_handler);
> -}
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 5967338..61af32e 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2022,14 +2022,15 @@ static int acpi_scan_attach_handler(struct acpi_device *device)
>  		handler = acpi_scan_match_handler(hwid->id, &devid);
>  		if (handler) {
>  			ret = handler->attach(device, devid);
> -			if (ret > 0) {
> +			if (ret > 0)
>  				device->handler = handler;
> -				break;
> -			} else if (ret < 0) {
> -				break;
> -			}
> +			if (ret)
> +				goto end;
>  		}
>  	}
> +end:
> +	if (!list_empty(&device->pnp.ids) && !device->handler)

I'm a bit concerned that this check will create platform devices for too many
ACPI device objects.  Shouldn't we require that _HID or at least _CID is
present for that?

> +		acpi_create_platform_device(device, NULL);
>  	return ret;
>  }
>  
> @@ -2185,7 +2186,6 @@ int __init acpi_scan_init(void)
>  	acpi_pci_root_init();
>  	acpi_pci_link_init();
>  	acpi_processor_init();
> -	acpi_platform_init();
>  	acpi_lpss_init();
>  	acpi_cmos_rtc_init();
>  	acpi_container_init();
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH 0/8] ACPI: change the way of enumerating PNPACPI/Platform devices

[Zhang Rui]

On Wed, 2014-02-26 at 16:47 +0000, Matthew Garrett wrote:
> On Wed, 2014-02-26 at 17:11 +0800, Zhang Rui wrote:
> 
> > 1. without the patch, the acpi_platform scan handler white list is
> >    continuously growing.
> > 2. with the patch set, the PNPACPI scan handler white list will become
> >    smaller and smaller by
> >    a) remove the ids from the PNPACPI white list, for the devices
> >       that are never enumerated via ACPI
> >    b) remove the ids from the PNPACPI whilte list and convert the drivers to
> >       platform bus drivers, for the devices that are not PNP devices in nature.
> >    which will be done later.
> 
> I agree that this is the right direction to go in. The number of acpipnp
> devices that are also pnpbios or (even worse) isapnp is (a) unlikely to
> increase, and (b) *very* small, and pretty much limited to superio and a
> small number of sound cards on old laptops. We may come up with a better
> solution for them later, but right now shifting away from requiring that
> every new ACPI device driver also bump a whitelist is an improvement.
> 
thanks for your comments, Matthew.

thanks,
rui

Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus for _HID enumeration

[Zhang Rui]

On Mon, 2014-03-03 at 00:51 +0100, Rafael J. Wysocki wrote:
> On Wednesday, February 26, 2014 05:11:12 PM Zhang Rui wrote:
> > Because of the growing demand for enumerating ACPI devices to platform bus,
> > this patch changes the code to enumerate ACPI devices with _HID/_CID to
> > platform bus by default, unless the device already has a scan handler attached.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/acpi/acpi_platform.c |   28 ----------------------------
> >  drivers/acpi/scan.c          |   12 ++++++------
> >  2 files changed, 6 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> > index dbfe49e..33376a9 100644
> > --- a/drivers/acpi/acpi_platform.c
> > +++ b/drivers/acpi/acpi_platform.c
> > @@ -22,24 +22,6 @@
> >  
> >  ACPI_MODULE_NAME("platform");
> >  
> > -/*
> > - * The following ACPI IDs are known to be suitable for representing as
> > - * platform devices.
> > - */
> > -static const struct acpi_device_id acpi_platform_device_ids[] = {
> > -
> > -	{ "PNP0D40" },
> > -	{ "ACPI0003" },
> > -	{ "VPC2004" },
> > -	{ "BCM4752" },
> > -
> > -	/* Intel Smart Sound Technology */
> > -	{ "INT33C8" },
> > -	{ "80860F28" },
> > -
> > -	{ }
> > -};
> > -
> >  /**
> >   * acpi_create_platform_device - Create platform device for ACPI device node
> >   * @adev: ACPI device node to create a platform device for.
> > @@ -125,13 +107,3 @@ int acpi_create_platform_device(struct acpi_device *adev,
> >  	kfree(resources);
> >  	return 1;
> >  }
> > -
> > -static struct acpi_scan_handler platform_handler = {
> > -	.ids = acpi_platform_device_ids,
> > -	.attach = acpi_create_platform_device,
> > -};
> > -
> > -void __init acpi_platform_init(void)
> > -{
> > -	acpi_scan_add_handler(&platform_handler);
> > -}
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 5967338..61af32e 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -2022,14 +2022,15 @@ static int acpi_scan_attach_handler(struct acpi_device *device)
> >  		handler = acpi_scan_match_handler(hwid->id, &devid);
> >  		if (handler) {
> >  			ret = handler->attach(device, devid);
> > -			if (ret > 0) {
> > +			if (ret > 0)
> >  				device->handler = handler;
> > -				break;
> > -			} else if (ret < 0) {
> > -				break;
> > -			}
> > +			if (ret)
> > +				goto end;
> >  		}
> >  	}
> > +end:
> > +	if (!list_empty(&device->pnp.ids) && !device->handler)
> 
> I'm a bit concerned that this check will create platform devices for too many
> ACPI device objects.

agreed. there are some devices created unexpected by this patch, e.g. on
my test machine, I can see
 
/sys/bus/platform/devices/LNXSYSTM:00 (ACPI system bus/root node)
/sys/bus/platform/devices/PNP0000:00  (PIC)
/sys/bus/platform/devices/PNP0100:00  (system timer?)

>   Shouldn't we require that _HID or at least _CID is
> present for that?
> 
I do not think so.
only devices that invoke acpi_add_ids() may have pnp.ids but no
_HID/_CID, right?
I did a check in the code, those devices include:
ACPI root node
ACPI video
ACPI bay
ACPI dock
IBM SMBus
ACPI Power resource
ACPI processor
ACPI thermal
ACPI fixed power/sleep button

IMO, only the ACPI root node, ACPI power resource, possibly ACPI
processor are the ones that we do not want to see in platform bus.

Thus IMO, we can have an exclude list for those devices.

thanks,
rui
> > +		acpi_create_platform_device(device, NULL);
> >  	return ret;
> >  }
> >  
> > @@ -2185,7 +2186,6 @@ int __init acpi_scan_init(void)
> >  	acpi_pci_root_init();
> >  	acpi_pci_link_init();
> >  	acpi_processor_init();
> > -	acpi_platform_init();
> >  	acpi_lpss_init();
> >  	acpi_cmos_rtc_init();
> >  	acpi_container_init();
> > 
> 

Re: [RFC PATCH 8/8] PNPACPI: create both PNP and Platform device nodes for PNP0C01/PNP0C02

[Zhang Rui]

Hi, Bjorn,

do you have comments for this particular patch?

thanks,
rui

On Wed, 2014-02-26 at 17:11 +0800, Zhang Rui wrote:
> ACPI devices with id "PNP0C01/PNP0C02" means that we need to
> protect their resources from being allocated by others.
> 
> Currently, this is done in drivers/pnp/system.c.
> 
> But the problem is that, there are some devices with extra ids besides
> PNP0C01/PNP0C02, and for these devices,
> 1) PNP0C01/PNP0C02 suggest that resource reservation is still needed.
> 2) the other ids suggest that we should enumerate them to platform bus
> 
> To reserve resources for those devices, we should either use the current code
> by exporting the device to PNP bus, or introduce resource reservation support
> in platform bus/ACPI.
> 
> This patch follows the first way by enumerating an ACPI device to platform bus
> AND pnp bus at the same time.
> Its PNP device node will be probed by drivers/pnp/system.c and do
> everything as we do today.
> Its platform device node will also be created so that a platform bus
> driver can be probed.
> 
> The advantage is that, it brings little change to the current code,
> the patch itself looks safe and clear.
> The disadvantage is that
> 1) we create two physical device nodes for the same ACPI node,
>    this is against our effort that has been doing recently.
> 2) we still depend on PNP bus to do this (resouce reservation) for us,
>    which is still a problem we need to fix sooner or later.
> 
> An alternative proposal is to remove the depedency of PNP bus and
> do resource management in ACPI for all PNP0C01/PNP0C02 devices instead,
> no matter what bus they are enumerated to.
> To do this, we need to
> 1) introduce a fs_initcall() in ACPI, to reserve all PNP0C01/PNP0C02 resources
> in ACPI, something like we did via drivers/acpi/motherboard.c before
> (but the code needs to follow drivers/pnp/quirks.c and system.c strictly).
> This initcall will be run after PCI claiming BARs and before PCI assigning
> resources for uninitialized devices.
> 2) skip drivers/pnp/quirks.c and drivers/pnp/system.c for ACPI
>    enumerted PNP devices, by checking pnp_device->protocal.
> 3) remove PNP0C01/PNP0C02 from PNPACPI white list.
> 
> By doing this, we can remove the depedency of PNP bus, but this requires
> a lot of code duplication(need to copy quirks.c and system.c logic into ACPI),
> which does not look good neither.
> 
> Any comments will be appreciated.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/pnp/pnpacpi/core.c |   20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
> index 739fa24..5b13600 100644
> --- a/drivers/pnp/pnpacpi/core.c
> +++ b/drivers/pnp/pnpacpi/core.c
> @@ -173,9 +173,6 @@ static const struct acpi_device_id acpi_pnp_device_ids[]= {
>  	{"FUJ02bf"},
>  	{"FUJ02B1"},
>  	{"FUJ02E3"},
> -	/* system */
> -	{"PNP0c02"}, /* General ID for reserving resources */
> -	{"PNP0c01"}, /* memory controller */
>  	/* c6xdigio */
>          {"PNP0400"}, /* Standard LPT Printer Port */
>          {"PNP0401"}, /* ECP Printer Port */
> @@ -677,6 +674,20 @@ static int is_cmos_rtc_device(struct acpi_device *adev)
>  	return !acpi_match_device_ids(adev, ids);
>  }
>  
> +/*
> + * For devices with id "PNP0C01"/"PNP0C02", they will be enumerated
> + * to PNP bus anyway to do resource reservation.
> + */
> +static int is_system_device(struct acpi_device *adev)
> +{
> +	struct acpi_device_id ids[] = {
> +		{"PNP0C02"},
> +		{"PNP0C01"},
> +		{""},
> +	};
> +	return !acpi_match_device_ids(adev, ids);
> +}
> +
>  static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
>  						     u32 lvl, void *context,
>  						     void **rv)
> @@ -685,7 +696,8 @@ static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
>  
>  	if (acpi_bus_get_device(handle, &device))
>  		return AE_CTRL_DEPTH;
> -	if (device->handler == &pnpacpi_handler || is_cmos_rtc_device(device))
> +	if (device->handler == &pnpacpi_handler || is_system_device(device) ||
> +	    is_cmos_rtc_device(device))
>  		pnpacpi_add_device(device);
>  	return AE_OK;
>  }

Re: [RFC PATCH 8/8] PNPACPI: create both PNP and Platform device nodes for PNP0C01/PNP0C02

[Bjorn Helgaas]

On Mon, Mar 3, 2014 at 7:17 AM, Zhang Rui <rui.zhang@intel.com> wrote:
> Hi, Bjorn,
>
> do you have comments for this particular patch?

Nope, I'm not paying attention to this area any more.

> On Wed, 2014-02-26 at 17:11 +0800, Zhang Rui wrote:
>> ACPI devices with id "PNP0C01/PNP0C02" means that we need to
>> protect their resources from being allocated by others.
>>
>> Currently, this is done in drivers/pnp/system.c.
>>
>> But the problem is that, there are some devices with extra ids besides
>> PNP0C01/PNP0C02, and for these devices,
>> 1) PNP0C01/PNP0C02 suggest that resource reservation is still needed.
>> 2) the other ids suggest that we should enumerate them to platform bus
>>
>> To reserve resources for those devices, we should either use the current code
>> by exporting the device to PNP bus, or introduce resource reservation support
>> in platform bus/ACPI.
>>
>> This patch follows the first way by enumerating an ACPI device to platform bus
>> AND pnp bus at the same time.
>> Its PNP device node will be probed by drivers/pnp/system.c and do
>> everything as we do today.
>> Its platform device node will also be created so that a platform bus
>> driver can be probed.
>>
>> The advantage is that, it brings little change to the current code,
>> the patch itself looks safe and clear.
>> The disadvantage is that
>> 1) we create two physical device nodes for the same ACPI node,
>>    this is against our effort that has been doing recently.
>> 2) we still depend on PNP bus to do this (resouce reservation) for us,
>>    which is still a problem we need to fix sooner or later.
>>
>> An alternative proposal is to remove the depedency of PNP bus and
>> do resource management in ACPI for all PNP0C01/PNP0C02 devices instead,
>> no matter what bus they are enumerated to.
>> To do this, we need to
>> 1) introduce a fs_initcall() in ACPI, to reserve all PNP0C01/PNP0C02 resources
>> in ACPI, something like we did via drivers/acpi/motherboard.c before
>> (but the code needs to follow drivers/pnp/quirks.c and system.c strictly).
>> This initcall will be run after PCI claiming BARs and before PCI assigning
>> resources for uninitialized devices.
>> 2) skip drivers/pnp/quirks.c and drivers/pnp/system.c for ACPI
>>    enumerted PNP devices, by checking pnp_device->protocal.
>> 3) remove PNP0C01/PNP0C02 from PNPACPI white list.
>>
>> By doing this, we can remove the depedency of PNP bus, but this requires
>> a lot of code duplication(need to copy quirks.c and system.c logic into ACPI),
>> which does not look good neither.
>>
>> Any comments will be appreciated.
>>
>> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>> ---
>>  drivers/pnp/pnpacpi/core.c |   20 ++++++++++++++++----
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
>> index 739fa24..5b13600 100644
>> --- a/drivers/pnp/pnpacpi/core.c
>> +++ b/drivers/pnp/pnpacpi/core.c
>> @@ -173,9 +173,6 @@ static const struct acpi_device_id acpi_pnp_device_ids[]= {
>>       {"FUJ02bf"},
>>       {"FUJ02B1"},
>>       {"FUJ02E3"},
>> -     /* system */
>> -     {"PNP0c02"}, /* General ID for reserving resources */
>> -     {"PNP0c01"}, /* memory controller */
>>       /* c6xdigio */
>>          {"PNP0400"}, /* Standard LPT Printer Port */
>>          {"PNP0401"}, /* ECP Printer Port */
>> @@ -677,6 +674,20 @@ static int is_cmos_rtc_device(struct acpi_device *adev)
>>       return !acpi_match_device_ids(adev, ids);
>>  }
>>
>> +/*
>> + * For devices with id "PNP0C01"/"PNP0C02", they will be enumerated
>> + * to PNP bus anyway to do resource reservation.
>> + */
>> +static int is_system_device(struct acpi_device *adev)
>> +{
>> +     struct acpi_device_id ids[] = {
>> +             {"PNP0C02"},
>> +             {"PNP0C01"},
>> +             {""},
>> +     };
>> +     return !acpi_match_device_ids(adev, ids);
>> +}
>> +
>>  static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
>>                                                    u32 lvl, void *context,
>>                                                    void **rv)
>> @@ -685,7 +696,8 @@ static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
>>
>>       if (acpi_bus_get_device(handle, &device))
>>               return AE_CTRL_DEPTH;
>> -     if (device->handler == &pnpacpi_handler || is_cmos_rtc_device(device))
>> +     if (device->handler == &pnpacpi_handler || is_system_device(device) ||
>> +         is_cmos_rtc_device(device))
>>               pnpacpi_add_device(device);
>>       return AE_OK;
>>  }
>
>

Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus for _HID enumeration

[Rafael J. Wysocki]

On Monday, March 03, 2014 10:11:48 PM Zhang Rui wrote:
> On Mon, 2014-03-03 at 00:51 +0100, Rafael J. Wysocki wrote:
> > On Wednesday, February 26, 2014 05:11:12 PM Zhang Rui wrote:
> > > Because of the growing demand for enumerating ACPI devices to platform bus,
> > > this patch changes the code to enumerate ACPI devices with _HID/_CID to
> > > platform bus by default, unless the device already has a scan handler attached.
> > > 
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >  drivers/acpi/acpi_platform.c |   28 ----------------------------
> > >  drivers/acpi/scan.c          |   12 ++++++------
> > >  2 files changed, 6 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> > > index dbfe49e..33376a9 100644
> > > --- a/drivers/acpi/acpi_platform.c
> > > +++ b/drivers/acpi/acpi_platform.c
> > > @@ -22,24 +22,6 @@
> > >  
> > >  ACPI_MODULE_NAME("platform");
> > >  
> > > -/*
> > > - * The following ACPI IDs are known to be suitable for representing as
> > > - * platform devices.
> > > - */
> > > -static const struct acpi_device_id acpi_platform_device_ids[] = {
> > > -
> > > -	{ "PNP0D40" },
> > > -	{ "ACPI0003" },
> > > -	{ "VPC2004" },
> > > -	{ "BCM4752" },
> > > -
> > > -	/* Intel Smart Sound Technology */
> > > -	{ "INT33C8" },
> > > -	{ "80860F28" },
> > > -
> > > -	{ }
> > > -};
> > > -
> > >  /**
> > >   * acpi_create_platform_device - Create platform device for ACPI device node
> > >   * @adev: ACPI device node to create a platform device for.
> > > @@ -125,13 +107,3 @@ int acpi_create_platform_device(struct acpi_device *adev,
> > >  	kfree(resources);
> > >  	return 1;
> > >  }
> > > -
> > > -static struct acpi_scan_handler platform_handler = {
> > > -	.ids = acpi_platform_device_ids,
> > > -	.attach = acpi_create_platform_device,
> > > -};
> > > -
> > > -void __init acpi_platform_init(void)
> > > -{
> > > -	acpi_scan_add_handler(&platform_handler);
> > > -}
> > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > index 5967338..61af32e 100644
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -2022,14 +2022,15 @@ static int acpi_scan_attach_handler(struct acpi_device *device)
> > >  		handler = acpi_scan_match_handler(hwid->id, &devid);
> > >  		if (handler) {
> > >  			ret = handler->attach(device, devid);
> > > -			if (ret > 0) {
> > > +			if (ret > 0)
> > >  				device->handler = handler;
> > > -				break;
> > > -			} else if (ret < 0) {
> > > -				break;
> > > -			}
> > > +			if (ret)
> > > +				goto end;
> > >  		}
> > >  	}
> > > +end:
> > > +	if (!list_empty(&device->pnp.ids) && !device->handler)
> > 
> > I'm a bit concerned that this check will create platform devices for too many
> > ACPI device objects.
> 
> agreed. there are some devices created unexpected by this patch, e.g. on
> my test machine, I can see
>  
> /sys/bus/platform/devices/LNXSYSTM:00 (ACPI system bus/root node)
> /sys/bus/platform/devices/PNP0000:00  (PIC)
> /sys/bus/platform/devices/PNP0100:00  (system timer?)
> 
> >   Shouldn't we require that _HID or at least _CID is
> > present for that?
> > 
> I do not think so.
> only devices that invoke acpi_add_ids() may have pnp.ids but no
> _HID/_CID, right?
> I did a check in the code, those devices include:

Well, I did that too.

> ACPI root node
> ACPI video
> ACPI bay
> ACPI dock
> IBM SMBus
> ACPI Power resource
> ACPI processor
> ACPI thermal
> ACPI fixed power/sleep button
> 
> IMO, only the ACPI root node, ACPI power resource, possibly ACPI
> processor are the ones that we do not want to see in platform bus.

No, we don't want any of them.  So pretty much as I said, only if _HID/_CID
is present, please?

Rafael

RE: [RFC PATCH 6/8] ACPI: use platform bus as the default bus for _HID enumeration

[Zhang, Rui]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4708 bytes --]



> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Tuesday, March 04, 2014 7:23 AM
> To: Zhang, Rui
> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org;
> bhelgaas@google.com; matthew.garrett@nebula.com; Wysocki, Rafael J;
> dmitry.torokhov@gmail.com
> Subject: Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus
> for _HID enumeration
> Importance: High
> 
> On Monday, March 03, 2014 10:11:48 PM Zhang Rui wrote:
> > On Mon, 2014-03-03 at 00:51 +0100, Rafael J. Wysocki wrote:
> > > On Wednesday, February 26, 2014 05:11:12 PM Zhang Rui wrote:
> > > > Because of the growing demand for enumerating ACPI devices to
> > > > platform bus, this patch changes the code to enumerate ACPI
> > > > devices with _HID/_CID to platform bus by default, unless the
> device already has a scan handler attached.
> > > >
> > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > > ---
> > > >  drivers/acpi/acpi_platform.c |   28 ----------------------------
> > > >  drivers/acpi/scan.c          |   12 ++++++------
> > > >  2 files changed, 6 insertions(+), 34 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/acpi_platform.c
> > > > b/drivers/acpi/acpi_platform.c index dbfe49e..33376a9 100644
> > > > --- a/drivers/acpi/acpi_platform.c
> > > > +++ b/drivers/acpi/acpi_platform.c
> > > > @@ -22,24 +22,6 @@
> > > >
> > > >  ACPI_MODULE_NAME("platform");
> > > >
> > > > -/*
> > > > - * The following ACPI IDs are known to be suitable for
> > > > representing as
> > > > - * platform devices.
> > > > - */
> > > > -static const struct acpi_device_id acpi_platform_device_ids[] =
> {
> > > > -
> > > > -	{ "PNP0D40" },
> > > > -	{ "ACPI0003" },
> > > > -	{ "VPC2004" },
> > > > -	{ "BCM4752" },
> > > > -
> > > > -	/* Intel Smart Sound Technology */
> > > > -	{ "INT33C8" },
> > > > -	{ "80860F28" },
> > > > -
> > > > -	{ }
> > > > -};
> > > > -
> > > >  /**
> > > >   * acpi_create_platform_device - Create platform device for ACPI
> device node
> > > >   * @adev: ACPI device node to create a platform device for.
> > > > @@ -125,13 +107,3 @@ int acpi_create_platform_device(struct
> acpi_device *adev,
> > > >  	kfree(resources);
> > > >  	return 1;
> > > >  }
> > > > -
> > > > -static struct acpi_scan_handler platform_handler = {
> > > > -	.ids = acpi_platform_device_ids,
> > > > -	.attach = acpi_create_platform_device,
> > > > -};
> > > > -
> > > > -void __init acpi_platform_init(void) -{
> > > > -	acpi_scan_add_handler(&platform_handler);
> > > > -}
> > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index
> > > > 5967338..61af32e 100644
> > > > --- a/drivers/acpi/scan.c
> > > > +++ b/drivers/acpi/scan.c
> > > > @@ -2022,14 +2022,15 @@ static int
> acpi_scan_attach_handler(struct acpi_device *device)
> > > >  		handler = acpi_scan_match_handler(hwid->id, &devid);
> > > >  		if (handler) {
> > > >  			ret = handler->attach(device, devid);
> > > > -			if (ret > 0) {
> > > > +			if (ret > 0)
> > > >  				device->handler = handler;
> > > > -				break;
> > > > -			} else if (ret < 0) {
> > > > -				break;
> > > > -			}
> > > > +			if (ret)
> > > > +				goto end;
> > > >  		}
> > > >  	}
> > > > +end:
> > > > +	if (!list_empty(&device->pnp.ids) && !device->handler)
> > >
> > > I'm a bit concerned that this check will create platform devices
> for
> > > too many ACPI device objects.
> >
> > agreed. there are some devices created unexpected by this patch, e.g.
> > on my test machine, I can see
> >
> > /sys/bus/platform/devices/LNXSYSTM:00 (ACPI system bus/root node)
> > /sys/bus/platform/devices/PNP0000:00  (PIC)
> > /sys/bus/platform/devices/PNP0100:00  (system timer?)
> >
> > >   Shouldn't we require that _HID or at least _CID is present for
> > > that?
> > >
> > I do not think so.
> > only devices that invoke acpi_add_ids() may have pnp.ids but no
> > _HID/_CID, right?
> > I did a check in the code, those devices include:
> 
> Well, I did that too.
> 
> > ACPI root node
> > ACPI video
> > ACPI bay
> > ACPI dock
> > IBM SMBus
> > ACPI Power resource
> > ACPI processor
> > ACPI thermal
> > ACPI fixed power/sleep button
> >
> > IMO, only the ACPI root node, ACPI power resource, possibly ACPI
> > processor are the ones that we do not want to see in platform bus.
> 
> No, we don't want any of them.  So pretty much as I said, only if
> _HID/_CID is present, please?
> 

Why? We will convert the drivers for most of those devices from ACPI bus to platform bus sooner or later.
We need to see them in platform bus...

Thanks,
rui

> Rafael

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus for _HID enumeration

[Rafael J. Wysocki]

On 3/4/2014 1:27 AM, Zhang, Rui wrote:
>
>> -----Original Message-----
>> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
>> Sent: Tuesday, March 04, 2014 7:23 AM
>> To: Zhang, Rui
>> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org;
>> bhelgaas@google.com; matthew.garrett@nebula.com; Wysocki, Rafael J;
>> dmitry.torokhov@gmail.com
>> Subject: Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus
>> for _HID enumeration
>> Importance: High
>>
>> On Monday, March 03, 2014 10:11:48 PM Zhang Rui wrote:
>>> On Mon, 2014-03-03 at 00:51 +0100, Rafael J. Wysocki wrote:
>>>> On Wednesday, February 26, 2014 05:11:12 PM Zhang Rui wrote:
>>>>> Because of the growing demand for enumerating ACPI devices to
>>>>> platform bus, this patch changes the code to enumerate ACPI
>>>>> devices with _HID/_CID to platform bus by default, unless the
>> device already has a scan handler attached.
>>>>> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>>>>> ---
>>>>>   drivers/acpi/acpi_platform.c |   28 ----------------------------
>>>>>   drivers/acpi/scan.c          |   12 ++++++------
>>>>>   2 files changed, 6 insertions(+), 34 deletions(-)
>>>>>
>>>>> diff --git a/drivers/acpi/acpi_platform.c
>>>>> b/drivers/acpi/acpi_platform.c index dbfe49e..33376a9 100644
>>>>> --- a/drivers/acpi/acpi_platform.c
>>>>> +++ b/drivers/acpi/acpi_platform.c
>>>>> @@ -22,24 +22,6 @@
>>>>>
>>>>>   ACPI_MODULE_NAME("platform");
>>>>>
>>>>> -/*
>>>>> - * The following ACPI IDs are known to be suitable for
>>>>> representing as
>>>>> - * platform devices.
>>>>> - */
>>>>> -static const struct acpi_device_id acpi_platform_device_ids[] =
>> {
>>>>> -
>>>>> -	{ "PNP0D40" },
>>>>> -	{ "ACPI0003" },
>>>>> -	{ "VPC2004" },
>>>>> -	{ "BCM4752" },
>>>>> -
>>>>> -	/* Intel Smart Sound Technology */
>>>>> -	{ "INT33C8" },
>>>>> -	{ "80860F28" },
>>>>> -
>>>>> -	{ }
>>>>> -};
>>>>> -
>>>>>   /**
>>>>>    * acpi_create_platform_device - Create platform device for ACPI
>> device node
>>>>>    * @adev: ACPI device node to create a platform device for.
>>>>> @@ -125,13 +107,3 @@ int acpi_create_platform_device(struct
>> acpi_device *adev,
>>>>>   	kfree(resources);
>>>>>   	return 1;
>>>>>   }
>>>>> -
>>>>> -static struct acpi_scan_handler platform_handler = {
>>>>> -	.ids = acpi_platform_device_ids,
>>>>> -	.attach = acpi_create_platform_device,
>>>>> -};
>>>>> -
>>>>> -void __init acpi_platform_init(void) -{
>>>>> -	acpi_scan_add_handler(&platform_handler);
>>>>> -}
>>>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index
>>>>> 5967338..61af32e 100644
>>>>> --- a/drivers/acpi/scan.c
>>>>> +++ b/drivers/acpi/scan.c
>>>>> @@ -2022,14 +2022,15 @@ static int
>> acpi_scan_attach_handler(struct acpi_device *device)
>>>>>   		handler = acpi_scan_match_handler(hwid->id, &devid);
>>>>>   		if (handler) {
>>>>>   			ret = handler->attach(device, devid);
>>>>> -			if (ret > 0) {
>>>>> +			if (ret > 0)
>>>>>   				device->handler = handler;
>>>>> -				break;
>>>>> -			} else if (ret < 0) {
>>>>> -				break;
>>>>> -			}
>>>>> +			if (ret)
>>>>> +				goto end;
>>>>>   		}
>>>>>   	}
>>>>> +end:
>>>>> +	if (!list_empty(&device->pnp.ids) && !device->handler)
>>>> I'm a bit concerned that this check will create platform devices
>> for
>>>> too many ACPI device objects.
>>> agreed. there are some devices created unexpected by this patch, e.g.
>>> on my test machine, I can see
>>>
>>> /sys/bus/platform/devices/LNXSYSTM:00 (ACPI system bus/root node)
>>> /sys/bus/platform/devices/PNP0000:00  (PIC)
>>> /sys/bus/platform/devices/PNP0100:00  (system timer?)
>>>
>>>>    Shouldn't we require that _HID or at least _CID is present for
>>>> that?
>>>>
>>> I do not think so.
>>> only devices that invoke acpi_add_ids() may have pnp.ids but no
>>> _HID/_CID, right?
>>> I did a check in the code, those devices include:
>> Well, I did that too.
>>
>>> ACPI root node
>>> ACPI video
>>> ACPI bay
>>> ACPI dock
>>> IBM SMBus
>>> ACPI Power resource
>>> ACPI processor
>>> ACPI thermal
>>> ACPI fixed power/sleep button
>>>
>>> IMO, only the ACPI root node, ACPI power resource, possibly ACPI
>>> processor are the ones that we do not want to see in platform bus.
>> No, we don't want any of them.  So pretty much as I said, only if
>> _HID/_CID is present, please?
>>
> Why? We will convert the drivers for most of those devices from ACPI bus to platform bus sooner or later.
> We need to see them in platform bus...

No, we don't.

I'm not sure about IBM SMBus to be honest, but as for the rest:

Why would we want one for the ACPI root?

And for video?  Those things are PCI usually devices anyway and we just 
add "artificial" HIDs for them.

ACPI docks and bays are handled by the dock driver which creates 
platform devices for them already if needed and we don't want duplicates 
there.

ACPI processor has its own scan handler that binds those objects to 
system devices.

Power resources - no need.

Do we need platform devices for ACPI thermal zones?

Yes, we will need them for fixed buttons, but that's a special case anyway.

Thanks,
Rafael

Re: [RFC PATCH 2/8] PNPACPI: use whilte list for pnpacpi device enumeration

[Rafael J. Wysocki]

On Wednesday, February 26, 2014 05:11:08 PM Zhang Rui wrote:
> ACPI can be used to enumerate PNP devices, but the code does not
> handle this in a good manner.
> 
> Currently, if an ACPI device
> 1. has _CRS method,
> 2. has an identifications of
>    "three capital charactors followed by four hex numbers",
> 3. is not in the exclude list,
> it is enumerated to PNP bus.
> 
> So actually, PNP bus is used as the default bus for enumerating _HID devices.
> 
> But, nowadays, more and more _HID devices are needed to be enumerate to
> platform bus instead. And a white list is used for those devices to avoid
> overlapping with PNP bus.
> The problem is that this list is continuously growing.
> 
> So, a solution that uses platform bus as the default bus for _HID enumeration
> is preferred.
> In order to do this, this patch changes the way of enumerating PNP devices.
> As the first step, we use a white list to create PNP devices instead.
> This white list contains all the pnp_device_id strings in all the pnp drivers,
> thus this change is transparent to PNP core and all the PNP drivers.
> 
> Note: I just grep all the id strings in all pnp_device_id instances and
>       copy them to the new white list, with a few changes to the comments
>       only, to follow the format of:
> 
>       /* driver name, or file name if not a PNP driver */
>       {"id-string"}, /* optional comments for the id-string */
>       ...
> 
> The next steps would be reduce this PNPACPI white list by
> 1. remove the ids for the devices that are never enumerated via ACPI
> 2. remove the ids and convert the drivers to platform bus drivers
>    for the devices that are not PNP devices in nature.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/scan.c        |    2 +
>  drivers/pnp/pnpacpi/core.c |  395 ++++++++++++++++++++++++++++++++++++++++----
>  include/linux/pnp.h        |    3 +
>  3 files changed, 366 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index dca22eb..5967338 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -11,6 +11,7 @@
>  #include <linux/kthread.h>
>  #include <linux/dmi.h>
>  #include <linux/nls.h>
> +#include <linux/pnp.h>
>  
>  #include <asm/pgtable.h>
>  
> @@ -2190,6 +2191,7 @@ int __init acpi_scan_init(void)
>  	acpi_container_init();
>  	acpi_memory_hotplug_init();
>  	acpi_dock_init();
> +	acpi_pnp_init();
>  
>  	mutex_lock(&acpi_scan_lock);
>  	/*
> diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
> index 9f611cb..76df7fc 100644
> --- a/drivers/pnp/pnpacpi/core.c
> +++ b/drivers/pnp/pnpacpi/core.c
> @@ -30,26 +30,6 @@
>  
>  static int num;
>  
> -/* We need only to blacklist devices that have already an acpi driver that
> - * can't use pnp layer. We don't need to blacklist device that are directly
> - * used by the kernel (PCI root, ...), as it is harmless and there were
> - * already present in pnpbios. But there is an exception for devices that
> - * have irqs (PIC, Timer) because we call acpi_register_gsi.
> - * Finally, only devices that have a CRS method need to be in this list.
> - */
> -static struct acpi_device_id excluded_id_list[] __initdata = {
> -	{"PNP0C09", 0},		/* EC */
> -	{"PNP0C0F", 0},		/* Link device */
> -	{"PNP0000", 0},		/* PIC */
> -	{"PNP0100", 0},		/* Timer */
> -	{"", 0},
> -};
> -
> -static inline int __init is_exclusive_device(struct acpi_device *dev)
> -{
> -	return (!acpi_match_device_ids(dev, excluded_id_list));
> -}
> -
>  /*
>   * Compatible Device IDs
>   */
> @@ -73,6 +53,317 @@ static int __init ispnpidacpi(const char *id)
>  	return 1;
>  }
>  
> +static const struct acpi_device_id acpi_pnp_device_ids[]= {
> +	/* pata_isapnp */
> +	{"PNP0600"}, /* Generic ESDI/IDE/ATA compatible hard disk controller */
> +	/* floppy */
> +	{"PNP0700"},
> +	/* ipmi_si */
> +	{"IPI0001"},
> +	/* tpm_inf_pnp */
> +	{"IFX0101"}, /* Infineon TPMs */
> +	{"IFX0102"}, /* Infineon TPMs */
> +	/*tpm_tis */
> +	{"PNP0C31"}, /* TPM */
> +	{"ATM1200"}, /* Atmel */
> +	{"IFX0102"}, /* Infineon */
> +	{"BCM0101"}, /* Broadcom */
> +	{"BCM0102"}, /* Broadcom */
> +	{"NSC1200"}, /* National */
> +	{"ICO0102"}, /* Intel */
> +	/* ide	 */
> +	{"PNP0600"}, /* Generic ESDI/IDE/ATA compatible hard disk controller */
> +	/* ns558 */
> +	{"@P@0001"}, /* ALS 100 */
> +	{"@P@0020"}, /* ALS 200 */
> +	{"@P@1001"}, /* ALS 100+ */
> +	{"@P@2001"}, /* ALS 120 */
> +	{"ASB16fd"}, /* AdLib NSC16 */
> +	{"AZT3001"}, /* AZT1008 */
> +	{"CDC0001"}, /* Opl3-SAx */
> +	{"CSC0001"}, /* CS4232 */
> +	{"CSC000f"}, /* CS4236 */
> +	{"CSC0101"}, /* CS4327 */
> +	{"CTL7001"}, /* SB16 */
> +	{"CTL7002"}, /* AWE64 */
> +	{"CTL7005"}, /* Vibra16 */
> +	{"ENS2020"}, /* SoundscapeVIVO */
> +	{"ESS0001"}, /* ES1869 */
> +	{"ESS0005"}, /* ES1878 */
> +	{"ESS6880"}, /* ES688 */
> +	{"IBM0012"}, /* CS4232 */
> +	{"OPT0001"}, /* OPTi Audio16 */
> +	{"YMH0006"}, /* Opl3-SA */
> +	{"YMH0022"}, /* Opl3-SAx */
> +	{"PNPb02f"}, /* Generic */
> +	/* i8042 kbd */
> +	{"PNP0300"},
> +	{"PNP0301"},
> +	{"PNP0302"},
> +	{"PNP0303"},
> +	{"PNP0304"},
> +	{"PNP0305"},
> +	{"PNP0306"},
> +	{"PNP0309"},
> +	{"PNP030a"},
> +	{"PNP030b"},
> +	{"PNP0320"},
> +	{"PNP0343"},
> +	{"PNP0344"},
> +	{"PNP0345"},
> +	{"CPQA0D7"},
> +	/* i8042 aux */
> +	{"AUI0200"},
> +	{"FJC6000"},
> +	{"FJC6001"},
> +	{"PNP0f03"},
> +	{"PNP0f0b"},
> +	{"PNP0f0e"},
> +	{"PNP0f12"},
> +	{"PNP0f13"},
> +	{"PNP0f19"},
> +	{"PNP0f1c"},
> +	{"SYN0801"},
> +	/* fcpnp */
> +	{"AVM0900"},
> +	/* radio-cadet */
> +	{"MSM0c24"}, /* ADS Cadet AM/FM Radio Card */
> +	/* radio-gemtek */
> +	{"ADS7183"}, /* AOpen FX-3D/Pro Radio */
> +	/* radio-sf16fmr2 */
> +	{"MFRad13"}, /* tuner subdevice of SF16-FMD2 */
> +	/* ene_ir */
> +	{"ENE0100"},
> +	{"ENE0200"},
> +	{"ENE0201"},
> +	{"ENE0202"},
> +	/* fintek-cir */
> +	{"FIT0002"}, /* CIR */
> +	/* ite-cir */
> +	{"ITE8704"}, /* Default model */
> +	{"ITE8713"}, /* CIR found in EEEBox 1501U */
> +	{"ITE8708"}, /* Bridged IT8512 */
> +	{"ITE8709"}, /* SRAM-Bridged IT8512 */
> +	/* nuvoton-cir */
> +	{"WEC0530"}, /* CIR */
> +	{"NTN0530"}, /* CIR for new chip's pnp id*/
> +	/* Winbond CIR */
> +	{"WEC1022"},
> +	/* wbsd */
> +	{"WEC0517"},
> +	{"WEC0518"},
> +	/* Winbond CIR */
> +	{"TCM5090"}, /* 3Com Etherlink III (TP) */
> +	{"TCM5091"}, /* 3Com Etherlink III */
> +	{"TCM5094"}, /* 3Com Etherlink III (combo) */
> +	{"TCM5095"}, /* 3Com Etherlink III (TPO) */
> +	{"TCM5098"}, /* 3Com Etherlink III (TPC) */
> +	{"PNP80f7"}, /* 3Com Etherlink III compatible */
> +	{"PNP80f8"}, /* 3Com Etherlink III compatible */
> +	/* nsc-ircc */
> +	{"NSC6001"},
> +	{"HWPC224"},
> +	{"IBM0071"},
> +	/* smsc-ircc2 */
> +	{"SMCf010"},
> +	/* sb1000 */
> +	{"GIC1000"},
> +	/* parport_pc */
> +	{"PNP0400"}, /* Standard LPT Printer Port */
> +	{"PNP0401"}, /* ECP Printer Port */
> +	/* apple-gmux */
> +	{"APP000B"},
> +	/* fujitsu-laptop.c */
> +	{"FUJ02bf"},
> +	{"FUJ02B1"},
> +	{"FUJ02E3"},
> +	/* system */
> +	{"PNP0c02"}, /* General ID for reserving resources */
> +	{"PNP0c01"}, /* memory controller */
> +	/* rtc_cmos */
> +	{"PNP0b00"},
> +	{"PNP0b01"},
> +	{"PNP0b02"},
> +	/* c6xdigio */
> +        {"PNP0400"}, /* Standard LPT Printer Port */
> +        {"PNP0401"}, /* ECP Printer Port */
> +	/* ni_atmio.c */
> +	{"NIC1900"},
> +	{"NIC2400"},
> +	{"NIC2500"},
> +	{"NIC2600"},
> +	{"NIC2700"},
> +	/* serial */
> +	{"AAC000F"}, /* Archtek America Corp. Archtek SmartLink Modem 3334BT Plug & Play */
> +	{"ADC0001"}, /* Anchor Datacomm BV. SXPro 144 External Data Fax Modem Plug & Play */
> +	{"ADC0002"}, /* SXPro 288 External Data Fax Modem Plug & Play */
> +	{"AEI0250"}, /* PROLiNK 1456VH ISA PnP K56flex Fax Modem */
> +	{"AEI1240"}, /* Actiontec ISA PNP 56K X2 Fax Modem */
> +	{"AKY1021"}, /* Rockwell 56K ACF II Fax+Data+Voice Modem */
> +	{"AZT4001"}, /* AZT3005 PnP SOUND DEVICE */
> +	{"BDP3336"}, /* Best Data Products Inc. Smart One 336F PnP Modem */
> +	{"BRI0A49"}, /* Boca Complete Ofc Communicator 14.4 Data-FAX */
> +	{"BRI1400"}, /* Boca Research 33,600 ACF Modem */
> +	{"BRI3400"}, /* Boca 33.6 Kbps Internal FD34FSVD */
> +	{"BRI0A49"}, /* Boca 33.6 Kbps Internal FD34FSVD */
> +	{"BDP3336"}, /* Best Data Products Inc. Smart One 336F PnP Modem */
> +	{"CPI4050"}, /* Computer Peripherals Inc. EuroViVa CommCenter-33.6 SP PnP */
> +	{"CTL3001"}, /* Creative Labs Phone Blaster 28.8 DSVD PnP Voice */
> +	{"CTL3011"}, /* Creative Labs Modem Blaster 28.8 DSVD PnP Voice */
> +	{"DAV0336"}, /* Davicom ISA 33.6K Modem */
> +	{"DMB1032"}, /* Creative Modem Blaster Flash56 DI5601-1 */
> +	{"DMB2001"}, /* Creative Modem Blaster V.90 DI5660 */
> +	{"ETT0002"}, /* E-Tech CyberBULLET PC56RVP */
> +	{"FUJ0202"}, /* Fujitsu 33600 PnP-I2 R Plug & Play */
> +	{"FUJ0205"}, /* Fujitsu FMV-FX431 Plug & Play */
> +	{"FUJ0206"}, /* Fujitsu 33600 PnP-I4 R Plug & Play */
> +	{"FUJ0209"}, /* Fujitsu Fax Voice 33600 PNP-I5 R Plug & Play */
> +	{"GVC000F"}, /* Archtek SmartLink Modem 3334BT Plug & Play */
> +	{"GVC0303"}, /* Archtek SmartLink Modem 3334BRV 33.6K Data Fax Voice */
> +	{"HAY0001"}, /* Hayes Optima 288 V.34-V.FC + FAX + Voice Plug & Play */
> +	{"HAY000C"}, /* Hayes Optima 336 V.34 + FAX + Voice PnP */
> +	{"HAY000D"}, /* Hayes Optima 336B V.34 + FAX + Voice PnP */
> +	{"HAY5670"}, /* Hayes Accura 56K Ext Fax Modem PnP */
> +	{"HAY5674"}, /* Hayes Accura 56K Ext Fax Modem PnP */
> +	{"HAY5675"}, /* Hayes Accura 56K Fax Modem PnP */
> +	{"HAYF000"}, /* Hayes 288, V.34 + FAX */
> +	{"HAYF001"}, /* Hayes Optima 288 V.34 + FAX + Voice, Plug & Play */
> +	{"IBM0033"}, /* IBM Thinkpad 701 Internal Modem Voice */
> +	{"PNP4972"}, /* Intermec CV60 touchscreen port */
> +	{"IXDC801"}, /* Intertex 28k8 33k6 Voice EXT PnP */
> +	{"IXDC901"}, /* Intertex 33k6 56k Voice EXT PnP */
> +	{"IXDD801"}, /* Intertex 28k8 33k6 Voice SP EXT PnP */
> +	{"IXDD901"}, /* Intertex 33k6 56k Voice SP EXT PnP */
> +	{"IXDF401"}, /* Intertex 28k8 33k6 Voice SP INT PnP */
> +	{"IXDF801"}, /* Intertex 28k8 33k6 Voice SP EXT PnP */
> +	{"IXDF901"}, /* Intertex 33k6 56k Voice SP EXT PnP */
> +	{"KOR4522"}, /* KORTEX 28800 Externe PnP */
> +	{"KORF661"}, /* KXPro 33.6 Vocal ASVD PnP */
> +	{"LAS4040"}, /* LASAT Internet 33600 PnP */
> +	{"LAS4540"}, /* Lasat Safire 560 PnP */
> +	{"LAS5440"}, /* Lasat Safire 336  PnP */
> +	{"MNP0281"}, /* Microcom TravelPorte FAST V.34 Plug & Play */
> +	{"MNP0336"}, /* Microcom DeskPorte V.34 FAST or FAST+ Plug & Play */
> +	{"MNP0339"}, /* Microcom DeskPorte FAST EP 28.8 Plug & Play */
> +	{"MNP0342"}, /* Microcom DeskPorte 28.8P Plug & Play */
> +	{"MNP0500"}, /* Microcom DeskPorte FAST ES 28.8 Plug & Play */
> +	{"MNP0501"}, /* Microcom DeskPorte FAST ES 28.8 Plug & Play */
> +	{"MNP0502"}, /* Microcom DeskPorte 28.8S Internal Plug & Play */
> +	{"MOT1105"}, /* Motorola BitSURFR Plug & Play */
> +	{"MOT1111"}, /* Motorola TA210 Plug & Play */
> +	{"MOT1114"}, /* Motorola HMTA 200 (ISDN) Plug & Play */
> +	{"MOT1115"}, /* Motorola BitSURFR Plug & Play */
> +	{"MOT1190"}, /* Motorola Lifestyle 28.8 Internal */
> +	{"MOT1501"}, /* Motorola V.3400 Plug & Play */
> +	{"MOT1502"}, /* Motorola Lifestyle 28.8 V.34 Plug & Play */
> +	{"MOT1505"}, /* Motorola Power 28.8 V.34 Plug & Play */
> +	{"MOT1509"}, /* Motorola ModemSURFR External 28.8 Plug & Play */
> +	{"MOT150A"}, /* Motorola Premier 33.6 Desktop Plug & Play */
> +	{"MOT150F"}, /* Motorola VoiceSURFR 56K External PnP */
> +	{"MOT1510"}, /* Motorola ModemSURFR 56K External PnP */
> +	{"MOT1550"}, /* Motorola ModemSURFR 56K Internal PnP */
> +	{"MOT1560"}, /* Motorola ModemSURFR Internal 28.8 Plug & Play */
> +	{"MOT1580"}, /* Motorola Premier 33.6 Internal Plug & Play */
> +	{"MOT15B0"}, /* Motorola OnlineSURFR 28.8 Internal Plug & Play */
> +	{"MOT15F0"}, /* Motorola VoiceSURFR 56K Internal PnP */
> +	{"MVX00A1"}, /*  Deskline K56 Phone System PnP */
> +	{"MVX00F2"}, /* PC Rider K56 Phone System PnP */
> +	{"nEC8241"}, /* NEC 98NOTE SPEAKER PHONE FAX MODEM(33600bps) */
> +	{"PMC2430"}, /* Pace 56 Voice Internal Plug & Play Modem */
> +	{"PNP0500"}, /* Generic standard PC COM port	 */
> +	{"PNP0501"}, /* Generic 16550A-compatible COM port */
> +	{"PNPC000"}, /* Compaq 14400 Modem */
> +	{"PNPC001"}, /* Compaq 2400/9600 Modem */
> +	{"PNPC031"}, /* Dial-Up Networking Serial Cable between 2 PCs */
> +	{"PNPC032"}, /* Dial-Up Networking Parallel Cable between 2 PCs */
> +	{"PNPC100"}, /* Standard 9600 bps Modem */
> +	{"PNPC101"}, /* Standard 14400 bps Modem */
> +	{"PNPC102"}, /*  Standard 28800 bps Modem*/
> +	{"PNPC103"}, /*  Standard Modem*/
> +	{"PNPC104"}, /*  Standard 9600 bps Modem*/
> +	{"PNPC105"}, /*  Standard 14400 bps Modem*/
> +	{"PNPC106"}, /*  Standard 28800 bps Modem*/
> +	{"PNPC107"}, /*  Standard Modem */
> +	{"PNPC108"}, /* Standard 9600 bps Modem */
> +	{"PNPC109"}, /* Standard 14400 bps Modem */
> +	{"PNPC10A"}, /* Standard 28800 bps Modem */
> +	{"PNPC10B"}, /* Standard Modem */
> +	{"PNPC10C"}, /* Standard 9600 bps Modem */
> +	{"PNPC10D"}, /* Standard 14400 bps Modem */
> +	{"PNPC10E"}, /* Standard 28800 bps Modem */
> +	{"PNPC10F"}, /* Standard Modem */
> +	{"PNP2000"}, /* Standard PCMCIA Card Modem */
> +	{"ROK0030"}, /* Rockwell 33.6 DPF Internal PnP, Modular Technology 33.6 Internal PnP */
> +	{"ROK0100"}, /* KORTEX 14400 Externe PnP */
> +	{"ROK4120"}, /* Rockwell 28.8 */
> +	{"ROK4920"}, /* Viking 28.8 INTERNAL Fax+Data+Voice PnP */
> +	{"RSS00A0"}, /* Rockwell 33.6 DPF External PnP, BT Prologue 33.6 External PnP, Modular Technology 33.6 External PnP */
> +	{"RSS0262"}, /* Viking 56K FAX INT */
> +	{"RSS0250"}, /* K56 par,VV,Voice,Speakphone,AudioSpan,PnP */
> +	{"SUP1310"}, /* SupraExpress 28.8 Data/Fax PnP modem */
> +	{"SUP1381"}, /* SupraExpress 336i PnP Voice Modem */
> +	{"SUP1421"}, /* SupraExpress 33.6 Data/Fax PnP modem */
> +	{"SUP1590"}, /* SupraExpress 33.6 Data/Fax PnP modem */
> +	{"SUP1620"}, /* SupraExpress 336i Sp ASVD */
> +	{"SUP1760"}, /* SupraExpress 33.6 Data/Fax PnP modem */
> +	{"SUP2171"}, /* SupraExpress 56i Sp Intl */
> +	{"TEX0011"}, /* Phoebe Micro 33.6 Data Fax 1433VQH Plug & Play */
> +	{"UAC000F"}, /* Archtek SmartLink Modem 3334BT Plug & Play */
> +	{"USR0000"}, /* 3Com Corp. Gateway Telepath IIvi 33.6 */
> +	{"USR0002"}, /* U.S. Robotics Sporster 33.6K Fax INT PnP */
> +	{"USR0004"}, /*  Sportster Vi 14.4 PnP FAX Voicemail */
> +	{"USR0006"}, /* U.S. Robotics 33.6K Voice INT PnP */
> +	{"USR0007"}, /* U.S. Robotics 33.6K Voice EXT PnP */
> +	{"USR0009"}, /* U.S. Robotics Courier V.Everything INT PnP */
> +	{"USR2002"}, /* U.S. Robotics 33.6K Voice INT PnP */
> +	{"USR2070"}, /* U.S. Robotics 56K Voice INT PnP */
> +	{"USR2080"}, /* U.S. Robotics 56K Voice EXT PnP */
> +	{"USR3031"}, /* U.S. Robotics 56K FAX INT */
> +	{"USR3050"}, /* U.S. Robotics 56K FAX INT */
> +	{"USR3070"}, /* U.S. Robotics 56K Voice INT PnP */
> +	{"USR3080"}, /* U.S. Robotics 56K Voice EXT PnP */
> +	{"USR3090"}, /* U.S. Robotics 56K Voice INT PnP */
> +	{"USR9100"}, /* U.S. Robotics 56K Message  */
> +	{"USR9160"}, /* U.S. Robotics 56K FAX EXT PnP*/
> +	{"USR9170"}, /* U.S. Robotics 56K FAX INT PnP*/
> +	{"USR9180"}, /* U.S. Robotics 56K Voice EXT PnP*/
> +	{"USR9190"}, /* U.S. Robotics 56K Voice INT PnP*/
> +	{"WACFXXX"}, /* Wacom tablets */
> +	{"FPI2002"}, /* Compaq touchscreen */
> +	{"FUJ02B2"}, /* Fujitsu Stylistic touchscreens */
> +	{"FUJ02B3"},
> +	{"FUJ02B4"}, /* Fujitsu Stylistic LT touchscreens */
> +	{"FUJ02B6"}, /* Passive Fujitsu Stylistic touchscreens */
> +	{"FUJ02B7"},
> +	{"FUJ02B8"},
> +	{"FUJ02B9"},
> +	{"FUJ02BC"},
> +	{"FUJ02E5"}, /* Fujitsu Wacom Tablet PC device */
> +	{"FUJ02E6"}, /* Fujitsu P-series tablet PC device */
> +	{"FUJ02E7"}, /* Fujitsu Wacom 2FGT Tablet PC device */
> +	{"FUJ02E9"}, /* Fujitsu Wacom 1FGT Tablet PC device */
> +	{"LTS0001"}, /* LG C1 EXPRESS DUAL (C1-PB11A3) touch screen (actually a FUJ02E6 in disguise) */
> +	{"WCI0003"}, /* Rockwell's (PORALiNK) 33600 INT PNP */
> +	{"WEC1022"}, /* Winbond CIR port, should not be probed. We should keep track of it to prevent the legacy serial driver from probing it */
> +	{"PNPCXXX"}, /* Unknown PnP modems */
> +	{"PNPDXXX"}, /* More unknown PnP modems */
> +	/* scl200wdt */
> +	{"NSC0800"}, /* National Semiconductor PC87307/PC97307 watchdog component */
> +	/* mpu401 */
> +	{"PNPb006"},
> +	/* cs423x-pnpbios */
> +	{"CSC0100"},
> +	{"CSC0000"},
> +	{"GIM0100"}, /* Guillemot Turtlebeach something appears to be cs4232 compatible */
> +	/* es18xx-pnpbios */
> +	{"ESS1869"},
> +	{"ESS1879"},
> +	/* snd-opl3sa2-pnpbios */
> +	{"YMH0021"},
> +	{"NMX2210"}, /* Gateway Solo 2500 */
> +	{""},
> +};
> +
>  static int pnpacpi_get_resources(struct pnp_dev *dev)
>  {
>  	pnp_dbg(&dev->dev, "get resources\n");
> @@ -258,7 +549,7 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
>  	if (!pnpid)
>  		return 0;
>  
> -	if (is_exclusive_device(device) || !device->status.present)
> +	if (!device->status.present)
>  		return 0;
>  
>  	dev = pnp_alloc_dev(&pnpacpi_protocol, num, pnpid);
> @@ -312,19 +603,6 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
>  	return 0;
>  }
>  
> -static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
> -						     u32 lvl, void *context,
> -						     void **rv)
> -{
> -	struct acpi_device *device;
> -
> -	if (!acpi_bus_get_device(handle, &device))
> -		pnpacpi_add_device(device);
> -	else
> -		return AE_CTRL_DEPTH;
> -	return AE_OK;
> -}
> -
>  static int __init acpi_pnp_match(struct device *dev, void *_pnp)
>  {
>  	struct acpi_device *acpi = to_acpi_device(dev);
> @@ -361,6 +639,55 @@ static struct acpi_bus_type __initdata acpi_pnp_bus = {
>  };
>  
>  int pnpacpi_disabled __initdata;
> +
> +static int __init acpi_pnp_scan_handler_attach(struct acpi_device *adev,

This can't be __init.

> +                                   const struct acpi_device_id *id)
> +{
> +	return 1;
> +}
> +
> +static int __init acpi_pnp_scan_handler_match(char *devid, char *handlerid)

And this too.

> +{
> +	int i;
> +
> +	if (!ispnpidacpi(devid))
> +		return 0;
> +
> +	if (memcmp(devid, handlerid, 3))
> +		return 0;
> +
> +        for (i = 3; i < 7; i++) {
> +                if (handlerid[i] != 'X' &&
> +		    toupper(devid[i]) != toupper(handlerid[i]))
> +                        return 0;
> +        }
> +	return 1;
> +}
> +
> +static struct acpi_scan_handler pnpacpi_handler __initdata = {

And this cannot be __initdata, because the list of ACPI scan handlers is
walked during hotplug.

> +	.ids = acpi_pnp_device_ids,
> +	.match = acpi_pnp_scan_handler_match,
> +	.attach = acpi_pnp_scan_handler_attach,
> +};
> +
> +void __init acpi_pnp_init(void)
> +{
> +	acpi_scan_add_handler(&pnpacpi_handler);
> +}
> +
> +static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
> +						     u32 lvl, void *context,
> +						     void **rv)
> +{
> +	struct acpi_device *device;
> +
> +	if (acpi_bus_get_device(handle, &device))
> +		return AE_CTRL_DEPTH;
> +	if (device->handler == &pnpacpi_handler)
> +		pnpacpi_add_device(device);

Why don't you do that in acpi_pnp_scan_handler_attach() ?

> +	return AE_OK;
> +}
> +
>  static int __init pnpacpi_init(void)
>  {
>  	if (acpi_disabled || pnpacpi_disabled) {
> diff --git a/include/linux/pnp.h b/include/linux/pnp.h
> index 195aafc..b329699 100644
> --- a/include/linux/pnp.h
> +++ b/include/linux/pnp.h
> @@ -343,8 +343,11 @@ static inline struct acpi_device *pnp_acpi_device(struct pnp_dev *dev)
>  		return dev->data;
>  	return NULL;
>  }
> +
> +void acpi_pnp_init(void);
>  #else
>  #define pnp_acpi_device(dev) 0
> +static inline void acpi_pnp_init(void) {}
>  #endif
>  
>  /* status */
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus for _HID enumeration

[Rafael J. Wysocki]

On Tuesday, March 04, 2014 01:35:00 AM Rafael J. Wysocki wrote:
> On 3/4/2014 1:27 AM, Zhang, Rui wrote:
> >
> >> -----Original Message-----
> >> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> >> Sent: Tuesday, March 04, 2014 7:23 AM
> >> To: Zhang, Rui
> >> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> bhelgaas@google.com; matthew.garrett@nebula.com; Wysocki, Rafael J;
> >> dmitry.torokhov@gmail.com
> >> Subject: Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus
> >> for _HID enumeration
> >> Importance: High
> >>
> >> On Monday, March 03, 2014 10:11:48 PM Zhang Rui wrote:
> >>> On Mon, 2014-03-03 at 00:51 +0100, Rafael J. Wysocki wrote:
> >>>> On Wednesday, February 26, 2014 05:11:12 PM Zhang Rui wrote:
> >>>>> Because of the growing demand for enumerating ACPI devices to
> >>>>> platform bus, this patch changes the code to enumerate ACPI
> >>>>> devices with _HID/_CID to platform bus by default, unless the
> >> device already has a scan handler attached.
> >>>>> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> >>>>> ---
> >>>>>   drivers/acpi/acpi_platform.c |   28 ----------------------------
> >>>>>   drivers/acpi/scan.c          |   12 ++++++------
> >>>>>   2 files changed, 6 insertions(+), 34 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/acpi/acpi_platform.c
> >>>>> b/drivers/acpi/acpi_platform.c index dbfe49e..33376a9 100644
> >>>>> --- a/drivers/acpi/acpi_platform.c
> >>>>> +++ b/drivers/acpi/acpi_platform.c
> >>>>> @@ -22,24 +22,6 @@
> >>>>>
> >>>>>   ACPI_MODULE_NAME("platform");
> >>>>>
> >>>>> -/*
> >>>>> - * The following ACPI IDs are known to be suitable for
> >>>>> representing as
> >>>>> - * platform devices.
> >>>>> - */
> >>>>> -static const struct acpi_device_id acpi_platform_device_ids[] =
> >> {
> >>>>> -
> >>>>> -	{ "PNP0D40" },
> >>>>> -	{ "ACPI0003" },
> >>>>> -	{ "VPC2004" },
> >>>>> -	{ "BCM4752" },
> >>>>> -
> >>>>> -	/* Intel Smart Sound Technology */
> >>>>> -	{ "INT33C8" },
> >>>>> -	{ "80860F28" },
> >>>>> -
> >>>>> -	{ }
> >>>>> -};
> >>>>> -
> >>>>>   /**
> >>>>>    * acpi_create_platform_device - Create platform device for ACPI
> >> device node
> >>>>>    * @adev: ACPI device node to create a platform device for.
> >>>>> @@ -125,13 +107,3 @@ int acpi_create_platform_device(struct
> >> acpi_device *adev,
> >>>>>   	kfree(resources);
> >>>>>   	return 1;
> >>>>>   }
> >>>>> -
> >>>>> -static struct acpi_scan_handler platform_handler = {
> >>>>> -	.ids = acpi_platform_device_ids,
> >>>>> -	.attach = acpi_create_platform_device,
> >>>>> -};
> >>>>> -
> >>>>> -void __init acpi_platform_init(void) -{
> >>>>> -	acpi_scan_add_handler(&platform_handler);
> >>>>> -}
> >>>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index
> >>>>> 5967338..61af32e 100644
> >>>>> --- a/drivers/acpi/scan.c
> >>>>> +++ b/drivers/acpi/scan.c
> >>>>> @@ -2022,14 +2022,15 @@ static int
> >> acpi_scan_attach_handler(struct acpi_device *device)
> >>>>>   		handler = acpi_scan_match_handler(hwid->id, &devid);
> >>>>>   		if (handler) {
> >>>>>   			ret = handler->attach(device, devid);
> >>>>> -			if (ret > 0) {
> >>>>> +			if (ret > 0)
> >>>>>   				device->handler = handler;
> >>>>> -				break;
> >>>>> -			} else if (ret < 0) {
> >>>>> -				break;
> >>>>> -			}
> >>>>> +			if (ret)
> >>>>> +				goto end;
> >>>>>   		}
> >>>>>   	}
> >>>>> +end:
> >>>>> +	if (!list_empty(&device->pnp.ids) && !device->handler)
> >>>> I'm a bit concerned that this check will create platform devices
> >> for
> >>>> too many ACPI device objects.
> >>> agreed. there are some devices created unexpected by this patch, e.g.
> >>> on my test machine, I can see
> >>>
> >>> /sys/bus/platform/devices/LNXSYSTM:00 (ACPI system bus/root node)
> >>> /sys/bus/platform/devices/PNP0000:00  (PIC)
> >>> /sys/bus/platform/devices/PNP0100:00  (system timer?)
> >>>
> >>>>    Shouldn't we require that _HID or at least _CID is present for
> >>>> that?
> >>>>
> >>> I do not think so.
> >>> only devices that invoke acpi_add_ids() may have pnp.ids but no
> >>> _HID/_CID, right?
> >>> I did a check in the code, those devices include:
> >> Well, I did that too.
> >>
> >>> ACPI root node
> >>> ACPI video
> >>> ACPI bay
> >>> ACPI dock
> >>> IBM SMBus
> >>> ACPI Power resource
> >>> ACPI processor
> >>> ACPI thermal
> >>> ACPI fixed power/sleep button
> >>>
> >>> IMO, only the ACPI root node, ACPI power resource, possibly ACPI
> >>> processor are the ones that we do not want to see in platform bus.
> >> No, we don't want any of them.  So pretty much as I said, only if
> >> _HID/_CID is present, please?
> >>
> > Why? We will convert the drivers for most of those devices from ACPI bus to platform bus sooner or later.
> > We need to see them in platform bus...
> 
> No, we don't.
> 
> I'm not sure about IBM SMBus to be honest, but as for the rest:
> 
> Why would we want one for the ACPI root?
> 
> And for video?  Those things are PCI usually devices anyway and we just 
> add "artificial" HIDs for them.
> 
> ACPI docks and bays are handled by the dock driver which creates 
> platform devices for them already if needed and we don't want duplicates 
> there.
> 
> ACPI processor has its own scan handler that binds those objects to 
> system devices.
> 
> Power resources - no need.
> 
> Do we need platform devices for ACPI thermal zones?
> 
> Yes, we will need them for fixed buttons, but that's a special case anyway.

So, why don't we add an ACPI device object flag, say hid_device, such that if
set, the ACPI core will create a struct platform device for that device object.
Then, we can set hid_device for buttons and other stuff we care about.

Thanks,
Rafael

Re: [RFC PATCH 2/8] PNPACPI: use whilte list for pnpacpi device enumeration

[Zhang Rui]

On Fri, 2014-03-07 at 02:44 +0100, Rafael J. Wysocki wrote:
> On Wednesday, February 26, 2014 05:11:08 PM Zhang Rui wrote:
> > +
> > +static int __init acpi_pnp_scan_handler_attach(struct acpi_device *adev,
> 
> This can't be __init.
> 
> > +                                   const struct acpi_device_id *id)
> > +{
> > +	return 1;
> > +}
> > +
> > +static int __init acpi_pnp_scan_handler_match(char *devid, char *handlerid)
> 
> And this too.
> 
> > +{
> > +	int i;
> > +
> > +	if (!ispnpidacpi(devid))
> > +		return 0;
> > +
> > +	if (memcmp(devid, handlerid, 3))
> > +		return 0;
> > +
> > +        for (i = 3; i < 7; i++) {
> > +                if (handlerid[i] != 'X' &&
> > +		    toupper(devid[i]) != toupper(handlerid[i]))
> > +                        return 0;
> > +        }
> > +	return 1;
> > +}
> > +
> > +static struct acpi_scan_handler pnpacpi_handler __initdata = {
> 
> And this cannot be __initdata, because the list of ACPI scan handlers is
> walked during hotplug.
> 
right. will update it in next version.

> > +	.ids = acpi_pnp_device_ids,
> > +	.match = acpi_pnp_scan_handler_match,
> > +	.attach = acpi_pnp_scan_handler_attach,
> > +};
> > +
> > +void __init acpi_pnp_init(void)
> > +{
> > +	acpi_scan_add_handler(&pnpacpi_handler);
> > +}
> > +
> > +static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
> > +						     u32 lvl, void *context,
> > +						     void **rv)
> > +{
> > +	struct acpi_device *device;
> > +
> > +	if (acpi_bus_get_device(handle, &device))
> > +		return AE_CTRL_DEPTH;
> > +	if (device->handler == &pnpacpi_handler)
> > +		pnpacpi_add_device(device);
> 
> Why don't you do that in acpi_pnp_scan_handler_attach() ?
> 
because the PNP bus is not ready at that time.

thanks,
rui

Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus for _HID enumeration

[Zhang Rui]

On Fri, 2014-03-07 at 02:46 +0100, Rafael J. Wysocki wrote:
> On Tuesday, March 04, 2014 01:35:00 AM Rafael J. Wysocki wrote:
> > On 3/4/2014 1:27 AM, Zhang, Rui wrote:
> > >
> > >> -----Original Message-----
> > >> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > >> Sent: Tuesday, March 04, 2014 7:23 AM
> > >> To: Zhang, Rui
> > >> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org;
> > >> bhelgaas@google.com; matthew.garrett@nebula.com; Wysocki, Rafael J;
> > >> dmitry.torokhov@gmail.com
> > >> Subject: Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus
> > >> for _HID enumeration
> > >> Importance: High
> > >>
> > >> On Monday, March 03, 2014 10:11:48 PM Zhang Rui wrote:
> > >>> On Mon, 2014-03-03 at 00:51 +0100, Rafael J. Wysocki wrote:
> > >>>> On Wednesday, February 26, 2014 05:11:12 PM Zhang Rui wrote:
> > >>>>> Because of the growing demand for enumerating ACPI devices to
> > >>>>> platform bus, this patch changes the code to enumerate ACPI
> > >>>>> devices with _HID/_CID to platform bus by default, unless the
> > >> device already has a scan handler attached.
> > >>>>> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > >>>>> ---
> > >>>>>   drivers/acpi/acpi_platform.c |   28 ----------------------------
> > >>>>>   drivers/acpi/scan.c          |   12 ++++++------
> > >>>>>   2 files changed, 6 insertions(+), 34 deletions(-)
> > >>>>>
> > >>>>> diff --git a/drivers/acpi/acpi_platform.c
> > >>>>> b/drivers/acpi/acpi_platform.c index dbfe49e..33376a9 100644
> > >>>>> --- a/drivers/acpi/acpi_platform.c
> > >>>>> +++ b/drivers/acpi/acpi_platform.c
> > >>>>> @@ -22,24 +22,6 @@
> > >>>>>
> > >>>>>   ACPI_MODULE_NAME("platform");
> > >>>>>
> > >>>>> -/*
> > >>>>> - * The following ACPI IDs are known to be suitable for
> > >>>>> representing as
> > >>>>> - * platform devices.
> > >>>>> - */
> > >>>>> -static const struct acpi_device_id acpi_platform_device_ids[] =
> > >> {
> > >>>>> -
> > >>>>> -	{ "PNP0D40" },
> > >>>>> -	{ "ACPI0003" },
> > >>>>> -	{ "VPC2004" },
> > >>>>> -	{ "BCM4752" },
> > >>>>> -
> > >>>>> -	/* Intel Smart Sound Technology */
> > >>>>> -	{ "INT33C8" },
> > >>>>> -	{ "80860F28" },
> > >>>>> -
> > >>>>> -	{ }
> > >>>>> -};
> > >>>>> -
> > >>>>>   /**
> > >>>>>    * acpi_create_platform_device - Create platform device for ACPI
> > >> device node
> > >>>>>    * @adev: ACPI device node to create a platform device for.
> > >>>>> @@ -125,13 +107,3 @@ int acpi_create_platform_device(struct
> > >> acpi_device *adev,
> > >>>>>   	kfree(resources);
> > >>>>>   	return 1;
> > >>>>>   }
> > >>>>> -
> > >>>>> -static struct acpi_scan_handler platform_handler = {
> > >>>>> -	.ids = acpi_platform_device_ids,
> > >>>>> -	.attach = acpi_create_platform_device,
> > >>>>> -};
> > >>>>> -
> > >>>>> -void __init acpi_platform_init(void) -{
> > >>>>> -	acpi_scan_add_handler(&platform_handler);
> > >>>>> -}
> > >>>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index
> > >>>>> 5967338..61af32e 100644
> > >>>>> --- a/drivers/acpi/scan.c
> > >>>>> +++ b/drivers/acpi/scan.c
> > >>>>> @@ -2022,14 +2022,15 @@ static int
> > >> acpi_scan_attach_handler(struct acpi_device *device)
> > >>>>>   		handler = acpi_scan_match_handler(hwid->id, &devid);
> > >>>>>   		if (handler) {
> > >>>>>   			ret = handler->attach(device, devid);
> > >>>>> -			if (ret > 0) {
> > >>>>> +			if (ret > 0)
> > >>>>>   				device->handler = handler;
> > >>>>> -				break;
> > >>>>> -			} else if (ret < 0) {
> > >>>>> -				break;
> > >>>>> -			}
> > >>>>> +			if (ret)
> > >>>>> +				goto end;
> > >>>>>   		}
> > >>>>>   	}
> > >>>>> +end:
> > >>>>> +	if (!list_empty(&device->pnp.ids) && !device->handler)
> > >>>> I'm a bit concerned that this check will create platform devices
> > >> for
> > >>>> too many ACPI device objects.
> > >>> agreed. there are some devices created unexpected by this patch, e.g.
> > >>> on my test machine, I can see
> > >>>
> > >>> /sys/bus/platform/devices/LNXSYSTM:00 (ACPI system bus/root node)
> > >>> /sys/bus/platform/devices/PNP0000:00  (PIC)
> > >>> /sys/bus/platform/devices/PNP0100:00  (system timer?)
> > >>>
> > >>>>    Shouldn't we require that _HID or at least _CID is present for
> > >>>> that?
> > >>>>
> > >>> I do not think so.
> > >>> only devices that invoke acpi_add_ids() may have pnp.ids but no
> > >>> _HID/_CID, right?
> > >>> I did a check in the code, those devices include:
> > >> Well, I did that too.
> > >>
> > >>> ACPI root node
> > >>> ACPI video
> > >>> ACPI bay
> > >>> ACPI dock
> > >>> IBM SMBus
> > >>> ACPI Power resource
> > >>> ACPI processor
> > >>> ACPI thermal
> > >>> ACPI fixed power/sleep button
> > >>>
> > >>> IMO, only the ACPI root node, ACPI power resource, possibly ACPI
> > >>> processor are the ones that we do not want to see in platform bus.
> > >> No, we don't want any of them.  So pretty much as I said, only if
> > >> _HID/_CID is present, please?
> > >>
> > > Why? We will convert the drivers for most of those devices from ACPI bus to platform bus sooner or later.
> > > We need to see them in platform bus...
> > 
> > No, we don't.
> > 
> > I'm not sure about IBM SMBus to be honest, but as for the rest:
> > 
> > Why would we want one for the ACPI root?
> > 
> > And for video?  Those things are PCI usually devices anyway and we just 
> > add "artificial" HIDs for them.
> > 
> > ACPI docks and bays are handled by the dock driver which creates 
> > platform devices for them already if needed and we don't want duplicates 
> > there.
> > 
> > ACPI processor has its own scan handler that binds those objects to 
> > system devices.
> > 
> > Power resources - no need.
> > 
> > Do we need platform devices for ACPI thermal zones?
> > 
> > Yes, we will need them for fixed buttons, but that's a special case anyway.
> 
> So, why don't we add an ACPI device object flag, say hid_device, such that if
> set, the ACPI core will create a struct platform device for that device object.
> Then, we can set hid_device for buttons and other stuff we care about.
> 
agreed. I will do this in next version.
But anyway, the exclude list is still needed for the _HID devices that
we do not want to see in platform, e.g.
/sys/bus/platform/devices/PNP0000:00  (PIC)
/sys/bus/platform/devices/PNP0100:00  (system timer?)


thanks,
rui

> Thanks,
> Rafael
> 

Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus for _HID enumeration

[Zhang Rui]

On Wed, 2014-02-26 at 17:11 +0800, Zhang Rui wrote:
> Because of the growing demand for enumerating ACPI devices to platform bus,
> this patch changes the code to enumerate ACPI devices with _HID/_CID to
> platform bus by default, unless the device already has a scan handler attached.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/acpi_platform.c |   28 ----------------------------
>  drivers/acpi/scan.c          |   12 ++++++------
>  2 files changed, 6 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> index dbfe49e..33376a9 100644
> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -22,24 +22,6 @@
>  
>  ACPI_MODULE_NAME("platform");
>  
> -/*
> - * The following ACPI IDs are known to be suitable for representing as
> - * platform devices.
> - */
> -static const struct acpi_device_id acpi_platform_device_ids[] = {
> -
> -	{ "PNP0D40" },
> -	{ "ACPI0003" },
> -	{ "VPC2004" },
> -	{ "BCM4752" },
> -
> -	/* Intel Smart Sound Technology */
> -	{ "INT33C8" },
> -	{ "80860F28" },
> -
> -	{ }
> -};
> -
>  /**
>   * acpi_create_platform_device - Create platform device for ACPI device node
>   * @adev: ACPI device node to create a platform device for.
> @@ -125,13 +107,3 @@ int acpi_create_platform_device(struct acpi_device *adev,
>  	kfree(resources);
>  	return 1;
>  }
> -
> -static struct acpi_scan_handler platform_handler = {
> -	.ids = acpi_platform_device_ids,
> -	.attach = acpi_create_platform_device,
> -};
> -
> -void __init acpi_platform_init(void)
> -{
> -	acpi_scan_add_handler(&platform_handler);
> -}
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 5967338..61af32e 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2022,14 +2022,15 @@ static int acpi_scan_attach_handler(struct acpi_device *device)
>  		handler = acpi_scan_match_handler(hwid->id, &devid);
>  		if (handler) {
>  			ret = handler->attach(device, devid);
> -			if (ret > 0) {
> +			if (ret > 0)
>  				device->handler = handler;
> -				break;
> -			} else if (ret < 0) {
> -				break;
> -			}
> +			if (ret)
> +				goto end;
>  		}
>  	}
> +end:
> +	if (!list_empty(&device->pnp.ids) && !device->handler)
> +		acpi_create_platform_device(device, NULL);

I just found a big problem in this proposal, which affects all the
optional scan handlers.
The problem is that, if we disable a scan handler, platform device nodes
would be created instead by the code above, because there is no scan
handler attached for those ACPI nodes.

If we still want to use this proposal, in order to fix the problem, I
think we can
1. add an ACPI device object flag, as Rafael proposed, but with a
different name, say need_enumerate.
2. set the flag for the ACPI device objects with _HID/_CID, and other
specified devices like thermal, video, etc.
3. clear the need_enumerate flag for devices that HAVE MATCHED SCAN
HANDLER, no matter the return value of the .attach() callback.
4. introduce dummy scan handlers instead of stub functions for those
optional scan handlers, say, if CONFIG_X86_INTEL_LPSS is cleared, a
dummy lpss_handler is registered in acpi_lpss_init().
5. invoke acpi_create_platform_device() for the ACPI device objects with
need_enumerate flag set.

thanks,
rui

>  	return ret;
>  }
>  
> @@ -2185,7 +2186,6 @@ int __init acpi_scan_init(void)
>  	acpi_pci_root_init();
>  	acpi_pci_link_init();
>  	acpi_processor_init();
> -	acpi_platform_init();
>  	acpi_lpss_init();
>  	acpi_cmos_rtc_init();
>  	acpi_container_init();

Re: [RFC PATCH 2/8] PNPACPI: use whilte list for pnpacpi device enumeration

[Rafael J. Wysocki]

On Sunday, March 09, 2014 01:29:30 PM Zhang Rui wrote:
> On Fri, 2014-03-07 at 02:44 +0100, Rafael J. Wysocki wrote:
> > On Wednesday, February 26, 2014 05:11:08 PM Zhang Rui wrote:
> > > +
> > > +static int __init acpi_pnp_scan_handler_attach(struct acpi_device *adev,
> > 
> > This can't be __init.
> > 
> > > +                                   const struct acpi_device_id *id)
> > > +{
> > > +	return 1;
> > > +}
> > > +
> > > +static int __init acpi_pnp_scan_handler_match(char *devid, char *handlerid)
> > 
> > And this too.
> > 
> > > +{
> > > +	int i;
> > > +
> > > +	if (!ispnpidacpi(devid))
> > > +		return 0;
> > > +
> > > +	if (memcmp(devid, handlerid, 3))
> > > +		return 0;
> > > +
> > > +        for (i = 3; i < 7; i++) {
> > > +                if (handlerid[i] != 'X' &&
> > > +		    toupper(devid[i]) != toupper(handlerid[i]))
> > > +                        return 0;
> > > +        }
> > > +	return 1;
> > > +}
> > > +
> > > +static struct acpi_scan_handler pnpacpi_handler __initdata = {
> > 
> > And this cannot be __initdata, because the list of ACPI scan handlers is
> > walked during hotplug.
> > 
> right. will update it in next version.
> 
> > > +	.ids = acpi_pnp_device_ids,
> > > +	.match = acpi_pnp_scan_handler_match,
> > > +	.attach = acpi_pnp_scan_handler_attach,
> > > +};
> > > +
> > > +void __init acpi_pnp_init(void)
> > > +{
> > > +	acpi_scan_add_handler(&pnpacpi_handler);
> > > +}
> > > +
> > > +static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
> > > +						     u32 lvl, void *context,
> > > +						     void **rv)
> > > +{
> > > +	struct acpi_device *device;
> > > +
> > > +	if (acpi_bus_get_device(handle, &device))
> > > +		return AE_CTRL_DEPTH;
> > > +	if (device->handler == &pnpacpi_handler)
> > > +		pnpacpi_add_device(device);
> > 
> > Why don't you do that in acpi_pnp_scan_handler_attach() ?
> > 
> because the PNP bus is not ready at that time.

Do you mean it hasn't been initialized yet?

But we can initialize it before the scan handler initialization, can't we?

Rafael

Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus for _HID enumeration

[Rafael J. Wysocki]

On Sunday, March 09, 2014 01:33:27 PM Zhang Rui wrote:
> On Fri, 2014-03-07 at 02:46 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, March 04, 2014 01:35:00 AM Rafael J. Wysocki wrote:
> > > On 3/4/2014 1:27 AM, Zhang, Rui wrote:
> > > >
> > > >> -----Original Message-----
> > > >> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > >> Sent: Tuesday, March 04, 2014 7:23 AM
> > > >> To: Zhang, Rui
> > > >> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > >> bhelgaas@google.com; matthew.garrett@nebula.com; Wysocki, Rafael J;
> > > >> dmitry.torokhov@gmail.com
> > > >> Subject: Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus
> > > >> for _HID enumeration
> > > >> Importance: High
> > > >>
> > > >> On Monday, March 03, 2014 10:11:48 PM Zhang Rui wrote:
> > > >>> On Mon, 2014-03-03 at 00:51 +0100, Rafael J. Wysocki wrote:
> > > >>>> On Wednesday, February 26, 2014 05:11:12 PM Zhang Rui wrote:
> > > >>>>> Because of the growing demand for enumerating ACPI devices to
> > > >>>>> platform bus, this patch changes the code to enumerate ACPI
> > > >>>>> devices with _HID/_CID to platform bus by default, unless the
> > > >> device already has a scan handler attached.
> > > >>>>> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > >>>>> ---
> > > >>>>>   drivers/acpi/acpi_platform.c |   28 ----------------------------
> > > >>>>>   drivers/acpi/scan.c          |   12 ++++++------
> > > >>>>>   2 files changed, 6 insertions(+), 34 deletions(-)
> > > >>>>>
> > > >>>>> diff --git a/drivers/acpi/acpi_platform.c
> > > >>>>> b/drivers/acpi/acpi_platform.c index dbfe49e..33376a9 100644
> > > >>>>> --- a/drivers/acpi/acpi_platform.c
> > > >>>>> +++ b/drivers/acpi/acpi_platform.c
> > > >>>>> @@ -22,24 +22,6 @@
> > > >>>>>
> > > >>>>>   ACPI_MODULE_NAME("platform");
> > > >>>>>
> > > >>>>> -/*
> > > >>>>> - * The following ACPI IDs are known to be suitable for
> > > >>>>> representing as
> > > >>>>> - * platform devices.
> > > >>>>> - */
> > > >>>>> -static const struct acpi_device_id acpi_platform_device_ids[] =
> > > >> {
> > > >>>>> -
> > > >>>>> -	{ "PNP0D40" },
> > > >>>>> -	{ "ACPI0003" },
> > > >>>>> -	{ "VPC2004" },
> > > >>>>> -	{ "BCM4752" },
> > > >>>>> -
> > > >>>>> -	/* Intel Smart Sound Technology */
> > > >>>>> -	{ "INT33C8" },
> > > >>>>> -	{ "80860F28" },
> > > >>>>> -
> > > >>>>> -	{ }
> > > >>>>> -};
> > > >>>>> -
> > > >>>>>   /**
> > > >>>>>    * acpi_create_platform_device - Create platform device for ACPI
> > > >> device node
> > > >>>>>    * @adev: ACPI device node to create a platform device for.
> > > >>>>> @@ -125,13 +107,3 @@ int acpi_create_platform_device(struct
> > > >> acpi_device *adev,
> > > >>>>>   	kfree(resources);
> > > >>>>>   	return 1;
> > > >>>>>   }
> > > >>>>> -
> > > >>>>> -static struct acpi_scan_handler platform_handler = {
> > > >>>>> -	.ids = acpi_platform_device_ids,
> > > >>>>> -	.attach = acpi_create_platform_device,
> > > >>>>> -};
> > > >>>>> -
> > > >>>>> -void __init acpi_platform_init(void) -{
> > > >>>>> -	acpi_scan_add_handler(&platform_handler);
> > > >>>>> -}
> > > >>>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index
> > > >>>>> 5967338..61af32e 100644
> > > >>>>> --- a/drivers/acpi/scan.c
> > > >>>>> +++ b/drivers/acpi/scan.c
> > > >>>>> @@ -2022,14 +2022,15 @@ static int
> > > >> acpi_scan_attach_handler(struct acpi_device *device)
> > > >>>>>   		handler = acpi_scan_match_handler(hwid->id, &devid);
> > > >>>>>   		if (handler) {
> > > >>>>>   			ret = handler->attach(device, devid);
> > > >>>>> -			if (ret > 0) {
> > > >>>>> +			if (ret > 0)
> > > >>>>>   				device->handler = handler;
> > > >>>>> -				break;
> > > >>>>> -			} else if (ret < 0) {
> > > >>>>> -				break;
> > > >>>>> -			}
> > > >>>>> +			if (ret)
> > > >>>>> +				goto end;
> > > >>>>>   		}
> > > >>>>>   	}
> > > >>>>> +end:
> > > >>>>> +	if (!list_empty(&device->pnp.ids) && !device->handler)
> > > >>>> I'm a bit concerned that this check will create platform devices
> > > >> for
> > > >>>> too many ACPI device objects.
> > > >>> agreed. there are some devices created unexpected by this patch, e.g.
> > > >>> on my test machine, I can see
> > > >>>
> > > >>> /sys/bus/platform/devices/LNXSYSTM:00 (ACPI system bus/root node)
> > > >>> /sys/bus/platform/devices/PNP0000:00  (PIC)
> > > >>> /sys/bus/platform/devices/PNP0100:00  (system timer?)
> > > >>>
> > > >>>>    Shouldn't we require that _HID or at least _CID is present for
> > > >>>> that?
> > > >>>>
> > > >>> I do not think so.
> > > >>> only devices that invoke acpi_add_ids() may have pnp.ids but no
> > > >>> _HID/_CID, right?
> > > >>> I did a check in the code, those devices include:
> > > >> Well, I did that too.
> > > >>
> > > >>> ACPI root node
> > > >>> ACPI video
> > > >>> ACPI bay
> > > >>> ACPI dock
> > > >>> IBM SMBus
> > > >>> ACPI Power resource
> > > >>> ACPI processor
> > > >>> ACPI thermal
> > > >>> ACPI fixed power/sleep button
> > > >>>
> > > >>> IMO, only the ACPI root node, ACPI power resource, possibly ACPI
> > > >>> processor are the ones that we do not want to see in platform bus.
> > > >> No, we don't want any of them.  So pretty much as I said, only if
> > > >> _HID/_CID is present, please?
> > > >>
> > > > Why? We will convert the drivers for most of those devices from ACPI bus to platform bus sooner or later.
> > > > We need to see them in platform bus...
> > > 
> > > No, we don't.
> > > 
> > > I'm not sure about IBM SMBus to be honest, but as for the rest:
> > > 
> > > Why would we want one for the ACPI root?
> > > 
> > > And for video?  Those things are PCI usually devices anyway and we just 
> > > add "artificial" HIDs for them.
> > > 
> > > ACPI docks and bays are handled by the dock driver which creates 
> > > platform devices for them already if needed and we don't want duplicates 
> > > there.
> > > 
> > > ACPI processor has its own scan handler that binds those objects to 
> > > system devices.
> > > 
> > > Power resources - no need.
> > > 
> > > Do we need platform devices for ACPI thermal zones?
> > > 
> > > Yes, we will need them for fixed buttons, but that's a special case anyway.
> > 
> > So, why don't we add an ACPI device object flag, say hid_device, such that if
> > set, the ACPI core will create a struct platform device for that device object.
> > Then, we can set hid_device for buttons and other stuff we care about.
> > 
> agreed. I will do this in next version.
> But anyway, the exclude list is still needed for the _HID devices that
> we do not want to see in platform, e.g.
> /sys/bus/platform/devices/PNP0000:00  (PIC)
> /sys/bus/platform/devices/PNP0100:00  (system timer?)

I see.

OK

Rafael

Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus for _HID enumeration

[Rafael J. Wysocki]

On Sunday, March 09, 2014 11:50:37 PM Zhang Rui wrote:
> On Wed, 2014-02-26 at 17:11 +0800, Zhang Rui wrote:
> > Because of the growing demand for enumerating ACPI devices to platform bus,
> > this patch changes the code to enumerate ACPI devices with _HID/_CID to
> > platform bus by default, unless the device already has a scan handler attached.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/acpi/acpi_platform.c |   28 ----------------------------
> >  drivers/acpi/scan.c          |   12 ++++++------
> >  2 files changed, 6 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> > index dbfe49e..33376a9 100644
> > --- a/drivers/acpi/acpi_platform.c
> > +++ b/drivers/acpi/acpi_platform.c
> > @@ -22,24 +22,6 @@
> >  
> >  ACPI_MODULE_NAME("platform");
> >  
> > -/*
> > - * The following ACPI IDs are known to be suitable for representing as
> > - * platform devices.
> > - */
> > -static const struct acpi_device_id acpi_platform_device_ids[] = {
> > -
> > -	{ "PNP0D40" },
> > -	{ "ACPI0003" },
> > -	{ "VPC2004" },
> > -	{ "BCM4752" },
> > -
> > -	/* Intel Smart Sound Technology */
> > -	{ "INT33C8" },
> > -	{ "80860F28" },
> > -
> > -	{ }
> > -};
> > -
> >  /**
> >   * acpi_create_platform_device - Create platform device for ACPI device node
> >   * @adev: ACPI device node to create a platform device for.
> > @@ -125,13 +107,3 @@ int acpi_create_platform_device(struct acpi_device *adev,
> >  	kfree(resources);
> >  	return 1;
> >  }
> > -
> > -static struct acpi_scan_handler platform_handler = {
> > -	.ids = acpi_platform_device_ids,
> > -	.attach = acpi_create_platform_device,
> > -};
> > -
> > -void __init acpi_platform_init(void)
> > -{
> > -	acpi_scan_add_handler(&platform_handler);
> > -}
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 5967338..61af32e 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -2022,14 +2022,15 @@ static int acpi_scan_attach_handler(struct acpi_device *device)
> >  		handler = acpi_scan_match_handler(hwid->id, &devid);
> >  		if (handler) {
> >  			ret = handler->attach(device, devid);
> > -			if (ret > 0) {
> > +			if (ret > 0)
> >  				device->handler = handler;
> > -				break;
> > -			} else if (ret < 0) {
> > -				break;
> > -			}
> > +			if (ret)
> > +				goto end;
> >  		}
> >  	}
> > +end:
> > +	if (!list_empty(&device->pnp.ids) && !device->handler)
> > +		acpi_create_platform_device(device, NULL);
> 
> I just found a big problem in this proposal, which affects all the
> optional scan handlers.

What do you mean by "optional"?  Such that can be compiled out?

> The problem is that, if we disable a scan handler, platform device nodes
> would be created instead by the code above, because there is no scan
> handler attached for those ACPI nodes.

If "we disable a scan handled" means "we compile it out", I'm not sure
why creating platform devices for the device objects in question will
be incorrect?

Rafael

Re: [RFC PATCH 2/8] PNPACPI: use whilte list for pnpacpi device enumeration

[Zhang Rui]

On Sun, 2014-03-09 at 18:49 +0100, Rafael J. Wysocki wrote:
> On Sunday, March 09, 2014 01:29:30 PM Zhang Rui wrote:
> > On Fri, 2014-03-07 at 02:44 +0100, Rafael J. Wysocki wrote:
> > > On Wednesday, February 26, 2014 05:11:08 PM Zhang Rui wrote:
> > > > +
> > > > +static int __init acpi_pnp_scan_handler_attach(struct acpi_device *adev,
> > > 
> > > This can't be __init.
> > > 
> > > > +                                   const struct acpi_device_id *id)
> > > > +{
> > > > +	return 1;
> > > > +}
> > > > +
> > > > +static int __init acpi_pnp_scan_handler_match(char *devid, char *handlerid)
> > > 
> > > And this too.
> > > 
> > > > +{
> > > > +	int i;
> > > > +
> > > > +	if (!ispnpidacpi(devid))
> > > > +		return 0;
> > > > +
> > > > +	if (memcmp(devid, handlerid, 3))
> > > > +		return 0;
> > > > +
> > > > +        for (i = 3; i < 7; i++) {
> > > > +                if (handlerid[i] != 'X' &&
> > > > +		    toupper(devid[i]) != toupper(handlerid[i]))
> > > > +                        return 0;
> > > > +        }
> > > > +	return 1;
> > > > +}
> > > > +
> > > > +static struct acpi_scan_handler pnpacpi_handler __initdata = {
> > > 
> > > And this cannot be __initdata, because the list of ACPI scan handlers is
> > > walked during hotplug.
> > > 
> > right. will update it in next version.
> > 
> > > > +	.ids = acpi_pnp_device_ids,
> > > > +	.match = acpi_pnp_scan_handler_match,
> > > > +	.attach = acpi_pnp_scan_handler_attach,
> > > > +};
> > > > +
> > > > +void __init acpi_pnp_init(void)
> > > > +{
> > > > +	acpi_scan_add_handler(&pnpacpi_handler);
> > > > +}
> > > > +
> > > > +static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
> > > > +						     u32 lvl, void *context,
> > > > +						     void **rv)
> > > > +{
> > > > +	struct acpi_device *device;
> > > > +
> > > > +	if (acpi_bus_get_device(handle, &device))
> > > > +		return AE_CTRL_DEPTH;
> > > > +	if (device->handler == &pnpacpi_handler)
> > > > +		pnpacpi_add_device(device);
> > > 
> > > Why don't you do that in acpi_pnp_scan_handler_attach() ?
> > > 
> > because the PNP bus is not ready at that time.
> 
> Do you mean it hasn't been initialized yet?
> 
right.
both acpi_init() and pnp_init() are subsys_initcall, but pnp_init() is
invoked later because of the link order.

> But we can initialize it before the scan handler initialization, can't we?
> 
well, I'm not sure, here is what I get from drivers/Makefile
obj-$(CONFIG_ACPI)              += acpi/
obj-$(CONFIG_SFI)               += sfi/
# PnP must come after ACPI since it will eventually need to check ifacpi
# was used and do nothing if so
obj-$(CONFIG_PNP)               += pnp/

this comment was added long time ago and I do not know the background of
it, maybe Len knows the reason of this?

thanks,
rui

Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus for _HID enumeration

[Zhang Rui]

On Sun, 2014-03-09 at 19:04 +0100, Rafael J. Wysocki wrote:
> On Sunday, March 09, 2014 11:50:37 PM Zhang Rui wrote:
> > On Wed, 2014-02-26 at 17:11 +0800, Zhang Rui wrote:
> > > Because of the growing demand for enumerating ACPI devices to platform bus,
> > > this patch changes the code to enumerate ACPI devices with _HID/_CID to
> > > platform bus by default, unless the device already has a scan handler attached.
> > > 
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >  drivers/acpi/acpi_platform.c |   28 ----------------------------
> > >  drivers/acpi/scan.c          |   12 ++++++------
> > >  2 files changed, 6 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> > > index dbfe49e..33376a9 100644
> > > --- a/drivers/acpi/acpi_platform.c
> > > +++ b/drivers/acpi/acpi_platform.c
> > > @@ -22,24 +22,6 @@
> > >  
> > >  ACPI_MODULE_NAME("platform");
> > >  
> > > -/*
> > > - * The following ACPI IDs are known to be suitable for representing as
> > > - * platform devices.
> > > - */
> > > -static const struct acpi_device_id acpi_platform_device_ids[] = {
> > > -
> > > -	{ "PNP0D40" },
> > > -	{ "ACPI0003" },
> > > -	{ "VPC2004" },
> > > -	{ "BCM4752" },
> > > -
> > > -	/* Intel Smart Sound Technology */
> > > -	{ "INT33C8" },
> > > -	{ "80860F28" },
> > > -
> > > -	{ }
> > > -};
> > > -
> > >  /**
> > >   * acpi_create_platform_device - Create platform device for ACPI device node
> > >   * @adev: ACPI device node to create a platform device for.
> > > @@ -125,13 +107,3 @@ int acpi_create_platform_device(struct acpi_device *adev,
> > >  	kfree(resources);
> > >  	return 1;
> > >  }
> > > -
> > > -static struct acpi_scan_handler platform_handler = {
> > > -	.ids = acpi_platform_device_ids,
> > > -	.attach = acpi_create_platform_device,
> > > -};
> > > -
> > > -void __init acpi_platform_init(void)
> > > -{
> > > -	acpi_scan_add_handler(&platform_handler);
> > > -}
> > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > index 5967338..61af32e 100644
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -2022,14 +2022,15 @@ static int acpi_scan_attach_handler(struct acpi_device *device)
> > >  		handler = acpi_scan_match_handler(hwid->id, &devid);
> > >  		if (handler) {
> > >  			ret = handler->attach(device, devid);
> > > -			if (ret > 0) {
> > > +			if (ret > 0)
> > >  				device->handler = handler;
> > > -				break;
> > > -			} else if (ret < 0) {
> > > -				break;
> > > -			}
> > > +			if (ret)
> > > +				goto end;
> > >  		}
> > >  	}
> > > +end:
> > > +	if (!list_empty(&device->pnp.ids) && !device->handler)
> > > +		acpi_create_platform_device(device, NULL);
> > 
> > I just found a big problem in this proposal, which affects all the
> > optional scan handlers.
> 
> What do you mean by "optional"?  Such that can be compiled out?
> 
yes.

> > The problem is that, if we disable a scan handler, platform device nodes
> > would be created instead by the code above, because there is no scan
> > handler attached for those ACPI nodes.
> 
> If "we disable a scan handled" means "we compile it out", I'm not sure
> why creating platform devices for the device objects in question will
> be incorrect?
> 
take lpss scan handler and 80860F0A device for example,
acpi_lpss_create_device() would invoke lpss_uart_setup() first and then
register 80860F0A as a platform device.
if the lpss scan handler is compiled out, we would do nothing but
register a platform device directly, thus the dw8250_platform_driver
driver is still loaded, but probably breaks.

IMO, we should either have a full functional platform device (if the
scan handler is compiled in) or nothing (if the scan handler is compiled
out).

thanks,
rui
> Rafael
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 6/8] ACPI: use platform bus as the default bus for _HID enumeration

[Zhang Rui]

On Sun, 2014-03-09 at 19:04 +0100, Rafael J. Wysocki wrote:
> On Sunday, March 09, 2014 11:50:37 PM Zhang Rui wrote:
> > On Wed, 2014-02-26 at 17:11 +0800, Zhang Rui wrote:
> > > Because of the growing demand for enumerating ACPI devices to platform bus,
> > > this patch changes the code to enumerate ACPI devices with _HID/_CID to
> > > platform bus by default, unless the device already has a scan handler attached.
> > > 
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >  drivers/acpi/acpi_platform.c |   28 ----------------------------
> > >  drivers/acpi/scan.c          |   12 ++++++------
> > >  2 files changed, 6 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> > > index dbfe49e..33376a9 100644
> > > --- a/drivers/acpi/acpi_platform.c
> > > +++ b/drivers/acpi/acpi_platform.c
> > > @@ -22,24 +22,6 @@
> > >  
> > >  ACPI_MODULE_NAME("platform");
> > >  
> > > -/*
> > > - * The following ACPI IDs are known to be suitable for representing as
> > > - * platform devices.
> > > - */
> > > -static const struct acpi_device_id acpi_platform_device_ids[] = {
> > > -
> > > -	{ "PNP0D40" },
> > > -	{ "ACPI0003" },
> > > -	{ "VPC2004" },
> > > -	{ "BCM4752" },
> > > -
> > > -	/* Intel Smart Sound Technology */
> > > -	{ "INT33C8" },
> > > -	{ "80860F28" },
> > > -
> > > -	{ }
> > > -};
> > > -
> > >  /**
> > >   * acpi_create_platform_device - Create platform device for ACPI device node
> > >   * @adev: ACPI device node to create a platform device for.
> > > @@ -125,13 +107,3 @@ int acpi_create_platform_device(struct acpi_device *adev,
> > >  	kfree(resources);
> > >  	return 1;
> > >  }
> > > -
> > > -static struct acpi_scan_handler platform_handler = {
> > > -	.ids = acpi_platform_device_ids,
> > > -	.attach = acpi_create_platform_device,
> > > -};
> > > -
> > > -void __init acpi_platform_init(void)
> > > -{
> > > -	acpi_scan_add_handler(&platform_handler);
> > > -}
> > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > index 5967338..61af32e 100644
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -2022,14 +2022,15 @@ static int acpi_scan_attach_handler(struct acpi_device *device)
> > >  		handler = acpi_scan_match_handler(hwid->id, &devid);
> > >  		if (handler) {
> > >  			ret = handler->attach(device, devid);
> > > -			if (ret > 0) {
> > > +			if (ret > 0)
> > >  				device->handler = handler;
> > > -				break;
> > > -			} else if (ret < 0) {
> > > -				break;
> > > -			}
> > > +			if (ret)
> > > +				goto end;
> > >  		}
> > >  	}
> > > +end:
> > > +	if (!list_empty(&device->pnp.ids) && !device->handler)
> > > +		acpi_create_platform_device(device, NULL);
> > 
> > I just found a big problem in this proposal, which affects all the
> > optional scan handlers.
> 
> What do you mean by "optional"?  Such that can be compiled out?
> 
yes.

> > The problem is that, if we disable a scan handler, platform device nodes
> > would be created instead by the code above, because there is no scan
> > handler attached for those ACPI nodes.
> 
> If "we disable a scan handled" means "we compile it out", I'm not sure
> why creating platform devices for the device objects in question will
> be incorrect?
> 
take lpss scan handler and 80860F0A device for example,
acpi_lpss_create_device() would invoke lpss_uart_setup() first and then
register 80860F0A as a platform device.
if the lpss scan handler is compiled out, we would do nothing but
register a platform device directly, thus the dw8250_platform_driver
driver is still loaded, but probably breaks.

IMO, we should either have a full functional platform device (if the
scan handler is compiled in) or nothing (if the scan handler is compiled
out).

thanks,
rui
> Rafael
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html