[dpdk-dev] pci: Add the class_id support in pci probe
Commit Message
This patch is used to add the class_id (class_code,
subclass_code, programming_interface) support for
pci_device probe. With this patch, it will be
flexible for users to probe a class of devices
by class_id.
Signed-off-by: Ziye Yang <ziye.yang@intel.com>
---
lib/librte_eal/bsdapp/eal/eal_pci.c | 4 ++++
lib/librte_eal/common/eal_common_pci.c | 3 +++
lib/librte_eal/common/include/rte_pci.h | 8 ++++++--
lib/librte_eal/linuxapp/eal/eal_pci.c | 9 +++++++++
4 files changed, 22 insertions(+), 2 deletions(-)
Comments
On Wed, 11 May 2016 14:08:15 +0800
Ziye Yang <ziye.yang@intel.com> wrote:
> This patch is used to add the class_id (class_code,
> subclass_code, programming_interface) support for
> pci_device probe. With this patch, it will be
> flexible for users to probe a class of devices
> by class_id.
>
> Signed-off-by: Ziye Yang <ziye.yang@intel.com>
I like this, and it is necessary but since rte_pci_id is a visible
data structure it causes ABI breakage.
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Wednesday, May 11, 2016 4:21 PM
> To: Yang, Ziye <ziye.yang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] pci: Add the class_id support in pci probe
>
> On Wed, 11 May 2016 14:08:15 +0800
> Ziye Yang <ziye.yang@intel.com> wrote:
>
> > This patch is used to add the class_id (class_code, subclass_code,
> > programming_interface) support for pci_device probe. With this patch,
> > it will be flexible for users to probe a class of devices by class_id.
> >
> > Signed-off-by: Ziye Yang <ziye.yang@intel.com>
>
> I like this, and it is necessary but since rte_pci_id is a visible data
> structure it causes ABI breakage.
A notice was published for this change in 16.04, so we should be ok ABI-wise.
/Bruce
2016-05-11 14:08, Ziye Yang:
> This patch is used to add the class_id (class_code,
> subclass_code, programming_interface) support for
> pci_device probe. With this patch, it will be
> flexible for users to probe a class of devices
> by class_id.
>
> Signed-off-by: Ziye Yang <ziye.yang@intel.com>
> ---
> lib/librte_eal/bsdapp/eal/eal_pci.c | 4 ++++
> lib/librte_eal/common/eal_common_pci.c | 3 +++
> lib/librte_eal/common/include/rte_pci.h | 8 ++++++--
> lib/librte_eal/linuxapp/eal/eal_pci.c | 9 +++++++++
> 4 files changed, 22 insertions(+), 2 deletions(-)
Please remove the deprecation notice.
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -129,6 +129,7 @@ struct rte_pci_id {
> uint16_t device_id; /**< Device ID or PCI_ANY_ID. */
> uint16_t subsystem_vendor_id; /**< Subsystem vendor ID or PCI_ANY_ID. */
> uint16_t subsystem_device_id; /**< Subsystem device ID or PCI_ANY_ID. */
> + uint32_t class_id; /**< Class ID (class, subclass, pi) or CLASS_ANY_ID. */
> };
A space is missing.
It would be more logical to put the class_id at the beginning of the struct.
> /** Any PCI device identifier (vendor, device, ...) */
> #define PCI_ANY_ID (0xffff)
> +#define CLASS_ANY_ID (0xffffff)
These constants should be prefixed with RTE_.
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> + /* get class_id */
> + snprintf(filename, sizeof(filename), "%s/class",
> + dirname);
> + if (eal_parse_sysfs_value(filename, &tmp) < 0) {
> + free(dev);
> + return -1;
> + }
> + dev->id.class_id = (uint32_t)tmp && CLASS_ANY_ID;
Should be a bitwise &. Why masking is needed?
-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
Sent: Thursday, May 19, 2016 6:34 PM
To: Yang, Ziye <ziye.yang@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] pci: Add the class_id support in pci probe
2016-05-11 14:08, Ziye Yang:
> This patch is used to add the class_id (class_code, subclass_code,
> programming_interface) support for pci_device probe. With this patch,
> it will be flexible for users to probe a class of devices by class_id.
>
> Signed-off-by: Ziye Yang <ziye.yang@intel.com>
> ---
> lib/librte_eal/bsdapp/eal/eal_pci.c | 4 ++++
> lib/librte_eal/common/eal_common_pci.c | 3 +++
> lib/librte_eal/common/include/rte_pci.h | 8 ++++++--
> lib/librte_eal/linuxapp/eal/eal_pci.c | 9 +++++++++
> 4 files changed, 22 insertions(+), 2 deletions(-)
Please remove the deprecation notice.
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -129,6 +129,7 @@ struct rte_pci_id {
> uint16_t device_id; /**< Device ID or PCI_ANY_ID. */
> uint16_t subsystem_vendor_id; /**< Subsystem vendor ID or PCI_ANY_ID. */
> uint16_t subsystem_device_id; /**< Subsystem device ID or
> PCI_ANY_ID. */
> + uint32_t class_id; /**< Class ID (class, subclass, pi) or CLASS_ANY_ID. */
> };
A space is missing.
It would be more logical to put the class_id at the beginning of the struct.
> /** Any PCI device identifier (vendor, device, ...) */ #define
> PCI_ANY_ID (0xffff)
> +#define CLASS_ANY_ID (0xffffff)
These constants should be prefixed with RTE_.
[Ziye] suggest for doing another patch to change PCI_ANY_ID to RTE_PCI_ANY_ID since it will affect
Other files.
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> + /* get class_id */
> + snprintf(filename, sizeof(filename), "%s/class",
> + dirname);
> + if (eal_parse_sysfs_value(filename, &tmp) < 0) {
> + free(dev);
> + return -1;
> + }
> + dev->id.class_id = (uint32_t)tmp && CLASS_ANY_ID;
Should be a bitwise &. Why masking is needed?
[Ziye] Only 24bit info is needed.
2016-05-19 12:18, Yang, Ziye:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> 2016-05-11 14:08, Ziye Yang:
> > + dev->id.class_id = (uint32_t)tmp && CLASS_ANY_ID;
>
> Should be a bitwise &. Why masking is needed?
> [Ziye] Only 24bit info is needed.
What are the other bits?
Please put a comment in the code.
-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
Sent: Thursday, May 19, 2016 8:57 PM
To: Yang, Ziye <ziye.yang@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] pci: Add the class_id support in pci probe
2016-05-19 12:18, Yang, Ziye:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> 2016-05-11 14:08, Ziye Yang:
> > + dev->id.class_id = (uint32_t)tmp && CLASS_ANY_ID;
>
> Should be a bitwise &. Why masking is needed?
> [Ziye] Only 24bit info is needed.
What are the other bits?
Please put a comment in the code.
[Ziye] Revision ID is defined in pci spec, classid has 24 bits. And when we read from the system, we will only get class_id, subclass and program interface. I will put the comment
@@ -278,6 +278,10 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
/* get subsystem_device id */
dev->id.subsystem_device_id = conf->pc_subdevice;
+ dev->id.class_id = (conf->pc_class << 16) |
+ (conf->pc_subclass << 8) |
+ (conf->pc_progif);
+
/* TODO: get max_vfs */
dev->max_vfs = 0;
@@ -162,6 +162,9 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
if (id_table->subsystem_device_id != dev->id.subsystem_device_id &&
id_table->subsystem_device_id != PCI_ANY_ID)
continue;
+ if (id_table->class_id != dev->id.class_id &&
+ id_table->class_id != CLASS_ANY_ID)
+ continue;
struct rte_pci_addr *loc = &dev->addr;
@@ -129,6 +129,7 @@ struct rte_pci_id {
uint16_t device_id; /**< Device ID or PCI_ANY_ID. */
uint16_t subsystem_vendor_id; /**< Subsystem vendor ID or PCI_ANY_ID. */
uint16_t subsystem_device_id; /**< Subsystem device ID or PCI_ANY_ID. */
+ uint32_t class_id; /**< Class ID (class, subclass, pi) or CLASS_ANY_ID. */
};
/**
@@ -170,6 +171,7 @@ struct rte_pci_device {
/** Any PCI device identifier (vendor, device, ...) */
#define PCI_ANY_ID (0xffff)
+#define CLASS_ANY_ID (0xffffff)
#ifdef __cplusplus
/** C++ macro used to help building up tables of device IDs */
@@ -177,14 +179,16 @@ struct rte_pci_device {
(vend), \
(dev), \
PCI_ANY_ID, \
- PCI_ANY_ID
+ PCI_ANY_ID, \
+ CLASS_ANY_ID
#else
/** Macro used to help building up tables of device IDs */
#define RTE_PCI_DEVICE(vend, dev) \
.vendor_id = (vend), \
.device_id = (dev), \
.subsystem_vendor_id = PCI_ANY_ID, \
- .subsystem_device_id = PCI_ANY_ID
+ .subsystem_device_id = PCI_ANY_ID, \
+ .class_id = CLASS_ANY_ID
#endif
struct rte_pci_driver;
@@ -306,6 +306,15 @@ pci_scan_one(const char *dirname, uint16_t domain, uint8_t bus,
}
dev->id.subsystem_device_id = (uint16_t)tmp;
+ /* get class_id */
+ snprintf(filename, sizeof(filename), "%s/class",
+ dirname);
+ if (eal_parse_sysfs_value(filename, &tmp) < 0) {
+ free(dev);
+ return -1;
+ }
+ dev->id.class_id = (uint32_t)tmp && CLASS_ANY_ID;
+
/* get max_vfs */
dev->max_vfs = 0;
snprintf(filename, sizeof(filename), "%s/max_vfs", dirname);