Re: [PATCH 3/5] net/mlx4: fix some error handling in mlx4_multi_func_init()
Ouch! Egg on my face! Sorry about that.
You are correct! while (--i >= 0) IS exactly equivalent to
while (i--). (the while condition is fully evaluated before the loop is
entered; pre or post increment only influences which value is tested
for true in the while condition -- the pre-value (with post-increment)
or the post-value (with pre-increment)).
In that case, my comment below regarding the double-free is also not
correct. Setting the freed pointer to NULL is not needed.
My bad. We should go with your format: while (i--)
-Jack
On Thu, 11 Feb 2016 11:29:43 +0200
Jack Morgenstein wrote:
> On Wed, 10 Feb 2016 19:15:20 +0100
> Rasmus Villemoes wrote:
>
> > On Wed, Feb 10 2016, Yishai Hadas
> > wrote:
> >
> > >> @@ -2429,7 +2429,7 @@ err_thread:
> > >> flush_workqueue(priv->mfunc.master.comm_wq);
> > >> destroy_workqueue(priv->mfunc.master.comm_wq);
> > >> err_slaves:
> > >> -while (--i) {
> > >> +while (i--) {
> > >
> > > This fix is wrong as it hits the case that i arrived the last
> > > value then below code will access to a non valid entry in the
> > > array.
> > >
> > > The expected fix should be:
> > > while (--i >= 0)
> > >
> >
> > Huh? They're completely equivalent (given that i is necessarily
> > non-negative before we evaluate the loop condition)
>
> No, they are not equivalent.
> if i == the max value (dev->num_slaves) when entering your proposed
> while loop, the kfree call index (i) will be out of range! This can
> happen, for example, if the failure occurs downstream from the "i"
> for-loop (e.g., if the call to mlx4_init_resource_tracker() fails).
>
> Therefore, we DO require the pre-decrement format. Therefore, the
> one-line fix proposed by Yishai is the correct fix.
> >. I don't really
> > care either way, but git grep says that 'while (i--)' is 5 times
> > more common than 'while (--i >= 0)'.
> Not relevant, while (i--) is simply not correct, because of the case
> where the for-loop involving i completes successfully and an error
> occurs later.
>
> FYI, you also had another bug in your solution -- a double-free when
> kzalloc for port 2 fails. For your code, you should also have reset
> s_state->vlan_filter[port] to NULL as shown below:
> for (port = 1; port <= MLX4_MAX_PORTS;
> port++) { struct mlx4_vport_state *admin_vport;
> struct mlx4_vport_state *oper_vport;
>
> s_state->vlan_filter[port] =
> kzalloc(sizeof(struct
> mlx4_vlan_fltr), GFP_KERNEL);
> if (!s_state->vlan_filter[port]) {
> if (--port) {
>
> kfree(s_state->vlan_filter[port]);
> ==> You should have added this
> s_state->vlan_filter[port] = NULL; }
> goto err_slaves;
> }
>
> However, again, the correct solution is to do what Yishai suggests:
> while (--i >= 0)
> so that if i is already zero the while-loop will not be entered.
>
> -Jack
> >
> > Rasmus
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-rdma" in the body of a message to majord...@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Re: [PATCH 3/5] net/mlx4: fix some error handling in mlx4_multi_func_init()
On Wed, 10 Feb 2016 19:15:20 +0100
Rasmus Villemoes wrote:
> On Wed, Feb 10 2016, Yishai Hadas wrote:
>
> >> @@ -2429,7 +2429,7 @@ err_thread:
> >>flush_workqueue(priv->mfunc.master.comm_wq);
> >>destroy_workqueue(priv->mfunc.master.comm_wq);
> >> err_slaves:
> >> - while (--i) {
> >> + while (i--) {
> >
> > This fix is wrong as it hits the case that i arrived the last value
> > then below code will access to a non valid entry in the array.
> >
> > The expected fix should be:
> > while (--i >= 0)
> >
>
> Huh? They're completely equivalent (given that i is necessarily
> non-negative before we evaluate the loop condition)
No, they are not equivalent.
if i == the max value (dev->num_slaves) when entering your proposed
while loop, the kfree call index (i) will be out of range! This can
happen, for example, if the failure occurs downstream from the "i"
for-loop (e.g., if the call to mlx4_init_resource_tracker() fails).
Therefore, we DO require the pre-decrement format. Therefore, the
one-line fix proposed by Yishai is the correct fix.
>. I don't really
> care either way, but git grep says that 'while (i--)' is 5 times more
> common than 'while (--i >= 0)'.
Not relevant, while (i--) is simply not correct, because of the case
where the for-loop involving i completes successfully and an error
occurs later.
FYI, you also had another bug in your solution -- a double-free when
kzalloc for port 2 fails. For your code, you should also have reset
s_state->vlan_filter[port] to NULL as shown below:
for (port = 1; port <= MLX4_MAX_PORTS; port++) {
struct mlx4_vport_state *admin_vport;
struct mlx4_vport_state *oper_vport;
s_state->vlan_filter[port] =
kzalloc(sizeof(struct
mlx4_vlan_fltr), GFP_KERNEL);
if (!s_state->vlan_filter[port]) {
if (--port) {
kfree(s_state->vlan_filter[port]);
==> You should have added this s_state->vlan_filter[port] =
NULL;
}
goto err_slaves;
}
However, again, the correct solution is to do what Yishai suggests:
while (--i >= 0)
so that if i is already zero the while-loop will not be entered.
-Jack
>
> Rasmus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma"
> in the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] infiniband/mlx4: check for mapping error
On Mon, 16 Mar 2015 18:49:59 +0100 (CET) Sebastian Ott wrote: > From: Sebastian Ott > To: linux-r...@vger.kernel.org, linux-kernel@vger.kernel.org > cc: Roland Dreier , Sean Hefty > , Hal Rosenstock , Or > Gerlitz , "David S. Miller" > , Yishai Hadas , Ira Weiny > , Jack Morgenstein , > Matan Barak , Moni Shoua , > Jiri Kosina Subject: [PATCH] infiniband/mlx4: check > for mapping error Date: Mon, 16 Mar 2015 18:49:59 +0100 (CET) > User-Agent: Alpine 2.11 (LFD 23 2013-08-11) Organization: "IBM > Deutschland Research & Development GmbH / Vorsitzende des > Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz > der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, > HRB 243294" > > > Since ib_dma_map_single can fail use ib_dma_mapping_error to check > for errors. > > Signed-off-by: Sebastian Ott Acked-by: Jack Morgenstein > --- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] net/mlx4_core: clean up cq_res_start_move_to()
ACK. OK.
-Jack
On Tue, 14 Jan 2014 20:45:36 +0100
Paul Bolle wrote:
> Building resource_tracker.o triggers a GCC warning:
> drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In
> function 'mlx4_HW2SW_CQ_wrapper':
> drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:3019:16:
> warning: 'cq' may be used uninitialized in this function
> [-Wmaybe-uninitialized] atomic_dec(&cq->mtt->ref_count); ^
>
> This is a false positive. But a cleanup of cq_res_start_move_to() can
> help GCC here. The code currently uses a switch statement where an
> if/else construct would do too, since only two of the switch's four
> cases can ever occur. Dropping that switch makes the warning go away.
>
> While we're at it, add some missing braces.
>
> Signed-off-by: Paul Bolle
> ---
> v2: adjust to Jack's review.
>
> .../net/ethernet/mellanox/mlx4/resource_tracker.c | 52
> -- 1 file changed, 19 insertions(+), 33
> deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c index
> 2f3f2bc..15cd659 100644 ---
> a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +++
> b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c @@ -1340,43
> +1340,29 @@ static int cq_res_start_move_to(struct mlx4_dev *dev, int
> slave, int cqn, spin_lock_irq(mlx4_tlock(dev));
> r = res_tracker_lookup(&tracker->res_tree[RES_CQ], cqn);
> - if (!r)
> + if (!r) {
> err = -ENOENT;
> - else if (r->com.owner != slave)
> + } else if (r->com.owner != slave) {
> err = -EPERM;
> - else {
> - switch (state) {
> - case RES_CQ_BUSY:
> - err = -EBUSY;
> - break;
> -
> - case RES_CQ_ALLOCATED:
> - if (r->com.state != RES_CQ_HW)
> - err = -EINVAL;
> - else if (atomic_read(&r->ref_count))
> - err = -EBUSY;
> - else
> - err = 0;
> - break;
> -
> - case RES_CQ_HW:
> - if (r->com.state != RES_CQ_ALLOCATED)
> - err = -EINVAL;
> - else
> - err = 0;
> - break;
> -
> - default:
> + } else if (state == RES_CQ_ALLOCATED) {
> + if (r->com.state != RES_CQ_HW)
> err = -EINVAL;
> - }
> + else if (atomic_read(&r->ref_count))
> + err = -EBUSY;
> + else
> + err = 0;
> + } else if (state != RES_CQ_HW || r->com.state !=
> RES_CQ_ALLOCATED) {
> + err = -EINVAL;
> + } else {
> + err = 0;
> + }
>
> - if (!err) {
> - r->com.from_state = r->com.state;
> - r->com.to_state = state;
> - r->com.state = RES_CQ_BUSY;
> - if (cq)
> - *cq = r;
> - }
> + if (!err) {
> + r->com.from_state = r->com.state;
> + r->com.to_state = state;
> + r->com.state = RES_CQ_BUSY;
> + if (cq)
> + *cq = r;
> }
>
> spin_unlock_irq(mlx4_tlock(dev));
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] net/mlx4_core: clean up srq_res_start_move_to()
ACK. OK.
-Jack
On Tue, 14 Jan 2014 20:46:52 +0100
Paul Bolle wrote:
> Building resource_tracker.o triggers a GCC warning:
> drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In
> function 'mlx4_HW2SW_SRQ_wrapper':
> drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:3202:17:
> warning: 'srq' may be used uninitialized in this function
> [-Wmaybe-uninitialized] atomic_dec(&srq->mtt->ref_count); ^
>
> This is a false positive. But a cleanup of srq_res_start_move_to() can
> help GCC here. The code currently uses a switch statement where a
> plain if/else would do, since only two of the switch's four cases can
> ever occur. Dropping that switch makes the warning go away.
>
> While we're at it, add some missing braces, and convert state to the
> correct type.
>
> Signed-off-by: Paul Bolle
> ---
> v2: adjust to Jack's review.
>
> .../net/ethernet/mellanox/mlx4/resource_tracker.c | 46
> -- 1 file changed, 16 insertions(+), 30
> deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c index
> 15cd659..4acd84c 100644 ---
> a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +++
> b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c @@ -1371,7
> +1371,7 @@ static int cq_res_start_move_to(struct mlx4_dev *dev, int
> slave, int cqn, }
> static int srq_res_start_move_to(struct mlx4_dev *dev, int slave,
> int index,
> - enum res_cq_states state, struct
> res_srq **srq)
> + enum res_srq_states state, struct
> res_srq **srq) {
> struct mlx4_priv *priv = mlx4_priv(dev);
> struct mlx4_resource_tracker *tracker =
> &priv->mfunc.master.res_tracker; @@ -1380,39 +1380,25 @@ static int
> srq_res_start_move_to(struct mlx4_dev *dev, int slave, int index,
> spin_lock_irq(mlx4_tlock(dev));
> r = res_tracker_lookup(&tracker->res_tree[RES_SRQ], index);
> - if (!r)
> + if (!r) {
> err = -ENOENT;
> - else if (r->com.owner != slave)
> + } else if (r->com.owner != slave) {
> err = -EPERM;
> - else {
> - switch (state) {
> - case RES_SRQ_BUSY:
> - err = -EINVAL;
> - break;
> -
> - case RES_SRQ_ALLOCATED:
> - if (r->com.state != RES_SRQ_HW)
> - err = -EINVAL;
> - else if (atomic_read(&r->ref_count))
> - err = -EBUSY;
> - break;
> -
> - case RES_SRQ_HW:
> - if (r->com.state != RES_SRQ_ALLOCATED)
> - err = -EINVAL;
> - break;
> -
> - default:
> + } else if (state == RES_SRQ_ALLOCATED) {
> + if (r->com.state != RES_SRQ_HW)
> err = -EINVAL;
> - }
> + else if (atomic_read(&r->ref_count))
> + err = -EBUSY;
> + } else if (state != RES_SRQ_HW || r->com.state !=
> RES_SRQ_ALLOCATED) {
> + err = -EINVAL;
> + }
>
> - if (!err) {
> - r->com.from_state = r->com.state;
> - r->com.to_state = state;
> - r->com.state = RES_SRQ_BUSY;
> - if (srq)
> - *srq = r;
> - }
> + if (!err) {
> + r->com.from_state = r->com.state;
> + r->com.to_state = state;
> + r->com.state = RES_SRQ_BUSY;
> + if (srq)
> + *srq = r;
> }
>
> spin_unlock_irq(mlx4_tlock(dev));
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] net/mlx4_core: clean up cq_res_start_move_to()
This time, replying to the list as well.
-Jack
P.S. sorry for the delay, I was swamped.
On Tue, 07 Jan 2014 14:01:18 +0100
Paul Bolle wrote:
> + } else {
> + /* state == RES_CQ_HW */
> + if (r->com.state != RES_CQ_ALLOCATED)
if (state != RES_CQ_HW || r->com.state != RES_CQ_ALLOCATED)
to protect against any bad calls to this function
(although I know that currently there are none).
This also preserves the behavior of the switch statement.
> err = -EINVAL;
> - }
> + else
> + err = 0;
> + }
>
> - if (!err) {
> - r->com.from_state = r->com.state;
> - r->com.to_state = state;
> - r->com.state = RES_CQ_BUSY;
> - if (cq)
> - *cq = r;
> - }
> + if (!err) {
> + r->com.from_state = r->com.state;
> + r->com.to_state = state;
> + r->com.state = RES_CQ_BUSY;
Please keep the "if" here. Protects against (future) bad calls.
> + *cq = r;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] net/mlx4_core: clean up srq_res_start_move_to()
On Tue, 07 Jan 2014 14:02:14 +0100
Paul Bolle wrote:
> diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c index
> a41f01e..8ace450 100644 ---
> a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +++
> b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c @@ -1372,7
> +1372,7 @@ static int cq_res_start_move_to(struct mlx4_dev *dev, int
> slave, int cqn, }
> static int srq_res_start_move_to(struct mlx4_dev *dev, int slave,
> int index,
> - enum res_cq_states state, struct res_srq **srq)
> + enum res_srq_states state, struct res_srq
> **srq) {
ACK
> + /* state == RES_SRQ_HW */
> + if (r->com.state != RES_SRQ_ALLOCATED)
if (state != RES_SRQ_HW || r->com.state != RES_SRQ_ALLOCATED)
> err = -EINVAL;
> - }
> + }
>
> - if (!err) {
> - r->com.from_state = r->com.state;
> - r->com.to_state = state;
> - r->com.state = RES_SRQ_BUSY;
> - if (srq)
> - *srq = r;
> - }
> + if (!err) {
> + r->com.from_state = r->com.state;
> + r->com.to_state = state;
> + r->com.state = RES_SRQ_BUSY;
please leave in the if (srq). Not currently needed, but if this
changes in the future, we will get an Oops.
> + *srq = r;
> }
>
> spin_unlock_irq(mlx4_tlock(dev));
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/25] IB/mlx4: fix error return code
On Sun, 29 Dec 2013 23:47:20 +0100
Julia Lawall wrote:
> diff --git a/drivers/infiniband/hw/mlx4/sysfs.c
> b/drivers/infiniband/hw/mlx4/sysfs.c index 97516eb..db2ea31 100644
> --- a/drivers/infiniband/hw/mlx4/sysfs.c
> +++ b/drivers/infiniband/hw/mlx4/sysfs.c
> @@ -582,8 +582,10 @@ static int add_port(struct mlx4_ib_dev *dev, int
> port_num, int slave) p->pkey_group.attrs =
> alloc_group_attrs(show_port_pkey, store_port_pkey,
> dev->dev->caps.pkey_table_len[port_num]);
> - if (!p->pkey_group.attrs)
> + if (!p->pkey_group.attrs) {
> + ret = -ENOMEM;
> goto err_alloc;
> + }
>
> ret = sysfs_create_group(&p->kobj, &p->pkey_group);
> if (ret)
> @@ -591,8 +593,10 @@ static int add_port(struct mlx4_ib_dev *dev, int
> port_num, int slave)
> p->gid_group.name = "gid_idx";
> p->gid_group.attrs = alloc_group_attrs(show_port_gid_idx,
> NULL, 1);
> - if (!p->gid_group.attrs)
> + if (!p->gid_group.attrs) {
> + ret = -ENOMEM;
> goto err_free_pkey;
> + }
>
ACK. Julia's patch is correct -- this is indeed a bug-fix.
-Jack
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 51/77] mthca: Update MSI/MSI-X interrupts enablement code
On Wed, 2 Oct 2013 12:49:07 +0200 Alexander Gordeev wrote: > Subject: [PATCH RFC 51/77] mthca: Update MSI/MSI-X interrupts > enablement code Date: Wed, 2 Oct 2013 12:49:07 +0200 > Sender: linux-rdma-ow...@vger.kernel.org > X-Mailer: git-send-email 1.7.7.6 > > As result of recent re-design of the MSI/MSI-X interrupts enabling > pattern this driver has to be updated to use the new technique to > obtain a optimal number of MSI/MSI-X interrupts required. > > Signed-off-by: Alexander Gordeev > --- ACK. -Jack -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 46/77] mlx4: Update MSI/MSI-X interrupts enablement code
On Wed, 2 Oct 2013 12:49:02 +0200 Alexander Gordeev wrote: > As result of recent re-design of the MSI/MSI-X interrupts enabling > pattern this driver has to be updated to use the new technique to > obtain a optimal number of MSI/MSI-X interrupts required. > > Signed-off-by: Alexander Gordeev New review -- ACK (i.e., patch is OK), subject to acceptance of patches 05 and 07 of this patch set. I sent my previous review (NACK) when I was not yet aware that changes proposed were due to the two earlier patches (mentioned above) in the current patch set. The change log here should actually read something like the following: As a result of changes to the MSI/MSI_X enabling procedures, this driver must be modified in order to preserve its current msi/msi_x enablement logic. -Jack -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 46/77] mlx4: Update MSI/MSI-X interrupts enablement code
On Wed, 2 Oct 2013 12:49:02 +0200
Alexander Gordeev wrote:
UPDATING THIS REPLY.
Your change log confused me. The change below is not from a "recent
re-design", it is required due to an earlier patch in this patch set.
>From the log, I assumed that the change you are talking about is already
upstream.
I will re-review.
-Jack
NACK. This change does not do anything logically as far as I can tell.
pci_enable_msix in the current upstream kernel itself calls
pci_msix_table_size. The current code yields the same resultswill
as the code suggested below. (i.e., the suggested code has no effect on
optimality).
BTW, pci_msix_table_size never returns a value < 0 (if msix is not
enabled, it returns 0 for the table size), so the (err < 0) check here
is not correct. (I also do not like using "err" here anyway for the
value returned by pci_msix_table_size(). There is no error here, and
it is simply confusing.
-Jack
> As result of recent re-design of the MSI/MSI-X interrupts enabling
> pattern this driver has to be updated to use the new technique to
> obtain a optimal number of MSI/MSI-X interrupts required.
>
> Signed-off-by: Alexander Gordeev
> ---
> drivers/net/ethernet/mellanox/mlx4/main.c | 17 -
> 1 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c
> b/drivers/net/ethernet/mellanox/mlx4/main.c index 60c9f4f..377a5ea
> 100644 --- a/drivers/net/ethernet/mellanox/mlx4/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
> @@ -1852,8 +1852,16 @@ static void mlx4_enable_msi_x(struct mlx4_dev
> *dev) int i;
>
> if (msi_x) {
> + err = pci_msix_table_size(dev->pdev);
> + if (err < 0)
> + goto no_msi;
> +
> + /* Try if at least 2 vectors are available */
> nreq = min_t(int, dev->caps.num_eqs -
> dev->caps.reserved_eqs, nreq);
> + nreq = min_t(int, nreq, err);
> + if (nreq < 2)
> + goto no_msi;
>
> entries = kcalloc(nreq, sizeof *entries, GFP_KERNEL);
> if (!entries)
> @@ -1862,17 +1870,8 @@ static void mlx4_enable_msi_x(struct mlx4_dev
> *dev) for (i = 0; i < nreq; ++i)
> entries[i].entry = i;
>
> - retry:
> err = pci_enable_msix(dev->pdev, entries, nreq);
> if (err) {
> - /* Try again if at least 2 vectors are
> available */
> - if (err > 1) {
> - mlx4_info(dev, "Requested %d
> vectors, "
> - "but only %d MSI-X vectors
> available, "
> - "trying again\n", nreq,
> err);
> - nreq = err;
> - goto retry;
> - }
> kfree(entries);
> goto no_msi;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 46/77] mlx4: Update MSI/MSI-X interrupts enablement code
On Wed, 2 Oct 2013 12:49:02 +0200
Alexander Gordeev wrote:
NACK. This change does not do anything logically as far as I can tell.
pci_enable_msix in the current upstream kernel itself calls
pci_msix_table_size. The current code yields the same results
as the code suggested below. (i.e., the suggested code has no effect on
optimality).
BTW, pci_msix_table_size never returns a value < 0 (if msix is not
enabled, it returns 0 for the table size), so the (err < 0) check here
is not correct. (I also do not like using "err" here anyway for the
value returned by pci_msix_table_size(). There is no error here, and
it is simply confusing.
-Jack
> As result of recent re-design of the MSI/MSI-X interrupts enabling
> pattern this driver has to be updated to use the new technique to
> obtain a optimal number of MSI/MSI-X interrupts required.
>
> Signed-off-by: Alexander Gordeev
> ---
> drivers/net/ethernet/mellanox/mlx4/main.c | 17 -
> 1 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c
> b/drivers/net/ethernet/mellanox/mlx4/main.c index 60c9f4f..377a5ea
> 100644 --- a/drivers/net/ethernet/mellanox/mlx4/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
> @@ -1852,8 +1852,16 @@ static void mlx4_enable_msi_x(struct mlx4_dev
> *dev) int i;
>
> if (msi_x) {
> + err = pci_msix_table_size(dev->pdev);
> + if (err < 0)
> + goto no_msi;
> +
> + /* Try if at least 2 vectors are available */
> nreq = min_t(int, dev->caps.num_eqs -
> dev->caps.reserved_eqs, nreq);
> + nreq = min_t(int, nreq, err);
> + if (nreq < 2)
> + goto no_msi;
>
> entries = kcalloc(nreq, sizeof *entries, GFP_KERNEL);
> if (!entries)
> @@ -1862,17 +1870,8 @@ static void mlx4_enable_msi_x(struct mlx4_dev
> *dev) for (i = 0; i < nreq; ++i)
> entries[i].entry = i;
>
> - retry:
> err = pci_enable_msix(dev->pdev, entries, nreq);
> if (err) {
> - /* Try again if at least 2 vectors are
> available */
> - if (err > 1) {
> - mlx4_info(dev, "Requested %d
> vectors, "
> - "but only %d MSI-X vectors
> available, "
> - "trying again\n", nreq,
> err);
> - nreq = err;
> - goto retry;
> - }
> kfree(entries);
> goto no_msi;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] IB/mlx4: silence GCC warning
On Monday 25 February 2013 19:23, Roland Dreier wrote: > On Mon, Feb 25, 2013 at 8:54 AM, Roland Dreier wrote: > > I'm finally noticing that this is in the build_mlx_header() function, > > which is pretty much a slow path. Certainly another compare isn't > > going to change performance given all the other stuff we do there. > > > > Let me look at the patches that have gone by and see what the cleanest > > way to handle this is. > > OK, after playing around a bit, I see that just initializing vlan > doesn't really change the generated code (my gcc at least was already > if effect setting vlan in the generated assembly code), so I'll just > merge that. > > - R. Thanks! -Jack -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] IB/mlx4: silence GCC warning
On Thursday 21 February 2013 11:02, Paul Bolle wrote:
> diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
> index 19e0637..512fde3 100644
> --- a/drivers/infiniband/hw/mlx4/qp.c
> +++ b/drivers/infiniband/hw/mlx4/qp.c
> @@ -1778,8 +1778,8 @@ static int build_mlx_header(struct mlx4_ib_sqp *sqp,
> struct ib_send_wr *wr,
> }
>
> vlan = rdma_get_vlan_id(&sgid);
> - is_vlan = vlan < 0x1000;
> }
Nice try!
However, this approach does add the line below to processing for an IB port
(ETH/RoCE port stays same, more or less).
Processing time is therefore increased (at least on the IB side) relative to
just living with the warning.
Roland?
> + is_vlan = vlan < 0x1000; <=== Code line added to IB-side processing.
> ib_ud_header_init(send_size, !is_eth, is_eth, is_vlan, is_grh, 0,
> &sqp->ud_header);
>
> if (!is_eth) {
-Jack
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] IB/lmx4: silence GCC warning
You could use:
u16 uninitialized_var(vlan);
instead.
Although this in the special QP data flow, I still prefer to avoid adding extra
code (even setting
initial values at procedure entry). The line above will also do the job.
"uninitialized_var"
is used elsewhere in the driver. See, for example, mlx4_ib_post_send() in the
same file (qp.c).
-Jack
On Friday 28 September 2012 14:48, Paul Bolle wrote:
> Building qp.o (part of the "Mellanox ConnectX HCA support" driver)
> triggers this GCC warning:
> drivers/infiniband/hw/mlx4/qp.c: In function ‘mlx4_ib_post_send’:
> drivers/infiniband/hw/mlx4/qp.c:1828:30: warning: ‘vlan’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> drivers/infiniband/hw/mlx4/qp.c:1718:6: note: ‘vlan’ was declared here
>
> Looking at the code it is clear 'vlan' is only set and used if 'is_eth'
> is non-zero. But there's no harm in initializing 'vlan' to 0 (which
> matches ib_get_cached_gid()'s default return) to silence GCC.
>
> Signed-off-by: Paul Bolle
> ---
> 0) I noticed this warning while building v3.6-rc7 on current Fedora 17,
> using Fedora's default config.
>
> 1) Compile tested only. I tested against v3.6-rc7, with commit
> a41262bb5721f2b708ee8b23f67be2f2e16a2fab ("IB/mlx4: SR-IOV IB context
> objects and proxy/tunnel SQP") from linux-next cherry-picked, to take
> into account a trivial context change in linux-next.
>
> drivers/infiniband/hw/mlx4/qp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
> index a862251..71fdda6 100644
> --- a/drivers/infiniband/hw/mlx4/qp.c
> +++ b/drivers/infiniband/hw/mlx4/qp.c
> @@ -1715,7 +1715,7 @@ static int build_mlx_header(struct mlx4_ib_sqp *sqp,
> struct ib_send_wr *wr,
> int is_eth;
> int is_vlan = 0;
> int is_grh;
> - u16 vlan;
> + u16 vlan = 0;
> int err = 0;
>
> send_size = 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: linux-next: build failure after merge of the akpm tree
Hi Roland,
I am on vacation until next Tuesday -- I'll look at this then.
-Jack
On Monday 24 September 2012 21:36, Roland Dreier wrote:
> On Mon, Sep 24, 2012 at 7:02 AM, Stephen Rothwell
> wrote:
> > After merging the akpm tree, today's linux-next build (powerpc
> > ppc64_defconfig) failed like this:
> >
> > drivers/infiniband/hw/mlx4/cm.c: In function 'id_map_alloc':
> > drivers/infiniband/hw/mlx4/cm.c:228:36: error: 'MAX_ID_MASK' undeclared
> > (first use in this function)
> >
> > Caused by commit d7a4e9b679e9 ("IB/mlx4: Add CM paravirtualization") from
> > the infiniband tree interacting with commit "idr: rename MAX_LEVEL to
> > MAX_IDR_LEVEL" from the akpm tree.
> >
> > I have added the following merge fix patch for today:
> >
> > From: Stephen Rothwell
> > Date: Mon, 24 Sep 2012 23:57:53 +1000
> > Subject: [PATCH] IB/mlx4: fix for MAX_ID_MASK to MAX_IDR_MASK name change
> >
> > Signed-off-by: Stephen Rothwell
> > ---
> > drivers/infiniband/hw/mlx4/cm.c |2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/hw/mlx4/cm.c
> > b/drivers/infiniband/hw/mlx4/cm.c
> > index e25e4da..80079e5 100644
> > --- a/drivers/infiniband/hw/mlx4/cm.c
> > +++ b/drivers/infiniband/hw/mlx4/cm.c
> > @@ -225,7 +225,7 @@ id_map_alloc(struct ib_device *ibdev, int slave_id, u32
> > sl_cm_id)
> > ret = idr_get_new_above(&sriov->pv_id_table, ent,
> > next_id, &id);
> > if (!ret) {
> > - next_id = ((unsigned) id + 1) & MAX_ID_MASK;
> > + next_id = ((unsigned) id + 1) & MAX_IDR_MASK;
> > ent->pv_cm_id = (u32)id;
> > sl_id_map_add(ibdev, ent);
> > }
>
> Andrew, any preference on how to handle this merge?
>
> Jack/Amir, I wonder if there's some way we can avoid this code
> entirely? Is an IDR the right structure to use here, or would we
> be better off with a radix tree maybe (where we can assign our
> own ID)?
>
> - R.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: linux-next: manual merge of the net-next tree with the infiniband tree
On Thursday 12 July 2012 05:13, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the net-next tree got a conflict in
> include/linux/mlx4/device.h between commit 396f2feb05d7 ("mlx4_core:
> Implement mechanism for reserved Q_Keys") from the infiniband tree and
> commit 0ff1fb654bec ("{NET, IB}/mlx4: Add device managed flow steering
> firmware API") from the net-next tree.
>
> Just context changes. I fixed it up (see below) and can carry the fix
> as necessary.
Thanks, Stephen!
Your merge looks fine. Ack.
-Jack
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: linux-next: manual merge of the net-next tree with the infiniband tree
On Thursday 12 July 2012 05:09, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the net-next tree got a conflict in
> drivers/net/ethernet/mellanox/mlx4/main.c between commit 6634961c14d3
> ("mlx4: Put physical GID and P_Key table sizes in mlx4_phys_caps struct
> and paravirtualize them") from the infiniband tree and commit
> 0ff1fb654bec ("{NET, IB}/mlx4: Add device managed flow steering firmware
> API") from the net-next tree.
>
> Just context changes (I think). I have fixed it up (see below) and can
> carry the fix as necessary.
Thanks, Stephen!
Ack for IB side.
-Jack
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mlx4: fix build break
On Tuesday 12 February 2008 00:18, Roland Dreier wrote: > Thanks, applied. > > Jack, I thought you guys tested the build on powerpc. How did this > sneak through? > It did not sneak through, because the problem does not exist in the OFED git. The following commit was performed to git://git.openfabrics.org/ofed_1_3/linux-2.6.git on Sept 25, 2007: === commit 4a5709b81dfd249c98271801ddc01decb7acd466 Author: Eli Cohen <[EMAIL PROTECTED]> Date: Tue Sep 25 12:41:30 2007 +0200 add missing include file. ia64 requires it. Signed-off-by: Eli Cohen <[EMAIL PROTECTED]> diff --git a/drivers/net/mlx4/alloc.c b/drivers/net/mlx4/alloc.c index f8d63d3..704a56b 100644 --- a/drivers/net/mlx4/alloc.c +++ b/drivers/net/mlx4/alloc.c @@ -34,6 +34,7 @@ #include #include #include +#include #include "mlx4.h" I guess this just fell through the cracks with regard to posting it to the list. Sorry about that. - Jack -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] [GIT PULL] please pull infiniband.git
On Tuesday 27 November 2007 08:21, Roland Dreier wrote: > Linus, please pull from > > master.kernel.org:/pub/scm/linux/kernel/git/roland/infiniband.git > for-linus > > > Jack Morgenstein (1): > mlx4_core: Fix state check in mlx4_qp_modify() > MUST also enter the patch I send yesterday: [PATCH] ipoib: fix kernel Oops resulting from xmit when priv->broadcast is NULL. (critical bug fix -- will get kernel Oopses whenever ports on the network go down!). (patch given again below) - Jack == IPoIB: Fix kernel Oops resulting from xmit following dev_down. If a port goes down, ipoib_ib_dev_down is invoked -- which flushed the mcasts (clearing priv->broadcast) and clearing the path record cache. If ipoib_start_xmit is then invoked (before the port is upped), a kernel Oops results from attempting to access priv->broadcast. Returning NULL if priv->broadcast is NULL is a harmless way of bypassing the problem -- the offending packet is simply discarded "without prejudice". Signed-off-by: Jack Morgenstein <[EMAIL PROTECTED]> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c index a03a65e..c9f6077 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c @@ -460,6 +460,9 @@ static struct ipoib_path *path_rec_create(struct net_device *dev, void *gid) struct ipoib_dev_priv *priv = netdev_priv(dev); struct ipoib_path *path; + if (!priv->broadcast) + return NULL; + path = kzalloc(sizeof *path, GFP_ATOMIC); if (!path) return NULL; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Updated InfiniBand/RDMA merge plans for 2.6.24
On Saturday 06 October 2007 01:18, Roland Dreier wrote: > - XRC. Given the length of the backlog above and the fact that a > first draft of this code has not been posted yet, I don't see any > way that we could have something this major ready in time. > I posted the first draft patch set to the OpenFabrics list on September 18: [ofa-general] [PATCH 0 of 5] XRC implementation patches (libibverbs, libmlx4, core, mlx4) Jack Morgenstein [ofa-general] [PATCH 1 of 5] libibverbs: XRC implementation Jack Morgenstein [ofa-general] [PATCH 2 of 5] libmlx4: XRC implementation Jack Morgenstein [ofa-general] [PATCH 3 of 5] core: XRC implementation for fd = -1 when opening an xrc domain Jack Morgenstein [ofa-general] [PATCH 4 of 5] core: XRC implementation -- add support for working with file descriptors Jack Morgenstein [ofa-general] [PATCH 5 of 5] mlx4: XRC implementation Jack Morgenstein The above patch set implements XRC for userspace verbs layer and below. The Kernel-space verbs implementation (1 more patch) is finished, but as yet untested. - Jack - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] InfiniBand/RDMA merge plans for 2.6.24
On Thursday 13 September 2007 20:57, Roland Dreier wrote:
> HW specific:
>
> - I already merged patches to enable MSI-X by default for mthca and
> mlx4. I hope there aren't too many systems that get hosed if a
> MSI-X interrupt is generated.
>
> - Jack and Michael's mlx4 FMR support. Will merge I guess, although
> I do hope to have time to address the DMA API abuse that is being
> copied from mthca, so that mlx4 and mthca work in Xen domU.
>
> - ehca patch queue. Will merge, pending fixes for the few minor
> issues I commented on.
>
> - Steve's mthca router mode support. Would be nice to see a review
> from someone at Mellanox.
>
> - Arthur's mthca doorbell alignment fixes. I will experiment with a
> few different approaches and post what I like (and fix mlx4 as
> well). I hope Arthur can review.
>
> - Michael's mlx4 WQE shrinking patch. Not sure yet; I'll reply to
> the latest patch directly.
>
Missing from this list (IMPORTANT patch!):
[ofa-general] [PATCH 2 of 2] IB/mlx4: Handle new FW requirement for send
request prefetching, for WQE sg lists
(Posted by me to list on Sept 4)
{patch header:
This is an addendum to Roland's commit 0e6e74162164d908edf7889ac66dca09e7505745
(June 18). This addendum adds prefetch headroom marking processing for s/g
segments.
We write s/g segments in reverse order into the WQE, in order to guarantee
that the first dword of all cachelines containing s/g segments is written last
(overwriting the headroom invalidation pattern). The entire cacheline will thus
contain valid data when the invalidation pattern is overwritten.
}
This patch series (1 of 2 is for libmlx4, the same issue).
Also, I'm now posting (in a separate post) the following patch to mlx4, which
is important:
display the following device information via sysfs:
board_id, fw_ver, hw_rev, hca_type.
The info is displayed under directory /sys/class/infiniband/mlx4_x, where x is
the pci bus sequence number (starting from zero).
This patch makes information available to ibstat and ibv_devinfo under the
same directory as is used for tavor/arbel/sinai -- thus requiring no userspace
modifications.
- Jack
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
