DPDK patches and discussions
 help / color / mirror / Atom feed
From: Tiwei Bie <tiwei.bie@intel.com>
To: Luca Boccassi <bluca@debian.org>
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: Thu, 16 Aug 2018 14:46:00 +0800
Message-ID: <20180816064600.GA17647@debian> (raw)
In-Reply-To: <1534326657.5764.11.camel@debian.org>

On Wed, Aug 15, 2018 at 10:50:57AM +0100, Luca Boccassi wrote:
> 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.

Could you be a bit more specific about which errors
were logged if possible?

If my understanding is correct, you mean the errors
were logged because less than the required amount of
bytes were read?

> 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.

If it's a fix, we need a fixes line.

> 
> > > 
[...]
> > > @@ -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)) {

Above code has to successfully read a full virtio
PCI capability during each read, otherwise it will
give up reading other capabilities and may fallback
to the legacy mode. In which case it will fail to
read the requested amount of bytes? Should we try
to read the generic PCI fields first?

Besides, you also need to update other calls to
rte_pci_read_config(), e.g.:

https://github.com/DPDK/dpdk/blob/76b9d9de5c7d/drivers/net/virtio/virtio_pci.c#L696

Thanks

> > > +			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

  reply	other threads:[~2018-08-16  6:46 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
2018-08-16  6:46       ` Tiwei Bie [this message]
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=20180816064600.GA17647@debian \
    --to=tiwei.bie@intel.com \
    --cc=bluca@debian.org \
    --cc=brian.russell@intl.att.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=maxime.coquelin@redhat.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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git