Re: [RFC net-next 11/13] nfp: Handle SWITCHDEV_PORT_ATTR_GET event

2019-02-01 Thread Florian Fainelli
Le 2/1/19 à 7:45 PM, Jakub Kicinski a écrit :
> On Fri,  1 Feb 2019 14:06:55 -0800, Florian Fainelli wrote:
>> Following patches will change the way we communicate getting or setting
>> a port's attribute and use a blocking notifier to perform those tasks.
>>
>> Prepare nfp to support receiving notifier events targeting
>> SWITCHDEV_PORT_ATTR_GET and simply translate that into the existing
>> switchdev_ops::switchdev_port_attr_get operation.
>>
>> We register a single blocking switchdev notifier for the entire driver
>> instance and we differentiate a "net" from a "repr" by comparing the
>> network device's netdev_ops with the ones that this driver manages.
>>
>> Signed-off-by: Florian Fainelli 
> 
> Thanks Florian, the code looks good, only nit I have is - could you
> move nfp_switchdev_blocking_event() to nfp_port.c and nfp_port.h?
> We shouldn't touch nfp_net_common.c here.

Sounds good, thanks for the suggestion.

> 
> In general calling a notifier to get the parent_id (which is the only
> thing all these SR-IOV NIC drivers implement) seems a tad heavy.  It's
> an immutable, read-only attribute of a port, perhaps we can break it
> out?  Could we make it an NDO, perhaps?

A NDO would be fine with me, Ido, what do you think?

> 
> That's just my knee jerk reaction, given that NIC drivers don't
> implement any of the bridging side of switchdev I may not have a full
> appreciation of the abstraction you are building here :)
> 

Ido convinced me to convert the switchdev_port_attr_set() into a
blocking notifier such that we could veto operations in switch drivers,
once you do that, leaving the getter as switchdev_ops became pointless
:) but yes, it's a lot of code just to get there.
-- 
Florian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [RFC net-next 11/13] nfp: Handle SWITCHDEV_PORT_ATTR_GET event

2019-02-01 Thread Jakub Kicinski
On Fri,  1 Feb 2019 14:06:55 -0800, Florian Fainelli wrote:
> Following patches will change the way we communicate getting or setting
> a port's attribute and use a blocking notifier to perform those tasks.
> 
> Prepare nfp to support receiving notifier events targeting
> SWITCHDEV_PORT_ATTR_GET and simply translate that into the existing
> switchdev_ops::switchdev_port_attr_get operation.
> 
> We register a single blocking switchdev notifier for the entire driver
> instance and we differentiate a "net" from a "repr" by comparing the
> network device's netdev_ops with the ones that this driver manages.
> 
> Signed-off-by: Florian Fainelli 

Thanks Florian, the code looks good, only nit I have is - could you
move nfp_switchdev_blocking_event() to nfp_port.c and nfp_port.h?
We shouldn't touch nfp_net_common.c here.

In general calling a notifier to get the parent_id (which is the only
thing all these SR-IOV NIC drivers implement) seems a tad heavy.  It's
an immutable, read-only attribute of a port, perhaps we can break it
out?  Could we make it an NDO, perhaps?

That's just my knee jerk reaction, given that NIC drivers don't
implement any of the bridging side of switchdev I may not have a full
appreciation of the abstraction you are building here :)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[RFC net-next 11/13] nfp: Handle SWITCHDEV_PORT_ATTR_GET event

2019-02-01 Thread Florian Fainelli
Following patches will change the way we communicate getting or setting
a port's attribute and use a blocking notifier to perform those tasks.

Prepare nfp to support receiving notifier events targeting
SWITCHDEV_PORT_ATTR_GET and simply translate that into the existing
switchdev_ops::switchdev_port_attr_get operation.

We register a single blocking switchdev notifier for the entire driver
instance and we differentiate a "net" from a "repr" by comparing the
network device's netdev_ops with the ones that this driver manages.

Signed-off-by: Florian Fainelli 
---
 drivers/net/ethernet/netronome/nfp/nfp_main.c   | 14 +-
 drivers/net/ethernet/netronome/nfp/nfp_net.h|  3 +++
 .../net/ethernet/netronome/nfp/nfp_net_common.c | 17 +
 drivers/net/ethernet/netronome/nfp/nfp_port.c   | 15 +++
 drivers/net/ethernet/netronome/nfp/nfp_port.h   |  5 -
 5 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.c 
b/drivers/net/ethernet/netronome/nfp/nfp_main.c
index 6c10e8d119e4..50c111280cd4 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.c
@@ -17,6 +17,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "nfpcore/nfp.h"
 #include "nfpcore/nfp_cpp.h"
@@ -708,6 +709,10 @@ static void nfp_pci_remove(struct pci_dev *pdev)
pci_disable_device(pdev);
 }
 
+static struct notifier_block nfp_swdev_blocking_nb = {
+   .notifier_call  = nfp_switchdev_blocking_event,
+};
+
 static struct pci_driver nfp_pci_driver = {
.name   = nfp_driver_name,
.id_table   = nfp_pci_device_ids,
@@ -725,9 +730,13 @@ static int __init nfp_main_init(void)
 
nfp_net_debugfs_create();
 
+   err = register_switchdev_blocking_notifier(_swdev_blocking_nb);
+   if (err)
+   goto err_destroy_debugfs;
+
err = pci_register_driver(_pci_driver);
if (err < 0)
-   goto err_destroy_debugfs;
+   goto err_unreg_notifier;
 
err = pci_register_driver(_netvf_pci_driver);
if (err)
@@ -737,6 +746,8 @@ static int __init nfp_main_init(void)
 
 err_unreg_pf:
pci_unregister_driver(_pci_driver);
+err_unreg_notifier:
+   unregister_switchdev_blocking_notifier(_swdev_blocking_nb);
 err_destroy_debugfs:
nfp_net_debugfs_destroy();
return err;
@@ -746,6 +757,7 @@ static void __exit nfp_main_exit(void)
 {
pci_unregister_driver(_netvf_pci_driver);
pci_unregister_driver(_pci_driver);
+   unregister_switchdev_blocking_notifier(_swdev_blocking_nb);
nfp_net_debugfs_destroy();
 }
 
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net.h 
b/drivers/net/ethernet/netronome/nfp/nfp_net.h
index be37c2d6151c..57f7d6d634ea 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net.h
@@ -915,4 +915,7 @@ static inline void nfp_net_debugfs_dir_clean(struct dentry 
**dir)
 }
 #endif /* CONFIG_NFP_DEBUG */
 
+int nfp_switchdev_blocking_event(struct notifier_block *nb,
+unsigned long event, void *ptr);
+
 #endif /* _NFP_NET_H_ */
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c 
b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 7d2d4241498f..c2c5e7e3aab0 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -3955,3 +3955,20 @@ void nfp_net_clean(struct nfp_net *nn)
unregister_netdev(nn->dp.netdev);
nfp_net_reconfig_wait_posted(nn);
 }
+
+int nfp_switchdev_blocking_event(struct notifier_block *nb,
+unsigned long event, void *ptr)
+{
+   struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
+
+   if (!nfp_netdev_is_nfp_repr(dev) &&
+   !nfp_netdev_is_nfp_net(dev))
+   return NOTIFY_DONE;
+
+   switch (event) {
+   case SWITCHDEV_PORT_ATTR_GET:
+   return nfp_port_switchdev_attr_event(event, dev, ptr);
+   }
+
+   return NOTIFY_DONE;
+}
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_port.c 
b/drivers/net/ethernet/netronome/nfp/nfp_port.c
index 86bc149ca231..578477242ae9 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_port.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_port.c
@@ -59,6 +59,21 @@ const struct switchdev_ops nfp_port_switchdev_ops = {
.switchdev_port_attr_get= nfp_port_attr_get,
 };
 
+int nfp_port_switchdev_attr_event(unsigned long event,
+   struct net_device *netdev,
+   struct switchdev_notifier_port_attr_info *port_attr_info)
+{
+   int rc;
+
+   if (event != SWITCHDEV_PORT_ATTR_GET)
+   return NOTIFY_DONE;
+
+   rc = nfp_port_attr_get(netdev, port_attr_info->attr);
+   port_attr_info->handled = true;
+
+   return notifier_from_errno(rc);
+}
+
 int