mm/gup: migrate device coherent pages when pinning instead of failing

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?

    844         migrate_vma_finalize(&args);
    845
    846         if (src_pfn & MIGRATE_PFN_MIGRATE)
    847                 return 0;

Same check again.

    848         return -EBUSY;
    849 }

regards,
dan carpenter

Quoted 48 lines · Dan Carpenter

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

Quoted 1 line

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.