[bug report] qed*: Add support for VFs over legacy PFs

[bug report] qed*: Add support for VFs over legacy PFs

[Dan Carpenter]

Hello Yuval Mintz,

The patch d8c2c7e3404e: "qed*: Add support for VFs over legacy PFs"
from Aug 22, 2016, leads to the following static checker warning:

	drivers/net/ethernet/qlogic/qed/qed_vf.c:297 qed_vf_pf_acquire()
	warn: should this be a bitwise op?

drivers/net/ethernet/qlogic/qed/qed_vf.c
   289          /* Learn of the possibility of CMT */
   290          if (IS_LEAD_HWFN(p_hwfn)) {
   291                  if (resp->pfdev_info.capabilities & PFVF_ACQUIRE_CAP_100G) {
   292                          DP_NOTICE(p_hwfn, "100g VF\n");
   293                          p_hwfn->cdev->num_hwfns = 2;
   294                  }
   295          }
   296  
   297          if (!p_iov->b_pre_fp_hsi &&
   298              ETH_HSI_VER_MINOR &&
   299              (resp->pfdev_info.minor_fp_hsi < ETH_HSI_VER_MINOR)) {

It looks like this code works correctly.  I think the ETH_HSI_VER_MINOR
check is to silence a static checker warning because otherwise we are
occasionally comparing an unsigned with less than zero?  (Although I
think most static checkers will still complain so maybe that's not
true?).  Anyway, it's weird code.

It would probably be more clear to say "ETH_HSI_VER_MINOR > 0 &&".
When we're talking about comparisons to zero then it's idiomatic to use
"foo != 0" if we're talking about the number zero or strcmp() functions.
If it's used as a bool then we would say "!foo"...

   300                  DP_INFO(p_hwfn,
   301                          "PF is using older fastpath HSI; %02x.%02x is configured\n",
   302                          ETH_HSI_VER_MAJOR, resp->pfdev_info.minor_fp_hsi);
   303          }
   304  
   305  exit:
   306          qed_vf_pf_req_end(p_hwfn, rc);
   307  
   308          return rc;

regards,
dan carpenter

RE: [bug report] qed*: Add support for VFs over legacy PFs

[Mintz, Yuval]

> The patch d8c2c7e3404e: "qed*: Add support for VFs over legacy PFs"
> from Aug 22, 2016, leads to the following static checker warning:
> 
> 	drivers/net/ethernet/qlogic/qed/qed_vf.c:297 qed_vf_pf_acquire()
> 	warn: should this be a bitwise op?
> 
> drivers/net/ethernet/qlogic/qed/qed_vf.c
>    289          /* Learn of the possibility of CMT */
>    290          if (IS_LEAD_HWFN(p_hwfn)) {
>    291                  if (resp->pfdev_info.capabilities & PFVF_ACQUIRE_CAP_100G) {
>    292                          DP_NOTICE(p_hwfn, "100g VF\n");
>    293                          p_hwfn->cdev->num_hwfns = 2;
>    294                  }
>    295          }
>    296
>    297          if (!p_iov->b_pre_fp_hsi &&
>    298              ETH_HSI_VER_MINOR &&
>    299              (resp->pfdev_info.minor_fp_hsi < ETH_HSI_VER_MINOR)) {
> 
> It looks like this code works correctly.  I think the ETH_HSI_VER_MINOR check is
> to silence a static checker warning because otherwise we are occasionally
> comparing an unsigned with less than zero?  (Although I think most static
> checkers will still complain so maybe that's not true?).  Anyway, it's weird code.
> 
> It would probably be more clear to say "ETH_HSI_VER_MINOR > 0 &&".

Can't say I recall the exact rational here, but I think you're right.
Given that initially when this was submitted the fastpath HSI version was 3.0
[currently its 3.10], it makes sense that I've added this exactly due to the reason you're describing.

And I agrees that doing "> 0" is clearer.

Thanks,
Yuval