Re: [PATCH 04/14] iommu/pages: Add APIs to preserve/unpreserve/restore iommu pages

[Pranjal Shrivastava]

On Tue, Mar 17, 2026 at 01:59:03PM -0700, Vipin Sharma wrote:
> On Tue, Feb 03, 2026 at 10:09:38PM +0000, Samiullah Khawaja wrote:
> > +
> > +void iommu_unpreserve_pages(struct iommu_pages_list *list, int count)
> > +{
> > +	struct ioptdesc *iopt;
> > +
> > +	if (!count)
> > +		return;
> > +
> > +	/* If less than zero then unpreserve all pages. */
> > +	if (count < 0)
> > +		count = 0;
> > +
> > +	list_for_each_entry(iopt, &list->pages, iopt_freelist_elm) {
> > +		kho_unpreserve_folio(ioptdesc_folio(iopt));
> > +		if (count > 0 && --count ==  0)
> > +			break;
> > +	}
> > +}
> 
> I see you are trying to have common function for error handling in
> iommu_preserve_pages() down and unpreserving in the next patch with
> overloaded meaning of count. 
> 
>  < 0 means unpreserve all of the pages
>  = 0 means do nothing
>  > 0 unpreserve these many pages.
> 
> It seems non-intuitive to me and code also end up having multiple ifs. I
> will recommend to just have this function go through all of the pages
> without passing count argument. In iommu_preserve_pages() function down,
> handle the error case with a "goto err" statement. If this is the way
> things happen in iommu codebase feel free to ignore this suggestion.
> 

+1 to this. The current version of overloaded count is counter-intuitive
as it expects a back-handed API contract. Since we're using lists here,
I'd recommend using the list_for_each_entry_continue_reverse() helper:

+void iommu_unpreserve_pages(struct iommu_pages_list *list)
+{
+	struct ioptdesc *iopt;
+
+	list_for_each_entry(iopt, &list->pages, iopt_freelist_elm)
+		kho_unpreserve_folio(ioptdesc_folio(iopt));
+}
+EXPORT_SYMBOL_GPL(iommu_unpreserve_pages);
+
+int iommu_preserve_pages(struct iommu_pages_list *list)
+{
+	struct ioptdesc *iopt;
+	int ret;
+
+	list_for_each_entry(iopt, &list->pages, iopt_freelist_elm) {
+		ret = kho_preserve_folio(ioptdesc_folio(iopt));
+		if (ret)
+			goto err_rollback;
+	}
+	return 0;
+
+err_rollback:
+	/* Rollback only the successfully preserved pages */
+	list_for_each_entry_continue_reverse(iopt, &list->pages, iopt_freelist_elm)
+		kho_unpreserve_folio(ioptdesc_folio(iopt));
+	return ret;
+}

[ ---- snip >8---- ]

Thanks,
Praan