[PATCH] mlx4_core: Fix crash on uninitialized priv->cmd.slave_sem

[PATCH] mlx4_core: Fix crash on uninitialized priv->cmd.slave_sem

[Roland Dreier]

From: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>

On an SR-IOV master device, __mlx4_init_one() calls mlx4_init_hca()
before mlx4_multi_func_init().  However, for unlucky configurations,
mlx4_init_hca() might call mlx4_SENSE_PORT() (via mlx4_dev_cap()), and
that calls mlx4_cmd_imm() with MLX4_CMD_WRAPPED set.

However, on a multifunction device with MLX4_CMD_WRAPPED, __mlx4_cmd()
calls into mlx4_slave_cmd(), and that immediately tries to do

	down(&priv->cmd.slave_sem);

but priv->cmd.slave_sem isn't initialized until mlx4_multi_func_init()
(which we haven't called yet).  The next thing it tries to do is access
priv->mfunc.vhcr, but that hasn't been allocated yet.

Fix this by moving the initialization of slave_sem and vhcr up into
mlx4_cmd_init(). Also, since slave_sem is really just being used as a
mutex, convert it into a slave_cmd_mutex.

Signed-off-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
Jack, I needed this to get my (CX3 w/ FW 2.11.500) adapter to work in
SR-IOV mode.  Is it possible you never tested SR-IOV on an adapter
with ports in autosensing mode?

 drivers/net/ethernet/mellanox/mlx4/cmd.c  |   51 ++++++++++++++++++-----------
 drivers/net/ethernet/mellanox/mlx4/main.c |   11 ++++---
 drivers/net/ethernet/mellanox/mlx4/mlx4.h |    2 +-
 3 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/cmd.c b/drivers/net/ethernet/mellanox/mlx4/cmd.c
index 90774b7..3d1899f 100644
--- a/drivers/net/ethernet/mellanox/mlx4/cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx4/cmd.c
@@ -395,7 +395,8 @@ static int mlx4_slave_cmd(struct mlx4_dev *dev, u64 in_param, u64 *out_param,
 	struct mlx4_vhcr_cmd *vhcr = priv->mfunc.vhcr;
 	int ret;
 
-	down(&priv->cmd.slave_sem);
+	mutex_lock(&priv->cmd.slave_cmd_mutex);
+
 	vhcr->in_param = cpu_to_be64(in_param);
 	vhcr->out_param = out_param ? cpu_to_be64(*out_param) : 0;
 	vhcr->in_modifier = cpu_to_be32(in_modifier);
@@ -403,6 +404,7 @@ static int mlx4_slave_cmd(struct mlx4_dev *dev, u64 in_param, u64 *out_param,
 	vhcr->token = cpu_to_be16(CMD_POLL_TOKEN);
 	vhcr->status = 0;
 	vhcr->flags = !!(priv->cmd.use_events) << 6;
+
 	if (mlx4_is_master(dev)) {
 		ret = mlx4_master_process_vhcr(dev, dev->caps.function, vhcr);
 		if (!ret) {
@@ -439,7 +441,8 @@ static int mlx4_slave_cmd(struct mlx4_dev *dev, u64 in_param, u64 *out_param,
 			mlx4_err(dev, "failed execution of VHCR_POST command"
 				 "opcode 0x%x\n", op);
 	}
-	up(&priv->cmd.slave_sem);
+
+	mutex_unlock(&priv->cmd.slave_cmd_mutex);
 	return ret;
 }
 
@@ -1559,14 +1562,15 @@ static void mlx4_master_do_cmd(struct mlx4_dev *dev, int slave, u8 cmd,
 		if ((slave_state[slave].last_cmd != MLX4_COMM_CMD_VHCR_EN) &&
 		    (slave_state[slave].last_cmd != MLX4_COMM_CMD_VHCR_POST))
 			goto reset_slave;
-		down(&priv->cmd.slave_sem);
+
+		mutex_lock(&priv->cmd.slave_cmd_mutex);
 		if (mlx4_master_process_vhcr(dev, slave, NULL)) {
 			mlx4_err(dev, "Failed processing vhcr for slave:%d,"
 				 " resetting slave.\n", slave);
-			up(&priv->cmd.slave_sem);
+			mutex_unlock(&priv->cmd.slave_cmd_mutex);
 			goto reset_slave;
 		}
-		up(&priv->cmd.slave_sem);
+		mutex_unlock(&priv->cmd.slave_cmd_mutex);
 		break;
 	default:
 		mlx4_warn(dev, "Bad comm cmd:%d from slave:%d\n", cmd, slave);
@@ -1707,14 +1711,6 @@ int mlx4_multi_func_init(struct mlx4_dev *dev)
 	struct mlx4_slave_state *s_state;
 	int i, j, err, port;
 
-	priv->mfunc.vhcr = dma_alloc_coherent(&(dev->pdev->dev), PAGE_SIZE,
-					    &priv->mfunc.vhcr_dma,
-					    GFP_KERNEL);
-	if (!priv->mfunc.vhcr) {
-		mlx4_err(dev, "Couldn't allocate vhcr.\n");
-		return -ENOMEM;
-	}
-
 	if (mlx4_is_master(dev))
 		priv->mfunc.comm =
 		ioremap(pci_resource_start(dev->pdev, priv->fw.comm_bar) +
@@ -1777,7 +1773,6 @@ int mlx4_multi_func_init(struct mlx4_dev *dev)
 		if (mlx4_init_resource_tracker(dev))
 			goto err_thread;
 
-		sema_init(&priv->cmd.slave_sem, 1);
 		err = mlx4_ARM_COMM_CHANNEL(dev);
 		if (err) {
 			mlx4_err(dev, " Failed to arm comm channel eq: %x\n",
@@ -1791,8 +1786,6 @@ int mlx4_multi_func_init(struct mlx4_dev *dev)
 			mlx4_err(dev, "Couldn't sync toggles\n");
 			goto err_comm;
 		}
-
-		sema_init(&priv->cmd.slave_sem, 1);
 	}
 	return 0;
 
@@ -1822,6 +1815,7 @@ int mlx4_cmd_init(struct mlx4_dev *dev)
 	struct mlx4_priv *priv = mlx4_priv(dev);
 
 	mutex_init(&priv->cmd.hcr_mutex);
+	mutex_init(&priv->cmd.slave_cmd_mutex);
 	sema_init(&priv->cmd.poll_sem, 1);
 	priv->cmd.use_events = 0;
 	priv->cmd.toggle     = 1;
@@ -1838,14 +1832,30 @@ int mlx4_cmd_init(struct mlx4_dev *dev)
 		}
 	}
 
+	if (mlx4_is_mfunc(dev)) {
+		priv->mfunc.vhcr = dma_alloc_coherent(&(dev->pdev->dev), PAGE_SIZE,
+						      &priv->mfunc.vhcr_dma,
+						      GFP_KERNEL);
+		if (!priv->mfunc.vhcr) {
+			mlx4_err(dev, "Couldn't allocate VHCR.\n");
+			goto err_hcr;
+		}
+	}
+
 	priv->cmd.pool = pci_pool_create("mlx4_cmd", dev->pdev,
 					 MLX4_MAILBOX_SIZE,
 					 MLX4_MAILBOX_SIZE, 0);
 	if (!priv->cmd.pool)
-		goto err_hcr;
+		goto err_vhcr;
 
 	return 0;
 
+err_vhcr:
+	if (mlx4_is_mfunc(dev))
+		dma_free_coherent(&(dev->pdev->dev), PAGE_SIZE,
+				  priv->mfunc.vhcr, priv->mfunc.vhcr_dma);
+	priv->mfunc.vhcr = NULL;
+
 err_hcr:
 	if (!mlx4_is_slave(dev))
 		iounmap(priv->cmd.hcr);
@@ -1868,9 +1878,6 @@ void mlx4_multi_func_cleanup(struct mlx4_dev *dev)
 	}
 
 	iounmap(priv->mfunc.comm);
-	dma_free_coherent(&(dev->pdev->dev), PAGE_SIZE,
-		     priv->mfunc.vhcr, priv->mfunc.vhcr_dma);
-	priv->mfunc.vhcr = NULL;
 }
 
 void mlx4_cmd_cleanup(struct mlx4_dev *dev)
@@ -1881,6 +1888,10 @@ void mlx4_cmd_cleanup(struct mlx4_dev *dev)
 
 	if (!mlx4_is_slave(dev))
 		iounmap(priv->cmd.hcr);
+	if (mlx4_is_mfunc(dev))
+		dma_free_coherent(&(dev->pdev->dev), PAGE_SIZE,
+				  priv->mfunc.vhcr, priv->mfunc.vhcr_dma);
+	priv->mfunc.vhcr = NULL;
 }
 
 /*
diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index 8d3b475..9f06e04 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -1159,10 +1159,10 @@ static void mlx4_slave_exit(struct mlx4_dev *dev)
 {
 	struct mlx4_priv *priv = mlx4_priv(dev);
 
-	down(&priv->cmd.slave_sem);
+	mutex_lock(&priv->cmd.slave_cmd_mutex);
 	if (mlx4_comm_cmd(dev, MLX4_COMM_CMD_RESET, 0, MLX4_COMM_TIME))
 		mlx4_warn(dev, "Failed to close slave function.\n");
-	up(&priv->cmd.slave_sem);
+	mutex_unlock(&priv->cmd.slave_cmd_mutex);
 }
 
 static int map_bf_area(struct mlx4_dev *dev)
@@ -1214,7 +1214,7 @@ static int mlx4_init_slave(struct mlx4_dev *dev)
 	u32 slave_read;
 	u32 cmd_channel_ver;
 
-	down(&priv->cmd.slave_sem);
+	mutex_lock(&priv->cmd.slave_cmd_mutex);
 	priv->cmd.max_cmds = 1;
 	mlx4_warn(dev, "Sending reset\n");
 	ret_from_reset = mlx4_comm_cmd(dev, MLX4_COMM_CMD_RESET, 0,
@@ -1263,12 +1263,13 @@ static int mlx4_init_slave(struct mlx4_dev *dev)
 		goto err;
 	if (mlx4_comm_cmd(dev, MLX4_COMM_CMD_VHCR_EN, dma, MLX4_COMM_TIME))
 		goto err;
-	up(&priv->cmd.slave_sem);
+
+	mutex_unlock(&priv->cmd.slave_cmd_mutex);
 	return 0;
 
 err:
 	mlx4_comm_cmd(dev, MLX4_COMM_CMD_RESET, 0, 0);
-	up(&priv->cmd.slave_sem);
+	mutex_unlock(&priv->cmd.slave_cmd_mutex);
 	return -EIO;
 }
 
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4.h b/drivers/net/ethernet/mellanox/mlx4/mlx4.h
index e5eee31..1bb3e5e 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4.h
@@ -513,9 +513,9 @@ struct mlx4_cmd {
 	struct pci_pool	       *pool;
 	void __iomem	       *hcr;
 	struct mutex		hcr_mutex;
+	struct mutex		slave_cmd_mutex;
 	struct semaphore	poll_sem;
 	struct semaphore	event_sem;
-	struct semaphore	slave_sem;
 	int			max_cmds;
 	spinlock_t		context_lock;
 	int			free_head;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mlx4_core: Fix crash on uninitialized priv->cmd.slave_sem

[Roland Dreier]

By the way, I still get a steady stream of

mlx4_core 0000:05:00.0: Unknown command:0x4d accepted from slave:0
mlx4_core 0000:05:00.0: Sense command failed for port: 1

once I load the driver... it seems SENSE_PORT is a wrapped command
but there's no entry in cmd_info[] for it?

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mlx4_core: Fix crash on uninitialized priv->cmd.slave_sem

[Roland Dreier]

On Tue, Sep 25, 2012 at 9:46 PM, Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> By the way, I still get a steady stream of
>
> mlx4_core 0000:05:00.0: Unknown command:0x4d accepted from slave:0
> mlx4_core 0000:05:00.0: Sense command failed for port: 1
>
> once I load the driver... it seems SENSE_PORT is a wrapped command
> but there's no entry in cmd_info[] for it?

I guess Jack is on vacation until next week.  Or, anyone else who knows
anything about this stuff?

In particular I'm planning on including the patch in this thread in my pull
request with SR-IOV in it -- the patch is the difference between crashing
on driver load and mostly working on my setup.

But it seems you guys don't use SR-IOV in setups where the driver
calls SENSE_PORT?  Is something different about my test setup?

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mlx4_core: Fix crash on uninitialized priv->cmd.slave_sem

[Or Gerlitz]

On Wed, Sep 26, 2012 at 6:42 AM, Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> From: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
>
> On an SR-IOV master device, __mlx4_init_one() calls mlx4_init_hca()
> before mlx4_multi_func_init().  However, for unlucky configurations,
> mlx4_init_hca() might call mlx4_SENSE_PORT() (via mlx4_dev_cap()), and
> that calls mlx4_cmd_imm() with MLX4_CMD_WRAPPED set.
>
> However, on a multifunction device with MLX4_CMD_WRAPPED, __mlx4_cmd()
> calls into mlx4_slave_cmd(), and that immediately tries to do
>
>         down(&priv->cmd.slave_sem);
>
> but priv->cmd.slave_sem isn't initialized until mlx4_multi_func_init()
> (which we haven't called yet).  The next thing it tries to do is access
> priv->mfunc.vhcr, but that hasn't been allocated yet.
>
> Fix this by moving the initialization of slave_sem and vhcr up into
> mlx4_cmd_init(). Also, since slave_sem is really just being used as a
> mutex, convert it into a slave_cmd_mutex.
>
> Signed-off-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
> ---
> Jack, I needed this to get my (CX3 w/ FW 2.11.500) adapter to work in
> SR-IOV mode.  Is it possible you never tested SR-IOV on an adapter
> with ports in autosensing mode?

Roland,

What exactly did you mean by saying "for unlucky configurations"
above? what value did you use for mlx4_core's port_array_type module
param?

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mlx4_core: Fix crash on uninitialized priv->cmd.slave_sem

[Roland Dreier]

On Wed, Sep 26, 2012 at 2:51 PM, Or Gerlitz <or.gerlitz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> What exactly did you mean by saying "for unlucky configurations"
> above? what value did you use for mlx4_core's port_array_type module
> param?

I didn't set the parameter at all.  What I meant by "unlucky configs" was
all the conditions in the following code in mlx4_dev_cap():

		/*
		 * Link sensing is allowed on the port if 3 conditions are true:
		 * 1. Both protocols are supported on the port.
		 * 2. Different types are supported on the port
		 * 3. FW declared that it supports link sensing
		 */
		mlx4_priv(dev)->sense.sense_allowed[i] =
			((dev->caps.supported_type[i] == MLX4_PORT_TYPE_AUTO) &&
			 (dev->caps.flags & MLX4_DEV_CAP_FLAG_DPDP) &&
			 (dev->caps.flags & MLX4_DEV_CAP_FLAG_SENSE_SUPPORT));

		/*
		 * If "default_sense" bit is set, we move the port to "AUTO" mode
		 * and perform sense_port FW command to try and set the correct
		 * port type from beginning
		 */
		if (mlx4_priv(dev)->sense.sense_allowed[i] && dev->caps.default_sense[i]) {
			enum mlx4_port_type sensed_port = MLX4_PORT_TYPE_NONE;
			dev->caps.possible_type[i] = MLX4_PORT_TYPE_AUTO;
			mlx4_SENSE_PORT(dev, i, &sensed_port);
			if (sensed_port != MLX4_PORT_TYPE_NONE)
				dev->caps.port_type[i] = sensed_port;
		} else {

I'm beginning to think it might be that I had nothing hooked up
to my adapter initially, which seems to lead to the fatal SENSE_PORT
call here.  I notice that once I bring up the link, the stream of failing
SENSE_PORT calls (which don't ever work in SR-IOV mode
AFAICT) stops as well.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mlx4_core: Fix crash on uninitialized priv->cmd.slave_sem

[Or Gerlitz]

On 27/09/2012 08:46, Roland Dreier wrote:
> On Wed, Sep 26, 2012 at 2:51 PM, Or Gerlitz <or.gerlitz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> What exactly did you mean by saying "for unlucky configurations"
>> above? what value did you use for mlx4_core's port_array_type module
>> param?
> I didn't set the parameter at all.  What I meant by "unlucky configs" was
> all the conditions in the following code in mlx4_dev_cap():
>
> 		/*
> 		 * Link sensing is allowed on the port if 3 conditions are true:
> 		 * 1. Both protocols are supported on the port.
> 		 * 2. Different types are supported on the port
> 		 * 3. FW declared that it supports link sensing
> 		 */
> 		mlx4_priv(dev)->sense.sense_allowed[i] =
> 			((dev->caps.supported_type[i] == MLX4_PORT_TYPE_AUTO) &&
> 			 (dev->caps.flags & MLX4_DEV_CAP_FLAG_DPDP) &&
> 			 (dev->caps.flags & MLX4_DEV_CAP_FLAG_SENSE_SUPPORT));
>
> 		/*
> 		 * If "default_sense" bit is set, we move the port to "AUTO" mode
> 		 * and perform sense_port FW command to try and set the correct
> 		 * port type from beginning
> 		 */
> 		if (mlx4_priv(dev)->sense.sense_allowed[i] && dev->caps.default_sense[i]) {
> 			enum mlx4_port_type sensed_port = MLX4_PORT_TYPE_NONE;
> 			dev->caps.possible_type[i] = MLX4_PORT_TYPE_AUTO;
> 			mlx4_SENSE_PORT(dev, i, &sensed_port);
> 			if (sensed_port != MLX4_PORT_TYPE_NONE)
> 				dev->caps.port_type[i] = sensed_port;
> 		} else {
>
> I'm beginning to think it might be that I had nothing hooked up
> to my adapter initially, which seems to lead to the fatal SENSE_PORT
> call here.  I notice that once I bring up the link, the stream of failing
> SENSE_PORT calls (which don't ever work in SR-IOV mode AFAICT) stops as well.
>
>

Roland,

As I wrote you, the team was off over the last two days (*) and we will 
do some more detailed checks
to understand what is missing in the code, on the architecture level 
auto-sensing for link type isn't
supported in the current code also for the PPF in SRIOV mode, so at 
first sight, the above code
snippest seems  to miss checking if we're on "native" or mfunc mode 
before allowing itself to
go into auto-sense mode, but again, let us look there deeper. What do 
you mean by "I had nothing hooked
up to my adapter initially"?

Or.

(*) BTW - unluckily we're on a HHS {High Holiday Season} now, in 
conjunction with the coming opening
of the 3.7 merge window on the weekend, which means that many people are 
mostly off next week as
well, Jack will be around from Tuesday.



--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mlx4_core: Fix crash on uninitialized priv->cmd.slave_sem

[Or Gerlitz]

On 27/09/2012 10:17, Roland Dreier wrote:
> I think I had it cabled up directly to another HCA, and that HCA was in a
> system that was either off or at least didn't have the driver loaded.  So the
> port was in the physically DOWN state...
>
> However, I just tried it and even with that other HCA enabled (and running
> opensm), I still see
>
>      mlx4_core 0000:05:00.0: Unknown command:0x4d accepted from slave:0
>      mlx4_core 0000:05:00.0: Sense command failed for port: 1
>
> before init_hca finishes, so I think I would have crashed without my patch
> still.  Not sure I understand how you guys miss hitting this fatal call to
> SENSE_PORT during init (or what the intention for SENSE_PORT is
> since right now in SR-IOV mode it always fails due to no wrapper).
>
> I'm not doing anything funny with the port type, I have a one-port CX3
> running FW built with the exact default .ini file, except for enabling SR-IOV.
> And the only mlx4_core module parameter I'm passing in is "num_vfs=1".
>

Thanks for the further details, sounds like something/s is indeed broken 
here, in the code and/or
in the regression testing of it. We'll do our best to provide deeper 
reasoning of what's going on today,
and if not, by Tuesday when Jack is back. This way (your patch) or 
another (some other direction suggested
by the team or Jack) seems we're safe for the merge window.

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mlx4_core: Fix crash on uninitialized priv->cmd.slave_sem

[Or Gerlitz]

On 27/09/2012 08:46, Roland Dreier wrote:
> On Wed, Sep 26, 2012 at 2:51 PM, Or Gerlitz <or.gerlitz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> What exactly did you mean by saying "for unlucky configurations"
>> above? what value did you use for mlx4_core's port_array_type module
>> param?
> I didn't set the parameter at all.  What I meant by "unlucky configs" was
> all the conditions in the following code in mlx4_dev_cap():
>
> 		/*
> 		 * Link sensing is allowed on the port if 3 conditions are true:
> 		 * 1. Both protocols are supported on the port.
> 		 * 2. Different types are supported on the port
> 		 * 3. FW declared that it supports link sensing
> 		 */
> 		mlx4_priv(dev)->sense.sense_allowed[i] =
> 			((dev->caps.supported_type[i] == MLX4_PORT_TYPE_AUTO) &&
> 			 (dev->caps.flags & MLX4_DEV_CAP_FLAG_DPDP) &&
> 			 (dev->caps.flags & MLX4_DEV_CAP_FLAG_SENSE_SUPPORT));
>
> 		/*
> 		 * If "default_sense" bit is set, we move the port to "AUTO" mode
> 		 * and perform sense_port FW command to try and set the correct
> 		 * port type from beginning
> 		 */
> 		if (mlx4_priv(dev)->sense.sense_allowed[i] && dev->caps.default_sense[i]) {
> 			enum mlx4_port_type sensed_port = MLX4_PORT_TYPE_NONE;
> 			dev->caps.possible_type[i] = MLX4_PORT_TYPE_AUTO;
> 			mlx4_SENSE_PORT(dev, i, &sensed_port);
> 			if (sensed_port != MLX4_PORT_TYPE_NONE)
> 				dev->caps.port_type[i] = sensed_port;
> 		} else {

Roland,

Looking deeper on the issues you reported, we noted that the bug is up 
here on the RHS of

if (mlx4_priv(dev)->sense.sense_allowed[i] && dev->caps.default_sense[i])

as when a firmware sets the “do_sense” bit as 1 (which is the default in 
2.11.0500), the driver begins to do sensing. This firmware somehow went 
out without enough mfunc checking, bad. A fast solution would be a one 
liner patch that forces the LHS of this line to be false under mfunc 
mode, that is add forth condition to the three conditions above which is 
&& !mfunc

In the overall driver architecture level, auto-sending can/will be abled 
in later state, when the PPF
can signal the VFs to reset/re-create their mlx4 device when the link 
type change, as done in native
mode.

Or.



> I'm beginning to think it might be that I had nothing hooked up
> to my adapter initially, which seems to lead to the fatal SENSE_PORT
> call here.  I notice that once I bring up the link, the stream of failing
> SENSE_PORT calls (which don't ever work in SR-IOV mode AFAICT) stops as well.
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mlx4_core: Fix crash on uninitialized priv->cmd.slave_sem

[Jack Morgenstein]

Acked-by: Jack Morgenstein <jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>

Thanks, Roland! Good catches and good fixes!

Regarding the mutex replacing the semaphore, at one time we toyed
with the idea of multiple comm channel commands "in the air", but
we did not pursue the idea.

I agree with changing slave_sem to a mutex.  If we do decide at some point
to do multiple comm channel commands, we will change it back.

Regarding moving initialization of slave_sem and vhcr, your later patches
render it unnecessary.  However, I agree with this change -- this fixes
an "Oops waiting to happen". Though the change is no longer needed now, it
will be if SENSE_PORT is allowed in multifunction, and then we WILL start
experiencing the same Oops as we did before your patches disabled
SENSE_PORT for multifunction. Fixes like these are too easy to overlook.

-Jack

On Wednesday 26 September 2012 06:42, Roland Dreier wrote:
> From: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
> 
> On an SR-IOV master device, __mlx4_init_one() calls mlx4_init_hca()
> before mlx4_multi_func_init().  However, for unlucky configurations,
> mlx4_init_hca() might call mlx4_SENSE_PORT() (via mlx4_dev_cap()), and
> that calls mlx4_cmd_imm() with MLX4_CMD_WRAPPED set.
> 
> However, on a multifunction device with MLX4_CMD_WRAPPED, __mlx4_cmd()
> calls into mlx4_slave_cmd(), and that immediately tries to do
> 
> 	down(&priv->cmd.slave_sem);
> 
> but priv->cmd.slave_sem isn't initialized until mlx4_multi_func_init()
> (which we haven't called yet).  The next thing it tries to do is access
> priv->mfunc.vhcr, but that hasn't been allocated yet.
> 
> Fix this by moving the initialization of slave_sem and vhcr up into
> mlx4_cmd_init(). Also, since slave_sem is really just being used as a
> mutex, convert it into a slave_cmd_mutex.
> 
> Signed-off-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
> ---
> Jack, I needed this to get my (CX3 w/ FW 2.11.500) adapter to work in
> SR-IOV mode.  Is it possible you never tested SR-IOV on an adapter
> with ports in autosensing mode?
> 
>  drivers/net/ethernet/mellanox/mlx4/cmd.c  |   51 ++++++++++++++++++-----------
>  drivers/net/ethernet/mellanox/mlx4/main.c |   11 ++++---
>  drivers/net/ethernet/mellanox/mlx4/mlx4.h |    2 +-
>  3 files changed, 38 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/cmd.c b/drivers/net/ethernet/mellanox/mlx4/cmd.c
> index 90774b7..3d1899f 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/cmd.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/cmd.c
> @@ -395,7 +395,8 @@ static int mlx4_slave_cmd(struct mlx4_dev *dev, u64 in_param, u64 *out_param,
>  	struct mlx4_vhcr_cmd *vhcr = priv->mfunc.vhcr;
>  	int ret;
>  
> -	down(&priv->cmd.slave_sem);
> +	mutex_lock(&priv->cmd.slave_cmd_mutex);
> +
>  	vhcr->in_param = cpu_to_be64(in_param);
>  	vhcr->out_param = out_param ? cpu_to_be64(*out_param) : 0;
>  	vhcr->in_modifier = cpu_to_be32(in_modifier);
> @@ -403,6 +404,7 @@ static int mlx4_slave_cmd(struct mlx4_dev *dev, u64 in_param, u64 *out_param,
>  	vhcr->token = cpu_to_be16(CMD_POLL_TOKEN);
>  	vhcr->status = 0;
>  	vhcr->flags = !!(priv->cmd.use_events) << 6;
> +
>  	if (mlx4_is_master(dev)) {
>  		ret = mlx4_master_process_vhcr(dev, dev->caps.function, vhcr);
>  		if (!ret) {
> @@ -439,7 +441,8 @@ static int mlx4_slave_cmd(struct mlx4_dev *dev, u64 in_param, u64 *out_param,
>  			mlx4_err(dev, "failed execution of VHCR_POST command"
>  				 "opcode 0x%x\n", op);
>  	}
> -	up(&priv->cmd.slave_sem);
> +
> +	mutex_unlock(&priv->cmd.slave_cmd_mutex);
>  	return ret;
>  }
>  
> @@ -1559,14 +1562,15 @@ static void mlx4_master_do_cmd(struct mlx4_dev *dev, int slave, u8 cmd,
>  		if ((slave_state[slave].last_cmd != MLX4_COMM_CMD_VHCR_EN) &&
>  		    (slave_state[slave].last_cmd != MLX4_COMM_CMD_VHCR_POST))
>  			goto reset_slave;
> -		down(&priv->cmd.slave_sem);
> +
> +		mutex_lock(&priv->cmd.slave_cmd_mutex);
>  		if (mlx4_master_process_vhcr(dev, slave, NULL)) {
>  			mlx4_err(dev, "Failed processing vhcr for slave:%d,"
>  				 " resetting slave.\n", slave);
> -			up(&priv->cmd.slave_sem);
> +			mutex_unlock(&priv->cmd.slave_cmd_mutex);
>  			goto reset_slave;
>  		}
> -		up(&priv->cmd.slave_sem);
> +		mutex_unlock(&priv->cmd.slave_cmd_mutex);
>  		break;
>  	default:
>  		mlx4_warn(dev, "Bad comm cmd:%d from slave:%d\n", cmd, slave);
> @@ -1707,14 +1711,6 @@ int mlx4_multi_func_init(struct mlx4_dev *dev)
>  	struct mlx4_slave_state *s_state;
>  	int i, j, err, port;
>  
> -	priv->mfunc.vhcr = dma_alloc_coherent(&(dev->pdev->dev), PAGE_SIZE,
> -					    &priv->mfunc.vhcr_dma,
> -					    GFP_KERNEL);
> -	if (!priv->mfunc.vhcr) {
> -		mlx4_err(dev, "Couldn't allocate vhcr.\n");
> -		return -ENOMEM;
> -	}
> -
>  	if (mlx4_is_master(dev))
>  		priv->mfunc.comm =
>  		ioremap(pci_resource_start(dev->pdev, priv->fw.comm_bar) +
> @@ -1777,7 +1773,6 @@ int mlx4_multi_func_init(struct mlx4_dev *dev)
>  		if (mlx4_init_resource_tracker(dev))
>  			goto err_thread;
>  
> -		sema_init(&priv->cmd.slave_sem, 1);
>  		err = mlx4_ARM_COMM_CHANNEL(dev);
>  		if (err) {
>  			mlx4_err(dev, " Failed to arm comm channel eq: %x\n",
> @@ -1791,8 +1786,6 @@ int mlx4_multi_func_init(struct mlx4_dev *dev)
>  			mlx4_err(dev, "Couldn't sync toggles\n");
>  			goto err_comm;
>  		}
> -
> -		sema_init(&priv->cmd.slave_sem, 1);
>  	}
>  	return 0;
>  
> @@ -1822,6 +1815,7 @@ int mlx4_cmd_init(struct mlx4_dev *dev)
>  	struct mlx4_priv *priv = mlx4_priv(dev);
>  
>  	mutex_init(&priv->cmd.hcr_mutex);
> +	mutex_init(&priv->cmd.slave_cmd_mutex);
>  	sema_init(&priv->cmd.poll_sem, 1);
>  	priv->cmd.use_events = 0;
>  	priv->cmd.toggle     = 1;
> @@ -1838,14 +1832,30 @@ int mlx4_cmd_init(struct mlx4_dev *dev)
>  		}
>  	}
>  
> +	if (mlx4_is_mfunc(dev)) {
> +		priv->mfunc.vhcr = dma_alloc_coherent(&(dev->pdev->dev), PAGE_SIZE,
> +						      &priv->mfunc.vhcr_dma,
> +						      GFP_KERNEL);
> +		if (!priv->mfunc.vhcr) {
> +			mlx4_err(dev, "Couldn't allocate VHCR.\n");
> +			goto err_hcr;
> +		}
> +	}
> +
>  	priv->cmd.pool = pci_pool_create("mlx4_cmd", dev->pdev,
>  					 MLX4_MAILBOX_SIZE,
>  					 MLX4_MAILBOX_SIZE, 0);
>  	if (!priv->cmd.pool)
> -		goto err_hcr;
> +		goto err_vhcr;
>  
>  	return 0;
>  
> +err_vhcr:
> +	if (mlx4_is_mfunc(dev))
> +		dma_free_coherent(&(dev->pdev->dev), PAGE_SIZE,
> +				  priv->mfunc.vhcr, priv->mfunc.vhcr_dma);
> +	priv->mfunc.vhcr = NULL;
> +
>  err_hcr:
>  	if (!mlx4_is_slave(dev))
>  		iounmap(priv->cmd.hcr);
> @@ -1868,9 +1878,6 @@ void mlx4_multi_func_cleanup(struct mlx4_dev *dev)
>  	}
>  
>  	iounmap(priv->mfunc.comm);
> -	dma_free_coherent(&(dev->pdev->dev), PAGE_SIZE,
> -		     priv->mfunc.vhcr, priv->mfunc.vhcr_dma);
> -	priv->mfunc.vhcr = NULL;
>  }
>  
>  void mlx4_cmd_cleanup(struct mlx4_dev *dev)
> @@ -1881,6 +1888,10 @@ void mlx4_cmd_cleanup(struct mlx4_dev *dev)
>  
>  	if (!mlx4_is_slave(dev))
>  		iounmap(priv->cmd.hcr);
> +	if (mlx4_is_mfunc(dev))
> +		dma_free_coherent(&(dev->pdev->dev), PAGE_SIZE,
> +				  priv->mfunc.vhcr, priv->mfunc.vhcr_dma);
> +	priv->mfunc.vhcr = NULL;
>  }
>  
>  /*
> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
> index 8d3b475..9f06e04 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
> @@ -1159,10 +1159,10 @@ static void mlx4_slave_exit(struct mlx4_dev *dev)
>  {
>  	struct mlx4_priv *priv = mlx4_priv(dev);
>  
> -	down(&priv->cmd.slave_sem);
> +	mutex_lock(&priv->cmd.slave_cmd_mutex);
>  	if (mlx4_comm_cmd(dev, MLX4_COMM_CMD_RESET, 0, MLX4_COMM_TIME))
>  		mlx4_warn(dev, "Failed to close slave function.\n");
> -	up(&priv->cmd.slave_sem);
> +	mutex_unlock(&priv->cmd.slave_cmd_mutex);
>  }
>  
>  static int map_bf_area(struct mlx4_dev *dev)
> @@ -1214,7 +1214,7 @@ static int mlx4_init_slave(struct mlx4_dev *dev)
>  	u32 slave_read;
>  	u32 cmd_channel_ver;
>  
> -	down(&priv->cmd.slave_sem);
> +	mutex_lock(&priv->cmd.slave_cmd_mutex);
>  	priv->cmd.max_cmds = 1;
>  	mlx4_warn(dev, "Sending reset\n");
>  	ret_from_reset = mlx4_comm_cmd(dev, MLX4_COMM_CMD_RESET, 0,
> @@ -1263,12 +1263,13 @@ static int mlx4_init_slave(struct mlx4_dev *dev)
>  		goto err;
>  	if (mlx4_comm_cmd(dev, MLX4_COMM_CMD_VHCR_EN, dma, MLX4_COMM_TIME))
>  		goto err;
> -	up(&priv->cmd.slave_sem);
> +
> +	mutex_unlock(&priv->cmd.slave_cmd_mutex);
>  	return 0;
>  
>  err:
>  	mlx4_comm_cmd(dev, MLX4_COMM_CMD_RESET, 0, 0);
> -	up(&priv->cmd.slave_sem);
> +	mutex_unlock(&priv->cmd.slave_cmd_mutex);
>  	return -EIO;
>  }
>  
> diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4.h b/drivers/net/ethernet/mellanox/mlx4/mlx4.h
> index e5eee31..1bb3e5e 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/mlx4.h
> +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4.h
> @@ -513,9 +513,9 @@ struct mlx4_cmd {
>  	struct pci_pool	       *pool;
>  	void __iomem	       *hcr;
>  	struct mutex		hcr_mutex;
> +	struct mutex		slave_cmd_mutex;
>  	struct semaphore	poll_sem;
>  	struct semaphore	event_sem;
> -	struct semaphore	slave_sem;
>  	int			max_cmds;
>  	spinlock_t		context_lock;
>  	int			free_head;
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html