DPDK-dev Archive on lore.kernel.org
 help / color / mirror / Atom feed

* Re: [PATCH] crypto: remove unused digest-appended feature
From: Trahe, Fiona @ 2016-11-17 17:52 UTC (permalink / raw)
  To: dev@dpdk.org
  Cc: De Lara Guarch, Pablo, Griffin, John, Jastrzebski, MichalX K,
	Kusztal, ArkadiuszX, Trahe, Fiona
In-Reply-To: <1479403997-15437-1-git-send-email-fiona.trahe@intel.com>

Note, this is a cover letter - see important clarification below.
Sorry, I missed adding the [PATCH 0/1] and [PATCH 1/1] to the email subject.

> -----Original Message-----
> From: Trahe, Fiona
> Sent: Thursday, November 17, 2016 5:33 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Griffin, John <john.griffin@intel.com>; Jastrzebski,
> MichalX K <michalx.k.jastrzebski@intel.com>; Kusztal, ArkadiuszX
> <arkadiuszx.kusztal@intel.com>
> Subject: [PATCH] crypto: remove unused digest-appended feature
> 
> The cryptodev API had specified that if the digest address field was
> left empty on an authentication operation, then the PMD would assume
> the digest was appended to the source or destination data.
> This case was not handled at all by most PMDs and incorrectly handled
> by the QAT PMD.
> As no bugs were raised, it is assumed to be not needed, so this patch
> removes it, rather than add handling for the case on all PMDs.
> The digest can still be appended to the data, but its
> address must now be provided in the op.
> 
> I've added this cover letter to pre-empt the question which will arise
> in chained-mbuf case.
> (Chained-mbuf support will be added in next few weeks for 17.02)
> In a chained-mbuf case, if the digest is appended to the data, it could
> in theory end up split across the last 2 mbufs. Using the digest address
> field only works for the case where the digest is in a flat buffer. The
> digest-appended feature would have provided one way of handling this,
> pushing the responsibility down to the PMD to follow the chain to find
> the digest address and handle it whether in 1 or 2 mbufs.
> However, this is adding extra cycles on the data path in all PMDs
> to handle a case which may not arise in many applications.
> The alternative is to give the application the responsibility
> to make sure the digest is in a flat buffer.
> Specifically if an application knows cases of a digest spanning
> buffers can arise, then it can check for this case and memcpy the partial
> start of the digest from the second last buffer into the last buffer
> so it's no longer split.
> Does anyone see a problem with this?
> 
> 
> Fiona Trahe (1):
>   crypto: remove unused digest-appended feature
> 
>  drivers/crypto/qat/qat_adf/qat_algs_build_desc.c |  2 ++
>  drivers/crypto/qat/qat_crypto.c                  | 18 +-----------------
>  lib/librte_cryptodev/rte_crypto_sym.h            | 10 +---------
>  3 files changed, 4 insertions(+), 26 deletions(-)
> 
> --
> 2.5.0

^ permalink raw reply

* Re: dpdk/vpp and cross-version migration for vhost
From: Michael S. Tsirkin @ 2016-11-17 17:37 UTC (permalink / raw)
  To: Yuanhan Liu
  Cc: Maxime Coquelin, dev, Stephen Hemminger, qemu-devel, libvir-list,
	vpp-dev, Marc-André Lureau
In-Reply-To: <20161117094936.GN5048@yliu-dev.sh.intel.com>

On Thu, Nov 17, 2016 at 05:49:36PM +0800, Yuanhan Liu wrote:
> On Thu, Nov 17, 2016 at 09:47:09AM +0100, Maxime Coquelin wrote:
> > 
> > 
> > On 11/17/2016 09:29 AM, Yuanhan Liu wrote:
> > >As usaual, sorry for late response :/
> > >
> > >On Thu, Oct 13, 2016 at 08:50:52PM +0300, Michael S. Tsirkin wrote:
> > >>Hi!
> > >>So it looks like we face a problem with cross-version
> > >>migration when using vhost. It's not new but became more
> > >>acute with the advent of vhost user.
> > >>
> > >>For users to be able to migrate between different versions
> > >>of the hypervisor the interface exposed to guests
> > >>by hypervisor must stay unchanged.
> > >>
> > >>The problem is that a qemu device is connected
> > >>to a backend in another process, so the interface
> > >>exposed to guests depends on the capabilities of that
> > >>process.
> > >>
> > >>Specifically, for vhost user interface based on virtio, this includes
> > >>the "host features" bitmap that defines the interface, as well as more
> > >>host values such as the max ring size.  Adding new features/changing
> > >>values to this interface is required to make progress, but on the other
> > >>hand we need ability to get the old host features to be compatible.
> > >
> > >It looks like to the same issue of vhost-user reconnect to me. For example,
> > >
> > >- start dpdk 16.07 & qemu 2.5
> > >- kill dpdk
> > >- start dpdk 16.11
> > >
> > >Though DPDK 16.11 has more features comparing to dpdk 16.07 (say, indirect),
> > >above should work. Because qemu saves the negotiated features before the
> > >disconnect and stores it back after the reconnection.
> > >
> > >    commit a463215b087c41d7ca94e51aa347cde523831873
> > >    Author: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >    Date:   Mon Jun 6 18:45:05 2016 +0200
> > >
> > >        vhost-net: save & restore vhost-user acked features
> > >
> > >        The initial vhost-user connection sets the features to be negotiated
> > >        with the driver. Renegotiation isn't possible without device reset.
> > >
> > >        To handle reconnection of vhost-user backend, ensure the same set of
> > >        features are provided, and reuse already acked features.
> > >
> > >        Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > >
> > >So we could do similar to vhost-user? I mean, save the acked features
> > >before migration and store it back after it. This should be able to
> > >keep the compatibility. If user downgrades DPDK version, it also could
> > >be easily detected, and then exit with an error to user: migration
> > >failed due to un-compatible vhost features.
> > >
> > >Just some rough thoughts. Makes tiny sense?
> > 
> > My understanding is that the management tool has to know whether
> > versions are compatible before initiating the migration:
> 
> Makes sense. How about getting and restoring the acked features through
> qemu command lines then, say, through the monitor interface?
> 
> With that, it would be something like:
> 
> - start vhost-user backend (DPDK, VPP, or whatever) & qemu in the src host
> 
> - read the acked features (through monitor interface)
> 
> - start vhost-user backend in the dst host
> 
> - start qemu in the dst host with the just queried acked features
> 
>   QEMU then is expected to use this feature set for the later vhost-user
>   feature negotitation. Exit if features compatibility is broken.
> 
> Thoughts?
> 
> 	--yliu


You keep assuming that you have the VM started first and
figure out things afterwards, but this does not work.

Think about a cluster of machines. You want to start a VM in
a way that will ensure compatibility with all hosts
in a cluster.

If you don't, guest visible interface will change
and you won't be able to migrate.

It does not make sense to discuss feature bits specifically
since that is not the only part of interface.
For example, max ring size supported might change.


Let me describe how it works in qemu/libvirt.
When you install a VM, you can specify compatibility
level (aka "machine type"), and you can query the supported compatibility
levels. Management uses that to find the supported compatibility
and stores the compatibility in XML that is migrated with the VM.
There's also a way to find the latest level which is the
default unless overridden by user, again this level
is recorded and then
- management can make sure migration destination is compatible
- management can avoid migration to hosts without that support


We absolutely can QEMU be in control here, but what
is missing is ability to query compatibility as above.



> >  1. The downtime could be unpredictable if a VM has to move from hosts
> >     to hosts multiple times, which is problematic, especially for NFV.
> >  2. If migration is not possible, maybe the management tool would
> >     prefer not to interrupt the VM on current host.
> > 
> > I have little experience with migration though, so I could be mistaken.
> > 
> > Thanks,
> > Maxime
> > 
> > >
> > >	--yliu
> > >>
> > >>To solve this problem within qemu, qemu has a versioning system based on
> > >>a machine type concept which fundamentally is a version string, by
> > >>specifying that string one can get hardware compatible with a previous
> > >>qemu version. QEMU also reports the latest version and list of versions
> > >>supported so libvirt records the version at VM creation and then is
> > >>careful to use this machine version whenever it migrates a VM.
> > >>
> > >>One might wonder how is this solved with a kernel vhost backend. The
> > >>answer is that it mostly isn't - instead an assumption is made, that
> > >>qemu versions are deployed together with the kernel - this is generally
> > >>true for downstreams.  Thus whenever qemu gains a new feature, it is
> > >>already supported by the kernel as well.  However, if one attempts
> > >>migration with a new qemu from a system with a new to old kernel, one
> > >>would get a failure.
> > >>
> > >>In the world where we have multiple userspace backends, with some of
> > >>these supplied by ISVs, this seems non-realistic.
> > >>
> > >>IMO we need to support vhost backend versioning, ideally
> > >>in a way that will also work for vhost kernel backends.
> > >>
> > >>So I'd like to get some input from both backend and management
> > >>developers on what a good solution would look like.
> > >>
> > >>If we want to emulate the qemu solution, this involves adding the
> > >>concept of interface versions to dpdk.  For example, dpdk could supply a
> > >>file (or utility printing?) with list of versions: latest and versions
> > >>supported. libvirt could read that and
> > >>- store latest version at vm creation
> > >>- pass it around with the vm
> > >>- pass it to qemu
> > >>
> > >>>From here, qemu could pass this over the vhost-user channel,
> > >>thus making sure it's initialized with the correct
> > >>compatible interface.
> > >>
> > >>As version here is an opaque string for libvirt and qemu,
> > >>anything can be used - but I suggest either a list
> > >>of values defining the interface, e.g.
> > >>any_layout=on,max_ring=256
> > >>or a version including the name and vendor of the backend,
> > >>e.g. "org.dpdk.v4.5.6".
> > >>
> > >>Note that typically the list of supported versions can only be
> > >>extended, not shrunk. Also, if the host/guest interface
> > >>does not change, don't change the current version as
> > >>this just creates work for everyone.
> > >>
> > >>Thoughts? Would this work well for management? dpdk? vpp?
> > >>
> > >>Thanks!
> > >>
> > >>--
> > >>MST

^ permalink raw reply

* [PATCH] crypto: remove unused digest-appended feature
From: Fiona Trahe @ 2016-11-17 17:33 UTC (permalink / raw)
  To: dev
  Cc: pablo.de.lara.guarch, fiona.trahe, john.griffin,
	michalx.k.jastrzebski, arkadiuszx.kusztal
In-Reply-To: <1479403997-15437-1-git-send-email-fiona.trahe@intel.com>

The cryptodev API had specified that if the digest address field was
left empty on an authentication operation, then the PMD would assume
the digest was appended to the source or destination data.
This case was not handled at all by most PMDs and incorrectly handled
by the QAT PMD.
As no bugs were raised, it is assumed to be not needed, so this patch
removes it, rather than add handling for the case on all PMDs.
The digest can still be appended to the data, but its
address must now be provided in the op.

Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
---
 drivers/crypto/qat/qat_adf/qat_algs_build_desc.c |  2 ++
 drivers/crypto/qat/qat_crypto.c                  | 18 +-----------------
 lib/librte_cryptodev/rte_crypto_sym.h            | 10 +---------
 3 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/drivers/crypto/qat/qat_adf/qat_algs_build_desc.c b/drivers/crypto/qat/qat_adf/qat_algs_build_desc.c
index 8900668..f4e24b3 100644
--- a/drivers/crypto/qat/qat_adf/qat_algs_build_desc.c
+++ b/drivers/crypto/qat/qat_adf/qat_algs_build_desc.c
@@ -439,6 +439,8 @@ void qat_alg_init_common_hdr(struct icp_qat_fw_comn_req_hdr *header,
 				proto);
 	ICP_QAT_FW_LA_UPDATE_STATE_SET(header->serv_specif_flags,
 					   ICP_QAT_FW_LA_NO_UPDATE_STATE);
+	ICP_QAT_FW_LA_DIGEST_IN_BUFFER_SET(header->serv_specif_flags,
+					ICP_QAT_FW_LA_NO_DIGEST_IN_BUFFER);
 }
 
 int qat_alg_aead_session_create_content_desc_cipher(struct qat_session *cdesc,
diff --git a/drivers/crypto/qat/qat_crypto.c b/drivers/crypto/qat/qat_crypto.c
index 798cd98..6a6bd2e 100644
--- a/drivers/crypto/qat/qat_crypto.c
+++ b/drivers/crypto/qat/qat_crypto.c
@@ -955,7 +955,6 @@ qat_write_hw_desc_entry(struct rte_crypto_op *op, uint8_t *out_msg)
 	uint32_t cipher_len = 0, cipher_ofs = 0;
 	uint32_t auth_len = 0, auth_ofs = 0;
 	uint32_t min_ofs = 0;
-	uint32_t digest_appended = 1;
 	uint64_t buf_start = 0;
 
 
@@ -1068,14 +1067,7 @@ qat_write_hw_desc_entry(struct rte_crypto_op *op, uint8_t *out_msg)
 		}
 		min_ofs = auth_ofs;
 
-		if (op->sym->auth.digest.phys_addr) {
-			ICP_QAT_FW_LA_DIGEST_IN_BUFFER_SET(
-					qat_req->comn_hdr.serv_specif_flags,
-					ICP_QAT_FW_LA_NO_DIGEST_IN_BUFFER);
-			auth_param->auth_res_addr =
-					op->sym->auth.digest.phys_addr;
-			digest_appended = 0;
-		}
+		auth_param->auth_res_addr = op->sym->auth.digest.phys_addr;
 
 		auth_param->u1.aad_adr = op->sym->auth.aad.phys_addr;
 
@@ -1126,14 +1118,6 @@ qat_write_hw_desc_entry(struct rte_crypto_op *op, uint8_t *out_msg)
 		(cipher_param->cipher_offset + cipher_param->cipher_length)
 		: (auth_param->auth_off + auth_param->auth_len);
 
-	if (do_auth && digest_appended) {
-		if (ctx->auth_op == ICP_QAT_HW_AUTH_GENERATE)
-			qat_req->comn_mid.dst_length
-					+= op->sym->auth.digest.length;
-		else
-			qat_req->comn_mid.src_length
-				+= op->sym->auth.digest.length;
-	}
 
 	/* out-of-place operation (OOP) */
 	if (unlikely(op->sym->m_dst != NULL)) {
diff --git a/lib/librte_cryptodev/rte_crypto_sym.h b/lib/librte_cryptodev/rte_crypto_sym.h
index d3d38e4..d694723 100644
--- a/lib/librte_cryptodev/rte_crypto_sym.h
+++ b/lib/librte_cryptodev/rte_crypto_sym.h
@@ -541,8 +541,7 @@ struct rte_crypto_sym_op {
 
 		struct {
 			uint8_t *data;
-			/**< If this member of this structure is set this is a
-			 * pointer to the location where the digest result
+			/**< This points to the location where the digest result
 			 * should be inserted (in the case of digest generation)
 			 * or where the purported digest exists (in the case of
 			 * digest verification).
@@ -560,13 +559,6 @@ struct rte_crypto_sym_op {
 			 * @note
 			 * For GCM (@ref RTE_CRYPTO_AUTH_AES_GCM), for
 			 * "digest result" read "authentication tag T".
-			 *
-			 * If this member is not set the digest result is
-			 * understood to be in the destination buffer for
-			 * digest generation, and in the source buffer for
-			 * digest verification. The location of the digest
-			 * result in this case is immediately following the
-			 * region over which the digest is computed.
 			 */
 			phys_addr_t phys_addr;
 			/**< Physical address of digest */
-- 
2.5.0

^ permalink raw reply related

* [PATCH] crypto: remove unused digest-appended feature
From: Fiona Trahe @ 2016-11-17 17:33 UTC (permalink / raw)
  To: dev
  Cc: pablo.de.lara.guarch, fiona.trahe, john.griffin,
	michalx.k.jastrzebski, arkadiuszx.kusztal

The cryptodev API had specified that if the digest address field was
left empty on an authentication operation, then the PMD would assume
the digest was appended to the source or destination data.
This case was not handled at all by most PMDs and incorrectly handled
by the QAT PMD.
As no bugs were raised, it is assumed to be not needed, so this patch
removes it, rather than add handling for the case on all PMDs.
The digest can still be appended to the data, but its 
address must now be provided in the op.

I've added this cover letter to pre-empt the question which will arise 
in chained-mbuf case.
(Chained-mbuf support will be added in next few weeks for 17.02)
In a chained-mbuf case, if the digest is appended to the data, it could
in theory end up split across the last 2 mbufs. Using the digest address 
field only works for the case where the digest is in a flat buffer. The 
digest-appended feature would have provided one way of handling this, 
pushing the responsibility down to the PMD to follow the chain to find
the digest address and handle it whether in 1 or 2 mbufs. 
However, this is adding extra cycles on the data path in all PMDs  
to handle a case which may not arise in many applications.
The alternative is to give the application the responsibility 
to make sure the digest is in a flat buffer. 
Specifically if an application knows cases of a digest spanning
buffers can arise, then it can check for this case and memcpy the partial
start of the digest from the second last buffer into the last buffer
so it's no longer split.
Does anyone see a problem with this?


Fiona Trahe (1):
  crypto: remove unused digest-appended feature

 drivers/crypto/qat/qat_adf/qat_algs_build_desc.c |  2 ++
 drivers/crypto/qat/qat_crypto.c                  | 18 +-----------------
 lib/librte_cryptodev/rte_crypto_sym.h            | 10 +---------
 3 files changed, 4 insertions(+), 26 deletions(-)

-- 
2.5.0

^ permalink raw reply

* [PATCH] net/ixgbe: ensure link status is updated
From: Laurent Hardy @ 2016-11-17 17:29 UTC (permalink / raw)
  To: helin.zhang, konstantin.ananyev; +Cc: dev

In case of link speed set to 1Gb at peer side (with autoneg or with
defined speed) and cable not plugged-in when device is configured
and started, then link status is not updated properly with new speed
as no link setup is triggered.

To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link
setup each time link_update() is triggered and current link status is
down. When cable is plugged-in, link setup will be performed via
ixgbe_setup_link().

Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c |   20 ++++++++++++++++++++
 drivers/net/ixgbe/ixgbe_ethdev.h |    1 +
 2 files changed, 21 insertions(+)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 52ebbe4..513d1d5 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -2095,6 +2095,7 @@ ixgbe_dev_configure(struct rte_eth_dev *dev)
 
 	/* set flag to update link status after init */
 	intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE;
+	intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
 
 	/*
 	 * Initialize to TRUE. If any of Rx queues doesn't meet the bulk
@@ -3117,8 +3118,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct rte_eth_link link, old;
 	ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN;
+	struct ixgbe_interrupt *intr =
+		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
 	int link_up;
 	int diag;
+	u32 speed = 0;
+	bool autoneg = false;
 
 	link.link_status = ETH_LINK_DOWN;
 	link.link_speed = 0;
@@ -3128,6 +3133,19 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 
 	hw->mac.get_link_status = true;
 
+	if (intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) {
+		speed = hw->phy.autoneg_advertised;
+		if (!speed) {
+			ixgbe_get_link_capabilities(hw, &speed, &autoneg);
+			/* setup the highest link when no autoneg */
+			if (!autoneg) {
+				if (speed & IXGBE_LINK_SPEED_10GB_FULL)
+					speed = IXGBE_LINK_SPEED_10GB_FULL;
+			}
+		}
+		ixgbe_setup_link(hw, speed, true);
+	}
+
 	/* check if it needs to wait to complete, if lsc interrupt is enabled */
 	if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0)
 		diag = ixgbe_check_link(hw, &link_speed, &link_up, 0);
@@ -3145,10 +3163,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 
 	if (link_up == 0) {
 		rte_ixgbe_dev_atomic_write_link_status(dev, &link);
+		intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
 		if (link.link_status == old.link_status)
 			return -1;
 		return 0;
 	}
+	intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
 	link.link_status = ETH_LINK_UP;
 	link.link_duplex = ETH_LINK_FULL_DUPLEX;
 
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index e060c3d..9d335ba 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -43,6 +43,7 @@
 #define IXGBE_FLAG_NEED_LINK_UPDATE (uint32_t)(1 << 0)
 #define IXGBE_FLAG_MAILBOX          (uint32_t)(1 << 1)
 #define IXGBE_FLAG_PHY_INTERRUPT    (uint32_t)(1 << 2)
+#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 3)
 
 /*
  * Defines that were not part of ixgbe_type.h as they are not used by the
-- 
1.7.10.4

^ permalink raw reply related

* [PATCH v2 2/2] Move non-PCI related eth_dev initialization to rte_eth_dev_allocate()
From: Jan Blunck @ 2016-11-17 17:16 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit
In-Reply-To: <1479402972-27470-1-git-send-email-jblunck@infradead.org>

This moves the non-PCI related initialization of the link state interrupt
callback list and the setting of the default MTU to rte_eth_dev_allocate()
so that drivers only need to set non-default values.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 app/test/virtual_pmd.c                 |  2 --
 drivers/net/bonding/rte_eth_bond_api.c |  2 --
 drivers/net/cxgbe/cxgbe_main.c         |  2 --
 drivers/net/mlx4/mlx4.c                |  2 --
 drivers/net/mlx5/mlx5.c                |  3 ---
 drivers/net/null/rte_eth_null.c        |  2 --
 drivers/net/ring/rte_eth_ring.c        |  2 --
 drivers/net/vhost/rte_eth_vhost.c      |  2 --
 lib/librte_ether/rte_ethdev.c          | 11 +++--------
 9 files changed, 3 insertions(+), 25 deletions(-)

diff --git a/app/test/virtual_pmd.c b/app/test/virtual_pmd.c
index 65b44c6..bd969f9 100644
--- a/app/test/virtual_pmd.c
+++ b/app/test/virtual_pmd.c
@@ -601,8 +601,6 @@ virtual_ethdev_create(const char *name, struct ether_addr *mac_addr,
 	eth_dev->data->nb_rx_queues = (uint16_t)1;
 	eth_dev->data->nb_tx_queues = (uint16_t)1;
 
-	TAILQ_INIT(&(eth_dev->link_intr_cbs));
-
 	eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
 	eth_dev->data->dev_link.link_speed = ETH_SPEED_NUM_10G;
 	eth_dev->data->dev_link.link_duplex = ETH_LINK_FULL_DUPLEX;
diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index ed75c28..19347d8 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -200,8 +200,6 @@ rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 	eth_dev->data->nb_rx_queues = (uint16_t)1;
 	eth_dev->data->nb_tx_queues = (uint16_t)1;
 
-	TAILQ_INIT(&(eth_dev->link_intr_cbs));
-
 	eth_dev->data->mac_addrs = rte_zmalloc_socket(name, ETHER_ADDR_LEN, 0,
 			socket_id);
 	if (eth_dev->data->mac_addrs == NULL) {
diff --git a/drivers/net/cxgbe/cxgbe_main.c b/drivers/net/cxgbe/cxgbe_main.c
index 922155b..9e8402b 100644
--- a/drivers/net/cxgbe/cxgbe_main.c
+++ b/drivers/net/cxgbe/cxgbe_main.c
@@ -1172,8 +1172,6 @@ int cxgbe_probe(struct adapter *adapter)
 
 		rte_eth_copy_pci_info(pi->eth_dev, pi->eth_dev->pci_dev);
 
-		TAILQ_INIT(&pi->eth_dev->link_intr_cbs);
-
 		pi->eth_dev->data->mac_addrs = rte_zmalloc(name,
 							   ETHER_ADDR_LEN, 0);
 		if (!pi->eth_dev->data->mac_addrs) {
diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 9264242..d815a52 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -5834,7 +5834,6 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 			eth_dev->rx_pkt_burst = mlx4_rx_burst_secondary_setup;
 		} else {
 			eth_dev->data->dev_private = priv;
-			eth_dev->data->mtu = ETHER_MTU;
 			eth_dev->data->mac_addrs = priv->mac;
 		}
 		eth_dev->pci_dev = pci_dev;
@@ -5845,7 +5844,6 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 
 		priv->dev = eth_dev;
 		eth_dev->dev_ops = &mlx4_dev_ops;
-		TAILQ_INIT(&eth_dev->link_intr_cbs);
 
 		/* Bring Ethernet device up. */
 		DEBUG("forcing Ethernet interface up");
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index b57cad1..0e91f02 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -652,7 +652,6 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 			eth_dev->rx_pkt_burst = mlx5_rx_burst_secondary_setup;
 		} else {
 			eth_dev->data->dev_private = priv;
-			eth_dev->data->mtu = ETHER_MTU;
 			eth_dev->data->mac_addrs = priv->mac;
 		}
 
@@ -662,8 +661,6 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 		priv->dev = eth_dev;
 		eth_dev->dev_ops = &mlx5_dev_ops;
 
-		TAILQ_INIT(&eth_dev->link_intr_cbs);
-
 		/* Bring Ethernet device up. */
 		DEBUG("forcing Ethernet interface up");
 		priv_set_flags(priv, ~IFF_UP, IFF_UP);
diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
index 836d982..f09caf1 100644
--- a/drivers/net/null/rte_eth_null.c
+++ b/drivers/net/null/rte_eth_null.c
@@ -550,8 +550,6 @@ eth_dev_null_create(const char *name,
 	eth_dev->data = data;
 	eth_dev->dev_ops = &ops;
 
-	TAILQ_INIT(&eth_dev->link_intr_cbs);
-
 	eth_dev->driver = NULL;
 	data->dev_flags = RTE_ETH_DEV_DETACHABLE;
 	data->kdrv = RTE_KDRV_NONE;
diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index c1767c4..c7726f4 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -346,8 +346,6 @@ do_eth_dev_ring_create(const char *name,
 	data->drv_name = drivername;
 	data->numa_node = numa_node;
 
-	TAILQ_INIT(&(eth_dev->link_intr_cbs));
-
 	/* finally assign rx and tx ops */
 	eth_dev->rx_pkt_burst = eth_ring_rx;
 	eth_dev->tx_pkt_burst = eth_ring_tx;
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 766d4ef..1912346 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1032,8 +1032,6 @@ eth_dev_vhost_create(const char *name, char *iface_name, int16_t queues,
 	if (vring_state == NULL)
 		goto error;
 
-	TAILQ_INIT(&eth_dev->link_intr_cbs);
-
 	/* now put it all together
 	 * - store queue data in internal,
 	 * - store numa_node info in ethdev data
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 12af4b1..1def33e 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -215,6 +215,9 @@ rte_eth_dev_allocate(const char *name)
 	memset(eth_dev->data, 0, sizeof(*eth_dev->data));
 	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
 	eth_dev->data->port_id = port_id;
+	eth_dev->data->mtu = ETHER_MTU;
+	TAILQ_INIT(&(eth_dev->link_intr_cbs));
+
 	eth_dev->attached = DEV_ATTACHED;
 	eth_dev_last_created_port = port_id;
 	nb_ports++;
@@ -261,14 +264,6 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
 	eth_dev->pci_dev = pci_dev;
 	eth_dev->driver = eth_drv;
 
-	/* init user callbacks */
-	TAILQ_INIT(&(eth_dev->link_intr_cbs));
-
-	/*
-	 * Set the default MTU.
-	 */
-	eth_dev->data->mtu = ETHER_MTU;
-
 	/* Invoke PMD device initialization function */
 	diag = (*eth_drv->eth_dev_init)(eth_dev);
 	if (diag == 0)
-- 
2.7.4

^ permalink raw reply related

* [PATCH v2 1/2] Clear eth_dev->data in rte_eth_dev_allocate()
From: Jan Blunck @ 2016-11-17 17:16 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit

Lets clear the eth_dev->data when allocating a new rte_eth_dev so that
drivers only need to set non-zero values.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 drivers/net/bonding/rte_eth_bond_api.c | 7 -------
 drivers/net/mlx4/mlx4.c                | 1 -
 drivers/net/mlx5/mlx5.c                | 1 -
 drivers/net/mpipe/mpipe_tilegx.c       | 1 -
 lib/librte_ether/rte_ethdev.c          | 2 +-
 5 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 2a3893a..ed75c28 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -202,8 +202,6 @@ rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 
 	TAILQ_INIT(&(eth_dev->link_intr_cbs));
 
-	eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
-
 	eth_dev->data->mac_addrs = rte_zmalloc_socket(name, ETHER_ADDR_LEN, 0,
 			socket_id);
 	if (eth_dev->data->mac_addrs == NULL) {
@@ -211,11 +209,6 @@ rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 		goto err;
 	}
 
-	eth_dev->data->dev_started = 0;
-	eth_dev->data->promiscuous = 0;
-	eth_dev->data->scattered_rx = 0;
-	eth_dev->data->all_multicast = 0;
-
 	eth_dev->dev_ops = &default_dev_ops;
 	eth_dev->data->dev_flags = RTE_ETH_DEV_INTR_LSC |
 		RTE_ETH_DEV_DETACHABLE;
diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index da61a85..9264242 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -5834,7 +5834,6 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 			eth_dev->rx_pkt_burst = mlx4_rx_burst_secondary_setup;
 		} else {
 			eth_dev->data->dev_private = priv;
-			eth_dev->data->rx_mbuf_alloc_failed = 0;
 			eth_dev->data->mtu = ETHER_MTU;
 			eth_dev->data->mac_addrs = priv->mac;
 		}
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 90cc35e..b57cad1 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -652,7 +652,6 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 			eth_dev->rx_pkt_burst = mlx5_rx_burst_secondary_setup;
 		} else {
 			eth_dev->data->dev_private = priv;
-			eth_dev->data->rx_mbuf_alloc_failed = 0;
 			eth_dev->data->mtu = ETHER_MTU;
 			eth_dev->data->mac_addrs = priv->mac;
 		}
diff --git a/drivers/net/mpipe/mpipe_tilegx.c b/drivers/net/mpipe/mpipe_tilegx.c
index fbbbb00..792ab56 100644
--- a/drivers/net/mpipe/mpipe_tilegx.c
+++ b/drivers/net/mpipe/mpipe_tilegx.c
@@ -1603,7 +1603,6 @@ rte_pmd_mpipe_probe(const char *ifname,
 	eth_dev->data->dev_private = priv;
 	eth_dev->data->mac_addrs = &priv->mac_addr;
 
-	eth_dev->data->dev_flags = 0;
 	eth_dev->data->kdrv = RTE_KDRV_NONE;
 	eth_dev->driver = NULL;
 	eth_dev->data->drv_name = drivername;
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index fde8112..12af4b1 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -212,6 +212,7 @@ rte_eth_dev_allocate(const char *name)
 
 	eth_dev = &rte_eth_devices[port_id];
 	eth_dev->data = &rte_eth_dev_data[port_id];
+	memset(eth_dev->data, 0, sizeof(*eth_dev->data));
 	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
 	eth_dev->data->port_id = port_id;
 	eth_dev->attached = DEV_ATTACHED;
@@ -259,7 +260,6 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
 	}
 	eth_dev->pci_dev = pci_dev;
 	eth_dev->driver = eth_drv;
-	eth_dev->data->rx_mbuf_alloc_failed = 0;
 
 	/* init user callbacks */
 	TAILQ_INIT(&(eth_dev->link_intr_cbs));
-- 
2.7.4

^ permalink raw reply related

* Re: [RFC PATCH 0/6] Restructure EAL device model for bus support
From: Jan Blunck @ 2016-11-17 16:54 UTC (permalink / raw)
  To: Shreyansh Jain; +Cc: David Marchand, dev
In-Reply-To: <944308a3-8fb8-2c4d-8f0f-04ae48bf334c@nxp.com>

On Thu, Nov 17, 2016 at 2:08 PM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> On Thursday 17 November 2016 05:25 PM, Jan Blunck wrote:
>>
>> On Thu, Nov 17, 2016 at 6:29 AM, Shreyansh Jain <shreyansh.jain@nxp.com>
>> wrote:
>>>
>>> DPDK has been inherently a PCI inclined framework. Because of this, the
>>> design of device tree (or list) within DPDK is also PCI inclined. A
>>> non-PCI
>>> device doesn't have a way of being expressed without using hooks started
>>> from
>>> EAL to PMD.
>>>
>>> With this cover letter, some patches are presented which try to break
>>> this
>>> strict linkage of EAL with PCI devices. Aim is to generalize the device
>>> hierarchy on the lines of how Linux handles it:
>>>
>>>         device A1
>>>           |
>>>   +==.===='==============.============+ Bus A.
>>>      |                    `--> driver A11     \
>>>   device A2                `-> driver A12      \______
>>>                                                 |CPU |
>>>                                                 /`````
>>>         device A1                              /
>>>           |                                   /
>>>   +==.===='==============.============+ Bus A`
>>>      |                    `--> driver B11
>>>   device A2                `-> driver B12
>>>
>>> Simply put:
>>>  - a bus is connect to CPU (or core)
>>>  - devices are conneted to Bus
>>>  - drivers are running instances which manage one or more devices
>>>  - bus is responsible for identifying devices (and interrupt propogation)
>>>  - driver is responsible for initializing the device
>>>
>>> (*Reusing text from email [1])
>>> In context of DPDK EAL:
>>>  - a generic bus (not a driver, not a device). I don't know how to
>>> categorize
>>>    a bus. It is certainly not a device, and then handler for a bus
>>> (physical)
>>>    can be considered a 'bus driver'. So, just 'rte_bus'.
>>>  - there is a bus for each physical implementation (or virtual). So, a
>>> rte_bus
>>>    Object for PCI, VDEV, ABC, DEF and so on.
>>>  - Buses are registered just like a PMD - RTE_PMD_BUS_REGISTER()
>>>  - Each registered bus is part of a doubly list.
>>>    -- Each device inherits rte_bus
>>>    -- Each driver inherits rte_bus
>>>    -- Device and Drivers lists are part of rte_bus
>>>  - eth_driver is no more required - it was just a holder for PMDs to
>>> register
>>>    themselves. It can be replaced with rte_xxx_driver and corresponding
>>> init/
>>>    uninit moved to rte_driver
>>>  - rte_eth_dev modified to disassociate itself from rte_pci_device and
>>> connect
>>>    to generic rte_device
>>>
>>> Once again, improvising from [1]:
>>>
>>>                                   __ rte_bus_list
>>>                                  /
>>>                      +----------'---+
>>>                      |rte_bus       |
>>>                      | driver_list------> List of rte_bus specific
>>>                      | device_list----    devices
>>>                      | scan         | `-> List of rte_bus associated
>>>                      | match        |     drivers
>>>                      | dump         |
>>>                      | ..some refcnt| (#)
>>>                      +--|------|----+
>>>               _________/        \_________
>>>     +--------/----+                     +-\---------------+
>>>     |rte_device   |                     |rte_driver       |
>>>     | rte_bus     |                     | rte_bus         |
>>>     | rte_driver  |(#)                  | init            |
>>>     |             |                     | uninit          |
>>>     |  devargs    |                     | dev_private_size|
>>>     +---||--------+                     | drv_flags       |(#)
>>>         ||                              | intr_handle(2*) |(#)
>>>         | \                             +----------\\\----+
>>>         |  \_____________                           \\\
>>>         |                \                          |||
>>>  +------|---------+ +----|----------+               |||
>>>  |rte_pci_device  | |rte_xxx_device | (4*)          |||
>>>  | PCI specific   | | xxx device    |               |||
>>>  | info (mem,)    | | specific fns  |              / | \
>>>  +----------------+ +---------------+             /  |  \
>>>                             _____________________/  /    \
>>>                            /                    ___/      \
>>>             +-------------'--+    +------------'---+    +--'------------+
>>>             |rte_pci_driver  |    |rte_vdev_driver |    |rte_xxx_driver |
>>>             | PCI id table,  |    | <probably,     |    | ....          |
>>>             | other driver   |    |  nothing>      |    +---------------+
>>>             | data           |    +----------------+
>>>             +----------------+
>>>
>>>     (1*) Problem is that probe functions have different arguments. So,
>>>          generalizing them might be some rework in the respective device
>>>          layers
>>>     (2*) Interrupt handling for each driver type might be different. I am
>>> not
>>>          sure how to generalize that either. This is grey area for me.
>>>     (3*) Probably exposing a bitmask for device capabilities. Nothing
>>> similar
>>>          exists now to relate it. Don't know if that is useful. Allowing
>>>          applications to question a device about what it supports and
>>> what it
>>>          doesn't - making it more flexible at application layer (but more
>>> code
>>>          as well.)
>>>     (4*) Even vdev would be an instantiated as a device. It is not being
>>> done
>>>          currently.
>>>     (#)  Items which are still pending
>>>
>>> With this cover letter, some patches have been posted. These are _only_
>>> for
>>> discussion purpose. They are not complete and neither compilable.
>>> All the patches, except 0001, have sufficient context about what the
>>> changes
>>> are and rationale for same. Obviously, code is best answer.
>>>
>>> === Patch description: ===
>>>
>>> Patch 0001: Introduce container_of macro. Originally a patch from Jan.
>>>
>>> Patch 0002: introduce changes to rte_device/rte_driver for rte_bus, and
>>>             rte_bus definition itself.
>>>
>>> Patch 0003: Add a new layer for 'bus driver' with linuxapp PCI as an
>>> example
>>>
>>> Patch 0004: Changes with respect to rte_bus APIs and impact on
>>> eal_common_pci
>>
>>
>> From my point of view it is beneficial to keep the PCI support in one
>> place. Moving the match() and scan()
>> to the drivers directory doesn't seem to be technically required but
>> it makes the code harder to read and understand.
>
>
> It is not a technical requirement - just logical placement. These are bus
> specific logic and should exist with the bus driver - where ever that is
> placed.
> Though, I am not entirely sure about 'harder to read'. If a person is
> reading through PCI's bus implementation, I am assuming it would be nice to
> have all the hooks (or default hooks) at same place. Isn't it?
>
> Or, maybe you are suggesting move all librte_eal/*/*pci* out to some common
> location. If so, that is something I haven't yet given serious thought.
>

Yes, either have everything PCI related stuff in drivers or in
librte_eal (or librte_eal_pci). I don't believe it is required to have
two different locations for the rte_pci_bus and the rte_pci_*
functions that are now in librte_eal.


>>
>>
>>> Patch 0005: Change to rte_eal_init (of linuxapp only, for now) for
>>> supporting
>>>             bus->scan. Probe is still being done old way, but in a new
>>> wrapper
>>>
>>> Patch 0006: eth_driver removal and corresponding changes in ixgbe_ethdev,
>>> as
>>>             an example. Includes changes to rte_ethdev to remove most
>>> possible
>>>             PCI references. But, work still remains.
>>
>>
>> Making rte_ethdev independent from PCI device is not directly related
>> to the rest of the patches that add bus support. I think it makes
>> sense to handle this separately because of the impact of refactoring
>> rte_ethdev.
>
>
> Once rte_device is available, the change is independent.
> Only dependency on it was changes required to rte_ethdev.c file because of
> various PCI naming/variables/functions. And that is indeed a very large
> change - including changes to drivers/* which I haven't yet done.
>
> Are you suggesting breaking out of this series? If so, can be done but only
> when formal patches start coming out.
>

Yes, that is fine too.

>
>>
>>
>>>
>>> === Pending Items/Questions: ===
>>>
>>>  - Interrupt and notification handling. How to allow drivers to be
>>> notified
>>>    of presence/plugging of a device.
>>>  - Placement of bus driver/handling code. librte_bus, bus/, drivers/bus?
>>>  -- Also from a pespective of a external library and whether symbols
>>> would be
>>>     available in that.
>>>  -- and secondary processes
>>>  - VDEV bus is missing from current set.
>>>  - Locking of list for supporting hotplugging. Or, at the least safe add/
>>>    remove
>>>  - PMDINFOGEN support or lack of it.
>>>  - Is there ever a case where rte_eth_dev needs to be resolved from
>>>    rte_pci_device? I couldn't find any such use and neither a use-case
>>> for it.
>>>  - There should be a way for Bus APIs to return a generic list handle so
>>> that
>>>    EAL doesn't need to bother about bus->driver_list like dereferencing.
>>> This
>>>    is untidy as well as less portable (in terms of code movement, not
>>> arch).
>>>  - Are more helper hooks required for a bus?
>>>  -- I can think of scan, match, dump, find, plug (device), unplug
>>> (device),
>>>     associate (driver), disassociate (driver). But, most of the work is
>>>     already being done by lower instances (rte_device/driver etc).
>>>
>>> Further:
>>>  - In next few days I will make all necessary changes on the lines
>>> mentioned
>>>    in the patches. This would include changing the drivers/* and
>>> librte_eal/*
>>>  - As an when review comments float in and agreement reached, I will keep
>>>    changing the model
>>>  - There are grey areas like interrupt, notification, locking of bus/list
>>>    which require more discussion. I will try and post a rfc for those as
>>> well
>>>    or if someone can help me on those - great
>>
>>
>> As already hinted on IRC I'm taking a look at the interrupt usage of
>> ethdev.
>
>
> Great. Thank you. Do let me know feedback for any changes that you might
> require along the way.
>
>
>>
>>>  - Change would include PCI bus and VDEV bus handling. A new bus (NXP's
>>> FSLMC)
>>>    would also be layered over this series to verify the model of 'bus
>>>    registration'. This is also part of 17.02 roadmap.
>>>
>>> [1] http://dpdk.org/ml/archives/dev/2016-November/050186.html
>>>
>>> Jan Viktorin (1):
>>>   eal: define container macro
>>>
>>> Shreyansh Jain (5):
>>>   eal: introduce bus-device-driver structure
>>>   bus: add bus driver layer
>>>   eal/common: handle bus abstraction for device/driver objects
>>>   eal: supporting bus model in init process
>>>   eal: removing eth_driver
>>>
>>>  bus/Makefile                               |  36 +++
>>>  bus/pci/Makefile                           |  37 +++
>>>  bus/pci/linuxapp/pci_bus.c                 | 418
>>> +++++++++++++++++++++++++++++
>>>  bus/pci/linuxapp/pci_bus.h                 |  55 ++++
>>>  drivers/net/ixgbe/ixgbe_ethdev.c           |  49 ++--
>>>  lib/librte_eal/common/eal_common_bus.c     | 188 +++++++++++++
>>>  lib/librte_eal/common/eal_common_dev.c     |  31 ++-
>>>  lib/librte_eal/common/eal_common_pci.c     | 226 +++++++++-------
>>>  lib/librte_eal/common/include/rte_bus.h    | 243 +++++++++++++++++
>>>  lib/librte_eal/common/include/rte_common.h |  18 ++
>>>  lib/librte_eal/common/include/rte_dev.h    |  36 +--
>>>  lib/librte_eal/common/include/rte_pci.h    |  11 +-
>>>  lib/librte_eal/linuxapp/eal/Makefile       |   1 +
>>>  lib/librte_eal/linuxapp/eal/eal.c          |  51 +++-
>>>  lib/librte_eal/linuxapp/eal/eal_pci.c      | 298 --------------------
>>>  lib/librte_ether/rte_ethdev.c              |  36 ++-
>>>  lib/librte_ether/rte_ethdev.h              |   6 +-
>>>  17 files changed, 1262 insertions(+), 478 deletions(-)
>>>  create mode 100644 bus/Makefile
>>>  create mode 100644 bus/pci/Makefile
>>>  create mode 100644 bus/pci/linuxapp/pci_bus.c
>>>  create mode 100644 bus/pci/linuxapp/pci_bus.h
>>>  create mode 100644 lib/librte_eal/common/eal_common_bus.c
>>>  create mode 100644 lib/librte_eal/common/include/rte_bus.h
>>>
>>> --
>>> 2.7.4
>>>
>>
>
> -
> Shreyansh

^ permalink raw reply

* Re: [PATCH 2/2] Move non-PCI related eth_dev initialization to rte_eth_dev_allocate()
From: Jan Blunck @ 2016-11-17 16:47 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: dev
In-Reply-To: <5260eda3-89ed-ba1f-284e-5861bac2c724@intel.com>

On Thu, Nov 17, 2016 at 4:46 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 11/17/2016 2:24 PM, Jan Blunck wrote:
>> This moves the non-PCI related initialization of the link state interrupt
>> callback list and the setting of the default MTU to rte_eth_dev_allocate()
>> so that drivers only need to set non-default values.
>>
>> Signed-off-by: Jan Blunck <jblunck@infradead.org>
>> ---
>>  drivers/net/bonding/rte_eth_bond_api.c |  2 --
>>  drivers/net/cxgbe/cxgbe_main.c         |  2 --
>>  drivers/net/mlx4/mlx4.c                |  2 --
>>  drivers/net/mlx5/mlx5.c                |  3 ---
>>  drivers/net/null/rte_eth_null.c        |  2 --
>>  drivers/net/ring/rte_eth_ring.c        |  2 --
>>  drivers/net/vhost/rte_eth_vhost.c      |  2 --
>>  lib/librte_ether/rte_ethdev.c          | 12 ++++--------
>>  8 files changed, 4 insertions(+), 23 deletions(-)
>
> I think following also redundant and can be removed:
> app/test/virtual_pmd.c:
> 604:    TAILQ_INIT(&(eth_dev->link_intr_cbs));
>
> <...>
>
>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>> index 12af4b1..f58a995 100644
>> --- a/lib/librte_ether/rte_ethdev.c
>> +++ b/lib/librte_ether/rte_ethdev.c
>
> What do you think doing same thing for rte_cryptodev.c J
>

Thanks for the review Ferruh. I'll fixup the patches and resend. I'm
currently looking in the rte_bus and rte_eth_dev stuff. If nobody
volunteers to do the changes for cryptodev I can take a look at some
later point.

Thanks,
Jan

>> @@ -215,6 +215,10 @@ rte_eth_dev_allocate(const char *name)
>>       memset(eth_dev->data, 0, sizeof(*eth_dev->data));
>>       snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
>>       eth_dev->data->port_id = port_id;
>> +     eth_dev->data->rx_mbuf_alloc_failed = 0;
>
> This is no more required, because of memset
>
>> +     eth_dev->data->mtu = ETHER_MTU;
>> +     TAILQ_INIT(&(eth_dev->link_intr_cbs));
>> +
>>       eth_dev->attached = DEV_ATTACHED;
>>       eth_dev_last_created_port = port_id;
>>       nb_ports++;
>> @@ -261,14 +265,6 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>>       eth_dev->pci_dev = pci_dev;
>>       eth_dev->driver = eth_drv;
>>
>> -     /* init user callbacks */
>> -     TAILQ_INIT(&(eth_dev->link_intr_cbs));
>> -
>> -     /*
>> -      * Set the default MTU.
>> -      */
>> -     eth_dev->data->mtu = ETHER_MTU;
>> -
>>       /* Invoke PMD device initialization function */
>>       diag = (*eth_drv->eth_dev_init)(eth_dev);
>>       if (diag == 0)
>>
>

^ permalink raw reply

* Re: [RFC PATCH 2/6] eal: introduce bus-device-driver structure
From: Jan Blunck @ 2016-11-17 16:13 UTC (permalink / raw)
  To: Shreyansh Jain; +Cc: David Marchand, dev
In-Reply-To: <1dd1d34a-6e67-3e01-6d53-48222032cdb5@nxp.com>

On Thu, Nov 17, 2016 at 2:00 PM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>
> On Thursday 17 November 2016 04:49 PM, Jan Blunck wrote:
>>
>> On Thu, Nov 17, 2016 at 6:30 AM, Shreyansh Jain <shreyansh.jain@nxp.com>
>> wrote:
>>>
>>> A device is connected to a bus and services by a driver associated with
>>> the bus. It is responsibility of the bus to identify the devices (scan)
>>> and then assign each device to a matching driver.
>>>
>>> A PMD would allocate a rte_xxx_driver and rte_xxx_device.
>>> rte_xxx_driver has rte_driver and rte_bus embedded. Similarly,
>>> rte_xxx_device
>>> has rte_device and rte_bus embedded.
>>
>>
>> I don't think so: the rte_xxx_device embeds the generic rte_device and
>> references a the rte_bus
>> that it is attached to.
>
>
> You mean?
>
>  struct rte_pci_device {
>    struct rte_device device;
>    struct rte_bus *bus;
>    ...
> }
>
> If yes then I have a different view.
> 'device' is connected to a bus. pci_device is just a type of device. Only
> way it should know about the bus is through the parent rte_device.
> rte_device can reference the bus.
>

No. Actually my English was bad. I meant that the rte_device
references the rte_bus but shouldn't embed it.

>
>>
>>> When a ethernet or crypto device (rte_eth_dev, rte_cryptodev) is
>>> allocated,
>>> it contains a reference of rte_device and rte_driver.
>>> Each ethernet device implementation would use container_of for finding
>>> the
>>> enclosing structure of rte_xxx_*.
>>>
>>>                             +-------------------+
>>>  +--------------+           |rte_pci_device     |
>>>  |rte_eth_dev   |           |+-----------------+|
>>>  |+------------+|   .-------->rte_device       ||
>>>  ||rte_device*-----'        |+-----------------+|
>>>  |+------------+|           ||rte_bus          ||
>>>  |              |           |+-----------------+|
>>>  /              /           +-------------------+
>>>
>>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>>> ---
>>>  lib/librte_eal/common/include/rte_bus.h | 243
>>> ++++++++++++++++++++++++++++++++
>>>  lib/librte_eal/common/include/rte_dev.h |  36 ++---
>>>  2 files changed, 261 insertions(+), 18 deletions(-)
>>>  create mode 100644 lib/librte_eal/common/include/rte_bus.h
>>>
>>> diff --git a/lib/librte_eal/common/include/rte_bus.h
>>> b/lib/librte_eal/common/include/rte_bus.h
>>> new file mode 100644
>>> index 0000000..dc3aeb8
>>> --- /dev/null
>>> +++ b/lib/librte_eal/common/include/rte_bus.h
>>> @@ -0,0 +1,243 @@
>>> +/*-
>>> + *   BSD LICENSE
>>> + *
>>> + *   Copyright(c) 2016 NXP
>>> + *   All rights reserved.
>>> + *
>>> + *   Redistribution and use in source and binary forms, with or without
>>> + *   modification, are permitted provided that the following conditions
>>> + *   are met:
>>> + *
>>> + *     * Redistributions of source code must retain the above copyright
>>> + *       notice, this list of conditions and the following disclaimer.
>>> + *     * Redistributions in binary form must reproduce the above
>>> copyright
>>> + *       notice, this list of conditions and the following disclaimer in
>>> + *       the documentation and/or other materials provided with the
>>> + *       distribution.
>>> + *     * Neither the name of NXP nor the names of its
>>> + *       contributors may be used to endorse or promote products derived
>>> + *       from this software without specific prior written permission.
>>> + *
>>> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>>> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>>> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
>>> FOR
>>> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
>>> COPYRIGHT
>>> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
>>> INCIDENTAL,
>>> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>>> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
>>> USE,
>>> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
>>> ANY
>>> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>>> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
>>> USE
>>> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
>>> DAMAGE.
>>> + */
>>> +
>>> +#ifndef _RTE_BUS_H_
>>> +#define _RTE_BUS_H_
>>> +
>>> +/**
>>> + * @file
>>> + *
>>> + * RTE PMD Bus Abstraction interfaces
>>> + *
>>> + * This file exposes APIs and Interfaces for Bus Abstraction over the
>>> devices
>>> + * drivers in EAL.
>>> + */
>>> +
>>> +#ifdef __cplusplus
>>> +extern "C" {
>>> +#endif
>>> +
>>> +#include <stdio.h>
>>> +#include <sys/queue.h>
>>> +
>>> +#include <rte_log.h>
>>> +#include <rte_dev.h>
>>> +
>>> +
>>> +/** Double linked list of buses */
>>> +TAILQ_HEAD(rte_bus_list, rte_bus);
>>> +
>>> +/**
>>> + * Bus specific scan for devices attached on the bus.
>>> + * For each bus object, the scan would be reponsible for finding devices
>>> and
>>> + * adding them to its private device list.
>>> + *
>>> + * Successful detection of a device results in rte_device object which
>>> is
>>> + * embedded within the respective device type (rte_pci_device, for
>>> example).
>>> + * Thereafter, PCI specific bus would need to perform
>>> + * container_of(rte_pci_device) to obtain PCI device object.
>>> + *
>>> + * Scan failure of a bus is not treated as exit criteria for
>>> application. Scan
>>> + * for all other buses would still continue.
>>> + *
>>> + * @param void
>>> + * @return
>>> + *     0 for successful scan
>>> + *     !0 (<0) for unsuccessful scan with error value
>>> + */
>>> +typedef int (* bus_scan_t)(void);
>>> +
>>> +/**
>>> + * Bus specific match for devices and drivers which can service them.
>>> + * For each scanned device, during probe the match would link the
>>> devices with
>>> + * drivers which can service the device.
>>> + *
>>> + * It is the work of each bus handler to obtain the specific device
>>> object
>>> + * using container_of (or typecasting, as a less preferred way).
>>> + *
>>> + * @param drv
>>> + *     Driver object attached to the bus
>>> + * @param dev
>>> + *     Device object which is being probed.
>>> + * @return
>>> + *     0 for successful match
>>> + *     !0 for unsuccessful match
>>> + */
>>> +typedef int (* bus_match_t)(struct rte_driver *drv, struct rte_device
>>> *dev);
>>
>>
>> Do you think this should do match & probe?
>
>
> It is only matching as of now.
> rte_eal_probe() from eal.c calls this function for the bus and probe for
> each device of that bus. (Look at 'pci_probe_all_drivers' called from
> rte_eal_pci_probe).
>
>>
>> I believe it is better to separate this into two functions to match()
>> and probe(). The
>> result of matching tells if the driver would want to claim the device
>> in general. But
>> probe()ing the device should only happen if the device isn't claimed
>> by a driver yet and
>> is not blacklisted.
>
>
> Agree. This is what rte_eal_pci_probe() is doing for PCI. Similar
> implementation would exists in bus specific area (this is a different debate
> where...) for probe on each driver (associated with that bus).
>
>
>>
>>> +
>>> +/**
>>> + * Dump the devices on the bus.
>>> + * Each bus type can define its own definition of information to dump.
>>> + *
>>> + * @param bus
>>> + *     Handle for bus, device from which are to be dumped.
>>> + * @param f
>>> + *     Handle to output device or file.
>>> + * @return void
>>> + */
>>> +typedef void (* bus_dump_t)(struct rte_bus *bus, FILE *f);
>>> +
>>> +/**
>>> + * Search for a specific device in device list of the bus
>>> + * This would rely on the bus specific addressing. Each implementation
>>> would
>>> + * extract its specific device type and perform address compare.
>>> + *
>>> + * @param dev
>>> + *     device handle to search for.
>>> + * @return
>>> + *     rte_device handle for matched device, or NULL
>>> + */
>>> +typedef struct rte_device * (* bus_device_get_t)(struct rte_device
>>> *dev);
>>
>>
>> From my experience it is better to delegate this to a helper:
>>
>> int bus_for_each_dev(struct rte_bus *bus, struct rte_device *start,
>> int (*match)(struct rte_device *dev, void *data), void *data);
>>
>> Based on that you can easily implement all kinds of functions like
>> bus_find_dev(), bus_find_dev_by_name(), bus_dump(), ...
>
>
> Interesting idea. Thanks. I will have a look on this.
>
>>
>>> +
>>> +struct rte_bus {
>>> +       TAILQ_ENTRY(rte_bus) next;   /**< Next bus object in linked list
>>> */
>>> +       struct rte_driver_list driver_list; /**< List of all drivers of
>>> bus */
>>
>>
>> TAILQ_HEAD?
>
>
> Yes. That should be TAILQ_HEAD. I just reused the definition create in
> rte_dev.h using TAILQ_HEAD. I will change this.
>
>>
>>> +       struct rte_device_list device_list; /**< List of all devices on
>>> bus */
>>
>>
>> TAILQ_HEAD?
>
>
> Same as above. I will change this.
>
>>
>>> +       const char *name;            /**< Name of the bus */
>>> +       /* Mandatory hooks */
>>> +       bus_scan_t *scan;            /**< Hook for scanning for devices
>>> */
>>> +       bus_match_t *match;          /**< Hook for matching device &
>>> driver */
>>> +       /* Optional hooks */
>>> +       bus_dump_t *dump_dev;        /**< Hook for dumping devices on bus
>>> */
>>> +       bus_device_get_t *find_dev;  /**< Search for a device on bus */
>>> +};
>>> +
>>> +/** @internal
>>> + * Add a device to a bus.
>>> + *
>>> + * @param bus
>>> + *     Bus on which device is to be added
>>> + * @param dev
>>> + *     Device handle
>>> + * @return
>>> + *     None
>>> + */
>>> +void
>>> +rte_eal_bus_add_device(struct rte_bus *bus, struct rte_device *dev);
>>
>>
>> Why do we need this? From my understanding the rte_bus->scan() is
>> adding the devices to the rte_bus->device_list.
>
>
> Plugging a device *after* scan has been completed. Hotplugging.
> Specially for vdev, this would be required, in my opinion.
>
>
>>
>>> +/** @internal
>>> + * Remove a device from its bus.
>>> + *
>>> + * @param dev
>>> + *     Device handle to remove
>>> + * @return
>>> + *     None
>>> + */
>>> +void
>>> +rte_eal_bus_remove_device(struct rte_device *dev);
>>> +
>>> +/** @internal
>>> + * Associate a driver with a bus.
>>> + *
>>> + * @param bus
>>> + *     Bus on which driver is to be added
>>> + * @param dev
>>> + *     Driver handle
>>> + * @return
>>> + *     None
>>> + */
>>> +void
>>> +rte_eal_bus_add_driver(struct rte_bus *bus, struct rte_driver *drv);
>>> +
>>
>>
>> What happens if a driver is added at runtime to a bus? Does that
>> immediately
>> trigger a match & probe of unclaimed devices?
>
>
> My take:
>  - scan the bus for any newly plugged devices. Might be required in case a
> device is logical entity represented by multiple entries in sysfs.
>  -- add only those which are not already added - maybe address/id match
>  - Trigger driver probe for all devices which don't have a driver assigned
> to them (unclaimed, as you stated).
>
> (This is part of my further changes - I think I forgot to put them as note
> in cover letter).
>
>
>>
>>> +/** @internal
>>> + * Disassociate a driver from its bus.
>>> + *
>>> + * @param dev
>>> + *     Driver handle to remove
>>> + * @return
>>> + *     None
>>> + */
>>> +void
>>> +rte_eal_bus_remove_driver(struct rte_driver *drv);
>>> +
>>> +/**
>>> + * Register a Bus handler.
>>> + *
>>> + * @param driver
>>> + *   A pointer to a rte_bus structure describing the bus
>>> + *   to be registered.
>>> + */
>>> +void rte_eal_bus_register(struct rte_bus *bus);
>>> +
>>> +/**
>>> + * Unregister a Bus handler.
>>> + *
>>> + * @param driver
>>> + *   A pointer to a rte_bus structure describing the bus
>>> + *   to be unregistered.
>>> + */
>>> +void rte_eal_bus_unregister(struct rte_bus *bus);
>>> +
>>> +/**
>>> + * Obtain handle for bus given its name.
>>> + *
>>> + * @param bus_name
>>> + *     Name of the bus handle to search
>>> + * @return
>>> + *     Pointer to Bus object if name matches any registered bus object
>>> + *     NULL, if no matching bus found
>>> + */
>>> +struct rte_bus * rte_eal_get_bus(const char *bus_name);
>>> +
>>> +/**
>>> + * Register a device driver.
>>> + *
>>> + * @param driver
>>> + *   A pointer to a rte_dev structure describing the driver
>>> + *   to be registered.
>>> + */
>>> +void rte_eal_driver_register(struct rte_driver *driver);
>>> +
>>> +/**
>>> + * Unregister a device driver.
>>> + *
>>> + * @param driver
>>> + *   A pointer to a rte_dev structure describing the driver
>>> + *   to be unregistered.
>>> + */
>>> +void rte_eal_driver_unregister(struct rte_driver *driver);
>>> +
>>> +/** Helper for Bus registration */
>>> +#define RTE_PMD_REGISTER_BUS(nm, bus) \
>>> +RTE_INIT(businitfn_ ##nm); \
>>> +static void businitfn_ ##nm(void) \
>>> +{\
>>> +       (bus).name = RTE_STR(nm);\
>>> +       rte_eal_bus_register(&bus); \
>>> +}
>>> +
>>> +#ifdef __cplusplus
>>> +}
>>> +#endif
>>> +
>>> +#endif /* _RTE_BUS_H */
>>> diff --git a/lib/librte_eal/common/include/rte_dev.h
>>> b/lib/librte_eal/common/include/rte_dev.h
>>> index 8840380..b08bab5 100644
>>> --- a/lib/librte_eal/common/include/rte_dev.h
>>> +++ b/lib/librte_eal/common/include/rte_dev.h
>>> @@ -116,12 +116,14 @@ TAILQ_HEAD(rte_device_list, rte_device);
>>>
>>>  /* Forward declaration */
>>>  struct rte_driver;
>>> +struct rte_bus;
>>>
>>>  /**
>>>   * A structure describing a generic device.
>>>   */
>>>  struct rte_device {
>>>         TAILQ_ENTRY(rte_device) next; /**< Next device */
>>> +       struct rte_bus *bus;          /**< Bus on which device is placed
>>> */
>>>         struct rte_driver *driver;    /**< Associated driver */
>>>         int numa_node;                /**< NUMA node connection */
>>>         struct rte_devargs *devargs;  /**< Device user arguments */
>>> @@ -144,31 +146,29 @@ void rte_eal_device_insert(struct rte_device *dev);
>>>  void rte_eal_device_remove(struct rte_device *dev);
>>>
>>>  /**
>>> - * A structure describing a device driver.
>>> + * @internal
>>> + * TODO
>>>   */
>>> -struct rte_driver {
>>> -       TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
>>> -       const char *name;                   /**< Driver name. */
>>> -       const char *alias;              /**< Driver alias. */
>>> -};
>>> +typedef int (*driver_init_t)(struct rte_device *eth_dev);
>>>
>>>  /**
>>> - * Register a device driver.
>>> - *
>>> - * @param driver
>>> - *   A pointer to a rte_dev structure describing the driver
>>> - *   to be registered.
>>> + * @internal
>>> + * TODO
>>>   */
>>> -void rte_eal_driver_register(struct rte_driver *driver);
>>> +typedef int (*driver_uninit_t)(struct rte_device *eth_dev);
>>>
>>>  /**
>>> - * Unregister a device driver.
>>> - *
>>> - * @param driver
>>> - *   A pointer to a rte_dev structure describing the driver
>>> - *   to be unregistered.
>>> + * A structure describing a device driver.
>>>   */
>>> -void rte_eal_driver_unregister(struct rte_driver *driver);
>>> +struct rte_driver {
>>> +       TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
>>> +       struct rte_bus *bus;           /**< Bus which drivers services */
>>
>>
>> I think this should be TAILQ_ENTRY instead.
>
>
> rte_bus
>       `-> device_list-. <- TAILQ_HEAD (rte_device)
>                       |
>                 rte_pci_device:TAILQ_ENTRY(rte_device)
>
> rte_device just references the bus it belong to.
>
> Am I missing something?
>

This is the rte_driver though. Don't we keep the global list of all
registered drivers? If yes, we need a second TAILQ_ENTRY() that is
registered with the rte_bus.


>>
>>> +       const char *name;              /**< Driver name. */
>>> +       const char *alias;             /**< Driver alias. */
>>> +       driver_init_t *init;           /**< Driver initialization */
>>> +       driver_uninit_t *uninit;       /**< Driver uninitialization */
>>
>>
>> Shouldn't this be probe() and remove()?
>
>
> rte_xxx_driver already includes probe and remove.
> Mandate for init is to allocate the ethernet or cryptodev (or some other
> functional unit). Whereas, probe is responsible for driver specific
> intialization - PCI specific, in case of PCI driver.
>

There is close to zero PCI specific code left in
rte_eth_dev_pci_probe() function. Basically it generates the correct
name from the pci_dev. Everything else it delegates to eth_driver
(which should die anyway).

Example:

struct rte_pci_driver foo_ethernet_driver {
     .driver = {
               "net_foo",
     },
     .id_table = ...,
     .drv_flags = RTE_PCI_DRV_NEED_MAPPING,
     .probe = foo_probe_pci,
}

struct rte_pci_driver foo_crypto_driver {
     .driver = {
               "crypto_foo",
     },
    .id_table = ...,
     .drv_flags = RTE_PCI_DRV_NEED_MAPPING,
     .probe = foo_probe_pci,
}

The bus looks at the driver and does everything requested (mapping
etc) before it calls the driver specific probe. If we delegate the
functional initialization to the next layer (rte_driver) this mean the
probe() operates on a rte_device. In this case it would need to upcast
to the rte_pci_device.

>From my understanding this is going two steps forward and one step
back. Maybe this confusion arises because the rte_bus->probe()
function is missing? My understanding is that the PCI rte_pci_bus is
providing the functionality that is required (kernel driver unloading,
mapping, interrupt wiring, ...) by an instance of a rte_pci_driver.



> Initially I had thought of moving rte_pci_driver->probe() to rte_driver.
> But, I couldn't understand where would functional device initialization
> exist. It cannot be rte_xxx_driver. It should be generic place.
>
> Such functional device initialization is in path of the probe function, but
> so does driver specific information. If we remove rte_pci_driver->probe and
> move to rte_driver->probe(), it would only mean rte_driver calling a hook
> within the PCI driver specific domain.
>
>>
>>> +       unsigned int dev_private_size; /**< Size of device private data
>>> ??*/
>>
>>
>> I don't think that dev_private_size is really required at this level.
>> First of all this is related to the rte_eth_dev structure and
>> therefore it really depends on the driver_init_t (aka probe()) if it
>> is actually allocating an rte_eth_dev or not. Anyway that is up to the
>> drivers probe() function.
>
>
> I moved it based on input through ML (or IRC, I don't remember).
> My understanding: if this is common for all rte_xxx_driver instances (for
> allocating their ethernet/crypto structure), it would actually make sense to
> keep at this level so that init() can use it for allocating necessary space
> before handling over the rte_xxx_driver.
>
> But again, I am OK keeping it in the underlying layer - as you have rightly
> pointed out, it would only be used at rte_xxx_driver layer.
>
>>
>>> +};
>>>
>>>  /**
>>>   * Initalize all the registered drivers in this process
>>> --
>>> 2.7.4
>>>
>>
>
> -
> Shreyansh

^ permalink raw reply

* Re: [PATCH 1/2] Clear eth_dev->data in rte_eth_dev_allocate()
From: Ferruh Yigit @ 2016-11-17 16:05 UTC (permalink / raw)
  To: Jan Blunck, dev
In-Reply-To: <1479392685-19608-1-git-send-email-jblunck@infradead.org>

On 11/17/2016 2:24 PM, Jan Blunck wrote:
> Lets clear the eth_dev->data when allocating a new rte_eth_dev so that
> drivers only need to set non-zero values.
> 
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> ---
>  drivers/net/mlx4/mlx4.c       | 1 -
>  drivers/net/mlx5/mlx5.c       | 1 -
>  lib/librte_ether/rte_ethdev.c | 2 +-

+ drivers/net/bonding/rte_eth_bond_api.c ?

-       eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
-
...
-       eth_dev->data->dev_started = 0;
-       eth_dev->data->promiscuous = 0;
-       eth_dev->data->scattered_rx = 0;
-       eth_dev->data->all_multicast = 0;
-

+ drivers/net/mpipe/mpipe_tilegx.c ?

-       eth_dev->data->dev_flags = 0;


<...>

> @@ -212,6 +212,7 @@ rte_eth_dev_allocate(const char *name)
>  
>  	eth_dev = &rte_eth_devices[port_id];
>  	eth_dev->data = &rte_eth_dev_data[port_id];
> +	memset(eth_dev->data, 0, sizeof(*eth_dev->data));
>  	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
>  	eth_dev->data->port_id = port_id;
>  	eth_dev->attached = DEV_ATTACHED;
> @@ -259,7 +260,6 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>  	}
>  	eth_dev->pci_dev = pci_dev;
>  	eth_dev->driver = eth_drv;
> -	eth_dev->data->rx_mbuf_alloc_failed = 0;
>  
>  	/* init user callbacks */
>  	TAILQ_INIT(&(eth_dev->link_intr_cbs));
> 

^ permalink raw reply

* Re: [dpdk-announce] DPDK 16.11 released
From: Thomas Monjalon @ 2016-11-17 16:01 UTC (permalink / raw)
  To: Xu, Qian Q; +Cc: Liu, Yong, dev
In-Reply-To: <82F45D86ADE5454A95A89742C8D1410E3925F412@shsmsx102.ccr.corp.intel.com>

2016-11-17 03:51, Xu, Qian Q:
> It is good that RC1 on Jan.11th is a hard deadline. We also need ensure that RC1 is a complete package with all features. If RC1 is out but missing some big features, then the test results on RC1 vs RC2 may have big difference. 
> So, could we ensure that RC1 is a complete feature package, if any feature missing RC1, then we may postpone the feature to 17.05? Does it make sense? 

Absolutely, yes.


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, November 15, 2016 5:06 PM
> To: Liu, Yong <yong.liu@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [dpdk-announce] DPDK 16.11 released
> 
> Hi and thanks for sharing your time constraints,
> 
> 2016-11-15 01:46, Liu, Yong:
> > As prospect for 17.02, our intel validation team have some concern about the release date.
> > The official day off for Chinese Sprint Festival holiday will be from 27th Jan to 3th Feb.
> > Most of our members may ask for more days leave either before or after the official day off.
> > From our previous experience, it will take 3~4 weeks to do the full function and performance test.
> > If the first candidate release in the middle of Jan, we can do first 
> > round of validation and raise issues to developers.
> 
> The integration deadline is January 5.
> So we can target/expect a RC1 on January 11.
> 
> > And after the holiday, we can keep on the validation process and finish in two weeks.
> > If release date is after Feb, it will be hard for us to cover all cases in release window.
> 
> Yes, we must remind that mid-January is a hard deadline for RC1.
> Then the release will be in mid-February to make sure you have some time after the holidays.
> What about Valentine's day? :)

^ permalink raw reply

* Re: [PATCH 2/2] Move non-PCI related eth_dev initialization to rte_eth_dev_allocate()
From: Ferruh Yigit @ 2016-11-17 15:46 UTC (permalink / raw)
  To: Jan Blunck, dev
In-Reply-To: <1479392685-19608-2-git-send-email-jblunck@infradead.org>

On 11/17/2016 2:24 PM, Jan Blunck wrote:
> This moves the non-PCI related initialization of the link state interrupt
> callback list and the setting of the default MTU to rte_eth_dev_allocate()
> so that drivers only need to set non-default values.
> 
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> ---
>  drivers/net/bonding/rte_eth_bond_api.c |  2 --
>  drivers/net/cxgbe/cxgbe_main.c         |  2 --
>  drivers/net/mlx4/mlx4.c                |  2 --
>  drivers/net/mlx5/mlx5.c                |  3 ---
>  drivers/net/null/rte_eth_null.c        |  2 --
>  drivers/net/ring/rte_eth_ring.c        |  2 --
>  drivers/net/vhost/rte_eth_vhost.c      |  2 --
>  lib/librte_ether/rte_ethdev.c          | 12 ++++--------
>  8 files changed, 4 insertions(+), 23 deletions(-)

I think following also redundant and can be removed:
app/test/virtual_pmd.c:
604:    TAILQ_INIT(&(eth_dev->link_intr_cbs));

<...>

> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 12af4b1..f58a995 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c

What do you think doing same thing for rte_cryptodev.c J

> @@ -215,6 +215,10 @@ rte_eth_dev_allocate(const char *name)
>  	memset(eth_dev->data, 0, sizeof(*eth_dev->data));
>  	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
>  	eth_dev->data->port_id = port_id;
> +	eth_dev->data->rx_mbuf_alloc_failed = 0;

This is no more required, because of memset

> +	eth_dev->data->mtu = ETHER_MTU;
> +	TAILQ_INIT(&(eth_dev->link_intr_cbs));
> +
>  	eth_dev->attached = DEV_ATTACHED;
>  	eth_dev_last_created_port = port_id;
>  	nb_ports++;
> @@ -261,14 +265,6 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>  	eth_dev->pci_dev = pci_dev;
>  	eth_dev->driver = eth_drv;
>  
> -	/* init user callbacks */
> -	TAILQ_INIT(&(eth_dev->link_intr_cbs));
> -
> -	/*
> -	 * Set the default MTU.
> -	 */
> -	eth_dev->data->mtu = ETHER_MTU;
> -
>  	/* Invoke PMD device initialization function */
>  	diag = (*eth_drv->eth_dev_init)(eth_dev);
>  	if (diag == 0)
> 

^ permalink raw reply

* Re: [vpp-dev] dpdk/vpp and cross-version migration for vhost
From: Thomas F Herbert @ 2016-11-17 15:25 UTC (permalink / raw)
  To: Yuanhan Liu, Maxime Coquelin
  Cc: Michael S. Tsirkin, dev, qemu-devel, libvir-list, vpp-dev,
	Marc-André Lureau, Billy McFall
In-Reply-To: <20161117094936.GN5048@yliu-dev.sh.intel.com>

+Billy McFall


On 11/17/2016 04:49 AM, Yuanhan Liu wrote:
> On Thu, Nov 17, 2016 at 09:47:09AM +0100, Maxime Coquelin wrote:
>>
>> On 11/17/2016 09:29 AM, Yuanhan Liu wrote:
>>> As usaual, sorry for late response :/
>>>
>>> On Thu, Oct 13, 2016 at 08:50:52PM +0300, Michael S. Tsirkin wrote:
>>>> Hi!
>>>> So it looks like we face a problem with cross-version
>>>> migration when using vhost. It's not new but became more
>>>> acute with the advent of vhost user.
>>>>
>>>> For users to be able to migrate between different versions
>>>> of the hypervisor the interface exposed to guests
>>>> by hypervisor must stay unchanged.
>>>>
>>>> The problem is that a qemu device is connected
>>>> to a backend in another process, so the interface
>>>> exposed to guests depends on the capabilities of that
>>>> process.
>>>>
>>>> Specifically, for vhost user interface based on virtio, this includes
>>>> the "host features" bitmap that defines the interface, as well as more
>>>> host values such as the max ring size.  Adding new features/changing
>>>> values to this interface is required to make progress, but on the other
>>>> hand we need ability to get the old host features to be compatible.
>>> It looks like to the same issue of vhost-user reconnect to me. For example,
>>>
>>> - start dpdk 16.07 & qemu 2.5
>>> - kill dpdk
>>> - start dpdk 16.11
>>>
>>> Though DPDK 16.11 has more features comparing to dpdk 16.07 (say, indirect),
>>> above should work. Because qemu saves the negotiated features before the
>>> disconnect and stores it back after the reconnection.
>>>
>>>     commit a463215b087c41d7ca94e51aa347cde523831873
>>>     Author: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>     Date:   Mon Jun 6 18:45:05 2016 +0200
>>>
>>>         vhost-net: save & restore vhost-user acked features
>>>
>>>         The initial vhost-user connection sets the features to be negotiated
>>>         with the driver. Renegotiation isn't possible without device reset.
>>>
>>>         To handle reconnection of vhost-user backend, ensure the same set of
>>>         features are provided, and reuse already acked features.
>>>
>>>         Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>>
>>> So we could do similar to vhost-user? I mean, save the acked features
>>> before migration and store it back after it. This should be able to
>>> keep the compatibility. If user downgrades DPDK version, it also could
>>> be easily detected, and then exit with an error to user: migration
>>> failed due to un-compatible vhost features.
>>>
>>> Just some rough thoughts. Makes tiny sense?
>> My understanding is that the management tool has to know whether
>> versions are compatible before initiating the migration:
> Makes sense. How about getting and restoring the acked features through
> qemu command lines then, say, through the monitor interface?
>
> With that, it would be something like:
>
> - start vhost-user backend (DPDK, VPP, or whatever) & qemu in the src host
>
> - read the acked features (through monitor interface)
>
> - start vhost-user backend in the dst host
>
> - start qemu in the dst host with the just queried acked features
>
>    QEMU then is expected to use this feature set for the later vhost-user
>    feature negotitation. Exit if features compatibility is broken.
>
> Thoughts?
>
> 	--yliu
>
>>   1. The downtime could be unpredictable if a VM has to move from hosts
>>      to hosts multiple times, which is problematic, especially for NFV.
>>   2. If migration is not possible, maybe the management tool would
>>      prefer not to interrupt the VM on current host.
>>
>> I have little experience with migration though, so I could be mistaken.
>>
>> Thanks,
>> Maxime
>>
>>> 	--yliu
>>>> To solve this problem within qemu, qemu has a versioning system based on
>>>> a machine type concept which fundamentally is a version string, by
>>>> specifying that string one can get hardware compatible with a previous
>>>> qemu version. QEMU also reports the latest version and list of versions
>>>> supported so libvirt records the version at VM creation and then is
>>>> careful to use this machine version whenever it migrates a VM.
>>>>
>>>> One might wonder how is this solved with a kernel vhost backend. The
>>>> answer is that it mostly isn't - instead an assumption is made, that
>>>> qemu versions are deployed together with the kernel - this is generally
>>>> true for downstreams.  Thus whenever qemu gains a new feature, it is
>>>> already supported by the kernel as well.  However, if one attempts
>>>> migration with a new qemu from a system with a new to old kernel, one
>>>> would get a failure.
>>>>
>>>> In the world where we have multiple userspace backends, with some of
>>>> these supplied by ISVs, this seems non-realistic.
>>>>
>>>> IMO we need to support vhost backend versioning, ideally
>>>> in a way that will also work for vhost kernel backends.
>>>>
>>>> So I'd like to get some input from both backend and management
>>>> developers on what a good solution would look like.
>>>>
>>>> If we want to emulate the qemu solution, this involves adding the
>>>> concept of interface versions to dpdk.  For example, dpdk could supply a
>>>> file (or utility printing?) with list of versions: latest and versions
>>>> supported. libvirt could read that and
>>>> - store latest version at vm creation
>>>> - pass it around with the vm
>>>> - pass it to qemu
>>>>
>>>> >From here, qemu could pass this over the vhost-user channel,
>>>> thus making sure it's initialized with the correct
>>>> compatible interface.
>>>>
>>>> As version here is an opaque string for libvirt and qemu,
>>>> anything can be used - but I suggest either a list
>>>> of values defining the interface, e.g.
>>>> any_layout=on,max_ring=256
>>>> or a version including the name and vendor of the backend,
>>>> e.g. "org.dpdk.v4.5.6".
>>>>
>>>> Note that typically the list of supported versions can only be
>>>> extended, not shrunk. Also, if the host/guest interface
>>>> does not change, don't change the current version as
>>>> this just creates work for everyone.
>>>>
>>>> Thoughts? Would this work well for management? dpdk? vpp?
>>>>
>>>> Thanks!
>>>>
>>>> --
>>>> MST
> _______________________________________________
> vpp-dev mailing list
> vpp-dev@lists.fd.io
> https://lists.fd.io/mailman/listinfo/vpp-dev

-- 
*Thomas F Herbert*
SDN Group
Office of Technology
*Red Hat*

^ permalink raw reply

* [PATCH 2/2] Move non-PCI related eth_dev initialization to rte_eth_dev_allocate()
From: Jan Blunck @ 2016-11-17 14:24 UTC (permalink / raw)
  To: dev
In-Reply-To: <1479392685-19608-1-git-send-email-jblunck@infradead.org>

This moves the non-PCI related initialization of the link state interrupt
callback list and the setting of the default MTU to rte_eth_dev_allocate()
so that drivers only need to set non-default values.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 drivers/net/bonding/rte_eth_bond_api.c |  2 --
 drivers/net/cxgbe/cxgbe_main.c         |  2 --
 drivers/net/mlx4/mlx4.c                |  2 --
 drivers/net/mlx5/mlx5.c                |  3 ---
 drivers/net/null/rte_eth_null.c        |  2 --
 drivers/net/ring/rte_eth_ring.c        |  2 --
 drivers/net/vhost/rte_eth_vhost.c      |  2 --
 lib/librte_ether/rte_ethdev.c          | 12 ++++--------
 8 files changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 2a3893a..18237a3 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -200,8 +200,6 @@ rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 	eth_dev->data->nb_rx_queues = (uint16_t)1;
 	eth_dev->data->nb_tx_queues = (uint16_t)1;
 
-	TAILQ_INIT(&(eth_dev->link_intr_cbs));
-
 	eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
 
 	eth_dev->data->mac_addrs = rte_zmalloc_socket(name, ETHER_ADDR_LEN, 0,
diff --git a/drivers/net/cxgbe/cxgbe_main.c b/drivers/net/cxgbe/cxgbe_main.c
index 922155b..9e8402b 100644
--- a/drivers/net/cxgbe/cxgbe_main.c
+++ b/drivers/net/cxgbe/cxgbe_main.c
@@ -1172,8 +1172,6 @@ int cxgbe_probe(struct adapter *adapter)
 
 		rte_eth_copy_pci_info(pi->eth_dev, pi->eth_dev->pci_dev);
 
-		TAILQ_INIT(&pi->eth_dev->link_intr_cbs);
-
 		pi->eth_dev->data->mac_addrs = rte_zmalloc(name,
 							   ETHER_ADDR_LEN, 0);
 		if (!pi->eth_dev->data->mac_addrs) {
diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 9264242..d815a52 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -5834,7 +5834,6 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 			eth_dev->rx_pkt_burst = mlx4_rx_burst_secondary_setup;
 		} else {
 			eth_dev->data->dev_private = priv;
-			eth_dev->data->mtu = ETHER_MTU;
 			eth_dev->data->mac_addrs = priv->mac;
 		}
 		eth_dev->pci_dev = pci_dev;
@@ -5845,7 +5844,6 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 
 		priv->dev = eth_dev;
 		eth_dev->dev_ops = &mlx4_dev_ops;
-		TAILQ_INIT(&eth_dev->link_intr_cbs);
 
 		/* Bring Ethernet device up. */
 		DEBUG("forcing Ethernet interface up");
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index b57cad1..0e91f02 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -652,7 +652,6 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 			eth_dev->rx_pkt_burst = mlx5_rx_burst_secondary_setup;
 		} else {
 			eth_dev->data->dev_private = priv;
-			eth_dev->data->mtu = ETHER_MTU;
 			eth_dev->data->mac_addrs = priv->mac;
 		}
 
@@ -662,8 +661,6 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 		priv->dev = eth_dev;
 		eth_dev->dev_ops = &mlx5_dev_ops;
 
-		TAILQ_INIT(&eth_dev->link_intr_cbs);
-
 		/* Bring Ethernet device up. */
 		DEBUG("forcing Ethernet interface up");
 		priv_set_flags(priv, ~IFF_UP, IFF_UP);
diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
index 836d982..f09caf1 100644
--- a/drivers/net/null/rte_eth_null.c
+++ b/drivers/net/null/rte_eth_null.c
@@ -550,8 +550,6 @@ eth_dev_null_create(const char *name,
 	eth_dev->data = data;
 	eth_dev->dev_ops = &ops;
 
-	TAILQ_INIT(&eth_dev->link_intr_cbs);
-
 	eth_dev->driver = NULL;
 	data->dev_flags = RTE_ETH_DEV_DETACHABLE;
 	data->kdrv = RTE_KDRV_NONE;
diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index c1767c4..c7726f4 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -346,8 +346,6 @@ do_eth_dev_ring_create(const char *name,
 	data->drv_name = drivername;
 	data->numa_node = numa_node;
 
-	TAILQ_INIT(&(eth_dev->link_intr_cbs));
-
 	/* finally assign rx and tx ops */
 	eth_dev->rx_pkt_burst = eth_ring_rx;
 	eth_dev->tx_pkt_burst = eth_ring_tx;
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 766d4ef..1912346 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1032,8 +1032,6 @@ eth_dev_vhost_create(const char *name, char *iface_name, int16_t queues,
 	if (vring_state == NULL)
 		goto error;
 
-	TAILQ_INIT(&eth_dev->link_intr_cbs);
-
 	/* now put it all together
 	 * - store queue data in internal,
 	 * - store numa_node info in ethdev data
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 12af4b1..f58a995 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -215,6 +215,10 @@ rte_eth_dev_allocate(const char *name)
 	memset(eth_dev->data, 0, sizeof(*eth_dev->data));
 	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
 	eth_dev->data->port_id = port_id;
+	eth_dev->data->rx_mbuf_alloc_failed = 0;
+	eth_dev->data->mtu = ETHER_MTU;
+	TAILQ_INIT(&(eth_dev->link_intr_cbs));
+
 	eth_dev->attached = DEV_ATTACHED;
 	eth_dev_last_created_port = port_id;
 	nb_ports++;
@@ -261,14 +265,6 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
 	eth_dev->pci_dev = pci_dev;
 	eth_dev->driver = eth_drv;
 
-	/* init user callbacks */
-	TAILQ_INIT(&(eth_dev->link_intr_cbs));
-
-	/*
-	 * Set the default MTU.
-	 */
-	eth_dev->data->mtu = ETHER_MTU;
-
 	/* Invoke PMD device initialization function */
 	diag = (*eth_drv->eth_dev_init)(eth_dev);
 	if (diag == 0)
-- 
2.7.4

^ permalink raw reply related

* [PATCH 1/2] Clear eth_dev->data in rte_eth_dev_allocate()
From: Jan Blunck @ 2016-11-17 14:24 UTC (permalink / raw)
  To: dev

Lets clear the eth_dev->data when allocating a new rte_eth_dev so that
drivers only need to set non-zero values.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 drivers/net/mlx4/mlx4.c       | 1 -
 drivers/net/mlx5/mlx5.c       | 1 -
 lib/librte_ether/rte_ethdev.c | 2 +-
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index da61a85..9264242 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -5834,7 +5834,6 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 			eth_dev->rx_pkt_burst = mlx4_rx_burst_secondary_setup;
 		} else {
 			eth_dev->data->dev_private = priv;
-			eth_dev->data->rx_mbuf_alloc_failed = 0;
 			eth_dev->data->mtu = ETHER_MTU;
 			eth_dev->data->mac_addrs = priv->mac;
 		}
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 90cc35e..b57cad1 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -652,7 +652,6 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 			eth_dev->rx_pkt_burst = mlx5_rx_burst_secondary_setup;
 		} else {
 			eth_dev->data->dev_private = priv;
-			eth_dev->data->rx_mbuf_alloc_failed = 0;
 			eth_dev->data->mtu = ETHER_MTU;
 			eth_dev->data->mac_addrs = priv->mac;
 		}
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index fde8112..12af4b1 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -212,6 +212,7 @@ rte_eth_dev_allocate(const char *name)
 
 	eth_dev = &rte_eth_devices[port_id];
 	eth_dev->data = &rte_eth_dev_data[port_id];
+	memset(eth_dev->data, 0, sizeof(*eth_dev->data));
 	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
 	eth_dev->data->port_id = port_id;
 	eth_dev->attached = DEV_ATTACHED;
@@ -259,7 +260,6 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
 	}
 	eth_dev->pci_dev = pci_dev;
 	eth_dev->driver = eth_drv;
-	eth_dev->data->rx_mbuf_alloc_failed = 0;
 
 	/* init user callbacks */
 	TAILQ_INIT(&(eth_dev->link_intr_cbs));
-- 
2.7.4

^ permalink raw reply related

* Re: [PATCH 1/5] ethdev: add firmware version get
From: Thomas Monjalon @ 2016-11-17 13:36 UTC (permalink / raw)
  To: Qiming Yang; +Cc: dev, remy.horton, jingjing.wu, jing.d.chen
In-Reply-To: <1479375779-46629-2-git-send-email-qiming.yang@intel.com>

2016-11-17 17:42, Qiming Yang:
> This patch added API for 'rte_eth_dev_fwver_get'
> 
> void rte_eth_dev_fwver_get(uint8_t port_id,
> char *fw_version, int fw_length);

Copying some code here doesn't help really help.
Could you describe what we can expect in this string?
How can we compare this version number across different
devices of the same driver?
How does it apply to FPGA devices?
What is the potential use?

[...]
>  /**
> + * Retrieve the firmware version of an Ethernet device.

We should stop talking about Ethernet device.
Networking device is more appropriate.
And in this case, "device" is enough.

> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param fw_version
> + *   A pointer the firmware version of an Ethernet device
> + * @param fw_length
> + *   The size of the firmware version, which should be large enough to store
> + *   the firmware version of the device.

How do we know that the length is too small?

> + */
> +void rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int fw_length);

Why not returning some errors?

You forgot to remove the deprecation notice in this patch.

PS: the series is broken as some patches are not numbered and this one is
not the first one. Please use a cover-letter to introduce such series.

^ permalink raw reply

* [PATCH] doc: announce kni_vhost removal
From: Ferruh Yigit @ 2016-11-17 13:27 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 doc/guides/prog_guide/kernel_nic_interface.rst | 2 ++
 doc/guides/rel_notes/deprecation.rst           | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/doc/guides/prog_guide/kernel_nic_interface.rst b/doc/guides/prog_guide/kernel_nic_interface.rst
index eb16e2e..4f25595 100644
--- a/doc/guides/prog_guide/kernel_nic_interface.rst
+++ b/doc/guides/prog_guide/kernel_nic_interface.rst
@@ -169,6 +169,8 @@ This provides flexibility in multiprocess scenarios
 (where the KNI is created in the primary process but the callbacks are handled in the secondary one).
 The constraint is that a single process can register and handle the requests.
 
+.. _kni_vhost_backend-label:
+
 KNI Working as a Kernel vHost Backend
 -------------------------------------
 
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 2d17bc6..956473a 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -71,3 +71,9 @@ Deprecation Notices
 * mempool: The functions for single/multi producer/consumer are deprecated
   and will be removed in 17.02.
   It is replaced by ``rte_mempool_generic_get/put`` functions.
+
+* kni: Remove :ref:`kni_vhost_backend-label` feature (KNI_VHOST) in 17.05 release.
+  :doc:`Vhost Library </prog_guide/vhost_lib>` is currently preferred method for
+  guest - host communication. Just for clarification, this is not to remove KNI
+  or VHOST feature, but KNI_VHOST which is a KNI feature enabled via a compile
+  time option, and disabled by default.
-- 
2.9.3

^ permalink raw reply related

* Re: [RFC PATCH 0/6] Restructure EAL device model for bus support
From: Shreyansh Jain @ 2016-11-17 13:08 UTC (permalink / raw)
  To: Jan Blunck; +Cc: David Marchand, dev
In-Reply-To: <CALe+Z000tpD3SaREXcZNsW4fHLvKHwW60qxG5bDr_Q8RO908pA@mail.gmail.com>

On Thursday 17 November 2016 05:25 PM, Jan Blunck wrote:
> On Thu, Nov 17, 2016 at 6:29 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>> DPDK has been inherently a PCI inclined framework. Because of this, the
>> design of device tree (or list) within DPDK is also PCI inclined. A non-PCI
>> device doesn't have a way of being expressed without using hooks started from
>> EAL to PMD.
>>
>> With this cover letter, some patches are presented which try to break this
>> strict linkage of EAL with PCI devices. Aim is to generalize the device
>> hierarchy on the lines of how Linux handles it:
>>
>>         device A1
>>           |
>>   +==.===='==============.============+ Bus A.
>>      |                    `--> driver A11     \
>>   device A2                `-> driver A12      \______
>>                                                 |CPU |
>>                                                 /`````
>>         device A1                              /
>>           |                                   /
>>   +==.===='==============.============+ Bus A`
>>      |                    `--> driver B11
>>   device A2                `-> driver B12
>>
>> Simply put:
>>  - a bus is connect to CPU (or core)
>>  - devices are conneted to Bus
>>  - drivers are running instances which manage one or more devices
>>  - bus is responsible for identifying devices (and interrupt propogation)
>>  - driver is responsible for initializing the device
>>
>> (*Reusing text from email [1])
>> In context of DPDK EAL:
>>  - a generic bus (not a driver, not a device). I don't know how to categorize
>>    a bus. It is certainly not a device, and then handler for a bus (physical)
>>    can be considered a 'bus driver'. So, just 'rte_bus'.
>>  - there is a bus for each physical implementation (or virtual). So, a rte_bus
>>    Object for PCI, VDEV, ABC, DEF and so on.
>>  - Buses are registered just like a PMD - RTE_PMD_BUS_REGISTER()
>>  - Each registered bus is part of a doubly list.
>>    -- Each device inherits rte_bus
>>    -- Each driver inherits rte_bus
>>    -- Device and Drivers lists are part of rte_bus
>>  - eth_driver is no more required - it was just a holder for PMDs to register
>>    themselves. It can be replaced with rte_xxx_driver and corresponding init/
>>    uninit moved to rte_driver
>>  - rte_eth_dev modified to disassociate itself from rte_pci_device and connect
>>    to generic rte_device
>>
>> Once again, improvising from [1]:
>>
>>                                   __ rte_bus_list
>>                                  /
>>                      +----------'---+
>>                      |rte_bus       |
>>                      | driver_list------> List of rte_bus specific
>>                      | device_list----    devices
>>                      | scan         | `-> List of rte_bus associated
>>                      | match        |     drivers
>>                      | dump         |
>>                      | ..some refcnt| (#)
>>                      +--|------|----+
>>               _________/        \_________
>>     +--------/----+                     +-\---------------+
>>     |rte_device   |                     |rte_driver       |
>>     | rte_bus     |                     | rte_bus         |
>>     | rte_driver  |(#)                  | init            |
>>     |             |                     | uninit          |
>>     |  devargs    |                     | dev_private_size|
>>     +---||--------+                     | drv_flags       |(#)
>>         ||                              | intr_handle(2*) |(#)
>>         | \                             +----------\\\----+
>>         |  \_____________                           \\\
>>         |                \                          |||
>>  +------|---------+ +----|----------+               |||
>>  |rte_pci_device  | |rte_xxx_device | (4*)          |||
>>  | PCI specific   | | xxx device    |               |||
>>  | info (mem,)    | | specific fns  |              / | \
>>  +----------------+ +---------------+             /  |  \
>>                             _____________________/  /    \
>>                            /                    ___/      \
>>             +-------------'--+    +------------'---+    +--'------------+
>>             |rte_pci_driver  |    |rte_vdev_driver |    |rte_xxx_driver |
>>             | PCI id table,  |    | <probably,     |    | ....          |
>>             | other driver   |    |  nothing>      |    +---------------+
>>             | data           |    +----------------+
>>             +----------------+
>>
>>     (1*) Problem is that probe functions have different arguments. So,
>>          generalizing them might be some rework in the respective device
>>          layers
>>     (2*) Interrupt handling for each driver type might be different. I am not
>>          sure how to generalize that either. This is grey area for me.
>>     (3*) Probably exposing a bitmask for device capabilities. Nothing similar
>>          exists now to relate it. Don't know if that is useful. Allowing
>>          applications to question a device about what it supports and what it
>>          doesn't - making it more flexible at application layer (but more code
>>          as well.)
>>     (4*) Even vdev would be an instantiated as a device. It is not being done
>>          currently.
>>     (#)  Items which are still pending
>>
>> With this cover letter, some patches have been posted. These are _only_ for
>> discussion purpose. They are not complete and neither compilable.
>> All the patches, except 0001, have sufficient context about what the changes
>> are and rationale for same. Obviously, code is best answer.
>>
>> === Patch description: ===
>>
>> Patch 0001: Introduce container_of macro. Originally a patch from Jan.
>>
>> Patch 0002: introduce changes to rte_device/rte_driver for rte_bus, and
>>             rte_bus definition itself.
>>
>> Patch 0003: Add a new layer for 'bus driver' with linuxapp PCI as an example
>>
>> Patch 0004: Changes with respect to rte_bus APIs and impact on eal_common_pci
>
> From my point of view it is beneficial to keep the PCI support in one
> place. Moving the match() and scan()
> to the drivers directory doesn't seem to be technically required but
> it makes the code harder to read and understand.

It is not a technical requirement - just logical placement. These are 
bus specific logic and should exist with the bus driver - where ever 
that is placed.
Though, I am not entirely sure about 'harder to read'. If a person is 
reading through PCI's bus implementation, I am assuming it would be nice 
to have all the hooks (or default hooks) at same place. Isn't it?

Or, maybe you are suggesting move all librte_eal/*/*pci* out to some 
common location. If so, that is something I haven't yet given serious 
thought.

>
>
>> Patch 0005: Change to rte_eal_init (of linuxapp only, for now) for supporting
>>             bus->scan. Probe is still being done old way, but in a new wrapper
>>
>> Patch 0006: eth_driver removal and corresponding changes in ixgbe_ethdev, as
>>             an example. Includes changes to rte_ethdev to remove most possible
>>             PCI references. But, work still remains.
>
> Making rte_ethdev independent from PCI device is not directly related
> to the rest of the patches that add bus support. I think it makes
> sense to handle this separately because of the impact of refactoring
> rte_ethdev.

Once rte_device is available, the change is independent.
Only dependency on it was changes required to rte_ethdev.c file because 
of various PCI naming/variables/functions. And that is indeed a very 
large change - including changes to drivers/* which I haven't yet done.

Are you suggesting breaking out of this series? If so, can be done but 
only when formal patches start coming out.

>
>
>>
>> === Pending Items/Questions: ===
>>
>>  - Interrupt and notification handling. How to allow drivers to be notified
>>    of presence/plugging of a device.
>>  - Placement of bus driver/handling code. librte_bus, bus/, drivers/bus?
>>  -- Also from a pespective of a external library and whether symbols would be
>>     available in that.
>>  -- and secondary processes
>>  - VDEV bus is missing from current set.
>>  - Locking of list for supporting hotplugging. Or, at the least safe add/
>>    remove
>>  - PMDINFOGEN support or lack of it.
>>  - Is there ever a case where rte_eth_dev needs to be resolved from
>>    rte_pci_device? I couldn't find any such use and neither a use-case for it.
>>  - There should be a way for Bus APIs to return a generic list handle so that
>>    EAL doesn't need to bother about bus->driver_list like dereferencing. This
>>    is untidy as well as less portable (in terms of code movement, not arch).
>>  - Are more helper hooks required for a bus?
>>  -- I can think of scan, match, dump, find, plug (device), unplug (device),
>>     associate (driver), disassociate (driver). But, most of the work is
>>     already being done by lower instances (rte_device/driver etc).
>>
>> Further:
>>  - In next few days I will make all necessary changes on the lines mentioned
>>    in the patches. This would include changing the drivers/* and librte_eal/*
>>  - As an when review comments float in and agreement reached, I will keep
>>    changing the model
>>  - There are grey areas like interrupt, notification, locking of bus/list
>>    which require more discussion. I will try and post a rfc for those as well
>>    or if someone can help me on those - great
>
> As already hinted on IRC I'm taking a look at the interrupt usage of ethdev.

Great. Thank you. Do let me know feedback for any changes that you might 
require along the way.

>
>>  - Change would include PCI bus and VDEV bus handling. A new bus (NXP's FSLMC)
>>    would also be layered over this series to verify the model of 'bus
>>    registration'. This is also part of 17.02 roadmap.
>>
>> [1] http://dpdk.org/ml/archives/dev/2016-November/050186.html
>>
>> Jan Viktorin (1):
>>   eal: define container macro
>>
>> Shreyansh Jain (5):
>>   eal: introduce bus-device-driver structure
>>   bus: add bus driver layer
>>   eal/common: handle bus abstraction for device/driver objects
>>   eal: supporting bus model in init process
>>   eal: removing eth_driver
>>
>>  bus/Makefile                               |  36 +++
>>  bus/pci/Makefile                           |  37 +++
>>  bus/pci/linuxapp/pci_bus.c                 | 418 +++++++++++++++++++++++++++++
>>  bus/pci/linuxapp/pci_bus.h                 |  55 ++++
>>  drivers/net/ixgbe/ixgbe_ethdev.c           |  49 ++--
>>  lib/librte_eal/common/eal_common_bus.c     | 188 +++++++++++++
>>  lib/librte_eal/common/eal_common_dev.c     |  31 ++-
>>  lib/librte_eal/common/eal_common_pci.c     | 226 +++++++++-------
>>  lib/librte_eal/common/include/rte_bus.h    | 243 +++++++++++++++++
>>  lib/librte_eal/common/include/rte_common.h |  18 ++
>>  lib/librte_eal/common/include/rte_dev.h    |  36 +--
>>  lib/librte_eal/common/include/rte_pci.h    |  11 +-
>>  lib/librte_eal/linuxapp/eal/Makefile       |   1 +
>>  lib/librte_eal/linuxapp/eal/eal.c          |  51 +++-
>>  lib/librte_eal/linuxapp/eal/eal_pci.c      | 298 --------------------
>>  lib/librte_ether/rte_ethdev.c              |  36 ++-
>>  lib/librte_ether/rte_ethdev.h              |   6 +-
>>  17 files changed, 1262 insertions(+), 478 deletions(-)
>>  create mode 100644 bus/Makefile
>>  create mode 100644 bus/pci/Makefile
>>  create mode 100644 bus/pci/linuxapp/pci_bus.c
>>  create mode 100644 bus/pci/linuxapp/pci_bus.h
>>  create mode 100644 lib/librte_eal/common/eal_common_bus.c
>>  create mode 100644 lib/librte_eal/common/include/rte_bus.h
>>
>> --
>> 2.7.4
>>
>

-
Shreyansh

^ permalink raw reply

* Re: [RFC PATCH 1/6] eal: define container macro
From: Shreyansh Jain @ 2016-11-17 13:01 UTC (permalink / raw)
  To: Jan Blunck; +Cc: David Marchand, dev, Jan Viktorin
In-Reply-To: <CALe+Z00BZKT660kFvLQC9YxvbpgTki2CpWL9rCKdVW85Qmce+g@mail.gmail.com>

On Thursday 17 November 2016 05:36 PM, Jan Blunck wrote:
> On Thu, Nov 17, 2016 at 6:30 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>> From: Jan Viktorin <viktorin@rehivetech.com>
>>
>> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>> ---
>>  lib/librte_eal/common/include/rte_common.h | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
>> index db5ac91..8152bd9 100644
>> --- a/lib/librte_eal/common/include/rte_common.h
>> +++ b/lib/librte_eal/common/include/rte_common.h
>> @@ -331,6 +331,24 @@ rte_bsf32(uint32_t v)
>>  #define offsetof(TYPE, MEMBER)  __builtin_offsetof (TYPE, MEMBER)
>>  #endif
>>
>> +/**
>> + * Return pointer to the wrapping struct instance.
>> + * Example:
>> + *
>> + *  struct wrapper {
>> + *      ...
>> + *      struct child c;
>> + *      ...
>> + *  };
>> + *
>> + *  struct child *x = obtain(...);
>> + *  struct wrapper *w = container_of(x, struct wrapper, c);
>> + */
>> +#ifndef container_of
>> +#define container_of(p, type, member) \
>> +       ((type *) (((char *) (p)) - offsetof(type, member)))
>
> Are there any reasons why you choose to implement this in a non-type
> safe way? Catching obvious bugs at compile time is in the interest of
> us and our users from my point of view.

No specific reason. I just took an existing patchset floating in ML. If 
you can point me to some better implementation, I will use that.

>
>
>> +#endif
>> +
>>  #define _RTE_STR(x) #x
>>  /** Take a macro value and get a string version of it */
>>  #define RTE_STR(x) _RTE_STR(x)
>> --
>> 2.7.4
>>
>


-- 
-
Shreyansh

^ permalink raw reply

* Re: [RFC PATCH 2/6] eal: introduce bus-device-driver structure
From: Shreyansh Jain @ 2016-11-17 13:00 UTC (permalink / raw)
  To: Jan Blunck; +Cc: David Marchand, dev
In-Reply-To: <CALe+Z02UH-TZXEwvW2+ymnD7HZJ5AwhE-EJN=P=ABZ0o3F-ggQ@mail.gmail.com>

Hello Jan,

Thanks for comments. Replies inline.

On Thursday 17 November 2016 04:49 PM, Jan Blunck wrote:
> On Thu, Nov 17, 2016 at 6:30 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>> A device is connected to a bus and services by a driver associated with
>> the bus. It is responsibility of the bus to identify the devices (scan)
>> and then assign each device to a matching driver.
>>
>> A PMD would allocate a rte_xxx_driver and rte_xxx_device.
>> rte_xxx_driver has rte_driver and rte_bus embedded. Similarly, rte_xxx_device
>> has rte_device and rte_bus embedded.
>
> I don't think so: the rte_xxx_device embeds the generic rte_device and
> references a the rte_bus
> that it is attached to.

You mean?

  struct rte_pci_device {
    struct rte_device device;
    struct rte_bus *bus;
    ...
}

If yes then I have a different view.
'device' is connected to a bus. pci_device is just a type of device. 
Only way it should know about the bus is through the parent rte_device.
rte_device can reference the bus.

>
>> When a ethernet or crypto device (rte_eth_dev, rte_cryptodev) is allocated,
>> it contains a reference of rte_device and rte_driver.
>> Each ethernet device implementation would use container_of for finding the
>> enclosing structure of rte_xxx_*.
>>
>>                             +-------------------+
>>  +--------------+           |rte_pci_device     |
>>  |rte_eth_dev   |           |+-----------------+|
>>  |+------------+|   .-------->rte_device       ||
>>  ||rte_device*-----'        |+-----------------+|
>>  |+------------+|           ||rte_bus          ||
>>  |              |           |+-----------------+|
>>  /              /           +-------------------+
>>
>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>> ---
>>  lib/librte_eal/common/include/rte_bus.h | 243 ++++++++++++++++++++++++++++++++
>>  lib/librte_eal/common/include/rte_dev.h |  36 ++---
>>  2 files changed, 261 insertions(+), 18 deletions(-)
>>  create mode 100644 lib/librte_eal/common/include/rte_bus.h
>>
>> diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
>> new file mode 100644
>> index 0000000..dc3aeb8
>> --- /dev/null
>> +++ b/lib/librte_eal/common/include/rte_bus.h
>> @@ -0,0 +1,243 @@
>> +/*-
>> + *   BSD LICENSE
>> + *
>> + *   Copyright(c) 2016 NXP
>> + *   All rights reserved.
>> + *
>> + *   Redistribution and use in source and binary forms, with or without
>> + *   modification, are permitted provided that the following conditions
>> + *   are met:
>> + *
>> + *     * Redistributions of source code must retain the above copyright
>> + *       notice, this list of conditions and the following disclaimer.
>> + *     * Redistributions in binary form must reproduce the above copyright
>> + *       notice, this list of conditions and the following disclaimer in
>> + *       the documentation and/or other materials provided with the
>> + *       distribution.
>> + *     * Neither the name of NXP nor the names of its
>> + *       contributors may be used to endorse or promote products derived
>> + *       from this software without specific prior written permission.
>> + *
>> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + */
>> +
>> +#ifndef _RTE_BUS_H_
>> +#define _RTE_BUS_H_
>> +
>> +/**
>> + * @file
>> + *
>> + * RTE PMD Bus Abstraction interfaces
>> + *
>> + * This file exposes APIs and Interfaces for Bus Abstraction over the devices
>> + * drivers in EAL.
>> + */
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>> +#include <stdio.h>
>> +#include <sys/queue.h>
>> +
>> +#include <rte_log.h>
>> +#include <rte_dev.h>
>> +
>> +
>> +/** Double linked list of buses */
>> +TAILQ_HEAD(rte_bus_list, rte_bus);
>> +
>> +/**
>> + * Bus specific scan for devices attached on the bus.
>> + * For each bus object, the scan would be reponsible for finding devices and
>> + * adding them to its private device list.
>> + *
>> + * Successful detection of a device results in rte_device object which is
>> + * embedded within the respective device type (rte_pci_device, for example).
>> + * Thereafter, PCI specific bus would need to perform
>> + * container_of(rte_pci_device) to obtain PCI device object.
>> + *
>> + * Scan failure of a bus is not treated as exit criteria for application. Scan
>> + * for all other buses would still continue.
>> + *
>> + * @param void
>> + * @return
>> + *     0 for successful scan
>> + *     !0 (<0) for unsuccessful scan with error value
>> + */
>> +typedef int (* bus_scan_t)(void);
>> +
>> +/**
>> + * Bus specific match for devices and drivers which can service them.
>> + * For each scanned device, during probe the match would link the devices with
>> + * drivers which can service the device.
>> + *
>> + * It is the work of each bus handler to obtain the specific device object
>> + * using container_of (or typecasting, as a less preferred way).
>> + *
>> + * @param drv
>> + *     Driver object attached to the bus
>> + * @param dev
>> + *     Device object which is being probed.
>> + * @return
>> + *     0 for successful match
>> + *     !0 for unsuccessful match
>> + */
>> +typedef int (* bus_match_t)(struct rte_driver *drv, struct rte_device *dev);
>
> Do you think this should do match & probe?

It is only matching as of now.
rte_eal_probe() from eal.c calls this function for the bus and probe for 
each device of that bus. (Look at 'pci_probe_all_drivers' called from 
rte_eal_pci_probe).

>
> I believe it is better to separate this into two functions to match()
> and probe(). The
> result of matching tells if the driver would want to claim the device
> in general. But
> probe()ing the device should only happen if the device isn't claimed
> by a driver yet and
> is not blacklisted.

Agree. This is what rte_eal_pci_probe() is doing for PCI. Similar 
implementation would exists in bus specific area (this is a different 
debate where...) for probe on each driver (associated with that bus).

>
>> +
>> +/**
>> + * Dump the devices on the bus.
>> + * Each bus type can define its own definition of information to dump.
>> + *
>> + * @param bus
>> + *     Handle for bus, device from which are to be dumped.
>> + * @param f
>> + *     Handle to output device or file.
>> + * @return void
>> + */
>> +typedef void (* bus_dump_t)(struct rte_bus *bus, FILE *f);
>> +
>> +/**
>> + * Search for a specific device in device list of the bus
>> + * This would rely on the bus specific addressing. Each implementation would
>> + * extract its specific device type and perform address compare.
>> + *
>> + * @param dev
>> + *     device handle to search for.
>> + * @return
>> + *     rte_device handle for matched device, or NULL
>> + */
>> +typedef struct rte_device * (* bus_device_get_t)(struct rte_device *dev);
>
> From my experience it is better to delegate this to a helper:
>
> int bus_for_each_dev(struct rte_bus *bus, struct rte_device *start,
> int (*match)(struct rte_device *dev, void *data), void *data);
>
> Based on that you can easily implement all kinds of functions like
> bus_find_dev(), bus_find_dev_by_name(), bus_dump(), ...

Interesting idea. Thanks. I will have a look on this.

>
>> +
>> +struct rte_bus {
>> +       TAILQ_ENTRY(rte_bus) next;   /**< Next bus object in linked list */
>> +       struct rte_driver_list driver_list; /**< List of all drivers of bus */
>
> TAILQ_HEAD?

Yes. That should be TAILQ_HEAD. I just reused the definition create in 
rte_dev.h using TAILQ_HEAD. I will change this.

>
>> +       struct rte_device_list device_list; /**< List of all devices on bus */
>
> TAILQ_HEAD?

Same as above. I will change this.

>
>> +       const char *name;            /**< Name of the bus */
>> +       /* Mandatory hooks */
>> +       bus_scan_t *scan;            /**< Hook for scanning for devices */
>> +       bus_match_t *match;          /**< Hook for matching device & driver */
>> +       /* Optional hooks */
>> +       bus_dump_t *dump_dev;        /**< Hook for dumping devices on bus */
>> +       bus_device_get_t *find_dev;  /**< Search for a device on bus */
>> +};
>> +
>> +/** @internal
>> + * Add a device to a bus.
>> + *
>> + * @param bus
>> + *     Bus on which device is to be added
>> + * @param dev
>> + *     Device handle
>> + * @return
>> + *     None
>> + */
>> +void
>> +rte_eal_bus_add_device(struct rte_bus *bus, struct rte_device *dev);
>
> Why do we need this? From my understanding the rte_bus->scan() is
> adding the devices to the rte_bus->device_list.

Plugging a device *after* scan has been completed. Hotplugging.
Specially for vdev, this would be required, in my opinion.

>
>> +/** @internal
>> + * Remove a device from its bus.
>> + *
>> + * @param dev
>> + *     Device handle to remove
>> + * @return
>> + *     None
>> + */
>> +void
>> +rte_eal_bus_remove_device(struct rte_device *dev);
>> +
>> +/** @internal
>> + * Associate a driver with a bus.
>> + *
>> + * @param bus
>> + *     Bus on which driver is to be added
>> + * @param dev
>> + *     Driver handle
>> + * @return
>> + *     None
>> + */
>> +void
>> +rte_eal_bus_add_driver(struct rte_bus *bus, struct rte_driver *drv);
>> +
>
> What happens if a driver is added at runtime to a bus? Does that immediately
> trigger a match & probe of unclaimed devices?

My take:
  - scan the bus for any newly plugged devices. Might be required in 
case a device is logical entity represented by multiple entries in sysfs.
  -- add only those which are not already added - maybe address/id match
  - Trigger driver probe for all devices which don't have a driver 
assigned to them (unclaimed, as you stated).

(This is part of my further changes - I think I forgot to put them as 
note in cover letter).

>
>> +/** @internal
>> + * Disassociate a driver from its bus.
>> + *
>> + * @param dev
>> + *     Driver handle to remove
>> + * @return
>> + *     None
>> + */
>> +void
>> +rte_eal_bus_remove_driver(struct rte_driver *drv);
>> +
>> +/**
>> + * Register a Bus handler.
>> + *
>> + * @param driver
>> + *   A pointer to a rte_bus structure describing the bus
>> + *   to be registered.
>> + */
>> +void rte_eal_bus_register(struct rte_bus *bus);
>> +
>> +/**
>> + * Unregister a Bus handler.
>> + *
>> + * @param driver
>> + *   A pointer to a rte_bus structure describing the bus
>> + *   to be unregistered.
>> + */
>> +void rte_eal_bus_unregister(struct rte_bus *bus);
>> +
>> +/**
>> + * Obtain handle for bus given its name.
>> + *
>> + * @param bus_name
>> + *     Name of the bus handle to search
>> + * @return
>> + *     Pointer to Bus object if name matches any registered bus object
>> + *     NULL, if no matching bus found
>> + */
>> +struct rte_bus * rte_eal_get_bus(const char *bus_name);
>> +
>> +/**
>> + * Register a device driver.
>> + *
>> + * @param driver
>> + *   A pointer to a rte_dev structure describing the driver
>> + *   to be registered.
>> + */
>> +void rte_eal_driver_register(struct rte_driver *driver);
>> +
>> +/**
>> + * Unregister a device driver.
>> + *
>> + * @param driver
>> + *   A pointer to a rte_dev structure describing the driver
>> + *   to be unregistered.
>> + */
>> +void rte_eal_driver_unregister(struct rte_driver *driver);
>> +
>> +/** Helper for Bus registration */
>> +#define RTE_PMD_REGISTER_BUS(nm, bus) \
>> +RTE_INIT(businitfn_ ##nm); \
>> +static void businitfn_ ##nm(void) \
>> +{\
>> +       (bus).name = RTE_STR(nm);\
>> +       rte_eal_bus_register(&bus); \
>> +}
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif
>> +
>> +#endif /* _RTE_BUS_H */
>> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
>> index 8840380..b08bab5 100644
>> --- a/lib/librte_eal/common/include/rte_dev.h
>> +++ b/lib/librte_eal/common/include/rte_dev.h
>> @@ -116,12 +116,14 @@ TAILQ_HEAD(rte_device_list, rte_device);
>>
>>  /* Forward declaration */
>>  struct rte_driver;
>> +struct rte_bus;
>>
>>  /**
>>   * A structure describing a generic device.
>>   */
>>  struct rte_device {
>>         TAILQ_ENTRY(rte_device) next; /**< Next device */
>> +       struct rte_bus *bus;          /**< Bus on which device is placed */
>>         struct rte_driver *driver;    /**< Associated driver */
>>         int numa_node;                /**< NUMA node connection */
>>         struct rte_devargs *devargs;  /**< Device user arguments */
>> @@ -144,31 +146,29 @@ void rte_eal_device_insert(struct rte_device *dev);
>>  void rte_eal_device_remove(struct rte_device *dev);
>>
>>  /**
>> - * A structure describing a device driver.
>> + * @internal
>> + * TODO
>>   */
>> -struct rte_driver {
>> -       TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
>> -       const char *name;                   /**< Driver name. */
>> -       const char *alias;              /**< Driver alias. */
>> -};
>> +typedef int (*driver_init_t)(struct rte_device *eth_dev);
>>
>>  /**
>> - * Register a device driver.
>> - *
>> - * @param driver
>> - *   A pointer to a rte_dev structure describing the driver
>> - *   to be registered.
>> + * @internal
>> + * TODO
>>   */
>> -void rte_eal_driver_register(struct rte_driver *driver);
>> +typedef int (*driver_uninit_t)(struct rte_device *eth_dev);
>>
>>  /**
>> - * Unregister a device driver.
>> - *
>> - * @param driver
>> - *   A pointer to a rte_dev structure describing the driver
>> - *   to be unregistered.
>> + * A structure describing a device driver.
>>   */
>> -void rte_eal_driver_unregister(struct rte_driver *driver);
>> +struct rte_driver {
>> +       TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
>> +       struct rte_bus *bus;           /**< Bus which drivers services */
>
> I think this should be TAILQ_ENTRY instead.

rte_bus
       `-> device_list-. <- TAILQ_HEAD (rte_device)
                       |
                 rte_pci_device:TAILQ_ENTRY(rte_device)

rte_device just references the bus it belong to.

Am I missing something?

>
>> +       const char *name;              /**< Driver name. */
>> +       const char *alias;             /**< Driver alias. */
>> +       driver_init_t *init;           /**< Driver initialization */
>> +       driver_uninit_t *uninit;       /**< Driver uninitialization */
>
> Shouldn't this be probe() and remove()?

rte_xxx_driver already includes probe and remove.
Mandate for init is to allocate the ethernet or cryptodev (or some other 
functional unit). Whereas, probe is responsible for driver specific 
intialization - PCI specific, in case of PCI driver.

Initially I had thought of moving rte_pci_driver->probe() to rte_driver. 
But, I couldn't understand where would functional device initialization 
exist. It cannot be rte_xxx_driver. It should be generic place.

Such functional device initialization is in path of the probe function, 
but so does driver specific information. If we remove 
rte_pci_driver->probe and move to rte_driver->probe(), it would only 
mean rte_driver calling a hook within the PCI driver specific domain.

>
>> +       unsigned int dev_private_size; /**< Size of device private data ??*/
>
> I don't think that dev_private_size is really required at this level.
> First of all this is related to the rte_eth_dev structure and
> therefore it really depends on the driver_init_t (aka probe()) if it
> is actually allocating an rte_eth_dev or not. Anyway that is up to the
> drivers probe() function.

I moved it based on input through ML (or IRC, I don't remember).
My understanding: if this is common for all rte_xxx_driver instances 
(for allocating their ethernet/crypto structure), it would actually make 
sense to keep at this level so that init() can use it for allocating 
necessary space before handling over the rte_xxx_driver.

But again, I am OK keeping it in the underlying layer - as you have 
rightly pointed out, it would only be used at rte_xxx_driver layer.

>
>> +};
>>
>>  /**
>>   * Initalize all the registered drivers in this process
>> --
>> 2.7.4
>>
>

-
Shreyansh

^ permalink raw reply

* Re: [RFC PATCH 6/6] eal: removing eth_driver
From: Jan Blunck @ 2016-11-17 12:53 UTC (permalink / raw)
  To: Shreyansh Jain; +Cc: David Marchand, dev
In-Reply-To: <1479360605-20558-7-git-send-email-shreyansh.jain@nxp.com>

On Thu, Nov 17, 2016 at 6:30 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> This patch demonstrates how eth_driver can be replaced with appropriate
> changes for rte_xxx_driver from the PMD itself. It uses ixgbe_ethernet as
> an example.
>
> A large set of changes exists in the rte_ethdev.c - primarily because too
> much PCI centric code (names, assumption of rte_pci_device) still exists
> in it. Most, except symbol naming, has been changed in this patch.
>
> This proposes that:
>  - PMD would declare the rte_xxx_driver. In case of ixgbe, it would be
>    rte_pci_driver.
>  - Probe and remove continue to exists in rte_pci_driver. But, the
>    rte_driver has new hooks for init and uninit. The rationale is that
>    once a ethernet or cryto device is created, the rte_driver->init would
>    be responsible for initializing the device.
>    -- Eth_dev -> rte_driver -> rte_pci_driver
>                           |    `-> probe/remove
>                           `--> init/uninit

Hmm, from my perspective this moves struct rte_driver a step closer to
struct rte_eth_dev instead of decoupling them. It is up to the
rte_driver->probe if it wants to allocate a struct rte_eth_dev,
rte_crypto_dev or the famous rte_foo_dev.

Instead of explicitly modelling rte_eth_dev specifics like init, unit
or dev_private_size I think we should delegate this to the
rte_driver->probe instead. Most of what is in rte_eth_dev_pci_probe()
today is anyway a rte_eth_dev_allocate_priv() anyway. I already have
some patches in this area in my patch stack.


>  - necessary changes in the rte_eth_dev have also been done so that it
>    refers to the rte_device and rte_driver rather than rte_xxx_*. This
>    would imply, ethernet device is 'linked' to a rte_device/rte_driver
>    which in turn is a rte_xxx_device/rte_xxx_driver type.
>    - for all operations related to extraction relvant xxx type,
>      container_of would have to be used.
>
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c | 49 +++++++++++++++++++++-------------------
>  lib/librte_ether/rte_ethdev.c    | 36 +++++++++++++++++------------
>  lib/librte_ether/rte_ethdev.h    |  6 ++---
>  3 files changed, 51 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index edc9b22..acead31 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -1419,7 +1419,7 @@ eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev)
>                 return 0;
>         }
>
> -       pci_dev = eth_dev->pci_dev;
> +       pci_dev = container_of(eth_dev->device, struct rte_pci_device, device);
>
>         rte_eth_copy_pci_info(eth_dev, pci_dev);
>
> @@ -1532,7 +1532,9 @@ static int
>  eth_ixgbevf_dev_uninit(struct rte_eth_dev *eth_dev)
>  {
>         struct ixgbe_hw *hw;
> -       struct rte_pci_device *pci_dev = eth_dev->pci_dev;
> +       struct rte_pci_device *pci_dev;
> +
> +       pci_dev = container_of(eth_dev->device, struct rte_pci_device, device);
>
>         PMD_INIT_FUNC_TRACE();
>
> @@ -1562,32 +1564,33 @@ eth_ixgbevf_dev_uninit(struct rte_eth_dev *eth_dev)
>         return 0;
>  }
>
> -static struct eth_driver rte_ixgbe_pmd = {
> -       .pci_drv = {
> -               .id_table = pci_id_ixgbe_map,
> -               .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
> -                       RTE_PCI_DRV_DETACHABLE,
> -               .probe = rte_eth_dev_pci_probe,
> -               .remove = rte_eth_dev_pci_remove,
> +static struct rte_pci_driver rte_ixgbe_pci_driver = {
> +       .id_table = pci_id_ixgbe_map,
> +       .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
> +                    RTE_PCI_DRV_DETACHABLE,
> +       .probe = rte_eth_dev_pci_probe,
> +       .remove = rte_eth_dev_pci_remove,
> +       .driver = {
> +               .driver_init_t= eth_ixgbe_dev_init,
> +               .driver_uninit_t= eth_ixgbe_dev_uninit,
> +               .dev_private_size = sizeof(struct ixgbe_adapter),
>         },
> -       .eth_dev_init = eth_ixgbe_dev_init,
> -       .eth_dev_uninit = eth_ixgbe_dev_uninit,
> -       .dev_private_size = sizeof(struct ixgbe_adapter),
>  };
>
>  /*
>   * virtual function driver struct
>   */
> -static struct eth_driver rte_ixgbevf_pmd = {
> -       .pci_drv = {
> -               .id_table = pci_id_ixgbevf_map,
> -               .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_DETACHABLE,
> -               .probe = rte_eth_dev_pci_probe,
> -               .remove = rte_eth_dev_pci_remove,
> +static struct rte_pci_driver rte_ixgbevf_pci_driver = {
> +       .id_table = pci_id_ixgbevf_map,
> +       .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_DETACHABLE,
> +       .probe = rte_eth_dev_pci_probe,
> +       .remove = rte_eth_dev_pci_remove,
> +       .driver = {
> +               /* rte_driver hooks */
> +               .init = eth_ixgbevf_dev_init,
> +               .uninit = eth_ixgbevf_dev_uninit,
> +               .dev_private_size = sizeof(struct ixgbe_adapter),
>         },
> -       .eth_dev_init = eth_ixgbevf_dev_init,
> -       .eth_dev_uninit = eth_ixgbevf_dev_uninit,
> -       .dev_private_size = sizeof(struct ixgbe_adapter),
>  };
>
>  static int
> @@ -7592,7 +7595,7 @@ ixgbevf_dev_interrupt_handler(__rte_unused struct rte_intr_handle *handle,
>         ixgbevf_dev_interrupt_action(dev);
>  }
>
> -RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd.pci_drv);
> +RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pci_driver);
>  RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
> -RTE_PMD_REGISTER_PCI(net_ixgbe_vf, rte_ixgbevf_pmd.pci_drv);
> +RTE_PMD_REGISTER_PCI(net_ixgbe_vf, rte_ixgbevf_pci_driver);
>  RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe_vf, pci_id_ixgbevf_map);
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index fde8112..3535ff4 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -235,13 +235,13 @@ int
>  rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>                       struct rte_pci_device *pci_dev)
>  {
> -       struct eth_driver    *eth_drv;
> +       struct rte_driver *drv;
>         struct rte_eth_dev *eth_dev;
>         char ethdev_name[RTE_ETH_NAME_MAX_LEN];
>
>         int diag;
>
> -       eth_drv = (struct eth_driver *)pci_drv;
> +       drv = pci_drv->driver;
>
>         rte_eal_pci_device_name(&pci_dev->addr, ethdev_name,
>                         sizeof(ethdev_name));
> @@ -252,13 +252,13 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>
>         if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>                 eth_dev->data->dev_private = rte_zmalloc("ethdev private structure",
> -                                 eth_drv->dev_private_size,
> +                                 drv->dev_private_size,
>                                   RTE_CACHE_LINE_SIZE);
>                 if (eth_dev->data->dev_private == NULL)
>                         rte_panic("Cannot allocate memzone for private port data\n");
>         }
> -       eth_dev->pci_dev = pci_dev;
> -       eth_dev->driver = eth_drv;
> +       eth_dev->device = pci_dev->device;
> +       eth_dev->driver = drv;
>         eth_dev->data->rx_mbuf_alloc_failed = 0;
>
>         /* init user callbacks */
> @@ -270,7 +270,7 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>         eth_dev->data->mtu = ETHER_MTU;
>
>         /* Invoke PMD device initialization function */
> -       diag = (*eth_drv->eth_dev_init)(eth_dev);
> +       diag = (*drv->init)(eth_dev);
>         if (diag == 0)
>                 return 0;
>
> @@ -287,7 +287,7 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>  int
>  rte_eth_dev_pci_remove(struct rte_pci_device *pci_dev)
>  {
> -       const struct eth_driver *eth_drv;
> +       const struct rte_driver *drv;
>         struct rte_eth_dev *eth_dev;
>         char ethdev_name[RTE_ETH_NAME_MAX_LEN];
>         int ret;
> @@ -302,11 +302,11 @@ rte_eth_dev_pci_remove(struct rte_pci_device *pci_dev)
>         if (eth_dev == NULL)
>                 return -ENODEV;
>
> -       eth_drv = (const struct eth_driver *)pci_dev->driver;
> +       drv = pci_dev->driver;
>
>         /* Invoke PMD device uninit function */
> -       if (*eth_drv->eth_dev_uninit) {
> -               ret = (*eth_drv->eth_dev_uninit)(eth_dev);
> +       if (*drv->uninit) {
> +               ret = (*drv->uninit)(eth_dev);
>                 if (ret)
>                         return ret;
>         }
> @@ -317,7 +317,7 @@ rte_eth_dev_pci_remove(struct rte_pci_device *pci_dev)
>         if (rte_eal_process_type() == RTE_PROC_PRIMARY)
>                 rte_free(eth_dev->data->dev_private);
>
> -       eth_dev->pci_dev = NULL;
> +       eth_dev->device = NULL;
>         eth_dev->driver = NULL;
>         eth_dev->data = NULL;
>
> @@ -1556,7 +1556,7 @@ rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info)
>
>         RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
>         (*dev->dev_ops->dev_infos_get)(dev, dev_info);
> -       dev_info->pci_dev = dev->pci_dev;
> +       dev_info->device = dev->device;
>         dev_info->driver_name = dev->data->drv_name;
>         dev_info->nb_rx_queues = dev->data->nb_rx_queues;
>         dev_info->nb_tx_queues = dev->data->nb_tx_queues;
> @@ -2537,6 +2537,7 @@ rte_eth_dev_rx_intr_ctl(uint8_t port_id, int epfd, int op, void *data)
>  {
>         uint32_t vec;
>         struct rte_eth_dev *dev;
> +       struct rte_pci_device *pci_dev;
>         struct rte_intr_handle *intr_handle;
>         uint16_t qid;
>         int rc;
> @@ -2544,6 +2545,10 @@ rte_eth_dev_rx_intr_ctl(uint8_t port_id, int epfd, int op, void *data)
>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>
>         dev = &rte_eth_devices[port_id];
> +       /* TODO intr_handle is currently in rte_pci_device;
> +        * Below is incorrect until that time
> +        */
> +       pci_dev = container_of(dev->device, struct rte_pci_device, device);
>         intr_handle = &dev->pci_dev->intr_handle;
>         if (!intr_handle->intr_vec) {
>                 RTE_PMD_DEBUG_TRACE("RX Intr vector unset\n");
> @@ -2572,7 +2577,7 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
>         const struct rte_memzone *mz;
>
>         snprintf(z_name, sizeof(z_name), "%s_%s_%d_%d",
> -                dev->driver->pci_drv.driver.name, ring_name,
> +                dev->driver->name, ring_name,
>                  dev->data->port_id, queue_id);
>
>         mz = rte_memzone_lookup(z_name);
> @@ -2593,6 +2598,7 @@ rte_eth_dev_rx_intr_ctl_q(uint8_t port_id, uint16_t queue_id,
>  {
>         uint32_t vec;
>         struct rte_eth_dev *dev;
> +       struct rte_pci_device *pci_dev;
>         struct rte_intr_handle *intr_handle;
>         int rc;
>
> @@ -2604,7 +2610,9 @@ rte_eth_dev_rx_intr_ctl_q(uint8_t port_id, uint16_t queue_id,
>                 return -EINVAL;
>         }
>
> -       intr_handle = &dev->pci_dev->intr_handle;
> +       /* TODO; Until intr_handle is available in rte_device, below is incorrect */
> +       pci_dev = container_of(dev->device, struct rte_pci_device, device);
> +       intr_handle = &pci_dev->intr_handle;
>         if (!intr_handle->intr_vec) {
>                 RTE_PMD_DEBUG_TRACE("RX Intr vector unset\n");
>                 return -EPERM;
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 38641e8..2b1d826 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -876,7 +876,7 @@ struct rte_eth_conf {
>   * Ethernet device information
>   */
>  struct rte_eth_dev_info {
> -       struct rte_pci_device *pci_dev; /**< Device PCI information. */
> +       struct rte_device *device; /**< Device PCI information. */

We already the situation that virtual devices don't set the pci_dev
field. I wonder if it really makes sense to replace it with a struct
rte_device because that is not adding a lot of value (only numa_node).
If we break ABI we might want to add numa_node and dev_flags as
suggested by Stephen Hemminger already. If we choose to not break ABI
we can delegate the population of the pci_dev field to dev_infos_get.
I already have that patch in my patch stack too.

The problem with rte_eth_dev_info is that it doesn't support
extensions. Maybe its time to add rte_eth_dev_info_ex() ... or
rte_eth_dev_xinfo() if you don't like the IB verbs API.


>         const char *driver_name; /**< Device Driver name. */
>         unsigned int if_index; /**< Index to bound host interface, or 0 if none.
>                 Use if_indextoname() to translate into an interface name. */
> @@ -1623,9 +1623,9 @@ struct rte_eth_dev {
>         eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function. */
>         eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function. */
>         struct rte_eth_dev_data *data;  /**< Pointer to device data */
> -       const struct eth_driver *driver;/**< Driver for this device */
> +       const struct rte_driver *driver;/**< Driver for this device */
>         const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
> -       struct rte_pci_device *pci_dev; /**< PCI info. supplied by probing */
> +       struct rte_device *device; /**< Device instance */
>         /** User application callbacks for NIC interrupts */
>         struct rte_eth_dev_cb_list link_intr_cbs;
>         /**
> --
> 2.7.4
>

^ permalink raw reply

* Re: [RFC PATCH 1/6] eal: define container macro
From: Jan Blunck @ 2016-11-17 12:06 UTC (permalink / raw)
  To: Shreyansh Jain; +Cc: David Marchand, dev, Jan Viktorin
In-Reply-To: <1479360605-20558-2-git-send-email-shreyansh.jain@nxp.com>

On Thu, Nov 17, 2016 at 6:30 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> From: Jan Viktorin <viktorin@rehivetech.com>
>
> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---
>  lib/librte_eal/common/include/rte_common.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
> index db5ac91..8152bd9 100644
> --- a/lib/librte_eal/common/include/rte_common.h
> +++ b/lib/librte_eal/common/include/rte_common.h
> @@ -331,6 +331,24 @@ rte_bsf32(uint32_t v)
>  #define offsetof(TYPE, MEMBER)  __builtin_offsetof (TYPE, MEMBER)
>  #endif
>
> +/**
> + * Return pointer to the wrapping struct instance.
> + * Example:
> + *
> + *  struct wrapper {
> + *      ...
> + *      struct child c;
> + *      ...
> + *  };
> + *
> + *  struct child *x = obtain(...);
> + *  struct wrapper *w = container_of(x, struct wrapper, c);
> + */
> +#ifndef container_of
> +#define container_of(p, type, member) \
> +       ((type *) (((char *) (p)) - offsetof(type, member)))

Are there any reasons why you choose to implement this in a non-type
safe way? Catching obvious bugs at compile time is in the interest of
us and our users from my point of view.


> +#endif
> +
>  #define _RTE_STR(x) #x
>  /** Take a macro value and get a string version of it */
>  #define RTE_STR(x) _RTE_STR(x)
> --
> 2.7.4
>

^ permalink raw reply

* Re: [RFC PATCH 0/6] Restructure EAL device model for bus support
From: Jan Blunck @ 2016-11-17 11:55 UTC (permalink / raw)
  To: Shreyansh Jain; +Cc: David Marchand, dev
In-Reply-To: <1479360605-20558-1-git-send-email-shreyansh.jain@nxp.com>

On Thu, Nov 17, 2016 at 6:29 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> DPDK has been inherently a PCI inclined framework. Because of this, the
> design of device tree (or list) within DPDK is also PCI inclined. A non-PCI
> device doesn't have a way of being expressed without using hooks started from
> EAL to PMD.
>
> With this cover letter, some patches are presented which try to break this
> strict linkage of EAL with PCI devices. Aim is to generalize the device
> hierarchy on the lines of how Linux handles it:
>
>         device A1
>           |
>   +==.===='==============.============+ Bus A.
>      |                    `--> driver A11     \
>   device A2                `-> driver A12      \______
>                                                 |CPU |
>                                                 /`````
>         device A1                              /
>           |                                   /
>   +==.===='==============.============+ Bus A`
>      |                    `--> driver B11
>   device A2                `-> driver B12
>
> Simply put:
>  - a bus is connect to CPU (or core)
>  - devices are conneted to Bus
>  - drivers are running instances which manage one or more devices
>  - bus is responsible for identifying devices (and interrupt propogation)
>  - driver is responsible for initializing the device
>
> (*Reusing text from email [1])
> In context of DPDK EAL:
>  - a generic bus (not a driver, not a device). I don't know how to categorize
>    a bus. It is certainly not a device, and then handler for a bus (physical)
>    can be considered a 'bus driver'. So, just 'rte_bus'.
>  - there is a bus for each physical implementation (or virtual). So, a rte_bus
>    Object for PCI, VDEV, ABC, DEF and so on.
>  - Buses are registered just like a PMD - RTE_PMD_BUS_REGISTER()
>  - Each registered bus is part of a doubly list.
>    -- Each device inherits rte_bus
>    -- Each driver inherits rte_bus
>    -- Device and Drivers lists are part of rte_bus
>  - eth_driver is no more required - it was just a holder for PMDs to register
>    themselves. It can be replaced with rte_xxx_driver and corresponding init/
>    uninit moved to rte_driver
>  - rte_eth_dev modified to disassociate itself from rte_pci_device and connect
>    to generic rte_device
>
> Once again, improvising from [1]:
>
>                                   __ rte_bus_list
>                                  /
>                      +----------'---+
>                      |rte_bus       |
>                      | driver_list------> List of rte_bus specific
>                      | device_list----    devices
>                      | scan         | `-> List of rte_bus associated
>                      | match        |     drivers
>                      | dump         |
>                      | ..some refcnt| (#)
>                      +--|------|----+
>               _________/        \_________
>     +--------/----+                     +-\---------------+
>     |rte_device   |                     |rte_driver       |
>     | rte_bus     |                     | rte_bus         |
>     | rte_driver  |(#)                  | init            |
>     |             |                     | uninit          |
>     |  devargs    |                     | dev_private_size|
>     +---||--------+                     | drv_flags       |(#)
>         ||                              | intr_handle(2*) |(#)
>         | \                             +----------\\\----+
>         |  \_____________                           \\\
>         |                \                          |||
>  +------|---------+ +----|----------+               |||
>  |rte_pci_device  | |rte_xxx_device | (4*)          |||
>  | PCI specific   | | xxx device    |               |||
>  | info (mem,)    | | specific fns  |              / | \
>  +----------------+ +---------------+             /  |  \
>                             _____________________/  /    \
>                            /                    ___/      \
>             +-------------'--+    +------------'---+    +--'------------+
>             |rte_pci_driver  |    |rte_vdev_driver |    |rte_xxx_driver |
>             | PCI id table,  |    | <probably,     |    | ....          |
>             | other driver   |    |  nothing>      |    +---------------+
>             | data           |    +----------------+
>             +----------------+
>
>     (1*) Problem is that probe functions have different arguments. So,
>          generalizing them might be some rework in the respective device
>          layers
>     (2*) Interrupt handling for each driver type might be different. I am not
>          sure how to generalize that either. This is grey area for me.
>     (3*) Probably exposing a bitmask for device capabilities. Nothing similar
>          exists now to relate it. Don't know if that is useful. Allowing
>          applications to question a device about what it supports and what it
>          doesn't - making it more flexible at application layer (but more code
>          as well.)
>     (4*) Even vdev would be an instantiated as a device. It is not being done
>          currently.
>     (#)  Items which are still pending
>
> With this cover letter, some patches have been posted. These are _only_ for
> discussion purpose. They are not complete and neither compilable.
> All the patches, except 0001, have sufficient context about what the changes
> are and rationale for same. Obviously, code is best answer.
>
> === Patch description: ===
>
> Patch 0001: Introduce container_of macro. Originally a patch from Jan.
>
> Patch 0002: introduce changes to rte_device/rte_driver for rte_bus, and
>             rte_bus definition itself.
>
> Patch 0003: Add a new layer for 'bus driver' with linuxapp PCI as an example
>
> Patch 0004: Changes with respect to rte_bus APIs and impact on eal_common_pci

>From my point of view it is beneficial to keep the PCI support in one
place. Moving the match() and scan()
to the drivers directory doesn't seem to be technically required but
it makes the code harder to read and understand.


> Patch 0005: Change to rte_eal_init (of linuxapp only, for now) for supporting
>             bus->scan. Probe is still being done old way, but in a new wrapper
>
> Patch 0006: eth_driver removal and corresponding changes in ixgbe_ethdev, as
>             an example. Includes changes to rte_ethdev to remove most possible
>             PCI references. But, work still remains.

Making rte_ethdev independent from PCI device is not directly related
to the rest of the patches that add bus support. I think it makes
sense to handle this separately because of the impact of refactoring
rte_ethdev.


>
> === Pending Items/Questions: ===
>
>  - Interrupt and notification handling. How to allow drivers to be notified
>    of presence/plugging of a device.
>  - Placement of bus driver/handling code. librte_bus, bus/, drivers/bus?
>  -- Also from a pespective of a external library and whether symbols would be
>     available in that.
>  -- and secondary processes
>  - VDEV bus is missing from current set.
>  - Locking of list for supporting hotplugging. Or, at the least safe add/
>    remove
>  - PMDINFOGEN support or lack of it.
>  - Is there ever a case where rte_eth_dev needs to be resolved from
>    rte_pci_device? I couldn't find any such use and neither a use-case for it.
>  - There should be a way for Bus APIs to return a generic list handle so that
>    EAL doesn't need to bother about bus->driver_list like dereferencing. This
>    is untidy as well as less portable (in terms of code movement, not arch).
>  - Are more helper hooks required for a bus?
>  -- I can think of scan, match, dump, find, plug (device), unplug (device),
>     associate (driver), disassociate (driver). But, most of the work is
>     already being done by lower instances (rte_device/driver etc).
>
> Further:
>  - In next few days I will make all necessary changes on the lines mentioned
>    in the patches. This would include changing the drivers/* and librte_eal/*
>  - As an when review comments float in and agreement reached, I will keep
>    changing the model
>  - There are grey areas like interrupt, notification, locking of bus/list
>    which require more discussion. I will try and post a rfc for those as well
>    or if someone can help me on those - great

As already hinted on IRC I'm taking a look at the interrupt usage of ethdev.

>  - Change would include PCI bus and VDEV bus handling. A new bus (NXP's FSLMC)
>    would also be layered over this series to verify the model of 'bus
>    registration'. This is also part of 17.02 roadmap.
>
> [1] http://dpdk.org/ml/archives/dev/2016-November/050186.html
>
> Jan Viktorin (1):
>   eal: define container macro
>
> Shreyansh Jain (5):
>   eal: introduce bus-device-driver structure
>   bus: add bus driver layer
>   eal/common: handle bus abstraction for device/driver objects
>   eal: supporting bus model in init process
>   eal: removing eth_driver
>
>  bus/Makefile                               |  36 +++
>  bus/pci/Makefile                           |  37 +++
>  bus/pci/linuxapp/pci_bus.c                 | 418 +++++++++++++++++++++++++++++
>  bus/pci/linuxapp/pci_bus.h                 |  55 ++++
>  drivers/net/ixgbe/ixgbe_ethdev.c           |  49 ++--
>  lib/librte_eal/common/eal_common_bus.c     | 188 +++++++++++++
>  lib/librte_eal/common/eal_common_dev.c     |  31 ++-
>  lib/librte_eal/common/eal_common_pci.c     | 226 +++++++++-------
>  lib/librte_eal/common/include/rte_bus.h    | 243 +++++++++++++++++
>  lib/librte_eal/common/include/rte_common.h |  18 ++
>  lib/librte_eal/common/include/rte_dev.h    |  36 +--
>  lib/librte_eal/common/include/rte_pci.h    |  11 +-
>  lib/librte_eal/linuxapp/eal/Makefile       |   1 +
>  lib/librte_eal/linuxapp/eal/eal.c          |  51 +++-
>  lib/librte_eal/linuxapp/eal/eal_pci.c      | 298 --------------------
>  lib/librte_ether/rte_ethdev.c              |  36 ++-
>  lib/librte_ether/rte_ethdev.h              |   6 +-
>  17 files changed, 1262 insertions(+), 478 deletions(-)
>  create mode 100644 bus/Makefile
>  create mode 100644 bus/pci/Makefile
>  create mode 100644 bus/pci/linuxapp/pci_bus.c
>  create mode 100644 bus/pci/linuxapp/pci_bus.h
>  create mode 100644 lib/librte_eal/common/eal_common_bus.c
>  create mode 100644 lib/librte_eal/common/include/rte_bus.h
>
> --
> 2.7.4
>

^ permalink raw reply

* Re: [RFC PATCH 2/6] eal: introduce bus-device-driver structure
From: Jan Blunck @ 2016-11-17 11:19 UTC (permalink / raw)
  To: Shreyansh Jain; +Cc: David Marchand, dev
In-Reply-To: <1479360605-20558-3-git-send-email-shreyansh.jain@nxp.com>

On Thu, Nov 17, 2016 at 6:30 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> A device is connected to a bus and services by a driver associated with
> the bus. It is responsibility of the bus to identify the devices (scan)
> and then assign each device to a matching driver.
>
> A PMD would allocate a rte_xxx_driver and rte_xxx_device.
> rte_xxx_driver has rte_driver and rte_bus embedded. Similarly, rte_xxx_device
> has rte_device and rte_bus embedded.

I don't think so: the rte_xxx_device embeds the generic rte_device and
references a the rte_bus
that it is attached to.

> When a ethernet or crypto device (rte_eth_dev, rte_cryptodev) is allocated,
> it contains a reference of rte_device and rte_driver.
> Each ethernet device implementation would use container_of for finding the
> enclosing structure of rte_xxx_*.
>
>                             +-------------------+
>  +--------------+           |rte_pci_device     |
>  |rte_eth_dev   |           |+-----------------+|
>  |+------------+|   .-------->rte_device       ||
>  ||rte_device*-----'        |+-----------------+|
>  |+------------+|           ||rte_bus          ||
>  |              |           |+-----------------+|
>  /              /           +-------------------+
>
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---
>  lib/librte_eal/common/include/rte_bus.h | 243 ++++++++++++++++++++++++++++++++
>  lib/librte_eal/common/include/rte_dev.h |  36 ++---
>  2 files changed, 261 insertions(+), 18 deletions(-)
>  create mode 100644 lib/librte_eal/common/include/rte_bus.h
>
> diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
> new file mode 100644
> index 0000000..dc3aeb8
> --- /dev/null
> +++ b/lib/librte_eal/common/include/rte_bus.h
> @@ -0,0 +1,243 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2016 NXP
> + *   All rights reserved.
> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of NXP nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef _RTE_BUS_H_
> +#define _RTE_BUS_H_
> +
> +/**
> + * @file
> + *
> + * RTE PMD Bus Abstraction interfaces
> + *
> + * This file exposes APIs and Interfaces for Bus Abstraction over the devices
> + * drivers in EAL.
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <stdio.h>
> +#include <sys/queue.h>
> +
> +#include <rte_log.h>
> +#include <rte_dev.h>
> +
> +
> +/** Double linked list of buses */
> +TAILQ_HEAD(rte_bus_list, rte_bus);
> +
> +/**
> + * Bus specific scan for devices attached on the bus.
> + * For each bus object, the scan would be reponsible for finding devices and
> + * adding them to its private device list.
> + *
> + * Successful detection of a device results in rte_device object which is
> + * embedded within the respective device type (rte_pci_device, for example).
> + * Thereafter, PCI specific bus would need to perform
> + * container_of(rte_pci_device) to obtain PCI device object.
> + *
> + * Scan failure of a bus is not treated as exit criteria for application. Scan
> + * for all other buses would still continue.
> + *
> + * @param void
> + * @return
> + *     0 for successful scan
> + *     !0 (<0) for unsuccessful scan with error value
> + */
> +typedef int (* bus_scan_t)(void);
> +
> +/**
> + * Bus specific match for devices and drivers which can service them.
> + * For each scanned device, during probe the match would link the devices with
> + * drivers which can service the device.
> + *
> + * It is the work of each bus handler to obtain the specific device object
> + * using container_of (or typecasting, as a less preferred way).
> + *
> + * @param drv
> + *     Driver object attached to the bus
> + * @param dev
> + *     Device object which is being probed.
> + * @return
> + *     0 for successful match
> + *     !0 for unsuccessful match
> + */
> +typedef int (* bus_match_t)(struct rte_driver *drv, struct rte_device *dev);

Do you think this should do match & probe?

I believe it is better to separate this into two functions to match()
and probe(). The
result of matching tells if the driver would want to claim the device
in general. But
probe()ing the device should only happen if the device isn't claimed
by a driver yet and
is not blacklisted.

> +
> +/**
> + * Dump the devices on the bus.
> + * Each bus type can define its own definition of information to dump.
> + *
> + * @param bus
> + *     Handle for bus, device from which are to be dumped.
> + * @param f
> + *     Handle to output device or file.
> + * @return void
> + */
> +typedef void (* bus_dump_t)(struct rte_bus *bus, FILE *f);
> +
> +/**
> + * Search for a specific device in device list of the bus
> + * This would rely on the bus specific addressing. Each implementation would
> + * extract its specific device type and perform address compare.
> + *
> + * @param dev
> + *     device handle to search for.
> + * @return
> + *     rte_device handle for matched device, or NULL
> + */
> +typedef struct rte_device * (* bus_device_get_t)(struct rte_device *dev);

>From my experience it is better to delegate this to a helper:

int bus_for_each_dev(struct rte_bus *bus, struct rte_device *start,
int (*match)(struct rte_device *dev, void *data), void *data);

Based on that you can easily implement all kinds of functions like
bus_find_dev(), bus_find_dev_by_name(), bus_dump(), ...

> +
> +struct rte_bus {
> +       TAILQ_ENTRY(rte_bus) next;   /**< Next bus object in linked list */
> +       struct rte_driver_list driver_list; /**< List of all drivers of bus */

TAILQ_HEAD?

> +       struct rte_device_list device_list; /**< List of all devices on bus */

TAILQ_HEAD?

> +       const char *name;            /**< Name of the bus */
> +       /* Mandatory hooks */
> +       bus_scan_t *scan;            /**< Hook for scanning for devices */
> +       bus_match_t *match;          /**< Hook for matching device & driver */
> +       /* Optional hooks */
> +       bus_dump_t *dump_dev;        /**< Hook for dumping devices on bus */
> +       bus_device_get_t *find_dev;  /**< Search for a device on bus */
> +};
> +
> +/** @internal
> + * Add a device to a bus.
> + *
> + * @param bus
> + *     Bus on which device is to be added
> + * @param dev
> + *     Device handle
> + * @return
> + *     None
> + */
> +void
> +rte_eal_bus_add_device(struct rte_bus *bus, struct rte_device *dev);

Why do we need this? From my understanding the rte_bus->scan() is
adding the devices to the rte_bus->device_list.

> +/** @internal
> + * Remove a device from its bus.
> + *
> + * @param dev
> + *     Device handle to remove
> + * @return
> + *     None
> + */
> +void
> +rte_eal_bus_remove_device(struct rte_device *dev);
> +
> +/** @internal
> + * Associate a driver with a bus.
> + *
> + * @param bus
> + *     Bus on which driver is to be added
> + * @param dev
> + *     Driver handle
> + * @return
> + *     None
> + */
> +void
> +rte_eal_bus_add_driver(struct rte_bus *bus, struct rte_driver *drv);
> +

What happens if a driver is added at runtime to a bus? Does that immediately
trigger a match & probe of unclaimed devices?

> +/** @internal
> + * Disassociate a driver from its bus.
> + *
> + * @param dev
> + *     Driver handle to remove
> + * @return
> + *     None
> + */
> +void
> +rte_eal_bus_remove_driver(struct rte_driver *drv);
> +
> +/**
> + * Register a Bus handler.
> + *
> + * @param driver
> + *   A pointer to a rte_bus structure describing the bus
> + *   to be registered.
> + */
> +void rte_eal_bus_register(struct rte_bus *bus);
> +
> +/**
> + * Unregister a Bus handler.
> + *
> + * @param driver
> + *   A pointer to a rte_bus structure describing the bus
> + *   to be unregistered.
> + */
> +void rte_eal_bus_unregister(struct rte_bus *bus);
> +
> +/**
> + * Obtain handle for bus given its name.
> + *
> + * @param bus_name
> + *     Name of the bus handle to search
> + * @return
> + *     Pointer to Bus object if name matches any registered bus object
> + *     NULL, if no matching bus found
> + */
> +struct rte_bus * rte_eal_get_bus(const char *bus_name);
> +
> +/**
> + * Register a device driver.
> + *
> + * @param driver
> + *   A pointer to a rte_dev structure describing the driver
> + *   to be registered.
> + */
> +void rte_eal_driver_register(struct rte_driver *driver);
> +
> +/**
> + * Unregister a device driver.
> + *
> + * @param driver
> + *   A pointer to a rte_dev structure describing the driver
> + *   to be unregistered.
> + */
> +void rte_eal_driver_unregister(struct rte_driver *driver);
> +
> +/** Helper for Bus registration */
> +#define RTE_PMD_REGISTER_BUS(nm, bus) \
> +RTE_INIT(businitfn_ ##nm); \
> +static void businitfn_ ##nm(void) \
> +{\
> +       (bus).name = RTE_STR(nm);\
> +       rte_eal_bus_register(&bus); \
> +}
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_BUS_H */
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index 8840380..b08bab5 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -116,12 +116,14 @@ TAILQ_HEAD(rte_device_list, rte_device);
>
>  /* Forward declaration */
>  struct rte_driver;
> +struct rte_bus;
>
>  /**
>   * A structure describing a generic device.
>   */
>  struct rte_device {
>         TAILQ_ENTRY(rte_device) next; /**< Next device */
> +       struct rte_bus *bus;          /**< Bus on which device is placed */
>         struct rte_driver *driver;    /**< Associated driver */
>         int numa_node;                /**< NUMA node connection */
>         struct rte_devargs *devargs;  /**< Device user arguments */
> @@ -144,31 +146,29 @@ void rte_eal_device_insert(struct rte_device *dev);
>  void rte_eal_device_remove(struct rte_device *dev);
>
>  /**
> - * A structure describing a device driver.
> + * @internal
> + * TODO
>   */
> -struct rte_driver {
> -       TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
> -       const char *name;                   /**< Driver name. */
> -       const char *alias;              /**< Driver alias. */
> -};
> +typedef int (*driver_init_t)(struct rte_device *eth_dev);
>
>  /**
> - * Register a device driver.
> - *
> - * @param driver
> - *   A pointer to a rte_dev structure describing the driver
> - *   to be registered.
> + * @internal
> + * TODO
>   */
> -void rte_eal_driver_register(struct rte_driver *driver);
> +typedef int (*driver_uninit_t)(struct rte_device *eth_dev);
>
>  /**
> - * Unregister a device driver.
> - *
> - * @param driver
> - *   A pointer to a rte_dev structure describing the driver
> - *   to be unregistered.
> + * A structure describing a device driver.
>   */
> -void rte_eal_driver_unregister(struct rte_driver *driver);
> +struct rte_driver {
> +       TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
> +       struct rte_bus *bus;           /**< Bus which drivers services */

I think this should be TAILQ_ENTRY instead.

> +       const char *name;              /**< Driver name. */
> +       const char *alias;             /**< Driver alias. */
> +       driver_init_t *init;           /**< Driver initialization */
> +       driver_uninit_t *uninit;       /**< Driver uninitialization */

Shouldn't this be probe() and remove()?

> +       unsigned int dev_private_size; /**< Size of device private data ??*/

I don't think that dev_private_size is really required at this level.
First of all this is related to the rte_eth_dev structure and
therefore it really depends on the driver_init_t (aka probe()) if it
is actually allocating an rte_eth_dev or not. Anyway that is up to the
drivers probe() function.

> +};
>
>  /**
>   * Initalize all the registered drivers in this process
> --
> 2.7.4
>

^ permalink raw reply

page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox