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

[Robin Murphy <robin.murphy@arm.com>]

On 2024-11-04 10:50 am, Salil Mehta wrote:
>>   From: Salil Mehta <salil.mehta@huawei.com>
>>   Sent: Monday, November 4, 2024 10:41 AM
>>   To: Robin Murphy <robin.murphy@arm.com>; Arnd Bergmann
>>   <arnd@kernel.org>; shenjian (K) <shenjian15@huawei.com>
>>   
>>   HI Robin,
>>   
>>   >  From: Robin Murphy <robin.murphy@arm.com>
>>   >  Sent: Monday, November 4, 2024 10:29 AM
>>   >  To: Arnd Bergmann <arnd@kernel.org>; shenjian (K)
>>   > <shenjian15@huawei.com>; Salil Mehta <salil.mehta@huawei.com>
>>   >  Cc: Arnd Bergmann <arnd@arndb.de>; Will Deacon <will@kernel.org>;
>>   > Joerg Roedel <jroedel@suse.de>; iommu@lists.linux.dev; Andrew Lunn
>>   > <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>;
>>   Eric
>>   > Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
>>   > Paolo Abeni <pabeni@redhat.com>; shaojijie <shaojijie@huawei.com>;
>>   > wangpeiyang <wangpeiyang1@huawei.com>; netdev@vger.kernel.org;
>>   > linux-kernel@vger.kernel.org
>>   >  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@arndb.de>  >  > 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@kernel.org>
>>   >  > Cc: Joerg Roedel <jroedel@suse.de>
>>   >  > Cc: Robin Murphy <robin.murphy@arm.com>  > Cc:
>>   > iommu@lists.linux.dev  > Fixes: f2c14899caba ("net: hns3: add sync
>>   > command to sync io-pgtable")  > Signed-off-by: Arnd Bergmann
>>   > <arnd@arndb.de>  > ---  > 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@huawei.com/

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.