Message-ID: <aJiwrG-XD06gTKb3@linux.alibaba.com>
Date: Sun, 10 Aug 2025 22:46:04 +0800
From: Dust Li <dust.li@...ux.alibaba.com>
To: Alexandra Winter <wintera@...ux.ibm.com>,
	David Miller <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"D. Wythe" <alibuda@...ux.alibaba.com>,
	Sidraya Jayagond <sidraya@...ux.ibm.com>,
	Wenjia Zhang <wenjia@...ux.ibm.com>,
	Julian Ruess <julianr@...ux.ibm.com>
Cc: netdev@...r.kernel.org, linux-s390@...r.kernel.org,
	Heiko Carstens <hca@...ux.ibm.com>,
	Vasily Gorbik <gor@...ux.ibm.com>,
	Alexander Gordeev <agordeev@...ux.ibm.com>,
	Christian Borntraeger <borntraeger@...ux.ibm.com>,
	Sven Schnelle <svens@...ux.ibm.com>,
	Thorsten Winkler <twinkler@...ux.ibm.com>,
	Simon Horman <horms@...nel.org>,
	Mahanta Jambigi <mjambigi@...ux.ibm.com>,
	Tony Lu <tonylu@...ux.alibaba.com>,
	Wen Gu <guwen@...ux.alibaba.com>, Halil Pasic <pasic@...ux.ibm.com>,
	linux-rdma@...r.kernel.org
Subject: Re: [RFC net-next 08/17] net/dibs: Register ism as dibs device

On 2025-08-06 17:41:13, Alexandra Winter wrote:
>Register ism devices with the dibs layer. Follow-on patches will move
>functionality to the dibs layer.
>
>As DIBS is only a shim layer without any dependencies, we can depend ISM
>on DIBS without adding indirect dependencies. A follow-on patch will
>remove implication of SMC by ISM.
>
>Define struct dibs_dev. Follow-on patches will move more content into
>dibs_dev.  The goal of follow-on patches is that ism_dev will only
>contain fields that are special for this device driver. The same concept
>will apply to other dibs device drivers.
>
>Define dibs_dev_alloc(), dibs_dev_add() and dibs_dev_del() to be called
>by dibs device drivers and call them from ism_drv.c
>Use ism_dev.dibs for a pointer to dibs_dev.

I've been wondering whether we should completely remove the ISM concept
from SMC. Including rename smc_ism.c into smc_dibs.c.

Since DIBS already serves as the replacement for ISM, having both ISM
and DIBS coexist in the codebase seems a bit confusing and inconsistent.
Removing ISM could help streamline the code and improve clarity.

Best regards,
Dust

>
>Signed-off-by: Alexandra Winter <wintera@...ux.ibm.com>
>---
> drivers/s390/net/Kconfig   |  2 +-
> drivers/s390/net/ism.h     |  1 +
> drivers/s390/net/ism_drv.c | 83 ++++++++++++++++++++++++--------------
> include/linux/dibs.h       | 38 +++++++++++++++++
> include/linux/ism.h        |  1 +
> net/dibs/dibs_main.c       | 36 +++++++++++++++++
> 6 files changed, 129 insertions(+), 32 deletions(-)
>
>diff --git a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
>index 2b43f6f28362..92985f595d59 100644
>--- a/drivers/s390/net/Kconfig
>+++ b/drivers/s390/net/Kconfig
>@@ -81,7 +81,7 @@ config CCWGROUP
> 
> config ISM
> 	tristate "Support for ISM vPCI Adapter"
>-	depends on PCI
>+	depends on PCI && DIBS
> 	imply SMC
> 	default n
> 	help
>diff --git a/drivers/s390/net/ism.h b/drivers/s390/net/ism.h
>index b5b03db52fce..3078779fa71e 100644
>--- a/drivers/s390/net/ism.h
>+++ b/drivers/s390/net/ism.h
>@@ -5,6 +5,7 @@
> #include <linux/spinlock.h>
> #include <linux/types.h>
> #include <linux/pci.h>
>+#include <linux/dibs.h>
> #include <linux/ism.h>
> #include <net/smc.h>
> #include <asm/pci_insn.h>
>diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
>index a543e59818bb..7948334f8d30 100644
>--- a/drivers/s390/net/ism_drv.c
>+++ b/drivers/s390/net/ism_drv.c
>@@ -599,8 +599,39 @@ static void ism_dev_release(struct device *dev)
> 	kfree(ism);
> }
> 
>+static void ism_dev_exit(struct ism_dev *ism)
>+{
>+	struct pci_dev *pdev = ism->pdev;
>+	unsigned long flags;
>+	int i;
>+
>+	spin_lock_irqsave(&ism->lock, flags);
>+	for (i = 0; i < max_client; ++i)
>+		ism->subs[i] = NULL;
>+	spin_unlock_irqrestore(&ism->lock, flags);
>+
>+	mutex_lock(&ism_dev_list.mutex);
>+	mutex_lock(&clients_lock);
>+	for (i = 0; i < max_client; ++i) {
>+		if (clients[i])
>+			clients[i]->remove(ism);
>+	}
>+	mutex_unlock(&clients_lock);
>+
>+	if (ism_v2_capable)
>+		ism_del_vlan_id(ism, ISM_RESERVED_VLANID);
>+	unregister_ieq(ism);
>+	unregister_sba(ism);
>+	free_irq(pci_irq_vector(pdev, 0), ism);
>+	kfree(ism->sba_client_arr);
>+	pci_free_irq_vectors(pdev);
>+	list_del_init(&ism->list);
>+	mutex_unlock(&ism_dev_list.mutex);
>+}
>+
> static int ism_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
>+	struct dibs_dev *dibs;
> 	struct ism_dev *ism;
> 	int ret;
> 
>@@ -636,12 +667,28 @@ static int ism_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> 	dma_set_max_seg_size(&pdev->dev, SZ_1M);
> 	pci_set_master(pdev);
> 
>+	dibs = dibs_dev_alloc();
>+	if (!dibs) {
>+		ret = -ENOMEM;
>+		goto err_resource;
>+	}
>+	ism->dibs = dibs;
>+
> 	ret = ism_dev_init(ism);
> 	if (ret)
>-		goto err_resource;
>+		goto err_dibs;
>+
>+	ret = dibs_dev_add(dibs);
>+	if (ret)
>+		goto err_ism;
> 
> 	return 0;
> 
>+err_ism:
>+	ism_dev_exit(ism);
>+err_dibs:
>+	/* pairs with dibs_dev_alloc() */
>+	kfree(dibs);
> err_resource:
> 	pci_release_mem_regions(pdev);
> err_disable:
>@@ -655,41 +702,15 @@ static int ism_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> 	return ret;
> }
> 
>-static void ism_dev_exit(struct ism_dev *ism)
>-{
>-	struct pci_dev *pdev = ism->pdev;
>-	unsigned long flags;
>-	int i;
>-
>-	spin_lock_irqsave(&ism->lock, flags);
>-	for (i = 0; i < max_client; ++i)
>-		ism->subs[i] = NULL;
>-	spin_unlock_irqrestore(&ism->lock, flags);
>-
>-	mutex_lock(&ism_dev_list.mutex);
>-	mutex_lock(&clients_lock);
>-	for (i = 0; i < max_client; ++i) {
>-		if (clients[i])
>-			clients[i]->remove(ism);
>-	}
>-	mutex_unlock(&clients_lock);
>-
>-	if (ism_v2_capable)
>-		ism_del_vlan_id(ism, ISM_RESERVED_VLANID);
>-	unregister_ieq(ism);
>-	unregister_sba(ism);
>-	free_irq(pci_irq_vector(pdev, 0), ism);
>-	kfree(ism->sba_client_arr);
>-	pci_free_irq_vectors(pdev);
>-	list_del_init(&ism->list);
>-	mutex_unlock(&ism_dev_list.mutex);
>-}
>-
> static void ism_remove(struct pci_dev *pdev)
> {
> 	struct ism_dev *ism = dev_get_drvdata(&pdev->dev);
>+	struct dibs_dev *dibs = ism->dibs;
> 
>+	dibs_dev_del(dibs);
> 	ism_dev_exit(ism);
>+	/* pairs with dibs_dev_alloc() */
>+	kfree(dibs);
> 
> 	pci_release_mem_regions(pdev);
> 	pci_disable_device(pdev);
>diff --git a/include/linux/dibs.h b/include/linux/dibs.h
>index 5c432699becb..e9a66cc7f25d 100644
>--- a/include/linux/dibs.h
>+++ b/include/linux/dibs.h
>@@ -9,6 +9,7 @@
> #ifndef _DIBS_H
> #define _DIBS_H
> 
>+#include <linux/device.h>
> /* DIBS - Direct Internal Buffer Sharing - concept
>  * -----------------------------------------------
>  * In the case of multiple system sharing the same hardware, dibs fabrics can
>@@ -61,4 +62,41 @@ int dibs_register_client(struct dibs_client *client);
>  */
> int dibs_unregister_client(struct dibs_client *client);
> 
>+/* DIBS devices
>+ * ------------
>+ */
>+struct dibs_dev {
>+	struct list_head list;
>+};
>+
>+/* ------- End of client-only functions ----------- */
>+
>+/*
>+ * Functions to be called by dibs device drivers:
>+ */
>+/**
>+ * dibs_dev_alloc() - allocate and reference device structure
>+ *
>+ * The following fields will be valid upon successful return: dev
>+ * NOTE: Use put_device(dibs_get_dev(@dibs)) to give up your reference instead
>+ * of freeing @dibs @dev directly once you have successfully called this
>+ * function.
>+ * Return: Pointer to dibs device structure
>+ */
>+struct dibs_dev *dibs_dev_alloc(void);
>+/**
>+ * dibs_dev_add() - register with dibs layer and all clients
>+ * @dibs: dibs device
>+ *
>+ * The following fields must be valid upon entry: dev, ops, drv_priv
>+ * All fields will be valid upon successful return.
>+ * Return: zero on success
>+ */
>+int dibs_dev_add(struct dibs_dev *dibs);
>+/**
>+ * dibs_dev_del() - unregister from dibs layer and all clients
>+ * @dibs: dibs device
>+ */
>+void dibs_dev_del(struct dibs_dev *dibs);
>+
> #endif	/* _DIBS_H */
>diff --git a/include/linux/ism.h b/include/linux/ism.h
>index 8358b4cd7ba6..9a53d3c48c16 100644
>--- a/include/linux/ism.h
>+++ b/include/linux/ism.h
>@@ -30,6 +30,7 @@ struct ism_dev {
> 	spinlock_t lock; /* protects the ism device */
> 	spinlock_t cmd_lock; /* serializes cmds */
> 	struct list_head list;
>+	struct dibs_dev *dibs;
> 	struct pci_dev *pdev;
> 
> 	struct ism_sba *sba;
>diff --git a/net/dibs/dibs_main.c b/net/dibs/dibs_main.c
>index d91e461f2f06..5345e41ae5e4 100644
>--- a/net/dibs/dibs_main.c
>+++ b/net/dibs/dibs_main.c
>@@ -21,6 +21,15 @@ MODULE_LICENSE("GPL");
> static struct dibs_client *clients[MAX_DIBS_CLIENTS];
> static u8 max_client;
> static DEFINE_MUTEX(clients_lock);
>+struct dibs_dev_list {
>+	struct list_head list;
>+	struct mutex mutex; /* protects dibs device list */
>+};
>+
>+static struct dibs_dev_list dibs_dev_list = {
>+	.list = LIST_HEAD_INIT(dibs_dev_list.list),
>+	.mutex = __MUTEX_INITIALIZER(dibs_dev_list.mutex),
>+};
> 
> int dibs_register_client(struct dibs_client *client)
> {
>@@ -56,6 +65,33 @@ int dibs_unregister_client(struct dibs_client *client)
> }
> EXPORT_SYMBOL_GPL(dibs_unregister_client);
> 
>+struct dibs_dev *dibs_dev_alloc(void)
>+{
>+	struct dibs_dev *dibs;
>+
>+	dibs = kzalloc(sizeof(*dibs), GFP_KERNEL);
>+	return dibs;
>+}
>+EXPORT_SYMBOL_GPL(dibs_dev_alloc);
>+
>+int dibs_dev_add(struct dibs_dev *dibs)
>+{
>+	mutex_lock(&dibs_dev_list.mutex);
>+	list_add(&dibs->list, &dibs_dev_list.list);
>+	mutex_unlock(&dibs_dev_list.mutex);
>+
>+	return 0;
>+}
>+EXPORT_SYMBOL_GPL(dibs_dev_add);
>+
>+void dibs_dev_del(struct dibs_dev *dibs)
>+{
>+	mutex_lock(&dibs_dev_list.mutex);
>+	list_del_init(&dibs->list);
>+	mutex_unlock(&dibs_dev_list.mutex);
>+}
>+EXPORT_SYMBOL_GPL(dibs_dev_del);
>+
> static int __init dibs_init(void)
> {
> 	memset(clients, 0, sizeof(clients));
>-- 
>2.48.1

Powered by blists - more mailing lists