Re: [PATCH 1/2] iommu: Fix iommu_unmap and iommu_unmap_fast return type

[Robin Murphy]

On 01/02/18 05:03, Suravee Suthikulpanit wrote:
> Hi Robin,
>
> On 2/1/18 1:02 AM, Robin Murphy wrote:
> > HiÂSuravee,
> >
> > OnÂ31/01/18Â01:48,ÂSuraveeÂSuthikulpanitÂwrote:
> > > Currently,Âiommu_unmapÂandÂiommu_unmap_fastÂreturnÂunmapped
> > > pagesÂwithÂsize_t.ÂÂHowever,ÂtheÂactualÂvalueÂreturnedÂcould
> > > beÂerrorÂcodesÂ(<Â0),ÂwhichÂcanÂbeÂmisinterpretedÂasÂlarge
> > > numberÂofÂunmappedÂpages.ÂTherefore,ÂchangeÂtheÂreturnÂtypeÂtoÂssize_t.
> > >
> > > Cc:ÂJoergÂRoedel <joro@xxxxxxxxxx>
> > > Cc:ÂAlexÂWilliamson <alex.williamson@xxxxxxxxxx>
> > > Signed-off-by:ÂSuraveeÂSuthikulpanit <suravee.suthikulpanit@xxxxxxx>
> > > ---
> > > ÂÂdrivers/iommu/amd_iommu.cÂÂÂ|ÂÂ6Â+++---
> > > ÂÂdrivers/iommu/intel-iommu.cÂ|ÂÂ4Â++--
> >
> > Er,ÂthereÂareÂaÂfewÂmoreÂdriversÂthanÂthatÂimplementingÂiommu_ops ;)
>
> Ahh right.
> > It seems like it might be more sensible to fix the single instance of 
> > a driver returning -EINVAL (which appears to be a "should never happen 
> > if used correctly" kinda thing anyway) and leave the API-internal 
> > callback prototype as-is. I do agree the inconsistency of 
> > iommu_unmap() itself wants sorting, though (particularly the 
> > !IOMMU_API stubs which are wrong eitherÂway).
> >
> > Robin.
>
> Make sense. I'll leave the API alone, and change the code to not 
> returning error then.

Actually, on a second look I think that check in amd_iommu is 99% 
redundant anyway, since PAGE_MODE_NONE is only normally set for 
IOMMU_DOMAIN_IDENTITY domains, thus iommu_unmap() would have bailed out 
from the __IOMMU_DOMAIN_PAGING check before ops->unmap could be called. 
AFAICS the only way to hit it at all is if a caller somehow managed to 
get hold of the dev_state->domain set up in amd_iommu_init_device(), 
then tried to unmap something from that, which seems such a very wrong 
thing to do it should probably just crash and burn with extreme 
prejudice anyway.

Cheers,
Robin.