re: mlx4_core: Modify driver initialization flow to accommodate SRIOV for Ethernet

re: mlx4_core: Modify driver initialization flow to accommodate SRIOV for Ethernet

[Dan Carpenter]

Hello Jack Morgenstein,

The patch ab9c17a009ee: "mlx4_core: Modify driver initialization flow 
to accommodate SRIOV for Ethernet" from Dec 13, 2011, leads to the 
following static checker warning:

drivers/net/ethernet/mellanox/mlx4/main.c:424 mlx4_slave_cap()
	warn: suspicious bitop condition

   423          /*fail if the hca has an unknown capability */
   424          if ((hca_param.global_caps | HCA_GLOBAL_CAP_MASK) !=
   425              HCA_GLOBAL_CAP_MASK) {
   426                  mlx4_err(dev, "Unknown hca global capabilities\n");
   427                  return -ENOSYS;
   428          }

The test sort of makes sense but HCA_GLOBAL_CAP_MASK is zero so we could
as well say:

	if (hca_param.global_caps) { ...

regards,
dan carpenter

Re: mlx4_core: Modify driver initialization flow to accommodate SRIOV for Ethernet

[Jack Morgenstein]

On Saturday 09 June 2012 12:09, Dan Carpenter wrote:
>    423          /*fail if the hca has an unknown capability */
>    424          if ((hca_param.global_caps | HCA_GLOBAL_CAP_MASK) !=
>    425              HCA_GLOBAL_CAP_MASK) {
>    426                  mlx4_err(dev, "Unknown hca global capabilities\n");
>    427                  return -ENOSYS;
>    428          }
> 
> The test sort of makes sense but HCA_GLOBAL_CAP_MASK is zero so we could
> as well say:
> 
>         if (hca_param.global_caps) { ...
> 
The parameter global_caps is to guarantee that the guest OS is capable
of supporting the driver which is running on the master (hypervisor).

If a new feature impacts the guests, we will define a bit in the
global_caps bitmask and the master driver will set this bit and
the global_caps bitmask with the newly set bit will be passed
to the slaves.

The HCA_GLOBAL_CAP_MASK on the slave OS represents the set
of such features which the slave knows how to support.
If a new feature appears which is not set in the slave's
HCA_GLOBAL_CAP_MASK, this test detects the discrepancy and
the slave driver aborts.

This global_caps parameter passed from the master to the slave
is currently zero, because all current master capabilities
are supported on the slaves.

In the (probably near) future, when new capabilities are added,
Slave O/S's will also need to be updated.  If the master O/S driver is
updated, but an older slave O/S driver  isstill installed, the test
will fail and the driver on the slave will abort.

-Jack

Re: mlx4_core: Modify driver initialization flow to accommodate SRIOV for Ethernet

[Ben Hutchings]

On Sat, 2012-06-09 at 14:29 +0300, Jack Morgenstein wrote:
> On Saturday 09 June 2012 12:09, Dan Carpenter wrote:
> >    423          /*fail if the hca has an unknown capability */
> >    424          if ((hca_param.global_caps | HCA_GLOBAL_CAP_MASK) !=
> >    425              HCA_GLOBAL_CAP_MASK) {
> >    426                  mlx4_err(dev, "Unknown hca global capabilities\n");
> >    427                  return -ENOSYS;
> >    428          }
> > 
> > The test sort of makes sense but HCA_GLOBAL_CAP_MASK is zero so we could
> > as well say:
> > 
> >         if (hca_param.global_caps) { ...
> > 
> The parameter global_caps is to guarantee that the guest OS is capable
> of supporting the driver which is running on the master (hypervisor).
> 
> If a new feature impacts the guests, we will define a bit in the
> global_caps bitmask and the master driver will set this bit and
> the global_caps bitmask with the newly set bit will be passed
> to the slaves.
> 
> The HCA_GLOBAL_CAP_MASK on the slave OS represents the set
> of such features which the slave knows how to support.
> If a new feature appears which is not set in the slave's
> HCA_GLOBAL_CAP_MASK, this test detects the discrepancy and
> the slave driver aborts.
[...]

It would be more idiomatic to write the condition as:

	if (hca_param.global_caps & ~(u64)HCA_GLOBAL_CAP_MASK)

(although that cast probably belongs in the definition of
HCA_GLOBAL_CAP_MASK).

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: mlx4_core: Modify driver initialization flow to accommodate SRIOV for Ethernet

[Jack Morgenstein]

On Sunday 10 June 2012 04:03, Ben Hutchings wrote:
> It would be more idiomatic to write the condition as:
> 
>         if (hca_param.global_caps & ~(u64)HCA_GLOBAL_CAP_MASK)
> 
> (although that cast probably belongs in the definition of
> HCA_GLOBAL_CAP_MASK).
> 
You are correct!
We need to submit a patch to fix this.

-Jack