Re: [PATCH v1 1/1] ACPI: Switch to use list_entry_is_head() helper

2022-03-02 Thread Rafael J. Wysocki
On Wed, Mar 2, 2022 at 4:50 PM Andy Shevchenko
 wrote:
>
> On Fri, Feb 11, 2022 at 01:04:23PM +0200, Andy Shevchenko wrote:
> > Since we got list_entry_is_head() helper in the generic header,
> > we may switch the ACPI modules to use it. This eliminates the
> > need in additional variable. In some cases it reduces critical
> > sections as well.
>
> Besides the work required in a couple of cases (LKP) there is an
> ongoing discussion about list loops (and this particular API).
>
> Rafael, what do you think is the best course of action here?

I think the current approach is to do the opposite of what this patch
is attempting to do: avoid using the list iterator outside of the
loop.



Re: [PATCH v1 1/1] ACPI: NFIT: Import GUID before use

2021-12-17 Thread Rafael J. Wysocki
On Mon, Dec 13, 2021 at 9:46 PM Andy Shevchenko
 wrote:
>
> Strictly speaking the comparison between guid_t and raw buffer
> is not correct. Import GUID to variable of guid_t type and then
> compare.
>
> Signed-off-by: Andy Shevchenko 

Dan, are you going to take care of this or should I?

> ---
>  drivers/acpi/nfit/core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 7dd80acf92c7..e5d7f2bda13f 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -678,10 +678,12 @@ static const char *spa_type_name(u16 type)
>
>  int nfit_spa_type(struct acpi_nfit_system_address *spa)
>  {
> +   guid_t guid;
> int i;
>
> +   import_guid(, spa->range_guid);
> for (i = 0; i < NFIT_UUID_MAX; i++)
> -   if (guid_equal(to_nfit_uuid(i), (guid_t *)>range_guid))
> +   if (guid_equal(to_nfit_uuid(i), ))
> return i;
> return -1;
>  }
> --
> 2.33.0
>



Re: [PATCH] base: power: runtime.c: Remove a unnecessary space

2021-04-19 Thread Rafael J. Wysocki
On Sun, Apr 18, 2021 at 11:22 AM Joe Perches  wrote:
>
> On Sun, 2021-04-18 at 09:11 +, Sebastian Fricke wrote:
> > Hey Joe,
>
> Hi Sebastian.
>
> > On 18.04.2021 00:09, Joe Perches wrote:
> > > On Sun, 2021-04-18 at 06:08 +, Sebastian Fricke wrote:
> > > > Remove a redundant space to improve the quality of the comment.
> > > I think this patch is not useful.
> > > It's not redundant.
> >
> > Thank you, I actually found this pattern a few more times but I wanted
> > to check first if this is a mistake or chosen consciously.

I write a double space after a period ending a sentence as a rule and
it is not a mistake.

> []
> > > For drivers/base/power/runtime.c, that 2 space after period style is used
> > > dozens of times and changing a single instance of it isn't very useful.
>
> Even in that single file it's not consistent.
> It's something like 3:1 for 2 spaces over 1 space after period.
>
> I believe it's done more by habit and author age than anything.
> If you learned to type using a typewriter and not a keyboard, then
> you likely still use 2 spaces after a period.

By habit and because I prefer it this way (I find it somewhat easier
to separate sentences from one another this way).

> > True and if I understand you correctly you would rather keep it as is
> > right?
>
> Yes.  IMO: Whitespace in comments like this should not be changed
> unless there's some other significant benefit like better alignment.

Agreed.


Re: [PATCH] ACPI: PM: s2idle: Invoke _PTS for s2idle

2021-04-19 Thread Rafael J. Wysocki
On Mon, Apr 19, 2021 at 11:08 AM Kai-Heng Feng
 wrote:
>
> HP EliteBook 840 G8 reboots on s2idle resume, and HP EliteBook 845 G8
> wakes up immediately on s2idle. Both are caused by the XMM7360 WWAN PCI
> card.
>
> There's a WWAN specific method to really turn off the WWAN via EC:
> Method (_PTS, 1, NotSerialized)  // _PTS: Prepare To Sleep
> {
> ...
> If (CondRefOf (\_SB.PCI0.GP12.PTS))
> {
> \_SB.PCI0.GP12.PTS (Arg0)
> }
> ...
> }
>
> Scope (_SB.PCI0.GP12)
> {
> ...
> Method (PTS, 1, Serialized)
> {
> If (^^LPCB.EC0.ECRG)
> {
> If ((PDID == 0x))
> {
> Return (Zero)
> }
>
> POFF ()
> SGIO (WWBR, One)
> Sleep (0x1E)
> Acquire (^^LPCB.EC0.ECMX, 0x)
> ^^LPCB.EC0.WWP = One
> Release (^^LPCB.EC0.ECMX)
> Sleep (0x01F4)
> }
>
> Return (Zero)
> }
> ...
> }
>
> So let's also invok _PTS for s2idle.
>
> Signed-off-by: Kai-Heng Feng 
> ---
>  drivers/acpi/sleep.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 09fd13757b65..7e84b4b09919 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -698,6 +698,7 @@ int acpi_s2idle_prepare(void)
> }
>
> acpi_enable_wakeup_devices(ACPI_STATE_S0);
> +   acpi_enter_sleep_state_prep(ACPI_STATE_S0);

The system is in S0 already at this point, so not really.

Please use a quirk to address this.

>
> /* Change the configuration of GPEs to avoid spurious wakeup. */
> acpi_enable_all_wakeup_gpes();
> --


Re: [PATCH v1 1/1] PM / wakeup: use dev_set_name() directly

2021-04-15 Thread Rafael J. Wysocki
On Wed, Apr 14, 2021 at 2:53 PM Andy Shevchenko
 wrote:
>
> We have open coded dev_set_name() implementation, replace that
> with a direct call.
>
> Signed-off-by: Andy Shevchenko 
> ---
>  drivers/base/power/wakeup_stats.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/wakeup_stats.c 
> b/drivers/base/power/wakeup_stats.c
> index d638259b829a..5ade7539ac02 100644
> --- a/drivers/base/power/wakeup_stats.c
> +++ b/drivers/base/power/wakeup_stats.c
> @@ -154,7 +154,7 @@ static struct device *wakeup_source_device_create(struct 
> device *parent,
> dev_set_drvdata(dev, ws);
> device_set_pm_not_required(dev);
>
> -   retval = kobject_set_name(>kobj, "wakeup%d", ws->id);
> +   retval = dev_set_name(dev, "wakeup%d", ws->id);
> if (retval)
> goto error;
>
> --

Applied as 5.13 material, thanks!


[GIT PULL] ACPI fix for v5.12-rc8

2021-04-15 Thread Rafael J. Wysocki
Hi Linus,

Please pull from the tag

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 acpi-5.12-rc8

with top-most commit 6998a8800d73116187aad542391ce3b2dd0f9e30

 ACPI: x86: Call acpi_boot_table_init() after acpi_table_upgrade()

on top of commit d434405aaab7d0ebc516b68a8fc4100922d7f5ef

 Linux 5.12-rc7

to receive an ACPI fix for 5.12-rc8.

This restores the initrd-based ACPI table override functionality
broken by one of the recent fixes.

Thanks!


---

Rafael J. Wysocki (1):
  ACPI: x86: Call acpi_boot_table_init() after acpi_table_upgrade()

---

 arch/x86/kernel/setup.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)


Re: "Reporting issues" document feedback

2021-04-14 Thread Rafael J. Wysocki
On Wed, Apr 14, 2021 at 3:22 PM w4v3  wrote:
>
> Hi Thorsten,
>
> Thanks for the quick and illuminating response :)
>
> > Links to your bug report and the thread on the mailing list would have
> > helped here to understand better what's going on, but whatever, they are
> > not that important.
>
> Here you go: https://bugzilla.kernel.org/show_bug.cgi?id=212643
> https://marc.info/?l=linux-acpi=161824910030600=2
>
> > But it should, otherwise the subsystem should remove the line starting
> > with B: ("bugs:" in the webview).
> >
> > Rafael might be able to clarify things.
>
> > But afais it's appropriate there is a B: line: just a few weeks ago I
> > took a quick look at bugzilla and ACPI bugs in particular, and back then
> > most of the bug reports there got handled by the maintainers. That's why
> > I assume you were just unlucky and your report fall through the cracks
> > (but obviously I might be wrong here). And maybe your report even did
> > help: the developer that fixed the issue might have seen both the bug
> > entry and the mailed report, but simply forget to close the former.
>
> Good to know. It does seem like many recent ACPI bug reports on bugzilla
> have been processed by maintainers. Maybe it is the ACPI-subcomponent I
> chose for the bug: in Config-Tables, only two other bugs were submitted
> and they did not attract comments. Anyways, I understand now that it's
> not an issue with the document so thanks for forwarding it to Rafael.

As a rule, ACPI bugs submitted through the BZ are processed by the
ACPI team (not necessarily by me in person, though), but the response
time may vary, so it's better to report urgent issues by sending
e-mail to linux-a...@vger.kernel.org.

Definitely issues where table dumps or similar are requested are best
handled in the BZ, so reporters can be asked to create a BZ entry for
a bug reported by e-mail anyway.

If you are interested in the history (ie. what issues were reported in
the past), you need to look at both the BZ and the ml record.

HTH


Re: [PATCH] ACPI: x86: Call acpi_boot_table_init() after acpi_table_upgrade()

2021-04-14 Thread Rafael J. Wysocki
On Wed, Apr 14, 2021 at 10:13 AM Mike Rapoport  wrote:
>
> On Wed, Apr 14, 2021 at 09:42:01AM +0200, David Hildenbrand wrote:
> > On 13.04.21 19:53, Rafael J. Wysocki wrote:
> > > On Tue, Apr 13, 2021 at 7:43 PM David Hildenbrand  
> > > wrote:
> > > >
> > > > On 13.04.21 16:01, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki 
> > > > >
> > > > > Commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by
> > > > > ACPI tables") attempted to address an issue with reserving the memory
> > > > > occupied by ACPI tables, but it broke the initrd-based table override
> > > > > mechanism relied on by multiple users.
> > > > >
> > > > > To restore the initrd-based ACPI table override functionality, move
> > > > > the acpi_boot_table_init() invocation in setup_arch() on x86 after
> > > > > the acpi_table_upgrade() one.
> > > > >
> > > > > Fixes: 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by 
> > > > > ACPI tables")
> > > > > Reported-by: Hans de Goede 
> > > > > Tested-by: Hans de Goede 
> > > > > Signed-off-by: Rafael J. Wysocki 
> > > > > ---
> > > > >
> > > > > George, can you please check if this reintroduces the issue addressed 
> > > > > by
> > > > > the above commit for you?
> > > > >
> > > > > ---
> > > > >arch/x86/kernel/setup.c |5 ++---
> > > > >1 file changed, 2 insertions(+), 3 deletions(-)
> > > > >
> > > > > Index: linux-pm/arch/x86/kernel/setup.c
> > > > > ===
> > > > > --- linux-pm.orig/arch/x86/kernel/setup.c
> > > > > +++ linux-pm/arch/x86/kernel/setup.c
> > > > > @@ -1045,9 +1045,6 @@ void __init setup_arch(char **cmdline_p)
> > > > >
> > > > >cleanup_highmap();
> > > > >
> > > > > - /* Look for ACPI tables and reserve memory occupied by them. */
> > > > > - acpi_boot_table_init();
> > > > > -
> > > > >memblock_set_current_limit(ISA_END_ADDRESS);
> > > > >e820__memblock_setup();
> > > > >
> > > > > @@ -1132,6 +1129,8 @@ void __init setup_arch(char **cmdline_p)
> > > > >reserve_initrd();
> > > > >
> > > > >acpi_table_upgrade();
> > > > > + /* Look for ACPI tables and reserve memory occupied by them. */
> > > > > + acpi_boot_table_init();
> > > > >
> > > > >vsmp_init();
> > > >
> > > > This is fairly late; especially, it's after actual allocations -- see
> > > > e820__memblock_alloc_reserved_mpc_new().
> > > >
> > > > Can't the table upgrade mechanism fix up when adjusting something?
> > >
> > > Not at this point of the cycle I'm afraid.
> > >
> > > > Some details on what actually breaks would be helpful.
> > >
> > > Generally speaking, the table overrides that come from the initrd are
> > > not taken into account if acpi_boot_table_init() runs before
> > > acpi_table_upgrade() and the latter cannot run before
> > > reserve_initrd().
> >
> > I see. (looking at Documentation/acpi/initrd_table_override.txt I understand
> > what acpi table overrides are for :) )
> >
> > >
> > > Honestly, I'm not sure how much effort it would take to untangle this ATM.
> >
> > Also true; ideally, we wouldn't have any allocations (find+reserve) before
> > ordinary reservations are done.
> >
> > However, I have no idea if we can move
> > e820__memblock_alloc_reserved_mpc_new() and reserve_real_mode() around
> > easily. Also, reserve_initrd()->relocate_initrd() does allocations.
>
> Even if we can move e820__memblock_alloc_reserved_mpc_new() and
> reserve_real_mode(), the allocation in reserve_initrd() has to be before
> the tables override, we would only reduce the probability of allocating an
> ACPI page.
>
> I think what we can do is to override the ACPI tables separately from their
> initial parsing. Rafael, what do you say?

Well, possibly.  With one caveat that parsing a table that's going to
be overridden subsequently may not be a good idea.

Anyway, things like that can only be done in the next cycle or later.


Re: [PATCH] xen: Remove support for PV ACPI cpu/memory hotplug

2021-04-13 Thread Rafael J. Wysocki
On Tue, Apr 13, 2021 at 7:53 PM Boris Ostrovsky
 wrote:
>
> Commit 76fc253723ad ("xen/acpi-stub: Disable it b/c the acpi_processor_add
> is no longer called.") declared as BROKEN support for Xen ACPI stub (which
> is required for xen-acpi-{cpu|memory}-hotplug) and suggested that this
> is temporary and will be soon fixed. This was in March 2013.
>
> Further, commit cfafae940381 ("xen: rename dom0_op to platform_op")
> renamed an interface used by memory hotplug code without updating that
> code (as it was BROKEN and therefore not compiled). This was
> in November 2015 and has gone unnoticed for over 5 year.
>
> It is now clear that this code is of no interest to anyone and therefore
> should be removed.
>
> Signed-off-by: Boris Ostrovsky 

Acked-by: Rafael J. Wysocki 

Thanks for doing this!

> ---
>  drivers/xen/Kconfig   |  31 ---
>  drivers/xen/Makefile  |   3 -
>  drivers/xen/pcpu.c|  35 ---
>  drivers/xen/xen-acpi-cpuhotplug.c | 446 ---
>  drivers/xen/xen-acpi-memhotplug.c | 475 
> --
>  drivers/xen/xen-stub.c|  90 
>  include/xen/acpi.h|  35 ---
>  7 files changed, 1115 deletions(-)
>  delete mode 100644 drivers/xen/xen-acpi-cpuhotplug.c
>  delete mode 100644 drivers/xen/xen-acpi-memhotplug.c
>  delete mode 100644 drivers/xen/xen-stub.c
>
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index ea0efd290c37..5f1ce59b44b9 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -238,37 +238,6 @@ config XEN_PRIVCMD
> depends on XEN
> default m
>
> -config XEN_STUB
> -   bool "Xen stub drivers"
> -   depends on XEN && X86_64 && BROKEN
> -   help
> - Allow kernel to install stub drivers, to reserve space for Xen 
> drivers,
> - i.e. memory hotplug and cpu hotplug, and to block native drivers 
> loaded,
> - so that real Xen drivers can be modular.
> -
> - To enable Xen features like cpu and memory hotplug, select Y here.
> -
> -config XEN_ACPI_HOTPLUG_MEMORY
> -   tristate "Xen ACPI memory hotplug"
> -   depends on XEN_DOM0 && XEN_STUB && ACPI
> -   help
> - This is Xen ACPI memory hotplug.
> -
> - Currently Xen only support ACPI memory hot-add. If you want
> - to hot-add memory at runtime (the hot-added memory cannot be
> - removed until machine stop), select Y/M here, otherwise select N.
> -
> -config XEN_ACPI_HOTPLUG_CPU
> -   tristate "Xen ACPI cpu hotplug"
> -   depends on XEN_DOM0 && XEN_STUB && ACPI
> -   select ACPI_CONTAINER
> -   help
> - Xen ACPI cpu enumerating and hotplugging
> -
> - For hotplugging, currently Xen only support ACPI cpu hotadd.
> - If you want to hotadd cpu at runtime (the hotadded cpu cannot
> - be removed until machine stop), select Y/M here.
> -
>  config XEN_ACPI_PROCESSOR
> tristate "Xen ACPI processor"
> depends on XEN && XEN_DOM0 && X86 && ACPI_PROCESSOR && CPU_FREQ
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index c3621b9f4012..3434593455b2 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -26,9 +26,6 @@ obj-$(CONFIG_SWIOTLB_XEN) += swiotlb-xen.o
>  obj-$(CONFIG_XEN_MCE_LOG)  += mcelog.o
>  obj-$(CONFIG_XEN_PCIDEV_BACKEND)   += xen-pciback/
>  obj-$(CONFIG_XEN_PRIVCMD)  += xen-privcmd.o
> -obj-$(CONFIG_XEN_STUB) += xen-stub.o
> -obj-$(CONFIG_XEN_ACPI_HOTPLUG_MEMORY)  += xen-acpi-memhotplug.o
> -obj-$(CONFIG_XEN_ACPI_HOTPLUG_CPU) += xen-acpi-cpuhotplug.o
>  obj-$(CONFIG_XEN_ACPI_PROCESSOR)   += xen-acpi-processor.o
>  obj-$(CONFIG_XEN_EFI)  += efi.o
>  obj-$(CONFIG_XEN_SCSI_BACKEND) += xen-scsiback.o
> diff --git a/drivers/xen/pcpu.c b/drivers/xen/pcpu.c
> index cdc6daa7a9f6..1bcdd5227771 100644
> --- a/drivers/xen/pcpu.c
> +++ b/drivers/xen/pcpu.c
> @@ -345,41 +345,6 @@ static irqreturn_t xen_pcpu_interrupt(int irq, void 
> *dev_id)
> return IRQ_HANDLED;
>  }
>
> -/* Sync with Xen hypervisor after cpu hotadded */
> -void xen_pcpu_hotplug_sync(void)
> -{
> -   schedule_work(_pcpu_work);
> -}
> -EXPORT_SYMBOL_GPL(xen_pcpu_hotplug_sync);
> -
> -/*
> - * For hypervisor presented cpu, return logic cpu id;
> - * For hypervisor non-presented cpu, return -ENODEV.
> - */
> -int xen_pcpu_id(uint32_t acpi_id)
> -{
> -   int cpu_id = 0, max_id = 0;

Re: [PATCH] ACPI: x86: Call acpi_boot_table_init() after acpi_table_upgrade()

2021-04-13 Thread Rafael J. Wysocki
On Tue, Apr 13, 2021 at 7:43 PM David Hildenbrand  wrote:
>
> On 13.04.21 16:01, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki 
> >
> > Commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by
> > ACPI tables") attempted to address an issue with reserving the memory
> > occupied by ACPI tables, but it broke the initrd-based table override
> > mechanism relied on by multiple users.
> >
> > To restore the initrd-based ACPI table override functionality, move
> > the acpi_boot_table_init() invocation in setup_arch() on x86 after
> > the acpi_table_upgrade() one.
> >
> > Fixes: 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI 
> > tables")
> > Reported-by: Hans de Goede 
> > Tested-by: Hans de Goede 
> > Signed-off-by: Rafael J. Wysocki 
> > ---
> >
> > George, can you please check if this reintroduces the issue addressed by
> > the above commit for you?
> >
> > ---
> >   arch/x86/kernel/setup.c |5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > Index: linux-pm/arch/x86/kernel/setup.c
> > ===
> > --- linux-pm.orig/arch/x86/kernel/setup.c
> > +++ linux-pm/arch/x86/kernel/setup.c
> > @@ -1045,9 +1045,6 @@ void __init setup_arch(char **cmdline_p)
> >
> >   cleanup_highmap();
> >
> > - /* Look for ACPI tables and reserve memory occupied by them. */
> > - acpi_boot_table_init();
> > -
> >   memblock_set_current_limit(ISA_END_ADDRESS);
> >   e820__memblock_setup();
> >
> > @@ -1132,6 +1129,8 @@ void __init setup_arch(char **cmdline_p)
> >   reserve_initrd();
> >
> >   acpi_table_upgrade();
> > + /* Look for ACPI tables and reserve memory occupied by them. */
> > + acpi_boot_table_init();
> >
> >   vsmp_init();
>
> This is fairly late; especially, it's after actual allocations -- see
> e820__memblock_alloc_reserved_mpc_new().
>
> Can't the table upgrade mechanism fix up when adjusting something?

Not at this point of the cycle I'm afraid.

> Some details on what actually breaks would be helpful.

Generally speaking, the table overrides that come from the initrd are
not taken into account if acpi_boot_table_init() runs before
acpi_table_upgrade() and the latter cannot run before
reserve_initrd().

Honestly, I'm not sure how much effort it would take to untangle this ATM.


[PATCH] ACPI: x86: Call acpi_boot_table_init() after acpi_table_upgrade()

2021-04-13 Thread Rafael J. Wysocki
From: Rafael J. Wysocki 

Commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by
ACPI tables") attempted to address an issue with reserving the memory
occupied by ACPI tables, but it broke the initrd-based table override
mechanism relied on by multiple users.

To restore the initrd-based ACPI table override functionality, move
the acpi_boot_table_init() invocation in setup_arch() on x86 after
the acpi_table_upgrade() one.

Fixes: 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI 
tables")
Reported-by: Hans de Goede 
Tested-by: Hans de Goede 
Signed-off-by: Rafael J. Wysocki 
---

George, can you please check if this reintroduces the issue addressed by
the above commit for you?

---
 arch/x86/kernel/setup.c |5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Index: linux-pm/arch/x86/kernel/setup.c
===
--- linux-pm.orig/arch/x86/kernel/setup.c
+++ linux-pm/arch/x86/kernel/setup.c
@@ -1045,9 +1045,6 @@ void __init setup_arch(char **cmdline_p)
 
cleanup_highmap();
 
-   /* Look for ACPI tables and reserve memory occupied by them. */
-   acpi_boot_table_init();
-
memblock_set_current_limit(ISA_END_ADDRESS);
e820__memblock_setup();
 
@@ -1132,6 +1129,8 @@ void __init setup_arch(char **cmdline_p)
reserve_initrd();
 
acpi_table_upgrade();
+   /* Look for ACPI tables and reserve memory occupied by them. */
+   acpi_boot_table_init();
 
vsmp_init();
 





Re: [PATCH v2 2/2] ACPI: utils: Capitalize abbreviations in the comments

2021-04-13 Thread Rafael J. Wysocki
On Tue, Apr 13, 2021 at 1:21 AM Andy Shevchenko
 wrote:
>
> The DSDT and ACPI should be capitalized.
>
> Signed-off-by: Andy Shevchenko 
> ---
> v2: split from patch 1 as per Rafael's request
>  drivers/acpi/utils.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 60e46efc1bc8..3b54b8fd7396 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -811,7 +811,7 @@ static int acpi_dev_match_cb(struct device *dev, const 
> void *data)
>   * Note that if the device is pluggable, it may since have disappeared.
>   *
>   * Note that unlike acpi_dev_found() this function checks the status
> - * of the device. So for devices which are present in the dsdt, but
> + * of the device. So for devices which are present in the DSDT, but
>   * which are disabled (their _STA callback returns 0) this function
>   * will return false.
>   *
> @@ -838,7 +838,7 @@ EXPORT_SYMBOL(acpi_dev_present);
>
>  /**
>   * acpi_dev_get_next_match_dev - Return the next match of ACPI device
> - * @adev: Pointer to the previous acpi_device matching this @hid, @uid and 
> @hrv
> + * @adev: Pointer to the previous ACPI device matching this @hid, @uid and 
> @hrv
>   * @hid: Hardware ID of the device.
>   * @uid: Unique ID of the device, pass NULL to not check _UID
>   * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
> --

Applied as 5.13 material along with the [1/2], thanks!


Re: [PATCH v2 1/1] ACPI: bus: Introduce acpi_dev_get() and reuse it in ACPI code

2021-04-13 Thread Rafael J. Wysocki
On Tue, Apr 13, 2021 at 12:24 AM Andy Shevchenko
 wrote:
>
> Introduce acpi_dev_get() to have a symmetrical API with acpi_dev_put()
> and reuse both in ACPI code under drivers/acpi folder.
>
> While at it, use acpi_bus_put_acpi_device() in one place rather than above.
>
> Signed-off-by: Andy Shevchenko 
> ---
> v2: made acpi_dev_get() to return pointer as get_device() does (Rafael)
>  drivers/acpi/device_sysfs.c | 4 ++--
>  drivers/acpi/glue.c | 8 
>  drivers/acpi/scan.c | 9 -
>  include/acpi/acpi_bus.h | 5 +
>  4 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index a07d4ade5835..fa2c1c93072c 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -377,12 +377,12 @@ eject_store(struct device *d, struct device_attribute 
> *attr,
> if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
> return -ENODEV;
>
> -   get_device(_device->dev);
> +   acpi_dev_get(acpi_device);
> status = acpi_hotplug_schedule(acpi_device, ACPI_OST_EC_OSPM_EJECT);
> if (ACPI_SUCCESS(status))
> return count;
>
> -   put_device(_device->dev);
> +   acpi_dev_put(acpi_device);
> acpi_evaluate_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT,
>   ACPI_OST_SC_NON_SPECIFIC_FAILURE, NULL);
> return status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN;
> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> index 36b24b0658cb..0715e3be99a0 100644
> --- a/drivers/acpi/glue.c
> +++ b/drivers/acpi/glue.c
> @@ -190,7 +190,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
> *acpi_dev)
> if (!acpi_dev)
> return -EINVAL;
>
> -   get_device(_dev->dev);
> +   acpi_dev_get(acpi_dev);
> get_device(dev);
> physical_node = kzalloc(sizeof(*physical_node), GFP_KERNEL);
> if (!physical_node) {
> @@ -217,7 +217,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
> *acpi_dev)
> goto err;
>
> put_device(dev);
> -   put_device(_dev->dev);
> +   acpi_dev_put(acpi_dev);
> return 0;
> }
> if (pn->node_id == node_id) {
> @@ -257,7 +257,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
> *acpi_dev)
>   err:
> ACPI_COMPANION_SET(dev, NULL);
> put_device(dev);
> -   put_device(_dev->dev);
> +   acpi_dev_put(acpi_dev);
> return retval;
>  }
>  EXPORT_SYMBOL_GPL(acpi_bind_one);
> @@ -285,7 +285,7 @@ int acpi_unbind_one(struct device *dev)
> ACPI_COMPANION_SET(dev, NULL);
> /* Drop references taken by acpi_bind_one(). */
> put_device(dev);
> -   put_device(_dev->dev);
> +   acpi_dev_put(acpi_dev);
> kfree(entry);
> break;
> }
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index d9e024fc6e70..bc973fbd70b2 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -530,7 +530,7 @@ static void acpi_device_del_work_fn(struct work_struct 
> *work_not_used)
>  * used by the device.
>  */
> acpi_power_transition(adev, ACPI_STATE_D3_COLD);
> -   put_device(>dev);
> +   acpi_dev_put(adev);
> }
>  }
>
> @@ -604,8 +604,7 @@ EXPORT_SYMBOL(acpi_bus_get_device);
>
>  static void get_acpi_device(void *dev)
>  {
> -   if (dev)
> -   get_device(&((struct acpi_device *)dev)->dev);
> +   acpi_dev_get(dev);
>  }
>
>  struct acpi_device *acpi_bus_get_acpi_device(acpi_handle handle)
> @@ -615,7 +614,7 @@ struct acpi_device *acpi_bus_get_acpi_device(acpi_handle 
> handle)
>
>  void acpi_bus_put_acpi_device(struct acpi_device *adev)
>  {
> -   put_device(>dev);
> +   acpi_dev_put(adev);
>  }
>
>  static struct acpi_device_bus_id *acpi_device_bus_id_match(const char 
> *dev_id)
> @@ -2355,7 +2354,7 @@ int __init acpi_scan_init(void)
> acpi_detach_data(acpi_root->handle,
>  acpi_scan_drop_device);
> acpi_device_del(acpi_root);
> -   put_device(_root->dev);
> +   acpi_bus_put_acpi_device(acpi_root);
> goto out;
> }
> }
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index d631cb52283e..d2f5afb04a0b 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -695,6 +695,11 @@ acpi_dev_get_first_match_dev(const char *hid, const char 
> *uid, s64 hrv);
>  adev;  \
>  adev = 

Re: [PATCH v1 1/1] ACPI: bus: Introduce acpi_dev_get() and reuse it in ACPI code

2021-04-12 Thread Rafael J. Wysocki
On Mon, Apr 12, 2021 at 8:10 PM Andy Shevchenko
 wrote:
>
> On Mon, Apr 12, 2021 at 9:05 PM Rafael J. Wysocki  wrote:
> >
> > On Mon, Apr 12, 2021 at 7:47 PM Andy Shevchenko
> >  wrote:
> > >
> > > On Mon, Apr 12, 2021 at 8:32 PM Rafael J. Wysocki  
> > > wrote:
> > > > On Sat, Apr 10, 2021 at 3:47 PM Andy Shevchenko
> > > >  wrote:
> > >
> > > ...
> > >
> > > > >  static void get_acpi_device(void *dev)
> > > > >  {
> > > > > -   if (dev)
> > > > > -   get_device(&((struct acpi_device *)dev)->dev);
> > > > > +   acpi_dev_get(dev);
> > > >
> > > > I would do
> > > >
> > > > if (dev)
> > > > acpi_dev_get(dev);
> > > >
> > > > here.
> > >
> > > Hmm... I don't see a point. acpi_dev_get() guaranteed to perform this 
> > > check.
> > >
> > > > >  }
> > >
> > >
> > > > > +static inline void acpi_dev_get(struct acpi_device *adev)
> > > > > +{
> > > > > +   if (adev)
> > > > > +   get_device(>dev);
> > > >
> > > > And I would drop the adev check from here (because the code calling it
> > > > may be running with wrong assumptions if adev is NULL).  Or it should
> > > > return adev and the caller should be held responsible for checking it
> > > > against NULL (if they care).
> > >
> > > But this follows the get_device() / put_device() logic.
> >
> > Not really.  get_device() returns a pointer.
> >
> > > Personally I don't think this is a good idea to deviate.
> >
> > Well, exactly. :-)
> >
> > > Note the acpi_bus_get_acpi_device()
> >
> > This also returns a pointer.
>
> Is it okay to return a pointer in acpi_dev_get() then?

Yes, it is, as I've said already.


Re: [PATCH v1 1/1] ACPI: bus: Introduce acpi_dev_get() and reuse it in ACPI code

2021-04-12 Thread Rafael J. Wysocki
On Mon, Apr 12, 2021 at 7:47 PM Andy Shevchenko
 wrote:
>
> On Mon, Apr 12, 2021 at 8:32 PM Rafael J. Wysocki  wrote:
> > On Sat, Apr 10, 2021 at 3:47 PM Andy Shevchenko
> >  wrote:
>
> ...
>
> > >  static void get_acpi_device(void *dev)
> > >  {
> > > -   if (dev)
> > > -   get_device(&((struct acpi_device *)dev)->dev);
> > > +   acpi_dev_get(dev);
> >
> > I would do
> >
> > if (dev)
> > acpi_dev_get(dev);
> >
> > here.
>
> Hmm... I don't see a point. acpi_dev_get() guaranteed to perform this check.
>
> > >  }
>
>
> > > +static inline void acpi_dev_get(struct acpi_device *adev)
> > > +{
> > > +   if (adev)
> > > +   get_device(>dev);
> >
> > And I would drop the adev check from here (because the code calling it
> > may be running with wrong assumptions if adev is NULL).  Or it should
> > return adev and the caller should be held responsible for checking it
> > against NULL (if they care).
>
> But this follows the get_device() / put_device() logic.

Not really.  get_device() returns a pointer.

> Personally I don't think this is a good idea to deviate.

Well, exactly. :-)

> Note the acpi_bus_get_acpi_device()

This also returns a pointer.

> / acpi_bus_put_acpi_device() as well.


Re: [PATCH v1 1/1] ACPI: scan: Utilize match_string() API

2021-04-12 Thread Rafael J. Wysocki
On Sat, Apr 10, 2021 at 4:02 PM Andy Shevchenko
 wrote:
>
> We have already an API to match a string in the array of strings.
> Utilize it instead of open coded analogues.
>
> Signed-off-by: Andy Shevchenko 
> ---
>  drivers/acpi/scan.c | 22 ++
>  1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index b1d1f1a8ce69..bba6b529cf6c 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -756,27 +756,25 @@ static bool acpi_info_matches_ids(struct 
> acpi_device_info *info,
>   const char * const ids[])
>  {
> struct acpi_pnp_device_id_list *cid_list = NULL;
> -   int i;
> +   int i, index;
>
> if (!(info->valid & ACPI_VALID_HID))
> return false;
>
> +   index = match_string(ids, -1, info->hardware_id.string);
> +   if (index >= 0)
> +   return true;
> +
> if (info->valid & ACPI_VALID_CID)
> cid_list = >compatible_id_list;
>
> -   for (i = 0; ids[i]; i++) {
> -   int j;
> +   if (!cid_list)
> +   return false;
>
> -   if (!strcmp(info->hardware_id.string, ids[i]))
> +   for (i = 0; i < cid_list->count; i++) {
> +   index = match_string(ids, -1, cid_list->ids[i].string);
> +   if (index >= 0)
> return true;
> -
> -   if (!cid_list)
> -   continue;
> -
> -   for (j = 0; j < cid_list->count; j++) {
> -   if (!strcmp(cid_list->ids[j].string, ids[i]))
> -   return true;
> -   }
> }
>
> return false;
> --

Applied as 5.13 material, thanks!


Re: [PATCH v1 1/1] ACPI: bus: Introduce acpi_dev_get() and reuse it in ACPI code

2021-04-12 Thread Rafael J. Wysocki
On Sat, Apr 10, 2021 at 3:47 PM Andy Shevchenko
 wrote:
>
> Introduce acpi_dev_get() to have a symmetrical API with acpi_dev_put()
> and reuse both in ACPI code under drivers/acpi folder.
>
> While at it, use acpi_bus_put_acpi_device() in one place rather than above.
>
> Signed-off-by: Andy Shevchenko 
> ---
>  drivers/acpi/device_sysfs.c | 4 ++--
>  drivers/acpi/glue.c | 8 
>  drivers/acpi/scan.c | 9 -
>  include/acpi/acpi_bus.h | 6 ++
>  4 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index da4ff2a8b06a..35757c3c1b71 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -376,12 +376,12 @@ eject_store(struct device *d, struct device_attribute 
> *attr,
> if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
> return -ENODEV;
>
> -   get_device(_device->dev);
> +   acpi_dev_get(acpi_device);
> status = acpi_hotplug_schedule(acpi_device, ACPI_OST_EC_OSPM_EJECT);
> if (ACPI_SUCCESS(status))
> return count;
>
> -   put_device(_device->dev);
> +   acpi_dev_put(acpi_device);
> acpi_evaluate_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT,
>   ACPI_OST_SC_NON_SPECIFIC_FAILURE, NULL);
> return status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN;
> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> index 36b24b0658cb..0715e3be99a0 100644
> --- a/drivers/acpi/glue.c
> +++ b/drivers/acpi/glue.c
> @@ -190,7 +190,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
> *acpi_dev)
> if (!acpi_dev)
> return -EINVAL;
>
> -   get_device(_dev->dev);
> +   acpi_dev_get(acpi_dev);
> get_device(dev);
> physical_node = kzalloc(sizeof(*physical_node), GFP_KERNEL);
> if (!physical_node) {
> @@ -217,7 +217,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
> *acpi_dev)
> goto err;
>
> put_device(dev);
> -   put_device(_dev->dev);
> +   acpi_dev_put(acpi_dev);
> return 0;
> }
> if (pn->node_id == node_id) {
> @@ -257,7 +257,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
> *acpi_dev)
>   err:
> ACPI_COMPANION_SET(dev, NULL);
> put_device(dev);
> -   put_device(_dev->dev);
> +   acpi_dev_put(acpi_dev);
> return retval;
>  }
>  EXPORT_SYMBOL_GPL(acpi_bind_one);
> @@ -285,7 +285,7 @@ int acpi_unbind_one(struct device *dev)
> ACPI_COMPANION_SET(dev, NULL);
> /* Drop references taken by acpi_bind_one(). */
> put_device(dev);
> -   put_device(_dev->dev);
> +   acpi_dev_put(acpi_dev);
> kfree(entry);
> break;
> }
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index ad2541c0aece..bba6b529cf6c 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -530,7 +530,7 @@ static void acpi_device_del_work_fn(struct work_struct 
> *work_not_used)
>  * used by the device.
>  */
> acpi_power_transition(adev, ACPI_STATE_D3_COLD);
> -   put_device(>dev);
> +   acpi_dev_put(adev);
> }
>  }
>
> @@ -604,8 +604,7 @@ EXPORT_SYMBOL(acpi_bus_get_device);
>
>  static void get_acpi_device(void *dev)
>  {
> -   if (dev)
> -   get_device(&((struct acpi_device *)dev)->dev);
> +   acpi_dev_get(dev);

I would do

if (dev)
acpi_dev_get(dev);

here.

>  }
>
>  struct acpi_device *acpi_bus_get_acpi_device(acpi_handle handle)
> @@ -615,7 +614,7 @@ struct acpi_device *acpi_bus_get_acpi_device(acpi_handle 
> handle)
>
>  void acpi_bus_put_acpi_device(struct acpi_device *adev)
>  {
> -   put_device(>dev);
> +   acpi_dev_put(adev);
>  }
>
>  static struct acpi_device_bus_id *acpi_device_bus_id_match(const char 
> *dev_id)
> @@ -2386,7 +2385,7 @@ int __init acpi_scan_init(void)
> acpi_detach_data(acpi_root->handle,
>  acpi_scan_drop_device);
> acpi_device_del(acpi_root);
> -   put_device(_root->dev);
> +   acpi_bus_put_acpi_device(acpi_root);
> goto out;
> }
> }
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 834b7a1f7405..b728173a6171 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -707,6 +707,12 @@ acpi_dev_get_first_match_dev(const char *hid, const char 
> *uid, s64 hrv);
>  adev;  \
>  adev = acpi_dev_get_next_match_dev(adev, hid, uid, 

Re: [PATCH v1 1/1] ACPI: utils: Document for_each_acpi_dev_match() macro

2021-04-12 Thread Rafael J. Wysocki
On Sat, Apr 10, 2021 at 3:29 PM Andy Shevchenko
 wrote:
>
> The macro requires to call acpi_dev_put() on each iteration.
> Due to this it doesn't tolerate sudden disappearence of the devices.
>
> Document all these nuances to prevent users blindly call it without
> understanding the possible issues.
>
> While at it, add the note to the acpi_dev_get_next_match_dev() and
> advertise acpi_dev_put() instead of put_device() in the whole family
> of the helper functions.
>
> Fixes: bf263f64e804 ("media: ACPI / bus: Add acpi_dev_get_next_match_dev() 
> and helper macro")
> Cc: Daniel Scally 
> Signed-off-by: Andy Shevchenko 
> ---
>  drivers/acpi/utils.c| 12 
>  include/acpi/acpi_bus.h | 13 +
>  2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index f1aff4dab476..3f3171e9aef5 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -811,7 +811,7 @@ static int acpi_dev_match_cb(struct device *dev, const 
> void *data)
>   * Note that if the device is pluggable, it may since have disappeared.
>   *
>   * Note that unlike acpi_dev_found() this function checks the status
> - * of the device. So for devices which are present in the dsdt, but
> + * of the device. So for devices which are present in the DSDT, but
>   * which are disabled (their _STA callback returns 0) this function
>   * will return false.
>   *
> @@ -838,7 +838,7 @@ EXPORT_SYMBOL(acpi_dev_present);
>
>  /**
>   * acpi_dev_get_next_match_dev - Return the next match of ACPI device
> - * @adev: Pointer to the previous acpi_device matching this @hid, @uid and 
> @hrv
> + * @adev: Pointer to the previous ACPI device matching this @hid, @uid and 
> @hrv
>   * @hid: Hardware ID of the device.
>   * @uid: Unique ID of the device, pass NULL to not check _UID
>   * @hrv: Hardware Revision of the device, pass -1 to not check _HRV

The two cleanups above are not related to the subject of the patch.
Please separate them.

> @@ -846,7 +846,11 @@ EXPORT_SYMBOL(acpi_dev_present);
>   * Return the next match of ACPI device if another matching device was 
> present
>   * at the moment of invocation, or NULL otherwise.
>   *
> - * The caller is responsible to call put_device() on the returned device.
> + * Note, the function does not tolerate the sudden disappearance of @adev, 
> e.g.
> + * in the case of hotplug event.

"of a hotplug event"

> That said, caller should ensure that this will

"the caller"

> + * never happen.
> + *
> + * The caller is responsible to call acpi_dev_put() on the returned device.

"responsible for"

And I would say "responsible for invoking".

>   *
>   * See additional information in acpi_dev_present() as well.
>   */
> @@ -875,7 +879,7 @@ EXPORT_SYMBOL(acpi_dev_get_next_match_dev);
>   * Return the first match of ACPI device if a matching device was present
>   * at the moment of invocation, or NULL otherwise.
>   *
> - * The caller is responsible to call put_device() on the returned device.
> + * The caller is responsible to call acpi_dev_put() on the returned device.
>   *
>   * See additional information in acpi_dev_present() as well.
>   */
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index f28b097c658f..834b7a1f7405 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -689,6 +689,19 @@ acpi_dev_get_next_match_dev(struct acpi_device *adev, 
> const char *hid, const cha
>  struct acpi_device *
>  acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv);
>
> +/**
> + * for_each_acpi_dev_match - iterate over ACPI devices that matching the 
> criteria
> + * @adev: pointer to the matching ACPI device, NULL at the end of the loop
> + * @hid: Hardware ID of the device.
> + * @uid: Unique ID of the device, pass NULL to not check _UID
> + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
> + *
> + * The caller is responsible to call acpi_dev_put() on the returned device.

As per the above.

> + *
> + * Due to above requirement there is a window that may invalidate @adev and
> + * next iteration will use a dangling pointer, e.g. in the case of hotplug
> + * event. That said, caller should ensure that this will never happen.
> + */
>  #define for_each_acpi_dev_match(adev, hid, uid, hrv)   \
> for (adev = acpi_dev_get_first_match_dev(hid, uid, hrv);\
>  adev;  \
> --
> 2.31.1
>


[GIT PULL] ACPI fix for v5.12-rc7

2021-04-09 Thread Rafael J. Wysocki
Hi Linus,

Please pull from the tag

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 acpi-5.12-rc7

with top-most commit fa26d0c778b432d3d9814ea82552e813b33eeb5c

 ACPI: processor: Fix build when CONFIG_ACPI_PROCESSOR=m

on top of commit e49d033bddf5b565044e2abe4241353959bc9120

 Linux 5.12-rc6

to receive an ACPI fix for 5.12-rc7.

This fixes a build issue introduced by a previous fix in the ACPI
processor driver (Vitaly Kuznetsov).

Thanks!


---

Vitaly Kuznetsov (1):
  ACPI: processor: Fix build when CONFIG_ACPI_PROCESSOR=m

---

 arch/x86/include/asm/smp.h|  2 +-
 arch/x86/kernel/smpboot.c | 26 --
 drivers/acpi/processor_idle.c |  4 +---
 3 files changed, 14 insertions(+), 18 deletions(-)


Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called

2021-04-09 Thread Rafael J. Wysocki
On Fri, Apr 9, 2021 at 10:36 AM Chunfeng Yun  wrote:
>
> On Fri, 2021-04-09 at 08:39 +0300, Tony Lindgren wrote:
> > * Chunfeng Yun  [210409 01:54]:
> > > On Thu, 2021-04-08 at 19:41 +0200, Rafael J. Wysocki wrote:
> > > > On Thu, Apr 8, 2021 at 11:35 AM Chunfeng Yun 
> > > >  wrote:
> > > > >
> > > > > When the dedicated wake irq is level trigger, enable it before
> > > > > calling runtime_suspend, will trigger an interrupt.
> > > > >
> > > > > e.g.
> > > > > for a low level trigger type, it's low level at running time (0),
> > > > > and becomes high level when enters suspend (runtime_suspend (1) is
> > > > > called), a wakeup signal at (2) make it become low level, wake irq
> > > > > will be triggered.
> > > > >
> > > > > --
> > > > >|   ^ ^|
> > > > >    | | --
> > > > >  |<---(0)--->|<--(1)--|   (3)   (2)(4)
> > > > >
> > > > > if we enable the wake irq before calling runtime_suspend during (0),
> > > > > an interrupt will arise, it causes resume immediately;
> > > >
> > > > But that's necessary to avoid missing a wakeup interrupt, isn't it?
> > > That's also what I worry about.
> >
> > Yeah sounds like this patch will lead into missed wakeirqs.
> If miss level trigger wakeirqs, that means HW doesn't latch it? is it HW
> limitation?

If it's level-triggered, it won't be missed, but then it is just
pointless to suspend the device when wakeup is being signaled in the
first place.

I'm not sure if I understand the underlying problem correctly.  Is it
about addressing spurious wakeups?


Re: [GIT PULL] cpuidle for v5.13-rc1

2021-04-08 Thread Rafael J. Wysocki
On Thu, Apr 8, 2021 at 8:02 PM Daniel Lezcano  wrote:
>
> On 08/04/2021 19:24, Rafael J. Wysocki wrote:
> > On Thu, Apr 8, 2021 at 5:10 PM Daniel Lezcano  
> > wrote:
> >>
> >>
> >> Hi Rafael,
> >>
> >> please consider pulling the following change for cpuidle on ARM for
> >> v5.13-rc1
> >>
> >> Thanks
> >>
> >>   -- Daniel
> >>
> >>
> >> The following changes since commit 
> >> dde8740bd9b505c58ec8b2277d5d55c6951b7e42:
> >>
> >>   Merge branch 'acpi-processor-fixes' into linux-next (2021-04-07
> >> 19:02:56 +0200)
> >
> > Can you please rebase this on 5.12-rc6?  My linux-next branch is
> > re-merged on a regular basis.
> >
> > Generally speaking, if you want me to pull from a branch, please make
> > sure that this branch is based on a commit present in the Linus' tree,
> > preferably one of the commits tagged as -rc or a specific merge.
> >
>
> Sure, here is the pull request based on v5.12-rc6 with the signed tag
> cpuidle-v5.13-rc1

Pulled, thanks!


> The following changes since commit e49d033bddf5b565044e2abe4241353959bc9120:
>
>   Linux 5.12-rc6 (2021-04-04 14:15:36 -0700)
>
> are available in the Git repository at:
>
>   https://git.linaro.org/people/daniel.lezcano/linux tags/cpuidle-v5.13-rc1
>
> for you to fetch changes up to 498ba2a8a2756694b6f357426dbc8a5e6b6c:
>
>   cpuidle: Fix ARM_QCOM_SPM_CPUIDLE configuration (2021-04-08 19:54:14
> +0200)
>
> 
> - Fix the C7 state on the tegra114 by setting the L2-no-flush flag
>   unconditionally (Dmitry Osipenko)
>
> - Remove the do_idle firmware call as it is not supported by the ATF
>   on tegra SoC (Dmitry Osipenko)
>
> - Add a missing dependency on CONFIG_MMU to prevent linkage error (He
>   Ying)
>
> 
> Dmitry Osipenko (2):
>   cpuidle: tegra: Fix C7 idling state on Tegra114
>   cpuidle: tegra: Remove do_idle firmware call
>
> He Ying (1):
>   cpuidle: Fix ARM_QCOM_SPM_CPUIDLE configuration
>
>  drivers/cpuidle/Kconfig.arm |  2 +-
>  drivers/cpuidle/cpuidle-tegra.c | 19 ---
>  2 files changed, 5 insertions(+), 16 deletions(-)
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog


Re: [PATCH] ACPI / CPPC: Replace cppc_attr with kobj_attribute

2021-04-08 Thread Rafael J. Wysocki
On Wed, Apr 7, 2021 at 11:32 PM Nathan Chancellor  wrote:
>
> All of the CPPC sysfs show functions are called via indirect call in
> kobj_attr_show(), where they should be of type
>
> ssize_t (*show)(struct kobject *kobj, struct kobj_attribute *attr, char *buf);
>
> because that is the type of the ->show() member in
> 'struct kobj_attribute' but they are actually of type
>
> ssize_t (*show)(struct kobject *kobj, struct attribute *attr, char *buf);
>
> because of the ->show() member in 'struct cppc_attr', resulting in a
> Control Flow Integrity violation [1].
>
> $ cat /sys/devices/system/cpu/cpu0/acpi_cppc/highest_perf
> 3400
>
> $ dmesg | grep "CFI failure"
> [  175.970559] CFI failure (target: show_highest_perf+0x0/0x8):
>
> As far as I can tell, the only different between 'struct cppc_attr' and
> 'struct kobj_attribute' aside from the type of the attr parameter is the
> type of the count parameter in the ->store() member (ssize_t vs.
> size_t), which does not actually matter because all of these nodes are
> read-only.
>
> Eliminate 'struct cppc_attr' in favor of 'struct kobj_attribute' to fix
> the violation.
>
> [1]: 
> https://lore.kernel.org/r/20210401233216.2540591-1-samitolva...@google.com/
>
> Fixes: 158c998ea44b ("ACPI / CPPC: add sysfs support to compute delivered 
> performance")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1343
> Signed-off-by: Nathan Chancellor 
> ---
>  drivers/acpi/cppc_acpi.c | 14 +++---
>  1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 69057fcd2c04..a5e6fd0bafa1 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -119,23 +119,15 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
>   */
>  #define NUM_RETRIES 500ULL
>
> -struct cppc_attr {
> -   struct attribute attr;
> -   ssize_t (*show)(struct kobject *kobj,
> -   struct attribute *attr, char *buf);
> -   ssize_t (*store)(struct kobject *kobj,
> -   struct attribute *attr, const char *c, ssize_t count);
> -};
> -
>  #define define_one_cppc_ro(_name)  \
> -static struct cppc_attr _name =\
> +static struct kobj_attribute _name =   \
>  __ATTR(_name, 0444, show_##_name, NULL)
>
>  #define to_cpc_desc(a) container_of(a, struct cpc_desc, kobj)
>
>  #define show_cppc_data(access_fn, struct_name, member_name)\
> static ssize_t show_##member_name(struct kobject *kobj, \
> -   struct attribute *attr, char *buf) \
> +   struct kobj_attribute *attr, char *buf) \
> {   \
> struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);   \
> struct struct_name st_name = {0};   \
> @@ -161,7 +153,7 @@ show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, 
> reference_perf);
>  show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, wraparound_time);
>
>  static ssize_t show_feedback_ctrs(struct kobject *kobj,
> -   struct attribute *attr, char *buf)
> +   struct kobj_attribute *attr, char *buf)
>  {
> struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
> struct cppc_perf_fb_ctrs fb_ctrs = {0};
>
> base-commit: 454859c552da78b0f587205d308401922b56863e
> --

Applied as 5.13 material, thanks!


Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called

2021-04-08 Thread Rafael J. Wysocki
On Thu, Apr 8, 2021 at 11:35 AM Chunfeng Yun  wrote:
>
> When the dedicated wake irq is level trigger, enable it before
> calling runtime_suspend, will trigger an interrupt.
>
> e.g.
> for a low level trigger type, it's low level at running time (0),
> and becomes high level when enters suspend (runtime_suspend (1) is
> called), a wakeup signal at (2) make it become low level, wake irq
> will be triggered.
>
> --
>|   ^ ^|
>    | | --
>  |<---(0)--->|<--(1)--|   (3)   (2)(4)
>
> if we enable the wake irq before calling runtime_suspend during (0),
> an interrupt will arise, it causes resume immediately;

But that's necessary to avoid missing a wakeup interrupt, isn't it?

> enable wake irq after calling runtime_suspend, e.g. at (3) or (4),
> will works.
>
> This patch seems no side effect on edge trigger wake irq.
>
> Signed-off-by: Chunfeng Yun 
> ---
>  drivers/base/power/runtime.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index a46a7e30881b..796739a015a5 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -619,12 +619,12 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> __update_runtime_status(dev, RPM_SUSPENDING);
>
> callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> -
> -   dev_pm_enable_wake_irq_check(dev, true);
> retval = rpm_callback(callback, dev);
> if (retval)
> goto fail;
>
> +   dev_pm_enable_wake_irq_check(dev, true);
> +
>   no_callback:
> __update_runtime_status(dev, RPM_SUSPENDED);
> pm_runtime_deactivate_timer(dev);
> @@ -659,7 +659,6 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> return retval;
>
>   fail:
> -   dev_pm_disable_wake_irq_check(dev);
> __update_runtime_status(dev, RPM_ACTIVE);
> dev->power.deferred_resume = false;
> wake_up_all(>power.wait_queue);
> --
> 2.18.0
>


Re: [PATCH -next] PM: fix typos in comments

2021-04-08 Thread Rafael J. Wysocki
On Thu, Apr 8, 2021 at 10:14 AM Lu Jialin  wrote:
>
> Change occured to occurred in kernel/power/autosleep.c.
> Change consiting to consisting in kernel/power/snapshot.c.
> Change avaiable to available in kernel/power/swap.c.
> No functionality changed.
>
> Signed-off-by: Lu Jialin 
> ---
>  kernel/power/autosleep.c | 2 +-
>  kernel/power/snapshot.c  | 2 +-
>  kernel/power/swap.c  | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
> index 9af5a50d3489..b29c8aca7486 100644
> --- a/kernel/power/autosleep.c
> +++ b/kernel/power/autosleep.c
> @@ -54,7 +54,7 @@ static void try_to_suspend(struct work_struct *work)
> goto out;
>
> /*
> -* If the wakeup occured for an unknown reason, wait to prevent the
> +* If the wakeup occurred for an unknown reason, wait to prevent the
>  * system from trying to suspend and waking up in a tight loop.
>  */
> if (final_count == initial_count)
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index 64b7aab9aee4..27cb4e7086b7 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -329,7 +329,7 @@ static void *chain_alloc(struct chain_allocator *ca, 
> unsigned int size)
>  /**
>   * Data types related to memory bitmaps.
>   *
> - * Memory bitmap is a structure consiting of many linked lists of
> + * Memory bitmap is a structure consisting of many linked lists of
>   * objects.  The main list's elements are of type struct zone_bitmap
>   * and each of them corresonds to one zone.  For each zone bitmap
>   * object there is a list of objects of type struct bm_block that
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index 72e33054a2e1..bea3cb8afa11 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -884,7 +884,7 @@ static int save_image_lzo(struct swap_map_handle *handle,
>   * enough_swap - Make sure we have enough swap to save the image.
>   *
>   * Returns TRUE or FALSE after checking the total amount of swap
> - * space avaiable from the resume partition.
> + * space available from the resume partition.
>   */
>
>  static int enough_swap(unsigned int nr_pages)
> --

Applied as 5.13 material, thanks!


Re: [GIT PULL] devfreq next for v5.13

2021-04-08 Thread Rafael J. Wysocki
On Thu, Apr 8, 2021 at 8:12 AM Chanwoo Choi  wrote:
>
> Dear Rafael,
>
> This is devfreq-next pull request for v5.13-rc1. I add detailed description of
> this pull request on the following tag. Please pull devfreq with following 
> updates.
> - tag name : devfreq-next-for-5.12
>
> This pull request contains the immutable branch to keep the immutable patch[1]
> between devfreq and drm for gpu driver.
> [1] 
> https://patchwork.kernel.org/project/linux-pm/patch/20210308133041.10516-1-daniel.lezc...@linaro.org/
>
> Best Regards,
> Chanwoo Choi
>
>
> The following changes since commit e49d033bddf5b565044e2abe4241353959bc9120:
>
>   Linux 5.12-rc6 (2021-04-04 14:15:36 -0700)
>
> are available in the Git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git 
> tags/devfreq-next-for-5.13
>
> for you to fetch changes up to 0a7dc8318c2817fb33dc50946f7ca6e0ff28f036:
>
>   PM / devfreq: imx8m-ddrc: Remove unneeded of_match_ptr() (2021-04-08 
> 13:14:51 +0900)

Pulled, thanks!


Re: [GIT PULL] cpuidle for v5.13-rc1

2021-04-08 Thread Rafael J. Wysocki
On Thu, Apr 8, 2021 at 5:10 PM Daniel Lezcano  wrote:
>
>
> Hi Rafael,
>
> please consider pulling the following change for cpuidle on ARM for
> v5.13-rc1
>
> Thanks
>
>   -- Daniel
>
>
> The following changes since commit dde8740bd9b505c58ec8b2277d5d55c6951b7e42:
>
>   Merge branch 'acpi-processor-fixes' into linux-next (2021-04-07
> 19:02:56 +0200)

Can you please rebase this on 5.12-rc6?  My linux-next branch is
re-merged on a regular basis.

Generally speaking, if you want me to pull from a branch, please make
sure that this branch is based on a commit present in the Linus' tree,
preferably one of the commits tagged as -rc or a specific merge.


Re: [PATCH -next] PM: runtime: Replace inline function pm_runtime_callbacks_present()

2021-04-08 Thread Rafael J. Wysocki
On Fri, Apr 2, 2021 at 8:14 AM YueHaibing  wrote:
>
> commit 9a7875461fd0 ("PM: runtime: Replace pm_runtime_callbacks_present()")
> forget to change the inline version.
>
> Fixes: 9a7875461fd0 ("PM: runtime: Replace pm_runtime_callbacks_present()")
> Signed-off-by: YueHaibing 
> ---
>  include/linux/pm_runtime.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index b492ae00cc90..6c08a085367b 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -265,7 +265,7 @@ static inline void pm_runtime_no_callbacks(struct device 
> *dev) {}
>  static inline void pm_runtime_irq_safe(struct device *dev) {}
>  static inline bool pm_runtime_is_irq_safe(struct device *dev) { return 
> false; }
>
> -static inline bool pm_runtime_callbacks_present(struct device *dev) { return 
> false; }
> +static inline bool pm_runtime_has_no_callbacks(struct device *dev) { return 
> false; }
>  static inline void pm_runtime_mark_last_busy(struct device *dev) {}
>  static inline void __pm_runtime_use_autosuspend(struct device *dev,
> bool use) {}
> --

Applied as 5.13 material, thanks!


Re: [PATCH -next] freezer: Remove unused inline function try_to_freeze_nowarn()

2021-04-08 Thread Rafael J. Wysocki
On Thu, Apr 1, 2021 at 7:58 PM YueHaibing  wrote:
>
> There is no caller in tree, so can remove it.
>
> Signed-off-by: YueHaibing 
> ---
>  include/linux/freezer.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index 27828145ca09..0621c5f86c39 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -279,7 +279,6 @@ static inline int freeze_kernel_threads(void) { return 
> -ENOSYS; }
>  static inline void thaw_processes(void) {}
>  static inline void thaw_kernel_threads(void) {}
>
> -static inline bool try_to_freeze_nowarn(void) { return false; }
>  static inline bool try_to_freeze(void) { return false; }
>
>  static inline void freezer_do_not_count(void) {}
> --

Applied as 5.13 material, thanks!


Re: [PATCH] linux/intel_rapl.h: Modify struct declaration

2021-04-08 Thread Rafael J. Wysocki
On Tue, Mar 30, 2021 at 8:40 AM Wan Jiabing  wrote:
>
> struct rapl_package is declared twice. One has been declared
> at 80th line.
> By reviewing the code, it should declare struct rapl_domain
> rather than rapl_package. Modify it.
>
> Signed-off-by: Wan Jiabing 
> ---
>  include/linux/intel_rapl.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/intel_rapl.h b/include/linux/intel_rapl.h
> index 50b8398ffd21..93780834fc8f 100644
> --- a/include/linux/intel_rapl.h
> +++ b/include/linux/intel_rapl.h
> @@ -33,7 +33,7 @@ enum rapl_domain_reg_id {
> RAPL_DOMAIN_REG_MAX,
>  };
>
> -struct rapl_package;
> +struct rapl_domain;
>
>  enum rapl_primitives {
> ENERGY_COUNTER,
> --

Applied as 5.13 material with edited subject and changelog, thanks!


Re: [PATCH v5 1/1] use crc32 instead of md5 for hibernation e820 integrity check

2021-04-08 Thread Rafael J. Wysocki
On Thu, Apr 8, 2021 at 5:26 PM Eric Biggers  wrote:
>
> On Thu, Apr 08, 2021 at 03:32:38PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 8, 2021 at 3:15 PM Chris von Recklinghausen
> >  wrote:
> > >
> > > Suspend fails on a system in fips mode because md5 is used for the e820
> > > integrity check and is not available. Use crc32 instead.
> > >
> > > This patch changes the integrity check algorithm from md5 to
> > > crc32. This integrity check is used only to verify accidental
> > > corruption of the hybernation data
> >
> > It isn't used for that.
> >
> > In fact, it is used to detect differences between the memory map used
> > before hibernation and the one made available by the BIOS during the
> > subsequent resume.  And the check is there, because it is generally
> > unsafe to load the hibernation image into memory if the current memory
> > map doesn't match the one used when the image was created.
>
> So what types of "differences" are you trying to detect?  If you need to 
> detect
> differences caused by someone who maliciously made changes ("malicious" 
> implies
> they may try to avoid detection), then you need to use a cryptographic hash
> function (or a cryptographic MAC if the hash value isn't stored separately).  
> If
> you only need to detect non-malicious changes (normally these would be called
> "accidental" changes, but sure, it could be changes that are "intentionally"
> made provided that the other side can be trusted to not try to avoid
> detection...)

That's the case here.

> then a non-cryptographic checksum would be sufficient.


Re: [PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails

2021-04-08 Thread Rafael J. Wysocki
On Thu, Apr 8, 2021 at 5:18 PM Andy Shevchenko
 wrote:
>
> On Thu, Apr 08, 2021 at 05:04:32PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 8, 2021 at 4:50 PM Andy Shevchenko
> >  wrote:
> > > On Thu, Apr 08, 2021 at 04:15:37PM +0200, Rafael J. Wysocki wrote:
> > > > On Wed, Mar 31, 2021 at 1:06 PM Heikki Krogerus
> > > >  wrote:
> > > > >
> > > > > On Mon, Mar 29, 2021 at 06:12:02PM +0300, Andy Shevchenko wrote:
> > > > > > Currently we have a slightly twisted logic in swnode_register().
> > > > > > It frees resources that it doesn't allocate on error path and
> > > > > > in once case it relies on the ->release() implementation.
> > > > > >
> > > > > > Untwist the logic by freeing resources explicitly when 
> > > > > > swnode_register()
> > > > > > fails. Currently it happens only in fwnode_create_software_node().
> > > > > >
> > > > > > Signed-off-by: Andy Shevchenko 
> > > > >
> > > > > It all looks OK to me. FWIW, for the whole series:
> > > > >
> > > > > Reviewed-by: Heikki Krogerus 
> > > >
> > > > Whole series applied (with some minor changelog edits) as 5.13 
> > > > material, thanks!
> > >
> > > It seems Greg applied it already. Was it dropped there?
> >
> > Did he?
> >
> > OK, so please let me know if it's still there in the Greg's tree.
>
> Here [1] what I see. Seems still there.
>
> [1]: 
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next=6e11b376fd74356e32d842be588e12dc9bf6e197

I will not be applying it then, sorry for the confusion.


Re: [PATCH v2] ACPI / hotplug / PCI: fix memory leak in enable_slot()

2021-04-08 Thread Rafael J. Wysocki
On Thu, Mar 25, 2021 at 8:27 AM Zhiqiang Liu  wrote:
>
> From: Feilong Lin 
>
> In enable_slot() in drivers/pci/hotplug/acpiphp_glue.c, if pci_get_slot()
> will return NULL, we will do not set SLOT_ENABLED flag of slot. if one
> device is found by calling pci_get_slot(), its reference count will be
> increased. In this case, we did not call pci_dev_put() to decrement the
> its reference count, the memory of the device (struct pci_dev type) will
> leak.
>
> Fix it by calling pci_dev_put() to decrement its reference count after that
> pci_get_slot() returns a PCI device.
>
> Signed-off-by: Feilong Lin 
> Signed-off-by: Zhiqiang Liu 
> --
> v2: rewrite subject and commit log as suggested by Bjorn Helgaas.

The fix is correct AFAICS, so

Reviewed-by: Rafael J. Wysocki 

Bjorn, has this been applied already?  If not, do you want me to take
it or are you going to queue it up yourself?

> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c 
> b/drivers/pci/hotplug/acpiphp_glue.c
> index 3365c93abf0e..f031302ad401 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -533,6 +533,7 @@ static void enable_slot(struct acpiphp_slot *slot, bool 
> bridge)
> slot->flags &= ~SLOT_ENABLED;
> continue;
> }
> +   pci_dev_put(dev);
> }
>  }
>
> --
> 2.19.1
>


Re: [PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails

2021-04-08 Thread Rafael J. Wysocki
On Thu, Apr 8, 2021 at 4:50 PM Andy Shevchenko
 wrote:
>
> On Thu, Apr 08, 2021 at 04:15:37PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Mar 31, 2021 at 1:06 PM Heikki Krogerus
> >  wrote:
> > >
> > > On Mon, Mar 29, 2021 at 06:12:02PM +0300, Andy Shevchenko wrote:
> > > > Currently we have a slightly twisted logic in swnode_register().
> > > > It frees resources that it doesn't allocate on error path and
> > > > in once case it relies on the ->release() implementation.
> > > >
> > > > Untwist the logic by freeing resources explicitly when swnode_register()
> > > > fails. Currently it happens only in fwnode_create_software_node().
> > > >
> > > > Signed-off-by: Andy Shevchenko 
> > >
> > > It all looks OK to me. FWIW, for the whole series:
> > >
> > > Reviewed-by: Heikki Krogerus 
> >
> > Whole series applied (with some minor changelog edits) as 5.13 material, 
> > thanks!
>
> It seems Greg applied it already. Was it dropped there?

Did he?

OK, so please let me know if it's still there in the Greg's tree.


Re: [PATCH v3 00/12] acpi: fix some coding style issues

2021-04-08 Thread Rafael J. Wysocki
On Sat, Mar 27, 2021 at 1:11 PM Xiaofei Tan  wrote:
>
> Fix some coding style issues reported by checkpatch.pl.
> Only cleanup and no function changes.
>
> Differences from v2 to v3:
> - Remove the modifications that may cause function change.
>
> Differences from v1 to v2:
> - Add subsystem and module name in the name of patch 05/15.
> - Change to use more proper module name for some patch names.
>
> Xiaofei Tan (12):
>   ACPI: APD: fix a block comment align issue
>   ACPI: processor: fix some coding style issues
>   ACPI: ipmi: remove useless return statement for void function
>   ACPI: LPSS: add a missed blank line after declarations
>   ACPI: acpi_pad: add a missed blank line after declarations
>   ACPI: battery: fix some coding style issues
>   ACPI: button: fix some coding style issues
>   ACPI: CPPC: fix some coding style issues
>   ACPI: custom_method: fix a coding style issue
>   ACPI: PM: add a missed blank line after declarations
>   ACPI: sysfs: fix some coding style issues
>   ACPI: dock: fix some coding style issues

All applied as 5.13 material, thanks!


Re: [PATCH v2] ACPI: AC: fix some coding style issues

2021-04-08 Thread Rafael J. Wysocki
On Sat, Mar 27, 2021 at 4:50 AM Xiaofei Tan  wrote:
>
> Fix some coding style issues reported by checkpatch.pl, including
> following types:
>
> ERROR: "foo * bar" should be "foo *bar"
> ERROR: code indent should use tabs where possible
> WARNING: Block comments use a trailing */ on a separate line
> WARNING: braces {} are not necessary for single statement blocks
> WARNING: void function return statements are not generally useful
> WARNING: CVS style keyword markers, these will _not_ be updated
>
> Signed-off-by: Xiaofei Tan 
> ---
>  drivers/acpi/ac.c | 28 
>  1 file changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
> index b86ee6e..b0cb662 100644
> --- a/drivers/acpi/ac.c
> +++ b/drivers/acpi/ac.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
> - *  acpi_ac.c - ACPI AC Adapter Driver ($Revision: 27 $)
> + *  acpi_ac.c - ACPI AC Adapter Driver (Revision: 27)
>   *
>   *  Copyright (C) 2001, 2002 Andy Grover 
>   *  Copyright (C) 2001, 2002 Paul Diefenbaugh 
> @@ -78,17 +78,14 @@ static struct acpi_driver acpi_ac_driver = {
>  struct acpi_ac {
> struct power_supply *charger;
> struct power_supply_desc charger_desc;
> -   struct acpi_device * device;
> +   struct acpi_device *device;
> unsigned long long state;
> struct notifier_block battery_nb;
>  };
>
>  #define to_acpi_ac(x) power_supply_get_drvdata(x)
>
> -/* --
> -   AC Adapter Management
> -   
> -- */
> -
> +/* AC Adapter Management */
>  static int acpi_ac_get_state(struct acpi_ac *ac)
>  {
> acpi_status status = AE_OK;
> @@ -109,9 +106,7 @@ static int acpi_ac_get_state(struct acpi_ac *ac)
> return 0;
>  }
>
> -/* --
> -sysfs I/F
> -   
> -- */
> +/* sysfs I/F */
>  static int get_ac_property(struct power_supply *psy,
>enum power_supply_property psp,
>union power_supply_propval *val)
> @@ -138,10 +133,7 @@ static enum power_supply_property ac_props[] = {
> POWER_SUPPLY_PROP_ONLINE,
>  };
>
> -/* --
> -   Driver Model
> -   
> -- */
> -
> +/* Driver Model */
>  static void acpi_ac_notify(struct acpi_device *device, u32 event)
>  {
> struct acpi_ac *ac = acpi_driver_data(device);
> @@ -174,8 +166,6 @@ static void acpi_ac_notify(struct acpi_device *device, 
> u32 event)
> acpi_notifier_call_chain(device, event, (u32) ac->state);
> kobject_uevent(>charger->dev.kobj, KOBJ_CHANGE);
> }
> -
> -   return;
>  }
>
>  static int acpi_ac_battery_notify(struct notifier_block *nb,
> @@ -282,9 +272,8 @@ static int acpi_ac_add(struct acpi_device *device)
> ac->battery_nb.notifier_call = acpi_ac_battery_notify;
> register_acpi_notifier(>battery_nb);
>  end:
> -   if (result) {
> +   if (result)
> kfree(ac);
> -   }
>
> return result;
>  }
> @@ -293,7 +282,7 @@ static int acpi_ac_add(struct acpi_device *device)
>  static int acpi_ac_resume(struct device *dev)
>  {
> struct acpi_ac *ac;
> -   unsigned old_state;
> +   unsigned int old_state;
>
> if (!dev)
> return -EINVAL;
> @@ -352,9 +341,8 @@ static int __init acpi_ac_init(void)
> }
>
> result = acpi_bus_register_driver(_ac_driver);
> -   if (result < 0) {
> +   if (result < 0)
> return -ENODEV;
> -   }
>
> return 0;
>  }
> --

Applied as 5.13 material, thanks!


Re: [PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails

2021-04-08 Thread Rafael J. Wysocki
On Wed, Mar 31, 2021 at 1:06 PM Heikki Krogerus
 wrote:
>
> On Mon, Mar 29, 2021 at 06:12:02PM +0300, Andy Shevchenko wrote:
> > Currently we have a slightly twisted logic in swnode_register().
> > It frees resources that it doesn't allocate on error path and
> > in once case it relies on the ->release() implementation.
> >
> > Untwist the logic by freeing resources explicitly when swnode_register()
> > fails. Currently it happens only in fwnode_create_software_node().
> >
> > Signed-off-by: Andy Shevchenko 
>
> It all looks OK to me. FWIW, for the whole series:
>
> Reviewed-by: Heikki Krogerus 

Whole series applied (with some minor changelog edits) as 5.13 material, thanks!


Re: [PATCH v5 1/1] use crc32 instead of md5 for hibernation e820 integrity check

2021-04-08 Thread Rafael J. Wysocki
On Thu, Apr 8, 2021 at 3:15 PM Chris von Recklinghausen
 wrote:
>
> Suspend fails on a system in fips mode because md5 is used for the e820
> integrity check and is not available. Use crc32 instead.
>
> This patch changes the integrity check algorithm from md5 to
> crc32. This integrity check is used only to verify accidental
> corruption of the hybernation data

It isn't used for that.

In fact, it is used to detect differences between the memory map used
before hibernation and the one made available by the BIOS during the
subsequent resume.  And the check is there, because it is generally
unsafe to load the hibernation image into memory if the current memory
map doesn't match the one used when the image was created.

> and is not intended as a cryptographic integrity check.

But this is true nevertheless, so I would write:

"The purpose of the integrity check is to detect possible differences
between the memory map used at the time when the hibernation image is
about to be loaded into memory and the memory map used at the image
creation time, because it is generally unsafe to load the image if the
current memory map doesn't match the one used when it was created. so
it is not intended as a cryptographic integrity check."

And please make the md5 spelling consistent.

> Md5 is overkill in this case and also disabled in FIPS mode because it
> is known to be broken for cryptographic purposes.
>
> Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
>by md5 digest")
>
> Tested-by: Dexuan Cui 
> Reviewed-by: Dexuan Cui 
> Signed-off-by: Chris von Recklinghausen 
> ---
> v1 -> v2
>bump up RESTORE_MAGIC
> v2 -> v3
>move embelishment from cover letter to commit comments (no code change)
> v3 -> v4
>add note to comments that md5 isn't used for encryption here.
> v4 -> v5
>reword comment per Simo's suggestion
>
>  arch/x86/power/hibernate.c | 35 +++
>  1 file changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
> index cd3914fc9f3d..b56172553275 100644
> --- a/arch/x86/power/hibernate.c
> +++ b/arch/x86/power/hibernate.c
> @@ -55,31 +55,31 @@ int pfn_is_nosave(unsigned long pfn)
>  }
>
>
> -#define MD5_DIGEST_SIZE 16
> +#define CRC32_DIGEST_SIZE 16
>
>  struct restore_data_record {
> unsigned long jump_address;
> unsigned long jump_address_phys;
> unsigned long cr3;
> unsigned long magic;
> -   u8 e820_digest[MD5_DIGEST_SIZE];
> +   u8 e820_digest[CRC32_DIGEST_SIZE];
>  };
>
> -#if IS_BUILTIN(CONFIG_CRYPTO_MD5)
> +#if IS_BUILTIN(CONFIG_CRYPTO_CRC32)
>  /**
> - * get_e820_md5 - calculate md5 according to given e820 table
> + * get_e820_crc32 - calculate crc32 according to given e820 table
>   *
>   * @table: the e820 table to be calculated
> - * @buf: the md5 result to be stored to
> + * @buf: the crc32 result to be stored to
>   */
> -static int get_e820_md5(struct e820_table *table, void *buf)
> +static int get_e820_crc32(struct e820_table *table, void *buf)
>  {
> struct crypto_shash *tfm;
> struct shash_desc *desc;
> int size;
> int ret = 0;
>
> -   tfm = crypto_alloc_shash("md5", 0, 0);
> +   tfm = crypto_alloc_shash("crc32", 0, 0);
> if (IS_ERR(tfm))
> return -ENOMEM;
>
> @@ -107,24 +107,24 @@ static int get_e820_md5(struct e820_table *table, void 
> *buf)
>
>  static int hibernation_e820_save(void *buf)
>  {
> -   return get_e820_md5(e820_table_firmware, buf);
> +   return get_e820_crc32(e820_table_firmware, buf);
>  }
>
>  static bool hibernation_e820_mismatch(void *buf)
>  {
> int ret;
> -   u8 result[MD5_DIGEST_SIZE];
> +   u8 result[CRC32_DIGEST_SIZE];
>
> -   memset(result, 0, MD5_DIGEST_SIZE);
> +   memset(result, 0, CRC32_DIGEST_SIZE);
> /* If there is no digest in suspend kernel, let it go. */
> -   if (!memcmp(result, buf, MD5_DIGEST_SIZE))
> +   if (!memcmp(result, buf, CRC32_DIGEST_SIZE))
> return false;
>
> -   ret = get_e820_md5(e820_table_firmware, result);
> +   ret = get_e820_crc32(e820_table_firmware, result);
> if (ret)
> return true;
>
> -   return memcmp(result, buf, MD5_DIGEST_SIZE) ? true : false;
> +   return memcmp(result, buf, CRC32_DIGEST_SIZE) ? true : false;
>  }
>  #else
>  static int hibernation_e820_save(void *buf)
> @@ -134,15 +134,15 @@ static int hibernation_e820_save(void *buf)
>
>  static bool hibernation_e820_mismatch(void *buf)
>  {
> -   /* If md5 is not builtin for restore kernel, let it go. */
> +   /* If crc32 is not builtin for restore kernel, let it go. */
> return false;
>  }
>  #endif
>
>  #ifdef CONFIG_X86_64
> -#define RESTORE_MAGIC  0x23456789ABCDEF01UL
> +#define RESTORE_MAGIC  0x23456789ABCDEF02UL
>  #else
> -#define RESTORE_MAGIC  0x12345678UL
> +#define RESTORE_MAGIC  0x12345679UL
>  #endif
>

Re: [PATCH v1 0/5] cpuidle: Take possible negative "sleep length" values into account

2021-04-07 Thread Rafael J. Wysocki
On Mon, Mar 29, 2021 at 8:38 PM Rafael J. Wysocki  wrote:
>
> Hi All,
>
> As follows from the discussion triggered by the patch at
>
> https://lore.kernel.org/lkml/20210311123708.23501-2-frede...@kernel.org/
>
> the cpuidle governors using tick_nohz_get_sleep_length() assume it to always
> return positive values which is not correct in general.
>
> To address this issues, first document the fact that negative values can
> be returned by tick_nohz_get_sleep_length() (patch [1/5]).  Then, in
> preparation for more substantial changes, change the data type of two
> fields in struct cpuidle_state to s64 so they can be used in computations
> involving negative numbers safely (patch [2/5]).
>
> Next, adjust the teo governor a bit so that negative "sleep length" values
> are counted like zero by it (patch [3/5]) and modify it so as to avoid
> mishandling negative "sleep length" values (patch [4/5]).
>
> Finally, make the menu governor take negative "sleep length" values into
> account properly (patch [5/5]).
>
> Please see the changelogs of the patches for details.

Given no objections or concerns regarding this lot, let me queue it up.

Thanks!


Re: [PATCH 3/7] PM: runtime: remove kernel-doc warnings

2021-04-07 Thread Rafael J. Wysocki
On Thu, Apr 1, 2021 at 4:13 PM Pierre-Louis Bossart
 wrote:
>
>
>
> On 4/1/21 8:40 AM, Rafael J. Wysocki wrote:
> > On Thu, Apr 1, 2021 at 1:26 AM Pierre-Louis Bossart
> >  wrote:
> >>
> >> remove make W=1 warnings
> >>
> >> drivers/base/power/runtime.c:926: warning: Function parameter or
> >> member 'timer' not described in 'pm_suspend_timer_fn'
> >>
> >> drivers/base/power/runtime.c:926: warning: Excess function parameter
> >> 'data' description in 'pm_suspend_timer_fn'
> >>
> >> Signed-off-by: Pierre-Louis Bossart 
> >> ---
> >>   drivers/base/power/runtime.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> >> index fe1dad68aee4..1fc1a992f90c 100644
> >> --- a/drivers/base/power/runtime.c
> >> +++ b/drivers/base/power/runtime.c
> >> @@ -951,7 +951,7 @@ static void pm_runtime_work(struct work_struct *work)
> >>
> >>   /**
> >>* pm_suspend_timer_fn - Timer function for pm_schedule_suspend().
> >> - * @data: Device pointer passed by pm_schedule_suspend().
> >> + * @timer: hrtimer used by pm_schedule_suspend().
> >>*
> >>* Check if the time is right and queue a suspend request.
> >>*/
> >> --
> >
> > I can apply this along with the [4-5/7].  Do you want me to do that?
>
> Works for me. I wasn't sure by looking at the MAINTAINERS file which
> files in drivers/base/ are maintained by whom, so sent the patches as a
> single set.

All three applied now as 5.13 material, thanks!


Re: [PATCH] include: linux: pm: Remove duplicate declaration

2021-04-07 Thread Rafael J. Wysocki
On Wed, Mar 24, 2021 at 8:30 AM Wan Jiabing  wrote:
>
> struct device is declared twice.So remove the duplicate.
>
> Signed-off-by: Wan Jiabing 
> ---
>  include/linux/pm.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 482313a8ccfc..c9657408fee1 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -39,7 +39,6 @@ static inline void pm_vt_switch_unregister(struct device 
> *dev)
>   * Device power management
>   */
>
> -struct device;
>
>  #ifdef CONFIG_PM
>  extern const char power_group_name[];  /* = "power" */
> --

Applied as 5.13 material, thanks!


Re: [PATCH v3] ACPI: processor: Fix build when CONFIG_ACPI_PROCESSOR=m

2021-04-07 Thread Rafael J. Wysocki
On Tue, Apr 6, 2021 at 5:56 PM Vitaly Kuznetsov  wrote:
>
> Commit 8c182bd7 ("ACPI: processor: Fix CPU0 wakeup in
> acpi_idle_play_dead()") tried to fix CPU0 hotplug breakage by copying
> wakeup_cpu0() + start_cpu0() logic from hlt_play_dead()//mwait_play_dead()
> into acpi_idle_play_dead(). The problem is that these functions are not
> exported to modules so when CONFIG_ACPI_PROCESSOR=m build fails.
>
> The issue could've been fixed by exporting both wakeup_cpu0()/start_cpu0()
> (the later from assembly) but it seems putting the whole pattern into a
> new function and exporting it instead is better.
>
> Reported-by: kernel test robot 
> Fixes: 8c182bd7 ("CPI: processor: Fix CPU0 wakeup in 
> acpi_idle_play_dead()")
> Cc:  # 5.10+
> Signed-off-by: Vitaly Kuznetsov 

Applied as 5.12-rc material, thanks!

> ---
> Changes since v2:
> - Use proper kerneldoc format [Rafael J. Wysocki]
> ---
>  arch/x86/include/asm/smp.h|  2 +-
>  arch/x86/kernel/smpboot.c | 26 --
>  drivers/acpi/processor_idle.c |  4 +---
>  3 files changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index 57ef2094af93..630ff08532be 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -132,7 +132,7 @@ void native_play_dead(void);
>  void play_dead_common(void);
>  void wbinvd_on_cpu(int cpu);
>  int wbinvd_on_all_cpus(void);
> -bool wakeup_cpu0(void);
> +void cond_wakeup_cpu0(void);
>
>  void native_smp_send_reschedule(int cpu);
>  void native_send_call_func_ipi(const struct cpumask *mask);
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index f877150a91da..16703c35a944 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1659,13 +1659,17 @@ void play_dead_common(void)
> local_irq_disable();
>  }
>
> -bool wakeup_cpu0(void)
> +/**
> + * cond_wakeup_cpu0 - Wake up CPU0 if needed.
> + *
> + * If NMI wants to wake up CPU0, start CPU0.
> + */
> +void cond_wakeup_cpu0(void)
>  {
> if (smp_processor_id() == 0 && enable_start_cpu0)
> -   return true;
> -
> -   return false;
> +   start_cpu0();
>  }
> +EXPORT_SYMBOL_GPL(cond_wakeup_cpu0);
>
>  /*
>   * We need to flush the caches before going to sleep, lest we have
> @@ -1734,11 +1738,8 @@ static inline void mwait_play_dead(void)
> __monitor(mwait_ptr, 0, 0);
> mb();
> __mwait(eax, 0);
> -   /*
> -* If NMI wants to wake up CPU0, start CPU0.
> -*/
> -   if (wakeup_cpu0())
> -   start_cpu0();
> +
> +   cond_wakeup_cpu0();
> }
>  }
>
> @@ -1749,11 +1750,8 @@ void hlt_play_dead(void)
>
> while (1) {
> native_halt();
> -   /*
> -* If NMI wants to wake up CPU0, start CPU0.
> -*/
> -   if (wakeup_cpu0())
> -   start_cpu0();
> +
> +   cond_wakeup_cpu0();
> }
>  }
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 768a6b4d2368..4e2d76b8b697 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -544,9 +544,7 @@ static int acpi_idle_play_dead(struct cpuidle_device 
> *dev, int index)
> return -ENODEV;
>
>  #if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU)
> -   /* If NMI wants to wake up CPU0, start CPU0. */
> -   if (wakeup_cpu0())
> -   start_cpu0();
> +   cond_wakeup_cpu0();
>  #endif
> }
>
> --
> 2.30.2
>


[PATCH v1 0/5] ACPI: scan: acpi_bus_check_add() simplifications

2021-04-07 Thread Rafael J. Wysocki
Hi,

This series simplifies acpi_bus_check_add() and related code.

It mostly is not expected to alter functionality, except for patch [4/5] that
unifies the handling of device and processor objects.

Please refer to the patch changelogs for details.

Thanks!





[PATCH v1 1/5] ACPI: scan: Fold acpi_bus_type_and_status() into its caller

2021-04-07 Thread Rafael J. Wysocki
From: Rafael J. Wysocki 

There is only one caller of acpi_bus_type_and_status() which is
acpi_bus_check_add(), so fold the former into the latter and use
the observation that the initial status of the device is
ACPI_STA_DEFAULT in all cases except for ACPI_BUS_TYPE_PROCESSOR
to simplify the code.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki 
---
 drivers/acpi/scan.c |   80 
 1 file changed, 32 insertions(+), 48 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1763,50 +1763,6 @@ static bool acpi_device_should_be_hidden
return true;
 }
 
-static int acpi_bus_type_and_status(acpi_handle handle, int *type,
-   unsigned long long *sta)
-{
-   acpi_status status;
-   acpi_object_type acpi_type;
-
-   status = acpi_get_type(handle, _type);
-   if (ACPI_FAILURE(status))
-   return -ENODEV;
-
-   switch (acpi_type) {
-   case ACPI_TYPE_ANY: /* for ACPI_ROOT_OBJECT */
-   case ACPI_TYPE_DEVICE:
-   if (acpi_device_should_be_hidden(handle))
-   return -ENODEV;
-
-   *type = ACPI_BUS_TYPE_DEVICE;
-   /*
-* acpi_add_single_object updates this once we've an acpi_device
-* so that acpi_bus_get_status' quirk handling can be used.
-*/
-   *sta = ACPI_STA_DEFAULT;
-   break;
-   case ACPI_TYPE_PROCESSOR:
-   *type = ACPI_BUS_TYPE_PROCESSOR;
-   status = acpi_bus_get_status_handle(handle, sta);
-   if (ACPI_FAILURE(status))
-   return -ENODEV;
-   break;
-   case ACPI_TYPE_THERMAL:
-   *type = ACPI_BUS_TYPE_THERMAL;
-   *sta = ACPI_STA_DEFAULT;
-   break;
-   case ACPI_TYPE_POWER:
-   *type = ACPI_BUS_TYPE_POWER;
-   *sta = ACPI_STA_DEFAULT;
-   break;
-   default:
-   return -ENODEV;
-   }
-
-   return 0;
-}
-
 bool acpi_device_is_present(const struct acpi_device *adev)
 {
return adev->status.present || adev->status.functional;
@@ -1953,18 +1909,46 @@ static acpi_status acpi_bus_check_add(ac
  struct acpi_device **adev_p)
 {
struct acpi_device *device = NULL;
-   unsigned long long sta;
+   unsigned long long sta = ACPI_STA_DEFAULT;
+   acpi_object_type acpi_type;
int type;
-   int result;
 
acpi_bus_get_device(handle, );
if (device)
goto out;
 
-   result = acpi_bus_type_and_status(handle, , );
-   if (result)
+   if (ACPI_FAILURE(acpi_get_type(handle, _type)))
return AE_OK;
 
+   switch (acpi_type) {
+   case ACPI_TYPE_DEVICE:
+   if (acpi_device_should_be_hidden(handle))
+   return AE_OK;
+
+   fallthrough;
+   case ACPI_TYPE_ANY: /* for ACPI_ROOT_OBJECT */
+   type = ACPI_BUS_TYPE_DEVICE;
+   break;
+
+   case ACPI_TYPE_PROCESSOR:
+   if (ACPI_FAILURE(acpi_bus_get_status_handle(handle, )))
+   return AE_OK;
+
+   type = ACPI_BUS_TYPE_PROCESSOR;
+   break;
+
+   case ACPI_TYPE_THERMAL:
+   type = ACPI_BUS_TYPE_THERMAL;
+   break;
+
+   case ACPI_TYPE_POWER:
+   type = ACPI_BUS_TYPE_POWER;
+   break;
+
+   default:
+   return AE_OK;
+   }
+
if (type == ACPI_BUS_TYPE_POWER) {
acpi_add_power_resource(handle);
return AE_OK;





[PATCH v1 2/5] ACPI: scan: Rearrange checks in acpi_bus_check_add()

2021-04-07 Thread Rafael J. Wysocki
From: Rafael J. Wysocki 

Rearrange the checks in acpi_bus_check_add() to avoid checking
the "type" twice and take "check_dep" into account only for
ACPI_TYPE_DEVICE objects.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki 
---
 drivers/acpi/scan.c |   30 +++---
 1 file changed, 11 insertions(+), 19 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1831,7 +1831,7 @@ static void acpi_scan_init_hotplug(struc
}
 }
 
-static u32 acpi_scan_check_dep(acpi_handle handle)
+static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
 {
struct acpi_handle_list dep_devices;
acpi_status status;
@@ -1844,7 +1844,8 @@ static u32 acpi_scan_check_dep(acpi_hand
 * 2. ACPI nodes describing USB ports.
 * Still, checking for _HID catches more then just these cases ...
 */
-   if (!acpi_has_method(handle, "_DEP") || !acpi_has_method(handle, 
"_HID"))
+   if (!check_dep || !acpi_has_method(handle, "_DEP") ||
+   !acpi_has_method(handle, "_HID"))
return 0;
 
status = acpi_evaluate_reference(handle, "_DEP", NULL, _devices);
@@ -1925,6 +1926,12 @@ static acpi_status acpi_bus_check_add(ac
if (acpi_device_should_be_hidden(handle))
return AE_OK;
 
+   /* Bail out if there are dependencies. */
+   if (acpi_scan_check_dep(handle, check_dep) > 0) {
+   acpi_bus_scan_second_pass = true;
+   return AE_CTRL_DEPTH;
+   }
+
fallthrough;
case ACPI_TYPE_ANY: /* for ACPI_ROOT_OBJECT */
type = ACPI_BUS_TYPE_DEVICE;
@@ -1942,27 +1949,12 @@ static acpi_status acpi_bus_check_add(ac
break;
 
case ACPI_TYPE_POWER:
-   type = ACPI_BUS_TYPE_POWER;
-   break;
-
-   default:
-   return AE_OK;
-   }
-
-   if (type == ACPI_BUS_TYPE_POWER) {
acpi_add_power_resource(handle);
+   fallthrough;
+   default:
return AE_OK;
}
 
-   if (type == ACPI_BUS_TYPE_DEVICE && check_dep) {
-   u32 count = acpi_scan_check_dep(handle);
-   /* Bail out if the number of recorded dependencies is not 0. */
-   if (count > 0) {
-   acpi_bus_scan_second_pass = true;
-   return AE_CTRL_DEPTH;
-   }
-   }
-
acpi_add_single_object(, handle, type, sta);
if (!device)
return AE_CTRL_DEPTH;





[PATCH v1 3/5] ACPI: scan: Drop sta argument from acpi_add_single_object()

2021-04-07 Thread Rafael J. Wysocki
From: Rafael J. Wysocki 

Move the initial status check for ACPI_BUS_TYPE_PROCESSOR objects
into acpi_add_single_object() so it is not necessary to pass the
"sta" argument to it, get rid of that argument from there and update
the callers of that function accordingly.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki 
---
 drivers/acpi/scan.c |   24 ++--
 1 file changed, 10 insertions(+), 14 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1681,15 +1681,18 @@ void acpi_device_add_finalize(struct acp
 }
 
 static int acpi_add_single_object(struct acpi_device **child,
- acpi_handle handle, int type,
- unsigned long long sta)
+ acpi_handle handle, int type)
 {
struct acpi_device_info *info = NULL;
+   unsigned long long sta = ACPI_STA_DEFAULT;
struct acpi_device *device;
int result;
 
-   if (handle != ACPI_ROOT_OBJECT && type == ACPI_BUS_TYPE_DEVICE)
+   if (type == ACPI_BUS_TYPE_DEVICE && handle != ACPI_ROOT_OBJECT)
acpi_get_object_info(handle, );
+   else if (type == ACPI_BUS_TYPE_PROCESSOR &&
+ACPI_FAILURE(acpi_bus_get_status_handle(handle, )))
+   return -ENODEV;
 
device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
if (!device) {
@@ -1910,7 +1913,6 @@ static acpi_status acpi_bus_check_add(ac
  struct acpi_device **adev_p)
 {
struct acpi_device *device = NULL;
-   unsigned long long sta = ACPI_STA_DEFAULT;
acpi_object_type acpi_type;
int type;
 
@@ -1938,9 +1940,6 @@ static acpi_status acpi_bus_check_add(ac
break;
 
case ACPI_TYPE_PROCESSOR:
-   if (ACPI_FAILURE(acpi_bus_get_status_handle(handle, )))
-   return AE_OK;
-
type = ACPI_BUS_TYPE_PROCESSOR;
break;
 
@@ -1955,7 +1954,7 @@ static acpi_status acpi_bus_check_add(ac
return AE_OK;
}
 
-   acpi_add_single_object(, handle, type, sta);
+   acpi_add_single_object(, handle, type);
if (!device)
return AE_CTRL_DEPTH;
 
@@ -2229,8 +2228,7 @@ int acpi_bus_register_early_device(int t
struct acpi_device *device = NULL;
int result;
 
-   result = acpi_add_single_object(, NULL,
-   type, ACPI_STA_DEFAULT);
+   result = acpi_add_single_object(, NULL, type);
if (result)
return result;
 
@@ -2250,8 +2248,7 @@ static int acpi_bus_scan_fixed(void)
struct acpi_device *device = NULL;
 
result = acpi_add_single_object(, NULL,
-   ACPI_BUS_TYPE_POWER_BUTTON,
-   ACPI_STA_DEFAULT);
+   ACPI_BUS_TYPE_POWER_BUTTON);
if (result)
return result;
 
@@ -2267,8 +2264,7 @@ static int acpi_bus_scan_fixed(void)
struct acpi_device *device = NULL;
 
result = acpi_add_single_object(, NULL,
-   ACPI_BUS_TYPE_SLEEP_BUTTON,
-   ACPI_STA_DEFAULT);
+   ACPI_BUS_TYPE_SLEEP_BUTTON);
if (result)
return result;
 





[PATCH v1 4/5] ACPI: scan: Drop sta argument from acpi_init_device_object()

2021-04-07 Thread Rafael J. Wysocki
From: Rafael J. Wysocki 

Use the observation that the initial status check for
ACPI_BUS_TYPE_PROCESSOR objects can be carried out in the same way
as for ACPI_BUS_TYPE_DEVICE objects and it is not necessary to fail
acpi_add_single_object() if acpi_bus_get_status_handle() returns an
error for a processor (its status can be set to 0 instead) to
simplify acpi_add_single_object().

Accordingly, drop the "sta" argument from acpi_init_device_object()
as it can always set the initial status to ACPI_STA_DEFAULT and let
its caller correct that later on.

Signed-off-by: Rafael J. Wysocki 
---
 drivers/acpi/internal.h |3 +--
 drivers/acpi/power.c|3 +--
 drivers/acpi/scan.c |   28 ++--
 3 files changed, 16 insertions(+), 18 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1649,15 +1649,14 @@ static bool acpi_device_enumeration_by_p
 }
 
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
-int type, unsigned long long sta,
-struct acpi_device_info *info)
+int type, struct acpi_device_info *info)
 {
INIT_LIST_HEAD(>pnp.ids);
device->device_type = type;
device->handle = handle;
device->parent = acpi_bus_get_parent(handle);
fwnode_init(>fwnode, _device_fwnode_ops);
-   acpi_set_device_status(device, sta);
+   acpi_set_device_status(device, ACPI_STA_DEFAULT);
acpi_device_get_busid(device);
acpi_set_pnp_ids(handle, >pnp, type, info);
acpi_init_properties(device);
@@ -1680,19 +1679,21 @@ void acpi_device_add_finalize(struct acp
kobject_uevent(>dev.kobj, KOBJ_ADD);
 }
 
+static void acpi_scan_init_status(struct acpi_device *adev)
+{
+   if (acpi_bus_get_status(adev))
+   acpi_set_device_status(adev, 0);
+}
+
 static int acpi_add_single_object(struct acpi_device **child,
  acpi_handle handle, int type)
 {
struct acpi_device_info *info = NULL;
-   unsigned long long sta = ACPI_STA_DEFAULT;
struct acpi_device *device;
int result;
 
if (type == ACPI_BUS_TYPE_DEVICE && handle != ACPI_ROOT_OBJECT)
acpi_get_object_info(handle, );
-   else if (type == ACPI_BUS_TYPE_PROCESSOR &&
-ACPI_FAILURE(acpi_bus_get_status_handle(handle, )))
-   return -ENODEV;
 
device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
if (!device) {
@@ -1700,16 +1701,15 @@ static int acpi_add_single_object(struct
return -ENOMEM;
}
 
-   acpi_init_device_object(device, handle, type, sta, info);
+   acpi_init_device_object(device, handle, type, info);
kfree(info);
/*
-* For ACPI_BUS_TYPE_DEVICE getting the status is delayed till here so
-* that we can call acpi_bus_get_status() and use its quirk handling.
-* Note this must be done before the get power-/wakeup_dev-flags calls.
+* Getting the status is delayed till here so that we can call
+* acpi_bus_get_status() and use its quirk handling.  Note that
+* this must be done before the get power-/wakeup_dev-flags calls.
 */
-   if (type == ACPI_BUS_TYPE_DEVICE)
-   if (acpi_bus_get_status(device) < 0)
-   acpi_set_device_status(device, 0);
+   if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR)
+   acpi_scan_init_status(device);
 
acpi_bus_get_power_flags(device);
acpi_bus_get_wakeup_device_flags(device);
Index: linux-pm/drivers/acpi/internal.h
===
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -109,8 +109,7 @@ struct acpi_device_bus_id {
 int acpi_device_add(struct acpi_device *device,
void (*release)(struct device *));
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
-int type, unsigned long long sta,
-struct acpi_device_info *info);
+int type, struct acpi_device_info *info);
 int acpi_device_setup_files(struct acpi_device *dev);
 void acpi_device_remove_files(struct acpi_device *dev);
 void acpi_device_add_finalize(struct acpi_device *device);
Index: linux-pm/drivers/acpi/power.c
===
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -925,8 +925,7 @@ int acpi_add_power_resource(acpi_handle
return -ENOMEM;
 
device = >device;
-   acpi_init_device_object(device, handle, ACPI_BUS_TYPE_POWER,
-

[PATCH v1 5/5] ACPI: scan: Call acpi_get_object_info() from acpi_set_pnp_ids()

2021-04-07 Thread Rafael J. Wysocki
From: Rafael J. Wysocki 

Notice that it is not necessary to call acpi_get_object_info() from
acpi_add_single_object() in order to pass the pointer returned by it
to acpi_init_device_object() and from there to acpi_set_pnp_ids().

It is more straightforward to call acpi_get_object_info() from
acpi_set_pnp_ids() and avoid unnecessary pointer passing, so change
the code accordingly.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki 
---
 drivers/acpi/internal.h |2 +-
 drivers/acpi/power.c|2 +-
 drivers/acpi/scan.c |   21 +
 3 files changed, 11 insertions(+), 14 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1307,8 +1307,9 @@ static bool acpi_object_is_system_bus(ac
 }
 
 static void acpi_set_pnp_ids(acpi_handle handle, struct acpi_device_pnp *pnp,
-   int device_type, struct acpi_device_info *info)
+int device_type)
 {
+   struct acpi_device_info *info = NULL;
struct acpi_pnp_device_id_list *cid_list;
int i;
 
@@ -1319,6 +1320,7 @@ static void acpi_set_pnp_ids(acpi_handle
break;
}
 
+   acpi_get_object_info(handle, );
if (!info) {
pr_err(PREFIX "%s: Error reading device info\n",
__func__);
@@ -1344,6 +1346,8 @@ static void acpi_set_pnp_ids(acpi_handle
if (info->valid & ACPI_VALID_CLS)
acpi_add_id(pnp, info->class_code.string);
 
+   kfree(info);
+
/*
 * Some devices don't reliably have _HIDs & _CIDs, so add
 * synthetic HIDs to make sure drivers can find them.
@@ -1649,7 +1653,7 @@ static bool acpi_device_enumeration_by_p
 }
 
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
-int type, struct acpi_device_info *info)
+int type)
 {
INIT_LIST_HEAD(>pnp.ids);
device->device_type = type;
@@ -1658,7 +1662,7 @@ void acpi_init_device_object(struct acpi
fwnode_init(>fwnode, _device_fwnode_ops);
acpi_set_device_status(device, ACPI_STA_DEFAULT);
acpi_device_get_busid(device);
-   acpi_set_pnp_ids(handle, >pnp, type, info);
+   acpi_set_pnp_ids(handle, >pnp, type);
acpi_init_properties(device);
acpi_bus_get_flags(device);
device->flags.match_driver = false;
@@ -1688,21 +1692,14 @@ static void acpi_scan_init_status(struct
 static int acpi_add_single_object(struct acpi_device **child,
  acpi_handle handle, int type)
 {
-   struct acpi_device_info *info = NULL;
struct acpi_device *device;
int result;
 
-   if (type == ACPI_BUS_TYPE_DEVICE && handle != ACPI_ROOT_OBJECT)
-   acpi_get_object_info(handle, );
-
device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
-   if (!device) {
-   kfree(info);
+   if (!device)
return -ENOMEM;
-   }
 
-   acpi_init_device_object(device, handle, type, info);
-   kfree(info);
+   acpi_init_device_object(device, handle, type);
/*
 * Getting the status is delayed till here so that we can call
 * acpi_bus_get_status() and use its quirk handling.  Note that
Index: linux-pm/drivers/acpi/internal.h
===
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -109,7 +109,7 @@ struct acpi_device_bus_id {
 int acpi_device_add(struct acpi_device *device,
void (*release)(struct device *));
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
-int type, struct acpi_device_info *info);
+int type);
 int acpi_device_setup_files(struct acpi_device *dev);
 void acpi_device_remove_files(struct acpi_device *dev);
 void acpi_device_add_finalize(struct acpi_device *device);
Index: linux-pm/drivers/acpi/power.c
===
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -925,7 +925,7 @@ int acpi_add_power_resource(acpi_handle
return -ENOMEM;
 
device = >device;
-   acpi_init_device_object(device, handle, ACPI_BUS_TYPE_POWER, NULL);
+   acpi_init_device_object(device, handle, ACPI_BUS_TYPE_POWER);
mutex_init(>resource_lock);
INIT_LIST_HEAD(>list_node);
INIT_LIST_HEAD(>dependents);





[PATCH] cpufreq: intel_pstate: Simplify intel_pstate_update_perf_limits()

2021-04-07 Thread Rafael J. Wysocki
From: Rafael J. Wysocki 

Because pstate.max_freq is always equal to the product of
pstate.max_pstate and pstate.scaling and, analogously,
pstate.turbo_freq is always equal to the product of
pstate.turbo_pstate and pstate.scaling, the result of the
max_policy_perf computation in intel_pstate_update_perf_limits() is
always equal to the quotient of policy_max and pstate.scaling,
regardless of whether or not turbo is disabled.  Analogously, the
result of min_policy_perf in intel_pstate_update_perf_limits() is
always equal to the quotient of policy_min and pstate.scaling.

Accordingly, intel_pstate_update_perf_limits() need not check
whether or not turbo is enabled at all and in order to compute
max_policy_perf and min_policy_perf it can always divide policy_max
and policy_min, respectively, by pstate.scaling.  Make it do so.

While at it, move the definition and initialization of the
turbo_max local variable to the code branch using it.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki 
---

On top of linux-next.

---
 drivers/cpufreq/intel_pstate.c |   22 ++
 1 file changed, 6 insertions(+), 16 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -2195,9 +2195,8 @@ static void intel_pstate_update_perf_lim
unsigned int policy_min,
unsigned int policy_max)
 {
+   int scaling = cpu->pstate.scaling;
int32_t max_policy_perf, min_policy_perf;
-   int max_state, turbo_max;
-   int max_freq;
 
/*
 * HWP needs some special consideration, because HWP_REQUEST uses
@@ -2206,33 +2205,24 @@ static void intel_pstate_update_perf_lim
if (hwp_active)
intel_pstate_get_hwp_cap(cpu);
 
-   if (global.no_turbo || global.turbo_disabled) {
-   max_state = cpu->pstate.max_pstate;
-   max_freq = cpu->pstate.max_freq;
-   } else {
-   max_state = cpu->pstate.turbo_pstate;
-   max_freq = cpu->pstate.turbo_freq;
-   }
-
-   turbo_max = cpu->pstate.turbo_pstate;
-
-   max_policy_perf = max_state * policy_max / max_freq;
+   max_policy_perf = policy_max / scaling;
if (policy_max == policy_min) {
min_policy_perf = max_policy_perf;
} else {
-   min_policy_perf = max_state * policy_min / max_freq;
+   min_policy_perf = policy_min / scaling;
min_policy_perf = clamp_t(int32_t, min_policy_perf,
  0, max_policy_perf);
}
 
-   pr_debug("cpu:%d max_state %d min_policy_perf:%d max_policy_perf:%d\n",
-cpu->cpu, max_state, min_policy_perf, max_policy_perf);
+   pr_debug("cpu:%d min_policy_perf:%d max_policy_perf:%d\n",
+cpu->cpu, min_policy_perf, max_policy_perf);
 
/* Normalize user input to [min_perf, max_perf] */
if (per_cpu_limits) {
cpu->min_perf_ratio = min_policy_perf;
cpu->max_perf_ratio = max_policy_perf;
} else {
+   int turbo_max = cpu->pstate.turbo_pstate;
int32_t global_min, global_max;
 
/* Global limits are in percent of the maximum turbo P-state. */





Re: [PATCH] resource: Prevent irqresource_disabled() from erasing flags

2021-04-07 Thread Rafael J. Wysocki
On Wed, Apr 7, 2021 at 1:01 PM Angela Czubak  wrote:
>
> On Tue, Mar 30, 2021 at 5:50 PM Mika Westerberg
>  wrote:
> >
> > Hi,
> >
> > On Tue, Mar 30, 2021 at 05:09:42PM +0200, Rafael J. Wysocki wrote:
> > > On 3/29/2021 9:52 PM, Angela Czubak wrote:
> > > > Do not overwrite flags as it leads to erasing triggering and polarity
> > > > information which might be useful in case of hard-coded interrupts.
> > > > This way the information can be read later on even though mapping to
> > > > APIC domain failed.
> > > >
> > > > Signed-off-by: Angela Czubak 
> > > > ---
> > > > Some Chromebooks use hard-coded interrupts in their ACPI tables.
> > > > This is an excerpt as dumped on Relm:
> > > >
> > > > ...
> > > >  Name (_HID, "ELAN0001")  // _HID: Hardware ID
> > > >  Name (_DDN, "Elan Touchscreen ")  // _DDN: DOS Device Name
> > > >  Name (_UID, 0x05)  // _UID: Unique ID
> > > >  Name (ISTP, Zero)
> > > >  Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource 
> > > > Settings
> > > >  {
> > > >  Name (BUF0, ResourceTemplate ()
> > > >  {
> > > >  I2cSerialBusV2 (0x0010, ControllerInitiated, 
> > > > 0x00061A80,
> > > >  AddressingMode7Bit, "\\_SB.I2C1",
> > > >  0x00, ResourceConsumer, , Exclusive,
> > > >  )
> > > >  Interrupt (ResourceConsumer, Edge, ActiveLow, 
> > > > Exclusive, ,, )
> > > >  {
> > > >  0x00B8,
> > > >  }
> > > >  })
> > > >  Return (BUF0) /* \_SB_.I2C1.ETSA._CRS.BUF0 */
> > > >  }
> > > > ...
> > > >
> > > > This interrupt is hard-coded to 0xB8 = 184 which is too high to be 
> > > > mapped
> > > > to IO-APIC, so no triggering information is propagated as 
> > > > acpi_register_gsi()
> > > > fails and irqresource_disabled() is issued, which leads to erasing 
> > > > triggering
> > > > and polarity information.
> > > > If that function added its flags instead of overwriting them the 
> > > > correct IRQ
> > > > type would be set even for the hard-coded interrupts, which allows 
> > > > device driver
> > > > to retrieve it.
> > > > Please, let me know if this kind of modification is acceptable.
> > >
> > > From the quick look it should not be problematic, but it needs to be 
> > > checked
> > > more carefully.
> > >
> > > Mika, what do you think?
> >
> > I think it makes sense. We still set IORESOURCE_DISABLED unconditionally
> > so this should not cause issues. In theory at least :)
> >
> Is there anything else you would need me to do regarding the patch?

Yes, please resend it with a CC to linux-a...@vger.kernel.org for more
visibility.

> I suppose there are more platforms that could benefit from not erasing
> the flags, so if this patch is fit for upstream, can we continue the
> process?
>
> > > >   include/linux/ioport.h | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> > > > index 55de385c839cf..647744d8514e0 100644
> > > > --- a/include/linux/ioport.h
> > > > +++ b/include/linux/ioport.h
> > > > @@ -331,7 +331,7 @@ static inline void irqresource_disabled(struct 
> > > > resource *res, u32 irq)
> > > >   {
> > > > res->start = irq;
> > > > res->end = irq;
> > > > -   res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED | 
> > > > IORESOURCE_UNSET;
> > > > +   res->flags |= IORESOURCE_IRQ | IORESOURCE_DISABLED | 
> > > > IORESOURCE_UNSET;
> > > >   }
> > > >   extern struct address_space *iomem_get_mapping(void);
> > >


Re: [bug report] Memory leak from acpi_ev_install_space_handler()

2021-04-06 Thread Rafael J. Wysocki
On Tue, Apr 6, 2021 at 5:51 PM John Garry  wrote:
>
> Hi guys,
>
> On next-20210406, I enabled CONFIG_DEBUG_KMEMLEAK and
> CONFIG_DEBUG_TEST_DRIVER_REMOVE for my arm64 system, and see this:

Why exactly do you think that acpi_ev_install_space_handler() leaks memory?

> root@debian:/home/john# more /sys/kernel/debug/kmemleak
> unreferenced object 0x202803c11f00 (size 128):
> comm "swapper/0", pid 1, jiffies 4294894325 (age 337.524s)
> hex dump (first 32 bytes):
> 00 00 00 00 02 00 00 00 08 1f c1 03 28 20 ff ff( ..
> 08 1f c1 03 28 20 ff ff 00 00 00 00 00 00 00 00( ..
> backtrace:
> [<670a0938>] slab_post_alloc_hook+0x9c/0x2f8
> [] kmem_cache_alloc+0x198/0x2a8
> [<2bdba864>] acpi_os_create_semaphore+0x54/0xe0
> [] acpi_ev_install_space_handler+0x24c/0x300
> [<02e116e2>] acpi_install_address_space_handler+0x64/0xb0
> [] i2c_acpi_install_space_handler+0xd4/0x138
> [<8da42058>] i2c_register_adapter+0x368/0x910
> [] i2c_add_adapter+0x9c/0x100
> [<00ba2fcf>] i2c_add_numbered_adapter+0x44/0x58
> [<7df22d67>] i2c_dw_probe_master+0x68c/0x900
> [<682dfc98>] dw_i2c_plat_probe+0x460/0x640
> [] platform_probe+0x8c/0x108
> [] really_probe+0x190/0x670
> [<66017341>] driver_probe_device+0x8c/0xf8
> [] device_driver_attach+0x9c/0xa8
> [] __driver_attach+0x88/0x138
> unreferenced object 0x00280452c100 (size 128):
> comm "swapper/0", pid 1, jiffies 4294894558 (age 336.604s)
> hex dump (first 32 bytes):
> 00 00 00 00 02 00 00 00 08 c1 52 04 28 00 ff ff..R.(...
> 08 c1 52 04 28 00 ff ff 00 00 00 00 00 00 00 00..R.(...
> backtrace:
> [<670a0938>] slab_post_alloc_hook+0x9c/0x2f8
> [] kmem_cache_alloc+0x198/0x2a8
> [<2bdba864>] acpi_os_create_semaphore+0x54/0xe0
> [] acpi_ev_install_space_handler+0x24c/0x300
> [<02e116e2>] acpi_install_address_space_handler+0x64/0xb0
> [<988d4f61>] acpi_gpiochip_add+0x20c/0x4a0
> [<73d4faab>] gpiochip_add_data_with_key+0xd10/0x1680
> [<1d50b98a>] devm_gpiochip_add_data_with_key+0x30/0x78
> [] dwapb_gpio_probe+0x828/0xb28
> [] platform_probe+0x8c/0x108
> [] really_probe+0x190/0x670
> [<66017341>] driver_probe_device+0x8c/0xf8
> [] device_driver_attach+0x9c/0xa8
> [] __driver_attach+0x88/0x138
> [] bus_for_each_dev+0xec/0x160
> [] driver_attach+0x34/0x48
> root@debian:/home/john#
>
> Thanks,
> John


Re: [PATCH v2] ACPI: processor: Fix build when CONFIG_ACPI_PROCESSOR=m

2021-04-06 Thread Rafael J. Wysocki
On Tue, Apr 6, 2021 at 5:39 PM Vitaly Kuznetsov  wrote:
>
> "Rafael J. Wysocki"  writes:
>
> > On Tue, Apr 6, 2021 at 4:01 PM Vitaly Kuznetsov  wrote:
> >>
> >> Commit 8c182bd7 ("ACPI: processor: Fix CPU0 wakeup in
> >> acpi_idle_play_dead()") tried to fix CPU0 hotplug breakage by copying
> >> wakeup_cpu0() + start_cpu0() logic from hlt_play_dead()//mwait_play_dead()
> >> into acpi_idle_play_dead(). The problem is that these functions are not
> >> exported to modules so when CONFIG_ACPI_PROCESSOR=m build fails.
> >>
> >> The issue could've been fixed by exporting both wakeup_cpu0()/start_cpu0()
> >> (the later from assembly) but it seems putting the whole pattern into a
> >> new function and exporting it instead is better.
> >>
> >> Reported-by: kernel test robot 
> >> Fixes: 8c182bd7 ("CPI: processor: Fix CPU0 wakeup in 
> >> acpi_idle_play_dead()")
> >> Cc:  # 5.10+
> >> Signed-off-by: Vitaly Kuznetsov 
> >> ---
> >> Changes since v1:
> >> - Rename wakeup_cpu0() to cond_wakeup_cpu0() and fold wakeup_cpu0() in
> >>  as it has no other users [Rafael J. Wysocki]
> >> ---
> >>  arch/x86/include/asm/smp.h|  2 +-
> >>  arch/x86/kernel/smpboot.c | 24 ++--
> >>  drivers/acpi/processor_idle.c |  4 +---
> >>  3 files changed, 12 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> >> index 57ef2094af93..630ff08532be 100644
> >> --- a/arch/x86/include/asm/smp.h
> >> +++ b/arch/x86/include/asm/smp.h
> >> @@ -132,7 +132,7 @@ void native_play_dead(void);
> >>  void play_dead_common(void);
> >>  void wbinvd_on_cpu(int cpu);
> >>  int wbinvd_on_all_cpus(void);
> >> -bool wakeup_cpu0(void);
> >> +void cond_wakeup_cpu0(void);
> >>
> >>  void native_smp_send_reschedule(int cpu);
> >>  void native_send_call_func_ipi(const struct cpumask *mask);
> >> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> >> index f877150a91da..147f1bba9736 100644
> >> --- a/arch/x86/kernel/smpboot.c
> >> +++ b/arch/x86/kernel/smpboot.c
> >> @@ -1659,13 +1659,15 @@ void play_dead_common(void)
> >> local_irq_disable();
> >>  }
> >>
> >> -bool wakeup_cpu0(void)
> >> +/*
> >> + * If NMI wants to wake up CPU0, start CPU0.
> >> + */
> >
> > Hasn't checkpatch.pl complained about this?
> >
>
> No, it didn't.
>
> > A proper kerneldoc would be something like:
> >
> > /**
> >  * cond_wakeup_cpu0 - Wake up CPU0 if needed.
> >  *
> >  * If NMI wants to wake up CPU0, start CPU0.
> >  */
>
> Yea, I didn't do that partly because of my laziness but partly because
> I don't see much usage of this format in arch/x86/kernel/[smpboot.c]. I
> can certainly do v3 if it's prefered.

Yes, please.

Exported functions generally need proper kerneldoc comments.


Re: [PATCH v2] ACPI: processor: Fix build when CONFIG_ACPI_PROCESSOR=m

2021-04-06 Thread Rafael J. Wysocki
On Tue, Apr 6, 2021 at 4:01 PM Vitaly Kuznetsov  wrote:
>
> Commit 8c182bd7 ("ACPI: processor: Fix CPU0 wakeup in
> acpi_idle_play_dead()") tried to fix CPU0 hotplug breakage by copying
> wakeup_cpu0() + start_cpu0() logic from hlt_play_dead()//mwait_play_dead()
> into acpi_idle_play_dead(). The problem is that these functions are not
> exported to modules so when CONFIG_ACPI_PROCESSOR=m build fails.
>
> The issue could've been fixed by exporting both wakeup_cpu0()/start_cpu0()
> (the later from assembly) but it seems putting the whole pattern into a
> new function and exporting it instead is better.
>
> Reported-by: kernel test robot 
> Fixes: 8c182bd7 ("CPI: processor: Fix CPU0 wakeup in 
> acpi_idle_play_dead()")
> Cc:  # 5.10+
> Signed-off-by: Vitaly Kuznetsov 
> ---
> Changes since v1:
> - Rename wakeup_cpu0() to cond_wakeup_cpu0() and fold wakeup_cpu0() in
>  as it has no other users [Rafael J. Wysocki]
> ---
>  arch/x86/include/asm/smp.h|  2 +-
>  arch/x86/kernel/smpboot.c | 24 ++--
>  drivers/acpi/processor_idle.c |  4 +---
>  3 files changed, 12 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index 57ef2094af93..630ff08532be 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -132,7 +132,7 @@ void native_play_dead(void);
>  void play_dead_common(void);
>  void wbinvd_on_cpu(int cpu);
>  int wbinvd_on_all_cpus(void);
> -bool wakeup_cpu0(void);
> +void cond_wakeup_cpu0(void);
>
>  void native_smp_send_reschedule(int cpu);
>  void native_send_call_func_ipi(const struct cpumask *mask);
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index f877150a91da..147f1bba9736 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1659,13 +1659,15 @@ void play_dead_common(void)
> local_irq_disable();
>  }
>
> -bool wakeup_cpu0(void)
> +/*
> + * If NMI wants to wake up CPU0, start CPU0.
> + */

Hasn't checkpatch.pl complained about this?

A proper kerneldoc would be something like:

/**
 * cond_wakeup_cpu0 - Wake up CPU0 if needed.
 *
 * If NMI wants to wake up CPU0, start CPU0.
 */

> +void cond_wakeup_cpu0(void)
>  {
> if (smp_processor_id() == 0 && enable_start_cpu0)
> -   return true;
> -
> -   return false;
> +   start_cpu0();
>  }
> +EXPORT_SYMBOL_GPL(cond_wakeup_cpu0);
>
>  /*
>   * We need to flush the caches before going to sleep, lest we have
> @@ -1734,11 +1736,8 @@ static inline void mwait_play_dead(void)
> __monitor(mwait_ptr, 0, 0);
> mb();
> __mwait(eax, 0);
> -   /*
> -* If NMI wants to wake up CPU0, start CPU0.
> -*/
> -   if (wakeup_cpu0())
> -   start_cpu0();
> +
> +   cond_wakeup_cpu0();
> }
>  }
>
> @@ -1749,11 +1748,8 @@ void hlt_play_dead(void)
>
> while (1) {
> native_halt();
> -   /*
> -* If NMI wants to wake up CPU0, start CPU0.
> -*/
> -   if (wakeup_cpu0())
> -   start_cpu0();
> +
> +   cond_wakeup_cpu0();
> }
>  }
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 768a6b4d2368..4e2d76b8b697 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -544,9 +544,7 @@ static int acpi_idle_play_dead(struct cpuidle_device 
> *dev, int index)
> return -ENODEV;
>
>  #if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU)
> -   /* If NMI wants to wake up CPU0, start CPU0. */
> -   if (wakeup_cpu0())
> -   start_cpu0();
> +   cond_wakeup_cpu0();
>  #endif
> }
>
> --
> 2.30.2
>


Re: [PATCH] ACPI: processor: Fix build when CONFIG_ACPI_PROCESSOR=m

2021-04-06 Thread Rafael J. Wysocki
On Tue, Apr 6, 2021 at 2:50 PM Vitaly Kuznetsov  wrote:
>
> Commit 8c182bd7 ("ACPI: processor: Fix CPU0 wakeup in
> acpi_idle_play_dead()") tried to fix CPU0 hotplug breakage by copying
> wakeup_cpu0() + start_cpu0() logic from hlt_play_dead()//mwait_play_dead()
> into acpi_idle_play_dead(). The problem is that these functions are not
> exported to modules so when CONFIG_ACPI_PROCESSOR=m build fails.
>
> The issue could've been fixed by exporting both wakeup_cpu0()/start_cpu0()
> (the later from assembly) but it seems putting the whole pattern into a
> new function and exporting it instead is better.
>
> Reported-by: kernel test robot 
> Fixes: 8c182bd7 ("CPI: processor: Fix CPU0 wakeup in 
> acpi_idle_play_dead()")
> Cc:  # 5.10+
> Signed-off-by: Vitaly Kuznetsov 
> ---
>  arch/x86/include/asm/smp.h|  2 +-
>  arch/x86/kernel/smpboot.c | 15 ++-
>  drivers/acpi/processor_idle.c |  3 +--
>  3 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index 57ef2094af93..6f79deb1f970 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -132,7 +132,7 @@ void native_play_dead(void);
>  void play_dead_common(void);
>  void wbinvd_on_cpu(int cpu);
>  int wbinvd_on_all_cpus(void);
> -bool wakeup_cpu0(void);
> +void wakeup_cpu0_if_needed(void);
>
>  void native_smp_send_reschedule(int cpu);
>  void native_send_call_func_ipi(const struct cpumask *mask);
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index f877150a91da..9547d870ee27 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1659,7 +1659,7 @@ void play_dead_common(void)
> local_irq_disable();
>  }
>
> -bool wakeup_cpu0(void)
> +static bool wakeup_cpu0(void)
>  {
> if (smp_processor_id() == 0 && enable_start_cpu0)
> return true;
> @@ -1667,6 +1667,13 @@ bool wakeup_cpu0(void)
> return false;
>  }
>
> +void wakeup_cpu0_if_needed(void)
> +{
> +   if (wakeup_cpu0())
> +   start_cpu0();

Note that all of the callers of wakeup_cpu0 do the above, so maybe
make them all use the new function?

In which case it can be rewritten in the following way

void cond_wakeup_cpu0(void)
{
if (smp_processor_id() == 0 && enable_start_cpu0)
start_cpu0();
}
EXPORT_SYMBOL_GPL(cond_wakeup_cpu0);

Also please add a proper kerneldoc comment to it and maybe drop the
comments at the call sites?


> +}
> +EXPORT_SYMBOL_GPL(wakeup_cpu0_if_needed);
> +
>  /*
>   * We need to flush the caches before going to sleep, lest we have
>   * dirty data in our caches when we come back up.
> @@ -1737,8 +1744,7 @@ static inline void mwait_play_dead(void)
> /*
>  * If NMI wants to wake up CPU0, start CPU0.
>  */
> -   if (wakeup_cpu0())
> -   start_cpu0();
> +   wakeup_cpu0_if_needed();
> }
>  }
>
> @@ -1752,8 +1758,7 @@ void hlt_play_dead(void)
> /*
>  * If NMI wants to wake up CPU0, start CPU0.
>  */
> -   if (wakeup_cpu0())
> -   start_cpu0();
> +   wakeup_cpu0_if_needed();
> }
>  }
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 768a6b4d2368..de15116b754a 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -545,8 +545,7 @@ static int acpi_idle_play_dead(struct cpuidle_device 
> *dev, int index)
>
>  #if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU)
> /* If NMI wants to wake up CPU0, start CPU0. */
> -   if (wakeup_cpu0())
> -   start_cpu0();
> +   wakeup_cpu0_if_needed();
>  #endif
> }
>
> --
> 2.30.2
>


Re: linux-next: build warning after merge of the pm tree

2021-04-02 Thread Rafael J. Wysocki
On Wednesday, March 31, 2021 8:22:54 AM CEST Vitaly Kuznetsov wrote:
> Stephen Rothwell  writes:
> 
> > Hi all,
> >
> > After merging the pm tree, today's linux-next build (x86_64 allmodconfig)
> > produced this warning:
> >
> > drivers/acpi/processor_idle.c: In function 'acpi_idle_play_dead':
> > drivers/acpi/processor_idle.c:542:15: warning: extra tokens at end of 
> > #ifdef directive
> >   542 | #ifdef defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU)
> >   |   ^
> >
> 
> Sigh, smaller the patch, more iterations it will take to make it
> right
> 
> Rafael,
> 
> something went wrong when you folded in my "[PATCH] ACPI: processor: Fix
> build when !CONFIG_HOTPLUG_CPU" here. This line should be:
> 
> #if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU)

Right, my mistake, sorry.  Fixed yesterday.

Thanks!





[GIT PULL] Power management fixes for v5.12-rc6

2021-04-02 Thread Rafael J. Wysocki
Hi Linus,

Please pull from the tag

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 pm-5.12-rc6

with top-most commit ac1790ad78f8f0cf9a588ffb530c700ad758e8b6

 Merge branch 'pm-cpufreq'

on top of commit a5e13c6df0e41702d2b2c77c8ad41677ebb065b3

 Linux 5.12-rc5

to receive power management fixes for 5.12-rc6.

These fix a race condition and an ordering issue related to
using device links in the runtime PM framework and two
kerneldoc comments in cpufreq.

Specifics:

 - Fix race condition related to the handling of supplier devices
   during consumer device probe and fix the order of decrementation
   of two related reference counters in the runtime PM core code
   handling supplier devices (Adrian Hunter).

 - Fix kerneldoc comments in cpufreq that have not been updated along
   with the functions documented by them (Geert Uytterhoeven).

Thanks!


---

Adrian Hunter (2):
  PM: runtime: Fix ordering in pm_runtime_get_suppliers()
  PM: runtime: Fix race getting/putting suppliers at probe

Geert Uytterhoeven (1):
  cpufreq: Fix scaling_{available,boost}_frequencies_show() comments

---

 drivers/base/power/runtime.c | 10 --
 drivers/cpufreq/freq_table.c |  4 ++--
 2 files changed, 10 insertions(+), 4 deletions(-)


Re: [PATCH 1/1] use crc32 instead of md5 for hibernation e820 integrity check

2021-04-01 Thread Rafael J. Wysocki
On Thu, Apr 1, 2021 at 3:34 PM Rafael J. Wysocki  wrote:
>
> On Thu, Apr 1, 2021 at 2:25 PM Chris von Recklinghausen
>  wrote:
> >
> > Suspend fails on a system in fips mode because md5 is used for the e820
> > integrity check and is not available. Use crc32 instead.
> >
> > Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory 
> > map
> >by md5 digest")
> > Signed-off-by: Chris von Recklinghausen 
> > ---
> >  arch/x86/power/hibernate.c | 31 +--
> >  1 file changed, 17 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
> > index cd3914fc9f3d..6a3f4e32e49c 100644
> > --- a/arch/x86/power/hibernate.c
> > +++ b/arch/x86/power/hibernate.c
> > @@ -55,31 +55,31 @@ int pfn_is_nosave(unsigned long pfn)
> >  }
> >
> >
> > -#define MD5_DIGEST_SIZE 16
> > +#define CRC32_DIGEST_SIZE 16
> >
> >  struct restore_data_record {
> > unsigned long jump_address;
> > unsigned long jump_address_phys;
> > unsigned long cr3;
> > unsigned long magic;
> > -   u8 e820_digest[MD5_DIGEST_SIZE];
> > +   u8 e820_digest[CRC32_DIGEST_SIZE];
> >  };
>
> No.
>
> CRC32 was used here before and it was deemed insufficient.
>
> Please find a different way to address this issue.

Well, I guess I'm going to change my mind on this, so we can go with
this patch, but please bump up RESTORE_MAGIC too.

> > -#if IS_BUILTIN(CONFIG_CRYPTO_MD5)
> > +#if IS_BUILTIN(CONFIG_CRYPTO_CRC32)
> >  /**
> > - * get_e820_md5 - calculate md5 according to given e820 table
> > + * get_e820_crc32 - calculate crc32 according to given e820 table
> >   *
> >   * @table: the e820 table to be calculated
> > - * @buf: the md5 result to be stored to
> > + * @buf: the crc32 result to be stored to
> >   */
> > -static int get_e820_md5(struct e820_table *table, void *buf)
> > +static int get_e820_crc32(struct e820_table *table, void *buf)
> >  {
> > struct crypto_shash *tfm;
> > struct shash_desc *desc;
> > int size;
> > int ret = 0;
> >
> > -   tfm = crypto_alloc_shash("md5", 0, 0);
> > +   tfm = crypto_alloc_shash("crc32", 0, 0);
> > if (IS_ERR(tfm))
> > return -ENOMEM;
> >
> > @@ -107,24 +107,24 @@ static int get_e820_md5(struct e820_table *table, 
> > void *buf)
> >
> >  static int hibernation_e820_save(void *buf)
> >  {
> > -   return get_e820_md5(e820_table_firmware, buf);
> > +   return get_e820_crc32(e820_table_firmware, buf);
> >  }
> >
> >  static bool hibernation_e820_mismatch(void *buf)
> >  {
> > int ret;
> > -   u8 result[MD5_DIGEST_SIZE];
> > +   u8 result[CRC32_DIGEST_SIZE];
> >
> > -   memset(result, 0, MD5_DIGEST_SIZE);
> > +   memset(result, 0, CRC32_DIGEST_SIZE);
> > /* If there is no digest in suspend kernel, let it go. */
> > -   if (!memcmp(result, buf, MD5_DIGEST_SIZE))
> > +   if (!memcmp(result, buf, CRC32_DIGEST_SIZE))
> > return false;
> >
> > -   ret = get_e820_md5(e820_table_firmware, result);
> > +   ret = get_e820_crc32(e820_table_firmware, result);
> > if (ret)
> > return true;
> >
> > -   return memcmp(result, buf, MD5_DIGEST_SIZE) ? true : false;
> > +   return memcmp(result, buf, CRC32_DIGEST_SIZE) ? true : false;
> >  }
> >  #else
> >  static int hibernation_e820_save(void *buf)
> > @@ -134,7 +134,7 @@ static int hibernation_e820_save(void *buf)
> >
> >  static bool hibernation_e820_mismatch(void *buf)
> >  {
> > -   /* If md5 is not builtin for restore kernel, let it go. */
> > +   /* If crc32 is not builtin for restore kernel, let it go. */
> > return false;
> >  }
> >  #endif
> > @@ -160,6 +160,9 @@ int arch_hibernation_header_save(void *addr, unsigned 
> > int max_size)
> > rdr->jump_address = (unsigned long)restore_registers;
> > rdr->jump_address_phys = __pa_symbol(restore_registers);
> >
> > +   /* crc32 digest size is 4 but digest buffer size is 16 so zero it 
> > all */
> > +   memset(rdr->e820_digest, 0, CRC32_DIGEST_SIZE);
> > +
> > /*
> >  * The restore code fixes up CR3 and CR4 in the following sequence:
> >  *
> > --
> > 2.18.1
> >


Re: Fix hibernation in FIPS mode?

2021-04-01 Thread Rafael J. Wysocki
On Thu, Apr 1, 2021 at 3:54 PM Ard Biesheuvel  wrote:
>
> On Thu, 1 Apr 2021 at 15:38, Rafael J. Wysocki  wrote:
> >
> > On Thu, Apr 1, 2021 at 10:47 AM Ard Biesheuvel  wrote:
> > >
> > > On Tue, 30 Mar 2021 at 21:56, Simo Sorce  wrote:
> > > >
> > > > On Tue, 2021-03-30 at 21:45 +0200, Ard Biesheuvel wrote:
> > > > > On Tue, 30 Mar 2021 at 20:05, Simo Sorce  wrote:
> > > > > > On Tue, 2021-03-30 at 16:46 +0200, Rafael J. Wysocki wrote:
> > > > > > > On Tue, Mar 30, 2021 at 12:14 AM Dexuan Cui  
> > > > > > > wrote:
> > > > > > > > Hi,
> > > > > > > > MD5 was marked incompliant with FIPS in 2009:
> > > > > > > > a3bef3a31a19 ("crypto: testmgr - Skip algs not flagged 
> > > > > > > > fips_allowed in fips mode")
> > > > > > > > a1915d51e8e7 ("crypto: testmgr - Mark algs allowed in fips 
> > > > > > > > mode")
> > > > > > > >
> > > > > > > > But hibernation_e820_save() is still using MD5, and fails in 
> > > > > > > > FIPS mode
> > > > > > > > due to the 2018 patch:
> > > > > > > > 749fa17093ff ("PM / hibernate: Check the success of generating 
> > > > > > > > md5 digest before hibernation")
> > > > > > > >
> > > > > > > > As a result, hibernation doesn't work when FIPS is on.
> > > > > > > >
> > > > > > > > Do you think if hibernation_e820_save() should be changed to 
> > > > > > > > use a
> > > > > > > > FIPS-compliant algorithm like SHA-1?
> > > > > > >
> > > > > > > I would say yes, it should.
> > > > > > >
> > > > > > > > PS, currently it looks like FIPS mode is broken in the mainline:
> > > > > > > > https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg49414.html
> > > > > >
> > > > > > FYI, SHA-1 is not a good choice, it is only permitted in HMAC
> > > > > > constructions and only for specified uses. If you need to change
> > > > > > algorithm you should go straight to SHA-2 or SHA-3 based hashes.
> > > > > >
> > > > >
> > > > > What is the reason for using a [broken] cryptographic hash here? if
> > > > > this is just an integrity check, better use CRC32
> >
> > Not really.
> >
> > CRC32 is not really sufficient for integrity checking here AFAICS.  It
> > might be made a fallback option if MD5 is not available, but making it
> > the default would be somewhat over the top IMO.
> >
> > > > If the integrity check is used exclusively to verify there were no
> > > > accidental changes and is not used as a security measure, by all means
> > > > I agree that using crc32 is a better idea.
> > > >
> > >
> > > Looking at 62a03defeabd58f74e07ca030d6c21e069d4d88e which introduced
> > > this, it is only a best effort check which is simply omitted if md5
> > > happens to be unavailable, so there is definitely no need for crypto
> > > here.
> >
> > Yes, it is about integrity checking only.  No, CRC32 is not equivalent
> > to MD5 in that respect AFAICS.
> >
>
> There are two possibilities:
> - we care about an adversary attempting to forge a collision, in which
> case you need a cryptographic hash which is not broken;
> - we only care about integrity, in which case crypto is overkill, and
> CRC32 is sufficient. (Note that the likelihood of an honest,
> inadvertent modification not being caught by CRC32 is 1 in 4 billion)

That depends on how you count.

Surely, there are modifications caught by MD5 that will not be caught by CRC32.

> MD5 does not meet either requirement, given that it is known to be
> broken, and overkill for simple integrity checks. MD5 should be phased
> out and removed, and moving this code onto the correct abstraction
> would be a reasonable step towards that goal.

This clearly is a matter of opinion.

I'm not religious about it though.  If there is a general consensus
that CRC32 is sufficient for error detection in hibernation files,
then it can be used.  So is there such a consensus and if so, can you
give me a pointer to some research that it is based on?


Re: [PATCH 3/7] PM: runtime: remove kernel-doc warnings

2021-04-01 Thread Rafael J. Wysocki
On Thu, Apr 1, 2021 at 1:26 AM Pierre-Louis Bossart
 wrote:
>
> remove make W=1 warnings
>
> drivers/base/power/runtime.c:926: warning: Function parameter or
> member 'timer' not described in 'pm_suspend_timer_fn'
>
> drivers/base/power/runtime.c:926: warning: Excess function parameter
> 'data' description in 'pm_suspend_timer_fn'
>
> Signed-off-by: Pierre-Louis Bossart 
> ---
>  drivers/base/power/runtime.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index fe1dad68aee4..1fc1a992f90c 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -951,7 +951,7 @@ static void pm_runtime_work(struct work_struct *work)
>
>  /**
>   * pm_suspend_timer_fn - Timer function for pm_schedule_suspend().
> - * @data: Device pointer passed by pm_schedule_suspend().
> + * @timer: hrtimer used by pm_schedule_suspend().
>   *
>   * Check if the time is right and queue a suspend request.
>   */
> --

I can apply this along with the [4-5/7].  Do you want me to do that?


Re: Fix hibernation in FIPS mode?

2021-04-01 Thread Rafael J. Wysocki
On Thu, Apr 1, 2021 at 10:47 AM Ard Biesheuvel  wrote:
>
> On Tue, 30 Mar 2021 at 21:56, Simo Sorce  wrote:
> >
> > On Tue, 2021-03-30 at 21:45 +0200, Ard Biesheuvel wrote:
> > > On Tue, 30 Mar 2021 at 20:05, Simo Sorce  wrote:
> > > > On Tue, 2021-03-30 at 16:46 +0200, Rafael J. Wysocki wrote:
> > > > > On Tue, Mar 30, 2021 at 12:14 AM Dexuan Cui  
> > > > > wrote:
> > > > > > Hi,
> > > > > > MD5 was marked incompliant with FIPS in 2009:
> > > > > > a3bef3a31a19 ("crypto: testmgr - Skip algs not flagged fips_allowed 
> > > > > > in fips mode")
> > > > > > a1915d51e8e7 ("crypto: testmgr - Mark algs allowed in fips mode")
> > > > > >
> > > > > > But hibernation_e820_save() is still using MD5, and fails in FIPS 
> > > > > > mode
> > > > > > due to the 2018 patch:
> > > > > > 749fa17093ff ("PM / hibernate: Check the success of generating md5 
> > > > > > digest before hibernation")
> > > > > >
> > > > > > As a result, hibernation doesn't work when FIPS is on.
> > > > > >
> > > > > > Do you think if hibernation_e820_save() should be changed to use a
> > > > > > FIPS-compliant algorithm like SHA-1?
> > > > >
> > > > > I would say yes, it should.
> > > > >
> > > > > > PS, currently it looks like FIPS mode is broken in the mainline:
> > > > > > https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg49414.html
> > > >
> > > > FYI, SHA-1 is not a good choice, it is only permitted in HMAC
> > > > constructions and only for specified uses. If you need to change
> > > > algorithm you should go straight to SHA-2 or SHA-3 based hashes.
> > > >
> > >
> > > What is the reason for using a [broken] cryptographic hash here? if
> > > this is just an integrity check, better use CRC32

Not really.

CRC32 is not really sufficient for integrity checking here AFAICS.  It
might be made a fallback option if MD5 is not available, but making it
the default would be somewhat over the top IMO.

> > If the integrity check is used exclusively to verify there were no
> > accidental changes and is not used as a security measure, by all means
> > I agree that using crc32 is a better idea.
> >
>
> Looking at 62a03defeabd58f74e07ca030d6c21e069d4d88e which introduced
> this, it is only a best effort check which is simply omitted if md5
> happens to be unavailable, so there is definitely no need for crypto
> here.

Yes, it is about integrity checking only.  No, CRC32 is not equivalent
to MD5 in that respect AFAICS.

Thanks!


Re: Fix hibernation in FIPS mode?

2021-04-01 Thread Rafael J. Wysocki
On Thu, Apr 1, 2021 at 6:22 PM Simo Sorce  wrote:
>
> On Thu, 2021-04-01 at 18:02 +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 1, 2021 at 3:54 PM Ard Biesheuvel  wrote:
> > > On Thu, 1 Apr 2021 at 15:38, Rafael J. Wysocki  wrote:
> > > > On Thu, Apr 1, 2021 at 10:47 AM Ard Biesheuvel  wrote:
> > > > > On Tue, 30 Mar 2021 at 21:56, Simo Sorce  wrote:
> > > > > > On Tue, 2021-03-30 at 21:45 +0200, Ard Biesheuvel wrote:
> > > > > > > On Tue, 30 Mar 2021 at 20:05, Simo Sorce  wrote:
> > > > > > > > On Tue, 2021-03-30 at 16:46 +0200, Rafael J. Wysocki wrote:
> > > > > > > > > On Tue, Mar 30, 2021 at 12:14 AM Dexuan Cui 
> > > > > > > > >  wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > MD5 was marked incompliant with FIPS in 2009:
> > > > > > > > > > a3bef3a31a19 ("crypto: testmgr - Skip algs not flagged 
> > > > > > > > > > fips_allowed in fips mode")
> > > > > > > > > > a1915d51e8e7 ("crypto: testmgr - Mark algs allowed in fips 
> > > > > > > > > > mode")
> > > > > > > > > >
> > > > > > > > > > But hibernation_e820_save() is still using MD5, and fails 
> > > > > > > > > > in FIPS mode
> > > > > > > > > > due to the 2018 patch:
> > > > > > > > > > 749fa17093ff ("PM / hibernate: Check the success of 
> > > > > > > > > > generating md5 digest before hibernation")
> > > > > > > > > >
> > > > > > > > > > As a result, hibernation doesn't work when FIPS is on.
> > > > > > > > > >
> > > > > > > > > > Do you think if hibernation_e820_save() should be changed 
> > > > > > > > > > to use a
> > > > > > > > > > FIPS-compliant algorithm like SHA-1?
> > > > > > > > >
> > > > > > > > > I would say yes, it should.
> > > > > > > > >
> > > > > > > > > > PS, currently it looks like FIPS mode is broken in the 
> > > > > > > > > > mainline:
> > > > > > > > > > https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg49414.html
> > > > > > > >
> > > > > > > > FYI, SHA-1 is not a good choice, it is only permitted in HMAC
> > > > > > > > constructions and only for specified uses. If you need to change
> > > > > > > > algorithm you should go straight to SHA-2 or SHA-3 based hashes.
> > > > > > > >
> > > > > > >
> > > > > > > What is the reason for using a [broken] cryptographic hash here? 
> > > > > > > if
> > > > > > > this is just an integrity check, better use CRC32
> > > >
> > > > Not really.
> > > >
> > > > CRC32 is not really sufficient for integrity checking here AFAICS.  It
> > > > might be made a fallback option if MD5 is not available, but making it
> > > > the default would be somewhat over the top IMO.
> > > >
> > > > > > If the integrity check is used exclusively to verify there were no
> > > > > > accidental changes and is not used as a security measure, by all 
> > > > > > means
> > > > > > I agree that using crc32 is a better idea.
> > > > > >
> > > > >
> > > > > Looking at 62a03defeabd58f74e07ca030d6c21e069d4d88e which introduced
> > > > > this, it is only a best effort check which is simply omitted if md5
> > > > > happens to be unavailable, so there is definitely no need for crypto
> > > > > here.
> > > >
> > > > Yes, it is about integrity checking only.  No, CRC32 is not equivalent
> > > > to MD5 in that respect AFAICS.
> > > >
> > >
> > > There are two possibilities:
> > > - we care about an adversary attempting to forge a collision, in which
> > > case you need a cryptographic hash which is not broken;
> > > - we only care about integrity, in which case crypto is overkill, and
> > > CRC32 is sufficient. (Note that the likelihood of an honest,
> > > inadverten

Re: [PATCH] drivers core: don't do anything in device_del() when device_add() fail

2021-04-01 Thread Rafael J. Wysocki
On Thu, Apr 1, 2021 at 2:56 PM Yufen Yu  wrote:
>
> Recently, our syzbot test reported NULL pointer dereference in
> device_del() by injecting memory allocation fail in device_add().
>
> For now, callers of device_add(), such as add_disk(), may ignore
> device_add()'s fail and go on working. In unregister path, it will
> call device_del() from del_gendisk(). That can cause various NULL
> pointer dereference, including dev->p is NULL in kill_device(),
> 'name' is NULL in sysfs_remove_link(), kobj->sd is 'NULL' when call
> dpm_sysfs_remove(), and so on.
>
> To avoid these kernel panic, we call device_del() only when device_add()
> is success.

The patch looks reasonable to me, but the above is not what it does.

It causes device_del() to be a no-op for devices that have not been
successfully added.

> Signed-off-by: Yufen Yu 
> ---
>  drivers/base/core.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index f29839382f81..a10ec5dbc577 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3380,6 +3380,9 @@ void device_del(struct device *dev)
> struct class_interface *class_intf;
> unsigned int noio_flag;
>
> +   if (!dev->p)
> +   return;
> +
> device_lock(dev);
> kill_device(dev);
> device_unlock(dev);
> --


Re: [PATCH 1/1] use crc32 instead of md5 for hibernation e820 integrity check

2021-04-01 Thread Rafael J. Wysocki
On Thu, Apr 1, 2021 at 3:59 PM Ard Biesheuvel  wrote:
>
> On Thu, 1 Apr 2021 at 15:34, Rafael J. Wysocki  wrote:
> >
> > On Thu, Apr 1, 2021 at 2:25 PM Chris von Recklinghausen
> >  wrote:
> > >
> > > Suspend fails on a system in fips mode because md5 is used for the e820
> > > integrity check and is not available. Use crc32 instead.
> > >
> > > Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 
> > > memory map
> > >by md5 digest")
> > > Signed-off-by: Chris von Recklinghausen 
> > > ---
> > >  arch/x86/power/hibernate.c | 31 +--
> > >  1 file changed, 17 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
> > > index cd3914fc9f3d..6a3f4e32e49c 100644
> > > --- a/arch/x86/power/hibernate.c
> > > +++ b/arch/x86/power/hibernate.c
> > > @@ -55,31 +55,31 @@ int pfn_is_nosave(unsigned long pfn)
> > >  }
> > >
> > >
> > > -#define MD5_DIGEST_SIZE 16
> > > +#define CRC32_DIGEST_SIZE 16
> > >
> > >  struct restore_data_record {
> > > unsigned long jump_address;
> > > unsigned long jump_address_phys;
> > > unsigned long cr3;
> > > unsigned long magic;
> > > -   u8 e820_digest[MD5_DIGEST_SIZE];
> > > +   u8 e820_digest[CRC32_DIGEST_SIZE];
> > >  };
> >
> > No.
> >
> > CRC32 was used here before and it was deemed insufficient.
> >
>
> Why? The git commit log does not have an explanation of this.

IIRC there was an example of a memory map that would produce the same
CRC32 value as the original or something like that.

But that said this code is all about failing more gracefully, so I
guess it isn't a big deal if the failure is more graceful in fewer
cases ...


Re: [PATCH v2 04/15] ACPI: table: replace __attribute__((packed)) by __packed

2021-04-01 Thread Rafael J. Wysocki
On Thu, Apr 1, 2021 at 11:00 AM David Laight  wrote:
>
> From: Bjorn Helgaas
> > Sent: 31 March 2021 18:22
> >
> > On Wed, Mar 31, 2021 at 11:55:08PM +0800, Zhang Rui wrote:
> > > ...
> >
> > > From e18c942855e2f51e814d057fff4dd951cd0d0907 Mon Sep 17 00:00:00 2001
> > > From: Zhang Rui 
> > > Date: Wed, 31 Mar 2021 20:34:13 +0800
> > > Subject: [PATCH] ACPI: tables: FPDT: Fix 64bit alignment issue
> > >
> > > Some of the 64bit items in FPDT table may be 32bit aligned.
> > > Using __attribute__((packed)) is not needed in this case, fixing it by
> > > allowing 32bit alignment for these 64bit items.
> >
> > 1) Can you please add a spec reference for this?  I think it's ACPI
> >v6.3, sec 5.2.23.5, or something close to that.
> >
> > 2) The exact layout in memory is prescribed by the spec.  I think
> >that's basically what "packed" accomplishes.  I don't understand
> >why using "aligned" would be preferable.  Using "aligned" means
> >things can be at different offsets depending on the starting
> >address of the structure.  We always want the identical layout, no
> >matter what the starting address is.
>
> Both 'packed' and 'aligned(4)' remove any structure alignment
> padding before 64bit items that aren't on an 8 byte boundary.
> (Because everything else in the structures is naturally aligned.)
>
> The difference is significant on cpu that don't support misaligned
> addresses.
> Assuming that the structure is always on a 4n byte boundary
> (which the ACPI spec probably requires) accesses to the 32-bit
> fields are always ok.
> It is only 64-bit fields that must be accessed as two 32-bit
> memory cycles, not all the fields using multiple single byte
> cycles.

So what exactly is wrong with using "packed"?  It is way easier to
understand for a casual reader of the code.


Re: [PATCH 1/1] use crc32 instead of md5 for hibernation e820 integrity check

2021-04-01 Thread Rafael J. Wysocki
On Thu, Apr 1, 2021 at 2:25 PM Chris von Recklinghausen
 wrote:
>
> Suspend fails on a system in fips mode because md5 is used for the e820
> integrity check and is not available. Use crc32 instead.
>
> Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
>by md5 digest")
> Signed-off-by: Chris von Recklinghausen 
> ---
>  arch/x86/power/hibernate.c | 31 +--
>  1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
> index cd3914fc9f3d..6a3f4e32e49c 100644
> --- a/arch/x86/power/hibernate.c
> +++ b/arch/x86/power/hibernate.c
> @@ -55,31 +55,31 @@ int pfn_is_nosave(unsigned long pfn)
>  }
>
>
> -#define MD5_DIGEST_SIZE 16
> +#define CRC32_DIGEST_SIZE 16
>
>  struct restore_data_record {
> unsigned long jump_address;
> unsigned long jump_address_phys;
> unsigned long cr3;
> unsigned long magic;
> -   u8 e820_digest[MD5_DIGEST_SIZE];
> +   u8 e820_digest[CRC32_DIGEST_SIZE];
>  };

No.

CRC32 was used here before and it was deemed insufficient.

Please find a different way to address this issue.

> -#if IS_BUILTIN(CONFIG_CRYPTO_MD5)
> +#if IS_BUILTIN(CONFIG_CRYPTO_CRC32)
>  /**
> - * get_e820_md5 - calculate md5 according to given e820 table
> + * get_e820_crc32 - calculate crc32 according to given e820 table
>   *
>   * @table: the e820 table to be calculated
> - * @buf: the md5 result to be stored to
> + * @buf: the crc32 result to be stored to
>   */
> -static int get_e820_md5(struct e820_table *table, void *buf)
> +static int get_e820_crc32(struct e820_table *table, void *buf)
>  {
> struct crypto_shash *tfm;
> struct shash_desc *desc;
> int size;
> int ret = 0;
>
> -   tfm = crypto_alloc_shash("md5", 0, 0);
> +   tfm = crypto_alloc_shash("crc32", 0, 0);
> if (IS_ERR(tfm))
> return -ENOMEM;
>
> @@ -107,24 +107,24 @@ static int get_e820_md5(struct e820_table *table, void 
> *buf)
>
>  static int hibernation_e820_save(void *buf)
>  {
> -   return get_e820_md5(e820_table_firmware, buf);
> +   return get_e820_crc32(e820_table_firmware, buf);
>  }
>
>  static bool hibernation_e820_mismatch(void *buf)
>  {
> int ret;
> -   u8 result[MD5_DIGEST_SIZE];
> +   u8 result[CRC32_DIGEST_SIZE];
>
> -   memset(result, 0, MD5_DIGEST_SIZE);
> +   memset(result, 0, CRC32_DIGEST_SIZE);
> /* If there is no digest in suspend kernel, let it go. */
> -   if (!memcmp(result, buf, MD5_DIGEST_SIZE))
> +   if (!memcmp(result, buf, CRC32_DIGEST_SIZE))
> return false;
>
> -   ret = get_e820_md5(e820_table_firmware, result);
> +   ret = get_e820_crc32(e820_table_firmware, result);
> if (ret)
> return true;
>
> -   return memcmp(result, buf, MD5_DIGEST_SIZE) ? true : false;
> +   return memcmp(result, buf, CRC32_DIGEST_SIZE) ? true : false;
>  }
>  #else
>  static int hibernation_e820_save(void *buf)
> @@ -134,7 +134,7 @@ static int hibernation_e820_save(void *buf)
>
>  static bool hibernation_e820_mismatch(void *buf)
>  {
> -   /* If md5 is not builtin for restore kernel, let it go. */
> +   /* If crc32 is not builtin for restore kernel, let it go. */
> return false;
>  }
>  #endif
> @@ -160,6 +160,9 @@ int arch_hibernation_header_save(void *addr, unsigned int 
> max_size)
> rdr->jump_address = (unsigned long)restore_registers;
> rdr->jump_address_phys = __pa_symbol(restore_registers);
>
> +   /* crc32 digest size is 4 but digest buffer size is 16 so zero it all 
> */
> +   memset(rdr->e820_digest, 0, CRC32_DIGEST_SIZE);
> +
> /*
>  * The restore code fixes up CR3 and CR4 in the following sequence:
>  *
> --
> 2.18.1
>


Re: [PATCH v2 04/15] ACPI: table: replace __attribute__((packed)) by __packed

2021-04-01 Thread Rafael J. Wysocki
On Thu, Apr 1, 2021 at 4:23 PM David Laight  wrote:
>
> From: Rafael J. Wysocki
> > Sent: 01 April 2021 14:50
> ...
> > So what exactly is wrong with using "packed"?  It is way easier to
> > understand for a casual reader of the code.
>
> Because it is usually wrong!
>
> If I have:
> struct foo {
> u64 val;
> } __packed;
>
> And then have:
> u64 bar(struct foo *foo)
> {
> return foo->val;
> }
>
> The on some cpu the compiler has to generate the equivalent of:
> u8 *x = (void *)>val;
> return x[0] | x[1] << 8 | x[2] << 16 | x[3] << 24 | x[4] << 32 | x[5] 
> << 40 | x[6] << 48 | x[7] << 56;
>
> If you can guarantee that the structure is 32bit aligned
> then it can generate the simpler:
> u32 *x = (void *)>val;
> return x[0] | x[1] << 32;
>
> (Yes I've missed out the 64-bit casts)
>
> This is why you should almost never use __packed.
>
> There are historic structures with 64 bit items on 4 byte boundaries
> (and 32 bit values on 2 byte boundaries).
> Typically most of the fields are shorter so can be read directly
> (although they might need a byte-swapping load).

The possible overhead impact is clear to me, but I really don't like
the "local" typedef idea.

It at least would need to be accompanied by a comment explaining why
it is there and why using it is better than using __packed and why
this needs to be defined locally and not in some generic header file.

Also, the FPDT code is just one function that parses the entire table
and there is no object passing between functions in it etc, so is
__packed still problematic in there?


Re: [PATCH] PCI: ACPI: PM: Fix debug message in acpi_pci_set_power_state()

2021-04-01 Thread Rafael J. Wysocki
On Wed, Mar 31, 2021 at 11:09 PM Bjorn Helgaas  wrote:
>
> On Thu, Mar 25, 2021 at 07:57:51PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki 
> >
> > If PCI_D3cold is passed to acpi_pci_set_power_state() as the second
> > argument and there is no ACPI D3cold support for the given device,
> > the debug message printed by that function will state that the
> > device power state has been changed to D3cold, while in fact it
> > will be D3hot, because acpi_device_set_power() falls back to D3hot
> > automatically if D3cold is not supported without returning an error.
> >
> > To address this issue, modify the debug message in question to print
> > the current power state of the target PCI device's ACPI companion
> > instead of printing the target power state which may not reflect
> > the real final power state of the device.
> >
> > Signed-off-by: Rafael J. Wysocki 
>
> Applied with Krzysztof's reviewed-by to pci/pm for v5.13, thanks!
>
> Let me know if you have nearby or related changes that you'd rather
> take via your tree.

I don't have any, thank you!


Re: linux-next: Tree for Mar 31 (acpi build warning)

2021-03-31 Thread Rafael J. Wysocki
On Wed, Mar 31, 2021 at 5:32 PM Randy Dunlap  wrote:
>
> On 3/31/21 2:43 AM, Stephen Rothwell wrote:
> > Hi all,
> >
> > News: there will be no linux-next release on Friday or the following
> > Monday.
> >
> > Changes since 20210330:
> >
>
> ../drivers/acpi/processor_idle.c:542:15: warning: extra tokens at end of 
> #ifdef directive
>
> #ifdef defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU)
>
> s/ifdef/if/

Yup, my mistake, sorry about that.  Will fix shortly.


Re: [PATCH] ACPI: processor: Fix build when !CONFIG_HOTPLUG_CPU

2021-03-30 Thread Rafael J. Wysocki
On Tue, Mar 30, 2021 at 12:27 PM Vitaly Kuznetsov  wrote:
>
> Kernel test robot reports build breakage with commit 5f5e49e999ac
> ("ACPI: processor: Fix CPU0 wakeup in acpi_idle_play_dead()") when
> !CONFIG_HOTPLUG_CPU/!CONFIG_SMP. wakeup_cpu0() is defined under
> CONFIG_SMP and start_cpu0() under CONFIG_HOTPLUG_CPU which, in its turn,
> depend on CONFIG_SMP. Add #ifdef CONFIG_HOTPLUG_CPU to the block, this
> should be sufficient.
>
> Reported-by: kernel test robot 
> Fixes: 5f5e49e999ac ("ACPI: processor: Fix CPU0 wakeup in 
> acpi_idle_play_dead()")

Folded into the above patch, thanks!

> Signed-off-by: Vitaly Kuznetsov 
> ---
>  drivers/acpi/processor_idle.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index f0c73f658880..0925b1477230 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -539,7 +539,7 @@ static int acpi_idle_play_dead(struct cpuidle_device 
> *dev, int index)
> } else
> return -ENODEV;
>
> -#ifdef CONFIG_X86
> +#if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU)
> /* If NMI wants to wake up CPU0, start CPU0. */
> if (wakeup_cpu0())
> start_cpu0();
> --
> 2.30.2
>


Re: [PATCH v2 04/15] ACPI: table: replace __attribute__((packed)) by __packed

2021-03-30 Thread Rafael J. Wysocki
On Tue, Mar 30, 2021 at 10:15 AM David Laight  wrote:
>
> From: Zhang Rui
> > Sent: 30 March 2021 09:00
> > To: Xiaofei Tan ; David Laight 
> > ; r...@rjwysocki.net;
> > l...@kernel.org; bhelg...@google.com
> > Cc: linux-a...@vger.kernel.org; linux-kernel@vger.kernel.org; 
> > linux-...@vger.kernel.org;
> > linux...@openeuler.org
> > Subject: Re: [PATCH v2 04/15] ACPI: table: replace __attribute__((packed)) 
> > by __packed
> >
> > On Tue, 2021-03-30 at 15:31 +0800, Zhang Rui wrote:
> > > On Tue, 2021-03-30 at 10:23 +0800, Xiaofei Tan wrote:
> > > > Hi David,
> > > >
> > > > On 2021/3/29 18:09, David Laight wrote:
> > > > > From: Xiaofei Tan
> > > > > > Sent: 27 March 2021 07:46
> > > > > >
> > > > > > Replace __attribute__((packed)) by __packed following the
> > > > > > advice of checkpatch.pl.
> > > > > >
> > > > > > Signed-off-by: Xiaofei Tan 
> > > > > > ---
> > > > > >  drivers/acpi/acpi_fpdt.c | 6 +++---
> > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/acpi/acpi_fpdt.c
> > > > > > b/drivers/acpi/acpi_fpdt.c
> > > > > > index a89a806..690a88a 100644
> > > > > > --- a/drivers/acpi/acpi_fpdt.c
> > > > > > +++ b/drivers/acpi/acpi_fpdt.c
> > > > > > @@ -53,7 +53,7 @@ struct resume_performance_record {
> > > > > >   u32 resume_count;
> > > > > >   u64 resume_prev;
> > > > > >   u64 resume_avg;
> > > > > > -} __attribute__((packed));
> > > > > > +} __packed;
> > > > > >
> > > > > >  struct boot_performance_record {
> > > > > >   struct fpdt_record_header header;
> > > > > > @@ -63,13 +63,13 @@ struct boot_performance_record {
> > > > > >   u64 bootloader_launch;
> > > > > >   u64 exitbootservice_start;
> > > > > >   u64 exitbootservice_end;
> > > > > > -} __attribute__((packed));
> > > > > > +} __packed;
> > > > > >
> > > > > >  struct suspend_performance_record {
> > > > > >   struct fpdt_record_header header;
> > > > > >   u64 suspend_start;
> > > > > >   u64 suspend_end;
> > > > > > -} __attribute__((packed));
> > > > > > +} __packed;
> > > > >
> > > > > My standard question about 'packed' is whether it is actually
> > > > > needed.
> > > > > It should only be used if the structures might be misaligned in
> > > > > memory.
> > > > > If the only problem is that a 64bit item needs to be 32bit
> > > > > aligned
> > > > > then a suitable type should be used for those specific fields.
> > > > >
> > > > > Those all look very dubious - the standard header isn't packed
> > > > > so everything must eb assumed to be at least 32bit aligned.
> > > > >
> > > > > There are also other sub-structures that contain 64bit values.
> > > > > These don't contain padding - but that requires 64bit alignement.
> > > > >
> > > > > The only problematic structure is the last one - which would have
> > > > > a 32bit pad after the header.
> > > > > Is this even right given than there are explicit alignment pads
> > > > > in some of the other structures.
> > > > >
> > > > > If 64bit alignment isn't guaranteed then a '64bit aligned to
> > > > > 32bit'
> > > > > type should be used for the u64 fields.
> > > > >
> > > >
> > > > Yes, some of them has been aligned already, then nothing changed
> > > > when
> > > > add this "packed ". Maybe the purpose of the original author is
> > > > for
> > > > extension, and can tell others that this struct need be packed.
> > > >
> > >
> > > The patch is upstreamed recently but it was made long time ago.
> > > I think the original problem is that one of the address, probably the
> > > suspend_performance record, is not 64bit aligned, thus we can not
> > > read
> > > the proper content of suspend_start and suspend_end, mapped from
> > > physical memory.
> > >
> > > I will try to find a machine to reproduce the problem with all
> > > __attribute__((packed)) removed to double confirm this.
> > >
> >
> > So here is the problem, without __attribute__((packed))
> >
> > [0.858442] suspend_record: 0xaad500175020
> > /sys/firmware/acpi/fpdt/suspend/suspend_end_ns:addr:
> > 0xaad500175030, 15998179292659843072
> > /sys/firmware/acpi/fpdt/suspend/suspend_start_ns:addr:
> > 0xaad500175028, 0
> >
> > suspend_record is mapped to 0xaad500175020, and it is combined with
> > one 32bit header and two 64bit fields (suspend_start and suspend_end),
> > this is how it is located in physical memory.
> > So the addresses of the two 64bit fields are actually not 64bit
> > aligned.
> >
> > David,
> > Is this the "a 64bit item needs to be 32bit aligned" problem you
> > referred?
> > If yes, what is the proper fix? should I used two 32bits for each of
> > the field instead?
>
> Define something like:
> typedef u64 __attribute__((aligned(4))) u64_align32;
> and then use it for the 64bit structure members.
>
> There doesn't seem to be a standard type name for it - although
> it is used in several places.
>
> I'm not entirely sure but is ACPI always LE?

Yes.

> (is it even x86 only??)

No.


Re: [PATCH] resource: Prevent irqresource_disabled() from erasing flags

2021-03-30 Thread Rafael J. Wysocki

On 3/29/2021 9:52 PM, Angela Czubak wrote:

Do not overwrite flags as it leads to erasing triggering and polarity
information which might be useful in case of hard-coded interrupts.
This way the information can be read later on even though mapping to
APIC domain failed.

Signed-off-by: Angela Czubak 
---
Some Chromebooks use hard-coded interrupts in their ACPI tables.
This is an excerpt as dumped on Relm:

...
             Name (_HID, "ELAN0001")  // _HID: Hardware ID
             Name (_DDN, "Elan Touchscreen ")  // _DDN: DOS Device Name
             Name (_UID, 0x05)  // _UID: Unique ID
             Name (ISTP, Zero)
             Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
             {
                 Name (BUF0, ResourceTemplate ()
                 {
                     I2cSerialBusV2 (0x0010, ControllerInitiated, 0x00061A80,
                         AddressingMode7Bit, "\\_SB.I2C1",
                         0x00, ResourceConsumer, , Exclusive,
                         )
                     Interrupt (ResourceConsumer, Edge, ActiveLow, Exclusive, 
,, )
                     {
                         0x00B8,
                     }
                 })
                 Return (BUF0) /* \_SB_.I2C1.ETSA._CRS.BUF0 */
             }
...

This interrupt is hard-coded to 0xB8 = 184 which is too high to be mapped
to IO-APIC, so no triggering information is propagated as acpi_register_gsi()
fails and irqresource_disabled() is issued, which leads to erasing triggering
and polarity information.
If that function added its flags instead of overwriting them the correct IRQ
type would be set even for the hard-coded interrupts, which allows device driver
to retrieve it.
Please, let me know if this kind of modification is acceptable.


From the quick look it should not be problematic, but it needs to be 
checked more carefully.


Mika, what do you think?



  include/linux/ioport.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 55de385c839cf..647744d8514e0 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -331,7 +331,7 @@ static inline void irqresource_disabled(struct resource 
*res, u32 irq)
  {
res->start = irq;
res->end = irq;
-   res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET;
+   res->flags |= IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET;
  }
  
  extern struct address_space *iomem_get_mapping(void);





Re: [PATCH v1 5/5] cpuidle: menu: Take negative "sleep length" values into account

2021-03-30 Thread Rafael J. Wysocki
On Tue, Mar 30, 2021 at 4:00 AM Zhou Ti (x2019cwm)  wrote:
>
> On Mon 2021-03-29 14:37 Rafael J. Wysocki wrote:
> > Make the menu governor check the tick_nohz_get_next_hrtimer()
> > return value so as to avoid dealing with negative "sleep length"
> > values and make it use that value directly when the tick is stopped.
> >
> > While at it, rename local variable delta_next in menu_select() to
> > delta_tick which better reflects its purpose.
> >
> > Signed-off-by: Rafael J. Wysocki 
> > ---
> >  drivers/cpuidle/governors/menu.c |   17 +++--
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > Index: linux-pm/drivers/cpuidle/governors/menu.c
> > ===
> > --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> > +++ linux-pm/drivers/cpuidle/governors/menu.c
> > @@ -271,7 +271,7 @@ static int menu_select(struct cpuidle_dr
> >  u64 predicted_ns;
> >  u64 interactivity_req;
> >  unsigned long nr_iowaiters;
> > -   ktime_t delta_next;
> > +   ktime_t delta, delta_tick;
> >  int i, idx;
> >
> >  if (data->needs_update) {
> > @@ -280,7 +280,12 @@ static int menu_select(struct cpuidle_dr
> >  }
> >
> >  /* determine the expected residency time, round up */
> > -   data->next_timer_ns = tick_nohz_get_sleep_length(_next);
> > +   delta = tick_nohz_get_sleep_length(_tick);
> > +   if (unlikely(delta < 0)) {
> > +   delta = 0;
> > +   delta_tick = 0;
> > +   }
> > +   data->next_timer_ns = delta;
> >
> >  nr_iowaiters = nr_iowait_cpu(dev->cpu);
> >  data->bucket = which_bucket(data->next_timer_ns, nr_iowaiters);
> > @@ -318,7 +323,7 @@ static int menu_select(struct cpuidle_dr
> >   * state selection.
> >   */
> >  if (predicted_ns < TICK_NSEC)
> > -   predicted_ns = delta_next;
> > +   predicted_ns = data->next_timer_ns;
> >  } else {
> >  /*
> >   * Use the performance multiplier and the user-configurable
> > @@ -377,7 +382,7 @@ static int menu_select(struct cpuidle_dr
> >   * stuck in the shallow one for too long.
> >   */
> >  if (drv->states[idx].target_residency_ns < 
> > TICK_NSEC &&
> > -   s->target_residency_ns <= delta_next)
> > +   s->target_residency_ns <= delta_tick)
> >  idx = i;
> >
> >  return idx;
> > @@ -399,7 +404,7 @@ static int menu_select(struct cpuidle_dr
> >   predicted_ns < TICK_NSEC) && !tick_nohz_tick_stopped()) {
> >  *stop_tick = false;
> >
> > -   if (idx > 0 && drv->states[idx].target_residency_ns > 
> > delta_next) {
> > +   if (idx > 0 && drv->states[idx].target_residency_ns > 
> > delta_tick) {
> >  /*
> >   * The tick is not going to be stopped and the 
> > target
> >   * residency of the state to be returned is not 
> > within
> > @@ -411,7 +416,7 @@ static int menu_select(struct cpuidle_dr
> >  continue;
> >
> >  idx = i;
> > -   if (drv->states[i].target_residency_ns <= 
> > delta_next)
> > +   if (drv->states[i].target_residency_ns <= 
> > delta_tick)
> >  break;
> >  }
> >  }
>
> How about this.
> I think it's possible to avoid the new variable delta.
>
> ---
>
> --- linux-pm/drivers/cpuidle/governors/menu.c.orig  2021-03-29 
> 22:44:02.316971970 -0300
> +++ linux-pm/drivers/cpuidle/governors/menu.c   2021-03-29 22:51:15.804377168 
> -0300
> @@ -271,7 +271,7 @@ static int menu_select(struct cpuidle_dr
> u64 predicted_ns;
> u64 interactivity_req;
> unsigned long nr_iowaiters;
> -   ktime_t delta_next;
> +   ktime_t delta_tick;
> int i, idx;
>
> if (data->needs_update) {
> @@ -280,7 +280,12 @@ static int menu_select(struct cpuidle_dr
> }
>
> /* determine the expected residency time, round up */
> -   data->next_timer_ns = tick_nohz_get_sleep_length(_next);
> +   data->next_timer_ns = tick_nohz_get_sleep_length(_tick);
> +
> +   if (unlikely(data->next_timer_ns >> 63)) {
> +   data->next_timer_ns = 0;
> +   delta_tick = 0;
> +   }

Well, not really.  Using a new local var is cleaner IMO.


Re: Fix hibernation in FIPS mode?

2021-03-30 Thread Rafael J. Wysocki
On Tue, Mar 30, 2021 at 12:14 AM Dexuan Cui  wrote:
>
> Hi,
> MD5 was marked incompliant with FIPS in 2009:
> a3bef3a31a19 ("crypto: testmgr - Skip algs not flagged fips_allowed in fips 
> mode")
> a1915d51e8e7 ("crypto: testmgr - Mark algs allowed in fips mode")
>
> But hibernation_e820_save() is still using MD5, and fails in FIPS mode
> due to the 2018 patch:
> 749fa17093ff ("PM / hibernate: Check the success of generating md5 digest 
> before hibernation")
>
> As a result, hibernation doesn't work when FIPS is on.
>
> Do you think if hibernation_e820_save() should be changed to use a
> FIPS-compliant algorithm like SHA-1?

I would say yes, it should.

> PS, currently it looks like FIPS mode is broken in the mainline:
> https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg49414.html


Re: [Devel] Re: [PATCH] ACPICA: Fix a typo

2021-03-30 Thread Rafael J. Wysocki
On Tue, Mar 30, 2021 at 2:19 AM Kaneda, Erik  wrote:
>
>
>
> > -Original Message-
> > From: Rafael J. Wysocki 
> > Sent: Monday, March 29, 2021 5:48 AM
> > To: Bhaskar Chowdhury ; Kaneda, Erik
> > 
> > Cc: Wysocki, Rafael J ; ACPI Devel Maling List
> > ; open list:ACPI COMPONENT ARCHITECTURE
> > (ACPICA) ; Linux Kernel Mailing List  > ker...@vger.kernel.org>; Randy Dunlap 
> > Subject: [Devel] Re: [PATCH] ACPICA: Fix a typo
> >
> > On Fri, Mar 26, 2021 at 1:22 AM Bhaskar Chowdhury
> >  wrote:
> > >
> > >
> > > s/optimzation/optimization/
> > >
> > > Signed-off-by: Bhaskar Chowdhury 
> > > ---
> > >  include/acpi/acoutput.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/acpi/acoutput.h b/include/acpi/acoutput.h
> > > index 1538a6853822..1b4c45815695 100644
> > > --- a/include/acpi/acoutput.h
> > > +++ b/include/acpi/acoutput.h
> > > @@ -362,7 +362,7 @@
> > >   *
> > >   * A less-safe version of the macros is provided for optional use if the
> > >   * compiler uses excessive CPU stack (for example, this may happen in the
> > > - * debug case if code optimzation is disabled.)
> > > + * debug case if code optimization is disabled.)
> > >   */
> > >
> > >  /* Exit trace helper macro */
> > > --
> >
> > Erik, could you pick up this patch, please?  It is simple enough IMV ...
>
> No problem, I'll pick it up

Thanks!


[PATCH v1 1/5] tick/nohz: Improve tick_nohz_get_next_hrtimer() kerneldoc

2021-03-29 Thread Rafael J. Wysocki
From: "Rafael J. Wysocki" 

Make the tick_nohz_get_next_hrtimer() kerneldoc comment state clearly
that the function may return negative numbers.

Signed-off-by: Rafael J. Wysocki 
---
 kernel/time/tick-sched.c |6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-pm/kernel/time/tick-sched.c
===
--- linux-pm.orig/kernel/time/tick-sched.c
+++ linux-pm/kernel/time/tick-sched.c
@@ -1124,7 +1124,11 @@ ktime_t tick_nohz_get_next_hrtimer(void)
  * tick_nohz_get_sleep_length - return the expected length of the current sleep
  * @delta_next: duration until the next event if the tick cannot be stopped
  *
- * Called from power state control code with interrupts disabled
+ * Called from power state control code with interrupts disabled.
+ *
+ * The return value of this function and/or the value returned by it through 
the
+ * @delta_next pointer can be negative which must be taken into account by its
+ * callers.
  */
 ktime_t tick_nohz_get_sleep_length(ktime_t *delta_next)
 {





[PATCH v1 0/5] cpuidle: Take possible negative "sleep length" values into account

2021-03-29 Thread Rafael J. Wysocki
Hi All,

As follows from the discussion triggered by the patch at

https://lore.kernel.org/lkml/20210311123708.23501-2-frede...@kernel.org/

the cpuidle governors using tick_nohz_get_sleep_length() assume it to always
return positive values which is not correct in general.

To address this issues, first document the fact that negative values can
be returned by tick_nohz_get_sleep_length() (patch [1/5]).  Then, in
preparation for more substantial changes, change the data type of two
fields in struct cpuidle_state to s64 so they can be used in computations
involving negative numbers safely (patch [2/5]).

Next, adjust the teo governor a bit so that negative "sleep length" values
are counted like zero by it (patch [3/5]) and modify it so as to avoid
mishandling negative "sleep length" values (patch [4/5]).

Finally, make the menu governor take negative "sleep length" values into
account properly (patch [5/5]).

Please see the changelogs of the patches for details.

Thanks!





[RFC][PATCH 1/5] tick/nohz: Improve tick_nohz_get_next_hrtimer() kerneldoc

2021-03-29 Thread Rafael J. Wysocki
From: "Rafael J. Wysocki" 

Make the tick_nohz_get_next_hrtimer() kerneldoc comment state clearly
that the function may return negative numbers.

Signed-off-by: Rafael J. Wysocki 
---
 kernel/time/tick-sched.c |6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-pm/kernel/time/tick-sched.c
===
--- linux-pm.orig/kernel/time/tick-sched.c
+++ linux-pm/kernel/time/tick-sched.c
@@ -1124,7 +1124,11 @@ ktime_t tick_nohz_get_next_hrtimer(void)
  * tick_nohz_get_sleep_length - return the expected length of the current sleep
  * @delta_next: duration until the next event if the tick cannot be stopped
  *
- * Called from power state control code with interrupts disabled
+ * Called from power state control code with interrupts disabled.
+ *
+ * The return value of this function and/or the value returned by it through 
the
+ * @delta_next pointer can be negative which must be taken into account by its
+ * callers.
  */
 ktime_t tick_nohz_get_sleep_length(ktime_t *delta_next)
 {





[PATCH v1 4/5] cpuidle: teo: Take negative "sleep length" values into account

2021-03-29 Thread Rafael J. Wysocki
From: "Rafael J. Wysocki" 

Modify the TEO governor to take possible negative return values of
tick_nohz_get_next_hrtimer() into account by changing the data type
of some variables used by it to s64 which allows it to carry out
computations without potentially problematic data type conversions
into u64.

Also change the computations in teo_select() so that the negative
values themselves are handled in a natural way to avoid adding extra
negative value checks to that function.

Signed-off-by: Rafael J. Wysocki 
---
 drivers/cpuidle/governors/teo.c |   22 +++---
 1 file changed, 15 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/cpuidle/governors/teo.c
===
--- linux-pm.orig/drivers/cpuidle/governors/teo.c
+++ linux-pm/drivers/cpuidle/governors/teo.c
@@ -100,8 +100,8 @@ struct teo_idle_state {
  * @intervals: Saved idle duration values.
  */
 struct teo_cpu {
-   u64 time_span_ns;
-   u64 sleep_length_ns;
+   s64 time_span_ns;
+   s64 sleep_length_ns;
struct teo_idle_state states[CPUIDLE_STATE_MAX];
int interval_idx;
u64 intervals[INTERVALS];
@@ -214,7 +214,7 @@ static bool teo_time_ok(u64 interval_ns)
  */
 static int teo_find_shallower_state(struct cpuidle_driver *drv,
struct cpuidle_device *dev, int state_idx,
-   u64 duration_ns)
+   s64 duration_ns)
 {
int i;
 
@@ -240,10 +240,10 @@ static int teo_select(struct cpuidle_dri
 {
struct teo_cpu *cpu_data = per_cpu_ptr(_cpus, dev->cpu);
s64 latency_req = cpuidle_governor_latency_req(dev->cpu);
-   u64 duration_ns;
+   int max_early_idx, prev_max_early_idx, constraint_idx, idx0, idx, i;
unsigned int hits, misses, early_hits;
-   int max_early_idx, prev_max_early_idx, constraint_idx, idx, i;
ktime_t delta_tick;
+   s64 duration_ns;
 
if (dev->last_state_idx >= 0) {
teo_update(drv, dev);
@@ -262,6 +262,7 @@ static int teo_select(struct cpuidle_dri
prev_max_early_idx = -1;
constraint_idx = drv->state_count;
idx = -1;
+   idx0 = idx;
 
for (i = 0; i < drv->state_count; i++) {
struct cpuidle_state *s = >states[i];
@@ -322,6 +323,7 @@ static int teo_select(struct cpuidle_dri
idx = i; /* first enabled state */
hits = cpu_data->states[i].hits;
misses = cpu_data->states[i].misses;
+   idx0 = i;
}
 
if (s->target_residency_ns > duration_ns)
@@ -374,11 +376,16 @@ static int teo_select(struct cpuidle_dri
 
if (idx < 0) {
idx = 0; /* No states enabled. Must use 0. */
-   } else if (idx > 0) {
+   } else if (idx > idx0) {
unsigned int count = 0;
u64 sum = 0;
 
/*
+* The target residencies of at least two different enabled idle
+* states are less than or equal to the current expected idle
+* duration.  Try to refine the selection using the most recent
+* measured idle duration values.
+*
 * Count and sum the most recent idle duration values less than
 * the current expected idle duration value.
 */
@@ -426,7 +433,8 @@ static int teo_select(struct cpuidle_dri
 * till the closest timer including the tick, try to correct
 * that.
 */
-   if (idx > 0 && drv->states[idx].target_residency_ns > 
delta_tick)
+   if (idx > idx0 &&
+   drv->states[idx].target_residency_ns > delta_tick)
idx = teo_find_shallower_state(drv, dev, idx, 
delta_tick);
}
 





[PATCH v1 2/5] cpuidle: Use s64 as exit_latency_ns and target_residency_ns data type

2021-03-29 Thread Rafael J. Wysocki
From: "Rafael J. Wysocki" 

Subsequent changes will cause the exit_latency_ns and target_residency_ns
fields in struct cpuidle_state to be used in computations in which data
type conversions to u64 may turn a negative number close to zero into
a verly large positive number leading to incorrect results.

In preparation for that, change the data type of the fields mentioned
above to s64, but ensure that they will not be negative themselves.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki 
---
 drivers/cpuidle/driver.c |4 
 include/linux/cpuidle.h  |4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

Index: linux-pm/include/linux/cpuidle.h
===
--- linux-pm.orig/include/linux/cpuidle.h
+++ linux-pm/include/linux/cpuidle.h
@@ -49,8 +49,8 @@ struct cpuidle_state {
charname[CPUIDLE_NAME_LEN];
chardesc[CPUIDLE_DESC_LEN];
 
-   u64 exit_latency_ns;
-   u64 target_residency_ns;
+   s64 exit_latency_ns;
+   s64 target_residency_ns;
unsigned intflags;
unsigned intexit_latency; /* in US */
int power_usage; /* in mW */
Index: linux-pm/drivers/cpuidle/driver.c
===
--- linux-pm.orig/drivers/cpuidle/driver.c
+++ linux-pm/drivers/cpuidle/driver.c
@@ -181,9 +181,13 @@ static void __cpuidle_driver_init(struct
 */
if (s->target_residency > 0)
s->target_residency_ns = s->target_residency * 
NSEC_PER_USEC;
+   else if (s->target_residency_ns < 0)
+   s->target_residency_ns = 0;
 
if (s->exit_latency > 0)
s->exit_latency_ns = s->exit_latency * NSEC_PER_USEC;
+   else if (s->exit_latency_ns < 0)
+   s->exit_latency_ns =  0;
}
 }
 





[PATCH v1 3/5] cpuidle: teo: Adjust handling of very short idle times

2021-03-29 Thread Rafael J. Wysocki
From: "Rafael J. Wysocki" 

If the time till the next timer event is shorter than the target
residency of the first idle state (state 0), the TEO governor does
not update its metrics for any idle states, but arguably it should
record a "hit" for idle state 0 in that case, so modify it to do
that.

Accordingly, also make it record an "early hit" for idle state 0 if
the measured idle duration is less than its target residency, which
allows one branch more to be dropped from teo_update().

Signed-off-by: Rafael J. Wysocki 
---
 drivers/cpuidle/governors/teo.c |   32 +++-
 1 file changed, 15 insertions(+), 17 deletions(-)

Index: linux-pm/drivers/cpuidle/governors/teo.c
===
--- linux-pm.orig/drivers/cpuidle/governors/teo.c
+++ linux-pm/drivers/cpuidle/governors/teo.c
@@ -117,7 +117,8 @@ static DEFINE_PER_CPU(struct teo_cpu, te
 static void teo_update(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
struct teo_cpu *cpu_data = per_cpu_ptr(_cpus, dev->cpu);
-   int i, idx_hit = -1, idx_timer = -1;
+   int i, idx_hit = 0, idx_timer = 0;
+   unsigned int hits, misses;
u64 measured_ns;
 
if (cpu_data->time_span_ns >= cpu_data->sleep_length_ns) {
@@ -174,25 +175,22 @@ static void teo_update(struct cpuidle_dr
 * also increase the "early hits" metric for the state that actually
 * matches the measured idle duration.
 */
-   if (idx_timer >= 0) {
-   unsigned int hits = cpu_data->states[idx_timer].hits;
-   unsigned int misses = cpu_data->states[idx_timer].misses;
-
-   hits -= hits >> DECAY_SHIFT;
-   misses -= misses >> DECAY_SHIFT;
-
-   if (idx_timer > idx_hit) {
-   misses += PULSE;
-   if (idx_hit >= 0)
-   cpu_data->states[idx_hit].early_hits += PULSE;
-   } else {
-   hits += PULSE;
-   }
+   hits = cpu_data->states[idx_timer].hits;
+   hits -= hits >> DECAY_SHIFT;
+
+   misses = cpu_data->states[idx_timer].misses;
+   misses -= misses >> DECAY_SHIFT;
 
-   cpu_data->states[idx_timer].misses = misses;
-   cpu_data->states[idx_timer].hits = hits;
+   if (idx_timer == idx_hit) {
+   hits += PULSE;
+   } else {
+   misses += PULSE;
+   cpu_data->states[idx_hit].early_hits += PULSE;
}
 
+   cpu_data->states[idx_timer].misses = misses;
+   cpu_data->states[idx_timer].hits = hits;
+
/*
 * Save idle duration values corresponding to non-timer wakeups for
 * pattern detection.





[PATCH v1 5/5] cpuidle: menu: Take negative "sleep length" values into account

2021-03-29 Thread Rafael J. Wysocki
From: "Rafael J. Wysocki" 
Subject: [PATCH] cpuidle: menu: Take negative "sleep length" values into account

Make the menu governor check the tick_nohz_get_next_hrtimer()
return value so as to avoid dealing with negative "sleep length"
values and make it use that value directly when the tick is stopped.

While at it, rename local variable delta_next in menu_select() to
delta_tick which better reflects its purpose.

Signed-off-by: Rafael J. Wysocki 
---
 drivers/cpuidle/governors/menu.c |   17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/cpuidle/governors/menu.c
===
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -271,7 +271,7 @@ static int menu_select(struct cpuidle_dr
u64 predicted_ns;
u64 interactivity_req;
unsigned long nr_iowaiters;
-   ktime_t delta_next;
+   ktime_t delta, delta_tick;
int i, idx;
 
if (data->needs_update) {
@@ -280,7 +280,12 @@ static int menu_select(struct cpuidle_dr
}
 
/* determine the expected residency time, round up */
-   data->next_timer_ns = tick_nohz_get_sleep_length(_next);
+   delta = tick_nohz_get_sleep_length(_tick);
+   if (unlikely(delta < 0)) {
+   delta = 0;
+   delta_tick = 0;
+   }
+   data->next_timer_ns = delta;
 
nr_iowaiters = nr_iowait_cpu(dev->cpu);
data->bucket = which_bucket(data->next_timer_ns, nr_iowaiters);
@@ -318,7 +323,7 @@ static int menu_select(struct cpuidle_dr
 * state selection.
 */
if (predicted_ns < TICK_NSEC)
-   predicted_ns = delta_next;
+   predicted_ns = data->next_timer_ns;
} else {
/*
 * Use the performance multiplier and the user-configurable
@@ -377,7 +382,7 @@ static int menu_select(struct cpuidle_dr
 * stuck in the shallow one for too long.
 */
if (drv->states[idx].target_residency_ns < TICK_NSEC &&
-   s->target_residency_ns <= delta_next)
+   s->target_residency_ns <= delta_tick)
idx = i;
 
return idx;
@@ -399,7 +404,7 @@ static int menu_select(struct cpuidle_dr
 predicted_ns < TICK_NSEC) && !tick_nohz_tick_stopped()) {
*stop_tick = false;
 
-   if (idx > 0 && drv->states[idx].target_residency_ns > 
delta_next) {
+   if (idx > 0 && drv->states[idx].target_residency_ns > 
delta_tick) {
/*
 * The tick is not going to be stopped and the target
 * residency of the state to be returned is not within
@@ -411,7 +416,7 @@ static int menu_select(struct cpuidle_dr
continue;
 
idx = i;
-   if (drv->states[i].target_residency_ns <= 
delta_next)
+   if (drv->states[i].target_residency_ns <= 
delta_tick)
break;
}
}





Re: [PATCH] ACPI: processor: Fix CPU0 wakeup in acpi_idle_play_dead()

2021-03-29 Thread Rafael J. Wysocki
On Wed, Mar 24, 2021 at 4:37 PM Rafael J. Wysocki  wrote:
>
> On Wed, Mar 24, 2021 at 4:23 PM Vitaly Kuznetsov  wrote:
> >
> > Commit 496121c02127 ("ACPI: processor: idle: Allow probing on platforms
> > with one ACPI C-state") broke CPU0 hotplug on certain systems, e.g.
> > I'm observing the following on AWS Nitro (e.g r5b.xlarge but other
> > instance types are affected as well):
> >
> >  # echo 0 > /sys/devices/system/cpu/cpu0/online
> >  # echo 1 > /sys/devices/system/cpu/cpu0/online
> >  <10 seconds delay>
> >  -bash: echo: write error: Input/output error
> >
> > In fact, the above mentioned commit only revealed the problem and did
> > not introduce it. On x86, to wakeup CPU an NMI is being used and
> > hlt_play_dead()/mwait_play_dead() loops are prepared to handle it:
> >
> > /*
> >  * If NMI wants to wake up CPU0, start CPU0.
> >  */
> > if (wakeup_cpu0())
> > start_cpu0();
> >
> > cpuidle_play_dead() -> acpi_idle_play_dead() (which is now being called on
> > systems where it wasn't called before the above mentioned commit) serves
> > the same purpose but it doesn't have a path for CPU0. What happens now on
> > wakeup is:
> > - NMI is sent to CPU0
> > - wakeup_cpu0_nmi() works as expected
> > - we get back to while (1) loop in acpi_idle_play_dead()
> > - safe_halt() puts CPU0 to sleep again.
> >
> > The straightforward/minimal fix is add the special handling for CPU0 on x86
> > and that's what the patch is doing.
> >
> > Fixes: 496121c02127 ("ACPI: processor: idle: Allow probing on platforms 
> > with one ACPI C-state")
> > Signed-off-by: Vitaly Kuznetsov 
>
> This looks reasonable to me, but I'll give others some time to respond.

No comments, so applied as 5.12-rc material, thanks!

> > ---
> >  arch/x86/include/asm/smp.h| 1 +
> >  arch/x86/kernel/smpboot.c | 2 +-
> >  drivers/acpi/processor_idle.c | 7 +++
> >  3 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> > index c0538f82c9a2..57ef2094af93 100644
> > --- a/arch/x86/include/asm/smp.h
> > +++ b/arch/x86/include/asm/smp.h
> > @@ -132,6 +132,7 @@ void native_play_dead(void);
> >  void play_dead_common(void);
> >  void wbinvd_on_cpu(int cpu);
> >  int wbinvd_on_all_cpus(void);
> > +bool wakeup_cpu0(void);
> >
> >  void native_smp_send_reschedule(int cpu);
> >  void native_send_call_func_ipi(const struct cpumask *mask);
> > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> > index 02813a7f3a7c..f877150a91da 100644
> > --- a/arch/x86/kernel/smpboot.c
> > +++ b/arch/x86/kernel/smpboot.c
> > @@ -1659,7 +1659,7 @@ void play_dead_common(void)
> > local_irq_disable();
> >  }
> >
> > -static bool wakeup_cpu0(void)
> > +bool wakeup_cpu0(void)
> >  {
> > if (smp_processor_id() == 0 && enable_start_cpu0)
> > return true;
> > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> > index d93e400940a3..eb1101b16c08 100644
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -29,6 +29,7 @@
> >   */
> >  #ifdef CONFIG_X86
> >  #include 
> > +#include 
> >  #endif
> >
> >  #define _COMPONENT  ACPI_PROCESSOR_COMPONENT
> > @@ -541,6 +542,12 @@ static int acpi_idle_play_dead(struct cpuidle_device 
> > *dev, int index)
> > wait_for_freeze();
> > } else
> > return -ENODEV;
> > +
> > +#ifdef CONFIG_X86
> > +   /* If NMI wants to wake up CPU0, start CPU0. */
> > +   if (wakeup_cpu0())
> > +   start_cpu0();
> > +#endif
> > }
> >
> > /* Never reached */
> > --
> > 2.30.2
> >


Re: [PATCH v3 00/12] acpi: fix some coding style issues

2021-03-29 Thread Rafael J. Wysocki
On Sat, Mar 27, 2021 at 1:11 PM Xiaofei Tan  wrote:
>
> Fix some coding style issues reported by checkpatch.pl.
> Only cleanup and no function changes.
>
> Differences from v2 to v3:
> - Remove the modifications that may cause function change.
>
> Differences from v1 to v2:
> - Add subsystem and module name in the name of patch 05/15.
> - Change to use more proper module name for some patch names.
>
> Xiaofei Tan (12):
>   ACPI: APD: fix a block comment align issue
>   ACPI: processor: fix some coding style issues
>   ACPI: ipmi: remove useless return statement for void function
>   ACPI: LPSS: add a missed blank line after declarations
>   ACPI: acpi_pad: add a missed blank line after declarations
>   ACPI: battery: fix some coding style issues
>   ACPI: button: fix some coding style issues
>   ACPI: CPPC: fix some coding style issues
>   ACPI: custom_method: fix a coding style issue
>   ACPI: PM: add a missed blank line after declarations
>   ACPI: sysfs: fix some coding style issues
>   ACPI: dock: fix some coding style issues
>
>  drivers/acpi/acpi_apd.c   |  8 ++---
>  drivers/acpi/acpi_ipmi.c  |  1 -
>  drivers/acpi/acpi_lpss.c  |  2 ++
>  drivers/acpi/acpi_pad.c   |  4 +++
>  drivers/acpi/acpi_processor.c | 18 +++
>  drivers/acpi/battery.c| 63 --
>  drivers/acpi/button.c |  9 ++
>  drivers/acpi/cppc_acpi.c  | 71 
> ++-
>  drivers/acpi/custom_method.c  |  2 +-
>  drivers/acpi/device_pm.c  |  3 ++
>  drivers/acpi/device_sysfs.c   | 15 ++---
>  drivers/acpi/dock.c   |  7 +++--
>  12 files changed, 106 insertions(+), 97 deletions(-)
>
> --

Can you please stop sending new versions of this for a while?

You've sent three of them over the last weekend and honestly I haven't
had a chance to look at the first one even.


Re: [PATCH] ACPICA: Fix a typo

2021-03-29 Thread Rafael J. Wysocki
On Fri, Mar 26, 2021 at 1:22 AM Bhaskar Chowdhury  wrote:
>
>
> s/optimzation/optimization/
>
> Signed-off-by: Bhaskar Chowdhury 
> ---
>  include/acpi/acoutput.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/acpi/acoutput.h b/include/acpi/acoutput.h
> index 1538a6853822..1b4c45815695 100644
> --- a/include/acpi/acoutput.h
> +++ b/include/acpi/acoutput.h
> @@ -362,7 +362,7 @@
>   *
>   * A less-safe version of the macros is provided for optional use if the
>   * compiler uses excessive CPU stack (for example, this may happen in the
> - * debug case if code optimzation is disabled.)
> + * debug case if code optimization is disabled.)
>   */
>
>  /* Exit trace helper macro */
> --

Erik, could you pick up this patch, please?  It is simple enough IMV ...


Re: 回复: [PATCH 01/10] tick/nohz: Prevent tick_nohz_get_sleep_length() from returning negative value

2021-03-29 Thread Rafael J. Wysocki
On Fri, Mar 26, 2021 at 11:53 PM Zhou Ti (x2019cwm)  wrote:
>
> On Fri, 26 Mar 2021 19:54:26 +0100, Rafael J. Wysocki wrote:
> > On Fri, Mar 26, 2021 at 6:53 PM Zhou Ti (x2019cwm)  wrote:
> > >
> > > On Fri, 26 Mar 2021 18:01:47 +0100, Rafael J. Wysocki wrote:
> > > > On Thu, Mar 25, 2021 at 9:37 PM Zhou Ti (x2019cwm)  
> > > > wrote:
> > > > >
> > > > > On March 25, 2021 15:50, Rafael J. Wysocki wrote:
> > > > > > On Thu, Mar 25, 2021 at 8:18 PM Zhou Ti (x2019cwm) 
> > > > > >  wrote:
> > > > > > >
> > > > > > > On March 25, 2021 14:56, Rafael J. Wysocki wrote:
> > > > > > > > On Thursday, March 25, 2021 2:14:00 PM CET Frederic Weisbecker 
> > > > > > > > wrote:
> > > > > > > > > On Tue, Mar 16, 2021 at 04:08:08PM +, Zhou Ti (x2019cwm) 
> > > > > > > > > wrote:
> > > > > > > > > > But I don't think it's a good idea to handle this in 
> > > > > > > > > > callers, because logically the function shouldn't return 
> > > > > > > > > > negative values. Returning 0 directly would allow idle 
> > > > > > > > > > governors to get another chance to select again.
> > > > > > > > >
> > > > > > > > > Hmm, I'm going to leave the last word to Rafael since cpuidle 
> > > > > > > > > are the only
> > > > > > > > > callers of this. In any case we need to fix it.
> > > > > > > >
> > > > > > > > Yes, we do.
> > > > > > > >
> > > > > > > > So I said that I preferred to address this in the callers and 
> > > > > > > > the reason why
> > > > > > > > is because, for example, for the teo governor it would be a 
> > > > > > > > matter of using
> > > > > > > > a different data type to store the tick_nohz_get_sleep_length() 
> > > > > > > > return value,
> > > > > > > > like in the (untested) patch below.
> > > > > > > >
> > > > > > > > So at least in this case there is no need to add any new 
> > > > > > > > branches anywhere.
> > > > > > > >
> > > > > > > > I'm still not sure about menu, because it is more complicated, 
> > > > > > > > but even if
> > > > > > > > that one needs an extra branch, that is a win already.
> > > > > > >
> > > > > > > I would like to point out the potential trouble that fixing this 
> > > > > > > issue in the
> > > > > > > callers could cause.
> > > > > > >
> > > > > > > 1. This function is called multiple times in menu governor and TEO
> > > > > > > governor.
> > > > > >
> > > > > > What do you mean by "multiple times"?
> > > > > >
> > > > > > Each of the governors calls it once per cycle and its previous 
> > > > > > return
> > > > > > value is not used in the next cycle at least in teo.
> > > > >
> > > > > I remember a governor called this function twice in a cycle, I guess 
> > > > > I remember
> > > > > wrong.
> > > >
> > > > That obviously depends on the governor, but both teo and menu call it
> > > > once per cycle.
> > > >
> > > > > > > I'm not sure that receiving results using signed integers is 
> > > > > > > enough
> > > > > > > to solve all the problems, in the worst case it may require 
> > > > > > > increasing
> > > > > > > the logical complexity of the code.
> > > > > >
> > > > > > That is a valid concern, so it is a tradeoff between increasing the
> > > > > > logical complexity of the code and adding branches to it.
> > > > > >
> > > > > > > 2. This function is important for developing idle governor.
> > > > > > > If the problem is not fixed in the function itself, then this 
> > > > > > > potential
> > > > > > > pitfall should be explicitly stated in the documentation.
> &

Re: 回复: [PATCH 01/10] tick/nohz: Prevent tick_nohz_get_sleep_length() from returning negative value

2021-03-26 Thread Rafael J. Wysocki
On Fri, Mar 26, 2021 at 6:53 PM Zhou Ti (x2019cwm)  wrote:
>
> On Fri, 26 Mar 2021 18:01:47 +0100, Rafael J. Wysocki wrote:
> > On Thu, Mar 25, 2021 at 9:37 PM Zhou Ti (x2019cwm)  wrote:
> > >
> > > On March 25, 2021 15:50, Rafael J. Wysocki wrote:
> > > > On Thu, Mar 25, 2021 at 8:18 PM Zhou Ti (x2019cwm)  
> > > > wrote:
> > > > >
> > > > > On March 25, 2021 14:56, Rafael J. Wysocki wrote:
> > > > > > On Thursday, March 25, 2021 2:14:00 PM CET Frederic Weisbecker 
> > > > > > wrote:
> > > > > > > On Tue, Mar 16, 2021 at 04:08:08PM +, Zhou Ti (x2019cwm) 
> > > > > > > wrote:
> > > > > > > > But I don't think it's a good idea to handle this in callers, 
> > > > > > > > because logically the function shouldn't return negative 
> > > > > > > > values. Returning 0 directly would allow idle governors to get 
> > > > > > > > another chance to select again.
> > > > > > >
> > > > > > > Hmm, I'm going to leave the last word to Rafael since cpuidle are 
> > > > > > > the only
> > > > > > > callers of this. In any case we need to fix it.
> > > > > >
> > > > > > Yes, we do.
> > > > > >
> > > > > > So I said that I preferred to address this in the callers and the 
> > > > > > reason why
> > > > > > is because, for example, for the teo governor it would be a matter 
> > > > > > of using
> > > > > > a different data type to store the tick_nohz_get_sleep_length() 
> > > > > > return value,
> > > > > > like in the (untested) patch below.
> > > > > >
> > > > > > So at least in this case there is no need to add any new branches 
> > > > > > anywhere.
> > > > > >
> > > > > > I'm still not sure about menu, because it is more complicated, but 
> > > > > > even if
> > > > > > that one needs an extra branch, that is a win already.
> > > > >
> > > > > I would like to point out the potential trouble that fixing this 
> > > > > issue in the
> > > > > callers could cause.
> > > > >
> > > > > 1. This function is called multiple times in menu governor and TEO
> > > > > governor.
> > > >
> > > > What do you mean by "multiple times"?
> > > >
> > > > Each of the governors calls it once per cycle and its previous return
> > > > value is not used in the next cycle at least in teo.
> > >
> > > I remember a governor called this function twice in a cycle, I guess I 
> > > remember
> > > wrong.
> >
> > That obviously depends on the governor, but both teo and menu call it
> > once per cycle.
> >
> > > > > I'm not sure that receiving results using signed integers is enough
> > > > > to solve all the problems, in the worst case it may require increasing
> > > > > the logical complexity of the code.
> > > >
> > > > That is a valid concern, so it is a tradeoff between increasing the
> > > > logical complexity of the code and adding branches to it.
> > > >
> > > > > 2. This function is important for developing idle governor.
> > > > > If the problem is not fixed in the function itself, then this 
> > > > > potential
> > > > > pitfall should be explicitly stated in the documentation.
> > > >
> > > > That I can agree with.
> > > >
> > > > > This is because
> > > > > it is difficult to predict from the definition and naming of the 
> > > > > function
> > > > > that it might return a negative number. I actually discovered this 
> > > > > anomaly
> > > > > when I was doing data analysis on my own idle governor. For some idle 
> > > > > control
> > > > > algorithms, this exception return could lead to serious consequences,
> > > > > because negative return logically won't happen.
> > > >
> > > > Well, it's a matter of how to take the possible negative return value
> > > > into account so it does not affect the result of the computations.
> > >
> > > I think it is challenging for some algorithms to take negative 

Re: 回复: [PATCH 01/10] tick/nohz: Prevent tick_nohz_get_sleep_length() from returning negative value

2021-03-26 Thread Rafael J. Wysocki
On Thu, Mar 25, 2021 at 9:37 PM Zhou Ti (x2019cwm)  wrote:
>
> On March 25, 2021 15:50, Rafael J. Wysocki wrote:
> > On Thu, Mar 25, 2021 at 8:18 PM Zhou Ti (x2019cwm)  wrote:
> > >
> > > On March 25, 2021 14:56, Rafael J. Wysocki wrote:
> > > > On Thursday, March 25, 2021 2:14:00 PM CET Frederic Weisbecker wrote:
> > > > > On Tue, Mar 16, 2021 at 04:08:08PM +, Zhou Ti (x2019cwm) wrote:
> > > > > > But I don't think it's a good idea to handle this in callers, 
> > > > > > because logically the function shouldn't return negative values. 
> > > > > > Returning 0 directly would allow idle governors to get another 
> > > > > > chance to select again.
> > > > >
> > > > > Hmm, I'm going to leave the last word to Rafael since cpuidle are the 
> > > > > only
> > > > > callers of this. In any case we need to fix it.
> > > >
> > > > Yes, we do.
> > > >
> > > > So I said that I preferred to address this in the callers and the 
> > > > reason why
> > > > is because, for example, for the teo governor it would be a matter of 
> > > > using
> > > > a different data type to store the tick_nohz_get_sleep_length() return 
> > > > value,
> > > > like in the (untested) patch below.
> > > >
> > > > So at least in this case there is no need to add any new branches 
> > > > anywhere.
> > > >
> > > > I'm still not sure about menu, because it is more complicated, but even 
> > > > if
> > > > that one needs an extra branch, that is a win already.
> > >
> > > I would like to point out the potential trouble that fixing this issue in 
> > > the
> > > callers could cause.
> > >
> > > 1. This function is called multiple times in menu governor and TEO
> > > governor.
> >
> > What do you mean by "multiple times"?
> >
> > Each of the governors calls it once per cycle and its previous return
> > value is not used in the next cycle at least in teo.
>
> I remember a governor called this function twice in a cycle, I guess I 
> remember
> wrong.

That obviously depends on the governor, but both teo and menu call it
once per cycle.

> > > I'm not sure that receiving results using signed integers is enough
> > > to solve all the problems, in the worst case it may require increasing
> > > the logical complexity of the code.
> >
> > That is a valid concern, so it is a tradeoff between increasing the
> > logical complexity of the code and adding branches to it.
> >
> > > 2. This function is important for developing idle governor.
> > > If the problem is not fixed in the function itself, then this potential
> > > pitfall should be explicitly stated in the documentation.
> >
> > That I can agree with.
> >
> > > This is because
> > > it is difficult to predict from the definition and naming of the function
> > > that it might return a negative number. I actually discovered this anomaly
> > > when I was doing data analysis on my own idle governor. For some idle 
> > > control
> > > algorithms, this exception return could lead to serious consequences,
> > > because negative return logically won't happen.
> >
> > Well, it's a matter of how to take the possible negative return value
> > into account so it does not affect the result of the computations.
>
> I think it is challenging for some algorithms to take negative return values
> into account properly. For TEO (and even menu), it is possible to
> solve the problem by just changing the way the data is received is because the
> learning mechanism for both algorithms is simple.

Of course this depends on the governor.

> One of the interesting things about the CPUIdle subsystem is that it is well
> suited to introduce machine learning and probabilistic statistical methods.

You need to remember that the governor code runs in the idle loop
context which is expected to be reasonably fast.

That's why we are worrying about individual branches here.

> This means that many of the more complex and data-sensitive algorithms can
> potentially be explored. In the best case we will still need to add additional
> code complexity to a new algorithm.

So I'm not sure what the problem with adding an upfront negative value
check to the governor is.

> It would reduce a lot of unnecessary considerations (for example, highlight
> this shortcoming in the documentation) if we could ensure that this function
>

Re: [PATCH] thermal/drivers/netlink: Add the temperature when crossing a trip point

2021-03-26 Thread Rafael J. Wysocki
On Thu, Mar 25, 2021 at 8:38 PM Daniel Lezcano
 wrote:
>
> The slope of the temperature increase or decrease can be high and when
> the temperature crosses the trip point, there could be a significant
> difference between the trip temperature and the measured temperatures.
>
> That forces the userspace to read the temperature back right after
> receiving a trip violation notification.
>
> In order to be efficient, give the temperature which resulted in the
> trip violation.
>
> Signed-off-by: Daniel Lezcano 

Srinivas, what do you think?

> ---
>  drivers/thermal/thermal_core.c|  6 --
>  drivers/thermal/thermal_netlink.c | 11 ++-
>  drivers/thermal/thermal_netlink.h |  8 
>  include/uapi/linux/thermal.h  |  2 +-
>  4 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 996c038f83a4..948020ef51b1 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -430,10 +430,12 @@ static void handle_thermal_trip(struct 
> thermal_zone_device *tz, int trip)
> if (tz->last_temperature != THERMAL_TEMP_INVALID) {
> if (tz->last_temperature < trip_temp &&
> tz->temperature >= trip_temp)
> -   thermal_notify_tz_trip_up(tz->id, trip);
> +   thermal_notify_tz_trip_up(tz->id, trip,
> + tz->temperature);
> if (tz->last_temperature >= trip_temp &&
> tz->temperature < (trip_temp - hyst))
> -   thermal_notify_tz_trip_down(tz->id, trip);
> +   thermal_notify_tz_trip_down(tz->id, trip,
> +   tz->temperature);
> }
>
> if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
> diff --git a/drivers/thermal/thermal_netlink.c 
> b/drivers/thermal/thermal_netlink.c
> index 1234dbe95895..a16dd4d5d710 100644
> --- a/drivers/thermal/thermal_netlink.c
> +++ b/drivers/thermal/thermal_netlink.c
> @@ -121,7 +121,8 @@ static int thermal_genl_event_tz(struct param *p)
>  static int thermal_genl_event_tz_trip_up(struct param *p)
>  {
> if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id) ||
> -   nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TRIP_ID, p->trip_id))
> +   nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TRIP_ID, p->trip_id) ||
> +   nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TEMP, p->temp))
> return -EMSGSIZE;
>
> return 0;
> @@ -285,16 +286,16 @@ int thermal_notify_tz_disable(int tz_id)
> return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_DISABLE, );
>  }
>
> -int thermal_notify_tz_trip_down(int tz_id, int trip_id)
> +int thermal_notify_tz_trip_down(int tz_id, int trip_id, int temp)
>  {
> -   struct param p = { .tz_id = tz_id, .trip_id = trip_id };
> +   struct param p = { .tz_id = tz_id, .trip_id = trip_id, .temp = temp };
>
> return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_DOWN, );
>  }
>
> -int thermal_notify_tz_trip_up(int tz_id, int trip_id)
> +int thermal_notify_tz_trip_up(int tz_id, int trip_id, int temp)
>  {
> -   struct param p = { .tz_id = tz_id, .trip_id = trip_id };
> +   struct param p = { .tz_id = tz_id, .trip_id = trip_id, .temp = temp };
>
> return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_UP, );
>  }
> diff --git a/drivers/thermal/thermal_netlink.h 
> b/drivers/thermal/thermal_netlink.h
> index 828d1dddfa98..e554f76291f4 100644
> --- a/drivers/thermal/thermal_netlink.h
> +++ b/drivers/thermal/thermal_netlink.h
> @@ -11,8 +11,8 @@ int thermal_notify_tz_create(int tz_id, const char *name);
>  int thermal_notify_tz_delete(int tz_id);
>  int thermal_notify_tz_enable(int tz_id);
>  int thermal_notify_tz_disable(int tz_id);
> -int thermal_notify_tz_trip_down(int tz_id, int id);
> -int thermal_notify_tz_trip_up(int tz_id, int id);
> +int thermal_notify_tz_trip_down(int tz_id, int id, int temp);
> +int thermal_notify_tz_trip_up(int tz_id, int id, int temp);
>  int thermal_notify_tz_trip_delete(int tz_id, int id);
>  int thermal_notify_tz_trip_add(int tz_id, int id, int type,
>int temp, int hyst);
> @@ -49,12 +49,12 @@ static inline int thermal_notify_tz_disable(int tz_id)
> return 0;
>  }
>
> -static inline int thermal_notify_tz_trip_down(int tz_id, int id)
> +static inline int thermal_notify_tz_trip_down(int tz_id, int id, int temp)
>  {
> return 0;
>  }
>
> -static inline int thermal_notify_tz_trip_up(int tz_id, int id)
> +static inline int thermal_notify_tz_trip_up(int tz_id, int id, int temp)
>  {
> return 0;
>  }
> diff --git a/include/uapi/linux/thermal.h b/include/uapi/linux/thermal.h
> index c105054cbb57..bf5d9c8ef16f 100644
> --- a/include/uapi/linux/thermal.h
> +++ b/include/uapi/linux/thermal.h
> @@ -18,7 +18,7 @@ enum 

[GIT PULL] ACPI fixes for v5.12-rc5

2021-03-26 Thread Rafael J. Wysocki
Hi Linus,

Please pull from the tag

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 acpi-5.12-rc5

with top-most commit e1db18b59729e24f001459b98955019344d5b12b

 Merge branches 'acpi-video' and 'acpi-scan'

on top of commit 0d02ec6b3136c73c09e7859f0d0e4e2c4c07b49b

 Linux 5.12-rc4

to receive ACPI fixes for 5.12-rc5.

These fix a memory management regression in ACPICA, repair an ACPI
blacklist entry damaged inadvertently during the 5.11 cycle and fix
the bookkeeping of devices with the same primary device ID in the
ACPI core.

Specifics:

 - Make ACPICA use the same object cache consistently when allocating
   and freeing objects (Vegard Nossum).

 - Add a callback pointer removed inadvertently during the 5.11 cycle
   to the ACPI backlight blacklist entry for Sony VPCEH3U1E (Chris
   Chiu).

 - Make the ACPI device enumeration core use IDA for creating names of
   ACPI device objects with the same primary device ID to avoid using
   duplicate device object names in some cases (Andy Shevchenko).

Thanks!


---

Andy Shevchenko (1):
  ACPI: scan: Use unique number for instance_no

Chris Chiu (1):
  ACPI: video: Add missing callback back for Sony VPCEH3U1E

Vegard Nossum (1):
  ACPICA: Always create namespace nodes using acpi_ns_create_node()

---

 drivers/acpi/acpica/nsaccess.c |  3 +--
 drivers/acpi/internal.h|  6 +-
 drivers/acpi/scan.c| 33 -
 drivers/acpi/video_detect.c|  1 +
 include/acpi/acpi_bus.h|  1 +
 5 files changed, 36 insertions(+), 8 deletions(-)


[GIT PULL] Power management fixes for v5.12-rc5

2021-03-26 Thread Rafael J. Wysocki
Hi Linus,

Please pull from the tag

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 pm-5.12-rc5

with top-most commit 6f3a283c2f65a9a6801a0befa61cb60195f1

 Merge branch 'pm-em'

on top of commit 0d02ec6b3136c73c09e7859f0d0e4e2c4c07b49b

 Linux 5.12-rc4

to receive power management fixes for 5.12-rc5.

These fix an issue related to device links in the runtime PM
framework and debugfs usage in the Energy Model code.

Specifics:

 - Modify the runtime PM device suspend to avoid suspending
   supplier devices before the consumer device's status changes
   to RPM_SUSPENDED (Rafael Wysocki).

 - Change the Energy Model code to prevent it from attempting to
   create its main debugfs directory too early (Lukasz Luba).

Thanks!


---

Lukasz Luba (1):
  PM: EM: postpone creating the debugfs dir till fs_initcall

Rafael J. Wysocki (1):
  PM: runtime: Defer suspending suppliers

---

 drivers/base/power/runtime.c | 45 ++--
 kernel/power/energy_model.c  |  2 +-
 2 files changed, 40 insertions(+), 7 deletions(-)


Re: 回复: [PATCH 01/10] tick/nohz: Prevent tick_nohz_get_sleep_length() from returning negative value

2021-03-25 Thread Rafael J. Wysocki
On Thu, Mar 25, 2021 at 8:18 PM Zhou Ti (x2019cwm)  wrote:
>
> On March 25, 2021 14:56, Rafael J. Wysocki wrote:
> > On Thursday, March 25, 2021 2:14:00 PM CET Frederic Weisbecker wrote:
> > > On Tue, Mar 16, 2021 at 04:08:08PM +, Zhou Ti (x2019cwm) wrote:
> > > > But I don't think it's a good idea to handle this in callers, because 
> > > > logically the function shouldn't return negative values. Returning 0 
> > > > directly would allow idle governors to get another chance to select 
> > > > again.
> > >
> > > Hmm, I'm going to leave the last word to Rafael since cpuidle are the only
> > > callers of this. In any case we need to fix it.
> >
> > Yes, we do.
> >
> > So I said that I preferred to address this in the callers and the reason why
> > is because, for example, for the teo governor it would be a matter of using
> > a different data type to store the tick_nohz_get_sleep_length() return 
> > value,
> > like in the (untested) patch below.
> >
> > So at least in this case there is no need to add any new branches anywhere.
> >
> > I'm still not sure about menu, because it is more complicated, but even if
> > that one needs an extra branch, that is a win already.
>
> I would like to point out the potential trouble that fixing this issue in the
> callers could cause.
>
> 1. This function is called multiple times in menu governor and TEO
> governor.

What do you mean by "multiple times"?

Each of the governors calls it once per cycle and its previous return
value is not used in the next cycle at least in teo.

> I'm not sure that receiving results using signed integers is enough
> to solve all the problems, in the worst case it may require increasing
> the logical complexity of the code.

That is a valid concern, so it is a tradeoff between increasing the
logical complexity of the code and adding branches to it.

> 2. This function is important for developing idle governor.
> If the problem is not fixed in the function itself, then this potential
> pitfall should be explicitly stated in the documentation.

That I can agree with.

> This is because
> it is difficult to predict from the definition and naming of the function
> that it might return a negative number. I actually discovered this anomaly
> when I was doing data analysis on my own idle governor. For some idle control
> algorithms, this exception return could lead to serious consequences,
> because negative return logically won't happen.

Well, it's a matter of how to take the possible negative return value
into account so it does not affect the result of the computations.

> >
> > ---
> >  drivers/cpuidle/governors/teo.c |8 
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > Index: linux-pm/drivers/cpuidle/governors/teo.c
> > ===
> > --- linux-pm.orig/drivers/cpuidle/governors/teo.c
> > +++ linux-pm/drivers/cpuidle/governors/teo.c
> > @@ -100,8 +100,8 @@ struct teo_idle_state {
> >   * @intervals: Saved idle duration values.
> >   */
> >  struct teo_cpu {
> > -   u64 time_span_ns;
> > -   u64 sleep_length_ns;
> > +   s64 time_span_ns;
> > +   s64 sleep_length_ns;
> > struct teo_idle_state states[CPUIDLE_STATE_MAX];
> > int interval_idx;
> > u64 intervals[INTERVALS];
> > @@ -216,7 +216,7 @@ static bool teo_time_ok(u64 interval_ns)
> >   */
> >  static int teo_find_shallower_state(struct cpuidle_driver *drv,
> > struct cpuidle_device *dev, int 
> > state_idx,
> > -   u64 duration_ns)
> > +   s64 duration_ns)
> >  {
> > int i;
> >
> > @@ -242,7 +242,7 @@ static int teo_select(struct cpuidle_dri
> >  {
> > struct teo_cpu *cpu_data = per_cpu_ptr(_cpus, dev->cpu);
> > s64 latency_req = cpuidle_governor_latency_req(dev->cpu);
> > -   u64 duration_ns;
> > +   s64 duration_ns;
> > unsigned int hits, misses, early_hits;
> > int max_early_idx, prev_max_early_idx, constraint_idx, idx, i;
> > ktime_t delta_tick;
>


[PATCH] PCI: ACPI: PM: Fix debug message in acpi_pci_set_power_state()

2021-03-25 Thread Rafael J. Wysocki
From: Rafael J. Wysocki 

If PCI_D3cold is passed to acpi_pci_set_power_state() as the second
argument and there is no ACPI D3cold support for the given device,
the debug message printed by that function will state that the
device power state has been changed to D3cold, while in fact it
will be D3hot, because acpi_device_set_power() falls back to D3hot
automatically if D3cold is not supported without returning an error.

To address this issue, modify the debug message in question to print
the current power state of the target PCI device's ACPI companion
instead of printing the target power state which may not reflect
the real final power state of the device.

Signed-off-by: Rafael J. Wysocki 
---
 drivers/pci/pci-acpi.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-pm/drivers/pci/pci-acpi.c
===
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -1021,7 +1021,7 @@ static int acpi_pci_set_power_state(stru
 
if (!error)
pci_dbg(dev, "power state changed by ACPI to %s\n",
-acpi_power_state_string(state_conv[state]));
+   acpi_power_state_string(adev->power.state));
 
return error;
 }





Re: 回复: [PATCH 01/10] tick/nohz: Prevent tick_nohz_get_sleep_length() from returning negative value

2021-03-25 Thread Rafael J. Wysocki
On Thursday, March 25, 2021 2:14:00 PM CET Frederic Weisbecker wrote:
> On Tue, Mar 16, 2021 at 04:08:08PM +, Zhou Ti (x2019cwm) wrote:
> > But I don't think it's a good idea to handle this in callers, because 
> > logically the function shouldn't return negative values. Returning 0 
> > directly would allow idle governors to get another chance to select again.
> 
> Hmm, I'm going to leave the last word to Rafael since cpuidle are the only
> callers of this. In any case we need to fix it.

Yes, we do.

So I said that I preferred to address this in the callers and the reason why
is because, for example, for the teo governor it would be a matter of using
a different data type to store the tick_nohz_get_sleep_length() return value,
like in the (untested) patch below.

So at least in this case there is no need to add any new branches anywhere.

I'm still not sure about menu, because it is more complicated, but even if
that one needs an extra branch, that is a win already.

---
 drivers/cpuidle/governors/teo.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/cpuidle/governors/teo.c
===
--- linux-pm.orig/drivers/cpuidle/governors/teo.c
+++ linux-pm/drivers/cpuidle/governors/teo.c
@@ -100,8 +100,8 @@ struct teo_idle_state {
  * @intervals: Saved idle duration values.
  */
 struct teo_cpu {
-   u64 time_span_ns;
-   u64 sleep_length_ns;
+   s64 time_span_ns;
+   s64 sleep_length_ns;
struct teo_idle_state states[CPUIDLE_STATE_MAX];
int interval_idx;
u64 intervals[INTERVALS];
@@ -216,7 +216,7 @@ static bool teo_time_ok(u64 interval_ns)
  */
 static int teo_find_shallower_state(struct cpuidle_driver *drv,
struct cpuidle_device *dev, int state_idx,
-   u64 duration_ns)
+   s64 duration_ns)
 {
int i;
 
@@ -242,7 +242,7 @@ static int teo_select(struct cpuidle_dri
 {
struct teo_cpu *cpu_data = per_cpu_ptr(_cpus, dev->cpu);
s64 latency_req = cpuidle_governor_latency_req(dev->cpu);
-   u64 duration_ns;
+   s64 duration_ns;
unsigned int hits, misses, early_hits;
int max_early_idx, prev_max_early_idx, constraint_idx, idx, i;
ktime_t delta_tick;





Re: [PATCH 1/4] PCI: acpi_pcihp: Correct acpi_pci_check_ejectable() function name in the header

2021-03-25 Thread Rafael J. Wysocki
On Thu, Mar 25, 2021 at 8:50 AM Xiongfeng Wang
 wrote:
>
> Fixes the following W=1 kernel build warning(s):
>
>  drivers/pci/hotplug/acpi_pcihp.c:167: warning: expecting prototype for 
> acpi_pcihp_check_ejectable(). Prototype was for acpi_pci_check_ejectable() 
> instead
>
> Reported-by: Hulk Robot 
> Signed-off-by: Xiongfeng Wang 
> ---
>  drivers/pci/hotplug/acpi_pcihp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/hotplug/acpi_pcihp.c 
> b/drivers/pci/hotplug/acpi_pcihp.c
> index 2750a64cecd3..4fedebf2f8c1 100644
> --- a/drivers/pci/hotplug/acpi_pcihp.c
> +++ b/drivers/pci/hotplug/acpi_pcihp.c
> @@ -157,7 +157,7 @@ static int pcihp_is_ejectable(acpi_handle handle)
>  }
>
>  /**
> - * acpi_pcihp_check_ejectable - check if handle is ejectable ACPI PCI slot
> + * acpi_pci_check_ejectable - check if handle is ejectable ACPI PCI slot

Again, this is about fixing a kerneldoc comment, so please change the
subject to reflect that more precisely.

This applies to all of the patches in the series AFAICS.

>   * @pbus: the PCI bus of the PCI slot corresponding to 'handle'
>   * @handle: ACPI handle to check
>   *
> --
> 2.20.1
>


Re: [PATCH 2/4] PCI/AER: Correct function names in the header

2021-03-25 Thread Rafael J. Wysocki
On Thu, Mar 25, 2021 at 8:50 AM Xiongfeng Wang
 wrote:
>
> Fixes the following W=1 kernel build warning(s):
>
>  drivers/pci/pcie/aer.c:138: warning: expecting prototype for 
> enable_ercr_checking(). Prototype was for enable_ecrc_checking() instead
>  drivers/pci/pcie/aer.c:162: warning: expecting prototype for 
> disable_ercr_checking(). Prototype was for disable_ecrc_checking() instead
>  drivers/pci/pcie/aer.c:1450: warning: expecting prototype for 
> aer_service_init(). Prototype was for pcie_aer_init() instead
>
> Reported-by: Hulk Robot 
> Signed-off-by: Xiongfeng Wang 

The subject is somewhat inaccurate, because you're fixing function
names in kerneldoc comments.

If you say "a header", people may think that this is about a header file.

> ---
>  drivers/pci/pcie/aer.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index ba22388342d1..ec943cee5ecc 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -129,7 +129,7 @@ static const char * const ecrc_policy_str[] = {
>  };
>
>  /**
> - * enable_ercr_checking - enable PCIe ECRC checking for a device
> + * enable_ecrc_checking - enable PCIe ECRC checking for a device
>   * @dev: the PCI device
>   *
>   * Returns 0 on success, or negative on failure.
> @@ -153,7 +153,7 @@ static int enable_ecrc_checking(struct pci_dev *dev)
>  }
>
>  /**
> - * disable_ercr_checking - disables PCIe ECRC checking for a device
> + * disable_ecrc_checking - disables PCIe ECRC checking for a device
>   * @dev: the PCI device
>   *
>   * Returns 0 on success, or negative on failure.
> @@ -1442,7 +1442,7 @@ static struct pcie_port_service_driver aerdriver = {
>  };
>
>  /**
> - * aer_service_init - register AER root service driver
> + * pcie_aer_init - register AER root service driver
>   *
>   * Invoked when AER root service driver is loaded.
>   */
> --
> 2.20.1
>


Re: [PATCH] ACPI: AC: fix some errors and warnings reported by checkpatch.pl

2021-03-24 Thread Rafael J. Wysocki
On Tue, Mar 23, 2021 at 2:01 PM Xiaofei Tan  wrote:
>
> Fix some errors and warnings reported by checkpatch.pl, including
> following five types:

Well, they are coding style issues rather than errors.

> ERROR: "foo * bar" should be "foo *bar"
> ERROR: code indent should use tabs where possible
> WARNING: Block comments use a trailing */ on a separate line
> WARNING: braces {} are not necessary for single statement blocks
> WARNING: void function return statements are not generally useful
>
> Signed-off-by: Xiaofei Tan 
> ---
>  drivers/acpi/ac.c | 32 +---
>  1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
> index b86ee6e..07987854 100644
> --- a/drivers/acpi/ac.c
> +++ b/drivers/acpi/ac.c
> @@ -78,17 +78,16 @@ static struct acpi_driver acpi_ac_driver = {
>  struct acpi_ac {
> struct power_supply *charger;
> struct power_supply_desc charger_desc;
> -   struct acpi_device * device;
> +   struct acpi_device *device;
> unsigned long long state;
> struct notifier_block battery_nb;
>  };
>
>  #define to_acpi_ac(x) power_supply_get_drvdata(x)
>
> -/* --
> -   AC Adapter Management
> -   
> -- */
> -
> +/*
> + * AC Adapter Management
> + */

Please use the /* ... */ (one-line) comment format here and below for
comments that aren't longer than one line.

>  static int acpi_ac_get_state(struct acpi_ac *ac)
>  {
> acpi_status status = AE_OK;
> @@ -109,9 +108,9 @@ static int acpi_ac_get_state(struct acpi_ac *ac)
> return 0;
>  }
>
> -/* --
> -sysfs I/F
> -   
> -- */
> +/*
> + * sysfs I/F
> + */
>  static int get_ac_property(struct power_supply *psy,
>enum power_supply_property psp,
>union power_supply_propval *val)
> @@ -138,10 +137,9 @@ static enum power_supply_property ac_props[] = {
> POWER_SUPPLY_PROP_ONLINE,
>  };
>
> -/* --
> -   Driver Model
> -   
> -- */
> -
> +/*
> + * Driver Model
> + */
>  static void acpi_ac_notify(struct acpi_device *device, u32 event)
>  {
> struct acpi_ac *ac = acpi_driver_data(device);
> @@ -174,8 +172,6 @@ static void acpi_ac_notify(struct acpi_device *device, 
> u32 event)
> acpi_notifier_call_chain(device, event, (u32) ac->state);
> kobject_uevent(>charger->dev.kobj, KOBJ_CHANGE);
> }
> -
> -   return;
>  }
>
>  static int acpi_ac_battery_notify(struct notifier_block *nb,
> @@ -282,9 +278,8 @@ static int acpi_ac_add(struct acpi_device *device)
> ac->battery_nb.notifier_call = acpi_ac_battery_notify;
> register_acpi_notifier(>battery_nb);
>  end:
> -   if (result) {
> +   if (result)
> kfree(ac);
> -   }
>
> return result;
>  }
> @@ -293,7 +288,7 @@ static int acpi_ac_add(struct acpi_device *device)
>  static int acpi_ac_resume(struct device *dev)
>  {
> struct acpi_ac *ac;
> -   unsigned old_state;
> +   unsigned int old_state;
>
> if (!dev)
> return -EINVAL;
> @@ -352,9 +347,8 @@ static int __init acpi_ac_init(void)
> }
>
> result = acpi_bus_register_driver(_ac_driver);
> -   if (result < 0) {
> +   if (result < 0)
> return -ENODEV;
> -   }
>
> return 0;
>  }
> --


Re: [PATCH] ACPI: tables: x86: Reserve memory occupied by ACPI tables

2021-03-24 Thread Rafael J. Wysocki
On Wed, Mar 24, 2021 at 4:42 PM George Kennedy
 wrote:
>
>
>
> On 3/24/2021 9:27 AM, Rafael J. Wysocki wrote:
> > On Wed, Mar 24, 2021 at 9:24 AM Mike Rapoport  wrote:
> >> On Tue, Mar 23, 2021 at 08:26:52PM +0100, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki 
> >>>
> >>> The following problem has been reported by George Kennedy:
> >>>
> >>>   Since commit 7fef431be9c9 ("mm/page_alloc: place pages to tail
> >>>   in __free_pages_core()") the following use after free occurs
> >>>   intermittently when ACPI tables are accessed.
> >>>
> >>>   BUG: KASAN: use-after-free in ibft_init+0x134/0xc49
> >>>   Read of size 4 at addr 8880be453004 by task swapper/0/1
> >>>   CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc1-7a7fd0d #1
> >>>   Call Trace:
> >>>dump_stack+0xf6/0x158
> >>>print_address_description.constprop.9+0x41/0x60
> >>>kasan_report.cold.14+0x7b/0xd4
> >>>__asan_report_load_n_noabort+0xf/0x20
> >>>ibft_init+0x134/0xc49
> >>>do_one_initcall+0xc4/0x3e0
> >>>kernel_init_freeable+0x5af/0x66b
> >>>kernel_init+0x16/0x1d0
> >>>ret_from_fork+0x22/0x30
> >>>
> >>>   ACPI tables mapped via kmap() do not have their mapped pages
> >>>   reserved and the pages can be "stolen" by the buddy allocator.
> >>>
> >>> Apparently, on the affected system, the ACPI table in question is
> >>> not located in "reserved" memory, like ACPI NVS or ACPI Data, that
> >>> will not be used by the buddy allocator, so the memory occupied by
> >>> that table has to be explicitly reserved to prevent the buddy
> >>> allocator from using it.
> >>>
> >>> In order to address this problem, rearrange the initialization of the
> >>> ACPI tables on x86 to locate the initial tables earlier and reserve
> >>> the memory occupied by them.
> >>>
> >>> The other architectures using ACPI should not be affected by this
> >>> change.
> >>>
> >>> Link: 
> >>> https://lore.kernel.org/linux-acpi/1614802160-29362-1-git-send-email-george.kenn...@oracle.com/
> >>> Reported-by: George Kennedy 
> >>> Signed-off-by: Rafael J. Wysocki 
> >> FWIW:
> >> Reviewed-by: Mike Rapoport 
> > Thank you!
> >
> > George, can you please try this patch on the affected system?
>
> Rafael,
>
> 10 for 10 successful reboots with your patch.
>
> First, verified the failure is still there with latest 5.12.0-rc4.

Thank you!

I'll add a Tested-by from you to it, then.


Re: [PATCH] PCI: PM: Do not read power state in pci_enable_device_flags()

2021-03-24 Thread Rafael J. Wysocki
On Mon, Mar 22, 2021 at 3:32 PM Rafael J. Wysocki  wrote:
>
> On Tue, Mar 16, 2021 at 4:52 PM Rafael J. Wysocki  wrote:
> >
> > From: Rafael J. Wysocki 
> >
> > It should not be necessary to update the current_state field of
> > struct pci_dev in pci_enable_device_flags() before calling
> > do_pci_enable_device() for the device, because none of the
> > code between that point and the pci_set_power_state() call in
> > do_pci_enable_device() invoked later depends on it.
> >
> > Moreover, doing that is actively harmful in some cases.  For example,
> > if the given PCI device depends on an ACPI power resource whose _STA
> > method initially returns 0 ("off"), but the config space of the PCI
> > device is accessible and the power state retrieved from the
> > PCI_PM_CTRL register is D0, the current_state field in the struct
> > pci_dev representing that device will get out of sync with the
> > power.state of its ACPI companion object and that will lead to
> > power management issues going forward.
> >
> > To avoid such issues it is better to leave the current_state value
> > as is until it is changed to PCI_D0 by do_pci_enable_device() as
> > appropriate.  However, the power state of the device is not changed
> > to PCI_D0 if it is already enabled when pci_enable_device_flags()
> > gets called for it, so update its current_state in that case, but
> > use pci_update_current_state() covering platform PM too for that.
> >
> > Link: 
> > https://lore.kernel.org/lkml/20210314000439.3138941-1-luzmaximil...@gmail.com/
> > Reported-by: Maximilian Luz 
> > Tested-by: Maximilian Luz 
> > Signed-off-by: Rafael J. Wysocki 
>
> Bjorn, can I take this, or do you want to take care of it yourself?

I'm taking the silence as consent, so the patch has been applied as
5.13 material with the R-by from Mika.

> > ---
> >
> > Max, I've added a T-by from you even though the patch is slightly different
> > from what you have tested, but the difference shouldn't matter for your 
> > case.
> >
> > ---
> >  drivers/pci/pci.c |   16 +++-
> >  1 file changed, 3 insertions(+), 13 deletions(-)
> >
> > Index: linux-pm/drivers/pci/pci.c
> > ===
> > --- linux-pm.orig/drivers/pci/pci.c
> > +++ linux-pm/drivers/pci/pci.c
> > @@ -1870,20 +1870,10 @@ static int pci_enable_device_flags(struc
> > int err;
> > int i, bars = 0;
> >
> > -   /*
> > -* Power state could be unknown at this point, either due to a fresh
> > -* boot or a device removal call.  So get the current power state
> > -* so that things like MSI message writing will behave as expected
> > -* (e.g. if the device really is in D0 at enable time).
> > -*/
> > -   if (dev->pm_cap) {
> > -   u16 pmcsr;
> > -   pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 
> > );
> > -   dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
> > -   }
> > -
> > -   if (atomic_inc_return(>enable_cnt) > 1)
> > +   if (atomic_inc_return(>enable_cnt) > 1) {
> > +   pci_update_current_state(dev, dev->current_state);
> > return 0;   /* already enabled */
> > +   }
> >
> > bridge = pci_upstream_bridge(dev);
> > if (bridge)
> >
> >
> >


Re: [PATCH] ACPI: processor: Fix CPU0 wakeup in acpi_idle_play_dead()

2021-03-24 Thread Rafael J. Wysocki
On Wed, Mar 24, 2021 at 4:23 PM Vitaly Kuznetsov  wrote:
>
> Commit 496121c02127 ("ACPI: processor: idle: Allow probing on platforms
> with one ACPI C-state") broke CPU0 hotplug on certain systems, e.g.
> I'm observing the following on AWS Nitro (e.g r5b.xlarge but other
> instance types are affected as well):
>
>  # echo 0 > /sys/devices/system/cpu/cpu0/online
>  # echo 1 > /sys/devices/system/cpu/cpu0/online
>  <10 seconds delay>
>  -bash: echo: write error: Input/output error
>
> In fact, the above mentioned commit only revealed the problem and did
> not introduce it. On x86, to wakeup CPU an NMI is being used and
> hlt_play_dead()/mwait_play_dead() loops are prepared to handle it:
>
> /*
>  * If NMI wants to wake up CPU0, start CPU0.
>  */
> if (wakeup_cpu0())
> start_cpu0();
>
> cpuidle_play_dead() -> acpi_idle_play_dead() (which is now being called on
> systems where it wasn't called before the above mentioned commit) serves
> the same purpose but it doesn't have a path for CPU0. What happens now on
> wakeup is:
> - NMI is sent to CPU0
> - wakeup_cpu0_nmi() works as expected
> - we get back to while (1) loop in acpi_idle_play_dead()
> - safe_halt() puts CPU0 to sleep again.
>
> The straightforward/minimal fix is add the special handling for CPU0 on x86
> and that's what the patch is doing.
>
> Fixes: 496121c02127 ("ACPI: processor: idle: Allow probing on platforms with 
> one ACPI C-state")
> Signed-off-by: Vitaly Kuznetsov 

This looks reasonable to me, but I'll give others some time to respond.

> ---
>  arch/x86/include/asm/smp.h| 1 +
>  arch/x86/kernel/smpboot.c | 2 +-
>  drivers/acpi/processor_idle.c | 7 +++
>  3 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index c0538f82c9a2..57ef2094af93 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -132,6 +132,7 @@ void native_play_dead(void);
>  void play_dead_common(void);
>  void wbinvd_on_cpu(int cpu);
>  int wbinvd_on_all_cpus(void);
> +bool wakeup_cpu0(void);
>
>  void native_smp_send_reschedule(int cpu);
>  void native_send_call_func_ipi(const struct cpumask *mask);
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 02813a7f3a7c..f877150a91da 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1659,7 +1659,7 @@ void play_dead_common(void)
> local_irq_disable();
>  }
>
> -static bool wakeup_cpu0(void)
> +bool wakeup_cpu0(void)
>  {
> if (smp_processor_id() == 0 && enable_start_cpu0)
> return true;
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index d93e400940a3..eb1101b16c08 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -29,6 +29,7 @@
>   */
>  #ifdef CONFIG_X86
>  #include 
> +#include 
>  #endif
>
>  #define _COMPONENT  ACPI_PROCESSOR_COMPONENT
> @@ -541,6 +542,12 @@ static int acpi_idle_play_dead(struct cpuidle_device 
> *dev, int index)
> wait_for_freeze();
> } else
> return -ENODEV;
> +
> +#ifdef CONFIG_X86
> +   /* If NMI wants to wake up CPU0, start CPU0. */
> +   if (wakeup_cpu0())
> +   start_cpu0();
> +#endif
> }
>
> /* Never reached */
> --
> 2.30.2
>


Re: [PATCH] ACPI: tables: x86: Reserve memory occupied by ACPI tables

2021-03-24 Thread Rafael J. Wysocki
On Wed, Mar 24, 2021 at 9:24 AM Mike Rapoport  wrote:
>
> On Tue, Mar 23, 2021 at 08:26:52PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki 
> >
> > The following problem has been reported by George Kennedy:
> >
> >  Since commit 7fef431be9c9 ("mm/page_alloc: place pages to tail
> >  in __free_pages_core()") the following use after free occurs
> >  intermittently when ACPI tables are accessed.
> >
> >  BUG: KASAN: use-after-free in ibft_init+0x134/0xc49
> >  Read of size 4 at addr 8880be453004 by task swapper/0/1
> >  CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc1-7a7fd0d #1
> >  Call Trace:
> >   dump_stack+0xf6/0x158
> >   print_address_description.constprop.9+0x41/0x60
> >   kasan_report.cold.14+0x7b/0xd4
> >   __asan_report_load_n_noabort+0xf/0x20
> >   ibft_init+0x134/0xc49
> >   do_one_initcall+0xc4/0x3e0
> >   kernel_init_freeable+0x5af/0x66b
> >   kernel_init+0x16/0x1d0
> >   ret_from_fork+0x22/0x30
> >
> >  ACPI tables mapped via kmap() do not have their mapped pages
> >  reserved and the pages can be "stolen" by the buddy allocator.
> >
> > Apparently, on the affected system, the ACPI table in question is
> > not located in "reserved" memory, like ACPI NVS or ACPI Data, that
> > will not be used by the buddy allocator, so the memory occupied by
> > that table has to be explicitly reserved to prevent the buddy
> > allocator from using it.
> >
> > In order to address this problem, rearrange the initialization of the
> > ACPI tables on x86 to locate the initial tables earlier and reserve
> > the memory occupied by them.
> >
> > The other architectures using ACPI should not be affected by this
> > change.
> >
> > Link: 
> > https://lore.kernel.org/linux-acpi/1614802160-29362-1-git-send-email-george.kenn...@oracle.com/
> > Reported-by: George Kennedy 
> > Signed-off-by: Rafael J. Wysocki 
>
> FWIW:
> Reviewed-by: Mike Rapoport 

Thank you!

George, can you please try this patch on the affected system?

> > ---
> >  arch/x86/kernel/acpi/boot.c |   25 -
> >  arch/x86/kernel/setup.c |8 +++-
> >  drivers/acpi/tables.c   |   42 
> > +++---
> >  include/linux/acpi.h|9 -
> >  4 files changed, 62 insertions(+), 22 deletions(-)
> >
> > Index: linux-pm/arch/x86/kernel/acpi/boot.c
> > ===
> > --- linux-pm.orig/arch/x86/kernel/acpi/boot.c
> > +++ linux-pm/arch/x86/kernel/acpi/boot.c
> > @@ -1554,10 +1554,18 @@ void __init acpi_boot_table_init(void)
> >   /*
> >* Initialize the ACPI boot-time table parser.
> >*/
> > - if (acpi_table_init()) {
> > + if (acpi_locate_initial_tables())
> >   disable_acpi();
> > - return;
> > - }
> > + else
> > + acpi_reserve_initial_tables();
> > +}
> > +
> > +int __init early_acpi_boot_init(void)
> > +{
> > + if (acpi_disabled)
> > + return 1;
> > +
> > + acpi_table_init_complete();
> >
> >   acpi_table_parse(ACPI_SIG_BOOT, acpi_parse_sbf);
> >
> > @@ -1570,18 +1578,9 @@ void __init acpi_boot_table_init(void)
> >   } else {
> >   printk(KERN_WARNING PREFIX "Disabling ACPI 
> > support\n");
> >   disable_acpi();
> > - return;
> > + return 1;
> >   }
> >   }
> > -}
> > -
> > -int __init early_acpi_boot_init(void)
> > -{
> > - /*
> > -  * If acpi_disabled, bail out
> > -  */
> > - if (acpi_disabled)
> > - return 1;
> >
> >   /*
> >* Process the Multiple APIC Description Table (MADT), if present
> > Index: linux-pm/arch/x86/kernel/setup.c
> > ===
> > --- linux-pm.orig/arch/x86/kernel/setup.c
> > +++ linux-pm/arch/x86/kernel/setup.c
> > @@ -1045,6 +1045,9 @@ void __init setup_arch(char **cmdline_p)
> >
> >   cleanup_highmap();
> >
> > + /* Look for ACPI tables and reserve memory occupied by them. */
> > + acpi_boot_table_init();
> > +
> >   memblock_set_current_limit(ISA_END_ADDRESS);
> >   e820__memblock_setup();
> >
> > @@ -1136,11 +1139,6 @@ void __init se

[PATCH] ACPI: tables: x86: Reserve memory occupied by ACPI tables

2021-03-23 Thread Rafael J. Wysocki
From: Rafael J. Wysocki 

The following problem has been reported by George Kennedy:

 Since commit 7fef431be9c9 ("mm/page_alloc: place pages to tail
 in __free_pages_core()") the following use after free occurs
 intermittently when ACPI tables are accessed.

 BUG: KASAN: use-after-free in ibft_init+0x134/0xc49
 Read of size 4 at addr 8880be453004 by task swapper/0/1
 CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc1-7a7fd0d #1
 Call Trace:
  dump_stack+0xf6/0x158
  print_address_description.constprop.9+0x41/0x60
  kasan_report.cold.14+0x7b/0xd4
  __asan_report_load_n_noabort+0xf/0x20
  ibft_init+0x134/0xc49
  do_one_initcall+0xc4/0x3e0
  kernel_init_freeable+0x5af/0x66b
  kernel_init+0x16/0x1d0
  ret_from_fork+0x22/0x30

 ACPI tables mapped via kmap() do not have their mapped pages
 reserved and the pages can be "stolen" by the buddy allocator.

Apparently, on the affected system, the ACPI table in question is
not located in "reserved" memory, like ACPI NVS or ACPI Data, that
will not be used by the buddy allocator, so the memory occupied by
that table has to be explicitly reserved to prevent the buddy
allocator from using it.

In order to address this problem, rearrange the initialization of the
ACPI tables on x86 to locate the initial tables earlier and reserve
the memory occupied by them.

The other architectures using ACPI should not be affected by this
change.

Link: 
https://lore.kernel.org/linux-acpi/1614802160-29362-1-git-send-email-george.kenn...@oracle.com/
Reported-by: George Kennedy 
Signed-off-by: Rafael J. Wysocki 
---
 arch/x86/kernel/acpi/boot.c |   25 -
 arch/x86/kernel/setup.c |8 +++-
 drivers/acpi/tables.c   |   42 +++---
 include/linux/acpi.h|9 -
 4 files changed, 62 insertions(+), 22 deletions(-)

Index: linux-pm/arch/x86/kernel/acpi/boot.c
===
--- linux-pm.orig/arch/x86/kernel/acpi/boot.c
+++ linux-pm/arch/x86/kernel/acpi/boot.c
@@ -1554,10 +1554,18 @@ void __init acpi_boot_table_init(void)
/*
 * Initialize the ACPI boot-time table parser.
 */
-   if (acpi_table_init()) {
+   if (acpi_locate_initial_tables())
disable_acpi();
-   return;
-   }
+   else
+   acpi_reserve_initial_tables();
+}
+
+int __init early_acpi_boot_init(void)
+{
+   if (acpi_disabled)
+   return 1;
+
+   acpi_table_init_complete();
 
acpi_table_parse(ACPI_SIG_BOOT, acpi_parse_sbf);
 
@@ -1570,18 +1578,9 @@ void __init acpi_boot_table_init(void)
} else {
printk(KERN_WARNING PREFIX "Disabling ACPI support\n");
disable_acpi();
-   return;
+   return 1;
}
}
-}
-
-int __init early_acpi_boot_init(void)
-{
-   /*
-* If acpi_disabled, bail out
-*/
-   if (acpi_disabled)
-   return 1;
 
/*
 * Process the Multiple APIC Description Table (MADT), if present
Index: linux-pm/arch/x86/kernel/setup.c
===
--- linux-pm.orig/arch/x86/kernel/setup.c
+++ linux-pm/arch/x86/kernel/setup.c
@@ -1045,6 +1045,9 @@ void __init setup_arch(char **cmdline_p)
 
cleanup_highmap();
 
+   /* Look for ACPI tables and reserve memory occupied by them. */
+   acpi_boot_table_init();
+
memblock_set_current_limit(ISA_END_ADDRESS);
e820__memblock_setup();
 
@@ -1136,11 +1139,6 @@ void __init setup_arch(char **cmdline_p)
 
early_platform_quirks();
 
-   /*
-* Parse the ACPI tables for possible boot-time SMP configuration.
-*/
-   acpi_boot_table_init();
-
early_acpi_boot_init();
 
initmem_init();
Index: linux-pm/include/linux/acpi.h
===
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -222,10 +222,14 @@ void __iomem *__acpi_map_table(unsigned
 void __acpi_unmap_table(void __iomem *map, unsigned long size);
 int early_acpi_boot_init(void);
 int acpi_boot_init (void);
+void acpi_boot_table_prepare (void);
 void acpi_boot_table_init (void);
 int acpi_mps_check (void);
 int acpi_numa_init (void);
 
+int acpi_locate_initial_tables (void);
+void acpi_reserve_initial_tables (void);
+void acpi_table_init_complete (void);
 int acpi_table_init (void);
 int acpi_table_parse(char *id, acpi_tbl_table_handler handler);
 int __init acpi_table_parse_entries(char *id, unsigned long table_size,
@@ -814,9 +818,12 @@ static inline int acpi_boot_init(void)
return 0;
 }
 
+static inline void acpi_boot_table_prepare(void)
+{
+}
+
 static inline void acpi_boot_table_init(void)
 {
-   return;
 }
 
 static inline int acpi_mps_check(voi

  1   2   3   4   5   6   7   8   9   10   >