From: "Mody, Rasesh" <Rasesh.Mody@cavium.com>
To: Yuanhan Liu <yuanhan.liu@linux.intel.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: Sun, 7 May 2017 05:15:47 +0000 [thread overview]
Message-ID: <BLUPR0701MB157227E7D07598C45FD8DF5A9FE90@BLUPR0701MB1572.namprd07.prod.outlook.com> (raw)
In-Reply-To: <20170504021115.GD3005@yliu-dev.sh.intel.com>
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Wednesday, May 03, 2017 7:11 PM
>
> 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.
We can skip this patch for stable release backport.
Thanks!
-Rasesh
>
> --yliu
next prev parent reply other threads:[~2017-05-07 5: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
2017-05-07 5:15 ` Mody, Rasesh [this message]
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=BLUPR0701MB157227E7D07598C45FD8DF5A9FE90@BLUPR0701MB1572.namprd07.prod.outlook.com \
--to=rasesh.mody@cavium.com \
--cc=Dept-EngDPDKDev@cavium.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=stable@dpdk.org \
--cc=thomas@monjalon.net \
--cc=yuanhan.liu@linux.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).