[dpdk-dev,v6,06/13] virtio: read virtio_net_config correctly
Commit Message
From: Changchun Ouyang <changchun.ouyang@intel.com>
The old code adjusts the config bytes we want to read depending on
what kind of features we have, but we later cast the entire buf we
read with "struct virtio_net_config", which is obviously wrong.
The right way to go is to read related config bytes when corresponding
feature is set, which is exactly what this patch does.
Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
v6: read mac unconditionally.
---
drivers/net/virtio/virtio_ethdev.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
Comments
2015-10-09 13:46, Yuanhan Liu:
> From: Changchun Ouyang <changchun.ouyang@intel.com>
>
> The old code adjusts the config bytes we want to read depending on
> what kind of features we have, but we later cast the entire buf we
> read with "struct virtio_net_config", which is obviously wrong.
When describing a bug, it is important to explain what is the consequence,
i.e. which use case is failing. If it is only to prepare the new feature,
it is better to clearly state that the bug had no impact until now.
And as usual, the "fix" word in the title and the "Fixes" tag are required.
On Tue, Oct 20, 2015 at 09:07:49AM +0200, Thomas Monjalon wrote:
> 2015-10-09 13:46, Yuanhan Liu:
> > From: Changchun Ouyang <changchun.ouyang@intel.com>
> >
> > The old code adjusts the config bytes we want to read depending on
> > what kind of features we have, but we later cast the entire buf we
> > read with "struct virtio_net_config", which is obviously wrong.
>
> When describing a bug, it is important to explain what is the consequence,
> i.e. which use case is failing. If it is only to prepare the new feature,
> it is better to clearly state that the bug had no impact until now.
>
> And as usual, the "fix" word in the title and the "Fixes" tag are required.
What's the right way supposed to use "Fixes" tag, BTW? Checking the git
hisotry, I saw something like:
Fixes: $commit_hash ($commit_log).
Which basically means it's a regression fix. However, in this case, it's
more than like a bug, but not a regression.
--yliu
2015-10-20 15:23, Yuanhan Liu:
> On Tue, Oct 20, 2015 at 09:07:49AM +0200, Thomas Monjalon wrote:
> > 2015-10-09 13:46, Yuanhan Liu:
> > > From: Changchun Ouyang <changchun.ouyang@intel.com>
> > >
> > > The old code adjusts the config bytes we want to read depending on
> > > what kind of features we have, but we later cast the entire buf we
> > > read with "struct virtio_net_config", which is obviously wrong.
> >
> > When describing a bug, it is important to explain what is the consequence,
> > i.e. which use case is failing. If it is only to prepare the new feature,
> > it is better to clearly state that the bug had no impact until now.
> >
> > And as usual, the "fix" word in the title and the "Fixes" tag are required.
>
> What's the right way supposed to use "Fixes" tag, BTW? Checking the git
> hisotry, I saw something like:
>
> Fixes: $commit_hash ($commit_log).
In http://dpdk.org/dev, this git alias is recommended:
fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")'
> Which basically means it's a regression fix. However, in this case, it's
> more than like a bug, but not a regression.
Referencing the original commit (introducing the bug) makes it clear.
On Tue, Oct 20, 2015 at 09:27:37AM +0200, Thomas Monjalon wrote:
> 2015-10-20 15:23, Yuanhan Liu:
> > On Tue, Oct 20, 2015 at 09:07:49AM +0200, Thomas Monjalon wrote:
> > > 2015-10-09 13:46, Yuanhan Liu:
> > > > From: Changchun Ouyang <changchun.ouyang@intel.com>
> > > >
> > > > The old code adjusts the config bytes we want to read depending on
> > > > what kind of features we have, but we later cast the entire buf we
> > > > read with "struct virtio_net_config", which is obviously wrong.
> > >
> > > When describing a bug, it is important to explain what is the consequence,
> > > i.e. which use case is failing. If it is only to prepare the new feature,
> > > it is better to clearly state that the bug had no impact until now.
> > >
> > > And as usual, the "fix" word in the title and the "Fixes" tag are required.
> >
> > What's the right way supposed to use "Fixes" tag, BTW? Checking the git
> > hisotry, I saw something like:
> >
> > Fixes: $commit_hash ($commit_log).
>
> In http://dpdk.org/dev, this git alias is recommended:
> fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")'
Thanks for the tip.
>
> > Which basically means it's a regression fix. However, in this case, it's
> > more than like a bug, but not a regression.
>
> Referencing the original commit (introducing the bug) makes it clear.
Okay.
--yliu
@@ -1162,7 +1162,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
struct virtio_hw *hw = eth_dev->data->dev_private;
struct virtio_net_config *config;
struct virtio_net_config local_config;
- uint32_t offset_conf = sizeof(config->mac);
struct rte_pci_device *pci_dev;
RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr));
@@ -1221,8 +1220,14 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
if (vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_VQ)) {
config = &local_config;
+ vtpci_read_dev_config(hw,
+ offsetof(struct virtio_net_config, mac),
+ &config->mac, sizeof(config->mac));
+
if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) {
- offset_conf += sizeof(config->status);
+ vtpci_read_dev_config(hw,
+ offsetof(struct virtio_net_config, status),
+ &config->status, sizeof(config->status));
} else {
PMD_INIT_LOG(DEBUG,
"VIRTIO_NET_F_STATUS is not supported");
@@ -1230,15 +1235,16 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
}
if (vtpci_with_feature(hw, VIRTIO_NET_F_MQ)) {
- offset_conf += sizeof(config->max_virtqueue_pairs);
+ vtpci_read_dev_config(hw,
+ offsetof(struct virtio_net_config, max_virtqueue_pairs),
+ &config->max_virtqueue_pairs,
+ sizeof(config->max_virtqueue_pairs));
} else {
PMD_INIT_LOG(DEBUG,
"VIRTIO_NET_F_MQ is not supported");
config->max_virtqueue_pairs = 1;
}
- vtpci_read_dev_config(hw, 0, (uint8_t *)config, offset_conf);
-
hw->max_rx_queues =
(VIRTIO_MAX_RX_QUEUES < config->max_virtqueue_pairs) ?
VIRTIO_MAX_RX_QUEUES : config->max_virtqueue_pairs;