Re: [bug report] mm/gup: migrate device coherent pages when pinning instead of failing

[Dan Carpenter <dan.carpenter@oracle.com>]

On Tue, Jul 19, 2022 at 10:04:57PM +1000, Alistair Popple wrote:
> 
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> 
> > Hello Alistair Popple,
> >
> > The patch b05a79d4377f: "mm/gup: migrate device coherent pages when
> > pinning instead of failing" from Jul 15, 2022, leads to the following
> > Smatch static checker warning:
> >
> > 	mm/migrate_device.c:842 migrate_device_coherent_page()
> > 	warn: duplicate check 'src_pfn & (1 << 1)' (previous on line 832)
> >
> > mm/migrate_device.c
> >     810 int migrate_device_coherent_page(struct page *page)
> >     811 {
> >     812         unsigned long src_pfn, dst_pfn = 0;
> >     813         struct migrate_vma args;
> >     814         struct page *dpage;
> >     815
> >     816         WARN_ON_ONCE(PageCompound(page));
> >     817
> >     818         lock_page(page);
> >     819         src_pfn = migrate_pfn(page_to_pfn(page)) | MIGRATE_PFN_MIGRATE;
> >     820         args.src = &src_pfn;
> >     821         args.dst = &dst_pfn;
> >     822         args.cpages = 1;
> >     823         args.npages = 1;
> >     824         args.vma = NULL;
> >     825
> >     826         /*
> >     827          * We don't have a VMA and don't need to walk the page tables to find
> >     828          * the source page. So call migrate_vma_unmap() directly to unmap the
> >     829          * page as migrate_vma_setup() will fail if args.vma == NULL.
> >     830          */
> >     831         migrate_vma_unmap(&args);
> >     832         if (!(src_pfn & MIGRATE_PFN_MIGRATE))
> >     833                 return -EBUSY;
> >
> > Return here.
> >
> >     834
> >     835         dpage = alloc_page(GFP_USER | __GFP_NOWARN);
> >     836         if (dpage) {
> >     837                 lock_page(dpage);
> >     838                 dst_pfn = migrate_pfn(page_to_pfn(dpage));
> >     839         }
> >     840
> >     841         migrate_vma_pages(&args);
> > --> 842         if (src_pfn & MIGRATE_PFN_MIGRATE)
> >     843                 copy_highpage(dpage, page);
> >
> > No need to check again.  Was dst_pfn intended?
> 
> No. The check is necessary and correct. migrate_vma_pages() may clear
> MIGRATE_PFN_MIGRATE from src_pfn if the page could not be migrated for
> some reason.

Ah.  Sorry.  I missed that we save args.src = &src_pfn at the top.  I'll
update the checker for this.  It might take a week or two to get around
to it...

regards,
dan carpenter