Hi Vikram, > -----Original Message----- > Subject: [XEN][PATCH v6 14/19] common/device_tree: Add rwlock for dt_host > > Dynamic programming ops will modify the dt_host and there might be other > function which are browsing the dt_host at the same time. To avoid the race > conditions, adding rwlock for browsing the dt_host during runtime.
While now I understand why you use rwlock instead of spinlock in this patch since you explained it in replying my comment in v5 (Thanks!). I would still suggest that you can add that kind of explanation in the commit message to make the commit message clear to everyone that reading this patch. > > Signed-off-by: Vikram Garhwal <vikram.garh...@amd.com> > --- > xen/common/device_tree.c | 4 ++++ > xen/drivers/passthrough/device_tree.c | 18 ++++++++++++++++++ > xen/include/xen/device_tree.h | 6 ++++++ > 3 files changed, 28 insertions(+) > [...] > ret = iommu_add_dt_device(dev); > if ( ret < 0 ) > @@ -310,6 +321,8 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, > struct domain *d, > printk(XENLOG_G_ERR "XEN_DOMCTL_assign_dt_device: assign > \"%s\"" > " to dom%u failed (%d)\n", > dt_node_full_name(dev), d->domain_id, ret); > + > + read_unlock(&dt_host->lock); Since you added "read_unlock(&dt_host->lock);" before the final return, i.e. "return ret", I don't think you need to add "read_unlock(&dt_host->lock);" here before the break. Or am I missing something? > break; > > case XEN_DOMCTL_deassign_device: > @@ -328,11 +341,15 @@ int iommu_do_dt_domctl(struct xen_domctl > *domctl, struct domain *d, > break; > > ret = xsm_deassign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev)); > + Nit: Unnecessary blank line addition here. Kind regards, Henry
