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

[Vikram Garhwal <vikram.garhwal@amd.com>]

Hi Jen,
On Mon, Aug 21, 2023 at 08:53:38AM +0200, Jan Beulich wrote:
> On 19.08.2023 02:28, Vikram Garhwal wrote:
> > Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_locked().
> > Remove static type so this can also be used by SMMU drivers to check if the
> > device is being used before removing.
> > 
> > Moving spin_lock to caller was done to prevent the concurrent access to
> > iommu_dt_device_is_assigned while doing add/remove/assign/deassign. Follow-up
> > patches in this series introduces node add/remove feature.
> > 
> > Also, caller is required hold the correct lock so moved the function prototype
> > to a private header.
> > 
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> > 
> > ---
> > Changes from v7:
> >     Update commit message.
> >     Add ASSERT().
> > ---
> > ---
> >  xen/drivers/passthrough/device_tree.c | 26 +++++++++++++++++++++----
> >  xen/include/xen/iommu-private.h       | 28 +++++++++++++++++++++++++++
> >  2 files changed, 50 insertions(+), 4 deletions(-)
> >  create mode 100644 xen/include/xen/iommu-private.h
> 
> I find it odd for new versions to be sent when discussion on the prior
> version hasn't settled yet, and when you then also don't incorporate
> the feedback given. I also find it odd to now be Cc-ed on the entire
> series. Imo instead of that patches which aren't self-explanatory /
> self-consistent simply need their descriptions improved (in the case
> here to mention what further use of a function is intended). Yet
> better would be to add declarations (and remove static) only at the
> point that's actually needed. This then eliminates the risk of
> subsequent changes in response to feedback (like Julien's here)
> resulting in the declaration no longer being needed, thus leading to
> a Misra violation (besides the general tidiness aspect).
> 
Reason behind sending new version: Patch 15/19 is largest patch in terms of change
and there was a long discussion with Julien and Stefano regarding a big change.
Last week(after v8) we agreed on change and Stefano and Julien were okay to send
v9 so it will be easier to review with that change.

Regarding cc-ing you to whole series, there was following comment on v8 09/1
"I don't think private headers should live in include/xen/. Judging from only
the patches I was Cc-ed on." I thought you wanted to see the whole full series.
Looks like i misunderstood the comment and Will remove the from cc-list in next
version.

The function itself will be needed in further patches in this series.
I am okay with keeping static and other things same here to avoid MISRA violation
and change wherever they are called first time.

Regarding Julien's comment i am reply back in same thread on why this was
not taken in account.
> Jan