Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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.
> 844 migrate_vma_finalize(&args);
> 845
> 846 if (src_pfn & MIGRATE_PFN_MIGRATE)
> 847 return 0;
>
> Same check again.
I don't see the problem with this?
- Alistair
> 848 return -EBUSY;
> 849 }
>
> regards,
> dan carpenter
