Re: [PATCH v2 06/11] drm/panfrost: Give back the pages of a heap chunk that failed to map

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/panfrost: false drm_WARN_ON() in
  panfrost_mmu_map_fault_addr()
--

commit 455071c10d7f5d95d3ce18757e41ef79db6b9400
Author: Arun Iyengar <aiyengar1990@gmail.com>

drm/panfrost: Give back the pages of a heap chunk that failed to map

This commit releases the shmem folios read during a heap BO fault and
clears their entries in bo->base.pages if mapping the 2 MB fault chunk
fails.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 35fb86b6b3242..6847359374e53 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
[ ... ]
> @@ -670,17 +696,23 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>  	mapping = bo->base.base.filp->f_mapping;
>  	mapping_set_unevictable(mapping);
>  
> +	/* The page array range whose folio references this fault takes. */
> +	got_start = page_offset;
> +	got_end = page_offset;
> +
>  	for (pgoff_t pg = page_offset; pg < page_offset + NUM_FAULT_PAGES;) {
>  		bool already_owned = false;
>  		struct folio *folio;
> +		pgoff_t first;
>  
>  		folio = shmem_read_folio(mapping, pg);
>  		if (IS_ERR(folio)) {
>  			ret = PTR_ERR(folio);
> -			goto err_unlock;
> +			goto err_put_chunk;
>  		}
>  
>  		pg &= ~(folio_nr_pages(folio) - 1);
> +		first = pg;
>  		for (u32 i = 0; i < folio_nr_pages(folio) && pg < nr_pages; i++) {
>  			if (pages[pg])
>  				already_owned = true;
> @@ -692,14 +724,18 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>  		 * there's no valid reason for a folio range to be partially
>  		 * populated.
>  		 */
> -		if (drm_WARN_ON(&pfdev->base, already_owned))
> +		if (drm_WARN_ON(&pfdev->base, already_owned)) {
>  			folio_put(folio);
> +		} else {
> +			got_start = min(got_start, first);
> +			got_end = max(got_end, pg);
> +		}
>  	}

[Severity: Medium]
Should drm_WARN_ON() be removed when already_owned is true for a folio
larger than 2 MB?

When shmem returns a folio larger than the 2 MB fault chunk (such as a
PMD-sized or multi-megabyte THP folio on 16K or 64K page kernels), the
first chunk fault populates pages[] for the entire folio across
neighbouring 2 MB chunks, while only mapping the sg_table for the faulting
2 MB chunk.  The new else branch relies on already_owned being true when
those neighbouring 2 MB chunks later fault so that [got_start, got_end)
does not claim their shared folio, but keeping drm_WARN_ON(&pfdev->base,
already_owned) in the condition triggers a warning on every normal fault
of those neighbouring chunks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-panfrost-long-jobs-v2-0-a06830da3d99@gmail.com?part=6