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

[Vipin Sharma <vipinsh@google.com>]

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.

> +EXPORT_SYMBOL_GPL(iommu_unpreserve_pages);
> +
> +void iommu_restore_page(u64 phys)
> +{
> +	struct ioptdesc *iopt;
> +	struct folio *folio;
> +	unsigned long pgcnt;
> +	unsigned int order;
> +
> +	folio = kho_restore_folio(phys);
> +	BUG_ON(!folio);
> +
> +	iopt = folio_ioptdesc(folio);
> +
> +	order = folio_order(folio);
> +	pgcnt = 1UL << order;
> +	mod_node_page_state(folio_pgdat(folio), NR_IOMMU_PAGES, pgcnt);
> +	lruvec_stat_mod_folio(folio, NR_SECONDARY_PAGETABLE, pgcnt);

Above two functions are also used in other two places in this file, lets
create a common function for these two operations.

> +}
> +EXPORT_SYMBOL_GPL(iommu_restore_page);
> +