Re: linux-next: Tree for Oct 3 (net/smc/smc_ism.o)

Re: linux-next: Tree for Oct 3 (net/smc/smc_ism.o)

[Randy Dunlap]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]



On 10/2/23 20:33, Stephen Rothwell wrote:
> Hi all,
> 
> Changes since 20230929:
> 

on s390: when

CONFIG_ISM=m
CONFIG_SMC=y

s390-linux-ld: net/smc/smc_ism.o: in function `smcd_register_dev':
smc_ism.c:(.text+0x55c): undefined reference to `ism_get_smcd_ops'
s390-linux-ld: net/smc/smc_ism.o: in function `smc_ism_init':
smc_ism.c:(.text+0x1dc2): undefined reference to `ism_register_client'
s390-linux-ld: net/smc/smc_ism.o: in function `smc_ism_exit':
smc_ism.c:(.text+0x1e08): undefined reference to `ism_unregister_client'


Full randconfig file is attached.

-- 
~Randy

[-- Attachment #2: config-r8109.gz --]
[-- Type: application/gzip, Size: 30465 bytes --]

Re: linux-next: Tree for Oct 3 (net/smc/smc_ism.o)

[Wenjia Zhang]

On 04.10.23 03:08, Randy Dunlap wrote:
> 
> 
> On 10/2/23 20:33, Stephen Rothwell wrote:
>> Hi all,
>>
>> Changes since 20230929:
>>
> 
> on s390: when
> 
> CONFIG_ISM=m
> CONFIG_SMC=y
> 
> s390-linux-ld: net/smc/smc_ism.o: in function `smcd_register_dev':
> smc_ism.c:(.text+0x55c): undefined reference to `ism_get_smcd_ops'
> s390-linux-ld: net/smc/smc_ism.o: in function `smc_ism_init':
> smc_ism.c:(.text+0x1dc2): undefined reference to `ism_register_client'
> s390-linux-ld: net/smc/smc_ism.o: in function `smc_ism_exit':
> smc_ism.c:(.text+0x1e08): undefined reference to `ism_unregister_client'
> 
> 
> Full randconfig file is attached.
> 
Hi Randy,

Thank you for the finding! I'll send the fix soon.

Best
Wenjia

[PATCH net] net/smc: Fix dependency of SMC on ISM

[Gerd Bayer]

When the SMC protocol is built into the kernel proper while ISM is
configured to be built as module, linking the kernel fails due to
unresolved dependencies out of net/smc/smc_ism.o to
ism_get_smcd_ops, ism_register_client, and ism_unregister_client
as reported via the linux-next test automation (see link).
This however is a bug introduced a while ago.

Correct the dependency list in ISM's and SMC's Kconfig to reflect the
dependencies that are actually inverted. With this you cannot build a
kernel with CONFIG_SMC=y and CONFIG_ISM=m. Either ISM needs to be 'y',
too - or a 'n'. That way, SMC can still be configured on non-s390
architectures that do not have (nor need) an ISM driver.

Fixes: 89e7d2ba61b7 ("net/ism: Add new API for client registration")

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Closes: https://lore.kernel.org/linux-next/d53b5b50-d894-4df8-8969-fd39e63440ae@infradead.org/
Co-developed-by: Wenjia Zhang <wenjia@linux.ibm.com>
Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>
Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
---
 drivers/s390/net/Kconfig | 2 +-
 net/smc/Kconfig          | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
index 74760c1a163b..4902d45e929c 100644
--- a/drivers/s390/net/Kconfig
+++ b/drivers/s390/net/Kconfig
@@ -102,7 +102,7 @@ config CCWGROUP
 
 config ISM
 	tristate "Support for ISM vPCI Adapter"
-	depends on PCI && SMC
+	depends on PCI
 	default n
 	help
 	  Select this option if you want to use the Internal Shared Memory
diff --git a/net/smc/Kconfig b/net/smc/Kconfig
index 1ab3c5a2c5ad..746be3996768 100644
--- a/net/smc/Kconfig
+++ b/net/smc/Kconfig
@@ -2,6 +2,7 @@
 config SMC
 	tristate "SMC socket protocol family"
 	depends on INET && INFINIBAND
+	depends on m || ISM != m
 	help
 	  SMC-R provides a "sockets over RDMA" solution making use of
 	  RDMA over Converged Ethernet (RoCE) technology to upgrade
-- 
2.39.2

Re: [PATCH net] net/smc: Fix dependency of SMC on ISM

[Simon Horman]

On Fri, Oct 06, 2023 at 02:58:47PM +0200, Gerd Bayer wrote:
> When the SMC protocol is built into the kernel proper while ISM is
> configured to be built as module, linking the kernel fails due to
> unresolved dependencies out of net/smc/smc_ism.o to
> ism_get_smcd_ops, ism_register_client, and ism_unregister_client
> as reported via the linux-next test automation (see link).
> This however is a bug introduced a while ago.
> 
> Correct the dependency list in ISM's and SMC's Kconfig to reflect the
> dependencies that are actually inverted. With this you cannot build a
> kernel with CONFIG_SMC=y and CONFIG_ISM=m. Either ISM needs to be 'y',
> too - or a 'n'. That way, SMC can still be configured on non-s390
> architectures that do not have (nor need) an ISM driver.
> 
> Fixes: 89e7d2ba61b7 ("net/ism: Add new API for client registration")
> 
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Closes: https://lore.kernel.org/linux-next/d53b5b50-d894-4df8-8969-fd39e63440ae@infradead.org/
> Co-developed-by: Wenjia Zhang <wenjia@linux.ibm.com>
> Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>

Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Simon Horman <horms@kernel.org> # build-tested

Re: [PATCH net] net/smc: Fix dependency of SMC on ISM

[Randy Dunlap]

Hi,

On 10/6/23 05:58, Gerd Bayer wrote:
> When the SMC protocol is built into the kernel proper while ISM is
> configured to be built as module, linking the kernel fails due to
> unresolved dependencies out of net/smc/smc_ism.o to
> ism_get_smcd_ops, ism_register_client, and ism_unregister_client
> as reported via the linux-next test automation (see link).
> This however is a bug introduced a while ago.
> 
> Correct the dependency list in ISM's and SMC's Kconfig to reflect the
> dependencies that are actually inverted. With this you cannot build a
> kernel with CONFIG_SMC=y and CONFIG_ISM=m. Either ISM needs to be 'y',
> too - or a 'n'. That way, SMC can still be configured on non-s390
> architectures that do not have (nor need) an ISM driver.
> 
> Fixes: 89e7d2ba61b7 ("net/ism: Add new API for client registration")
> 
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Closes: https://lore.kernel.org/linux-next/d53b5b50-d894-4df8-8969-fd39e63440ae@infradead.org/
> Co-developed-by: Wenjia Zhang <wenjia@linux.ibm.com>
> Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>

Works for me. Thanks.

Acked-by: Randy Dunlap <rdunlap@infradead.org>
Tested-by: Randy Dunlap <rdunlap@infradead.org> # build-tested


> ---
>  drivers/s390/net/Kconfig | 2 +-
>  net/smc/Kconfig          | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
> index 74760c1a163b..4902d45e929c 100644
> --- a/drivers/s390/net/Kconfig
> +++ b/drivers/s390/net/Kconfig
> @@ -102,7 +102,7 @@ config CCWGROUP
>  
>  config ISM
>  	tristate "Support for ISM vPCI Adapter"
> -	depends on PCI && SMC
> +	depends on PCI
>  	default n
>  	help
>  	  Select this option if you want to use the Internal Shared Memory
> diff --git a/net/smc/Kconfig b/net/smc/Kconfig
> index 1ab3c5a2c5ad..746be3996768 100644
> --- a/net/smc/Kconfig
> +++ b/net/smc/Kconfig
> @@ -2,6 +2,7 @@
>  config SMC
>  	tristate "SMC socket protocol family"
>  	depends on INET && INFINIBAND
> +	depends on m || ISM != m
>  	help
>  	  SMC-R provides a "sockets over RDMA" solution making use of
>  	  RDMA over Converged Ethernet (RoCE) technology to upgrade

-- 
~Randy

Re: [PATCH net] net/smc: Fix dependency of SMC on ISM

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri,  6 Oct 2023 14:58:47 +0200 you wrote:
> When the SMC protocol is built into the kernel proper while ISM is
> configured to be built as module, linking the kernel fails due to
> unresolved dependencies out of net/smc/smc_ism.o to
> ism_get_smcd_ops, ism_register_client, and ism_unregister_client
> as reported via the linux-next test automation (see link).
> This however is a bug introduced a while ago.
> 
> [...]

Here is the summary with links:
  - [net] net/smc: Fix dependency of SMC on ISM
    https://git.kernel.org/netdev/net/c/a72178cfe855

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] net/smc: Fix dependency of SMC on ISM

[Randy Dunlap]

Hi Gerd,

On 10/6/23 05:58, Gerd Bayer wrote:
> When the SMC protocol is built into the kernel proper while ISM is
> configured to be built as module, linking the kernel fails due to
> unresolved dependencies out of net/smc/smc_ism.o to
> ism_get_smcd_ops, ism_register_client, and ism_unregister_client
> as reported via the linux-next test automation (see link).
> This however is a bug introduced a while ago.
> 
> Correct the dependency list in ISM's and SMC's Kconfig to reflect the
> dependencies that are actually inverted. With this you cannot build a
> kernel with CONFIG_SMC=y and CONFIG_ISM=m. Either ISM needs to be 'y',
> too - or a 'n'. That way, SMC can still be configured on non-s390
> architectures that do not have (nor need) an ISM driver.
> 
> Fixes: 89e7d2ba61b7 ("net/ism: Add new API for client registration")
> 
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Closes: https://lore.kernel.org/linux-next/d53b5b50-d894-4df8-8969-fd39e63440ae@infradead.org/
> Co-developed-by: Wenjia Zhang <wenjia@linux.ibm.com>
> Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>

With this patch, ISM can be build without SMC being enabled.

This leads to some build warnings:

../drivers/s390/net/ism_drv.c:572:12: warning: 'ism_get_local_gid' defined but not used [-Wunused-function]
  572 | static u64 ism_get_local_gid(struct ism_dev *ism)
      |            ^~~~~~~~~~~~~~~~~
../drivers/s390/net/ism_drv.c:506:12: warning: 'ism_get_chid' defined but not used [-Wunused-function]
  506 | static u16 ism_get_chid(struct ism_dev *ism)
      |            ^~~~~~~~~~~~
../drivers/s390/net/ism_drv.c:432:12: warning: 'ism_signal_ieq' defined but not used [-Wunused-function]
  432 | static int ism_signal_ieq(struct ism_dev *ism, u64 rgid, u32 trigger_irq,
      |            ^~~~~~~~~~~~~~
../drivers/s390/net/ism_drv.c:292:12: warning: 'ism_query_rgid' defined but not used [-Wunused-function]
  292 | static int ism_query_rgid(struct ism_dev *ism, u64 rgid, u32 vid_valid,
      |            ^~~~~~~~~~~~~~

> ---
>  drivers/s390/net/Kconfig | 2 +-
>  net/smc/Kconfig          | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
> index 74760c1a163b..4902d45e929c 100644
> --- a/drivers/s390/net/Kconfig
> +++ b/drivers/s390/net/Kconfig
> @@ -102,7 +102,7 @@ config CCWGROUP
>  
>  config ISM
>  	tristate "Support for ISM vPCI Adapter"
> -	depends on PCI && SMC
> +	depends on PCI
>  	default n
>  	help
>  	  Select this option if you want to use the Internal Shared Memory
> diff --git a/net/smc/Kconfig b/net/smc/Kconfig
> index 1ab3c5a2c5ad..746be3996768 100644
> --- a/net/smc/Kconfig
> +++ b/net/smc/Kconfig
> @@ -2,6 +2,7 @@
>  config SMC
>  	tristate "SMC socket protocol family"
>  	depends on INET && INFINIBAND
> +	depends on m || ISM != m
>  	help
>  	  SMC-R provides a "sockets over RDMA" solution making use of
>  	  RDMA over Converged Ethernet (RoCE) technology to upgrade

-- 
~Randy

Re: [PATCH net] net/smc: Fix dependency of SMC on ISM

[Gerd Bayer]

On Mon, 2023-10-16 at 16:09 -0700, Randy Dunlap wrote:
> Hi Gerd,
> 
> On 10/6/23 05:58, Gerd Bayer wrote:
> > When the SMC protocol is built into the kernel proper while ISM is
> > configured to be built as module, linking the kernel fails due to
> > unresolved dependencies out of net/smc/smc_ism.o to
> > ism_get_smcd_ops, ism_register_client, and ism_unregister_client
> > as reported via the linux-next test automation (see link).
> > This however is a bug introduced a while ago.
> > 
> > Correct the dependency list in ISM's and SMC's Kconfig to reflect
> > the
> > dependencies that are actually inverted. With this you cannot build
> > a
> > kernel with CONFIG_SMC=y and CONFIG_ISM=m. Either ISM needs to be
> > 'y',
> > too - or a 'n'. That way, SMC can still be configured on non-s390
> > architectures that do not have (nor need) an ISM driver.
> > 
> > Fixes: 89e7d2ba61b7 ("net/ism: Add new API for client
> > registration")
> > 
> > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > Closes:
> > https://lore.kernel.org/linux-next/d53b5b50-d894-4df8-8969-fd39e63440ae@infradead.org/
> > Co-developed-by: Wenjia Zhang <wenjia@linux.ibm.com>
> > Signed-off-by: Wenjia Zhang <wenjia@linux.ibm.com>
> > Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> 
> With this patch, ISM can be build without SMC being enabled.
> 
> This leads to some build warnings:
> 
> ../drivers/s390/net/ism_drv.c:572:12: warning: 'ism_get_local_gid'
> defined but not used [-Wunused-function]
>   572 | static u64 ism_get_local_gid(struct ism_dev *ism)
>       |            ^~~~~~~~~~~~~~~~~
> ../drivers/s390/net/ism_drv.c:506:12: warning: 'ism_get_chid' defined
> but not used [-Wunused-function]
>   506 | static u16 ism_get_chid(struct ism_dev *ism)
>       |            ^~~~~~~~~~~~
> ../drivers/s390/net/ism_drv.c:432:12: warning: 'ism_signal_ieq'
> defined but not used [-Wunused-function]
>   432 | static int ism_signal_ieq(struct ism_dev *ism, u64 rgid, u32
> trigger_irq,
>       |            ^~~~~~~~~~~~~~
> ../drivers/s390/net/ism_drv.c:292:12: warning: 'ism_query_rgid'
> defined but not used [-Wunused-function]
>   292 | static int ism_query_rgid(struct ism_dev *ism, u64 rgid, u32
> vid_valid,
>       |            ^~~~~~~~~~~~~~

Hi Randy,

I must have missed testing a config of ISM without SMC. I'm working on
a fix.

Thanks for reporting,
Gerd

[PATCH net] s390/ism: ism driver implies smc protocol

[Gerd Bayer]

Since commit a72178cfe855 ("net/smc: Fix dependency of SMC on ISM")
you can build the ism code without selecting the SMC network protocol.
That leaves some ism functions be reported as unused. Move these
functions under the conditional compile with CONFIG_SMC.

Also codify the suggestion to also configure the SMC protocol in ism's
Kconfig - but with an "imply" rather than a "select" as SMC depends on
other config options and allow for a deliberate decision not to build
SMC. Also, mention that in ISM's help.

Fixes: a72178cfe855 ("net/smc: Fix dependency of SMC on ISM")
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Closes: https://lore.kernel.org/netdev/afd142a2-1fa0-46b9-8b2d-7652d41d3ab8@infradead.org/
Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
---

Hi Randy,

sorry for the long wait. We had some internal discussions about how to 
tackle this and decided to send out the short-term solution first and 
work on better isolating the ISM device and SMC protocol together 
with the work to extend ISM,
e.g. at https://lore.kernel.org/netdev/1695568613-125057-1-git-send-email-guwen@linux.alibaba.com/

Cheers, Gerd

 drivers/s390/net/Kconfig   |  3 +-
 drivers/s390/net/ism_drv.c | 92 +++++++++++++++++++-------------------
 2 files changed, 48 insertions(+), 47 deletions(-)

diff --git a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
index 4902d45e929c..c61e6427384c 100644
--- a/drivers/s390/net/Kconfig
+++ b/drivers/s390/net/Kconfig
@@ -103,10 +103,11 @@ config CCWGROUP
 config ISM
 	tristate "Support for ISM vPCI Adapter"
 	depends on PCI
+	imply SMC
 	default n
 	help
 	  Select this option if you want to use the Internal Shared Memory
-	  vPCI Adapter.
+	  vPCI Adapter. The adapter can be used with the SMC network protocol.
 
 	  To compile as a module choose M. The module name is ism.
 	  If unsure, choose N.
diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
index 6df7f377d2f9..ec112a00b135 100644
--- a/drivers/s390/net/ism_drv.c
+++ b/drivers/s390/net/ism_drv.c
@@ -289,22 +289,6 @@ static int ism_read_local_gid(struct ism_dev *ism)
 	return ret;
 }
 
-static int ism_query_rgid(struct ism_dev *ism, u64 rgid, u32 vid_valid,
-			  u32 vid)
-{
-	union ism_query_rgid cmd;
-
-	memset(&cmd, 0, sizeof(cmd));
-	cmd.request.hdr.cmd = ISM_QUERY_RGID;
-	cmd.request.hdr.len = sizeof(cmd.request);
-
-	cmd.request.rgid = rgid;
-	cmd.request.vlan_valid = vid_valid;
-	cmd.request.vlan_id = vid;
-
-	return ism_cmd(ism, &cmd);
-}
-
 static void ism_free_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
 {
 	clear_bit(dmb->sba_idx, ism->sba_bitmap);
@@ -429,23 +413,6 @@ static int ism_del_vlan_id(struct ism_dev *ism, u64 vlan_id)
 	return ism_cmd(ism, &cmd);
 }
 
-static int ism_signal_ieq(struct ism_dev *ism, u64 rgid, u32 trigger_irq,
-			  u32 event_code, u64 info)
-{
-	union ism_sig_ieq cmd;
-
-	memset(&cmd, 0, sizeof(cmd));
-	cmd.request.hdr.cmd = ISM_SIGNAL_IEQ;
-	cmd.request.hdr.len = sizeof(cmd.request);
-
-	cmd.request.rgid = rgid;
-	cmd.request.trigger_irq = trigger_irq;
-	cmd.request.event_code = event_code;
-	cmd.request.info = info;
-
-	return ism_cmd(ism, &cmd);
-}
-
 static unsigned int max_bytes(unsigned int start, unsigned int len,
 			      unsigned int boundary)
 {
@@ -503,14 +470,6 @@ u8 *ism_get_seid(void)
 }
 EXPORT_SYMBOL_GPL(ism_get_seid);
 
-static u16 ism_get_chid(struct ism_dev *ism)
-{
-	if (!ism || !ism->pdev)
-		return 0;
-
-	return to_zpci(ism->pdev)->pchid;
-}
-
 static void ism_handle_event(struct ism_dev *ism)
 {
 	struct ism_event *entry;
@@ -569,11 +528,6 @@ static irqreturn_t ism_handle_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static u64 ism_get_local_gid(struct ism_dev *ism)
-{
-	return ism->local_gid;
-}
-
 static int ism_dev_init(struct ism_dev *ism)
 {
 	struct pci_dev *pdev = ism->pdev;
@@ -774,6 +728,22 @@ module_exit(ism_exit);
 /*************************** SMC-D Implementation *****************************/
 
 #if IS_ENABLED(CONFIG_SMC)
+static int ism_query_rgid(struct ism_dev *ism, u64 rgid, u32 vid_valid,
+			  u32 vid)
+{
+	union ism_query_rgid cmd;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.request.hdr.cmd = ISM_QUERY_RGID;
+	cmd.request.hdr.len = sizeof(cmd.request);
+
+	cmd.request.rgid = rgid;
+	cmd.request.vlan_valid = vid_valid;
+	cmd.request.vlan_id = vid;
+
+	return ism_cmd(ism, &cmd);
+}
+
 static int smcd_query_rgid(struct smcd_dev *smcd, u64 rgid, u32 vid_valid,
 			   u32 vid)
 {
@@ -811,6 +781,23 @@ static int smcd_reset_vlan_required(struct smcd_dev *smcd)
 	return ism_cmd_simple(smcd->priv, ISM_RESET_VLAN);
 }
 
+static int ism_signal_ieq(struct ism_dev *ism, u64 rgid, u32 trigger_irq,
+			  u32 event_code, u64 info)
+{
+	union ism_sig_ieq cmd;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.request.hdr.cmd = ISM_SIGNAL_IEQ;
+	cmd.request.hdr.len = sizeof(cmd.request);
+
+	cmd.request.rgid = rgid;
+	cmd.request.trigger_irq = trigger_irq;
+	cmd.request.event_code = event_code;
+	cmd.request.info = info;
+
+	return ism_cmd(ism, &cmd);
+}
+
 static int smcd_signal_ieq(struct smcd_dev *smcd, u64 rgid, u32 trigger_irq,
 			   u32 event_code, u64 info)
 {
@@ -830,11 +817,24 @@ static int smcd_supports_v2(void)
 		SYSTEM_EID.type[0] != '0';
 }
 
+static u64 ism_get_local_gid(struct ism_dev *ism)
+{
+	return ism->local_gid;
+}
+
 static u64 smcd_get_local_gid(struct smcd_dev *smcd)
 {
 	return ism_get_local_gid(smcd->priv);
 }
 
+static u16 ism_get_chid(struct ism_dev *ism)
+{
+	if (!ism || !ism->pdev)
+		return 0;
+
+	return to_zpci(ism->pdev)->pchid;
+}
+
 static u16 smcd_get_chid(struct smcd_dev *smcd)
 {
 	return ism_get_chid(smcd->priv);
-- 
2.39.2

Re: [PATCH net] s390/ism: ism driver implies smc protocol

[Simon Horman]

On Tue, Nov 14, 2023 at 10:17:18AM +0100, Gerd Bayer wrote:
> Since commit a72178cfe855 ("net/smc: Fix dependency of SMC on ISM")
> you can build the ism code without selecting the SMC network protocol.
> That leaves some ism functions be reported as unused. Move these
> functions under the conditional compile with CONFIG_SMC.
> 
> Also codify the suggestion to also configure the SMC protocol in ism's
> Kconfig - but with an "imply" rather than a "select" as SMC depends on
> other config options and allow for a deliberate decision not to build
> SMC. Also, mention that in ISM's help.
> 
> Fixes: a72178cfe855 ("net/smc: Fix dependency of SMC on ISM")
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Closes: https://lore.kernel.org/netdev/afd142a2-1fa0-46b9-8b2d-7652d41d3ab8@infradead.org/
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>

Hi Gerd,

In a similar vein, I am wondering if the forward declaration of ism_ops
could be removed.  In my very light compile test it shows up as unused when
CONFIG_SMC is unset.

Re: [PATCH net] s390/ism: ism driver implies smc protocol

[Gerd Bayer]

On Wed, 2023-11-15 at 10:23 +0000, Simon Horman wrote:
> Hi Gerd,
> 
> In a similar vein, I am wondering if the forward declaration of
> ism_ops could be removed.  In my very light compile test it shows up
> as unused when CONFIG_SMC is unset.

Hi Simon,

sure, will do. Guess I need to make it a habit to compile with W=1.
ism (and smc) need some more work, though anyway.
I'll send a v2.

Thanks for spotting this!

Gerd

[PATCH net v2] s390/ism: ism driver implies smc protocol

[Gerd Bayer]

Since commit a72178cfe855 ("net/smc: Fix dependency of SMC on ISM")
you can build the ism code without selecting the SMC network protocol.
That leaves some ism functions be reported as unused. Move these
functions under the conditional compile with CONFIG_SMC.

Also codify the suggestion to also configure the SMC protocol in ism's
Kconfig - but with an "imply" rather than a "select" as SMC depends on
other config options and allow for a deliberate decision not to build
SMC. Also, mention that in ISM's help.

Fixes: a72178cfe855 ("net/smc: Fix dependency of SMC on ISM")
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Closes: https://lore.kernel.org/netdev/afd142a2-1fa0-46b9-8b2d-7652d41d3ab8@infradead.org/
Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
---
 drivers/s390/net/Kconfig   |  3 +-
 drivers/s390/net/ism_drv.c | 93 +++++++++++++++++++-------------------
 2 files changed, 48 insertions(+), 48 deletions(-)

Hi Simon,

this is version 2, that removes the unused forward declaration that you
found in v1 per:
https://lore.kernel.org/netdev/20231115102304.GN74656@kernel.org/#t
Other than that the patch is unchanged.

Thanks,
Gerd


diff --git a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
index 4902d45e929c..c61e6427384c 100644
--- a/drivers/s390/net/Kconfig
+++ b/drivers/s390/net/Kconfig
@@ -103,10 +103,11 @@ config CCWGROUP
 config ISM
 	tristate "Support for ISM vPCI Adapter"
 	depends on PCI
+	imply SMC
 	default n
 	help
 	  Select this option if you want to use the Internal Shared Memory
-	  vPCI Adapter.
+	  vPCI Adapter. The adapter can be used with the SMC network protocol.
 
 	  To compile as a module choose M. The module name is ism.
 	  If unsure, choose N.
diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
index 6df7f377d2f9..81aabbfbbe2c 100644
--- a/drivers/s390/net/ism_drv.c
+++ b/drivers/s390/net/ism_drv.c
@@ -30,7 +30,6 @@ static const struct pci_device_id ism_device_table[] = {
 MODULE_DEVICE_TABLE(pci, ism_device_table);
 
 static debug_info_t *ism_debug_info;
-static const struct smcd_ops ism_ops;
 
 #define NO_CLIENT		0xff		/* must be >= MAX_CLIENTS */
 static struct ism_client *clients[MAX_CLIENTS];	/* use an array rather than */
@@ -289,22 +288,6 @@ static int ism_read_local_gid(struct ism_dev *ism)
 	return ret;
 }
 
-static int ism_query_rgid(struct ism_dev *ism, u64 rgid, u32 vid_valid,
-			  u32 vid)
-{
-	union ism_query_rgid cmd;
-
-	memset(&cmd, 0, sizeof(cmd));
-	cmd.request.hdr.cmd = ISM_QUERY_RGID;
-	cmd.request.hdr.len = sizeof(cmd.request);
-
-	cmd.request.rgid = rgid;
-	cmd.request.vlan_valid = vid_valid;
-	cmd.request.vlan_id = vid;
-
-	return ism_cmd(ism, &cmd);
-}
-
 static void ism_free_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
 {
 	clear_bit(dmb->sba_idx, ism->sba_bitmap);
@@ -429,23 +412,6 @@ static int ism_del_vlan_id(struct ism_dev *ism, u64 vlan_id)
 	return ism_cmd(ism, &cmd);
 }
 
-static int ism_signal_ieq(struct ism_dev *ism, u64 rgid, u32 trigger_irq,
-			  u32 event_code, u64 info)
-{
-	union ism_sig_ieq cmd;
-
-	memset(&cmd, 0, sizeof(cmd));
-	cmd.request.hdr.cmd = ISM_SIGNAL_IEQ;
-	cmd.request.hdr.len = sizeof(cmd.request);
-
-	cmd.request.rgid = rgid;
-	cmd.request.trigger_irq = trigger_irq;
-	cmd.request.event_code = event_code;
-	cmd.request.info = info;
-
-	return ism_cmd(ism, &cmd);
-}
-
 static unsigned int max_bytes(unsigned int start, unsigned int len,
 			      unsigned int boundary)
 {
@@ -503,14 +469,6 @@ u8 *ism_get_seid(void)
 }
 EXPORT_SYMBOL_GPL(ism_get_seid);
 
-static u16 ism_get_chid(struct ism_dev *ism)
-{
-	if (!ism || !ism->pdev)
-		return 0;
-
-	return to_zpci(ism->pdev)->pchid;
-}
-
 static void ism_handle_event(struct ism_dev *ism)
 {
 	struct ism_event *entry;
@@ -569,11 +527,6 @@ static irqreturn_t ism_handle_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static u64 ism_get_local_gid(struct ism_dev *ism)
-{
-	return ism->local_gid;
-}
-
 static int ism_dev_init(struct ism_dev *ism)
 {
 	struct pci_dev *pdev = ism->pdev;
@@ -774,6 +727,22 @@ module_exit(ism_exit);
 /*************************** SMC-D Implementation *****************************/
 
 #if IS_ENABLED(CONFIG_SMC)
+static int ism_query_rgid(struct ism_dev *ism, u64 rgid, u32 vid_valid,
+			  u32 vid)
+{
+	union ism_query_rgid cmd;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.request.hdr.cmd = ISM_QUERY_RGID;
+	cmd.request.hdr.len = sizeof(cmd.request);
+
+	cmd.request.rgid = rgid;
+	cmd.request.vlan_valid = vid_valid;
+	cmd.request.vlan_id = vid;
+
+	return ism_cmd(ism, &cmd);
+}
+
 static int smcd_query_rgid(struct smcd_dev *smcd, u64 rgid, u32 vid_valid,
 			   u32 vid)
 {
@@ -811,6 +780,23 @@ static int smcd_reset_vlan_required(struct smcd_dev *smcd)
 	return ism_cmd_simple(smcd->priv, ISM_RESET_VLAN);
 }
 
+static int ism_signal_ieq(struct ism_dev *ism, u64 rgid, u32 trigger_irq,
+			  u32 event_code, u64 info)
+{
+	union ism_sig_ieq cmd;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.request.hdr.cmd = ISM_SIGNAL_IEQ;
+	cmd.request.hdr.len = sizeof(cmd.request);
+
+	cmd.request.rgid = rgid;
+	cmd.request.trigger_irq = trigger_irq;
+	cmd.request.event_code = event_code;
+	cmd.request.info = info;
+
+	return ism_cmd(ism, &cmd);
+}
+
 static int smcd_signal_ieq(struct smcd_dev *smcd, u64 rgid, u32 trigger_irq,
 			   u32 event_code, u64 info)
 {
@@ -830,11 +816,24 @@ static int smcd_supports_v2(void)
 		SYSTEM_EID.type[0] != '0';
 }
 
+static u64 ism_get_local_gid(struct ism_dev *ism)
+{
+	return ism->local_gid;
+}
+
 static u64 smcd_get_local_gid(struct smcd_dev *smcd)
 {
 	return ism_get_local_gid(smcd->priv);
 }
 
+static u16 ism_get_chid(struct ism_dev *ism)
+{
+	if (!ism || !ism->pdev)
+		return 0;
+
+	return to_zpci(ism->pdev)->pchid;
+}
+
 static u16 smcd_get_chid(struct smcd_dev *smcd)
 {
 	return ism_get_chid(smcd->priv);
-- 
2.39.2

Re: [PATCH net v2] s390/ism: ism driver implies smc protocol

[Simon Horman]

On Wed, Nov 15, 2023 at 04:59:58PM +0100, Gerd Bayer wrote:
> Since commit a72178cfe855 ("net/smc: Fix dependency of SMC on ISM")
> you can build the ism code without selecting the SMC network protocol.
> That leaves some ism functions be reported as unused. Move these
> functions under the conditional compile with CONFIG_SMC.
> 
> Also codify the suggestion to also configure the SMC protocol in ism's
> Kconfig - but with an "imply" rather than a "select" as SMC depends on
> other config options and allow for a deliberate decision not to build
> SMC. Also, mention that in ISM's help.
> 
> Fixes: a72178cfe855 ("net/smc: Fix dependency of SMC on ISM")
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Closes: https://lore.kernel.org/netdev/afd142a2-1fa0-46b9-8b2d-7652d41d3ab8@infradead.org/
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
> ---
>  drivers/s390/net/Kconfig   |  3 +-
>  drivers/s390/net/ism_drv.c | 93 +++++++++++++++++++-------------------
>  2 files changed, 48 insertions(+), 48 deletions(-)
> 
> Hi Simon,
> 
> this is version 2, that removes the unused forward declaration that you
> found in v1 per:
> https://lore.kernel.org/netdev/20231115102304.GN74656@kernel.org/#t
> Other than that the patch is unchanged.

Thanks Gerd,

this version looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net v2] s390/ism: ism driver implies smc protocol

[Randy Dunlap]

On 11/15/23 07:59, Gerd Bayer wrote:
> Since commit a72178cfe855 ("net/smc: Fix dependency of SMC on ISM")
> you can build the ism code without selecting the SMC network protocol.
> That leaves some ism functions be reported as unused. Move these
> functions under the conditional compile with CONFIG_SMC.
> 
> Also codify the suggestion to also configure the SMC protocol in ism's
> Kconfig - but with an "imply" rather than a "select" as SMC depends on
> other config options and allow for a deliberate decision not to build
> SMC. Also, mention that in ISM's help.
> 
> Fixes: a72178cfe855 ("net/smc: Fix dependency of SMC on ISM")
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Closes: https://lore.kernel.org/netdev/afd142a2-1fa0-46b9-8b2d-7652d41d3ab8@infradead.org/
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>

Acked-by: Randy Dunlap <rdunlap@infradead.org>
Tested-by: Randy Dunlap <rdunlap@infradead.org> # build-tested

Thanks.

> ---
>  drivers/s390/net/Kconfig   |  3 +-
>  drivers/s390/net/ism_drv.c | 93 +++++++++++++++++++-------------------
>  2 files changed, 48 insertions(+), 48 deletions(-)
> 
> Hi Simon,
> 
> this is version 2, that removes the unused forward declaration that you
> found in v1 per:
> https://lore.kernel.org/netdev/20231115102304.GN74656@kernel.org/#t
> Other than that the patch is unchanged.
> 
> Thanks,
> Gerd
> 
> 
> diff --git a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
> index 4902d45e929c..c61e6427384c 100644
> --- a/drivers/s390/net/Kconfig
> +++ b/drivers/s390/net/Kconfig
> @@ -103,10 +103,11 @@ config CCWGROUP
>  config ISM
>  	tristate "Support for ISM vPCI Adapter"
>  	depends on PCI
> +	imply SMC
>  	default n
>  	help
>  	  Select this option if you want to use the Internal Shared Memory
> -	  vPCI Adapter.
> +	  vPCI Adapter. The adapter can be used with the SMC network protocol.
>  
>  	  To compile as a module choose M. The module name is ism.
>  	  If unsure, choose N.
> diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
> index 6df7f377d2f9..81aabbfbbe2c 100644
> --- a/drivers/s390/net/ism_drv.c
> +++ b/drivers/s390/net/ism_drv.c
> @@ -30,7 +30,6 @@ static const struct pci_device_id ism_device_table[] = {
>  MODULE_DEVICE_TABLE(pci, ism_device_table);
>  
>  static debug_info_t *ism_debug_info;
> -static const struct smcd_ops ism_ops;
>  
>  #define NO_CLIENT		0xff		/* must be >= MAX_CLIENTS */
>  static struct ism_client *clients[MAX_CLIENTS];	/* use an array rather than */
> @@ -289,22 +288,6 @@ static int ism_read_local_gid(struct ism_dev *ism)
>  	return ret;
>  }
>  
> -static int ism_query_rgid(struct ism_dev *ism, u64 rgid, u32 vid_valid,
> -			  u32 vid)
> -{
> -	union ism_query_rgid cmd;
> -
> -	memset(&cmd, 0, sizeof(cmd));
> -	cmd.request.hdr.cmd = ISM_QUERY_RGID;
> -	cmd.request.hdr.len = sizeof(cmd.request);
> -
> -	cmd.request.rgid = rgid;
> -	cmd.request.vlan_valid = vid_valid;
> -	cmd.request.vlan_id = vid;
> -
> -	return ism_cmd(ism, &cmd);
> -}
> -
>  static void ism_free_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
>  {
>  	clear_bit(dmb->sba_idx, ism->sba_bitmap);
> @@ -429,23 +412,6 @@ static int ism_del_vlan_id(struct ism_dev *ism, u64 vlan_id)
>  	return ism_cmd(ism, &cmd);
>  }
>  
> -static int ism_signal_ieq(struct ism_dev *ism, u64 rgid, u32 trigger_irq,
> -			  u32 event_code, u64 info)
> -{
> -	union ism_sig_ieq cmd;
> -
> -	memset(&cmd, 0, sizeof(cmd));
> -	cmd.request.hdr.cmd = ISM_SIGNAL_IEQ;
> -	cmd.request.hdr.len = sizeof(cmd.request);
> -
> -	cmd.request.rgid = rgid;
> -	cmd.request.trigger_irq = trigger_irq;
> -	cmd.request.event_code = event_code;
> -	cmd.request.info = info;
> -
> -	return ism_cmd(ism, &cmd);
> -}
> -
>  static unsigned int max_bytes(unsigned int start, unsigned int len,
>  			      unsigned int boundary)
>  {
> @@ -503,14 +469,6 @@ u8 *ism_get_seid(void)
>  }
>  EXPORT_SYMBOL_GPL(ism_get_seid);
>  
> -static u16 ism_get_chid(struct ism_dev *ism)
> -{
> -	if (!ism || !ism->pdev)
> -		return 0;
> -
> -	return to_zpci(ism->pdev)->pchid;
> -}
> -
>  static void ism_handle_event(struct ism_dev *ism)
>  {
>  	struct ism_event *entry;
> @@ -569,11 +527,6 @@ static irqreturn_t ism_handle_irq(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static u64 ism_get_local_gid(struct ism_dev *ism)
> -{
> -	return ism->local_gid;
> -}
> -
>  static int ism_dev_init(struct ism_dev *ism)
>  {
>  	struct pci_dev *pdev = ism->pdev;
> @@ -774,6 +727,22 @@ module_exit(ism_exit);
>  /*************************** SMC-D Implementation *****************************/
>  
>  #if IS_ENABLED(CONFIG_SMC)
> +static int ism_query_rgid(struct ism_dev *ism, u64 rgid, u32 vid_valid,
> +			  u32 vid)
> +{
> +	union ism_query_rgid cmd;
> +
> +	memset(&cmd, 0, sizeof(cmd));
> +	cmd.request.hdr.cmd = ISM_QUERY_RGID;
> +	cmd.request.hdr.len = sizeof(cmd.request);
> +
> +	cmd.request.rgid = rgid;
> +	cmd.request.vlan_valid = vid_valid;
> +	cmd.request.vlan_id = vid;
> +
> +	return ism_cmd(ism, &cmd);
> +}
> +
>  static int smcd_query_rgid(struct smcd_dev *smcd, u64 rgid, u32 vid_valid,
>  			   u32 vid)
>  {
> @@ -811,6 +780,23 @@ static int smcd_reset_vlan_required(struct smcd_dev *smcd)
>  	return ism_cmd_simple(smcd->priv, ISM_RESET_VLAN);
>  }
>  
> +static int ism_signal_ieq(struct ism_dev *ism, u64 rgid, u32 trigger_irq,
> +			  u32 event_code, u64 info)
> +{
> +	union ism_sig_ieq cmd;
> +
> +	memset(&cmd, 0, sizeof(cmd));
> +	cmd.request.hdr.cmd = ISM_SIGNAL_IEQ;
> +	cmd.request.hdr.len = sizeof(cmd.request);
> +
> +	cmd.request.rgid = rgid;
> +	cmd.request.trigger_irq = trigger_irq;
> +	cmd.request.event_code = event_code;
> +	cmd.request.info = info;
> +
> +	return ism_cmd(ism, &cmd);
> +}
> +
>  static int smcd_signal_ieq(struct smcd_dev *smcd, u64 rgid, u32 trigger_irq,
>  			   u32 event_code, u64 info)
>  {
> @@ -830,11 +816,24 @@ static int smcd_supports_v2(void)
>  		SYSTEM_EID.type[0] != '0';
>  }
>  
> +static u64 ism_get_local_gid(struct ism_dev *ism)
> +{
> +	return ism->local_gid;
> +}
> +
>  static u64 smcd_get_local_gid(struct smcd_dev *smcd)
>  {
>  	return ism_get_local_gid(smcd->priv);
>  }
>  
> +static u16 ism_get_chid(struct ism_dev *ism)
> +{
> +	if (!ism || !ism->pdev)
> +		return 0;
> +
> +	return to_zpci(ism->pdev)->pchid;
> +}
> +
>  static u16 smcd_get_chid(struct smcd_dev *smcd)
>  {
>  	return ism_get_chid(smcd->priv);

-- 
~Randy

Re: [PATCH net] s390/ism: ism driver implies smc protocol

[Wen Gu]

On 2023/11/14 17:17, Gerd Bayer wrote:
> Since commit a72178cfe855 ("net/smc: Fix dependency of SMC on ISM")
> you can build the ism code without selecting the SMC network protocol.
> That leaves some ism functions be reported as unused. Move these
> functions under the conditional compile with CONFIG_SMC.
> 
> Also codify the suggestion to also configure the SMC protocol in ism's
> Kconfig - but with an "imply" rather than a "select" as SMC depends on
> other config options and allow for a deliberate decision not to build
> SMC. Also, mention that in ISM's help.
> 
> Fixes: a72178cfe855 ("net/smc: Fix dependency of SMC on ISM")
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Closes: https://lore.kernel.org/netdev/afd142a2-1fa0-46b9-8b2d-7652d41d3ab8@infradead.org/
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
> ---
> 
> Hi Randy,
> 
> sorry for the long wait. We had some internal discussions about how to
> tackle this and decided to send out the short-term solution first and
> work on better isolating the ISM device and SMC protocol together
> with the work to extend ISM,
> e.g. at https://lore.kernel.org/netdev/1695568613-125057-1-git-send-email-guwen@linux.alibaba.com/
> 

Hi, Gerd

I like the idea of better isolating ISM and SMC, both from a compilation perspective
and a code perspective. If I come up with any ideas during extending ISM, I will give
a feedback.

Thanks and Regards,
Wen Gu

> Cheers, Gerd
> 

Re: [PATCH net v2] s390/ism: ism driver implies smc protocol

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 15 Nov 2023 16:59:58 +0100 you wrote:
> Since commit a72178cfe855 ("net/smc: Fix dependency of SMC on ISM")
> you can build the ism code without selecting the SMC network protocol.
> That leaves some ism functions be reported as unused. Move these
> functions under the conditional compile with CONFIG_SMC.
> 
> Also codify the suggestion to also configure the SMC protocol in ism's
> Kconfig - but with an "imply" rather than a "select" as SMC depends on
> other config options and allow for a deliberate decision not to build
> SMC. Also, mention that in ISM's help.
> 
> [...]

Here is the summary with links:
  - [net,v2] s390/ism: ism driver implies smc protocol
    https://git.kernel.org/netdev/net/c/d565fa4300d9

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html