Messages in this thread Patch in this message | | | Subject | Re: [PATCH v1 06/10] powerpc/pseries/iommu: Add ddw_list_add() helper | From | Alexey Kardashevskiy <> | Date | Mon, 24 Aug 2020 13:46:52 +1000 |
| |
On 18/08/2020 09:40, Leonardo Bras wrote: > There are two functions adding DDW to the direct_window_list in a > similar way, so create a ddw_list_add() to avoid duplicity and > simplify those functions. > > Also, on enable_ddw(), add list_del() on out_free_window to allow > removing the window from list if any error occurs. > > Signed-off-by: Leonardo Bras <leobras.c@gmail.com> > --- > arch/powerpc/platforms/pseries/iommu.c | 42 ++++++++++++++++---------- > 1 file changed, 26 insertions(+), 16 deletions(-) > > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c > index 39617ce0ec83..fcdefcc0f365 100644 > --- a/arch/powerpc/platforms/pseries/iommu.c > +++ b/arch/powerpc/platforms/pseries/iommu.c > @@ -872,6 +872,24 @@ static u64 find_existing_ddw(struct device_node *pdn) > return dma_addr; > } > > +static struct direct_window *ddw_list_add(struct device_node *pdn, > + const struct dynamic_dma_window_prop *dma64) > +{ > + struct direct_window *window; > + > + window = kzalloc(sizeof(*window), GFP_KERNEL); > + if (!window) > + return NULL; > + > + window->device = pdn; > + window->prop = dma64; > + spin_lock(&direct_window_list_lock); > + list_add(&window->list, &direct_window_list); > + spin_unlock(&direct_window_list_lock); > + > + return window; > +} > + > static int find_existing_ddw_windows(void) > { > int len; > @@ -887,18 +905,11 @@ static int find_existing_ddw_windows(void) > if (!direct64) > continue; > > - window = kzalloc(sizeof(*window), GFP_KERNEL); > - if (!window || len < sizeof(struct dynamic_dma_window_prop)) { > + window = ddw_list_add(pdn, direct64); > + if (!window || len < sizeof(*direct64)) {
Since you are touching this code, it looks like the "len < sizeof(*direct64)" part should go above to "if (!direct64)".
> kfree(window); > remove_ddw(pdn, true); > - continue; > } > - > - window->device = pdn; > - window->prop = direct64; > - spin_lock(&direct_window_list_lock); > - list_add(&window->list, &direct_window_list); > - spin_unlock(&direct_window_list_lock); > } > > return 0; > @@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > dev_dbg(&dev->dev, "created tce table LIOBN 0x%x for %pOF\n", > create.liobn, dn); > > - window = kzalloc(sizeof(*window), GFP_KERNEL); > + /* Add new window to existing DDW list */
The comment seems to duplicate what the ddw_list_add name already suggests.
> + window = ddw_list_add(pdn, ddwprop); > if (!window) > goto out_clear_window; > > @@ -1280,16 +1292,14 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > goto out_free_window; > } > > - window->device = pdn; > - window->prop = ddwprop; > - spin_lock(&direct_window_list_lock); > - list_add(&window->list, &direct_window_list); > - spin_unlock(&direct_window_list_lock);
I'd leave these 3 lines here and in find_existing_ddw_windows() (which would make ddw_list_add -> ddw_prop_alloc). In general you want to have less stuff to do on the failure path. kmalloc may fail and needs kfree but you can safely delay list_add (which cannot fail) and avoid having the lock help twice in the same function (one of them is hidden inside ddw_list_add).
Not sure if this change is really needed after all. Thanks,
> - > dma_addr = be64_to_cpu(ddwprop->dma_base); > goto out_unlock; > > out_free_window: > + spin_lock(&direct_window_list_lock); > + list_del(&window->list); > + spin_unlock(&direct_window_list_lock); > + > kfree(window); > > out_clear_window: >
-- Alexey
| |