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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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