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 groug@kaod.org --- 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 groug@kaod.org
OK, that looks safe enough Acked-by: Frederic Barrat fbarrat@linux.ibm.com
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 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 groug@kaod.org
I like defensive programming but for this case I don't really care too much either way
Acked-by: Andrew Donnellan andrew.donnellan@au1.ibm.com
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 Tue, 11 Dec 2018 12:05:49 +1100 Andrew Donnellan andrew.donnellan@au1.ibm.com wrote:
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 groug@kaod.org
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 andrew.donnellan@au1.ibm.com
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?
On Tue, 11 Dec 2018 20:13:21 +1100 Andrew Donnellan andrew.donnellan@au1.ibm.com wrote:
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?
Sure, will do.
linux-stable-mirror@lists.linaro.org
-
Andrew Donnellan -
Frederic Barrat -
Greg Kurz