[dpdk-dev] [PATCH v4 00/32] net/qede: update qede pmd to 1.2.0.1 and enable by default

[Mody, Rasesh]

Hi Thomas,

> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Wednesday, October 26, 2016 8:20 AM
> 
> 2016-10-24 14:41, Bruce Richardson:
> > On Tue, Oct 18, 2016 at 09:11:14PM -0700, Rasesh Mody wrote:
> > > Please apply to DPDK tree for v16.11 release.
> >
> > Patchset applied to dpdk_next_net/rel_16_11
> 
> It breaks compilation because it is enabled everywhere and zlib.h is still
> included without checking CONFIG_ECORE_ZIPPED_FW.
> The patch removing zlib dependency was not tested without zlib installed.
> I will fix it while applying with this change:

Sorry, we missed to test the patch removing zlib dependency from latest patch set when zlib headers are unavailable.
The zlib.h include is not needed in bcm_osal.c. It got left out there when zlib.h was included in ecore.h file by patch "[PATCH v4 03/32] net/qede: use FW CONFIG defines as needed". In ecore.h it is protected by ifdef, however, same is not true about bcm_osal.c. Hence compilation complains when zlib.h is not available.

--- a/drivers/net/qede/base/bcm_osal.c
+++ b/drivers/net/qede/base/bcm_osal.c
@@ -6,8 +6,6 @@
  * See LICENSE.qede_pmd for copyright and licensing details.
  */

-#include <zlib.h>
-
 #include <rte_memzone.h>
 #include <rte_errno.h>


> --- a/drivers/net/qede/base/bcm_osal.c
> +++ b/drivers/net/qede/base/bcm_osal.c
> @@ -6,7 +6,9 @@
>   * See LICENSE.qede_pmd for copyright and licensing details.
>   */
> 
> +#ifdef CONFIG_ECORE_ZIPPED_FW
>  #include <zlib.h>
> +#endif
> 
>  #include <rte_memzone.h>
>  #include <rte_errno.h>
> 

Above change looks fine. Thanks!
 
> I won't do any quality review of qede patches but from what I've seen
> before, there is some room for improvements.
> 
> Another nit, important to help reviews, please use --in-reply-to when
> sending a new revision of a patch to keep them in the same thread and allow
> us to understand the progress.
> I plan to do an automatic nack for patches missing the --in-reply-to.

Sure, will do.

Thanks!
-Rasesh