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@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx # v5.15 and up
> > > Depends-on: ("ACPI: Make acpi_fwnode_handle safer")
> > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > ---
> > > 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
