[PATCH] ocxl: Simplify free_spa()

The only users of free_spa() are alloc_link() and free_link(), and
in both cases:

- link->spa != NULL

- free_spa(link) is immediatly followed by kfree(link)

The check isn't needed, and it doesn't bring much to clear the link->spa
pointer. Drop both.

Signed-off-by: Greg Kurz <redacted>
---
 drivers/misc/ocxl/link.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
index 31695a078485..eed92055184d 100644
--- a/drivers/misc/ocxl/link.c
+++ b/drivers/misc/ocxl/link.c

@@ -352,11 +352,8 @@ static void free_spa(struct link *link)
 	pr_debug("Freeing SPA for %x:%x:%x\n", link->domain, link->bus,
 		link->dev);
 
-	if (spa && spa->spa_mem) {
-		free_pages((unsigned long) spa->spa_mem, spa->spa_order);
-		kfree(spa);
-		link->spa = NULL;
-	}
+	free_pages((unsigned long) spa->spa_mem, spa->spa_order);
+	kfree(spa);
 }
 
 static int alloc_link(struct pci_dev *dev, int PE_mask, struct link **out_link)

Le 10/12/2018 à 16:15, Greg Kurz a écrit :

The only users of free_spa() are alloc_link() and free_link(), and
in both cases:

- link->spa != NULL

- free_spa(link) is immediatly followed by kfree(link)

The check isn't needed, and it doesn't bring much to clear the link->spa
pointer. Drop both.

Signed-off-by: Greg Kurz <redacted>
---

OK, that looks safe enough
Acked-by: Frederic Barrat <redacted>

On 11/12/18 2:15 am, Greg Kurz wrote:

The only users of free_spa() are alloc_link() and free_link(), and
in both cases:

- link->spa != NULL

- free_spa(link) is immediatly followed by kfree(link)

The check isn't needed, and it doesn't bring much to clear the link->spa
pointer. Drop both.

Signed-off-by: Greg Kurz <redacted>

I like defensive programming but for this case I don't really care too 
much either way

Acked-by: Andrew Donnellan <redacted>

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

On Tue, 11 Dec 2018 12:05:49 +1100
Andrew Donnellan [off-list ref] wrote:

On 11/12/18 2:15 am, Greg Kurz wrote:

quoted

The only users of free_spa() are alloc_link() and free_link(), and
in both cases:

- link->spa != NULL

- free_spa(link) is immediatly followed by kfree(link)

The check isn't needed, and it doesn't bring much to clear the link->spa
pointer. Drop both.

Signed-off-by: Greg Kurz <redacted>  

I like defensive programming but for this case I don't really care too 
much either way

I now realize that I should have mentioned the real motivation for this
change. I'm working on refactoring the code so that we can use ocxl in a
KVM guest. The concept of link can be shared by both powernv and pseries
variants but the SPA is definitely a powernv only thingy. The benefit
of this patch is hence to kick 'struct link' out of free_spa() so that
it can be utimately moved to powernv specific code.

The initial version of this change was just moving the link->spa check
and link->spa = NULL to the callers, where it was quite obvious they're
not needed... Should I re-post this as two patches for clarity ?

Acked-by: Andrew Donnellan <redacted>

quoted

---
  drivers/misc/ocxl/link.c |    7 ++-----
  1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
index 31695a078485..eed92055184d 100644
--- a/drivers/misc/ocxl/link.c
+++ b/drivers/misc/ocxl/link.c

@@ -352,11 +352,8 @@ static void free_spa(struct link *link)
  	pr_debug("Freeing SPA for %x:%x:%x\n", link->domain, link->bus,
  		link->dev);
  
-	if (spa && spa->spa_mem) {
-		free_pages((unsigned long) spa->spa_mem, spa->spa_order);
-		kfree(spa);
-		link->spa = NULL;
-	}
+	free_pages((unsigned long) spa->spa_mem, spa->spa_order);
+	kfree(spa);
  }
  
  static int alloc_link(struct pci_dev *dev, int PE_mask, struct link **out_link)
  

On 11/12/18 7:57 pm, Greg Kurz wrote:

I now realize that I should have mentioned the real motivation for this
change. I'm working on refactoring the code so that we can use ocxl in a
KVM guest. The concept of link can be shared by both powernv and pseries
variants but the SPA is definitely a powernv only thingy. The benefit
of this patch is hence to kick 'struct link' out of free_spa() so that
it can be utimately moved to powernv specific code.

The initial version of this change was just moving the link->spa check
and link->spa = NULL to the callers, where it was quite obvious they're
not needed... Should I re-post this as two patches for clarity ?

Ah, that makes much more sense.

If that's the case, then why not go all the way and change the function 
signature while you're at it?

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

On Tue, 11 Dec 2018 20:13:21 +1100
Andrew Donnellan [off-list ref] wrote:

On 11/12/18 7:57 pm, Greg Kurz wrote:

quoted

I now realize that I should have mentioned the real motivation for this
change. I'm working on refactoring the code so that we can use ocxl in a
KVM guest. The concept of link can be shared by both powernv and pseries
variants but the SPA is definitely a powernv only thingy. The benefit
of this patch is hence to kick 'struct link' out of free_spa() so that
it can be utimately moved to powernv specific code.

The initial version of this change was just moving the link->spa check
and link->spa = NULL to the callers, where it was quite obvious they're
not needed... Should I re-post this as two patches for clarity ?  

Ah, that makes much more sense.

If that's the case, then why not go all the way and change the function 
signature while you're at it?

Sure, will do.