From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 77ABC10A3 for ; Thu, 16 Aug 2018 08:46:56 +0200 (CEST) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Aug 2018 23:46:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,246,1531810800"; d="scan'208";a="80688251" Received: from debian.sh.intel.com (HELO debian) ([10.67.104.194]) by fmsmga004.fm.intel.com with ESMTP; 15 Aug 2018 23:46:41 -0700 Date: Thu, 16 Aug 2018 14:46:00 +0800 From: Tiwei Bie To: Luca Boccassi Cc: dev@dpdk.org, maxime.coquelin@redhat.com, zhihong.wang@intel.com, bruce.richardson@intel.com, Brian Russell Message-ID: <20180816064600.GA17647@debian> References: <20180814143035.19640-1-bluca@debian.org> <20180814143035.19640-2-bluca@debian.org> <20180815031144.GA7324@debian> <1534326657.5764.11.camel@debian.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1534326657.5764.11.camel@debian.org> User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Aug 2018 06:46:57 -0000 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 > > > > > > 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