| Message ID | 20200305052319.14682-8-snelson@pensando.io |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | ionic updates | expand |
On Wed, 4 Mar 2020 21:23:18 -0800 Shannon Nelson wrote: > Add support for an additional device id. > > Signed-off-by: Shannon Nelson <snelson@pensando.io> I have thought about this for a while and I wanted to ask you to say a bit more about the use of the management device. Obviously this is not just "additional device id" in the traditional sense where device IDs differentiate HW SKUs or revisions. This is the same exact hardware, just a different local feature (as proven by the fact that you make 0 functional changes). In the past we (I?) rejected such extensions upstream from Netronome and Cavium, because there were no clear use cases which can't be solved by extending standard kernel APIs. Do you have any? > diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h > index bb106a32f416..c8ff33da243a 100644 > --- a/drivers/net/ethernet/pensando/ionic/ionic.h > +++ b/drivers/net/ethernet/pensando/ionic/ionic.h > @@ -18,6 +18,7 @@ struct ionic_lif; > > #define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_PF 0x1002 > #define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_VF 0x1003 > +#define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_MGMT 0x1004 > > #define DEVCMD_TIMEOUT 10 > > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c > index 59b0091146e6..3dc985cae391 100644 > --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c > +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c > @@ -15,6 +15,7 @@ > static const struct pci_device_id ionic_id_table[] = { > { PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_PF) }, > { PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_VF) }, > + { PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_MGMT) }, > { 0, } /* end of table */ > }; > MODULE_DEVICE_TABLE(pci, ionic_id_table);
On 3/5/20 2:03 PM, Jakub Kicinski wrote: > On Wed, 4 Mar 2020 21:23:18 -0800 Shannon Nelson wrote: >> Add support for an additional device id. >> >> Signed-off-by: Shannon Nelson <snelson@pensando.io> > I have thought about this for a while and I wanted to ask you to say > a bit more about the use of the management device. > > Obviously this is not just "additional device id" in the traditional > sense where device IDs differentiate HW SKUs or revisions. This is the > same exact hardware, just a different local feature (as proven by the > fact that you make 0 functional changes). > > In the past we (I?) rejected such extensions upstream from Netronome and > Cavium, because there were no clear use cases which can't be solved by > extending standard kernel APIs. Do you have any? Do you by chance have any references handy to such past discussions? I'd be interested in reading them to see what similarities and differences we have. The network device we present is only a portion of the DSC's functions. The device configuration and management for the various services is handled in userspace programs on the OS running inside the device. These are accessed through a secured REST API, typically through the external management ethernet port. In addition to our centralized management user interface, we have a command line tool for managing the device configuration using that same REST interface. In some configurations we make it possible to open a network connection into the device through the host PCI, just as if you were to connect through the external mgmt port. This is the PCI deviceid that corresponds to that port, and allows use of the command line tool on the host. The host network driver doesn't have access to the device management commands, it only can configure the NIC portion for what it needs for passing network packets. sln > >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h >> index bb106a32f416..c8ff33da243a 100644 >> --- a/drivers/net/ethernet/pensando/ionic/ionic.h >> +++ b/drivers/net/ethernet/pensando/ionic/ionic.h >> @@ -18,6 +18,7 @@ struct ionic_lif; >> >> #define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_PF 0x1002 >> #define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_VF 0x1003 >> +#define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_MGMT 0x1004 >> >> #define DEVCMD_TIMEOUT 10 >> >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c >> index 59b0091146e6..3dc985cae391 100644 >> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c >> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c >> @@ -15,6 +15,7 @@ >> static const struct pci_device_id ionic_id_table[] = { >> { PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_PF) }, >> { PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_VF) }, >> + { PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_MGMT) }, >> { 0, } /* end of table */ >> }; >> MODULE_DEVICE_TABLE(pci, ionic_id_table);
On Thu, 5 Mar 2020 16:41:48 -0800 Shannon Nelson wrote: > On 3/5/20 2:03 PM, Jakub Kicinski wrote: > > On Wed, 4 Mar 2020 21:23:18 -0800 Shannon Nelson wrote: > >> Add support for an additional device id. > >> > >> Signed-off-by: Shannon Nelson <snelson@pensando.io> > > I have thought about this for a while and I wanted to ask you to say > > a bit more about the use of the management device. > > > > Obviously this is not just "additional device id" in the traditional > > sense where device IDs differentiate HW SKUs or revisions. This is the > > same exact hardware, just a different local feature (as proven by the > > fact that you make 0 functional changes). > > > > In the past we (I?) rejected such extensions upstream from Netronome and > > Cavium, because there were no clear use cases which can't be solved by > > extending standard kernel APIs. Do you have any? > > Do you by chance have any references handy to such past discussions? > I'd be interested in reading them to see what similarities and > differences we have. Here you go: https://lore.kernel.org/netdev/20170718115827.7bd737f2@cakuba.netronome.com/ > The network device we present is only a portion of the DSC's functions. > The device configuration and management for the various services is > handled in userspace programs on the OS running inside the device. > These are accessed through a secured REST API, typically through the > external management ethernet port. In addition to our centralized > management user interface, we have a command line tool for managing the > device configuration using that same REST interface. We try to encourage vendors to create common interfaces, as you'd understand that command line tool is raising red flags. Admittedly most vendors have some form of command line tool which can poke directly into registers, anyway, but IMHO we should avoid any precedents of merging driver patches with explicit goal of enabling such tools. > In some configurations we make it possible to open a network connection > into the device through the host PCI, just as if you were to connect > through the external mgmt port. This is the PCI deviceid that > corresponds to that port, and allows use of the command line tool on the > host. > > The host network driver doesn't have access to the device management > commands, it only can configure the NIC portion for what it needs for > passing network packets.
On 3/5/20 5:18 PM, Jakub Kicinski wrote: > On Thu, 5 Mar 2020 16:41:48 -0800 Shannon Nelson wrote: >> On 3/5/20 2:03 PM, Jakub Kicinski wrote: >>> On Wed, 4 Mar 2020 21:23:18 -0800 Shannon Nelson wrote: >>>> Add support for an additional device id. >>>> >>>> Signed-off-by: Shannon Nelson <snelson@pensando.io> >>> I have thought about this for a while and I wanted to ask you to say >>> a bit more about the use of the management device. >>> >>> Obviously this is not just "additional device id" in the traditional >>> sense where device IDs differentiate HW SKUs or revisions. This is the >>> same exact hardware, just a different local feature (as proven by the >>> fact that you make 0 functional changes). >>> >>> In the past we (I?) rejected such extensions upstream from Netronome and >>> Cavium, because there were no clear use cases which can't be solved by >>> extending standard kernel APIs. Do you have any? >> Do you by chance have any references handy to such past discussions? >> I'd be interested in reading them to see what similarities and >> differences we have. > Here you go: > > https://lore.kernel.org/netdev/20170718115827.7bd737f2@cakuba.netronome.com/ Interesting - thanks. > >> The network device we present is only a portion of the DSC's functions. >> The device configuration and management for the various services is >> handled in userspace programs on the OS running inside the device. >> These are accessed through a secured REST API, typically through the >> external management ethernet port. In addition to our centralized >> management user interface, we have a command line tool for managing the >> device configuration using that same REST interface. > We try to encourage vendors to create common interfaces, as you'd > understand that command line tool is raising red flags. > > Admittedly most vendors have some form of command line tool which can > poke directly into registers, anyway, but IMHO we should avoid any > precedents of merging driver patches with explicit goal of enabling > such tools. Yes, and if we were just writing registers, that would make sense. When I can get to it I do intend to try expand our use of the devlink interfaces where it will work for us. However, this device id does exist on some of the DSC configurations, and I'd prefer to explicitly acknowledge its existence in the driver and perhaps keep better control over it, whether or not it gets used by our 3rd party tool, rather than leave it as some obscure port for someone to "discover". sln > >> In some configurations we make it possible to open a network connection >> into the device through the host PCI, just as if you were to connect >> through the external mgmt port. This is the PCI deviceid that >> corresponds to that port, and allows use of the command line tool on the >> host. >> >> The host network driver doesn't have access to the device management >> commands, it only can configure the NIC portion for what it needs for >> passing network packets.
On Thu, 5 Mar 2020 23:43:44 -0800 Shannon Nelson wrote: > Yes, and if we were just writing registers, that would make sense. When > I can get to it I do intend to try expand our use of the devlink > interfaces where it will work for us. Yes, please do that. > However, this device id does exist on some of the DSC configurations, > and I'd prefer to explicitly acknowledge its existence in the driver and > perhaps keep better control over it, whether or not it gets used by our > 3rd party tool, rather than leave it as some obscure port for someone to > "discover". I understand, but disagree. Your driver can certainly bind to that management device but it has to be for the internal use of the kernel. You shouldn't just expose that FW interface right out to user space as a netdev.
On 3/6/20 10:20 AM, Jakub Kicinski wrote: > On Thu, 5 Mar 2020 23:43:44 -0800 Shannon Nelson wrote: >> Yes, and if we were just writing registers, that would make sense. When >> I can get to it I do intend to try expand our use of the devlink >> interfaces where it will work for us. > Yes, please do that. > >> However, this device id does exist on some of the DSC configurations, >> and I'd prefer to explicitly acknowledge its existence in the driver and >> perhaps keep better control over it, whether or not it gets used by our >> 3rd party tool, rather than leave it as some obscure port for someone to >> "discover". > I understand, but disagree. Your driver can certainly bind to that > management device but it has to be for the internal use of the kernel. > You shouldn't just expose that FW interface right out to user space as > a netdev. So for now the driver should simply capture and configure the PCI device, but stop at that point and not setup a netdev. This would leave the device available for devlink commands. If that sounds reasonable to you, I'll add it and respin the patchset. sln
On Fri, 6 Mar 2020 12:32:51 -0800 Shannon Nelson wrote: > >> However, this device id does exist on some of the DSC configurations, > >> and I'd prefer to explicitly acknowledge its existence in the driver and > >> perhaps keep better control over it, whether or not it gets used by our > >> 3rd party tool, rather than leave it as some obscure port for someone to > >> "discover". > > I understand, but disagree. Your driver can certainly bind to that > > management device but it has to be for the internal use of the kernel. > > You shouldn't just expose that FW interface right out to user space as > > a netdev. > > So for now the driver should simply capture and configure the PCI > device, but stop at that point and not setup a netdev. This would leave > the device available for devlink commands. > > If that sounds reasonable to you, I'll add it and respin the patchset. I presume the driver currently creates a devlink instance per PCI function? (Given we have no real infrastructure in place to combine them.) It still feels a little strange to have a devlink instance that doesn't represent any entity user would care about, but a communication channel. It'd be better if other functions made use of the communication channel behind the scene. That said AFAIU driver with just a devlink instance won't allow passing arbitrary commands, so that would indeed address my biggest concern. What operations would that devlink instance expose?
On 3/6/20 1:28 PM, Jakub Kicinski wrote: > On Fri, 6 Mar 2020 12:32:51 -0800 Shannon Nelson wrote: >>>> However, this device id does exist on some of the DSC configurations, >>>> and I'd prefer to explicitly acknowledge its existence in the driver and >>>> perhaps keep better control over it, whether or not it gets used by our >>>> 3rd party tool, rather than leave it as some obscure port for someone to >>>> "discover". >>> I understand, but disagree. Your driver can certainly bind to that >>> management device but it has to be for the internal use of the kernel. >>> You shouldn't just expose that FW interface right out to user space as >>> a netdev. >> So for now the driver should simply capture and configure the PCI >> device, but stop at that point and not setup a netdev. This would leave >> the device available for devlink commands. >> >> If that sounds reasonable to you, I'll add it and respin the patchset. > I presume the driver currently creates a devlink instance per PCI > function? (Given we have no real infrastructure in place to combine > them.) It still feels a little strange to have a devlink instance that > doesn't represent any entity user would care about, but a communication > channel. It'd be better if other functions made use of the > communication channel behind the scene. That said AFAIU driver with just > a devlink instance won't allow passing arbitrary commands, so that would > indeed address my biggest concern. > > What operations would that devlink instance expose? Being as this is still a new idea for us and we aren't up to speed yet on what all devlink offers, I don't have a good answer at the moment. For now, nothing more than already exposed, which is simple device info. sln
diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h index bb106a32f416..c8ff33da243a 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic.h +++ b/drivers/net/ethernet/pensando/ionic/ionic.h @@ -18,6 +18,7 @@ struct ionic_lif; #define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_PF 0x1002 #define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_VF 0x1003 +#define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_MGMT 0x1004 #define DEVCMD_TIMEOUT 10 diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c index 59b0091146e6..3dc985cae391 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c @@ -15,6 +15,7 @@ static const struct pci_device_id ionic_id_table[] = { { PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_PF) }, { PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_VF) }, + { PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_MGMT) }, { 0, } /* end of table */ }; MODULE_DEVICE_TABLE(pci, ionic_id_table);
Add support for an additional device id. Signed-off-by: Shannon Nelson <snelson@pensando.io> --- drivers/net/ethernet/pensando/ionic/ionic.h | 1 + drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c | 1 + 2 files changed, 2 insertions(+)