Re: [XEN][PATCH v8 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller

[Vikram Garhwal <vikram.garhwal@xxxxxxx>]

Hi Julien
On Fri, Aug 18, 2023 at 09:35:02PM +0100, Julien Grall wrote:
> Hi Vikram,
> 
> On 18/08/2023 20:52, Vikram Garhwal wrote:
> > Hi Jan
> > On Thu, Aug 17, 2023 at 09:05:44AM +0200, Jan Beulich wrote:
> > > On 17.08.2023 02:39, Vikram Garhwal wrote:
> > > > --- /dev/null
> > > > +++ b/xen/include/xen/iommu-private.h
> > > 
> > > I don't think private headers should live in include/xen/. Judging from 
> > > only
> > > the patches I was Cc-ed on, ...
> > Thank you for suggestion. Do you where can i place it then?
> > Please see another comment down regarding who might be using this function.
> > > 
> > > > @@ -0,0 +1,28 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +/*
> > > > + * xen/iommu-private.h
> > > > + */
> > > > +#ifndef __XEN_IOMMU_PRIVATE_H__
> > > > +#define __XEN_IOMMU_PRIVATE_H__
> > > > +
> > > > +#ifdef CONFIG_HAS_DEVICE_TREE
> > > > +#include <xen/device_tree.h>
> > > > +
> > > > +/*
> > > > + * Checks if dt_device_node is assigned to a domain or not. This 
> > > > function
> > > > + * expects to be called with dtdevs_lock acquired by caller.
> > > > + */
> > > > +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node 
> > > > *dev);
> > > > +#endif
> > > 
> > > ... I don't even see the need for the declaration, as the function is used
> > > only from the file also defining it. But of course if there is a use
> > > elsewhere (in Arm-only code, as is suggested by the description here), 
> > > then
> > > the header (under a suitable name) wants to live under 
> > > drivers/passthrough/
> > > (and of course be included only from anywhere in that sub-tree).
> > > 
> > This is also use in smmu.c:arm_smmu_dt_remove_device_legacy(). This is 
> > added in
> > 12/19 patch(xen/smmu: Add remove_device callback for smmu_iommu ops).
> 
> AFAICT, the caller of this function (iommu_remove_dt_device()) will already
> check if the device was assigned and bail out if that's the case.
> 
> 
> So why do we need to check it again in the SMMU driver?
> 
This was comment from you in v2:
"Even if the IOMMU subsystem check it, it would be good that the SMMU 
driver also check the device is not currently used before removing it.

If it is, then we should return -EBUSY."
Link:https://patchew.org/Xen/1636441347-133850-1-git-send-email-fnu.vikram@xxxxxxxxxx/1636441347-133850-7-git-send-email-fnu.vikram@xxxxxxxxxx/

And there was similar comment from Michal in v5.

That's why this was kept.

Regards,
Vikram

> And if you really need to then you most likely want to check the internal
> state of the SMMU driver rather than the generic state.
> 
> Cheers,
> 
> -- 
> Julien Grall