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

Vipin Sharma
2026-03-17T20:59:09Z

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);
> +