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

[PATCH 0/2] 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.dev.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<------------------------

since v1:

- Drop the patch making acpi_fwnode_handle() NULL-safe.

- Use original %pfw patch on Fixes: line, cc stable beginning from v5.5.

- Get the parent from struct acpi_device.dev.parent, not struct
  acpi_device.parent.

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

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

-- 
2.30.2

[PATCH 1/2] 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 device, embedded in struct acpi_device, has a parent field
already. Use that field to get the parent instead of relying on
acpi_get_parent().

Fixes: 3bd32d6a2ee6 ("lib/vsprintf: Add %pfw conversion specifier for printing fwnode names")
Cc: stable@vger.kernel.org # v5.5+
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/property.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index e312ebaed8db4..dc97711ba8081 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1089,16 +1089,14 @@ 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 device *dev =
+			to_acpi_device_node(fwnode)->dev.parent;
 
-			if (!acpi_bus_get_device(parent_handle, &adev))
-				return acpi_fwnode_handle(adev);
-		}
+		if (dev)
+			return acpi_fwnode_handle(to_acpi_device(dev));
 	}
 
 	return NULL;
-- 
2.30.2

[PATCH 2/2] 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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
---
 drivers/acpi/property.c | 6 +++---
 include/linux/acpi.h    | 7 -------
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index dc97711ba8081..ea82e5e2fdbce 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 */
@@ -1092,8 +1093,7 @@ struct fwnode_handle *acpi_node_get_parent(const struct fwnode_handle *fwnode)
 	}
 
 	if (is_acpi_device_node(fwnode)) {
-		struct device *dev =
-			to_acpi_device_node(fwnode)->dev.parent;
+		struct device *dev = to_acpi_device_node(fwnode)->dev.parent;
 
 		if (dev)
 			return acpi_fwnode_handle(to_acpi_device(dev));
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 974d497a897dc..a8e84be083a26 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1170,7 +1170,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 *,
@@ -1275,12 +1274,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 1/2] ACPI: Get acpi_device's parent from the parent field

[Andy Shevchenko]

On Tue, Nov 09, 2021 at 01:19:34PM +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 device, embedded in struct acpi_device, has a parent field
> already. Use that field to get the parent instead of relying on
> acpi_get_parent().

Thanks, with the below addressed
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: 3bd32d6a2ee6 ("lib/vsprintf: Add %pfw conversion specifier for printing fwnode names")
> Cc: stable@vger.kernel.org # v5.5+
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/acpi/property.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index e312ebaed8db4..dc97711ba8081 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -1089,16 +1089,14 @@ 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)) {
> +	}

> +	if (is_acpi_device_node(fwnode)) {

Unneeded change. Yes I know that 'else' here can be skipped. But in such cases
it's a trade-off between changes, code readability and maintenance. Since here
it's a fix, backporting concerns are also play role.

> +		struct device *dev =
> +			to_acpi_device_node(fwnode)->dev.parent;

We are not so strict in terms of line length, code will be better
if this is located on one line.

> -			if (!acpi_bus_get_device(parent_handle, &adev))
> -				return acpi_fwnode_handle(adev);
> -		}
> +		if (dev)
> +			return acpi_fwnode_handle(to_acpi_device(dev));
>  	}
>  
>  	return NULL;
> -- 
> 2.30.2
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Nov 09, 2021 at 01:19:35PM +0200, Sakari Ailus wrote:
> 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>

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

Please, change this to @linux.intel.com.

...

> -		struct device *dev =
> -			to_acpi_device_node(fwnode)->dev.parent;
> +		struct device *dev = to_acpi_device_node(fwnode)->dev.parent;

Ping-pong? (see comment to the previous patch)

-- 
With Best Regards,
Andy Shevchenko

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

[Sakari Ailus]

On Tue, Nov 09, 2021 at 02:20:37PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 09, 2021 at 01:19:35PM +0200, Sakari Ailus wrote:
> > 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>
> 
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> 
> Please, change this to @linux.intel.com.
> 
> ...
> 
> > -		struct device *dev =
> > -			to_acpi_device_node(fwnode)->dev.parent;
> > +		struct device *dev = to_acpi_device_node(fwnode)->dev.parent;
> 
> Ping-pong? (see comment to the previous patch)

This was meant to be in the first patch, I'll send v3.

-- 
Sakari Ailus

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

[Sakari Ailus]

Hi Andy,

Thanks for the review.

On Tue, Nov 09, 2021 at 02:19:13PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 09, 2021 at 01:19:34PM +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 device, embedded in struct acpi_device, has a parent field
> > already. Use that field to get the parent instead of relying on
> > acpi_get_parent().
> 
> Thanks, with the below addressed
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> > Fixes: 3bd32d6a2ee6 ("lib/vsprintf: Add %pfw conversion specifier for printing fwnode names")
> > Cc: stable@vger.kernel.org # v5.5+
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/acpi/property.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > index e312ebaed8db4..dc97711ba8081 100644
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -1089,16 +1089,14 @@ 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)) {
> > +	}
> 
> > +	if (is_acpi_device_node(fwnode)) {
> 
> Unneeded change. Yes I know that 'else' here can be skipped. But in such cases
> it's a trade-off between changes, code readability and maintenance. Since here
> it's a fix, backporting concerns are also play role.

The patch applies cleanly to 5.5, the oldest kernel where it's needed.

Do you prefer another patch to remove the else clause? I think it's a bit
overkill...

> 
> > +		struct device *dev =
> > +			to_acpi_device_node(fwnode)->dev.parent;
> 
> We are not so strict in terms of line length, code will be better
> if this is located on one line.
> 
> > -			if (!acpi_bus_get_device(parent_handle, &adev))
> > -				return acpi_fwnode_handle(adev);
> > -		}
> > +		if (dev)
> > +			return acpi_fwnode_handle(to_acpi_device(dev));
> >  	}
> >  
> >  	return NULL;

-- 
Regards,

Sakari Ailus

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

[Andy Shevchenko]

On Wed, Nov 10, 2021 at 10:09:04AM +0200, Sakari Ailus wrote:
> On Tue, Nov 09, 2021 at 02:19:13PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 09, 2021 at 01:19:34PM +0200, Sakari Ailus wrote:

...

> > > -	} else if (is_acpi_device_node(fwnode)) {
> > > +	}
> > 
> > > +	if (is_acpi_device_node(fwnode)) {
> > 
> > Unneeded change. Yes I know that 'else' here can be skipped. But in such cases
> > it's a trade-off between changes, code readability and maintenance. Since here
> > it's a fix, backporting concerns are also play role.
> 
> The patch applies cleanly to 5.5, the oldest kernel where it's needed.

Why? I don't see how this affects the workflow.

> Do you prefer another patch to remove the else clause?

Nope.

> I think it's a bit overkill...

Exactly, that's why the question is why have you split the if-else-if to two if:s?

-- 
With Best Regards,
Andy Shevchenko

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

[Sakari Ailus]

On Wed, Nov 10, 2021 at 10:18:20AM +0200, Andy Shevchenko wrote:
> On Wed, Nov 10, 2021 at 10:09:04AM +0200, Sakari Ailus wrote:
> > On Tue, Nov 09, 2021 at 02:19:13PM +0200, Andy Shevchenko wrote:
> > > On Tue, Nov 09, 2021 at 01:19:34PM +0200, Sakari Ailus wrote:
> 
> ...
> 
> > > > -	} else if (is_acpi_device_node(fwnode)) {
> > > > +	}
> > > 
> > > > +	if (is_acpi_device_node(fwnode)) {
> > > 
> > > Unneeded change. Yes I know that 'else' here can be skipped. But in such cases
> > > it's a trade-off between changes, code readability and maintenance. Since here
> > > it's a fix, backporting concerns are also play role.
> > 
> > The patch applies cleanly to 5.5, the oldest kernel where it's needed.
> 
> Why? I don't see how this affects the workflow.
> 
> > Do you prefer another patch to remove the else clause?
> 
> Nope.
> 
> > I think it's a bit overkill...
> 
> Exactly, that's why the question is why have you split the if-else-if to
> two if:s?

The else clause is useless, I think the code simply looks better without
it.

-- 
Sakari Ailus

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

[Andy Shevchenko]

On Wed, Nov 10, 2021 at 10:21:36AM +0200, Sakari Ailus wrote:
> On Wed, Nov 10, 2021 at 10:18:20AM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 10, 2021 at 10:09:04AM +0200, Sakari Ailus wrote:
> > > On Tue, Nov 09, 2021 at 02:19:13PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Nov 09, 2021 at 01:19:34PM +0200, Sakari Ailus wrote:

...

> > > > > -	} else if (is_acpi_device_node(fwnode)) {
> > > > > +	}
> > > > 
> > > > > +	if (is_acpi_device_node(fwnode)) {
> > > > 
> > > > Unneeded change. Yes I know that 'else' here can be skipped. But in such cases
> > > > it's a trade-off between changes, code readability and maintenance. Since here
> > > > it's a fix, backporting concerns are also play role.
> > > 
> > > The patch applies cleanly to 5.5, the oldest kernel where it's needed.
> > 
> > Why? I don't see how this affects the workflow.
> > 
> > > Do you prefer another patch to remove the else clause?
> > 
> > Nope.
> > 
> > > I think it's a bit overkill...
> > 
> > Exactly, that's why the question is why have you split the if-else-if to
> > two if:s?
> 
> The else clause is useless, I think the code simply looks better without
> it.

I see a contradiction here:

Statement 1: 'else' is useless.
Statement 2: patch to remove it is overkill.

Either separate patch for that, or no need to touch this code, esp. taken into
consideration that this is a fix (subject to backport).

-- 
With Best Regards,
Andy Shevchenko

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

[Sakari Ailus]

On Wed, Nov 10, 2021 at 10:56:42AM +0200, Andy Shevchenko wrote:
> On Wed, Nov 10, 2021 at 10:21:36AM +0200, Sakari Ailus wrote:
> > On Wed, Nov 10, 2021 at 10:18:20AM +0200, Andy Shevchenko wrote:
> > > On Wed, Nov 10, 2021 at 10:09:04AM +0200, Sakari Ailus wrote:
> > > > On Tue, Nov 09, 2021 at 02:19:13PM +0200, Andy Shevchenko wrote:
> > > > > On Tue, Nov 09, 2021 at 01:19:34PM +0200, Sakari Ailus wrote:
> 
> ...
> 
> > > > > > -	} else if (is_acpi_device_node(fwnode)) {
> > > > > > +	}
> > > > > 
> > > > > > +	if (is_acpi_device_node(fwnode)) {
> > > > > 
> > > > > Unneeded change. Yes I know that 'else' here can be skipped. But in such cases
> > > > > it's a trade-off between changes, code readability and maintenance. Since here
> > > > > it's a fix, backporting concerns are also play role.
> > > > 
> > > > The patch applies cleanly to 5.5, the oldest kernel where it's needed.
> > > 
> > > Why? I don't see how this affects the workflow.
> > > 
> > > > Do you prefer another patch to remove the else clause?
> > > 
> > > Nope.
> > > 
> > > > I think it's a bit overkill...
> > > 
> > > Exactly, that's why the question is why have you split the if-else-if to
> > > two if:s?
> > 
> > The else clause is useless, I think the code simply looks better without
> > it.
> 
> I see a contradiction here:
> 
> Statement 1: 'else' is useless.
> Statement 2: patch to remove it is overkill.

There's no contradiction.

I argue doing that in a separate patch is waste of everyone's time. As
simple as that.

Sure, it could be done, but usually ends up being left as-is.

-- 
Sakari Ailus

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

[Andy Shevchenko]

On Wed, Nov 10, 2021 at 02:02:52PM +0200, Sakari Ailus wrote:
> On Wed, Nov 10, 2021 at 10:56:42AM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 10, 2021 at 10:21:36AM +0200, Sakari Ailus wrote:
> > > On Wed, Nov 10, 2021 at 10:18:20AM +0200, Andy Shevchenko wrote:
> > > > On Wed, Nov 10, 2021 at 10:09:04AM +0200, Sakari Ailus wrote:
> > > > > On Tue, Nov 09, 2021 at 02:19:13PM +0200, Andy Shevchenko wrote:
> > > > > > On Tue, Nov 09, 2021 at 01:19:34PM +0200, Sakari Ailus wrote:

...

> > > > > > > -	} else if (is_acpi_device_node(fwnode)) {
> > > > > > > +	}
> > > > > > 
> > > > > > > +	if (is_acpi_device_node(fwnode)) {
> > > > > > 
> > > > > > Unneeded change. Yes I know that 'else' here can be skipped. But in such cases
> > > > > > it's a trade-off between changes, code readability and maintenance. Since here
> > > > > > it's a fix, backporting concerns are also play role.
> > > > > 
> > > > > The patch applies cleanly to 5.5, the oldest kernel where it's needed.
> > > > 
> > > > Why? I don't see how this affects the workflow.
> > > > 
> > > > > Do you prefer another patch to remove the else clause?
> > > > 
> > > > Nope.
> > > > 
> > > > > I think it's a bit overkill...
> > > > 
> > > > Exactly, that's why the question is why have you split the if-else-if to
> > > > two if:s?
> > > 
> > > The else clause is useless, I think the code simply looks better without
> > > it.
> > 
> > I see a contradiction here:
> > 
> > Statement 1: 'else' is useless.
> > Statement 2: patch to remove it is overkill.
> 
> There's no contradiction.
> 
> I argue doing that in a separate patch is waste of everyone's time. As
> simple as that.

And this is precisely my point. But my other point is that doing this in the
fix patch is a churn. Bottom line, this part of the change shouldn't be here.

Also, it increases LOC counts. You may submit a separate patch to fix all of
the redundant 'else':s and we will see the necessity of them. But it's does
not belong to the patch you provided here.

Hope this clarifies my point.

-- 
With Best Regards,
Andy Shevchenko

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

[Rafael J. Wysocki]

On Wed, Nov 10, 2021 at 9:09 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Andy,
>
> Thanks for the review.
>
> On Tue, Nov 09, 2021 at 02:19:13PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 09, 2021 at 01:19:34PM +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 device, embedded in struct acpi_device, has a parent field
> > > already. Use that field to get the parent instead of relying on
> > > acpi_get_parent().
> >
> > Thanks, with the below addressed
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > > Fixes: 3bd32d6a2ee6 ("lib/vsprintf: Add %pfw conversion specifier for printing fwnode names")
> > > Cc: stable@vger.kernel.org # v5.5+
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/acpi/property.c | 14 ++++++--------
> > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > index e312ebaed8db4..dc97711ba8081 100644
> > > --- a/drivers/acpi/property.c
> > > +++ b/drivers/acpi/property.c
> > > @@ -1089,16 +1089,14 @@ 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)) {
> > > +   }
> >
> > > +   if (is_acpi_device_node(fwnode)) {
> >
> > Unneeded change. Yes I know that 'else' here can be skipped. But in such cases
> > it's a trade-off between changes, code readability and maintenance. Since here
> > it's a fix, backporting concerns are also play role.
>
> The patch applies cleanly to 5.5, the oldest kernel where it's needed.

Which doesn't matter too much.

The change above is not needed and there is no point making it in
which otherwise is a fix, not just because of the backporting
concerns, but also for the sake of cleanliness in general.

Have you posted the v3 already?  If not, please update the patch as
requested by Andy.

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

[Sakari Ailus]

On Wed, Nov 17, 2021 at 03:52:21PM +0100, Rafael J. Wysocki wrote:
> On Wed, Nov 10, 2021 at 9:09 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Andy,
> >
> > Thanks for the review.
> >
> > On Tue, Nov 09, 2021 at 02:19:13PM +0200, Andy Shevchenko wrote:
> > > On Tue, Nov 09, 2021 at 01:19:34PM +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 device, embedded in struct acpi_device, has a parent field
> > > > already. Use that field to get the parent instead of relying on
> > > > acpi_get_parent().
> > >
> > > Thanks, with the below addressed
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > > Fixes: 3bd32d6a2ee6 ("lib/vsprintf: Add %pfw conversion specifier for printing fwnode names")
> > > > Cc: stable@vger.kernel.org # v5.5+
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  drivers/acpi/property.c | 14 ++++++--------
> > > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > index e312ebaed8db4..dc97711ba8081 100644
> > > > --- a/drivers/acpi/property.c
> > > > +++ b/drivers/acpi/property.c
> > > > @@ -1089,16 +1089,14 @@ 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)) {
> > > > +   }
> > >
> > > > +   if (is_acpi_device_node(fwnode)) {
> > >
> > > Unneeded change. Yes I know that 'else' here can be skipped. But in such cases
> > > it's a trade-off between changes, code readability and maintenance. Since here
> > > it's a fix, backporting concerns are also play role.
> >
> > The patch applies cleanly to 5.5, the oldest kernel where it's needed.
> 
> Which doesn't matter too much.
> 
> The change above is not needed and there is no point making it in
> which otherwise is a fix, not just because of the backporting
> concerns, but also for the sake of cleanliness in general.
> 
> Have you posted the v3 already?  If not, please update the patch as
> requested by Andy.

I'll post v3 soon.

-- 
Sakari Ailus