[PATCH 0/2] NTB: Stash private data in transport client device

[PATCH 0/2] NTB: Stash private data in transport client device

[Aaron Sierra]

Support NTB transport client device drivers stashing a reference to
their private device representation within the transport client device
for trivial lookup.

Use the new stashing mechanism to simplify ntb_netdev device probe
and remove. This optimization will also benefit any out-of-tree
drivers that care to use it.

Aaron Sierra (2):
  NTB: transport: Support client device private data
  ntb_netdev: Simplify remove with client private data

 drivers/net/ntb_netdev.c      | 28 +++-------------------------
 drivers/ntb/ntb_transport.c   | 19 +++++++++++++++++++
 include/linux/ntb_transport.h |  2 ++
 3 files changed, 24 insertions(+), 25 deletions(-)

-- 
2.7.4

[PATCH 1/2] NTB: transport: Support client device private data

[Aaron Sierra]

Support NTB transport client device drivers stashing a reference to
their private device representation within the transport client device
for trivial lookup. This capability is provided by most other busses
(PCI, platform, VME, etc.).

This patch introduces two new exported functions:

  void ntb_transport_client_set_pdata(struct device *dev, void *pdata);
  void *ntb_transport_client_get_pdata(struct device *dev);

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/ntb/ntb_transport.c   | 19 +++++++++++++++++++
 include/linux/ntb_transport.h |  2 ++
 2 files changed, 21 insertions(+)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 9398959..812efd4 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -202,6 +202,9 @@ struct ntb_transport_client_dev {
 	struct list_head entry;
 	struct ntb_transport_ctx *nt;
 	struct device dev;
+
+	/* client private data */
+	void *private_data;
 };
 
 struct ntb_transport_ctx {
@@ -410,6 +413,22 @@ int ntb_transport_register_client_dev(char *device_name)
 }
 EXPORT_SYMBOL_GPL(ntb_transport_register_client_dev);
 
+void ntb_transport_client_set_pdata(struct device *dev, void *pdata)
+{
+	struct ntb_transport_client_dev *client_dev = dev_client_dev(dev);
+
+	client_dev->private_data = pdata;
+}
+EXPORT_SYMBOL_GPL(ntb_transport_client_set_pdata);
+
+void *ntb_transport_client_get_pdata(struct device *dev)
+{
+	struct ntb_transport_client_dev *client_dev = dev_client_dev(dev);
+
+	return client_dev->private_data;
+}
+EXPORT_SYMBOL_GPL(ntb_transport_client_get_pdata);
+
 /**
  * ntb_transport_register_client - Register NTB client driver
  * @drv: NTB client driver to be registered
diff --git a/include/linux/ntb_transport.h b/include/linux/ntb_transport.h
index 7243eb9..16c2a73 100644
--- a/include/linux/ntb_transport.h
+++ b/include/linux/ntb_transport.h
@@ -60,6 +60,8 @@ int ntb_transport_register_client(struct ntb_transport_client *drvr);
 void ntb_transport_unregister_client(struct ntb_transport_client *drvr);
 int ntb_transport_register_client_dev(char *device_name);
 void ntb_transport_unregister_client_dev(char *device_name);
+void ntb_transport_client_set_pdata(struct device *dev, void *pdata);
+void *ntb_transport_client_get_pdata(struct device *dev);
 
 struct ntb_queue_handlers {
 	void (*rx_handler)(struct ntb_transport_qp *qp, void *qp_data,
-- 
2.7.4

[PATCH 2/2] ntb_netdev: Simplify remove with client private data

[Aaron Sierra]

Replace the elaborate private structure global linked-list used in
ntb_netdev_probe() and ntb_netdev_remove() by using the new private
data stashing mechanism available to NTB transport client devices.

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/net/ntb_netdev.c | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index b12023b..1e76f7c 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -71,7 +71,6 @@ static unsigned int tx_start = 10;
 static unsigned int tx_stop = 5;
 
 struct ntb_netdev {
-	struct list_head list;
 	struct pci_dev *pdev;
 	struct net_device *ndev;
 	struct ntb_transport_qp *qp;
@@ -81,8 +80,6 @@ struct ntb_netdev {
 #define	NTB_TX_TIMEOUT_MS	1000
 #define	NTB_RXQ_SIZE		100
 
-static LIST_HEAD(dev_list);
-
 static void ntb_netdev_event_handler(void *data, int link_is_up)
 {
 	struct net_device *ndev = data;
@@ -452,7 +449,7 @@ static int ntb_netdev_probe(struct device *client_dev)
 	if (rc)
 		goto err1;
 
-	list_add(&dev->list, &dev_list);
+	ntb_transport_client_set_pdata(client_dev, ndev);
 	dev_info(&pdev->dev, "%s created\n", ndev->name);
 	return 0;
 
@@ -465,27 +462,8 @@ static int ntb_netdev_probe(struct device *client_dev)
 
 static void ntb_netdev_remove(struct device *client_dev)
 {
-	struct ntb_dev *ntb;
-	struct net_device *ndev;
-	struct pci_dev *pdev;
-	struct ntb_netdev *dev;
-	bool found = false;
-
-	ntb = dev_ntb(client_dev->parent);
-	pdev = ntb->pdev;
-
-	list_for_each_entry(dev, &dev_list, list) {
-		if (dev->pdev == pdev) {
-			found = true;
-			break;
-		}
-	}
-	if (!found)
-		return;
-
-	list_del(&dev->list);
-
-	ndev = dev->ndev;
+	struct net_device *ndev = ntb_transport_client_get_pdata(client_dev);
+	struct ntb_netdev *dev = netdev_priv(ndev);
 
 	unregister_netdev(ndev);
 	ntb_transport_free_queue(dev->qp);
-- 
2.7.4

Re: [PATCH 0/2] NTB: Stash private data in transport client device

[Allen Hubbe]

On Fri, Oct 12, 2018 at 4:20 PM Aaron Sierra <asierra@xes-inc.com> wrote:
>
> Support NTB transport client device drivers stashing a reference to
> their private device representation within the transport client device
> for trivial lookup.

Can dev_get/set_drvdata() be used instead?

> Use the new stashing mechanism to simplify ntb_netdev device probe
> and remove. This optimization will also benefit any out-of-tree
> drivers that care to use it.

Simplify ntb_netdev_remove() looks good.  Please also set(NULL) on remove.

>
> Aaron Sierra (2):
>   NTB: transport: Support client device private data
>   ntb_netdev: Simplify remove with client private data
>
>  drivers/net/ntb_netdev.c      | 28 +++-------------------------
>  drivers/ntb/ntb_transport.c   | 19 +++++++++++++++++++
>  include/linux/ntb_transport.h |  2 ++
>  3 files changed, 24 insertions(+), 25 deletions(-)
>
> --
> 2.7.4
>

Re: [PATCH 0/2] NTB: Stash private data in transport client device

[Aaron Sierra]

----- Original Message -----
> From: "Allen Hubbe" <allenbh@gmail.com>
> Sent: Monday, October 15, 2018 9:59:28 AM

> On Fri, Oct 12, 2018 at 4:20 PM Aaron Sierra <asierra@xes-inc.com> wrote:
>>
>> Support NTB transport client device drivers stashing a reference to
>> their private device representation within the transport client device
>> for trivial lookup.
> 
> Can dev_get/set_drvdata() be used instead?

Allen, sure that would work just fine. You're asking for dev_get_drvdata
and dev_set_drvdata to be used behind-the-scenes as an implementation
detail, right?
 
>> Use the new stashing mechanism to simplify ntb_netdev device probe
>> and remove. This optimization will also benefit any out-of-tree
>> drivers that care to use it.
> 
> Simplify ntb_netdev_remove() looks good.  Please also set(NULL) on remove.

I'd like to add this cleanup as a call to ntb_transport_client_set_pdata()
in ntb_transport_client_release(), so all transport client drivers get it for
free if that's alright with you.

-Aaron S.

>>
>> Aaron Sierra (2):
>>   NTB: transport: Support client device private data
>>   ntb_netdev: Simplify remove with client private data
>>
>>  drivers/net/ntb_netdev.c      | 28 +++-------------------------
>>  drivers/ntb/ntb_transport.c   | 19 +++++++++++++++++++
>>  include/linux/ntb_transport.h |  2 ++
>>  3 files changed, 24 insertions(+), 25 deletions(-)
>>
>> --
>> 2.7.4

Re: [PATCH 0/2] NTB: Stash private data in transport client device

[Aaron Sierra]

----- Original Message -----
> From: "Aaron Sierra" <asierra@xes-inc.com>
> Sent: Monday, October 15, 2018 11:27:33 AM

> ----- Original Message -----
>> From: "Allen Hubbe" <allenbh@gmail.com>
>> Sent: Monday, October 15, 2018 9:59:28 AM
> 
>> On Fri, Oct 12, 2018 at 4:20 PM Aaron Sierra <asierra@xes-inc.com> wrote:
>>>
>>> Support NTB transport client device drivers stashing a reference to
>>> their private device representation within the transport client device
>>> for trivial lookup.
>> 
>> Can dev_get/set_drvdata() be used instead?
> 
> Allen, sure that would work just fine. You're asking for dev_get_drvdata
> and dev_set_drvdata to be used behind-the-scenes as an implementation
> detail, right?
> 
>>> Use the new stashing mechanism to simplify ntb_netdev device probe
>>> and remove. This optimization will also benefit any out-of-tree
>>> drivers that care to use it.
>> 
>> Simplify ntb_netdev_remove() looks good.  Please also set(NULL) on remove.
> 
> I'd like to add this cleanup as a call to ntb_transport_client_set_pdata()
> in ntb_transport_client_release(), so all transport client drivers get it for
> free if that's alright with you.

Allen,

Actually, does it really make sense to explicitly set the private data to
NULL at all? Either location is pretty immediately before freeing the
device's memory. The initial value of the data will always be NULL based on
how the memory is allocated.

-Aaron S.

>>>
>>> Aaron Sierra (2):
>>>   NTB: transport: Support client device private data
>>>   ntb_netdev: Simplify remove with client private data
>>>
>>>  drivers/net/ntb_netdev.c      | 28 +++-------------------------
>>>  drivers/ntb/ntb_transport.c   | 19 +++++++++++++++++++
>>>  include/linux/ntb_transport.h |  2 ++
>>>  3 files changed, 24 insertions(+), 25 deletions(-)
>>>
>>> --
> >> 2.7.4

Re: [PATCH 0/2] NTB: Stash private data in transport client device

[Logan Gunthorpe]

On 2018-10-15 10:27 a.m., Aaron Sierra wrote:
>> On Fri, Oct 12, 2018 at 4:20 PM Aaron Sierra <asierra@xes-inc.com> wrote:
>>> Support NTB transport client device drivers stashing a reference to
>>> their private device representation within the transport client device
>>> for trivial lookup.
>>
>> Can dev_get/set_drvdata() be used instead?
> 
> Allen, sure that would work just fine. You're asking for dev_get_drvdata
> and dev_set_drvdata to be used behind-the-scenes as an implementation
> detail, right?

I agree with Allen that the drvdata functions already fill this need.
There's no reason to create NTB specific wrappers for them. Just use
them directly.

The cleanup for ntb_netdev_remove() is very nice and I'll be happy to
add my review when it uses drvdata.

> I'd like to add this cleanup as a call to ntb_transport_client_set_pdata()
> in ntb_transport_client_release(), so all transport client drivers get it for
> free if that's alright with you.

IMO setting it to NULL right before kfree-ing the structure is  unnecessary.

Logan

[PATCH] ntb_netdev: Simplify remove with client device drvdata

[Aaron Sierra]

Replace the elaborate private structure global linked-list used in
ntb_netdev_probe() and ntb_netdev_remove() by stashing our private
data in the NTB transport client device.

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/net/ntb_netdev.c | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index b12023b..378eb0f 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -71,7 +71,6 @@ static unsigned int tx_start = 10;
 static unsigned int tx_stop = 5;
 
 struct ntb_netdev {
-	struct list_head list;
 	struct pci_dev *pdev;
 	struct net_device *ndev;
 	struct ntb_transport_qp *qp;
@@ -81,8 +80,6 @@ struct ntb_netdev {
 #define	NTB_TX_TIMEOUT_MS	1000
 #define	NTB_RXQ_SIZE		100
 
-static LIST_HEAD(dev_list);
-
 static void ntb_netdev_event_handler(void *data, int link_is_up)
 {
 	struct net_device *ndev = data;
@@ -452,7 +449,7 @@ static int ntb_netdev_probe(struct device *client_dev)
 	if (rc)
 		goto err1;
 
-	list_add(&dev->list, &dev_list);
+	dev_set_drvdata(client_dev, ndev);
 	dev_info(&pdev->dev, "%s created\n", ndev->name);
 	return 0;
 
@@ -465,27 +462,8 @@ static int ntb_netdev_probe(struct device *client_dev)
 
 static void ntb_netdev_remove(struct device *client_dev)
 {
-	struct ntb_dev *ntb;
-	struct net_device *ndev;
-	struct pci_dev *pdev;
-	struct ntb_netdev *dev;
-	bool found = false;
-
-	ntb = dev_ntb(client_dev->parent);
-	pdev = ntb->pdev;
-
-	list_for_each_entry(dev, &dev_list, list) {
-		if (dev->pdev == pdev) {
-			found = true;
-			break;
-		}
-	}
-	if (!found)
-		return;
-
-	list_del(&dev->list);
-
-	ndev = dev->ndev;
+	struct net_device *ndev = dev_get_drvdata(client_dev);
+	struct ntb_netdev *dev = netdev_priv(ndev);
 
 	unregister_netdev(ndev);
 	ntb_transport_free_queue(dev->qp);
-- 
2.7.4

Re: [PATCH] ntb_netdev: Simplify remove with client device drvdata

[Logan Gunthorpe]

On 2018-10-15 2:32 p.m., Aaron Sierra wrote:
> Replace the elaborate private structure global linked-list used in
> ntb_netdev_probe() and ntb_netdev_remove() by stashing our private
> data in the NTB transport client device.
> 
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>

Looks good to me. Thanks.

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>