diff mbox series

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

Message ID 20241104082129.3142694-1-arnd@kernel.org (mailing list archive)
State Rejected
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: hns3: add IOMMU_SUPPORT dependency | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 4 this patch: 4
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 7 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-11-06--12-00 (tests: 782)

Commit Message

Arnd Bergmann Nov. 4, 2024, 8:21 a.m. UTC 
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.
---
 drivers/net/ethernet/hisilicon/Kconfig | 1 +
 1 file changed, 1 insertion(+)

Comments

Robin Murphy Nov. 4, 2024, 10:29 a.m. UTC | #1 
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.

Thanks,
Robin.

> ---
>   drivers/net/ethernet/hisilicon/Kconfig | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/hisilicon/Kconfig b/drivers/net/ethernet/hisilicon/Kconfig
> index 65302c41bfb1..790efc8d2de6 100644
> --- a/drivers/net/ethernet/hisilicon/Kconfig
> +++ b/drivers/net/ethernet/hisilicon/Kconfig
> @@ -91,6 +91,7 @@ config HNS_ENET
>   config HNS3
>   	tristate "Hisilicon Network Subsystem Support HNS3 (Framework)"
>   	depends on PCI
> +	depends on IOMMU_SUPPORT
>   	select NET_DEVLINK
>   	select PAGE_POOL
>   	help
Salil Mehta Nov. 4, 2024, 10:40 a.m. UTC | #2 
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.

Thanks
Salil.

>  
>  Thanks,
>  Robin.
>  
>  > ---
>  >   drivers/net/ethernet/hisilicon/Kconfig | 1 +
>  >   1 file changed, 1 insertion(+)
>  >
>  > diff --git a/drivers/net/ethernet/hisilicon/Kconfig
>  > b/drivers/net/ethernet/hisilicon/Kconfig
>  > index 65302c41bfb1..790efc8d2de6 100644
>  > --- a/drivers/net/ethernet/hisilicon/Kconfig
>  > +++ b/drivers/net/ethernet/hisilicon/Kconfig
>  > @@ -91,6 +91,7 @@ config HNS_ENET
>  >   config HNS3
>  >   	tristate "Hisilicon Network Subsystem Support HNS3 (Framework)"
>  >   	depends on PCI
>  > +	depends on IOMMU_SUPPORT
>  >   	select NET_DEVLINK
>  >   	select PAGE_POOL
>  >   	help
>
Salil Mehta Nov. 4, 2024, 10:50 a.m. UTC | #3 
>  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/


>  
>  Thanks
>  Salil.
>  
>  >
>  >  Thanks,
>  >  Robin.
>  >
>  >  > ---
>  >  >   drivers/net/ethernet/hisilicon/Kconfig | 1 +
>  >  >   1 file changed, 1 insertion(+)
>  >  >
>  >  > diff --git a/drivers/net/ethernet/hisilicon/Kconfig
>  >  > b/drivers/net/ethernet/hisilicon/Kconfig
>  >  > index 65302c41bfb1..790efc8d2de6 100644  > ---
>  > a/drivers/net/ethernet/hisilicon/Kconfig
>  >  > +++ b/drivers/net/ethernet/hisilicon/Kconfig
>  >  > @@ -91,6 +91,7 @@ config HNS_ENET
>  >  >   config HNS3
>  >  >   	tristate "Hisilicon Network Subsystem Support HNS3 (Framework)"
>  >  >   	depends on PCI
>  >  > +	depends on IOMMU_SUPPORT
>  >  >   	select NET_DEVLINK
>  >  >   	select PAGE_POOL
>  >  >   	help
>  >
Pranjal Shrivastava Nov. 4, 2024, 12:01 p.m. UTC | #4 
On Mon, Nov 04, 2024 at 10:29:28AM +0000, Robin Murphy wrote:
> 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.

+1. Device drivers shouldn't poke into IOMMU internals.

> 
> Thanks,
> Robin.

I see that the requirement in [1] is to somehow flush / sync the iotlb
right after dma mapping an address as a work around for a HW bug, right?

I'd like to point out that the dma_map* API take care of this if the
underlying iommu implements the `iotlb_sync_map` op (check iommu_map).

Thus, in case you require it and your iommu driver does NOT support
`iotlb_sync_map`, the right way would be to add this functionality to
*that* iommu driver instead of letting the device driver poke into IOMMU

Another thing worth noting is, we do have some quirks to address broken
hardware in the iommu drivers. For example, a broken pre-fetch command
for hisilicon HI161x in arm-smmu-v3 driver[2]. Such options can be added
to iommu drivers after discussion with the relevant stakeholders.

Thanks,
Praan

[1]
 https://lore.kernel.org/netdev/20241025092938.2912958-2-shaojijie@huawei.com/

[2]
 https://elixir.bootlin.com/linux/v6.11.6/source/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c#L81

> 
> > ---
> >   drivers/net/ethernet/hisilicon/Kconfig | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/net/ethernet/hisilicon/Kconfig b/drivers/net/ethernet/hisilicon/Kconfig
> > index 65302c41bfb1..790efc8d2de6 100644
> > --- a/drivers/net/ethernet/hisilicon/Kconfig
> > +++ b/drivers/net/ethernet/hisilicon/Kconfig
> > @@ -91,6 +91,7 @@ config HNS_ENET
> >   config HNS3
> >   	tristate "Hisilicon Network Subsystem Support HNS3 (Framework)"
> >   	depends on PCI
> > +	depends on IOMMU_SUPPORT
> >   	select NET_DEVLINK
> >   	select PAGE_POOL
> >   	help
> 
>
Robin Murphy Nov. 4, 2024, 4:34 p.m. UTC | #5 
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.
Jakub Kicinski Nov. 5, 2024, 2:10 a.m. UTC | #6 
On Mon, 4 Nov 2024 10:29:28 +0000 Robin Murphy wrote:
> 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.

Sorry, reverted in
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=249cfa318fb1b77eb726c2ff4f74c9685f04e568
Salil Mehta Nov. 6, 2024, 7:39 p.m. UTC | #7 
Hi Robin,

Sorry for the late reply. I was on a leave yesterday.

>  From: Robin Murphy <robin.murphy@arm.com>
>  Sent: Monday, November 4, 2024 4:35 PM
>  To: Salil Mehta <salil.mehta@huawei.com>; Arnd Bergmann
>  <arnd@kernel.org>; shenjian (K) <shenjian15@huawei.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@huaw
>  > ei.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.


Totally agree with that.  It breaks the design of the kernel fundamentally.
This was a mistake and on Monday I've requested the stakeholders to
address it exactly in the realms of what you have suggested above. 

Rest assured, we will address that in the correct way. Sorry, if this caused
any inconvenience to anymore including the MAINTAINER of `netdev`
I would have caught this much earlier had I been following the ML closely
but unfortunately I've not been in touch for long.

Thanks for the `nudge` though
diff mbox series

Patch

diff --git a/drivers/net/ethernet/hisilicon/Kconfig b/drivers/net/ethernet/hisilicon/Kconfig
index 65302c41bfb1..790efc8d2de6 100644
--- a/drivers/net/ethernet/hisilicon/Kconfig
+++ b/drivers/net/ethernet/hisilicon/Kconfig
@@ -91,6 +91,7 @@  config HNS_ENET
 config HNS3
 	tristate "Hisilicon Network Subsystem Support HNS3 (Framework)"
 	depends on PCI
+	depends on IOMMU_SUPPORT
 	select NET_DEVLINK
 	select PAGE_POOL
 	help