[V7,5/7] ACPI: Delay the enumeration on the devices whose dependency has not met

Commit Message

Zhichang Yuan March 13, 2017, 2:42 a.m. UTC

In commit 40e7fcb1929(ACPI: Add _DEP support to fix battery issue on Asus
T100TA), the '_DEP' was supported to solve the dependency of Asus battery. But
this patch is specific to Asus battery device.
In the real world, there are other devices which need the dependency to play the
role on the enumeration order. For example, all the Hip06 LPC
periperals(IPMI-BT, uart, etc) must be scanned after the LPC host driver
finished the probing. So, it makes sense to add a checking whether the ACPI
device meet all the dependencies during its enumeration slot, if not, the
enumeration will be delayed till all dependency master finish their work.

This patch adds the dependency checking in ACPI enumeration, also the
corresponding handling to retrigger the Hip06 LPC peripherals' scanning.

Signed-off-by: zhichang.yuan <[e-mail]>
---
 drivers/acpi/battery.c |  3 ---
 drivers/acpi/scan.c    |  3 +++
 drivers/bus/hisi_lpc.c | 12 +++++++++++-
 3 files changed, 14 insertions(+), 4 deletions(-)

Comments

Rafael J. Wysocki March 13, 2017, 9:24 p.m. UTC | #1

On Monday, March 13, 2017 10:42:41 AM zhichang.yuan wrote:
> In commit 40e7fcb1929(ACPI: Add _DEP support to fix battery issue on Asus
> T100TA), the '_DEP' was supported to solve the dependency of Asus battery. But
> this patch is specific to Asus battery device.
> In the real world, there are other devices which need the dependency to play the
> role on the enumeration order. For example, all the Hip06 LPC
> periperals(IPMI-BT, uart, etc) must be scanned after the LPC host driver
> finished the probing. So, it makes sense to add a checking whether the ACPI
> device meet all the dependencies during its enumeration slot, if not, the
> enumeration will be delayed till all dependency master finish their work.
> 
> This patch adds the dependency checking in ACPI enumeration, also the
> corresponding handling to retrigger the Hip06 LPC peripherals' scanning.

AFAICS, _DEP is generally abused in the wild and cannot be made generic.  Sorry.

Thanks,
Rafael

kernel test robot March 14, 2017, 5:11 a.m. UTC | #2

Hi zhichang.yuan,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc2 next-20170310]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/zhichang-yuan/LPC-legacy-ISA-I-O-support/20170314-114635
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=alpha 

All errors (new ones prefixed by >>):

   drivers/bus/hisi_lpc.c: In function 'hisilpc_probe':
>> drivers/bus/hisi_lpc.c:529:4: error: implicit declaration of function 'acpi_walk_dep_device_list' [-Werror=implicit-function-declaration]
       acpi_walk_dep_device_list(adev->handle);
       ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/bus/hisi_lpc.c:529:34: error: dereferencing pointer to incomplete type 'struct acpi_device'
       acpi_walk_dep_device_list(adev->handle);
                                     ^~
   cc1: some warnings being treated as errors

vim +/acpi_walk_dep_device_list +529 drivers/bus/hisi_lpc.c

   523			struct acpi_device *adev;
   524	
   525			adev = to_acpi_device_node(dev->fwnode);
   526			if (!adev)
   527				ret = -ENODEV;
   528			else
 > 529				acpi_walk_dep_device_list(adev->handle);
   530		} else {
   531			ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
   532			if (ret)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

kernel test robot March 14, 2017, 6:10 a.m. UTC | #3

Hi zhichang.yuan,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc2 next-20170310]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/zhichang-yuan/LPC-legacy-ISA-I-O-support/20170314-114635
config: tile-allyesconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 4.6.2
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=tile 

All errors (new ones prefixed by >>):

   drivers/bus/hisi_lpc.c: In function 'hisilpc_probe':
   drivers/bus/hisi_lpc.c:529:4: error: implicit declaration of function 'acpi_walk_dep_device_list'
>> drivers/bus/hisi_lpc.c:529:34: error: dereferencing pointer to incomplete type
   cc1: some warnings being treated as errors

vim +529 drivers/bus/hisi_lpc.c

   523			struct acpi_device *adev;
   524	
   525			adev = to_acpi_device_node(dev->fwnode);
   526			if (!adev)
   527				ret = -ENODEV;
   528			else
 > 529				acpi_walk_dep_device_list(adev->handle);
   530		} else {
   531			ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
   532			if (ret)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Gabriele Paoloni March 14, 2017, 8:14 a.m. UTC | #4

Hi Rafael

Many thanks for your review

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:[e-mail]]
> Sent: 13 March 2017 21:25
> To: Yuanzhichang
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-arm-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; Linuxarm; [e-mail]; linux-
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; John Garry; Gabriele
> Paoloni; [e-mail]; [e-mail]; xuwei (O)
> Subject: Re: [PATCH V7 5/7] ACPI: Delay the enumeration on the devices
> whose dependency has not met
> 
> On Monday, March 13, 2017 10:42:41 AM zhichang.yuan wrote:
> > In commit 40e7fcb1929(ACPI: Add _DEP support to fix battery issue on
> Asus
> > T100TA), the '_DEP' was supported to solve the dependency of Asus
> battery. But
> > this patch is specific to Asus battery device.
> > In the real world, there are other devices which need the dependency
> to play the
> > role on the enumeration order. For example, all the Hip06 LPC
> > periperals(IPMI-BT, uart, etc) must be scanned after the LPC host
> driver
> > finished the probing. So, it makes sense to add a checking whether
> the ACPI
> > device meet all the dependencies during its enumeration slot, if not,
> the
> > enumeration will be delayed till all dependency master finish their
> work.
> >
> > This patch adds the dependency checking in ACPI enumeration, also the
> > corresponding handling to retrigger the Hip06 LPC peripherals'
> scanning.
> 
> AFAICS, _DEP is generally abused in the wild and cannot be made
> generic.  Sorry.

Another option here would be to revert this patch and add a dependency
check in the probe functions of the LPC possible children nodes (e.g. 
in the IPMI driver:
http://elixir.free-electrons.com/source/drivers/char/ipmi/ipmi_si_intf.c?v=4.10#L2683
)  

we could add 

	if (device->dep_unmet)
		return -EPROBE_DEFER;

as we now have in acpi/battery.c...

I think this should not make any difference for current shipped FW that has
got no DEP method...

What do you think?

Many Thanks
Gab

> 
> Thanks,
> Rafael

Zhichang Yuan March 16, 2017, 2:21 a.m. UTC | #5

Hi, Rafael,

Thanks for your review!

On 2017/3/14 5:24, Rafael J. Wysocki wrote:
> On Monday, March 13, 2017 10:42:41 AM zhichang.yuan wrote:
>> In commit 40e7fcb1929(ACPI: Add _DEP support to fix battery issue on Asus
>> T100TA), the '_DEP' was supported to solve the dependency of Asus battery. But
>> this patch is specific to Asus battery device.
>> In the real world, there are other devices which need the dependency to play the
>> role on the enumeration order. For example, all the Hip06 LPC
>> periperals(IPMI-BT, uart, etc) must be scanned after the LPC host driver
>> finished the probing. So, it makes sense to add a checking whether the ACPI
>> device meet all the dependencies during its enumeration slot, if not, the
>> enumeration will be delayed till all dependency master finish their work.
>>
>> This patch adds the dependency checking in ACPI enumeration, also the
>> corresponding handling to retrigger the Hip06 LPC peripherals' scanning.
> 
> AFAICS, _DEP is generally abused in the wild and cannot be made generic.  Sorry.
> 

From the ACPI specification, _DEP is for operation region accesses.
You are right...

How about we add a ACPI handler for our LPC bus?? Just like amba.
In this way, we also can solve the issue about LPC enumeration order.

Thanks,
Zhichang


> Thanks,
> Rafael
> 
> 
> .
>

Arnd Bergmann March 16, 2017, 10:13 a.m. UTC | #6

On Thu, Mar 16, 2017 at 3:21 AM, zhichang.yuan
<[e-mail]> wrote:
> Hi, Rafael,
>
> Thanks for your review!
>
> On 2017/3/14 5:24, Rafael J. Wysocki wrote:
>> On Monday, March 13, 2017 10:42:41 AM zhichang.yuan wrote:
>>> In commit 40e7fcb1929(ACPI: Add _DEP support to fix battery issue on Asus
>>> T100TA), the '_DEP' was supported to solve the dependency of Asus battery. But
>>> this patch is specific to Asus battery device.
>>> In the real world, there are other devices which need the dependency to play the
>>> role on the enumeration order. For example, all the Hip06 LPC
>>> periperals(IPMI-BT, uart, etc) must be scanned after the LPC host driver
>>> finished the probing. So, it makes sense to add a checking whether the ACPI
>>> device meet all the dependencies during its enumeration slot, if not, the
>>> enumeration will be delayed till all dependency master finish their work.
>>>
>>> This patch adds the dependency checking in ACPI enumeration, also the
>>> corresponding handling to retrigger the Hip06 LPC peripherals' scanning.
>>
>> AFAICS, _DEP is generally abused in the wild and cannot be made generic.  Sorry.
>>
>
> From the ACPI specification, _DEP is for operation region accesses.
> You are right...
>
> How about we add a ACPI handler for our LPC bus?? Just like amba.
> In this way, we also can solve the issue about LPC enumeration order.

As far as I can tell, PCI and LPC have exactly the same requirement here,
so whatever you end up doing for one should be used for the other as well.

      Arnd

zhichang March 16, 2017, 2:56 p.m. UTC | #7

Hi, Arnd,



On 03/16/2017 06:13 PM, Arnd Bergmann wrote:
> On Thu, Mar 16, 2017 at 3:21 AM, zhichang.yuan
> <[e-mail]> wrote:
>> Hi, Rafael,
>>
>> Thanks for your review!
>>
>> On 2017/3/14 5:24, Rafael J. Wysocki wrote:
>>> On Monday, March 13, 2017 10:42:41 AM zhichang.yuan wrote:
>>>> In commit 40e7fcb1929(ACPI: Add _DEP support to fix battery issue on Asus
>>>> T100TA), the '_DEP' was supported to solve the dependency of Asus battery. But
>>>> this patch is specific to Asus battery device.
>>>> In the real world, there are other devices which need the dependency to play the
>>>> role on the enumeration order. For example, all the Hip06 LPC
>>>> periperals(IPMI-BT, uart, etc) must be scanned after the LPC host driver
>>>> finished the probing. So, it makes sense to add a checking whether the ACPI
>>>> device meet all the dependencies during its enumeration slot, if not, the
>>>> enumeration will be delayed till all dependency master finish their work.
>>>>
>>>> This patch adds the dependency checking in ACPI enumeration, also the
>>>> corresponding handling to retrigger the Hip06 LPC peripherals' scanning.
>>>
>>> AFAICS, _DEP is generally abused in the wild and cannot be made generic.  Sorry.
>>>
>>
>> From the ACPI specification, _DEP is for operation region accesses.
>> You are right...
>>
>> How about we add a ACPI handler for our LPC bus?? Just like amba.
>> In this way, we also can solve the issue about LPC enumeration order.
> 
> As far as I can tell, PCI and LPC have exactly the same requirement here,
> so whatever you end up doing for one should be used for the other as well.
> 

Thanks for your remind!

To some extend, our LPC is similar to PCI. We also need to setup the host
resources before scanning the children. For PCI, there is an ACPI handler of
pci_root_handler. Although our LPC is not so complicated and powerful as PCI(for
example, LPC can't probe downside devices), I think, we can still make LPC host
ready to work in its own ACPI handler callback....

Cheers,
Zhichang


>       Arnd
>

Gabriele Paoloni March 16, 2017, 4:13 p.m. UTC | #8

Hi Arnd

> -----Original Message-----

> From: [e-mail] [mailto:[e-mail]] On Behalf

> Of Arnd Bergmann

> Sent: 16 March 2017 10:13

> To: Yuanzhichang

> Cc: Rafael J. Wysocki; Catalin Marinas; Will Deacon; Rob Herring; Frank

> Rowand; Bjorn Helgaas; Rafael Wysocki; Mark Rutland; Linux ARM; ACPI

> Devel Maling List; Lorenzo Pieralisi; Benjamin Herrenschmidt; Linux

> Kernel Mailing List; Linuxarm; [e-mail]; linux-pci;

> [e-mail]; Corey Minyard; [e-mail]; Zou

> Rongrong; John Garry; Gabriele Paoloni; [e-mail];

> [e-mail]; xuwei (O)

> Subject: Re: [PATCH V7 5/7] ACPI: Delay the enumeration on the devices

> whose dependency has not met

> 

> On Thu, Mar 16, 2017 at 3:21 AM, zhichang.yuan

> <[e-mail]> wrote:

> > Hi, Rafael,

> >

> > Thanks for your review!

> >

> > On 2017/3/14 5:24, Rafael J. Wysocki wrote:

> >> On Monday, March 13, 2017 10:42:41 AM zhichang.yuan wrote:

> >>> In commit 40e7fcb1929(ACPI: Add _DEP support to fix battery issue

> on Asus

> >>> T100TA), the '_DEP' was supported to solve the dependency of Asus

> battery. But

> >>> this patch is specific to Asus battery device.

> >>> In the real world, there are other devices which need the

> dependency to play the

> >>> role on the enumeration order. For example, all the Hip06 LPC

> >>> periperals(IPMI-BT, uart, etc) must be scanned after the LPC host

> driver

> >>> finished the probing. So, it makes sense to add a checking whether

> the ACPI

> >>> device meet all the dependencies during its enumeration slot, if

> not, the

> >>> enumeration will be delayed till all dependency master finish their

> work.

> >>>

> >>> This patch adds the dependency checking in ACPI enumeration, also

> the

> >>> corresponding handling to retrigger the Hip06 LPC peripherals'

> scanning.

> >>

> >> AFAICS, _DEP is generally abused in the wild and cannot be made

> generic.  Sorry.

> >>

> >

> > From the ACPI specification, _DEP is for operation region accesses.

> > You are right...

> >

> > How about we add a ACPI handler for our LPC bus?? Just like amba.

> > In this way, we also can solve the issue about LPC enumeration order.

> 

> As far as I can tell, PCI and LPC have exactly the same requirement

> here,

> so whatever you end up doing for one should be used for the other as

> well.


Well as you know PCI has got his own handler, identified by his own
namespace id "PNP0A03".
Now when you say "you end up doing for one should be used for the other"
are you saying that we should introduce a new class of devices?
i.e. should we have an ACPI namespace identifier for non-PCI IO Host
Controllers?  

Otherwise, if my understanding is correct, having a specific new ACPI
handler for HiSilicon LPC would mean to adding another function_init()
in the list of acpi handlers inits in acpi_scan_init().

But then every vendor would declare his own one...is this really correct?

Many Thanks
Gab

> 

>       Arnd

Gabriele Paoloni March 24, 2017, 12:23 a.m. UTC | #9

Hi Arnd

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> On Behalf Of Gabriele Paoloni
> Sent: 16 March 2017 16:14
> To: Arnd Bergmann; Yuanzhichang
> Cc: Mark Rutland; Benjamin Herrenschmidt; Rafael Wysocki; linux-pci;
> Will Deacon; Linuxarm; Frank Rowand; Lorenzo Pieralisi; ACPI Devel
> Maling List; [e-mail]; Catalin Marinas;
> [e-mail]; Corey Minyard; [e-mail]; Rob
> Herring; Bjorn Helgaas; [e-mail]; [e-mail];
> Linux ARM; Rafael J. Wysocki; Linux Kernel Mailing List; Zou Rongrong
> Subject: RE: [PATCH V7 5/7] ACPI: Delay the enumeration on the devices
> whose dependency has not met
> 
> Hi Arnd
> 
> > -----Original Message-----
> > From: [e-mail] [mailto:[e-mail]] On
> Behalf
> > Of Arnd Bergmann
> > Sent: 16 March 2017 10:13
> > To: Yuanzhichang
> > Cc: Rafael J. Wysocki; Catalin Marinas; Will Deacon; Rob Herring;
> Frank
> > Rowand; Bjorn Helgaas; Rafael Wysocki; Mark Rutland; Linux ARM; ACPI
> > Devel Maling List; Lorenzo Pieralisi; Benjamin Herrenschmidt; Linux
> > Kernel Mailing List; Linuxarm; [e-mail]; linux-pci;
> > [e-mail]; Corey Minyard; [e-mail]; Zou
> > Rongrong; John Garry; Gabriele Paoloni; [e-mail];
> > [e-mail]; xuwei (O)
> > Subject: Re: [PATCH V7 5/7] ACPI: Delay the enumeration on the
> devices
> > whose dependency has not met
> >
> > On Thu, Mar 16, 2017 at 3:21 AM, zhichang.yuan
> > <[e-mail]> wrote:
> > > Hi, Rafael,
> > >
> > > Thanks for your review!
> > >
> > > On 2017/3/14 5:24, Rafael J. Wysocki wrote:
> > >> On Monday, March 13, 2017 10:42:41 AM zhichang.yuan wrote:
> > >>> In commit 40e7fcb1929(ACPI: Add _DEP support to fix battery issue
> > on Asus
> > >>> T100TA), the '_DEP' was supported to solve the dependency of Asus
> > battery. But
> > >>> this patch is specific to Asus battery device.
> > >>> In the real world, there are other devices which need the
> > dependency to play the
> > >>> role on the enumeration order. For example, all the Hip06 LPC
> > >>> periperals(IPMI-BT, uart, etc) must be scanned after the LPC host
> > driver
> > >>> finished the probing. So, it makes sense to add a checking
> whether
> > the ACPI
> > >>> device meet all the dependencies during its enumeration slot, if
> > not, the
> > >>> enumeration will be delayed till all dependency master finish
> their
> > work.
> > >>>
> > >>> This patch adds the dependency checking in ACPI enumeration, also
> > the
> > >>> corresponding handling to retrigger the Hip06 LPC peripherals'
> > scanning.
> > >>
> > >> AFAICS, _DEP is generally abused in the wild and cannot be made
> > generic.  Sorry.
> > >>
> > >
> > > From the ACPI specification, _DEP is for operation region accesses.
> > > You are right...
> > >
> > > How about we add a ACPI handler for our LPC bus?? Just like amba.
> > > In this way, we also can solve the issue about LPC enumeration
> order.
> >
> > As far as I can tell, PCI and LPC have exactly the same requirement
> > here,
> > so whatever you end up doing for one should be used for the other as
> > well.
> 
> Well as you know PCI has got his own handler, identified by his own
> namespace id "PNP0A03".
> Now when you say "you end up doing for one should be used for the
> other"
> are you saying that we should introduce a new class of devices?
> i.e. should we have an ACPI namespace identifier for non-PCI IO Host
> Controllers?
> 
> Otherwise, if my understanding is correct, having a specific new ACPI
> handler for HiSilicon LPC would mean to adding another function_init()
> in the list of acpi handlers inits in acpi_scan_init().
> 
> But then every vendor would declare his own one...is this really
> correct?

Do you have any feedback on this?

Otherwise I think that maybe we could consider moving back to the
arch_initcall approach as proposed in V6:
https://lkml.org/lkml/2017/1/24/28

Thanks
Gab

> 
> Many Thanks
> Gab
> 
> >
> >       Arnd
> _______________________________________________
> linuxarm mailing list
> [e-mail]
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm

Patch

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 4ef1e46..e8d1af1 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -1210,9 +1210,6 @@  static int acpi_battery_add(struct acpi_device *device)
 	if (!device)
 		return -EINVAL;
 
-	if (device->dep_unmet)
-		return -EPROBE_DEFER;
-
 	battery = kzalloc(sizeof(struct acpi_battery), GFP_KERNEL);
 	if (!battery)
 		return -ENOMEM;
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 1926918..97721b1 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1843,6 +1843,9 @@  static void acpi_bus_attach(struct acpi_device *device)
 	if (device->handler)
 		goto ok;
 
+	if (device->dep_unmet)
+		return;
+
 	if (!device->flags.initialized) {
 		device->flags.power_manageable =
 			device->power.states[ACPI_STATE_D0].flags.valid;
diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 03cf19a..345ea12 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -516,8 +516,18 @@  static int hisilpc_probe(struct platform_device *pdev)
 
 	/*
 	 * It is time to start the children scannings....
+	 * For ACPI children, the corresponding devices will be created after
+	 * retriggering the ACPI scanning by removing the dependency blocking.
 	 */
-	if (!has_acpi_companion(dev)) {
+	if (has_acpi_companion(dev)) {
+		struct acpi_device *adev;
+
+		adev = to_acpi_device_node(dev->fwnode);
+		if (!adev)
+			ret = -ENODEV;
+		else
+			acpi_walk_dep_device_list(adev->handle);
+	} else {
 		ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
 		if (ret)
 			dev_err(dev, "OF: enumerate LPC bus fail(%d)\n", ret);