From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
To: "Mody, Rasesh" <Rasesh.Mody@cavium.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
Dept-Eng DPDK Dev <Dept-EngDPDKDev@cavium.com>,
"stable@dpdk.org" <stable@dpdk.org>,
Ferruh Yigit <ferruh.yigit@intel.com>,
Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [dpdk-stable] [PATCH 11/11] net/qede: fix to limit CFLAGS to base files
Date: Thu, 4 May 2017 10:11:15 +0800 [thread overview]
Message-ID: <20170504021115.GD3005@yliu-dev.sh.intel.com> (raw)
In-Reply-To: <BLUPR0701MB1572244C8BC76C935B8B888E9FEA0@BLUPR0701MB1572.namprd07.prod.outlook.com>
On Thu, May 04, 2017 at 12:14:30AM +0000, Mody, Rasesh wrote:
>
>
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > Sent: Monday, May 01, 2017 11:15 PM
> >
> > On Tue, Apr 25, 2017 at 12:28:46AM -0700, Rasesh Mody wrote:
> > > From: Rasesh Mody <rasesh.mody@qlogic.com>
> > >
> > > Changes included in this fix
> > > - limit CFLAGS to base files
> > > - fix to remove/mark unused members
> > > - add checks for debug config option
> > > - make qede_set_mtu() and qede_udp_dst_port_del() static and others
> > > non-static as appropriate
> > > - move local APIs qede_vlan_offload_set() and
> > > qede_rx_cqe_to_pkt_type()
> > > - initialize variables as required
> >
> > When there are so many items in one single patch, it basically means it's
> > done wrongly. Generally, we should make one patch for each item.
> >
> > > Fixes: ec94dbc57362 ("qede: add base driver")
> > > Cc: stable@dpdk.org
> >
> > It's also not a good idea to put "Cc: stable" tag in a huge fix patch.
> > It's very likely it won't apply cleanly to a stable/LTS release. For instance, I
> > failed to apply it to 16.11.2 (LTS).
> >
> > >
> > > Signed-off-by: Rasesh Mody <rasesh.mody@qlogic.com>
> > > ---
> > > drivers/net/qede/Makefile | 32 ++++-----
> > > drivers/net/qede/base/ecore.h | 4 +-
> > > drivers/net/qede/base/ecore_int_api.h | 4 +-
> > > drivers/net/qede/qede_ethdev.c | 120 ++++++++++++++++++--------
> > -------
> > > drivers/net/qede/qede_ethdev.h | 32 ++++-----
> > > drivers/net/qede/qede_fdir.c | 13 +---
> > > drivers/net/qede/qede_if.h | 4 ++
> > > drivers/net/qede/qede_main.c | 8 +--
> > > drivers/net/qede/qede_rxtx.c | 118 +++++++++++++++++-------------
> > --
> > > 9 files changed, 171 insertions(+), 164 deletions(-)
> >
> > It's also a clear sign of bad patch: too many changes for a single bug fix patch.
> >
> > Most of them look like minor fixes to me. So my question is are there any
> > important items really should be picked for stable and LTS release?
> > More specifically, do they really fix any (fatal) issues? If no, I will drop it. If
> > yes, please send a (or some) patch with the real fixes backported only to
> > stable ML, so that I could pick them.
>
> The patch is a Makefile change to restrict the CFLAG only to the base files. Once Makefile was changed it exposed few issues with PMD.
In such case, you could make patches to fix those issues first, one
patch for one issue, and then put the CFLAG change to the last.
> Hence, we thought of putting all the changes in single patch since they were relevant changes.
>
> As you stated most of them are minor fixes. We'll evaluate the patch if anything specifically need to go into the stable release and get back.
Thanks. The answer seems to be "NO" to me though.
--yliu
next prev parent reply other threads:[~2017-05-04 2:15 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-25 7:28 [dpdk-stable] [PATCH 01/11] net/qede: fix default MAC address handling Rasesh Mody
2017-04-25 7:28 ` [dpdk-stable] [PATCH 02/11] net/qede: fix reset of fastpath rings after port stop Rasesh Mody
2017-04-25 7:28 ` [dpdk-stable] [PATCH 07/11] net/qede: fix FW version string display for SRIOV Rasesh Mody
2017-04-25 7:28 ` [dpdk-stable] [PATCH 08/11] net/qede/base: fix coverity issues Rasesh Mody
2017-04-25 7:28 ` [dpdk-stable] [PATCH 10/11] net/qede/base: fix find zero bit macro Rasesh Mody
2017-04-25 7:28 ` [dpdk-stable] [PATCH 11/11] net/qede: fix to limit CFLAGS to base files Rasesh Mody
2017-05-02 6:15 ` Yuanhan Liu
2017-05-04 0:14 ` Mody, Rasesh
2017-05-04 2:11 ` Yuanhan Liu [this message]
2017-05-07 5:15 ` Mody, Rasesh
2017-04-26 8:15 ` [dpdk-stable] [PATCH 01/11] net/qede: fix default MAC address handling Ferruh Yigit
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=20170504021115.GD3005@yliu-dev.sh.intel.com \
--to=yuanhan.liu@linux.intel.com \
--cc=Dept-EngDPDKDev@cavium.com \
--cc=Rasesh.Mody@cavium.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=stable@dpdk.org \
--cc=thomas@monjalon.net \
/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).