[PATCH 0/3] Get device's parent from parent field, fix sleeping IRQs disabled

[PATCH 0/3] Get device's parent from parent field, fix sleeping IRQs disabled

[Sakari Ailus]

Hi all,

This set changes getting fwnode's parent on ACPI fwnode so it no longer
needs a semaphore, using struct acpi_device->parent field instead of
calling acpi_get_parent(). The semaphore is being acquired when the
device's full path is printed which now takes place local IRQs disabled:

--------8<------------------------
BUG: sleeping function called from invalid context at kernel/locking/semaphore.c:163

...

Call Trace:
 <TASK>
 dump_stack_lvl+0x57/0x7d
 __might_resched.cold+0xf4/0x12f
 down_timeout+0x21/0x70
 acpi_os_wait_semaphore+0x63/0x180
 acpi_ut_acquire_mutex+0x123/0x1ba
 acpi_get_parent+0x30/0x71
 acpi_node_get_parent+0x64/0x90
 ? lock_acquire+0x1a0/0x300
 fwnode_count_parents+0x6d/0xb0
 fwnode_full_name_string+0x18/0x90
 fwnode_string+0xd7/0x140
 vsnprintf+0x1ec/0x4f0
 va_format.constprop.0+0x6a/0x130
 vsnprintf+0x1ec/0x4f0
 vprintk_store+0x271/0x5a0
 ? rcu_read_lock_sched_held+0x12/0x70
 ? lock_release+0x228/0x310
 ? acpi_initialize_hp_context+0x50/0x50
 vprintk_emit+0xd5/0x340
 _printk+0x58/0x6f
 __dynamic_pr_debug+0xe2/0x100
 __v4l2_fwnode_endpoint_parse+0x10c/0x450 [v4l2_fwnode]
 v4l2_fwnode_endpoint_parse+0x11/0x40 [v4l2_fwnode]
 cio2_pci_probe.cold+0x7fb/0x969 [ipu3_cio2]
 ? _raw_spin_unlock_irqrestore+0x42/0x70
 pci_device_probe+0xb6/0x140
 really_probe+0x1f5/0x3f0
 __driver_probe_device+0xfe/0x180
 driver_probe_device+0x1e/0x90
 __driver_attach+0xc4/0x1d0
 ? __device_attach_driver+0xe0/0xe0
 ? __device_attach_driver+0xe0/0xe0
 bus_for_each_dev+0x7b/0xc0
 bus_add_driver+0x14c/0x1f0
 driver_register+0x8b/0xe0
 ? 0xffffffffa0428000
 do_one_initcall+0x5b/0x300
 ? rcu_read_lock_sched_held+0x12/0x70
 ? trace_kmalloc+0x29/0xd0
 ? kmem_cache_alloc_trace+0x11d/0x630
 do_init_module+0x5c/0x260
 __do_sys_finit_module+0xae/0x110
 do_syscall_64+0x3b/0x90
 entry_SYSCALL_64_after_hwframe+0x44/0xae
--------8<------------------------

I guess one could argue it wasn't great to begin with that getting
fwnode's parent required a semaphore to begin with, nevertheless John's
patch made it a concrete problem. Added Cc: stable, too.

Sakari Ailus (3):
  ACPI: Make acpi_fwnode_handle safer
  ACPI: Get acpi_device's parent from the parent field
  ACPI: Make acpi_node_get_parent() local

 drivers/acpi/property.c | 15 ++++++---------
 include/acpi/acpi_bus.h |  2 +-
 include/linux/acpi.h    |  7 -------
 3 files changed, 7 insertions(+), 17 deletions(-)

-- 
2.30.2

[PATCH 1/3] ACPI: Make acpi_fwnode_handle safer

[Sakari Ailus]

Check that the fwnode argument passed to acpi_fwnode_handle is non-NULL,
and return NULL if it is, otherwise the fwnode. Thus the caller doesn't
have to ensure the argument is a valid non-NULL fwnode.

Cc: stable@vger.kernel.org # v5.15 and up
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/acpi/acpi_bus.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 53b6e9f9de7b4..c34d94521d40c 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -445,7 +445,7 @@ static inline bool acpi_data_node_match(const struct fwnode_handle *fwnode,
 
 static inline struct fwnode_handle *acpi_fwnode_handle(struct acpi_device *adev)
 {
-	return &adev->fwnode;
+	return adev ? &adev->fwnode : NULL;
 }
 
 static inline void *acpi_driver_data(struct acpi_device *d)
-- 
2.30.2

[PATCH 2/3] ACPI: Get acpi_device's parent from the parent field

[Sakari Ailus]

Printk modifier %pfw is used to print the full path of the device name.
This is obtained device by device until a device no longer has a parent.

On ACPI getting the parent fwnode is done by calling acpi_get_parent()
which tries to down() a semaphore. But local IRQs are now disabled in
vprintk_store() before the mutex is acquired. This is obviously a problem.

Luckily struct acpi_device has a parent field already. Use that field to
get the parent instead of relying on acpi_get_parent().

Fixes: 002eb6ad0751 ("printk: track/limit recursion")
Cc: stable@vger.kernel.org # v5.15 and up
Depends-on: ("ACPI: Make acpi_fwnode_handle safer")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/property.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index e312ebaed8db4..7403ee2816eb8 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1089,16 +1089,12 @@ struct fwnode_handle *acpi_node_get_parent(const struct fwnode_handle *fwnode)
 	if (is_acpi_data_node(fwnode)) {
 		/* All data nodes have parent pointer so just return that */
 		return to_acpi_data_node(fwnode)->parent;
-	} else if (is_acpi_device_node(fwnode)) {
-		acpi_handle handle, parent_handle;
+	}
 
-		handle = to_acpi_device_node(fwnode)->handle;
-		if (ACPI_SUCCESS(acpi_get_parent(handle, &parent_handle))) {
-			struct acpi_device *adev;
+	if (is_acpi_device_node(fwnode)) {
+		struct acpi_device *device = to_acpi_device_node(fwnode);
 
-			if (!acpi_bus_get_device(parent_handle, &adev))
-				return acpi_fwnode_handle(adev);
-		}
+		return acpi_fwnode_handle(device->parent);
 	}
 
 	return NULL;
-- 
2.30.2

[PATCH 3/3] ACPI: Make acpi_node_get_parent() local

[Sakari Ailus]

acpi_node_get_parent() isn't used outside drivers/acpi/property.c. Make it
local.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/property.c | 3 ++-
 include/linux/acpi.h    | 7 -------
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 7403ee2816eb8..49301d1bba4ff 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1084,7 +1084,8 @@ struct fwnode_handle *acpi_get_next_subnode(const struct fwnode_handle *fwnode,
  * Returns parent node of an ACPI device or data firmware node or %NULL if
  * not available.
  */
-struct fwnode_handle *acpi_node_get_parent(const struct fwnode_handle *fwnode)
+static struct fwnode_handle *
+acpi_node_get_parent(const struct fwnode_handle *fwnode)
 {
 	if (is_acpi_data_node(fwnode)) {
 		/* All data nodes have parent pointer so just return that */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 375715b0535fb..c65a754b1db53 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1168,7 +1168,6 @@ int acpi_node_prop_get(const struct fwnode_handle *fwnode, const char *propname,
 
 struct fwnode_handle *acpi_get_next_subnode(const struct fwnode_handle *fwnode,
 					    struct fwnode_handle *child);
-struct fwnode_handle *acpi_node_get_parent(const struct fwnode_handle *fwnode);
 
 struct acpi_probe_entry;
 typedef bool (*acpi_probe_entry_validate_subtbl)(struct acpi_subtable_header *,
@@ -1273,12 +1272,6 @@ acpi_get_next_subnode(const struct fwnode_handle *fwnode,
 	return NULL;
 }
 
-static inline struct fwnode_handle *
-acpi_node_get_parent(const struct fwnode_handle *fwnode)
-{
-	return NULL;
-}
-
 static inline struct fwnode_handle *
 acpi_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
 			     struct fwnode_handle *prev)
-- 
2.30.2

Re: [PATCH 0/3] Get device's parent from parent field, fix sleeping IRQs disabled

[John Ogness]

added CC: printk maintainer (Petr Mladek)

On 2021-11-03, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> This set changes getting fwnode's parent on ACPI fwnode so it no longer
> needs a semaphore, using struct acpi_device->parent field instead of
> calling acpi_get_parent(). The semaphore is being acquired when the
> device's full path is printed which now takes place local IRQs disabled:
>
> --------8<------------------------
> BUG: sleeping function called from invalid context at kernel/locking/semaphore.c:163
>
> ...
>
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x57/0x7d
>  __might_resched.cold+0xf4/0x12f
>  down_timeout+0x21/0x70
>  acpi_os_wait_semaphore+0x63/0x180
>  acpi_ut_acquire_mutex+0x123/0x1ba
>  acpi_get_parent+0x30/0x71
>  acpi_node_get_parent+0x64/0x90
>  ? lock_acquire+0x1a0/0x300
>  fwnode_count_parents+0x6d/0xb0
>  fwnode_full_name_string+0x18/0x90
>  fwnode_string+0xd7/0x140
>  vsnprintf+0x1ec/0x4f0
>  va_format.constprop.0+0x6a/0x130
>  vsnprintf+0x1ec/0x4f0
>  vprintk_store+0x271/0x5a0
>  ? rcu_read_lock_sched_held+0x12/0x70
>  ? lock_release+0x228/0x310
>  ? acpi_initialize_hp_context+0x50/0x50
>  vprintk_emit+0xd5/0x340
>  _printk+0x58/0x6f
...
> --------8<------------------------
>
> I guess one could argue it wasn't great to begin with that getting
> fwnode's parent required a semaphore to begin with, nevertheless John's
> patch made it a concrete problem. Added Cc: stable, too.

Well, before my work it was vprintk_emit() that was disabling local
interrupts. So this has always been broken.

Really it should be:

Fixes: 3bd32d6a2ee6 ("lib/vsprintf: Add %pfw conversion specifier for
printing fwnode names")

Regardless, the fix should go into 5.10 and 5.14 stables.

John Ogness

Re: [PATCH 0/3] Get device's parent from parent field, fix sleeping IRQs disabled

[Petr Mladek]

On Wed 2021-11-03 15:50:04, John Ogness wrote:
> added CC: printk maintainer (Petr Mladek)
> 
> On 2021-11-03, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > This set changes getting fwnode's parent on ACPI fwnode so it no longer
> > needs a semaphore, using struct acpi_device->parent field instead of
> > calling acpi_get_parent(). The semaphore is being acquired when the
> > device's full path is printed which now takes place local IRQs disabled:
> >
> > --------8<------------------------
> > BUG: sleeping function called from invalid context at kernel/locking/semaphore.c:163
> >
> > ...
> >
> > Call Trace:
> >  <TASK>
> >  dump_stack_lvl+0x57/0x7d
> >  __might_resched.cold+0xf4/0x12f
> >  down_timeout+0x21/0x70
> >  acpi_os_wait_semaphore+0x63/0x180
> >  acpi_ut_acquire_mutex+0x123/0x1ba
> >  acpi_get_parent+0x30/0x71
> >  acpi_node_get_parent+0x64/0x90
> >  ? lock_acquire+0x1a0/0x300
> >  fwnode_count_parents+0x6d/0xb0
> >  fwnode_full_name_string+0x18/0x90
> >  fwnode_string+0xd7/0x140
> >  vsnprintf+0x1ec/0x4f0
> >  va_format.constprop.0+0x6a/0x130
> >  vsnprintf+0x1ec/0x4f0
> >  vprintk_store+0x271/0x5a0
> >  ? rcu_read_lock_sched_held+0x12/0x70
> >  ? lock_release+0x228/0x310
> >  ? acpi_initialize_hp_context+0x50/0x50
> >  vprintk_emit+0xd5/0x340
> >  _printk+0x58/0x6f
> ...
> > --------8<------------------------
> >
> > I guess one could argue it wasn't great to begin with that getting
> > fwnode's parent required a semaphore to begin with, nevertheless John's
> > patch made it a concrete problem. Added Cc: stable, too.

It looks like a generic problem.

If I get it properly, we should make sure that any struct
fwnode_operations implementation will _not_ use sleeping locks in
the .get_parent() callback. Or anything that is called indirectly
from from vsprintf.

Adding Andy into Cc.

> Well, before my work it was vprintk_emit() that was disabling local
> interrupts. So this has always been broken.
> 
> Really it should be:
> 
> Fixes: 3bd32d6a2ee6 ("lib/vsprintf: Add %pfw conversion specifier for
> printing fwnode names")

Yes, I consider this to be the culprit.

> Regardless, the fix should go into 5.10 and 5.14 stables.

Please add the Fixes tag and the following into the commit message:

Cc: stable@vger.kernel.org # v5.5+

Best Regards,
Petr

Re: [PATCH 0/3] Get device's parent from parent field, fix sleeping IRQs disabled

[Petr Mladek]

(and really add Andy)

On Wed 2021-11-03 15:50:04, John Ogness wrote:
> added CC: printk maintainer (Petr Mladek)
> 
> On 2021-11-03, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > This set changes getting fwnode's parent on ACPI fwnode so it no longer
> > needs a semaphore, using struct acpi_device->parent field instead of
> > calling acpi_get_parent(). The semaphore is being acquired when the
> > device's full path is printed which now takes place local IRQs disabled:
> >
> > --------8<------------------------
> > BUG: sleeping function called from invalid context at kernel/locking/semaphore.c:163
> >
> > ...
> >
> > Call Trace:
> >  <TASK>
> >  dump_stack_lvl+0x57/0x7d
> >  __might_resched.cold+0xf4/0x12f
> >  down_timeout+0x21/0x70
> >  acpi_os_wait_semaphore+0x63/0x180
> >  acpi_ut_acquire_mutex+0x123/0x1ba
> >  acpi_get_parent+0x30/0x71
> >  acpi_node_get_parent+0x64/0x90
> >  ? lock_acquire+0x1a0/0x300
> >  fwnode_count_parents+0x6d/0xb0
> >  fwnode_full_name_string+0x18/0x90
> >  fwnode_string+0xd7/0x140
> >  vsnprintf+0x1ec/0x4f0
> >  va_format.constprop.0+0x6a/0x130
> >  vsnprintf+0x1ec/0x4f0
> >  vprintk_store+0x271/0x5a0
> >  ? rcu_read_lock_sched_held+0x12/0x70
> >  ? lock_release+0x228/0x310
> >  ? acpi_initialize_hp_context+0x50/0x50
> >  vprintk_emit+0xd5/0x340
> >  _printk+0x58/0x6f
> ...
> > --------8<------------------------
> >
> > I guess one could argue it wasn't great to begin with that getting
> > fwnode's parent required a semaphore to begin with, nevertheless John's
> > patch made it a concrete problem. Added Cc: stable, too.

It looks like a generic problem.

If I get it properly, we should make sure that any struct
fwnode_operations implementation will _not_ use sleeping locks in
the .get_parent() callback. Or anything that is called indirectly
from from vsprintf.

Adding Andy into Cc.

> Well, before my work it was vprintk_emit() that was disabling local
> interrupts. So this has always been broken.
> 
> Really it should be:
> 
> Fixes: 3bd32d6a2ee6 ("lib/vsprintf: Add %pfw conversion specifier for
> printing fwnode names")

Yes, I consider this to be the culprit.

> Regardless, the fix should go into 5.10 and 5.14 stables.

Please add the Fixes tag and the following into the commit message:

Cc: stable@vger.kernel.org # v5.5+

Best Regards,
Petr

Re: [PATCH 1/3] ACPI: Make acpi_fwnode_handle safer

[Andy Shevchenko]

On Wed, Nov 03, 2021 at 03:34:04PM +0200, Sakari Ailus wrote:
> Check that the fwnode argument passed to acpi_fwnode_handle is non-NULL,

acpi_fwnode_handle()

> and return NULL if it is, otherwise the fwnode. Thus the caller doesn't
> have to ensure the argument is a valid non-NULL fwnode.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

> Cc: stable@vger.kernel.org # v5.15 and up
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/acpi/acpi_bus.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 53b6e9f9de7b4..c34d94521d40c 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -445,7 +445,7 @@ static inline bool acpi_data_node_match(const struct fwnode_handle *fwnode,
>  
>  static inline struct fwnode_handle *acpi_fwnode_handle(struct acpi_device *adev)
>  {
> -	return &adev->fwnode;
> +	return adev ? &adev->fwnode : NULL;
>  }
>  
>  static inline void *acpi_driver_data(struct acpi_device *d)
> -- 
> 2.30.2
> 
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] ACPI: Get acpi_device's parent from the parent field

[Andy Shevchenko]

On Wed, Nov 03, 2021 at 03:34:05PM +0200, Sakari Ailus wrote:
> Printk modifier %pfw is used to print the full path of the device name.
> This is obtained device by device until a device no longer has a parent.
> 
> On ACPI getting the parent fwnode is done by calling acpi_get_parent()
> which tries to down() a semaphore. But local IRQs are now disabled in
> vprintk_store() before the mutex is acquired. This is obviously a problem.
> 
> Luckily struct acpi_device has a parent field already. Use that field to
> get the parent instead of relying on acpi_get_parent().

I think the best if Rafael can confirm that we may use it like this.
If he approved, nothing would stop me to give you
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

> Fixes: 002eb6ad0751 ("printk: track/limit recursion")
> Cc: stable@vger.kernel.org # v5.15 and up
> Depends-on: ("ACPI: Make acpi_fwnode_handle safer")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/acpi/property.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index e312ebaed8db4..7403ee2816eb8 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -1089,16 +1089,12 @@ struct fwnode_handle *acpi_node_get_parent(const struct fwnode_handle *fwnode)
>  	if (is_acpi_data_node(fwnode)) {
>  		/* All data nodes have parent pointer so just return that */
>  		return to_acpi_data_node(fwnode)->parent;
> -	} else if (is_acpi_device_node(fwnode)) {
> -		acpi_handle handle, parent_handle;
> +	}
>  
> -		handle = to_acpi_device_node(fwnode)->handle;
> -		if (ACPI_SUCCESS(acpi_get_parent(handle, &parent_handle))) {
> -			struct acpi_device *adev;
> +	if (is_acpi_device_node(fwnode)) {
> +		struct acpi_device *device = to_acpi_device_node(fwnode);
>  
> -			if (!acpi_bus_get_device(parent_handle, &adev))
> -				return acpi_fwnode_handle(adev);
> -		}
> +		return acpi_fwnode_handle(device->parent);
>  	}
>  
>  	return NULL;
> -- 
> 2.30.2
> 
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] ACPI: Make acpi_node_get_parent() local

[Andy Shevchenko]

On Wed, Nov 03, 2021 at 03:34:06PM +0200, Sakari Ailus wrote:
> acpi_node_get_parent() isn't used outside drivers/acpi/property.c. Make it
> local.

Always in favour for such patches!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/acpi/property.c | 3 ++-
>  include/linux/acpi.h    | 7 -------
>  2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 7403ee2816eb8..49301d1bba4ff 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -1084,7 +1084,8 @@ struct fwnode_handle *acpi_get_next_subnode(const struct fwnode_handle *fwnode,
>   * Returns parent node of an ACPI device or data firmware node or %NULL if
>   * not available.
>   */
> -struct fwnode_handle *acpi_node_get_parent(const struct fwnode_handle *fwnode)
> +static struct fwnode_handle *
> +acpi_node_get_parent(const struct fwnode_handle *fwnode)
>  {
>  	if (is_acpi_data_node(fwnode)) {
>  		/* All data nodes have parent pointer so just return that */
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 375715b0535fb..c65a754b1db53 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1168,7 +1168,6 @@ int acpi_node_prop_get(const struct fwnode_handle *fwnode, const char *propname,
>  
>  struct fwnode_handle *acpi_get_next_subnode(const struct fwnode_handle *fwnode,
>  					    struct fwnode_handle *child);
> -struct fwnode_handle *acpi_node_get_parent(const struct fwnode_handle *fwnode);
>  
>  struct acpi_probe_entry;
>  typedef bool (*acpi_probe_entry_validate_subtbl)(struct acpi_subtable_header *,
> @@ -1273,12 +1272,6 @@ acpi_get_next_subnode(const struct fwnode_handle *fwnode,
>  	return NULL;
>  }
>  
> -static inline struct fwnode_handle *
> -acpi_node_get_parent(const struct fwnode_handle *fwnode)
> -{
> -	return NULL;
> -}
> -
>  static inline struct fwnode_handle *
>  acpi_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
>  			     struct fwnode_handle *prev)
> -- 
> 2.30.2
> 
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] Get device's parent from parent field, fix sleeping IRQs disabled

[Andy Shevchenko]

On Wed, Nov 03, 2021 at 04:47:44PM +0100, Petr Mladek wrote:
> On Wed 2021-11-03 15:50:04, John Ogness wrote:
> > On 2021-11-03, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > This set changes getting fwnode's parent on ACPI fwnode so it no longer
> > > needs a semaphore, using struct acpi_device->parent field instead of
> > > calling acpi_get_parent(). The semaphore is being acquired when the
> > > device's full path is printed which now takes place local IRQs disabled:
> > >
> > > --------8<------------------------
> > > BUG: sleeping function called from invalid context at kernel/locking/semaphore.c:163
> > >
> > > ...
> > >
> > > Call Trace:
> > >  <TASK>
> > >  dump_stack_lvl+0x57/0x7d
> > >  __might_resched.cold+0xf4/0x12f
> > >  down_timeout+0x21/0x70
> > >  acpi_os_wait_semaphore+0x63/0x180
> > >  acpi_ut_acquire_mutex+0x123/0x1ba
> > >  acpi_get_parent+0x30/0x71
> > >  acpi_node_get_parent+0x64/0x90
> > >  ? lock_acquire+0x1a0/0x300
> > >  fwnode_count_parents+0x6d/0xb0
> > >  fwnode_full_name_string+0x18/0x90
> > >  fwnode_string+0xd7/0x140
> > >  vsnprintf+0x1ec/0x4f0
> > >  va_format.constprop.0+0x6a/0x130
> > >  vsnprintf+0x1ec/0x4f0
> > >  vprintk_store+0x271/0x5a0
> > >  ? rcu_read_lock_sched_held+0x12/0x70
> > >  ? lock_release+0x228/0x310
> > >  ? acpi_initialize_hp_context+0x50/0x50
> > >  vprintk_emit+0xd5/0x340
> > >  _printk+0x58/0x6f
> > ...
> > > --------8<------------------------
> > >
> > > I guess one could argue it wasn't great to begin with that getting
> > > fwnode's parent required a semaphore to begin with, nevertheless John's
> > > patch made it a concrete problem. Added Cc: stable, too.
> 
> It looks like a generic problem.
> 
> If I get it properly, we should make sure that any struct
> fwnode_operations implementation will _not_ use sleeping locks in
> the .get_parent() callback. Or anything that is called indirectly
> from from vsprintf.

Not sure how I may help here, but if there is a requirement to all *printf()
to be non-sleepable, then yes, this should be done in that way. I'm not sure
if every single %p extension does this...

It may require to update fwnode (source code) documentation to reflect the
requirement. Also might_sleep() / migth_sleep_if() in the API calls may
shed a light on (potential) issues.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] ACPI: Get acpi_device's parent from the parent field

[Rafael J. Wysocki]

On Wed, Nov 3, 2021 at 6:02 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Nov 03, 2021 at 03:34:05PM +0200, Sakari Ailus wrote:
> > Printk modifier %pfw is used to print the full path of the device name.
> > This is obtained device by device until a device no longer has a parent.
> >
> > On ACPI getting the parent fwnode is done by calling acpi_get_parent()
> > which tries to down() a semaphore. But local IRQs are now disabled in
> > vprintk_store() before the mutex is acquired. This is obviously a problem.
> >
> > Luckily struct acpi_device has a parent field already.

Which I'm going to eliminate, because it is redundant.

The dev object embedded in struct acpi_device has a parent field
pointing to the same object and that one is used by the driver core.

> > Use that field to get the parent instead of relying on acpi_get_parent().
>
> I think the best if Rafael can confirm that we may use it like this.
> If he approved, nothing would stop me to give you
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

In fact, I would prefer the parent field of the dev object embedded in
struct acpi_device to be used and for completeness it should be tested
against NULL unless you know that the parent is not going away ATM.

> > Fixes: 002eb6ad0751 ("printk: track/limit recursion")
> > Cc: stable@vger.kernel.org # v5.15 and up
> > Depends-on: ("ACPI: Make acpi_fwnode_handle safer")
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/acpi/property.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > index e312ebaed8db4..7403ee2816eb8 100644
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -1089,16 +1089,12 @@ struct fwnode_handle *acpi_node_get_parent(const struct fwnode_handle *fwnode)
> >       if (is_acpi_data_node(fwnode)) {
> >               /* All data nodes have parent pointer so just return that */
> >               return to_acpi_data_node(fwnode)->parent;
> > -     } else if (is_acpi_device_node(fwnode)) {
> > -             acpi_handle handle, parent_handle;
> > +     }
> >
> > -             handle = to_acpi_device_node(fwnode)->handle;
> > -             if (ACPI_SUCCESS(acpi_get_parent(handle, &parent_handle))) {
> > -                     struct acpi_device *adev;
> > +     if (is_acpi_device_node(fwnode)) {
> > +             struct acpi_device *device = to_acpi_device_node(fwnode);

Call this variable adev please.

> >
> > -                     if (!acpi_bus_get_device(parent_handle, &adev))
> > -                             return acpi_fwnode_handle(adev);
> > -             }
> > +             return acpi_fwnode_handle(device->parent);

And then

             adev = to_acpi_device(&device->dev.parent);
             if (adev)
                          return acpi_fwnode_handle(adev);

> >       }
> >
> >       return NULL;
> > --
> > 2.30.2
> >
> >
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH 1/3] ACPI: Make acpi_fwnode_handle safer

[Rafael J. Wysocki]

On Wed, Nov 3, 2021 at 2:33 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Check that the fwnode argument passed to acpi_fwnode_handle is non-NULL,
> and return NULL if it is, otherwise the fwnode. Thus the caller doesn't
> have to ensure the argument is a valid non-NULL fwnode.
>
> Cc: stable@vger.kernel.org # v5.15 and up

Why?

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

That's because you want to avoid a NULL check in the second patch and
it adds a ton of redundant NULL checks all over the place.

Like for example in include/acpi/acpi.h:

#define ACPI_COMPANION_SET(dev, adev) set_primary_fwnode(dev, (adev) ? \
            acpi_fwnode_handle(adev) : NULL)

You should at least get rid of this one.

> ---
>  include/acpi/acpi_bus.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 53b6e9f9de7b4..c34d94521d40c 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -445,7 +445,7 @@ static inline bool acpi_data_node_match(const struct fwnode_handle *fwnode,
>
>  static inline struct fwnode_handle *acpi_fwnode_handle(struct acpi_device *adev)
>  {
> -       return &adev->fwnode;
> +       return adev ? &adev->fwnode : NULL;
>  }
>
>  static inline void *acpi_driver_data(struct acpi_device *d)
> --

Re: [PATCH 0/3] Get device's parent from parent field, fix sleeping IRQs disabled

[Sakari Ailus]

Hi Petr, John,

On Wed, Nov 03, 2021 at 04:47:44PM +0100, Petr Mladek wrote:
> On Wed 2021-11-03 15:50:04, John Ogness wrote:
> > added CC: printk maintainer (Petr Mladek)
> > 
> > On 2021-11-03, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > This set changes getting fwnode's parent on ACPI fwnode so it no longer
> > > needs a semaphore, using struct acpi_device->parent field instead of
> > > calling acpi_get_parent(). The semaphore is being acquired when the
> > > device's full path is printed which now takes place local IRQs disabled:
> > >
> > > --------8<------------------------
> > > BUG: sleeping function called from invalid context at kernel/locking/semaphore.c:163
> > >
> > > ...
> > >
> > > Call Trace:
> > >  <TASK>
> > >  dump_stack_lvl+0x57/0x7d
> > >  __might_resched.cold+0xf4/0x12f
> > >  down_timeout+0x21/0x70
> > >  acpi_os_wait_semaphore+0x63/0x180
> > >  acpi_ut_acquire_mutex+0x123/0x1ba
> > >  acpi_get_parent+0x30/0x71
> > >  acpi_node_get_parent+0x64/0x90
> > >  ? lock_acquire+0x1a0/0x300
> > >  fwnode_count_parents+0x6d/0xb0
> > >  fwnode_full_name_string+0x18/0x90
> > >  fwnode_string+0xd7/0x140
> > >  vsnprintf+0x1ec/0x4f0
> > >  va_format.constprop.0+0x6a/0x130
> > >  vsnprintf+0x1ec/0x4f0
> > >  vprintk_store+0x271/0x5a0
> > >  ? rcu_read_lock_sched_held+0x12/0x70
> > >  ? lock_release+0x228/0x310
> > >  ? acpi_initialize_hp_context+0x50/0x50
> > >  vprintk_emit+0xd5/0x340
> > >  _printk+0x58/0x6f
> > ...
> > > --------8<------------------------
> > >
> > > I guess one could argue it wasn't great to begin with that getting
> > > fwnode's parent required a semaphore to begin with, nevertheless John's
> > > patch made it a concrete problem. Added Cc: stable, too.
> 
> It looks like a generic problem.
> 
> If I get it properly, we should make sure that any struct
> fwnode_operations implementation will _not_ use sleeping locks in
> the .get_parent() callback. Or anything that is called indirectly
> from from vsprintf.
> 
> Adding Andy into Cc.

That holds for OF and swnode already, ACPI was an exception to this.

> 
> > Well, before my work it was vprintk_emit() that was disabling local
> > interrupts. So this has always been broken.

Fair enough.

> > 
> > Really it should be:
> > 
> > Fixes: 3bd32d6a2ee6 ("lib/vsprintf: Add %pfw conversion specifier for
> > printing fwnode names")
> 
> Yes, I consider this to be the culprit.
> 
> > Regardless, the fix should go into 5.10 and 5.14 stables.
> 
> Please add the Fixes tag and the following into the commit message:
> 
> Cc: stable@vger.kernel.org # v5.5+

I'll use that in v2.

-- 
Regards,

Sakari Ailus

Re: [PATCH 1/3] ACPI: Make acpi_fwnode_handle safer

[Sakari Ailus]

Hi Rafael,

On Wed, Nov 03, 2021 at 06:55:17PM +0100, Rafael J. Wysocki wrote:
> On Wed, Nov 3, 2021 at 2:33 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Check that the fwnode argument passed to acpi_fwnode_handle is non-NULL,
> > and return NULL if it is, otherwise the fwnode. Thus the caller doesn't
> > have to ensure the argument is a valid non-NULL fwnode.
> >
> > Cc: stable@vger.kernel.org # v5.15 and up
> 
> Why?
> 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> That's because you want to avoid a NULL check in the second patch and
> it adds a ton of redundant NULL checks all over the place.
> 
> Like for example in include/acpi/acpi.h:
> 
> #define ACPI_COMPANION_SET(dev, adev) set_primary_fwnode(dev, (adev) ? \
>             acpi_fwnode_handle(adev) : NULL)
> 
> You should at least get rid of this one.

That's a fair point.

acpi_fwnode_handle() doesn't have a tonne of users so it's entirely
feasible to change the users as necessary but in either case I guess I'll
drop this from stable as it's likely to conflict with some of these.

-- 
Regards,

Sakari Ailus

Re: [PATCH 2/3] ACPI: Get acpi_device's parent from the parent field

[Sakari Ailus]

Hi Rafael,

On Wed, Nov 03, 2021 at 06:48:31PM +0100, Rafael J. Wysocki wrote:
> On Wed, Nov 3, 2021 at 6:02 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> >
> > On Wed, Nov 03, 2021 at 03:34:05PM +0200, Sakari Ailus wrote:
> > > Printk modifier %pfw is used to print the full path of the device name.
> > > This is obtained device by device until a device no longer has a parent.
> > >
> > > On ACPI getting the parent fwnode is done by calling acpi_get_parent()
> > > which tries to down() a semaphore. But local IRQs are now disabled in
> > > vprintk_store() before the mutex is acquired. This is obviously a problem.
> > >
> > > Luckily struct acpi_device has a parent field already.
> 
> Which I'm going to eliminate, because it is redundant.
> 
> The dev object embedded in struct acpi_device has a parent field
> pointing to the same object and that one is used by the driver core.

Indeed, I'll do that in v2.

> 
> > > Use that field to get the parent instead of relying on acpi_get_parent().
> >
> > I think the best if Rafael can confirm that we may use it like this.
> > If he approved, nothing would stop me to give you
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> 
> In fact, I would prefer the parent field of the dev object embedded in
> struct acpi_device to be used and for completeness it should be tested
> against NULL unless you know that the parent is not going away ATM.
> 
> > > Fixes: 002eb6ad0751 ("printk: track/limit recursion")
> > > Cc: stable@vger.kernel.org # v5.15 and up
> > > Depends-on: ("ACPI: Make acpi_fwnode_handle safer")
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/acpi/property.c | 12 ++++--------
> > >  1 file changed, 4 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > index e312ebaed8db4..7403ee2816eb8 100644
> > > --- a/drivers/acpi/property.c
> > > +++ b/drivers/acpi/property.c
> > > @@ -1089,16 +1089,12 @@ struct fwnode_handle *acpi_node_get_parent(const struct fwnode_handle *fwnode)
> > >       if (is_acpi_data_node(fwnode)) {
> > >               /* All data nodes have parent pointer so just return that */
> > >               return to_acpi_data_node(fwnode)->parent;
> > > -     } else if (is_acpi_device_node(fwnode)) {
> > > -             acpi_handle handle, parent_handle;
> > > +     }
> > >
> > > -             handle = to_acpi_device_node(fwnode)->handle;
> > > -             if (ACPI_SUCCESS(acpi_get_parent(handle, &parent_handle))) {
> > > -                     struct acpi_device *adev;
> > > +     if (is_acpi_device_node(fwnode)) {
> > > +             struct acpi_device *device = to_acpi_device_node(fwnode);
> 
> Call this variable adev please.
> 
> > >
> > > -                     if (!acpi_bus_get_device(parent_handle, &adev))
> > > -                             return acpi_fwnode_handle(adev);
> > > -             }
> > > +             return acpi_fwnode_handle(device->parent);
> 
> And then
> 
>              adev = to_acpi_device(&device->dev.parent);
>              if (adev)
>                           return acpi_fwnode_handle(adev);

Ack.

> 
> > >       }
> > >
> > >       return NULL;

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 3/3] ACPI: Make acpi_node_get_parent() local

[Sakari Ailus]

On Wed, Nov 03, 2021 at 07:02:19PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 03, 2021 at 03:34:06PM +0200, Sakari Ailus wrote:
> > acpi_node_get_parent() isn't used outside drivers/acpi/property.c. Make it
> > local.
> 
> Always in favour for such patches!
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

Thanks, Andy!

-- 
Sakari Ailus

Re: [PATCH 1/3] ACPI: Make acpi_fwnode_handle safer

[Sakari Ailus]

On Wed, Nov 03, 2021 at 06:55:17PM +0100, Rafael J. Wysocki wrote:
> On Wed, Nov 3, 2021 at 2:33 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Check that the fwnode argument passed to acpi_fwnode_handle is non-NULL,
> > and return NULL if it is, otherwise the fwnode. Thus the caller doesn't
> > have to ensure the argument is a valid non-NULL fwnode.
> >
> > Cc: stable@vger.kernel.org # v5.15 and up
> 
> Why?
> 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> That's because you want to avoid a NULL check in the second patch and
> it adds a ton of redundant NULL checks all over the place.
> 
> Like for example in include/acpi/acpi.h:
> 
> #define ACPI_COMPANION_SET(dev, adev) set_primary_fwnode(dev, (adev) ? \
>             acpi_fwnode_handle(adev) : NULL)
> 
> You should at least get rid of this one.

I went through the users and it seems they don't actually even benefit from
this. So I'll drop the patch.

Most of these little macros are made safer that way but in this case it
seems callers have already acpi_device around, or do checks for other
purposes as well.

-- 
Sakari Ailus