[dpdk-dev] [dpdk-stable] [PATCH 11/11] net/qede: fix to limit CFLAGS to base files

[Yuanhan Liu]

On Thu, May 04, 2017 at 12:14:30AM +0000, Mody, Rasesh wrote:
> 
> 
> > From: Yuanhan Liu [mailto:yuanhan.liu at 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 at 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 at 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 at 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