From: Luca Boccassi <bluca@debian.org>
To: Tiwei Bie <tiwei.bie@intel.com>
Cc: dev@dpdk.org, maxime.coquelin@redhat.com, zhihong.wang@intel.com,
bruce.richardson@intel.com,
Brian Russell <brian.russell@intl.att.com>
Subject: Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling
Date: Wed, 15 Aug 2018 10:50:57 +0100 [thread overview]
Message-ID: <1534326657.5764.11.camel@debian.org> (raw)
In-Reply-To: <20180815031144.GA7324@debian>
On Wed, 2018-08-15 at 11:11 +0800, Tiwei Bie wrote:
> On Tue, Aug 14, 2018 at 03:30:35PM +0100, Luca Boccassi wrote:
> > From: Brian Russell <brussell@brocade.com>
> >
> > In virtio_read_caps, rte_pci_read_config returns the number of
> > bytes
> > read from PCI config or < 0 on error.
> > If less than the expected number of bytes are read then log the
> > failure and return rather than carrying on with garbage.
>
> Is this a fix or an improvement?
> Or did you see anything broken without this patch?
> If so, we may need a fixes line and Cc stable.
It is a fix, as it was creating problems in production due to the
constant flux of errors in the logs.
But given patch 1/2 is effectively doing a small change in the BSD bus
API, and it's a requirement for 2/2, I don't think we can include it in
the stable releases unfortunately.
> >
> > Signed-off-by: Brian Russell <brussell@brocade.com>
> > ---
> >
> > Follow-up from:
> > http://mails.dpdk.org/archives/dev/2017-June/067278.html
> > https://patches.dpdk.org/patch/25056/
> >
> > drivers/net/virtio/virtio_pci.c | 12 +++++++-----
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/virtio/virtio_pci.c
> > b/drivers/net/virtio/virtio_pci.c
> > index 6bd22e54a6..a10698aed8 100644
> > --- a/drivers/net/virtio/virtio_pci.c
> > +++ b/drivers/net/virtio/virtio_pci.c
> > @@ -567,16 +567,18 @@ virtio_read_caps(struct rte_pci_device *dev,
> > struct virtio_hw *hw)
> > }
> >
> > ret = rte_pci_read_config(dev, &pos, 1,
> > PCI_CAPABILITY_LIST);
> > - if (ret < 0) {
> > - PMD_INIT_LOG(DEBUG, "failed to read pci capability
> > list");
> > + if (ret != 1) {
> > + PMD_INIT_LOG(DEBUG,
> > + "failed to read pci capability list,
> > ret %d", ret);
> > return -1;
> > }
> >
> > while (pos) {
> > ret = rte_pci_read_config(dev, &cap, sizeof(cap),
> > pos);
> > - if (ret < 0) {
> > - PMD_INIT_LOG(ERR,
> > - "failed to read pci cap at pos:
> > %x", pos);
> > + if (ret != sizeof(cap)) {
> > + PMD_INIT_LOG(DEBUG,
>
> Why change the log level to DEBUG?
>
> Thanks
Beforehand reading less than the required amount of bytes caused
problems in the following code, so it warranted printing errors - but
now it will not go ahead without the right amount of data, so it's not
critical anymore to inform the user.
Main issue is, log will get very spammy with errors, and paying
customers don't like that :-)
--
Kind regards,
Luca Boccassi
next prev parent reply other threads:[~2018-08-15 9:50 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-14 14:30 [dpdk-dev] [PATCH 1/2] bus/pci: harmonize and document rte_pci_read_config return value Luca Boccassi
2018-08-14 14:30 ` [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling Luca Boccassi
2018-08-15 3:11 ` Tiwei Bie
2018-08-15 9:50 ` Luca Boccassi [this message]
2018-08-16 6:46 ` Tiwei Bie
2018-08-16 10:27 ` Luca Boccassi
2018-08-16 18:47 ` [dpdk-dev] [PATCH 1/2] bus/pci: harmonize and document rte_pci_read_config return value Luca Boccassi
2018-08-16 18:47 ` [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling Luca Boccassi
2018-08-16 18:49 ` Luca Boccassi
2018-08-20 8:18 ` Tiwei Bie
2018-08-20 16:45 ` Luca Boccassi
2018-08-21 2:40 ` Tiwei Bie
2018-08-23 12:52 ` Luca Boccassi
2018-08-24 4:23 ` Tiwei Bie
2018-08-24 17:14 ` Luca Boccassi
2018-08-20 16:44 ` [dpdk-dev] [PATCH v3 1/2] bus/pci: harmonize and document rte_pci_read_config return value Luca Boccassi
2018-08-20 16:44 ` [dpdk-dev] [PATCH v3 2/2] virtio: fix PCI config err handling Luca Boccassi
2018-08-24 17:14 ` [dpdk-dev] [PATCH v4 1/2] bus/pci: harmonize and document rte_pci_read_config return value Luca Boccassi
2018-08-24 17:14 ` [dpdk-dev] [PATCH v4 2/2] virtio: fix PCI config err handling Luca Boccassi
2018-08-27 5:29 ` Tiwei Bie
2018-08-27 16:52 ` Luca Boccassi
2018-08-28 6:47 ` Tiwei Bie
2018-08-27 16:52 ` [dpdk-dev] [PATCH v5 1/2] bus/pci: harmonize and document rte_pci_read_config return value Luca Boccassi
2018-08-27 16:52 ` [dpdk-dev] [PATCH v5 2/2] virtio: fix PCI config err handling Luca Boccassi
2018-08-28 6:43 ` Tiwei Bie
2018-08-28 10:14 ` Luca Boccassi
2018-08-28 10:12 ` [dpdk-dev] [PATCH v6 1/2] bus/pci: harmonize and document rte_pci_read_config return value Luca Boccassi
2018-08-28 10:12 ` [dpdk-dev] [PATCH v6 2/2] net/virtio: fix PCI config err handling Luca Boccassi
2018-10-11 10:27 ` Thomas Monjalon
2018-10-11 10:53 ` Luca Boccassi
2018-10-11 13:01 ` Tiwei Bie
2018-10-28 23:55 ` Thomas Monjalon
2018-10-17 9:58 ` [dpdk-dev] [PATCH v6 1/2] bus/pci: harmonize and document rte_pci_read_config return value Luca Boccassi
2018-10-17 10:57 ` Bruce Richardson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1534326657.5764.11.camel@debian.org \
--to=bluca@debian.org \
--cc=brian.russell@intl.att.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=maxime.coquelin@redhat.com \
--cc=tiwei.bie@intel.com \
--cc=zhihong.wang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).