RE: [PATCH 1/1] net: ethernet: broadcom: fix improper return value

[Kalderon, Michal]

> From: Pan Bian 
> 
> Marco BNX2X_ALLOC_AND_SET(arr, lbl, func) calls kmalloc() to allocate
> memory, and jumps to label "lbl" if the allocation fails. Label "lbl"
> first cleans memory and then returns variable rc. Before calling the macro, 
> the
> value of variable rc is 0. Because 0 means no error, the callers of
> bnx2x_init_firmware() may be misled. This patch fixes the bug, assigning "-
> ENOMEM" to rc before calling macro NX2X_ALLOC_AND_SET().
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=189141
> 
> Signed-off-by: Pan Bian 

The title is wrong from net-next; It should have been "bnx2x: Fix improper 
return value".

Acked-by: Michal Kalderon 

RE: [PATCH v2 2/2] RDMA/core: Initialize port_num in qp_attr

[Kalderon, Michal]

> From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
> ow...@vger.kernel.org] On Behalf Of Marciniszyn, Mike
> > Initialize the port_num for iWARP in rdma_init_qp_attr.
> >
> > Fixes: 5ecce4c9b17b("Check port number supplied by user verbs cmds")
> > Cc:  # v2.6.14+
> > Reviewed-by: Steve Wise 
> > Signed-off-by: Mustafa Ismail 
> 
Why is the second patch required if you only validate the port_num if the 
IB_QP_PORT mask is on? 
Given the first patch [PATCH v2 1/2] RDMA/uverbs: Fix the check for port 
number, this one seems
redundant. 

Thanks,
Michal

Re: [PATCH v2 2/2] RDMA/core: Initialize port_num in qp_attr

[Kalderon, Michal]

From: Ismail, Mustafa 
Sent: Wednesday, July 19, 2017 5:38 PM

> > > > Fixes: 5ecce4c9b17b("Check port number supplied by user verbs cmds")
> > > > Cc:  # v2.6.14+
> > > > Reviewed-by: Steve Wise 
> > > > Signed-off-by: Mustafa Ismail 
> > >
> > Why is the second patch required if you only validate the port_num if the
> > IB_QP_PORT mask is on?
> > Given the first patch [PATCH v2 1/2] RDMA/uverbs: Fix the check for port
> > number, this one seems redundant.
> Strictly speaking it is not required, but we felt it safer to always return a 
> valid port number
> as is done in the IB case.

It's not always initialized in the IB case either. More than that if at this 
point you'll
initialize it for ib as well you'll get a failure on ib_modify_qp_is_ok, since 
when
transitioning to RTR  / RTS providing IB_QP_PORT is not a valid option.
We actually hit this issue when running rping over RoCE. (prior to your fix i 
mean ) 
I agree that in general there's no real harm, but it seems a bit out of 
context, and if we
make the change common for ib/iwarp we'll have to modify ib_modify_qp_is_ok 
which 
is written close to the spec. 

thanks,
Michal

RE: [PATCH] RDMA/qedr: fix build error without ipv6

[Kalderon, Michal]

> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Tuesday, September 05, 2017 5:59 PM
> To: Amrani, Ram <ram.amr...@cavium.com>; Kalderon, Michal
> <michal.kalde...@cavium.com>; Elior, Ariel <ariel.el...@cavium.com>;
> Doug Ledford <dledf...@redhat.com>; Sean Hefty
> <sean.he...@intel.com>; Hal Rosenstock <hal.rosenst...@gmail.com>
> Cc: Arnd Bergmann <a...@arndb.de>; David S. Miller
> <da...@davemloft.net>; Mintz, Yuval <yuval.mi...@cavium.com>; linux-
> r...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] RDMA/qedr: fix build error without ipv6
> 
> When CONFIG_IPV6 disabled, we run into a link error:
> 
> drivers/infiniband/hw/qedr/qedr_iw_cm.o: In function
> `qedr_addr6_resolve.isra.3':
> qedr_iw_cm.c:(.text+0x4e0): undefined reference to
> `ip6_route_output_flags'
> 
> The ipv6 handling code is obviously not needed here, so this adds a compile-
> time check for the Kconfig symbol in all three places in the code that decide
> between ipv4 and ipv6.
> 
> We don't have to worry about a link error wtih QEDR=y/IPV6=m, as that
> configuration is already prohibited by CONFIG_INFINIBAND depending on "m
> || IPV6 != m".
> 
> Fixes: e411e0587e0d ("RDMA/qedr: Add iWARP connection management
> functions")
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> ---
>  drivers/infiniband/hw/qedr/qedr_iw_cm.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> b/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> index fe9b2b6149b0..2950d3f6ecb8 100644
> --- a/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> +++ b/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> @@ -98,7 +98,8 @@ qedr_iw_mpa_request(void *context, struct
> qed_iwarp_cm_event_params *params)
>   event.event = IW_CM_EVENT_CONNECT_REQUEST;
>   event.status = params->status;
> 
> - if (params->cm_info->ip_version == QED_TCP_IPV4)
> + if (!IS_ENABLED(CONFIG_IPV6) ||
> + params->cm_info->ip_version == QED_TCP_IPV4)
>   qedr_fill_sockaddr4(params->cm_info, );
>   else
>   qedr_fill_sockaddr6(params->cm_info, ); @@ -522,7
> +523,8 @@ int qedr_iw_connect(struct iw_cm_id *cm_id, struct
> iw_cm_conn_param *conn_param)
>   memset(cm_info->local_ip, 0, sizeof(cm_info->local_ip));
>   memset(cm_info->remote_ip, 0, sizeof(cm_info->remote_ip));
> 
> - if (cm_id->remote_addr.ss_family == AF_INET) {
> + if (!IS_ENABLED(CONFIG_IPV6) ||
> + cm_id->remote_addr.ss_family == AF_INET) {
>   cm_info->ip_version = QED_TCP_IPV4;
> 
>   cm_info->remote_ip[0] = ntohl(raddr->sin_addr.s_addr);
> @@ -616,7 +618,8 @@ int qedr_iw_create_listen(struct iw_cm_id *cm_id,
> int backlog)
>   iparams.event_cb = qedr_iw_event_handler;
>   iparams.max_backlog = backlog;
> 
> - if (cm_id->local_addr.ss_family == AF_INET) {
> + if (!IS_ENABLED(CONFIG_IPV6) ||
> + cm_id->local_addr.ss_family == AF_INET) {
>   iparams.ip_version = QED_TCP_IPV4;
>   memset(iparams.ip_addr, 0, sizeof(iparams.ip_addr));
> 
> --
> 2.9.0

Thanks!

Acked-by: Michal Kalderon <michal.kalde...@cavium.com>

Re: [PATCH net-next] qed: Set error code for allocation failures

[Kalderon, Michal]

From: Dan Carpenter 
Sent: Friday, October 27, 2017 2:52 PM

>On Fri, Oct 27, 2017 at 05:32:42PM +0800, Yunsheng Lin wrote:
>> > iwarp_info = _hwfn->p_rdma_info->iwarp;
>> > @@ -2696,6 +2696,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
>> > if (rc)
>> > goto err;
>> >
>> > +   rc = -ENOMEM;
>> > iwarp_info->partial_fpdus = kcalloc((u16)p_hwfn->p_rdma_info->num_qps,
>> > sizeof(*iwarp_info->partial_fpdus),
>> > GFP_KERNEL);
>>
>> Does the memory allocated here need to be freed when error happens below?
>>
>
>Hm...  I think you're right that it leaks.  Also I'm confused by the
>qed_iwarp_ll2_alloc_buffers() allocation.  The comment in there says
>that /* buffers will be deallocated by qed_ll2 */ but qed_ll2 is not
>a function name or something which is useful to grep.

Thanks Dan, partial_fpdus is released during qed_iwarp_resc_free, called from 
qed_rdma_resc_free
called on qed_rdma_stop and on failure during qed_rdma_start.
Regarding ll2 buffers. For each successfully allocated buffer we call
qed_iwarp_ll2_post_rx->qed_ll2_post_rx_buffer.
These buffers will get released and freed when we call 
qed_ll2_terminate_connection
called from qed_iwarp_ll2_stop ( which is called during qed_iwarp_ll2_start on 
error ).
thanks,
Michal 

Re: [PATCH net-next] qed: Set error code for allocation failures

[Kalderon, Michal]

From: Dan Carpenter 
Sent: Friday, October 27, 2017 9:40 AM

> There are several places where we accidentally return success when
> kcalloc() fails.
> 
> Fixes: fcb39f6c10b2 ("qed: Add mpa buffer descriptors for storing and 
> processing mpa fpdus")
> Signed-off-by: Dan Carpenter 
>
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c 
> b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> index 409041eab189..6366f2ef82b7 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> @@ -2585,7 +2585,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
> struct qed_ll2_cbs cbs;
> u32 mpa_buff_size;
> u16 n_ooo_bufs;
> -   int rc = 0;
> +   int rc;
> int i;
> 
> iwarp_info = _hwfn->p_rdma_info->iwarp;
> @@ -2696,6 +2696,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
> if (rc)
>goto err;
> 
> +   rc = -ENOMEM;
> iwarp_info->partial_fpdus = kcalloc((u16)p_hwfn->p_rdma_info->num_qps,
> 
> sizeof(*iwarp_info->partial_fpdus),
> GFP_KERNEL);
> @@ -2724,7 +2725,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
> for (i = 0; i < data.input.rx_num_desc; i++)
> list_add_tail(_info->mpa_bufs[i].list_entry,
>   _info->mpa_buf_list);
> -   return rc;
> +   return 0;
>  err:
> qed_iwarp_ll2_stop(p_hwfn, p_ptt);

Thanks Dan,
Acked-by: Michal Kalderon 

RE: [PATCH] qed: code indent should use tabs where possible

[Kalderon, Michal]

> From: Rohit Visavalia [mailto:rohit.visava...@softnautics.com]
> Sent: Thursday, January 25, 2018 12:26 PM
> To: Elior, Ariel ; Dept-Eng Everest Linux L2  engeverestlinu...@cavium.com>
> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; Rohit Visavalia
> 
> Subject: [PATCH] qed: code indent should use tabs where possible
> 
> Issue found by checkpatch.
> 
> Signed-off-by: Rohit Visavalia 
> ---
>  drivers/net/ethernet/qlogic/qed/qed_rdma.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> index b7abb8205d3a..ae399c48d4a3 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> @@ -360,13 +360,13 @@ static void qed_rdma_resc_free(struct qed_hwfn
> *p_hwfn)
> 
>  static void qed_rdma_free_tid(void *rdma_cxt, u32 itid)  {
> -struct qed_hwfn *p_hwfn = (struct qed_hwfn *)rdma_cxt;
> + struct qed_hwfn *p_hwfn = (struct qed_hwfn *)rdma_cxt;
> 
> -DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "itid = %08x\n", itid);
> + DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "itid = %08x\n", itid);
> 
> -spin_lock_bh(_hwfn->p_rdma_info->lock);
> -qed_bmap_release_id(p_hwfn, _hwfn->p_rdma_info->tid_map,
> itid);
> -spin_unlock_bh(_hwfn->p_rdma_info->lock);
> + spin_lock_bh(_hwfn->p_rdma_info->lock);
> + qed_bmap_release_id(p_hwfn, _hwfn->p_rdma_info->tid_map,
> itid);
> + spin_unlock_bh(_hwfn->p_rdma_info->lock);
>  }
> 
>  static void qed_rdma_free_reserved_lkey(struct qed_hwfn *p_hwfn)
> --
> 2.16.1

Thanks and apologies,

Acked-by: Michal Kalderon 

RE: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2

[Kalderon, Michal]

> From: Jason Gunthorpe [mailto:j...@ziepe.ca]
> Sent: Tuesday, April 03, 2018 11:04 PM
> 
> On Tue, Apr 03, 2018 at 07:42:28AM +0000, Kalderon, Michal wrote:
> > > From: Sinan Kaya [mailto:ok...@codeaurora.org]
> > > Sent: Tuesday, April 03, 2018 5:30 AM
> > > To: linux-r...@vger.kernel.org; ti...@codeaurora.org;
> > > sulr...@codeaurora.org
> > > Cc: linux-arm-...@vger.kernel.org;
> > > linux-arm-ker...@lists.infradead.org;
> > > Kalderon, Michal <michal.kalde...@cavium.com>; Elior, Ariel
> > > <ariel.el...@cavium.com>; Doug Ledford <dledf...@redhat.com>; Jason
> > > Gunthorpe <j...@ziepe.ca>; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers
> > > on weakly-ordered archs #2
> > >
> > > On 3/22/2018 12:26 PM, Sinan Kaya wrote:
> > > > @@ -860,7 +860,7 @@ static void doorbell_cq(struct qedr_cq *cq,
> > > > u32
> > > cons, u8 flags)
> > > > wmb();
> > > > cq->db.data.agg_flags = flags;
> > > > cq->db.data.value = cpu_to_le32(cons);
> > > > -   writeq(cq->db.raw, cq->db_addr);
> > > > +   writeq_relaxed(cq->db.raw, cq->db_addr);
> > >
> > > Given the direction to get rid of wmb() in front of writeX()
> > > functions, I have been reviewing this code. Under normal
> > > circumstances, I can get rid of all
> > > wmb() as follows.
> > >
> > > However, I started having my doubts now. Are these wmb() used as a
> > > SMP barrier too?
> > > I can't find any smp_Xmb() in drivers/infiniband/hw/qedr directory.
> >
> > Your doubts are in place. You initial patch series modified writel to
> > writel_relaxed Simply removing the wmb is dangerous. The wmb before
> > writel are used to make sure the HW observes the changes in memory
> > before we trigger the doorbell. Smp barriers here wouldn't suffice, as
> > on a single processor. we still need to make sure memory is updated and
> not remained in cache when HW accesses it.
> > Reviewing the qedr barriers, I can find places where this may have not
> > been necessary, But definitely you can't simply remove this wmb barriers.
> 
> As Sinan said, the consensus is that wmb();writel(); is redundant if the only
> purpose of the wmb is to order DMA and system memory.
> 
> So can you review these patches on that basis please? Is the WMB doing
> something else, eg SMP related? If yes, please send a patch adding
> appropriate comments.

Thanks Sinan and Jason for the references and explanations, I've reviewed the 
wmb
usages in qedr and am about to send a patch that replaces two of them with 
smp_wmb
and completely removes two of them that given your explanation, turned out to 
be redundant,
thanks.

> 
> Thanks,
> Jason

RE: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2

[Kalderon, Michal]

> From: Sinan Kaya [mailto:ok...@codeaurora.org]
> Sent: Tuesday, April 03, 2018 5:30 AM
> To: linux-r...@vger.kernel.org; ti...@codeaurora.org;
> sulr...@codeaurora.org
> Cc: linux-arm-...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> Kalderon, Michal <michal.kalde...@cavium.com>; Elior, Ariel
> <ariel.el...@cavium.com>; Doug Ledford <dledf...@redhat.com>; Jason
> Gunthorpe <j...@ziepe.ca>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on
> weakly-ordered archs #2
> 
> On 3/22/2018 12:26 PM, Sinan Kaya wrote:
> > @@ -860,7 +860,7 @@ static void doorbell_cq(struct qedr_cq *cq, u32
> cons, u8 flags)
> > wmb();
> > cq->db.data.agg_flags = flags;
> > cq->db.data.value = cpu_to_le32(cons);
> > -   writeq(cq->db.raw, cq->db_addr);
> > +   writeq_relaxed(cq->db.raw, cq->db_addr);
> 
> Given the direction to get rid of wmb() in front of writeX() functions, I have
> been reviewing this code. Under normal circumstances, I can get rid of all
> wmb() as follows.
> 
> However, I started having my doubts now. Are these wmb() used as a SMP
> barrier too?
> I can't find any smp_Xmb() in drivers/infiniband/hw/qedr directory.

Your doubts are in place. You initial patch series modified writel to 
writel_relaxed
Simply removing the wmb is dangerous. The wmb before writel are used to make 
sure the
HW observes the changes in memory before we trigger the doorbell. Smp barriers 
here
wouldn't suffice, as on a single processor. we still need to make sure memory 
is updated
and not remained in cache when HW accesses it.
Reviewing the qedr barriers, I can find places where this may have not been 
necessary, 
But definitely you can't simply remove this wmb barriers. 

> 
> static void doorbell_cq(struct qedr_cq *cq, u32 cons, u8 flags)  {
> -   /* Flush data before signalling doorbell */
> -   wmb();
> cq->db.data.agg_flags = flags;
> cq->db.data.value = cpu_to_le32(cons);
> writeq(cq->db.raw, cq->db_addr); @@ -1870,8 +1868,7 @@ static int
> qedr_update_qp_state(struct qedr_dev *dev,
>  */
> 
> if (rdma_protocol_roce(>ibdev, 1)) {
> -   wmb();
> -   writel_relaxed(qp->rq.db_data.raw, qp->rq.db);
> +   writel(qp->rq.db_data.raw, qp->rq.db);
> /* Make sure write takes effect */
> mmiowb();
> }
> @@ -3275,8 +3272,7 @@ int qedr_post_send(struct ib_qp *ibqp, struct
> ib_send_wr *wr,
>  * unchanged). For performance reasons we avoid checking for this
>  * redundant doorbell.
>  */
> -   wmb();
> -   writel_relaxed(qp->sq.db_data.raw, qp->sq.db);
> +   writel(qp->sq.db_data.raw, qp->sq.db);
> 
> /* Make sure write sticks */
> mmiowb();
> @@ -3362,9 +3358,6 @@ int qedr_post_recv(struct ib_qp *ibqp, struct
> ib_recv_wr *wr,
> 
> qedr_inc_sw_prod(>rq);
> 
> -   /* Flush all the writes before signalling doorbell */
> -   wmb();
> 
> 
> 
> 
> 
> --
> Sinan Kaya
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
> Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
> Foundation Collaborative Project.

RE: [PATCH v4 6/6] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2

[Kalderon, Michal]

> From: Sinan Kaya [mailto:ok...@codeaurora.org]
> Sent: Tuesday, March 20, 2018 4:48 AM
> 
> Code includes wmb() followed by writel() in multiple places. writel() already
> has a barrier on some architectures like arm64.
> 
> This ends up CPU observing two barriers back to back before executing the
> register write.
> 
> Since code already has an explicit barrier call, changing writel() to
> writel_relaxed().
> 
> Signed-off-by: Sinan Kaya 
> ---
>  drivers/infiniband/hw/qedr/verbs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/qedr/verbs.c
> b/drivers/infiniband/hw/qedr/verbs.c
> index ccd55f4..db60360 100644
> --- a/drivers/infiniband/hw/qedr/verbs.c
> +++ b/drivers/infiniband/hw/qedr/verbs.c
> @@ -860,7 +860,7 @@ static void doorbell_cq(struct qedr_cq *cq, u32 cons,
> u8 flags)
>   wmb();
>   cq->db.data.agg_flags = flags;
>   cq->db.data.value = cpu_to_le32(cons);
> - writeq(cq->db.raw, cq->db_addr);
> + writeq_relaxed(cq->db.raw, cq->db_addr);
> 
>   /* Make sure write would stick */
>   mmiowb();
> @@ -3338,7 +3338,7 @@ int qedr_post_recv(struct ib_qp *ibqp, struct
> ib_recv_wr *wr,
> 
>   qp->rq.db_data.data.value++;
> 
> - writel(qp->rq.db_data.raw, qp->rq.db);
> + writel_relaxed(qp->rq.db_data.raw, qp->rq.db);
> 
>   /* Make sure write sticks */
>   mmiowb();
> --
> 2.7.4
Acked-by: Michal Kalderon 

RE: [PATCH] qlogic/qed: Constify *pkt_type_str[]

[Kalderon, Michal]

> From: Hernán Gonzalez [mailto:her...@vanguardiasur.com.ar]
> Sent: Wednesday, February 28, 2018 12:32 AM
> 
> Note: This is compile only tested as I have no access to the hw.
> Constifying and declaring as static saves 24 bytes.
> 
> add/remove: 0/1 grow/shrink: 0/0 up/down: 0/-24 (-24)
> Function old new   delta
> pkt_type_str  24   - -24
> Total: Before=3599256, After=3599232, chg -0.00%
> 
> Signed-off-by: Hernán Gonzalez 
> ---
>  drivers/net/ethernet/qlogic/qed/qed_iwarp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> index ca4a81d..03ad4ee 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> @@ -1784,7 +1784,7 @@ enum qed_iwarp_mpa_pkt_type {
>  /* fpdu can be fragmented over maximum 3 bds: header, partial mpa,
> unaligned */  #define QED_IWARP_MAX_BDS_PER_FPDU 3
> 
> -char *pkt_type_str[] = {
> +static const char * const pkt_type_str[] = {
>   "QED_IWARP_MPA_PKT_PACKED",
>   "QED_IWARP_MPA_PKT_PARTIAL",
>   "QED_IWARP_MPA_PKT_UNALIGNED"
> --
> 2.7.4

Thanks

Acked-by: Michal Kalderon 

RE: [PATCH v2] RDMA/qedr: Remove enumerated type qed_roce_ll2_tx_dest

[Kalderon, Michal]

> From: Nick Desaulniers 
> Sent: Friday, September 28, 2018 1:30 AM
> 
> External Email
> 
> On Thu, Sep 27, 2018 at 1:57 PM Nathan Chancellor
>  wrote:
> >
> > Clang warns when one enumerated type is explicitly converted to another.
> >
> > drivers/infiniband/hw/qedr/qedr_roce_cm.c:198:28: warning: implicit
> > conversion from enumeration type 'enum qed_roce_ll2_tx_dest' to
> > different enumeration type 'enum qed_ll2_tx_dest' [-Wenum-conversion]
> > ll2_tx_pkt.tx_dest = pkt->tx_dest;
> >~ ~^~~
> > 1 warning generated.
> >
> > Turns out that QED_ROCE_LL2_TX_DEST_NW and
> QED_ROCE_LL2_TX_DEST_LB are
> > only used once in the whole tree and QED_ROCE_LL2_TX_DEST_MAX is
> used
> > nowhere. Remove them and use the equivalent values from
> > qed_ll2_tx_dest in their place.
> >
> > Reported-by: Nick Desaulniers 
> > Signed-off-by: Nathan Chancellor 
> > ---
> >
> > v1 -> v2:
> >
> > * Rather than using an explicit cast, just convert the uses to the
> >   appropriate values and delete the duplicated enum.
> >
> >  drivers/infiniband/hw/qedr/qedr_roce_cm.c |  4 ++--
> >  include/linux/qed/qed_rdma_if.h   | 11 +--
> >  2 files changed, 3 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > b/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > index 85578887421b..e1ac2fd60bb1 100644
> > --- a/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > +++ b/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > @@ -519,9 +519,9 @@ static inline int qedr_gsi_build_packet(struct
> qedr_dev *dev,
> > }
> >
> > if (ether_addr_equal(udh.eth.smac_h, udh.eth.dmac_h))
> > -   packet->tx_dest = QED_ROCE_LL2_TX_DEST_LB;
> > +   packet->tx_dest = QED_LL2_TX_DEST_LB;
> > else
> > -   packet->tx_dest = QED_ROCE_LL2_TX_DEST_NW;
> > +   packet->tx_dest = QED_LL2_TX_DEST_NW;
> 
> Yep, looks like these were the only two sites using enum
> qed_roce_ll2_tx_dest in the whole kernel.  The new enum values match the
> previous enum values, so this is no functional change while simplifying the
> code (one less enum to get wrong).
> Reviewed-by: Nick Desaulniers 
> 
> >
> > packet->roce_mode = roce_mode;
> > memcpy(packet->header.vaddr, ud_header_buffer, header_size);
> > diff --git a/include/linux/qed/qed_rdma_if.h
> > b/include/linux/qed/qed_rdma_if.h index df4d13f7e191..d15f8e4815e3
> > 100644
> > --- a/include/linux/qed/qed_rdma_if.h
> > +++ b/include/linux/qed/qed_rdma_if.h
> > @@ -39,15 +39,6 @@
> >  #include 
> >  #include 
> >
> > -enum qed_roce_ll2_tx_dest {
> > -   /* Light L2 TX Destination to the Network */
> > -   QED_ROCE_LL2_TX_DEST_NW,
> > -
> > -   /* Light L2 TX Destination to the Loopback */
> > -   QED_ROCE_LL2_TX_DEST_LB,
> > -   QED_ROCE_LL2_TX_DEST_MAX
> > -};
> > -
> >  #define QED_RDMA_MAX_CNQ_SIZE   (0x)
> >
> >  /* rdma interface */
> > @@ -581,7 +572,7 @@ struct qed_roce_ll2_packet {
> > int n_seg;
> > struct qed_roce_ll2_buffer payload[RDMA_MAX_SGE_PER_SQ_WQE];
> > int roce_mode;
> > -   enum qed_roce_ll2_tx_dest tx_dest;
> > +   enum qed_ll2_tx_dest tx_dest;
> >  };
> >
> >  enum qed_rdma_type {
> > --
> > 2.19.0
> >
> 
> 
> --
> Thanks,
> ~Nick Desaulniers

Thanks, definitely  looks cleaner. 
Acked-by: Michal Kalderon 

RE: [PATCH v4 6/6] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2

[Kalderon, Michal]

> From: Sinan Kaya [mailto:ok...@codeaurora.org]
> Sent: Tuesday, March 20, 2018 4:48 AM
> 
> Code includes wmb() followed by writel() in multiple places. writel() already
> has a barrier on some architectures like arm64.
> 
> This ends up CPU observing two barriers back to back before executing the
> register write.
> 
> Since code already has an explicit barrier call, changing writel() to
> writel_relaxed().
> 
> Signed-off-by: Sinan Kaya 
> ---
>  drivers/infiniband/hw/qedr/verbs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/qedr/verbs.c
> b/drivers/infiniband/hw/qedr/verbs.c
> index ccd55f4..db60360 100644
> --- a/drivers/infiniband/hw/qedr/verbs.c
> +++ b/drivers/infiniband/hw/qedr/verbs.c
> @@ -860,7 +860,7 @@ static void doorbell_cq(struct qedr_cq *cq, u32 cons,
> u8 flags)
>   wmb();
>   cq->db.data.agg_flags = flags;
>   cq->db.data.value = cpu_to_le32(cons);
> - writeq(cq->db.raw, cq->db_addr);
> + writeq_relaxed(cq->db.raw, cq->db_addr);
> 
>   /* Make sure write would stick */
>   mmiowb();
> @@ -3338,7 +3338,7 @@ int qedr_post_recv(struct ib_qp *ibqp, struct
> ib_recv_wr *wr,
> 
>   qp->rq.db_data.data.value++;
> 
> - writel(qp->rq.db_data.raw, qp->rq.db);
> + writel_relaxed(qp->rq.db_data.raw, qp->rq.db);
> 
>   /* Make sure write sticks */
>   mmiowb();
> --
> 2.7.4
Acked-by: Michal Kalderon 

Re: [PATCH net-next] qed: Set error code for allocation failures

[Kalderon, Michal]

From: Dan Carpenter 
Sent: Friday, October 27, 2017 9:40 AM

> There are several places where we accidentally return success when
> kcalloc() fails.
> 
> Fixes: fcb39f6c10b2 ("qed: Add mpa buffer descriptors for storing and 
> processing mpa fpdus")
> Signed-off-by: Dan Carpenter 
>
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c 
> b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> index 409041eab189..6366f2ef82b7 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> @@ -2585,7 +2585,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
> struct qed_ll2_cbs cbs;
> u32 mpa_buff_size;
> u16 n_ooo_bufs;
> -   int rc = 0;
> +   int rc;
> int i;
> 
> iwarp_info = _hwfn->p_rdma_info->iwarp;
> @@ -2696,6 +2696,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
> if (rc)
>goto err;
> 
> +   rc = -ENOMEM;
> iwarp_info->partial_fpdus = kcalloc((u16)p_hwfn->p_rdma_info->num_qps,
> 
> sizeof(*iwarp_info->partial_fpdus),
> GFP_KERNEL);
> @@ -2724,7 +2725,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
> for (i = 0; i < data.input.rx_num_desc; i++)
> list_add_tail(_info->mpa_bufs[i].list_entry,
>   _info->mpa_buf_list);
> -   return rc;
> +   return 0;
>  err:
> qed_iwarp_ll2_stop(p_hwfn, p_ptt);

Thanks Dan,
Acked-by: Michal Kalderon 

Re: [PATCH net-next] qed: Set error code for allocation failures

[Kalderon, Michal]

From: Dan Carpenter 
Sent: Friday, October 27, 2017 2:52 PM

>On Fri, Oct 27, 2017 at 05:32:42PM +0800, Yunsheng Lin wrote:
>> > iwarp_info = _hwfn->p_rdma_info->iwarp;
>> > @@ -2696,6 +2696,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
>> > if (rc)
>> > goto err;
>> >
>> > +   rc = -ENOMEM;
>> > iwarp_info->partial_fpdus = kcalloc((u16)p_hwfn->p_rdma_info->num_qps,
>> > sizeof(*iwarp_info->partial_fpdus),
>> > GFP_KERNEL);
>>
>> Does the memory allocated here need to be freed when error happens below?
>>
>
>Hm...  I think you're right that it leaks.  Also I'm confused by the
>qed_iwarp_ll2_alloc_buffers() allocation.  The comment in there says
>that /* buffers will be deallocated by qed_ll2 */ but qed_ll2 is not
>a function name or something which is useful to grep.

Thanks Dan, partial_fpdus is released during qed_iwarp_resc_free, called from 
qed_rdma_resc_free
called on qed_rdma_stop and on failure during qed_rdma_start.
Regarding ll2 buffers. For each successfully allocated buffer we call
qed_iwarp_ll2_post_rx->qed_ll2_post_rx_buffer.
These buffers will get released and freed when we call 
qed_ll2_terminate_connection
called from qed_iwarp_ll2_stop ( which is called during qed_iwarp_ll2_start on 
error ).
thanks,
Michal 

RE: [PATCH] RDMA/qedr: fix build error without ipv6

[Kalderon, Michal]

> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Tuesday, September 05, 2017 5:59 PM
> To: Amrani, Ram ; Kalderon, Michal
> ; Elior, Ariel ;
> Doug Ledford ; Sean Hefty
> ; Hal Rosenstock 
> Cc: Arnd Bergmann ; David S. Miller
> ; Mintz, Yuval ; linux-
> r...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] RDMA/qedr: fix build error without ipv6
> 
> When CONFIG_IPV6 disabled, we run into a link error:
> 
> drivers/infiniband/hw/qedr/qedr_iw_cm.o: In function
> `qedr_addr6_resolve.isra.3':
> qedr_iw_cm.c:(.text+0x4e0): undefined reference to
> `ip6_route_output_flags'
> 
> The ipv6 handling code is obviously not needed here, so this adds a compile-
> time check for the Kconfig symbol in all three places in the code that decide
> between ipv4 and ipv6.
> 
> We don't have to worry about a link error wtih QEDR=y/IPV6=m, as that
> configuration is already prohibited by CONFIG_INFINIBAND depending on "m
> || IPV6 != m".
> 
> Fixes: e411e0587e0d ("RDMA/qedr: Add iWARP connection management
> functions")
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/infiniband/hw/qedr/qedr_iw_cm.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> b/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> index fe9b2b6149b0..2950d3f6ecb8 100644
> --- a/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> +++ b/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> @@ -98,7 +98,8 @@ qedr_iw_mpa_request(void *context, struct
> qed_iwarp_cm_event_params *params)
>   event.event = IW_CM_EVENT_CONNECT_REQUEST;
>   event.status = params->status;
> 
> - if (params->cm_info->ip_version == QED_TCP_IPV4)
> + if (!IS_ENABLED(CONFIG_IPV6) ||
> + params->cm_info->ip_version == QED_TCP_IPV4)
>   qedr_fill_sockaddr4(params->cm_info, );
>   else
>   qedr_fill_sockaddr6(params->cm_info, ); @@ -522,7
> +523,8 @@ int qedr_iw_connect(struct iw_cm_id *cm_id, struct
> iw_cm_conn_param *conn_param)
>   memset(cm_info->local_ip, 0, sizeof(cm_info->local_ip));
>   memset(cm_info->remote_ip, 0, sizeof(cm_info->remote_ip));
> 
> - if (cm_id->remote_addr.ss_family == AF_INET) {
> + if (!IS_ENABLED(CONFIG_IPV6) ||
> + cm_id->remote_addr.ss_family == AF_INET) {
>   cm_info->ip_version = QED_TCP_IPV4;
> 
>   cm_info->remote_ip[0] = ntohl(raddr->sin_addr.s_addr);
> @@ -616,7 +618,8 @@ int qedr_iw_create_listen(struct iw_cm_id *cm_id,
> int backlog)
>   iparams.event_cb = qedr_iw_event_handler;
>   iparams.max_backlog = backlog;
> 
> - if (cm_id->local_addr.ss_family == AF_INET) {
> + if (!IS_ENABLED(CONFIG_IPV6) ||
> + cm_id->local_addr.ss_family == AF_INET) {
>   iparams.ip_version = QED_TCP_IPV4;
>   memset(iparams.ip_addr, 0, sizeof(iparams.ip_addr));
> 
> --
> 2.9.0

Thanks!

Acked-by: Michal Kalderon 

RE: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2

[Kalderon, Michal]

> From: Jason Gunthorpe [mailto:j...@ziepe.ca]
> Sent: Tuesday, April 03, 2018 11:04 PM
> 
> On Tue, Apr 03, 2018 at 07:42:28AM +0000, Kalderon, Michal wrote:
> > > From: Sinan Kaya [mailto:ok...@codeaurora.org]
> > > Sent: Tuesday, April 03, 2018 5:30 AM
> > > To: linux-r...@vger.kernel.org; ti...@codeaurora.org;
> > > sulr...@codeaurora.org
> > > Cc: linux-arm-...@vger.kernel.org;
> > > linux-arm-ker...@lists.infradead.org;
> > > Kalderon, Michal ; Elior, Ariel
> > > ; Doug Ledford ; Jason
> > > Gunthorpe ; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers
> > > on weakly-ordered archs #2
> > >
> > > On 3/22/2018 12:26 PM, Sinan Kaya wrote:
> > > > @@ -860,7 +860,7 @@ static void doorbell_cq(struct qedr_cq *cq,
> > > > u32
> > > cons, u8 flags)
> > > > wmb();
> > > > cq->db.data.agg_flags = flags;
> > > > cq->db.data.value = cpu_to_le32(cons);
> > > > -   writeq(cq->db.raw, cq->db_addr);
> > > > +   writeq_relaxed(cq->db.raw, cq->db_addr);
> > >
> > > Given the direction to get rid of wmb() in front of writeX()
> > > functions, I have been reviewing this code. Under normal
> > > circumstances, I can get rid of all
> > > wmb() as follows.
> > >
> > > However, I started having my doubts now. Are these wmb() used as a
> > > SMP barrier too?
> > > I can't find any smp_Xmb() in drivers/infiniband/hw/qedr directory.
> >
> > Your doubts are in place. You initial patch series modified writel to
> > writel_relaxed Simply removing the wmb is dangerous. The wmb before
> > writel are used to make sure the HW observes the changes in memory
> > before we trigger the doorbell. Smp barriers here wouldn't suffice, as
> > on a single processor. we still need to make sure memory is updated and
> not remained in cache when HW accesses it.
> > Reviewing the qedr barriers, I can find places where this may have not
> > been necessary, But definitely you can't simply remove this wmb barriers.
> 
> As Sinan said, the consensus is that wmb();writel(); is redundant if the only
> purpose of the wmb is to order DMA and system memory.
> 
> So can you review these patches on that basis please? Is the WMB doing
> something else, eg SMP related? If yes, please send a patch adding
> appropriate comments.

Thanks Sinan and Jason for the references and explanations, I've reviewed the 
wmb
usages in qedr and am about to send a patch that replaces two of them with 
smp_wmb
and completely removes two of them that given your explanation, turned out to 
be redundant,
thanks.

> 
> Thanks,
> Jason

RE: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs #2

[Kalderon, Michal]

> From: Sinan Kaya [mailto:ok...@codeaurora.org]
> Sent: Tuesday, April 03, 2018 5:30 AM
> To: linux-r...@vger.kernel.org; ti...@codeaurora.org;
> sulr...@codeaurora.org
> Cc: linux-arm-...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> Kalderon, Michal ; Elior, Ariel
> ; Doug Ledford ; Jason
> Gunthorpe ; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v5 3/3] RDMA/qedr: eliminate duplicate barriers on
> weakly-ordered archs #2
> 
> On 3/22/2018 12:26 PM, Sinan Kaya wrote:
> > @@ -860,7 +860,7 @@ static void doorbell_cq(struct qedr_cq *cq, u32
> cons, u8 flags)
> > wmb();
> > cq->db.data.agg_flags = flags;
> > cq->db.data.value = cpu_to_le32(cons);
> > -   writeq(cq->db.raw, cq->db_addr);
> > +   writeq_relaxed(cq->db.raw, cq->db_addr);
> 
> Given the direction to get rid of wmb() in front of writeX() functions, I have
> been reviewing this code. Under normal circumstances, I can get rid of all
> wmb() as follows.
> 
> However, I started having my doubts now. Are these wmb() used as a SMP
> barrier too?
> I can't find any smp_Xmb() in drivers/infiniband/hw/qedr directory.

Your doubts are in place. You initial patch series modified writel to 
writel_relaxed
Simply removing the wmb is dangerous. The wmb before writel are used to make 
sure the
HW observes the changes in memory before we trigger the doorbell. Smp barriers 
here
wouldn't suffice, as on a single processor. we still need to make sure memory 
is updated
and not remained in cache when HW accesses it.
Reviewing the qedr barriers, I can find places where this may have not been 
necessary, 
But definitely you can't simply remove this wmb barriers. 

> 
> static void doorbell_cq(struct qedr_cq *cq, u32 cons, u8 flags)  {
> -   /* Flush data before signalling doorbell */
> -   wmb();
> cq->db.data.agg_flags = flags;
> cq->db.data.value = cpu_to_le32(cons);
> writeq(cq->db.raw, cq->db_addr); @@ -1870,8 +1868,7 @@ static int
> qedr_update_qp_state(struct qedr_dev *dev,
>  */
> 
> if (rdma_protocol_roce(>ibdev, 1)) {
> -   wmb();
> -   writel_relaxed(qp->rq.db_data.raw, qp->rq.db);
> +   writel(qp->rq.db_data.raw, qp->rq.db);
> /* Make sure write takes effect */
> mmiowb();
> }
> @@ -3275,8 +3272,7 @@ int qedr_post_send(struct ib_qp *ibqp, struct
> ib_send_wr *wr,
>  * unchanged). For performance reasons we avoid checking for this
>  * redundant doorbell.
>  */
> -   wmb();
> -   writel_relaxed(qp->sq.db_data.raw, qp->sq.db);
> +   writel(qp->sq.db_data.raw, qp->sq.db);
> 
> /* Make sure write sticks */
> mmiowb();
> @@ -3362,9 +3358,6 @@ int qedr_post_recv(struct ib_qp *ibqp, struct
> ib_recv_wr *wr,
> 
> qedr_inc_sw_prod(>rq);
> 
> -   /* Flush all the writes before signalling doorbell */
> -   wmb();
> 
> 
> 
> 
> 
> --
> Sinan Kaya
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
> Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
> Foundation Collaborative Project.

RE: [PATCH] qlogic/qed: Constify *pkt_type_str[]

[Kalderon, Michal]

> From: Hernán Gonzalez [mailto:her...@vanguardiasur.com.ar]
> Sent: Wednesday, February 28, 2018 12:32 AM
> 
> Note: This is compile only tested as I have no access to the hw.
> Constifying and declaring as static saves 24 bytes.
> 
> add/remove: 0/1 grow/shrink: 0/0 up/down: 0/-24 (-24)
> Function old new   delta
> pkt_type_str  24   - -24
> Total: Before=3599256, After=3599232, chg -0.00%
> 
> Signed-off-by: Hernán Gonzalez 
> ---
>  drivers/net/ethernet/qlogic/qed/qed_iwarp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> index ca4a81d..03ad4ee 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> @@ -1784,7 +1784,7 @@ enum qed_iwarp_mpa_pkt_type {
>  /* fpdu can be fragmented over maximum 3 bds: header, partial mpa,
> unaligned */  #define QED_IWARP_MAX_BDS_PER_FPDU 3
> 
> -char *pkt_type_str[] = {
> +static const char * const pkt_type_str[] = {
>   "QED_IWARP_MPA_PKT_PACKED",
>   "QED_IWARP_MPA_PKT_PARTIAL",
>   "QED_IWARP_MPA_PKT_UNALIGNED"
> --
> 2.7.4

Thanks

Acked-by: Michal Kalderon 

RE: [PATCH v2 2/2] RDMA/core: Initialize port_num in qp_attr

[Kalderon, Michal]

> From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
> ow...@vger.kernel.org] On Behalf Of Marciniszyn, Mike
> > Initialize the port_num for iWARP in rdma_init_qp_attr.
> >
> > Fixes: 5ecce4c9b17b("Check port number supplied by user verbs cmds")
> > Cc:  # v2.6.14+
> > Reviewed-by: Steve Wise 
> > Signed-off-by: Mustafa Ismail 
> 
Why is the second patch required if you only validate the port_num if the 
IB_QP_PORT mask is on? 
Given the first patch [PATCH v2 1/2] RDMA/uverbs: Fix the check for port 
number, this one seems
redundant. 

Thanks,
Michal

Re: [PATCH v2 2/2] RDMA/core: Initialize port_num in qp_attr

[Kalderon, Michal]

From: Ismail, Mustafa 
Sent: Wednesday, July 19, 2017 5:38 PM

> > > > Fixes: 5ecce4c9b17b("Check port number supplied by user verbs cmds")
> > > > Cc:  # v2.6.14+
> > > > Reviewed-by: Steve Wise 
> > > > Signed-off-by: Mustafa Ismail 
> > >
> > Why is the second patch required if you only validate the port_num if the
> > IB_QP_PORT mask is on?
> > Given the first patch [PATCH v2 1/2] RDMA/uverbs: Fix the check for port
> > number, this one seems redundant.
> Strictly speaking it is not required, but we felt it safer to always return a 
> valid port number
> as is done in the IB case.

It's not always initialized in the IB case either. More than that if at this 
point you'll
initialize it for ib as well you'll get a failure on ib_modify_qp_is_ok, since 
when
transitioning to RTR  / RTS providing IB_QP_PORT is not a valid option.
We actually hit this issue when running rping over RoCE. (prior to your fix i 
mean ) 
I agree that in general there's no real harm, but it seems a bit out of 
context, and if we
make the change common for ib/iwarp we'll have to modify ib_modify_qp_is_ok 
which 
is written close to the spec. 

thanks,
Michal

RE: [PATCH] qed: code indent should use tabs where possible

[Kalderon, Michal]

> From: Rohit Visavalia [mailto:rohit.visava...@softnautics.com]
> Sent: Thursday, January 25, 2018 12:26 PM
> To: Elior, Ariel ; Dept-Eng Everest Linux L2  engeverestlinu...@cavium.com>
> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; Rohit Visavalia
> 
> Subject: [PATCH] qed: code indent should use tabs where possible
> 
> Issue found by checkpatch.
> 
> Signed-off-by: Rohit Visavalia 
> ---
>  drivers/net/ethernet/qlogic/qed/qed_rdma.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> index b7abb8205d3a..ae399c48d4a3 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> @@ -360,13 +360,13 @@ static void qed_rdma_resc_free(struct qed_hwfn
> *p_hwfn)
> 
>  static void qed_rdma_free_tid(void *rdma_cxt, u32 itid)  {
> -struct qed_hwfn *p_hwfn = (struct qed_hwfn *)rdma_cxt;
> + struct qed_hwfn *p_hwfn = (struct qed_hwfn *)rdma_cxt;
> 
> -DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "itid = %08x\n", itid);
> + DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "itid = %08x\n", itid);
> 
> -spin_lock_bh(_hwfn->p_rdma_info->lock);
> -qed_bmap_release_id(p_hwfn, _hwfn->p_rdma_info->tid_map,
> itid);
> -spin_unlock_bh(_hwfn->p_rdma_info->lock);
> + spin_lock_bh(_hwfn->p_rdma_info->lock);
> + qed_bmap_release_id(p_hwfn, _hwfn->p_rdma_info->tid_map,
> itid);
> + spin_unlock_bh(_hwfn->p_rdma_info->lock);
>  }
> 
>  static void qed_rdma_free_reserved_lkey(struct qed_hwfn *p_hwfn)
> --
> 2.16.1

Thanks and apologies,

Acked-by: Michal Kalderon 

RE: [PATCH v2] RDMA/qedr: Remove enumerated type qed_roce_ll2_tx_dest

[Kalderon, Michal]

> From: Nick Desaulniers 
> Sent: Friday, September 28, 2018 1:30 AM
> 
> External Email
> 
> On Thu, Sep 27, 2018 at 1:57 PM Nathan Chancellor
>  wrote:
> >
> > Clang warns when one enumerated type is explicitly converted to another.
> >
> > drivers/infiniband/hw/qedr/qedr_roce_cm.c:198:28: warning: implicit
> > conversion from enumeration type 'enum qed_roce_ll2_tx_dest' to
> > different enumeration type 'enum qed_ll2_tx_dest' [-Wenum-conversion]
> > ll2_tx_pkt.tx_dest = pkt->tx_dest;
> >~ ~^~~
> > 1 warning generated.
> >
> > Turns out that QED_ROCE_LL2_TX_DEST_NW and
> QED_ROCE_LL2_TX_DEST_LB are
> > only used once in the whole tree and QED_ROCE_LL2_TX_DEST_MAX is
> used
> > nowhere. Remove them and use the equivalent values from
> > qed_ll2_tx_dest in their place.
> >
> > Reported-by: Nick Desaulniers 
> > Signed-off-by: Nathan Chancellor 
> > ---
> >
> > v1 -> v2:
> >
> > * Rather than using an explicit cast, just convert the uses to the
> >   appropriate values and delete the duplicated enum.
> >
> >  drivers/infiniband/hw/qedr/qedr_roce_cm.c |  4 ++--
> >  include/linux/qed/qed_rdma_if.h   | 11 +--
> >  2 files changed, 3 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > b/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > index 85578887421b..e1ac2fd60bb1 100644
> > --- a/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > +++ b/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > @@ -519,9 +519,9 @@ static inline int qedr_gsi_build_packet(struct
> qedr_dev *dev,
> > }
> >
> > if (ether_addr_equal(udh.eth.smac_h, udh.eth.dmac_h))
> > -   packet->tx_dest = QED_ROCE_LL2_TX_DEST_LB;
> > +   packet->tx_dest = QED_LL2_TX_DEST_LB;
> > else
> > -   packet->tx_dest = QED_ROCE_LL2_TX_DEST_NW;
> > +   packet->tx_dest = QED_LL2_TX_DEST_NW;
> 
> Yep, looks like these were the only two sites using enum
> qed_roce_ll2_tx_dest in the whole kernel.  The new enum values match the
> previous enum values, so this is no functional change while simplifying the
> code (one less enum to get wrong).
> Reviewed-by: Nick Desaulniers 
> 
> >
> > packet->roce_mode = roce_mode;
> > memcpy(packet->header.vaddr, ud_header_buffer, header_size);
> > diff --git a/include/linux/qed/qed_rdma_if.h
> > b/include/linux/qed/qed_rdma_if.h index df4d13f7e191..d15f8e4815e3
> > 100644
> > --- a/include/linux/qed/qed_rdma_if.h
> > +++ b/include/linux/qed/qed_rdma_if.h
> > @@ -39,15 +39,6 @@
> >  #include 
> >  #include 
> >
> > -enum qed_roce_ll2_tx_dest {
> > -   /* Light L2 TX Destination to the Network */
> > -   QED_ROCE_LL2_TX_DEST_NW,
> > -
> > -   /* Light L2 TX Destination to the Loopback */
> > -   QED_ROCE_LL2_TX_DEST_LB,
> > -   QED_ROCE_LL2_TX_DEST_MAX
> > -};
> > -
> >  #define QED_RDMA_MAX_CNQ_SIZE   (0x)
> >
> >  /* rdma interface */
> > @@ -581,7 +572,7 @@ struct qed_roce_ll2_packet {
> > int n_seg;
> > struct qed_roce_ll2_buffer payload[RDMA_MAX_SGE_PER_SQ_WQE];
> > int roce_mode;
> > -   enum qed_roce_ll2_tx_dest tx_dest;
> > +   enum qed_ll2_tx_dest tx_dest;
> >  };
> >
> >  enum qed_rdma_type {
> > --
> > 2.19.0
> >
> 
> 
> --
> Thanks,
> ~Nick Desaulniers

Thanks, definitely  looks cleaner. 
Acked-by: Michal Kalderon 

RE: [PATCH 1/1] net: ethernet: broadcom: fix improper return value

[Kalderon, Michal]

> From: Pan Bian 
> 
> Marco BNX2X_ALLOC_AND_SET(arr, lbl, func) calls kmalloc() to allocate
> memory, and jumps to label "lbl" if the allocation fails. Label "lbl"
> first cleans memory and then returns variable rc. Before calling the macro, 
> the
> value of variable rc is 0. Because 0 means no error, the callers of
> bnx2x_init_firmware() may be misled. This patch fixes the bug, assigning "-
> ENOMEM" to rc before calling macro NX2X_ALLOC_AND_SET().
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=189141
> 
> Signed-off-by: Pan Bian 

The title is wrong from net-next; It should have been "bnx2x: Fix improper 
return value".

Acked-by: Michal Kalderon