Re: [PATCH] ocxl: Simplify free_spa()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 11 Dec 2018 12:05:49 +1100
Andrew Donnellan <andrew.donnellan@xxxxxxxxxxx> 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@xxxxxxxx>  
> 
> 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@xxxxxxxxxxx>
> 
> > ---
> >   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)
> >   
> 




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux