Re: [PATCH] [net-next] net: hns3: add IOMMU_SUPPORT dependency

[Robin Murphy]

On 2024-11-04 10:50 am, Salil Mehta wrote:
> >   From: Salil Mehta <salil.mehta@xxxxxxxxxx>
> >   Sent: Monday, November 4, 2024 10:41 AM
> >   To: Robin Murphy <robin.murphy@xxxxxxx>; Arnd Bergmann
> >   <arnd@xxxxxxxxxx>; shenjian (K) <shenjian15@xxxxxxxxxx>
> >   
> >   HI Robin,
> >   
> >   >  From: Robin Murphy <robin.murphy@xxxxxxx>
> >   >  Sent: Monday, November 4, 2024 10:29 AM
> >   >  To: Arnd Bergmann <arnd@xxxxxxxxxx>; shenjian (K)
> >   > <shenjian15@xxxxxxxxxx>; Salil Mehta <salil.mehta@xxxxxxxxxx>
> >   >  Cc: Arnd Bergmann <arnd@xxxxxxxx>; Will Deacon <will@xxxxxxxxxx>;
> >   > Joerg Roedel <jroedel@xxxxxxx>; iommu@xxxxxxxxxxxxxxx; Andrew Lunn
> >   > <andrew+netdev@xxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>;
> >   Eric
> >   > Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>;
> >   > Paolo Abeni <pabeni@xxxxxxxxxx>; shaojijie <shaojijie@xxxxxxxxxx>;
> >   > wangpeiyang <wangpeiyang1@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> >   > linux-kernel@xxxxxxxxxxxxxxx
> >   >  Subject: Re: [PATCH] [net-next] net: hns3: add IOMMU_SUPPORT
> >   > dependency
> >   >
> >   >  On 2024-11-04 8:21 am, Arnd Bergmann wrote:
> >   >  > From: Arnd Bergmann <arnd@xxxxxxxx>  >  > The hns3 driver started
> >   > filling iommu_iotlb_gather structures itself,  > which requires
> >   > CONFIG_IOMMU_SUPPORT is enabled:
> >   >  >
> >   >  > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c: In function
> >   >  'hns3_dma_map_sync':
> >   >  > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:395:14: error:
> >   > 'struct  iommu_iotlb_gather' has no member named 'start'
> >   >  >    395 |  iotlb_gather.start = iova;
> >   >  >        |              ^
> >   >  > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:396:14: error:
> >   > 'struct  iommu_iotlb_gather' has no member named 'end'
> >   >  >    396 |  iotlb_gather.end = iova + granule - 1;
> >   >  >        |              ^
> >   >  > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:397:14: error:
> >   > 'struct  iommu_iotlb_gather' has no member named 'pgsize'
> >   >  >    397 |  iotlb_gather.pgsize = granule;
> >   >  >        |              ^
> >   >  >
> >   >  > Add a Kconfig dependency to make it build in random configurations.
> >   >  >
> >   >  > Cc: Will Deacon <will@xxxxxxxxxx>
> >   >  > Cc: Joerg Roedel <jroedel@xxxxxxx>
> >   >  > Cc: Robin Murphy <robin.murphy@xxxxxxx>  > Cc:
> >   > iommu@xxxxxxxxxxxxxxx  > Fixes: f2c14899caba ("net: hns3: add sync
> >   > command to sync io-pgtable")  > Signed-off-by: Arnd Bergmann
> >   > <arnd@xxxxxxxx>  > ---  > I noticed that no other driver does this, so
> >   > it would be good to have  > a confirmation from the iommu maintainers
> >   > that this is how the  > interface and the dependency is intended to be
> >   > used.
> >   >
> >   >  WTF is that patch doing!? No, random device drivers should absolutely
> >   > not  be poking into IOMMU driver internals, this is egregiously wrong
> >   > and the  correct action is to drop it entirely.
> >   
> >   
> >   Absolutely agree with it. Sorry I haven't been in touch for quite some time.
> >   Let me catch the whole story.  Feel free to drop this patch.
>
>
> Just to make it clear I meant the culprit patch:
> https://lore.kernel.org/netdev/20241025092938.2912958-3-shaojijie@xxxxxxxxxx/

Right, if the HIP09 SMMU has a bug which requires an 
iommu_domain_ops::iotlb_sync_map workaround, then the SMMU driver should 
detect the HIP09 SMMU and implement that workaround for it. HNS3 trying 
to reach in to the SMMU driver's data and open-code iotlb_sync_map on 
its behalf is just as plain illogical as it is unacceptable.

Thanks,
Robin.