[PATCH net 0/2] net/mlx4_core: SR-IOV related fixes

[PATCH net 0/2] net/mlx4_core: SR-IOV related fixes

[Amir Vadai]

Hi,

This small patchset fixes two issues in VF/PF initialization and loading.
First patch fixes a typo that caused a memory access error during VF
initialization.
Second patch prevent the PF probe function from being blocked for many seconds
while probing VF's

Patches were tested and applied on commit 1b07da5: "hyperv: Move state setting
for link query"

Thanks,
Amir

Amir Vadai (2):
  net/mlx4_core: Fix memory access error in mlx4_QUERY_DEV_CAP_wrapper()
  net/mlx4_core: mlx4_init_slave() shouldn't access comm channel before
    PF is ready

 drivers/net/ethernet/mellanox/mlx4/fw.c   |  6 +++---
 drivers/net/ethernet/mellanox/mlx4/main.c | 11 +++++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)

-- 
1.8.3.4

[PATCH net 1/2] net/mlx4_core: Fix memory access error in mlx4_QUERY_DEV_CAP_wrapper()

[Amir Vadai]

Fix a regression introduced by [1]. outbox was accessed instead of
outbox->buf. Typo was copy-pasted to [2] and [3].

[1] - cc1ade9 mlx4_core: Disable memory windows for virtual functions
[2] - 4de6580 mlx4_core: Add support for steerable IB UD QPs
[3] - 7ffdf72 net/mlx4_core: Add basic support for TCP/IP offloads under
      tunneling

Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/fw.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/fw.c b/drivers/net/ethernet/mellanox/mlx4/fw.c
index 91b69ff..8726e34 100644
--- a/drivers/net/ethernet/mellanox/mlx4/fw.c
+++ b/drivers/net/ethernet/mellanox/mlx4/fw.c
@@ -859,7 +859,7 @@ int mlx4_QUERY_DEV_CAP_wrapper(struct mlx4_dev *dev, int slave,
 	MLX4_PUT(outbox->buf, field, QUERY_DEV_CAP_CQ_TS_SUPPORT_OFFSET);
 
 	/* For guests, disable vxlan tunneling */
-	MLX4_GET(field, outbox, QUERY_DEV_CAP_VXLAN);
+	MLX4_GET(field, outbox->buf, QUERY_DEV_CAP_VXLAN);
 	field &= 0xf7;
 	MLX4_PUT(outbox->buf, field, QUERY_DEV_CAP_VXLAN);
 
@@ -869,7 +869,7 @@ int mlx4_QUERY_DEV_CAP_wrapper(struct mlx4_dev *dev, int slave,
 	MLX4_PUT(outbox->buf, field, QUERY_DEV_CAP_BF_OFFSET);
 
 	/* For guests, disable mw type 2 */
-	MLX4_GET(bmme_flags, outbox, QUERY_DEV_CAP_BMME_FLAGS_OFFSET);
+	MLX4_GET(bmme_flags, outbox->buf, QUERY_DEV_CAP_BMME_FLAGS_OFFSET);
 	bmme_flags &= ~MLX4_BMME_FLAG_TYPE_2_WIN;
 	MLX4_PUT(outbox->buf, bmme_flags, QUERY_DEV_CAP_BMME_FLAGS_OFFSET);
 
@@ -883,7 +883,7 @@ int mlx4_QUERY_DEV_CAP_wrapper(struct mlx4_dev *dev, int slave,
 	}
 
 	/* turn off ipoib managed steering for guests */
-	MLX4_GET(field, outbox, QUERY_DEV_CAP_FLOW_STEERING_IPOIB_OFFSET);
+	MLX4_GET(field, outbox->buf, QUERY_DEV_CAP_FLOW_STEERING_IPOIB_OFFSET);
 	field &= ~0x80;
 	MLX4_PUT(outbox->buf, field, QUERY_DEV_CAP_FLOW_STEERING_IPOIB_OFFSET);
 
-- 
1.8.3.4

[PATCH net 2/2] net/mlx4_core: mlx4_init_slave() shouldn't access comm channel before PF is ready

[Amir Vadai]

Currently, the PF call to pci_enable_sriov from the PF probe function
stalls for 10 seconds times the number of VFs probed on the host. This
happens because the way for such VFs to determine of the PF
initialization finished, is by attempting to issue reset on the
comm-channel and get timeout (after 10s).

The PF probe function is called from a kenernel workqueue, and therefore
during that time, rcu lock is being held and kernel's workqueue is
stalled. This blocks other processes that try to use the workqueue
or rcu lock.  For example, interface renaming which is calling
rcu_synchronize is blocked, and timedout by systemd.

Changed mlx4_init_slave() to allow VF probed on the host to immediatly
detect that the PF is not ready, and return EPROBE_DEFER instantly.

Only when the PF finishes the initialization, allow such VFs to
access the comm channel.

This issue and fix are relevant only for probed VFs on the hypervisor,
there is no way to pass this information to a VM until comm channel is
ready, so in a VM, if PF is not ready, the first command will be timedout
after 10 seconds and return EPROBE_DEFER.

Signed-off-by: Amir Vadai <amirv@mellanox.com>
Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/main.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index 5a6105f..30a08a6 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -150,6 +150,8 @@ struct mlx4_port_config {
 	struct pci_dev *pdev;
 };
 
+static atomic_t pf_loading = ATOMIC_INIT(0);
+
 int mlx4_check_port_params(struct mlx4_dev *dev,
 			   enum mlx4_port_type *port_type)
 {
@@ -1407,6 +1409,11 @@ static int mlx4_init_slave(struct mlx4_dev *dev)
 	u32 slave_read;
 	u32 cmd_channel_ver;
 
+	if (atomic_read(&pf_loading)) {
+		mlx4_warn(dev, "PF is not ready. Deferring probe\n");
+		return -EPROBE_DEFER;
+	}
+
 	mutex_lock(&priv->cmd.slave_cmd_mutex);
 	priv->cmd.max_cmds = 1;
 	mlx4_warn(dev, "Sending reset\n");
@@ -2319,7 +2326,11 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data)
 
 		if (num_vfs) {
 			mlx4_warn(dev, "Enabling SR-IOV with %d VFs\n", num_vfs);
+
+			atomic_inc(&pf_loading);
 			err = pci_enable_sriov(pdev, num_vfs);
+			atomic_dec(&pf_loading);
+
 			if (err) {
 				mlx4_err(dev, "Failed to enable SR-IOV, continuing without SR-IOV (err = %d).\n",
 					 err);
-- 
1.8.3.4

Re: [PATCH net 2/2] net/mlx4_core: mlx4_init_slave() shouldn't access comm channel before PF is ready

[David Miller]

From: Amir Vadai <amirv@mellanox.com>
Date: Thu,  6 Mar 2014 18:28:17 +0200

> @@ -150,6 +150,8 @@ struct mlx4_port_config {
>  	struct pci_dev *pdev;
>  };
>  
> +static atomic_t pf_loading = ATOMIC_INIT(0);
> +
>  int mlx4_check_port_params(struct mlx4_dev *dev,
>  			   enum mlx4_port_type *port_type)
>  {
> @@ -1407,6 +1409,11 @@ static int mlx4_init_slave(struct mlx4_dev *dev)
>  	u32 slave_read;
>  	u32 cmd_channel_ver;
>  
> +	if (atomic_read(&pf_loading)) {
> +		mlx4_warn(dev, "PF is not ready. Deferring probe\n");
> +		return -EPROBE_DEFER;
> +	}
> +
 ...
> @@ -2319,7 +2326,11 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data)
>  
>  		if (num_vfs) {
>  			mlx4_warn(dev, "Enabling SR-IOV with %d VFs\n", num_vfs);
> +
> +			atomic_inc(&pf_loading);
>  			err = pci_enable_sriov(pdev, num_vfs);
> +			atomic_dec(&pf_loading);
> +

This synchronization scheme doesn't look right to me at all.

It's global, so VF's for _any_ PF will probe defer while one is enabling
SRIOV.

It doesn't seem correct to cause unrelated VF's to defer the probe.

You have absolutely have to maintain this state at least on a per-PF
level.

Re: [PATCH net 2/2] net/mlx4_core: mlx4_init_slave() shouldn't access comm channel before PF is ready

[Or Gerlitz]

On Thu, Mar 6, 2014 at 10:12 PM, David Miller <davem@davemloft.net> wrote:
> From: Amir Vadai <amirv@mellanox.com>
> Date: Thu,  6 Mar 2014 18:28:17 +0200
> > @@ -150,6 +150,8 @@ struct mlx4_port_config {
> >       struct pci_dev *pdev;
> >  };
> >
> > +static atomic_t pf_loading = ATOMIC_INIT(0);
> > +
> >  int mlx4_check_port_params(struct mlx4_dev *dev,
> >                          enum mlx4_port_type *port_type)
> >  {
> > @@ -1407,6 +1409,11 @@ static int mlx4_init_slave(struct mlx4_dev *dev)
> >       u32 slave_read;
> >       u32 cmd_channel_ver;
> >
> > +     if (atomic_read(&pf_loading)) {
> > +             mlx4_warn(dev, "PF is not ready. Deferring probe\n");
> > +             return -EPROBE_DEFER;
> > +     }
> > +
>  ...
> > @@ -2319,7 +2326,11 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data)
> >
> >               if (num_vfs) {
> >                       mlx4_warn(dev, "Enabling SR-IOV with %d VFs\n", num_vfs);
> > +
> > +                     atomic_inc(&pf_loading);
> >                       err = pci_enable_sriov(pdev, num_vfs);
> > +                     atomic_dec(&pf_loading);
> > +
>
> This synchronization scheme doesn't look right to me at all.
> It's global, so VF's for _any_ PF will probe defer while one is enabling SRIOV.
> It doesn't seem correct to cause unrelated VF's to defer the probe.

Hi Dave,

Can you please elaborate a bit why you find this approach to be
incorrect? basically, these nested VF probed are a bit headache
anyway, so we didn't find such global deferring to be problematic.

Or.

> You have absolutely have to maintain this state at least on a per-PF level.

Re: [PATCH net 2/2] net/mlx4_core: mlx4_init_slave() shouldn't access comm channel before PF is ready

[David Miller]

From: Or Gerlitz <or.gerlitz@gmail.com>
Date: Thu, 6 Mar 2014 22:19:48 +0200

> On Thu, Mar 6, 2014 at 10:12 PM, David Miller <davem@davemloft.net> wrote:
>> From: Amir Vadai <amirv@mellanox.com>
>> Date: Thu,  6 Mar 2014 18:28:17 +0200
>> > @@ -150,6 +150,8 @@ struct mlx4_port_config {
>> >       struct pci_dev *pdev;
>> >  };
>> >
>> > +static atomic_t pf_loading = ATOMIC_INIT(0);
>> > +
>> >  int mlx4_check_port_params(struct mlx4_dev *dev,
>> >                          enum mlx4_port_type *port_type)
>> >  {
>> > @@ -1407,6 +1409,11 @@ static int mlx4_init_slave(struct mlx4_dev *dev)
>> >       u32 slave_read;
>> >       u32 cmd_channel_ver;
>> >
>> > +     if (atomic_read(&pf_loading)) {
>> > +             mlx4_warn(dev, "PF is not ready. Deferring probe\n");
>> > +             return -EPROBE_DEFER;
>> > +     }
>> > +
>>  ...
>> > @@ -2319,7 +2326,11 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data)
>> >
>> >               if (num_vfs) {
>> >                       mlx4_warn(dev, "Enabling SR-IOV with %d VFs\n", num_vfs);
>> > +
>> > +                     atomic_inc(&pf_loading);
>> >                       err = pci_enable_sriov(pdev, num_vfs);
>> > +                     atomic_dec(&pf_loading);
>> > +
>>
>> This synchronization scheme doesn't look right to me at all.
>> It's global, so VF's for _any_ PF will probe defer while one is enabling SRIOV.
>> It doesn't seem correct to cause unrelated VF's to defer the probe.
> 
> Hi Dave,
> 
> Can you please elaborate a bit why you find this approach to be
> incorrect? basically, these nested VF probed are a bit headache
> anyway, so we didn't find such global deferring to be problematic.

What if a second PF starts to init and call pci_enable_sriov(), while the VFs
from a previous PF probed call mlx4_init_slave()?

It will increment pf_loading() and force those unreladed VFs to defer.

You must have a per-PF value to block the underlying VFs, rather than a global
one.

Re: [PATCH net 2/2] net/mlx4_core: mlx4_init_slave() shouldn't access comm channel before PF is ready

[Or Gerlitz]

On Thu, Mar 6, 2014 at 10:48 PM, David Miller <davem@davemloft.net> wrote:
> From: Or Gerlitz <or.gerlitz@gmail.com>
> Date: Thu, 6 Mar 2014 22:19:48 +0200
>> On Thu, Mar 6, 2014 at 10:12 PM, David Miller <davem@davemloft.net> wrote:

>>> > +static atomic_t pf_loading = ATOMIC_INIT(0);
>>> > @@ -1407,6 +1409,11 @@ static int mlx4_init_slave(struct mlx4_dev *dev)
>>> > +     if (atomic_read(&pf_loading)) {
>>> > +             mlx4_warn(dev, "PF is not ready. Deferring probe\n");
>>> > +             return -EPROBE_DEFER;
>>> > +     }
>>> > +
>>> > @@ -2319,7 +2326,11 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data)
>>> >
>>> >               if (num_vfs) {
>>> >                       mlx4_warn(dev, "Enabling SR-IOV with %d VFs\n",num_vfs);
>>> > +
>>> > +                     atomic_inc(&pf_loading);
>>> >                       err = pci_enable_sriov(pdev, num_vfs);
>>> > +                     atomic_dec(&pf_loading);
>>> > +

>>> This synchronization scheme doesn't look right to me at all.
>>> It's global, so VF's for _any_ PF will probe defer while one is enabling SRIOV.
>>> It doesn't seem correct to cause unrelated VF's to defer the probe.

>> Can you please elaborate a bit why you find this approach to be
>> incorrect? basically, these nested VF probed are a bit headache
>> anyway, so we didn't find such global deferring to be problematic.

> What if a second PF starts to init and call pci_enable_sriov(), while the VFs
> from a previous PF probed call mlx4_init_slave()?
> It will increment pf_loading() and force those unreladed VFs to defer.

By "unreladed VFs" I assume you mean unrelated VFs that belong to the
1st VF, which is OK for them to probe, right? so yes, this is sort of
conservative approach that wait till all PFs are fully ready, and I
understand you don't like it, but still, I would be happy to know
what's wrong in doing so..

> You must have a per-PF value to block the underlying VFs, rather than a global
> one.

Re: [PATCH net 2/2] net/mlx4_core: mlx4_init_slave() shouldn't access comm channel before PF is ready

[David Miller]

From: Or Gerlitz <or.gerlitz@gmail.com>
Date: Thu, 6 Mar 2014 23:08:36 +0200

> On Thu, Mar 6, 2014 at 10:48 PM, David Miller <davem@davemloft.net> wrote:
>> From: Or Gerlitz <or.gerlitz@gmail.com>
>> Date: Thu, 6 Mar 2014 22:19:48 +0200
>>> On Thu, Mar 6, 2014 at 10:12 PM, David Miller <davem@davemloft.net> wrote:
> 
>>>> > +static atomic_t pf_loading = ATOMIC_INIT(0);
>>>> > @@ -1407,6 +1409,11 @@ static int mlx4_init_slave(struct mlx4_dev *dev)
>>>> > +     if (atomic_read(&pf_loading)) {
>>>> > +             mlx4_warn(dev, "PF is not ready. Deferring probe\n");
>>>> > +             return -EPROBE_DEFER;
>>>> > +     }
>>>> > +
>>>> > @@ -2319,7 +2326,11 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data)
>>>> >
>>>> >               if (num_vfs) {
>>>> >                       mlx4_warn(dev, "Enabling SR-IOV with %d VFs\n",num_vfs);
>>>> > +
>>>> > +                     atomic_inc(&pf_loading);
>>>> >                       err = pci_enable_sriov(pdev, num_vfs);
>>>> > +                     atomic_dec(&pf_loading);
>>>> > +
> 
>>>> This synchronization scheme doesn't look right to me at all.
>>>> It's global, so VF's for _any_ PF will probe defer while one is enabling SRIOV.
>>>> It doesn't seem correct to cause unrelated VF's to defer the probe.
> 
>>> Can you please elaborate a bit why you find this approach to be
>>> incorrect? basically, these nested VF probed are a bit headache
>>> anyway, so we didn't find such global deferring to be problematic.
> 
>> What if a second PF starts to init and call pci_enable_sriov(), while the VFs
>> from a previous PF probed call mlx4_init_slave()?
>> It will increment pf_loading() and force those unreladed VFs to defer.
> 
> By "unreladed VFs" I assume you mean unrelated VFs that belong to the
> 1st VF, which is OK for them to probe, right? so yes, this is sort of
> conservative approach that wait till all PFs are fully ready, and I
> understand you don't like it, but still, I would be happy to know
> what's wrong in doing so..

My understanding is that the relationship between these devices is:

	PF --> VF1, VF2, VF3, ...

and these VF children are (essentially) instantiated by
pci_enable_sriov() calls.

Therefore if we:

	probe PF1

we go:

	pf_loading++
	pci_enable_sriov();
	PF1_VF1 defers
	PF1_VF2 defers
	PF1_VF3 defers
	...
	pf_loading--

next:

	probe PF2

	pf_loading++
..

at this point any attempt of PF1's VFs to init will defer, what will
cause them to properly retry that init?

Re: [PATCH net 2/2] net/mlx4_core: mlx4_init_slave() shouldn't access comm channel before PF is ready

[Or Gerlitz]

On Thu, Mar 6, 2014 at 11:12 PM, David Miller <davem@davemloft.net> wrote:
> From: Or Gerlitz <or.gerlitz@gmail.com>
> Date: Thu, 6 Mar 2014 23:08:36 +0200
>> On Thu, Mar 6, 2014 at 10:48 PM, David Miller <davem@davemloft.net> wrote:
>>> From: Or Gerlitz <or.gerlitz@gmail.com>
>>> Date: Thu, 6 Mar 2014 22:19:48 +0200

>>>>> This synchronization scheme doesn't look right to me at all.
>>>>> It's global, so VF's for _any_ PF will probe defer while one is enabling SRIOV.
>>>>> It doesn't seem correct to cause unrelated VF's to defer the probe.
>>>> Can you please elaborate a bit why you find this approach to be
>>>> incorrect? basically, these nested VF probed are a bit headache
>>>> anyway, so we didn't find such global deferring to be problematic.

>>> What if a second PF starts to init and call pci_enable_sriov(), while the VFs
>>> from a previous PF probed call mlx4_init_slave()?
>>> It will increment pf_loading() and force those unreladed VFs to defer.

>> By "unreladed VFs" I assume you mean unrelated VFs that belong to the
>> 1st VF, which is OK for them to probe, right? so yes, this is sort of
>> conservative approach that wait till all PFs are fully ready, and I
>> understand you don't like it, but still, I would be happy to know
>> what's wrong in doing so..

> My understanding is that the relationship between these devices is:
>         PF --> VF1, VF2, VF3, ...
>
> and these VF children are (essentially) instantiated by
> pci_enable_sriov() calls.
>
> Therefore if we:
>
>         probe PF1
>
> we go:
>
>         pf_loading++
>         pci_enable_sriov();
>         PF1_VF1 defers
>         PF1_VF2 defers
>         PF1_VF3 defers
>         ...
>         pf_loading--
>
> next:
>
>         probe PF2
>
>         pf_loading++
> ..

correct, that would be the situation

> at this point any attempt of PF1's VFs to init will defer, what will
> cause them to properly retry that init?

So... we were thinking that there is a mechanism that causes them to
retry that init as long as they return -EPROBE_DEFER or they succeed,
isn't that the case?

Re: [PATCH net 2/2] net/mlx4_core: mlx4_init_slave() shouldn't access comm channel before PF is ready

[David Miller]

From: Or Gerlitz <or.gerlitz@gmail.com>
Date: Thu, 6 Mar 2014 23:58:19 +0200

> So... we were thinking that there is a mechanism that causes them to
> retry that init as long as they return -EPROBE_DEFER or they succeed,
> isn't that the case?

Indeed.  When the PF returns from it's probe, all VFs that deferred
will retry.

Both patches applied, thanks.