On Mon, 2020-08-24 at 13:46 +1000, Alexey Kardashevskiy wrote: > > 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)".
Sure, makes sense. It will be fixed for v2. > > > > > 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. Ok, I will remove it then. > > + 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, I understand this leads to better performance in case anything fails. Also, I think list_add happening in the end is less error-prone (in case the list is checked between list_add and a fail). But what if we put it at the end? What is the chance of a kzalloc of 4 pointers (struct direct_window) failing after walk_system_ram_range? Is it not worthy doing that for making enable_ddw() easier to understand? Best regards, Leonardo