| Message ID | 20230819002850.32349-11-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
Hi Vikram, On 19/08/2023 01:28, Vikram Garhwal wrote: > Protect iommu_add_dt_device() with dtdevs_lock to prevent concurrent access > to add/remove/assign/deassign. > With addition of dynamic programming feature(follow-up patches in this series), Typo: missing space before '('. > this function can be concurrently access by pci device assign/deassign and also I couldn't find any use of this function in the PCI code. So are you talking about not yet upstreamed patches? Also, typo: s/access/accessed/ > by dynamic node add/remove using device tree overlays. > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> > Reviewed-by: Michal Orzel <michal.orzel@amd.com> The code itself looks good to me. So I will provide my reviewed-by tag once my question about the commit message is answered. Cheers,
Hi Julien, On Tue, Aug 22, 2023 at 08:47:10PM +0100, Julien Grall wrote: > Hi Vikram, > > On 19/08/2023 01:28, Vikram Garhwal wrote: > > Protect iommu_add_dt_device() with dtdevs_lock to prevent concurrent access > > to add/remove/assign/deassign. > > With addition of dynamic programming feature(follow-up patches in this series), > > Typo: missing space before '('. > > > this function can be concurrently access by pci device assign/deassign and also > > I couldn't find any use of this function in the PCI code. So are you talking > about not yet upstreamed patches? So, this assign and deassign is also used by pci-assignable-add from xl which "Make a device assignable for pci-passthru" > > Also, typo: s/access/accessed/ > > > by dynamic node add/remove using device tree overlays. > > > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > > Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> > > Reviewed-by: Michal Orzel <michal.orzel@amd.com> > > The code itself looks good to me. So I will provide my reviewed-by tag once > my question about the commit message is answered. > > Cheers, > > -- > Julien Grall
On 25/08/2023 05:44, Vikram Garhwal wrote: > Hi Julien, > On Tue, Aug 22, 2023 at 08:47:10PM +0100, Julien Grall wrote: >> Hi Vikram, >> >> On 19/08/2023 01:28, Vikram Garhwal wrote: >>> Protect iommu_add_dt_device() with dtdevs_lock to prevent concurrent access >>> to add/remove/assign/deassign. >>> With addition of dynamic programming feature(follow-up patches in this series), >> >> Typo: missing space before '('. >> >>> this function can be concurrently access by pci device assign/deassign and also >> >> I couldn't find any use of this function in the PCI code. So are you talking >> about not yet upstreamed patches? > So, this assign and deassign is also used by pci-assignable-add from xl which > "Make a device assignable for pci-passthru" Hmmm... But this is not something we currently support on Arm and in fact this is not plumbed. So please remove any reference to PCI because this is misleading. Cheers,
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c index 5796ee1f93..096ef2dd68 100644 --- a/xen/drivers/passthrough/device_tree.c +++ b/xen/drivers/passthrough/device_tree.c @@ -148,6 +148,8 @@ int iommu_add_dt_device(struct dt_device_node *np) if ( dev_iommu_fwspec_get(dev) ) return 0; + spin_lock(&dtdevs_lock); + /* * According to the Documentation/devicetree/bindings/iommu/iommu.txt * from Linux. @@ -160,7 +162,10 @@ int iommu_add_dt_device(struct dt_device_node *np) * these callback implemented. */ if ( !ops->add_device || !ops->dt_xlate ) - return -EINVAL; + { + rc = -EINVAL; + goto fail; + } if ( !dt_device_is_available(iommu_spec.np) ) break; @@ -191,6 +196,8 @@ int iommu_add_dt_device(struct dt_device_node *np) if ( rc < 0 ) iommu_fwspec_free(dev); + fail: + spin_unlock(&dtdevs_lock); return rc; }