[PATCH] ACPI / hotplug / PCI: lost acpiphp_put_context in acpiphp_grab_context()

[PATCH] ACPI / hotplug / PCI: lost acpiphp_put_context in acpiphp_grab_context()

[Vasily Averin]

Fixes: edf5bf34d408 ("ACPI / dock: Use callback pointers from devices' ACPI hotplug contexts")
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 drivers/pci/hotplug/acpiphp_glue.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index b386995..e49ec95 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -124,6 +124,8 @@ static struct acpiphp_context *acpiphp_grab_context(struct acpi_device *adev)
 	acpi_lock_hp_context();
 	context = acpiphp_get_context(adev);
 	if (!context || context->func.parent->is_going_away) {
+		if (context)
+			acpiphp_put_context(context);
 		acpi_unlock_hp_context();
 		return NULL;
 	}
-- 
1.8.3.1

Re: [PATCH] ACPI / hotplug / PCI: lost acpiphp_put_context in acpiphp_grab_context()

[Rafael J. Wysocki]

On Sat, Jun 13, 2020 at 9:41 AM Vasily Averin <vvs@virtuozzo.com> wrote:
>
> Fixes: edf5bf34d408 ("ACPI / dock: Use callback pointers from devices' ACPI hotplug contexts")

Thanks for the fix ->

> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index b386995..e49ec95 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -124,6 +124,8 @@ static struct acpiphp_context *acpiphp_grab_context(struct acpi_device *adev)
>         acpi_lock_hp_context();
>         context = acpiphp_get_context(adev);
>         if (!context || context->func.parent->is_going_away) {
> +               if (context)
> +                       acpiphp_put_context(context);

-> but I'd prefer:

        if (context && context->func.parent->is_going_away) {
                      acpiphp_put_context(context);
                      context = NULL;
       }
       if (!context) {

>                 acpi_unlock_hp_context();
>                 return NULL;
>         }
> --

Thanks!

[PATCH v2] ACPI / hotplug / PCI: lost acpiphp_put_context in acpiphp_grab_context()

[Vasily Averin]

v2: followed to rafael@'s proposal
Fixes: edf5bf34d408 ("ACPI / dock: Use callback pointers from devices' ACPI hotplug contexts")
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 drivers/pci/hotplug/acpiphp_glue.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index b4c92cee13f8..a0923a65e636 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -123,7 +123,11 @@ static struct acpiphp_context *acpiphp_grab_context(struct acpi_device *adev)
 
 	acpi_lock_hp_context();
 	context = acpiphp_get_context(adev);
-	if (!context || context->func.parent->is_going_away) {
+	if (context && context->func.parent->is_going_away) {
+		acpiphp_put_context(context);
+		context = NULL;
+	}
+	if (!context) {
 		acpi_unlock_hp_context();
 		return NULL;
 	}
-- 
2.17.1

Re: [PATCH v2] ACPI / hotplug / PCI: lost acpiphp_put_context in acpiphp_grab_context()

[Rafael J. Wysocki]

On Tuesday, June 23, 2020 1:17:43 AM CEST Vasily Averin wrote:
> v2: followed to rafael@'s proposal
> Fixes: edf5bf34d408 ("ACPI / dock: Use callback pointers from devices' ACPI hotplug contexts")
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index b4c92cee13f8..a0923a65e636 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -123,7 +123,11 @@ static struct acpiphp_context *acpiphp_grab_context(struct acpi_device *adev)
>  
>  	acpi_lock_hp_context();
>  	context = acpiphp_get_context(adev);
> -	if (!context || context->func.parent->is_going_away) {
> +	if (context && context->func.parent->is_going_away) {
> +		acpiphp_put_context(context);
> +		context = NULL;
> +	}
> +	if (!context) {
>  		acpi_unlock_hp_context();
>  		return NULL;
>  	}
> 

Thanks for following my suggestion, but it occurred to me that it could still be
done in a better way.

So instead of the above I'd prefer to apply the following change (added PCI and Bjorn
for visibility):

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] PCI: hotplug: ACPI: Fix context refcounting in acpiphp_grab_context()

If context is not NULL in acpiphp_grab_context(), but the
is_going_away flag is set for the device's parent, the reference
counter of the context needs to be decremented before returning
NULL or the context will never be freed, so make that happen.

Fixes: edf5bf34d408 ("ACPI / dock: Use callback pointers from devices' ACPI hotplug contexts")
Reported-by: Vasily Averin <vvs@virtuozzo.com>
Cc: 3.15+ <stable@vger.kernel.org> # 3.15+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
@@ -122,13 +122,21 @@ static struct acpiphp_context *acpiphp_g
 	struct acpiphp_context *context;
 
 	acpi_lock_hp_context();
+
 	context = acpiphp_get_context(adev);
-	if (!context || context->func.parent->is_going_away) {
-		acpi_unlock_hp_context();
-		return NULL;
+	if (!context)
+		goto unlock;
+
+	if (context->func.parent->is_going_away) {
+		acpiphp_put_context(context);
+		context = NULL;
+		goto unlock;
 	}
+
 	get_bridge(context->func.parent);
 	acpiphp_put_context(context);
+
+unlock:
 	acpi_unlock_hp_context();
 	return context;
 }

Re: [PATCH v2] ACPI / hotplug / PCI: lost acpiphp_put_context in acpiphp_grab_context()

[Vasily Averin]

On 6/23/20 7:29 PM, Rafael J. Wysocki wrote:
> On Tuesday, June 23, 2020 1:17:43 AM CEST Vasily Averin wrote:
>> v2: followed to rafael@'s proposal
>> Fixes: edf5bf34d408 ("ACPI / dock: Use callback pointers from devices' ACPI hotplug contexts")
>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> Thanks for following my suggestion, but it occurred to me that it could still be
> done in a better way.
> 
> So instead of the above I'd prefer to apply the following change (added PCI and Bjorn
> for visibility):

Thank you,
however could you please tell me what do you think about following variant?

 drivers/pci/hotplug/acpiphp_glue.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index b4c92cee13f8..5875c3654b52 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -119,16 +119,17 @@ static inline void put_bridge(struct acpiphp_bridge *bridge)
 
 static struct acpiphp_context *acpiphp_grab_context(struct acpi_device *adev)
 {
-	struct acpiphp_context *context;
+	struct acpiphp_context *c, *context = NULL;
 
 	acpi_lock_hp_context();
-	context = acpiphp_get_context(adev);
-	if (!context || context->func.parent->is_going_away) {
-		acpi_unlock_hp_context();
-		return NULL;
+	c = acpiphp_get_context(adev);
+	if (c) {
+		if (c->func.parent->is_going_away == false) {
+			get_bridge(c->func.parent);
+			context = c;
+		}
+		acpiphp_put_context(c);
 	}
-	get_bridge(context->func.parent);
-	acpiphp_put_context(context);
 	acpi_unlock_hp_context();
 	return context;
 }

Re: [PATCH v2] ACPI / hotplug / PCI: lost acpiphp_put_context in acpiphp_grab_context()

[Rafael J. Wysocki]

On Tue, Jun 23, 2020 at 9:01 PM Vasily Averin <vvs@virtuozzo.com> wrote:
>
> On 6/23/20 7:29 PM, Rafael J. Wysocki wrote:
> > On Tuesday, June 23, 2020 1:17:43 AM CEST Vasily Averin wrote:
> >> v2: followed to rafael@'s proposal
> >> Fixes: edf5bf34d408 ("ACPI / dock: Use callback pointers from devices' ACPI hotplug contexts")
> >> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> > Thanks for following my suggestion, but it occurred to me that it could still be
> > done in a better way.
> >
> > So instead of the above I'd prefer to apply the following change (added PCI and Bjorn
> > for visibility):
>
> Thank you,
> however could you please tell me what do you think about following variant?
>
>  drivers/pci/hotplug/acpiphp_glue.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index b4c92cee13f8..5875c3654b52 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -119,16 +119,17 @@ static inline void put_bridge(struct acpiphp_bridge *bridge)
>
>  static struct acpiphp_context *acpiphp_grab_context(struct acpi_device *adev)
>  {
> -       struct acpiphp_context *context;
> +       struct acpiphp_context *c, *context = NULL;
>
>         acpi_lock_hp_context();
> -       context = acpiphp_get_context(adev);
> -       if (!context || context->func.parent->is_going_away) {
> -               acpi_unlock_hp_context();
> -               return NULL;
> +       c = acpiphp_get_context(adev);
> +       if (c) {
> +               if (c->func.parent->is_going_away == false) {
> +                       get_bridge(c->func.parent);
> +                       context = c;
> +               }
> +               acpiphp_put_context(c);
>         }
> -       get_bridge(context->func.parent);
> -       acpiphp_put_context(context);
>         acpi_unlock_hp_context();
>         return context;
>  }

There are reasons to do it the way I want:
- The indentation level is minimum.
- The real purpose of this function is to get a parent bridge
reference, so that needs to be the central piece of it rather than the
checks for exceptional cases.
- The second local var is not necessary.

Thanks!