[dpdk-dev,04/13] eal: introduce driver type

  Since multiple buses and device types need to be supported.
Provide type field in driver.
---
 lib/librte_eal/common/include/rte_dev.h  | 15 ++++++++++++---
 lib/librte_eal/common/include/rte_pci.h  |  1 +
 lib/librte_eal/common/include/rte_vdev.h |  1 +
 3 files changed, 14 insertions(+), 3 deletions(-)
  

On Tuesday 20 December 2016 03:29 AM, Stephen Hemminger wrote:
> Since multiple buses and device types need to be supported.
> Provide type field in driver.
> ---
>  lib/librte_eal/common/include/rte_dev.h  | 15 ++++++++++++---
>  lib/librte_eal/common/include/rte_pci.h  |  1 +
>  lib/librte_eal/common/include/rte_vdev.h |  1 +
>  3 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index e5471a22..3f4e26e6 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -144,12 +144,21 @@ void rte_eal_device_insert(struct rte_device *dev);
>  void rte_eal_device_remove(struct rte_device *dev);
>
>  /**
> + * Type of device driver
> + */
> +enum rte_driver_type {
> +	PMD_VIRTUAL,
> +	PMD_PCI,
> +};

So the expectation is that if a new device type is introduced (anything 
other than virtual or PCI), this enum is expanded?

Broadly, we have two possible ways being discussed on ML for device 
relationship with PMD/APIs:

1. the way you have done: each device belongs to a particular type (enum 
rte_driver_type) and based on its type, operations would be performed on 
it (using conditional operators, for example). We continue to have a 
common device list containing all type of devices.

2. disassociating the device (rte_device) completely from its type and 
basing it on a Bus type. So, we have separate buses for each device type 
and hence no need for separation of logic based on device type.

I think implementation similar to (1) existed before it was removed in 
6751f6de.

I have an (obvious) inclination towards (2) because that helps plugging 
in more drivers/device types without expecting the contributor to change 
the EAL - however trivial the change is.

> +
> +/**
>   * A structure describing a device driver.
>   */
>  struct rte_driver {
>  	TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
> -	const char *name;                   /**< Driver name. */
> -	const char *alias;              /**< Driver alias. */
> +	const char *name;              /**< Driver name. */
> +	const char *alias;             /**< Driver alias. */
> +	enum rte_driver_type type;     /**< Driver type. */
>  };
>
>  /**
> @@ -243,4 +252,4 @@ __attribute__((used)) = str
>  }
>  #endif
>
> -#endif /* _RTE_VDEV_H_ */
> +#endif /* _RTE_DEV_H_ */
> diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
> index 9ce88472..d377d539 100644
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -492,6 +492,7 @@ RTE_INIT(pciinitfn_ ##nm); \
>  static void pciinitfn_ ##nm(void) \
>  {\
>  	(pci_drv).driver.name = RTE_STR(nm);\
> +	(pci_drv).driver.type = PMD_PCI; \
>  	rte_eal_pci_register(&pci_drv); \
>  } \
>  RTE_PMD_EXPORT_NAME(nm, __COUNTER__)
> diff --git a/lib/librte_eal/common/include/rte_vdev.h b/lib/librte_eal/common/include/rte_vdev.h
> index 784e837d..98fb5bb5 100644
> --- a/lib/librte_eal/common/include/rte_vdev.h
> +++ b/lib/librte_eal/common/include/rte_vdev.h
> @@ -88,6 +88,7 @@ static void vdrvinitfn_ ##vdrv(void)\
>  {\
>  	(vdrv).driver.name = RTE_STR(nm);\
>  	(vdrv).driver.alias = vdrvinit_ ## nm ## _alias;\
> +	(vdrv).driver.type = PMD_VIRTUAL;\
>  	rte_eal_vdrv_register(&vdrv);\
>  } \
>  RTE_PMD_EXPORT_NAME(nm, __COUNTER__)
>
  

On Mon, Dec 19, 2016 at 10:59 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> Since multiple buses and device types need to be supported.
> Provide type field in driver.
> ---
>  lib/librte_eal/common/include/rte_dev.h  | 15 ++++++++++++---
>  lib/librte_eal/common/include/rte_pci.h  |  1 +
>  lib/librte_eal/common/include/rte_vdev.h |  1 +
>  3 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index e5471a22..3f4e26e6 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -144,12 +144,21 @@ void rte_eal_device_insert(struct rte_device *dev);
>  void rte_eal_device_remove(struct rte_device *dev);
>
>  /**
> + * Type of device driver
> + */
> +enum rte_driver_type {
> +       PMD_VIRTUAL,
> +       PMD_PCI,
> +};
> +
> +/**
>   * A structure describing a device driver.
>   */
>  struct rte_driver {
>         TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
> -       const char *name;                   /**< Driver name. */
> -       const char *alias;              /**< Driver alias. */
> +       const char *name;              /**< Driver name. */
> +       const char *alias;             /**< Driver alias. */
> +       enum rte_driver_type type;     /**< Driver type. */
>  };

I wonder who is the user of the type. It can't be the driver itself
because it must be aware of what kind of low-level interface it
serves. So this seems to be a helper to allow users of rte_driver to
upcast to the object that embeds the rte_driver at runtime. I don't
believe that this is a good interface because it often leads to ugly
and error prone code.

>
>  /**
> @@ -243,4 +252,4 @@ __attribute__((used)) = str
>  }
>  #endif
>
> -#endif /* _RTE_VDEV_H_ */
> +#endif /* _RTE_DEV_H_ */
> diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
> index 9ce88472..d377d539 100644
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -492,6 +492,7 @@ RTE_INIT(pciinitfn_ ##nm); \
>  static void pciinitfn_ ##nm(void) \
>  {\
>         (pci_drv).driver.name = RTE_STR(nm);\
> +       (pci_drv).driver.type = PMD_PCI; \
>         rte_eal_pci_register(&pci_drv); \
>  } \
>  RTE_PMD_EXPORT_NAME(nm, __COUNTER__)
> diff --git a/lib/librte_eal/common/include/rte_vdev.h b/lib/librte_eal/common/include/rte_vdev.h
> index 784e837d..98fb5bb5 100644
> --- a/lib/librte_eal/common/include/rte_vdev.h
> +++ b/lib/librte_eal/common/include/rte_vdev.h
> @@ -88,6 +88,7 @@ static void vdrvinitfn_ ##vdrv(void)\
>  {\
>         (vdrv).driver.name = RTE_STR(nm);\
>         (vdrv).driver.alias = vdrvinit_ ## nm ## _alias;\
> +       (vdrv).driver.type = PMD_VIRTUAL;\
>         rte_eal_vdrv_register(&vdrv);\
>  } \
>  RTE_PMD_EXPORT_NAME(nm, __COUNTER__)
> --
> 2.11.0
>
  

On 12/19/2016 9:59 PM, Stephen Hemminger wrote:
> Since multiple buses and device types need to be supported.
> Provide type field in driver.
> ---
>  lib/librte_eal/common/include/rte_dev.h  | 15 ++++++++++++---
>  lib/librte_eal/common/include/rte_pci.h  |  1 +
>  lib/librte_eal/common/include/rte_vdev.h |  1 +
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index e5471a22..3f4e26e6 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -144,12 +144,21 @@ void rte_eal_device_insert(struct rte_device *dev);
>  void rte_eal_device_remove(struct rte_device *dev);
>  
>  /**
> + * Type of device driver
> + */
> +enum rte_driver_type {
> +	PMD_VIRTUAL,
> +	PMD_PCI,
> +};

There were types in v16.07 and previous.
Types has been removed in v16.11.
Now are we sure we want to add them back?
  

On Tue, 20 Dec 2016 14:00:55 +0100
Jan Blunck <jblunck@infradead.org> wrote:

> On Mon, Dec 19, 2016 at 10:59 PM, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> > Since multiple buses and device types need to be supported.
> > Provide type field in driver.
> > ---
> >  lib/librte_eal/common/include/rte_dev.h  | 15 ++++++++++++---
> >  lib/librte_eal/common/include/rte_pci.h  |  1 +
> >  lib/librte_eal/common/include/rte_vdev.h |  1 +
> >  3 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> > index e5471a22..3f4e26e6 100644
> > --- a/lib/librte_eal/common/include/rte_dev.h
> > +++ b/lib/librte_eal/common/include/rte_dev.h
> > @@ -144,12 +144,21 @@ void rte_eal_device_insert(struct rte_device *dev);
> >  void rte_eal_device_remove(struct rte_device *dev);
> >
> >  /**
> > + * Type of device driver
> > + */
> > +enum rte_driver_type {
> > +       PMD_VIRTUAL,
> > +       PMD_PCI,
> > +};
> > +
> > +/**
> >   * A structure describing a device driver.
> >   */
> >  struct rte_driver {
> >         TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
> > -       const char *name;                   /**< Driver name. */
> > -       const char *alias;              /**< Driver alias. */
> > +       const char *name;              /**< Driver name. */
> > +       const char *alias;             /**< Driver alias. */
> > +       enum rte_driver_type type;     /**< Driver type. */
> >  };  
> 
> I wonder who is the user of the type. It can't be the driver itself
> because it must be aware of what kind of low-level interface it
> serves. So this seems to be a helper to allow users of rte_driver to
> upcast to the object that embeds the rte_driver at runtime. I don't
> believe that this is a good interface because it often leads to ugly
> and error prone code.

The thing I ran into was the test code. The testpmd (and related code) is an
example of how some code might want to use dev_info_get to find out about a device
then take some action based on the driver type.  Testpmd has code that manipulates
PCI registers. Right now it is broken when run on virtual devices.
  

On Tue, 20 Dec 2016 12:46:17 +0530
Shreyansh Jain <shreyansh.jain@nxp.com> wrote:

> On Tuesday 20 December 2016 03:29 AM, Stephen Hemminger wrote:
> > Since multiple buses and device types need to be supported.
> > Provide type field in driver.
> > ---
> >  lib/librte_eal/common/include/rte_dev.h  | 15 ++++++++++++---
> >  lib/librte_eal/common/include/rte_pci.h  |  1 +
> >  lib/librte_eal/common/include/rte_vdev.h |  1 +
> >  3 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> > index e5471a22..3f4e26e6 100644
> > --- a/lib/librte_eal/common/include/rte_dev.h
> > +++ b/lib/librte_eal/common/include/rte_dev.h
> > @@ -144,12 +144,21 @@ void rte_eal_device_insert(struct rte_device *dev);
> >  void rte_eal_device_remove(struct rte_device *dev);
> >
> >  /**
> > + * Type of device driver
> > + */
> > +enum rte_driver_type {
> > +	PMD_VIRTUAL,
> > +	PMD_PCI,
> > +};  
> 
> So the expectation is that if a new device type is introduced (anything 
> other than virtual or PCI), this enum is expanded?
> 
> Broadly, we have two possible ways being discussed on ML for device 
> relationship with PMD/APIs:
> 
> 1. the way you have done: each device belongs to a particular type (enum 
> rte_driver_type) and based on its type, operations would be performed on 
> it (using conditional operators, for example). We continue to have a 
> common device list containing all type of devices.

It is more about providing feedback to applications in info_get which is
the only place they should care.

> 
> 2. disassociating the device (rte_device) completely from its type and 
> basing it on a Bus type. So, we have separate buses for each device type 
> and hence no need for separation of logic based on device type.
> 
> I think implementation similar to (1) existed before it was removed in 
> 6751f6de.
> 
> I have an (obvious) inclination towards (2) because that helps plugging 
> in more drivers/device types without expecting the contributor to change 
> the EAL - however trivial the change is.
> 

The difference is that virtual devices don't actually live on bus.
So it is really a property of the device not a bus per say.