* [PATCH] ocxl: Simplify free_spa()
@ 2018-12-10 15:15 Greg Kurz
2018-12-10 16:14 ` Frederic Barrat
2018-12-11 1:05 ` Andrew Donnellan
0 siblings, 2 replies; 6+ messages in thread
From: Greg Kurz @ 2018-12-10 15:15 UTC (permalink / raw)
To: linuxppc-dev
Cc: Christophe Lombard, Vaibhav Jain, stable, Frederic Barrat,
Andrew Donnellan
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)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ocxl: Simplify free_spa()
2018-12-10 15:15 [PATCH] ocxl: Simplify free_spa() Greg Kurz
@ 2018-12-10 16:14 ` Frederic Barrat
2018-12-11 1:05 ` Andrew Donnellan
1 sibling, 0 replies; 6+ messages in thread
From: Frederic Barrat @ 2018-12-10 16:14 UTC (permalink / raw)
To: Greg Kurz, linuxppc-dev
Cc: Christophe Lombard, Vaibhav Jain, stable, Frederic Barrat,
Andrew Donnellan
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)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ocxl: Simplify free_spa()
2018-12-10 15:15 [PATCH] ocxl: Simplify free_spa() Greg Kurz
2018-12-10 16:14 ` Frederic Barrat
@ 2018-12-11 1:05 ` Andrew Donnellan
2018-12-11 8:57 ` Greg Kurz
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Donnellan @ 2018-12-11 1:05 UTC (permalink / raw)
To: Greg Kurz, linuxppc-dev
Cc: stable, Christophe Lombard, Frederic Barrat, Vaibhav Jain
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)
>
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com IBM Australia Limited
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ocxl: Simplify free_spa()
2018-12-11 1:05 ` Andrew Donnellan
@ 2018-12-11 8:57 ` Greg Kurz
2018-12-11 9:13 ` Andrew Donnellan
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kurz @ 2018-12-11 8:57 UTC (permalink / raw)
To: Andrew Donnellan
Cc: Christophe Lombard, Vaibhav Jain, stable, Frederic Barrat, linuxppc-dev
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)
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ocxl: Simplify free_spa()
2018-12-11 8:57 ` Greg Kurz
@ 2018-12-11 9:13 ` Andrew Donnellan
2018-12-11 10:04 ` Greg Kurz
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Donnellan @ 2018-12-11 9:13 UTC (permalink / raw)
To: Greg Kurz
Cc: Christophe Lombard, Vaibhav Jain, stable, Frederic Barrat, linuxppc-dev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ocxl: Simplify free_spa()
2018-12-11 9:13 ` Andrew Donnellan
@ 2018-12-11 10:04 ` Greg Kurz
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kurz @ 2018-12-11 10:04 UTC (permalink / raw)
To: Andrew Donnellan
Cc: Christophe Lombard, Vaibhav Jain, stable, Frederic Barrat, linuxppc-dev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-11 10:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-10 15:15 [PATCH] ocxl: Simplify free_spa() Greg Kurz
2018-12-10 16:14 ` Frederic Barrat
2018-12-11 1:05 ` Andrew Donnellan
2018-12-11 8:57 ` Greg Kurz
2018-12-11 9:13 ` Andrew Donnellan
2018-12-11 10:04 ` Greg Kurz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).