RE: [PATCH net-next] qed*: Utilize FW 8.33.11.0
> From: Jason Gunthorpe [mailto:j...@ziepe.ca]
> Sent: Wednesday, March 28, 2018 1:27 AM
>
> On Tue, Mar 27, 2018 at 08:50:24PM +0300, Leon Romanovsky wrote:
> > On Tue, Mar 27, 2018 at 05:41:51PM +, Kalderon, Michal wrote:
> > > > From: Jason Gunthorpe [mailto:j...@ziepe.ca]
> > > > Sent: Tuesday, March 27, 2018 12:18 AM
> > > > > diff --git a/drivers/infiniband/hw/qedr/main.c
> > > > > b/drivers/infiniband/hw/qedr/main.c
> > > > > index db4bf97..7dbbe6d 100644
> > > > > +++ b/drivers/infiniband/hw/qedr/main.c
> > > > > @@ -51,6 +51,7 @@
> > > > > MODULE_DESCRIPTION("QLogic 40G/100G ROCE Driver");
> > > > > MODULE_AUTHOR("QLogic Corporation"); MODULE_LICENSE("Dual
> > > > BSD/GPL");
> > > > > +MODULE_VERSION(QEDR_MODULE_VERSION);
> > > > >
> > > > > #define QEDR_WQ_MULTIPLIER_DFT (3)
> > > > >
> > > > > diff --git a/drivers/infiniband/hw/qedr/qedr.h
> > > > > b/drivers/infiniband/hw/qedr/qedr.h
> > > > > index 86d4511..ab0d411 100644
> > > > > +++ b/drivers/infiniband/hw/qedr/qedr.h
> > > > > @@ -43,6 +43,8 @@
> > > > > #include "qedr_hsi_rdma.h"
> > > > >
> > > > > #define QEDR_NODE_DESC "QLogic 579xx RoCE HCA"
> > > > > +#define QEDR_MODULE_VERSION "8.33.11.20"
> > > > > +
> > > >
> > > > I thought we had a general prohibition against versions like this
> > > > in mainline drivers? And what does this hunk have to do with
> > > > supporting new firmware?
> > > >
> > > I'm assuming you refer only to rdma in regards to version
> > > prohibition right ? as looking at all other vendors (including
> > > Mellanox) all have module versions under net/ why is rdma different
> > > in this way ? I now searched back mails on the topic and found an
> > > email from Leon where he stated: " I am strongly against module
> > > versions. You should rely on official kernel version." But it's not
> > > always the inbox driver that is installed or probed, the kernel
> > > version is not enough. Given different distros, vanilla kernels,
> > > out of box drivers, etc... it is essential for us that based on logs
> > > And modinfo we can determine the qed* drivers that are running.
> >
> > We actually stopped to maintain driver versions, just ensure that
> > inbox, upstream and MLNX_OFED have different names.
> >
> > The discussion thread is here
> > https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-June/
> > 004426.html
> > https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-June/
> > 004441.html
>
> Hmm, Linus pretty clearly said No to MODULE_VERSION and related.
>
> So I can't take this hunk, and you shouldn't do in ethernet either, I guess.
>
> Honestly the idea that this version will somehow have meaning in the distro
> kernels is pretty far fetched. You think distros will backport patches
> changing
> version # in any way that will make some kind of sense?
Leon and Jason, thanks for the references and explanations.
I'll send a V2 without a version bump.
Michal
>
> Jason
RE: [PATCH net-next] qed*: Utilize FW 8.33.11.0
> From: Jason Gunthorpe [mailto:j...@ziepe.ca]
> Sent: Tuesday, March 27, 2018 12:18 AM
> > diff --git a/drivers/infiniband/hw/qedr/main.c
> > b/drivers/infiniband/hw/qedr/main.c
> > index db4bf97..7dbbe6d 100644
> > +++ b/drivers/infiniband/hw/qedr/main.c
> > @@ -51,6 +51,7 @@
> > MODULE_DESCRIPTION("QLogic 40G/100G ROCE Driver");
> > MODULE_AUTHOR("QLogic Corporation"); MODULE_LICENSE("Dual
> BSD/GPL");
> > +MODULE_VERSION(QEDR_MODULE_VERSION);
> >
> > #define QEDR_WQ_MULTIPLIER_DFT (3)
> >
> > diff --git a/drivers/infiniband/hw/qedr/qedr.h
> > b/drivers/infiniband/hw/qedr/qedr.h
> > index 86d4511..ab0d411 100644
> > +++ b/drivers/infiniband/hw/qedr/qedr.h
> > @@ -43,6 +43,8 @@
> > #include "qedr_hsi_rdma.h"
> >
> > #define QEDR_NODE_DESC "QLogic 579xx RoCE HCA"
> > +#define QEDR_MODULE_VERSION "8.33.11.20"
> > +
>
> I thought we had a general prohibition against versions like this in mainline
> drivers? And what does this hunk have to do with supporting new firmware?
>
> Jason
I'm assuming you refer only to rdma in regards to version prohibition right ?
as looking at all other vendors
(including Mellanox) all have module versions under net/ why is rdma different
in this way ?
I now searched back mails on the topic and found an email from Leon where he
stated:
" I am strongly against module versions. You should rely on official kernel
version."
But it's not always the inbox driver that is installed or probed, the kernel
version is not enough.
Given different distros, vanilla kernels, out of box drivers, etc... it is
essential for us that based on
logs And modinfo we can determine the qed* drivers that are running.
We have received complaints that our qedr module doesn't have a version whereas
all of our other
components do (qed, qede, qedi, qedf). We decided to add the qedr version with
the next version
update to align with the rest of the components.
We can move the driver version bump into a different commit for all components,
just made
sense to Add it to this one as it is the root of the version update.
Let me know if you think it is essential and I'll make the change for v2
Thanks,
Michal
RE: [PATCH net 2/2] qed: Fix non TCP packets should be dropped on iWARP ll2 connection
> From: Joe Perches [mailto:j...@perches.com]
> Sent: Wednesday, March 14, 2018 3:03 PM
> To: Kalderon, Michal ;
> da...@davemloft.net
> Cc: netdev@vger.kernel.org; dledf...@redhat.com; j...@mellanox.com;
> linux-r...@vger.kernel.org; Elior, Ariel
> Subject: Re: [PATCH net 2/2] qed: Fix non TCP packets should be dropped on
> iWARP ll2 connection
>
> [This sender failed our fraud detection checks and may not be who they
> appear to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing]
>
> On Wed, 2018-03-14 at 14:49 +0200, Michal Kalderon wrote:
> > FW workaround. The iWARP LL2 connection did not expect TCP packets to
> > arrive on it's connection. The fix drops any non-tcp packets
> []
> > diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> > b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> []
> > @@ -1703,6 +1703,13 @@ qed_iwarp_parse_rx_pkt(struct qed_hwfn
> *p_hwfn,
> > iph = (struct iphdr *)((u8 *)(ethh) + eth_hlen);
> >
> > if (eth_type == ETH_P_IP) {
> > + if (iph->protocol != IPPROTO_TCP) {
> > + DP_NOTICE(p_hwfn,
> > + "Unexpected ip protocol on ll2 %x\n",
> > + iph->protocol);
> > + return -EINVAL;
> > + }
>
> Perhaps this should be ratelimited.
The rate of the packets that could arrive here is very low. It has to do with a
corner case
Of RoCEv2 packets being sent to a device that was enabled with iWARP.
>
> > +
> > cm_info->local_ip[0] = ntohl(iph->daddr);
> > cm_info->remote_ip[0] = ntohl(iph->saddr);
> > cm_info->ip_version = TCP_IPV4; @@ -1711,6 +1718,14 @@
> > qed_iwarp_parse_rx_pkt(struct qed_hwfn *p_hwfn,
> > *payload_len = ntohs(iph->tot_len) - ip_hlen;
> > } else if (eth_type == ETH_P_IPV6) {
> > ip6h = (struct ipv6hdr *)iph;
> > +
> > + if (ip6h->nexthdr != IPPROTO_TCP) {
> > + DP_NOTICE(p_hwfn,
> > + "Unexpected ip protocol on ll2 %x\n",
> > + iph->protocol);
> > + return -EINVAL;
>
> here too
>
> > + }
> > +
> > for (i = 0; i < 4; i++) {
> > cm_info->local_ip[i] =
> > ntohl(ip6h->daddr.in6_u.u6_addr32[i]);
RE: [PATCH net] qed: Use after free in qed_rdma_free()
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Tuesday, March 13, 2018 11:10 AM
> To: Elior, Ariel ; Kalderon, Michal
>
> Cc: Dept-Eng Everest Linux L2 ;
> netdev@vger.kernel.org; kernel-janit...@vger.kernel.org
> Subject: [PATCH net] qed: Use after free in qed_rdma_free()
>
> We're dereferencing "p_hwfn->p_rdma_info" but that is freed on the line
> before in qed_rdma_resc_free(p_hwfn).
>
> Fixes: 9de506a547c0 ("qed: Free RoCE ILT Memory on rmmod qedr")
> Signed-off-by: Dan Carpenter
>
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> index f3ee6538b553..a411f9c702a1 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> @@ -379,8 +379,8 @@ static void qed_rdma_free(struct qed_hwfn
> *p_hwfn)
> DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "Freeing RDMA\n");
>
> qed_rdma_free_reserved_lkey(p_hwfn);
> - qed_rdma_resc_free(p_hwfn);
> qed_cxt_free_proto_ilt(p_hwfn, p_hwfn->p_rdma_info->proto);
> + qed_rdma_resc_free(p_hwfn);
> }
>
> static void qed_rdma_get_guid(struct qed_hwfn *p_hwfn, u8 *guid)
Thanks,
Acked-by: Michal Kalderon
RE: [PATCH V2 net] qed: Free RoCE ILT Memory on rmmod qedr
> From: Yuval Mintz [mailto:yuv...@mellanox.com] > Sent: Monday, March 05, 2018 11:24 PM > To: Kalderon, Michal ; > da...@davemloft.net > Cc: netdev@vger.kernel.org; dledf...@redhat.com; Jason Gunthorpe > ; linux-r...@vger.kernel.org; Elior, Ariel > > Subject: RE: [PATCH V2 net] qed: Free RoCE ILT Memory on rmmod qedr > > > - /* Free Task CXT */ > > + /* Free Task CXT ( Intentionally RoCE as task-id is shared between > > +* RoCE and iWARP > > +*/ > > Broken parenthesis In comment... Thanks Yuval, V3 on its way
RE: [PATCH net] qed: Free RoCE ILT Memory on rmmod qedr
> From: Leon Romanovsky [mailto:l...@kernel.org] > Sent: Monday, March 05, 2018 11:41 AM > To: Kalderon, Michal > Cc: da...@davemloft.net; netdev@vger.kernel.org; dledf...@redhat.com; > j...@mellanox.com; linux-r...@vger.kernel.org; Elior, Ariel > > Subject: Re: [PATCH net] qed: Free RoCE ILT Memory on rmmod qedr > > On Mon, Mar 05, 2018 at 11:04:11AM +0200, Michal Kalderon wrote: > > Rdma requires ILT Memory to be allocated for it's QPs. > > Each ILT entry points to a page used by several Rdma QPs. > > To avoid allocating all the memory in advance, the rdma > > implementation dynamically allocates memory as more QPs are > > added, however it does not dynamically free the memory. > > The memory should have been freed on rmmod qedr, but isn't. > > This patch adds the memory freeing on rmmod qedr (currently > > it will be freed with qed is removed). > > > > An outcome of this bug, is that if qedr is unloaded and loaded > > without unloaded qed, there will be no more RoCE traffic. > > > > The reason these are related, is that the logic of detecting the > > first QP ever opened is by asking whether ILT memory for RoCE has > > been allocated. > > > > In addition, this patch modifies freeing of the Task context to > > always use the PROTOCOLID_ROCE and not the protocol passed, > > this is because task context for iWARP and ROCE both use the > > ROCE protocol id, as opposed to the connection context. > > > > Fixes: dbb799c39717e7b7 > > Your fixes line was truncated. > > Thanks Thanks for noticing, sending V2
RE: [PATCH] qlogic/qed: Constify *pkt_type_str[]
> 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] qed: code indent should use tabs where possible
> 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: netdev@vger.kernel.org; linux-ker...@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(&p_hwfn->p_rdma_info->lock);
> -qed_bmap_release_id(p_hwfn, &p_hwfn->p_rdma_info->tid_map,
> itid);
> -spin_unlock_bh(&p_hwfn->p_rdma_info->lock);
> + spin_lock_bh(&p_hwfn->p_rdma_info->lock);
> + qed_bmap_release_id(p_hwfn, &p_hwfn->p_rdma_info->tid_map,
> itid);
> + spin_unlock_bh(&p_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 net-next] qed: Set error code for allocation failures
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 = &p_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(&iwarp_info->mpa_bufs[i].list_entry,
> &iwarp_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
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 = &p_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 v2 net-next 06/12] qed: Add LL2 slowpath handling
From: linux-rdma-ow...@vger.kernel.org on behalf of David Miller >From: "Kalderon, Michal" >Date: Thu, 5 Oct 2017 20:27:22 + > >> The spinlock is required for the case that rx buffers are posted >> from a different thread, where it could be run simultaneously to the >> rxq_completion. > >This only brings us back to my original argument, if the lock is >necessary in order to synchronize with those paths, how can you >possible drop the lock safely here? > >Is it because you re-read the head and tail pointers of the queue each >time around the loop? It's safe to drop the lock here because the implementation of the queue (qed_chain) maintains both a consumer pointer (tail) indices and producer pointer(head). The main loop reads the FWs value and stores it as the "new cons" and traverses the queue only until it reaches the FWs consumer value. The post function adds more buffers to the end of the queue and updates the producer. They will not affect the consumer pointers. So posting of buffers doesn't affect the main loop. The resources that are protected by the lock and accessed inside the loop and from post-buffers are three linked-lists, free-descq, posting_descq and active_descq, their head and tail are read on every access (elements are removed and moved between the lists). Following this discussion, it looks like there was no need to take the lock in the outer function, but only around the places that access these lists, this is a delicate change which affects the ll2 clients (iscsi,fcoe,roce,iwarp). As this series is rather large as it is and is intended for iWARP, please consider taking this one as it is. Since it doesn't change existing functionality and doesn't introduce risk to other components. thanks, Michal -- 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 v2 net-next 06/12] qed: Add LL2 slowpath handling
From: David Miller
Sent: Thursday, October 5, 2017 10:06 PM
>> From: Kalderon, Michal
>> Sent: Tuesday, October 3, 2017 9:05 PM
>> To: David Miller
>>>From: David Miller
>>>Sent: Tuesday, October 3, 2017 8:17 PM
>>>>> @@ -423,6 +423,41 @@ static void qed_ll2_rxq_parse_reg(struct qed_hwfn
>>>>> *p_hwfn,
>>>>> }
>>>>>
>>>>> static int
>>>>> +qed_ll2_handle_slowpath(struct qed_hwfn *p_hwfn,
>>>>> + struct qed_ll2_info *p_ll2_conn,
>>>>> + union core_rx_cqe_union *p_cqe,
>>>>> + unsigned long *p_lock_flags)
>>>>> +{
>>>>...
>>>>> + spin_unlock_irqrestore(&p_rx->lock, *p_lock_flags);
>>>>> +
>>>>
>>>>You can't drop this lock.
>>>>
>>>>Another thread can enter the loop of our caller and process RX queue
>>>>entries, then we would return from here and try to process the same
>>>>entries again.
>>>
>>>The lock is there to synchronize access to chains between
>>>qed_ll2_rxq_completion
>>>and qed_ll2_post_rx_buffer. qed_ll2_rxq_completion can't be called from
>>>different threads, the light l2 uses the single sp status block we have.
>>>The reason we release the lock is to avoid a deadlock where as a result of
>>>calling
>>>upper-layer driver it will potentially post additional rx-buffers.
>>
>> Dave, is there anything else needed from me on this?
>> Noticed the series is still in "Changes Requested".
>
>I'm still not convinced that the lock dropping is legitimate. What if a
>spurious interrupt arrives?
We're in the context of a dedicated tasklet here. So even if there is a spurious
interrupt, we're covered.
>
>If the execution path in the caller is serialized for some reason, why
>are you using a spinlock and don't use that serialization for the mutual
>exclusion necessary for these queue indexes?
Posting of rx-buffers back to the light-l2 is not always serialized and can be
called from different threads depending on the light-l2 client.
Unlocking before calling the callback enables the cb function to post rx
buffers,
in this case, serialization protects us. The spinlock is required for the case
that rx buffers are posted from a different thread, where it could be run
simultaneously to the rxq_completion.
Re: [PATCH v2 net-next 06/12] qed: Add LL2 slowpath handling
From: Kalderon, Michal
Sent: Tuesday, October 3, 2017 9:05 PM
To: David Miller
>From: David Miller
>Sent: Tuesday, October 3, 2017 8:17 PM
>>> @@ -423,6 +423,41 @@ static void qed_ll2_rxq_parse_reg(struct qed_hwfn
>>> *p_hwfn,
>>> }
>>>
>>> static int
>>> +qed_ll2_handle_slowpath(struct qed_hwfn *p_hwfn,
>>> + struct qed_ll2_info *p_ll2_conn,
>>> + union core_rx_cqe_union *p_cqe,
>>> + unsigned long *p_lock_flags)
>>> +{
>>...
>>> + spin_unlock_irqrestore(&p_rx->lock, *p_lock_flags);
>>> +
>>
>>You can't drop this lock.
>>
>>Another thread can enter the loop of our caller and process RX queue
>>entries, then we would return from here and try to process the same
>>entries again.
>
>The lock is there to synchronize access to chains between
>qed_ll2_rxq_completion
>and qed_ll2_post_rx_buffer. qed_ll2_rxq_completion can't be called from
>different threads, the light l2 uses the single sp status block we have.
>The reason we release the lock is to avoid a deadlock where as a result of
>calling
>upper-layer driver it will potentially post additional rx-buffers.
Dave, is there anything else needed from me on this?
Noticed the series is still in "Changes Requested".
thanks,
Michal
Re: [PATCH v2 net-next 06/12] qed: Add LL2 slowpath handling
From: Leon Romanovsky
Sent: Tuesday, October 3, 2017 4:26 PM
>On Tue, Oct 03, 2017 at 11:54:56AM +0300, Michal Kalderon wrote:
>> For iWARP unaligned MPA flow, a slowpath event of flushing an
>> MPA connection that entered an unaligned state is required.
>> The flush ramrod is received on the ll2 queue, and a pre-registered
>> callback function is called to handle the flush event.
>>
>> Signed-off-by: Michal Kalderon
>> Signed-off-by: Ariel Elior
>> ---
>> drivers/net/ethernet/qlogic/qed/qed_ll2.c | 40
>> +--
>> include/linux/qed/qed_ll2_if.h| 5
>> 2 files changed, 43 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/qlogic/qed/qed_ll2.c
>> b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
>> index 8eb9645..047f556 100644
>> --- a/drivers/net/ethernet/qlogic/qed/qed_ll2.c
>> +++ b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
>> @@ -423,6 +423,41 @@ static void qed_ll2_rxq_parse_reg(struct qed_hwfn
>> *p_hwfn,
>> }
>>
>> static int
>> +qed_ll2_handle_slowpath(struct qed_hwfn *p_hwfn,
>> + struct qed_ll2_info *p_ll2_conn,
>> + union core_rx_cqe_union *p_cqe,
>> + unsigned long *p_lock_flags)
>> +{
>> + struct qed_ll2_rx_queue *p_rx = &p_ll2_conn->rx_queue;
>> + struct core_rx_slow_path_cqe *sp_cqe;
>> +
>> + sp_cqe = &p_cqe->rx_cqe_sp;
>> + if (sp_cqe->ramrod_cmd_id != CORE_RAMROD_RX_QUEUE_FLUSH) {
>> + DP_NOTICE(p_hwfn,
>> + "LL2 - unexpected Rx CQE slowpath
>> ramrod_cmd_id:%d\n",
>> + sp_cqe->ramrod_cmd_id);
>> + return -EINVAL;
>> + }
>> +
>> + if (!p_ll2_conn->cbs.slowpath_cb) {
>> + DP_NOTICE(p_hwfn,
>> + "LL2 - received RX_QUEUE_FLUSH but no callback was
>> provided\n");
>> + return -EINVAL;
>> + }
>> +
>> + spin_unlock_irqrestore(&p_rx->lock, *p_lock_flags);
>
>Interesting, you are unlock the lock which was taken in upper layer.
>It is not actual error, but chances to have such error are pretty high
>(for example, after refactoring).
Thanks. Ensuring that the lock will only be unlocked inside the calling
function would make
the calling function long and less readable.
The risk exists, but I think the fact that p_lock_flags is passed as parameter
should
give a strong indication in the future that lock should be handled delicately.
Re: [PATCH v2 net-next 06/12] qed: Add LL2 slowpath handling
From: David Miller
Sent: Tuesday, October 3, 2017 8:17 PM
>> @@ -423,6 +423,41 @@ static void qed_ll2_rxq_parse_reg(struct qed_hwfn
>> *p_hwfn,
>> }
>>
>> static int
>> +qed_ll2_handle_slowpath(struct qed_hwfn *p_hwfn,
>> + struct qed_ll2_info *p_ll2_conn,
>> + union core_rx_cqe_union *p_cqe,
>> + unsigned long *p_lock_flags)
>> +{
>...
>> + spin_unlock_irqrestore(&p_rx->lock, *p_lock_flags);
>> +
>
>You can't drop this lock.
>
>Another thread can enter the loop of our caller and process RX queue
>entries, then we would return from here and try to process the same
>entries again.
The lock is there to synchronize access to chains between qed_ll2_rxq_completion
and qed_ll2_post_rx_buffer. qed_ll2_rxq_completion can't be called from
different threads, the light l2 uses the single sp status block we have.
The reason we release the lock is to avoid a deadlock where as a result of
calling
upper-layer driver it will potentially post additional rx-buffers.
Re: [PATCH net-next 2/4] qed: Add iWARP out of order support
From: Leon Romanovsky
Sent: Tuesday, September 19, 2017 8:45 PM
On Tue, Sep 19, 2017 at 08:26:17PM +0300, Michal Kalderon wrote:
>> iWARP requires OOO support which is already provided by the ll2
>> interface (until now was used only for iSCSI offload).
>> The changes mostly include opening a ll2 dedicated connection for
>> OOO and notifiying the FW about the handle id.
>>
>> Signed-off-by: Michal Kalderon
>> Signed-off-by: Ariel Elior
>> ---
>> drivers/net/ethernet/qlogic/qed/qed_iwarp.c | 44
>> +
>> drivers/net/ethernet/qlogic/qed/qed_iwarp.h | 11 +++-
>> drivers/net/ethernet/qlogic/qed/qed_rdma.c | 7 +++--
>> 3 files changed, 59 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
>> b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
>> index 9d989c9..568e985 100644
>> --- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
>> +++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
>> @@ -41,6 +41,7 @@
>> #include "qed_rdma.h"
>> #include "qed_reg_addr.h"
>> #include "qed_sp.h"
>> +#include "qed_ooo.h"
>>
>> #define QED_IWARP_ORD_DEFAULT32
>> #define QED_IWARP_IRD_DEFAULT32
>> @@ -119,6 +120,13 @@ static void qed_iwarp_cid_cleaned(struct qed_hwfn
>> *p_hwfn, u32 cid)
>> spin_unlock_bh(&p_hwfn->p_rdma_info->lock);
>> }
>>
>> +void qed_iwarp_init_fw_ramrod(struct qed_hwfn *p_hwfn,
>> + struct iwarp_init_func_params *p_ramrod)
>> +{
>> + p_ramrod->ll2_ooo_q_index = RESC_START(p_hwfn, QED_LL2_QUEUE) +
>> + p_hwfn->p_rdma_info->iwarp.ll2_ooo_handle;
>> +}
>> +
>> static int qed_iwarp_alloc_cid(struct qed_hwfn *p_hwfn, u32 *cid)
>> {
>> int rc;
>> @@ -1876,6 +1884,16 @@ static int qed_iwarp_ll2_stop(struct qed_hwfn
>> *p_hwfn, struct qed_ptt *p_ptt)
>> iwarp_info->ll2_syn_handle = QED_IWARP_HANDLE_INVAL;
>> }
>>
>> + if (iwarp_info->ll2_ooo_handle != QED_IWARP_HANDLE_INVAL) {
>> + rc = qed_ll2_terminate_connection(p_hwfn,
>> + iwarp_info->ll2_ooo_handle);
>> + if (rc)
>> + DP_INFO(p_hwfn, "Failed to terminate ooo
>> connection\n");
>
>What exactly will you do with this knowledge? Anyway you are not
>interested in return values of qed_ll2_terminate_connection function in
>this place and other places too.
>
>Why don't you handle EAGAIN returned from the qed_ll2_terminate_connection()?
>
>Thanks
Thanks for pointing this out, you're right we could have ignored the return
code, as there's
not much we can do at this point if it failed. But I still feel failures are
worth knowing about,
and could help in analysis if they unexpectedly lead to another issue.
As for EAGAIN, it is very unlikely that we'll get this return code. Will
consider adding generic
handling for this as a separate patch, as this currently isn't handled in any
of the ll2 flows.
thanks,
Re: [PATCH net-next 3/4] qed: Fix maximum number of CQs for iWARP
From: Leon Romanovsky Sent: Tuesday, September 19, 2017 8:46 PM On Tue, Sep 19, 2017 at 08:26:18PM +0300, Michal Kalderon wrote: >> The maximum number of CQs supported is bound to the number >> of connections supported, which differs between RoCE and iWARP. >> >> This fixes a crash that occurred in iWARP when running 1000 sessions >> using perftest. >> >> Signed-off-by: Michal Kalderon >> Signed-off-by: Ariel Elior >> --- > >It is worth to add Fixes line. > >Thanks The original code was there before we had iWARP support, so this doesn't exactly fix an older commit, but fixes iWARP code in general.
Re: [PATCH net-next 00/12] qed: Add iWARP support for QL4xxxx
From: David Miller Sent: Monday, July 3, 2017 11:59 AM > You really have to compile test your work and do something with > the warnings: > drivers/net/ethernet/qlogic/qed/qed_iwarp.c:1721:5: warning: ‘ll2_syn_handle’ > may be used uninitialized in this funct > This one is completely legitimate, you can goto "err" and use > the ll2_syn_handle without it being initialized. Sorry about that, this warning didn't appear locally (gcc 4.8.5). Fix is on its way (after verifying with newer gcc).
RE: [RFC 02/19] qed: Implement iWARP initialization, teardown and qp operations
From: Leon Romanovsky [mailto:l...@kernel.org] Sent: Tuesday, June 27, 2017 8:46 AM > > On Mon, Jun 26, 2017 at 09:06:52PM +0300, Michal Kalderon wrote: > > This patch adds iWARP support for flows that have common code between > > RoCE and iWARP, such as initialization, teardown and qp setup verbs: > > create, destroy, modify, query. > > It introduces the iWARP specific files qed_iwarp.[ch] and > > iwarp_common.h > > > > Signed-off-by: Michal Kalderon > > Signed-off-by: Yuval Mintz > > Signed-off-by: Ariel Elior > > > > --- > > <...> > > > +#define QED_IWARP_PARAM_P2P(1) > > <...> > > > + > > + iwarp_info->peer2peer = QED_IWARP_PARAM_P2P; > > Can you shed a light what is it? > Thanks It's a mode in MPA rev2, It's currently hard coded, and we plan on making this configurable in the future, we're looking into using the rdmatool,
RE: [RFC 15/19] RDMA/qedr: Add iWARP support in existing verbs.
From: Leon Romanovsky [mailto:l...@kernel.org]
Sent: Tuesday, June 27, 2017 8:27 AM
> On Mon, Jun 26, 2017 at 09:07:05PM +0300, Michal Kalderon wrote:
> > Make slight modifications to common RoCE/iWARP code.
> > Add additional doorbell for iWARP post_send.
> > iWARP QP pbl is allocated in qed and not in qedr.
> >
> > Signed-off-by: Michal Kalderon
> > Signed-off-by: Ram Amrani
> > Signed-off-by: Ariel Elior
> >
> > ---
> > drivers/infiniband/hw/qedr/qedr.h | 3 +
> > drivers/infiniband/hw/qedr/verbs.c | 171
> +
> > 2 files changed, 139 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/qedr/qedr.h
> b/drivers/infiniband/hw/qedr/qedr.h
> > index c52fde0..0c0a39a 100644
> > --- a/drivers/infiniband/hw/qedr/qedr.h
> > +++ b/drivers/infiniband/hw/qedr/qedr.h
> > @@ -319,6 +319,9 @@ struct qedr_qp_hwq_info {
> > /* DB */
> > void __iomem *db;
> > union db_prod32 db_data;
> > +
> > + void __iomem *iwarp_db2;
> > + union db_prod32 iwarp_db2_data;
>
> Why do you need two doorbells?
This is a hw requirement to handle error state in iWARP
> > };
RE: [RFC 01/19] qed: Introduce iWARP personality
From: Leon Romanovsky [mailto:l...@kernel.org] Sent: Tuesday, June 27, 2017 8:37 AM > > On Mon, Jun 26, 2017 at 09:06:51PM +0300, Michal Kalderon wrote: > > iWARP personality introduced the need for differentiating in several > > places in the code whether we are RoCE, iWARP or either. This leads to > > introducing new macros for querying the personality. > > > > Signed-off-by: Michal Kalderon > > Signed-off-by: Yuval Mintz > > Signed-off-by: Ariel Elior > > > > --- > > drivers/net/ethernet/qlogic/qed/qed.h | 26 +++-- > - > > drivers/net/ethernet/qlogic/qed/qed_cxt.c | 8 > > drivers/net/ethernet/qlogic/qed/qed_dev.c | 12 +--- > > drivers/net/ethernet/qlogic/qed/qed_l2.c | 3 +-- > > drivers/net/ethernet/qlogic/qed/qed_ll2.c | 2 +- > > drivers/net/ethernet/qlogic/qed/qed_main.c | 17 - > > include/linux/qed/common_hsi.h | 2 +- > > 7 files changed, 43 insertions(+), 27 deletions(-) > > I see that these changes are in Ethernet part of your driver, but for RDMA > part, > there are already available inline functions: > rdma_protocol_iwarp, rdma_protocol_roce. > > Please avoid introducing new IS_IWARP/IS_ROCE macros and the decision > should be taken on port level and not on device, despite the fact that > probably > your ib_device has only one port. > > Thanks We still need to differentiate between iWARP/RoCE before rdma_protocol_iwarp/rdma_protocol_roce will be valid (allocating resources, registering ib device, initializing ib_device, and capabilities. For the rest of the places, after the port capabilities are valid, we'll use the inline functions.
RE: [PATCH v2 net-next 0/7] qed*: RDMA and infrastructure for iWARP
From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Sunday, June 18, 2017 4:15 PM > On Sun, Jun 18, 2017 at 02:50:28PM +0300, Yuval Mintz wrote: > > This series focuses on RDMA in general with emphasis on required > > changes toward adding iWARP support. The vast majority of the changes > > introduced are in qed/qede, with a couple of small changes to qedr > > [mentioned below]. > > Btw, can you explain us how you are going to expose RoCE vs iWarp? > Is it going to be a per-port setting? Are you going to use different > ib_device > structures or do you plan to differenciate at another level? Initial submission will be based on iWARP or RoCE being part of the device configuration ( using EFI-Hii), Later on we're considering adding devlink support for changing the device configuration. This is not a port setting, pci devices sharing the same physical port can have different RDMA protocols.
RE: [PATCH v2 net-next 4/7] qed*: Rename qede_roce.[ch]
> From: Michal Kalderon > > Once we have iWARP support, the qede portion of the qedr<->qede would > serve all the RDMA protocols - so rename the file to be appropriate to its > function. > > CC: Michal Kalderon > Signed-off-by: Yuval Mintz Sorry, patch was mistakenly created without signed-off Signed-off-by: Michal Kalderon
RE: [PATCH 1/1] net: ethernet: broadcom: fix improper return value
> 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: [RFC 07/11] Add support for memory registeration verbs
> > +struct qedr_mr *__qedr_alloc_mr(struct ib_pd *ibpd, int
> > +max_page_list_len) {
> > + struct qedr_pd *pd = get_qedr_pd(ibpd);
> > + struct qedr_dev *dev = get_qedr_dev(ibpd->device);
> > + struct qedr_mr *mr;
> > + int rc = -ENOMEM;
> > +
> > + DP_VERBOSE(dev, QEDR_MSG_MR,
> > + "qedr_alloc_frmr pd = %d max_page_list_len= %d\n", pd-
> >pd_id,
> > + max_page_list_len);
> > +
> > + mr = kzalloc(sizeof(*mr), GFP_KERNEL);
> > + if (!mr)
> > + return ERR_PTR(rc);
> > +
> > + mr->dev = dev;
> > + mr->type = QEDR_MR_FRMR;
> > +
> > + rc = init_mr_info(dev, &mr->info, max_page_list_len, 1);
> > + if (rc)
> > + goto err0;
> > +
> > + rc = dev->ops->rdma_alloc_tid(dev->rdma_ctx, &mr->hw_mr.itid);
> > + if (rc) {
> > + DP_ERR(dev, "roce alloc tid returned an error %d\n", rc);
> > + goto err0;
> > + }
> > +
> > + /* Index only, 18 bit long, lkey = itid << 8 | key */
> > + mr->hw_mr.tid_type = QED_RDMA_TID_FMR;
> > + mr->hw_mr.key = 0;
> > + mr->hw_mr.pd = pd->pd_id;
>
> Do you have a real MR<->PD association in HW? If so, can you point me to the
> code that binds it? If not, any reason not to expose the local_dma_lkey?
>
Yes, we send the pd id to the FW in function qed_rdma_register_tid. In any
case, if we didn't have the association in HW
Wouldn't the local_dma_lkey be relevant only to dma_mr ? ( code snippet above
refers to FMR)
> > +struct ib_mr *qedr_get_dma_mr(struct ib_pd *ibpd, int acc) {
> > + struct qedr_dev *dev = get_qedr_dev(ibpd->device);
> > + struct qedr_pd *pd = get_qedr_pd(ibpd);
> > + struct qedr_mr *mr;
> > + int rc;
> > +
> > + if (acc & IB_ACCESS_MW_BIND) {
> > + DP_ERR(dev, "Unsupported access flags received for dma
> mr\n");
> > + return ERR_PTR(-EINVAL);
> > + }
>
> This check looks like it really belongs in the core, it would help everyone
> if you
> move it...
>
> Although I know Christoph is trying to get rid of this API altogether...
Sure, will do.
> --
> 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: [RFC 07/11] Add support for memory registeration verbs
> >>> +struct qedr_mr *__qedr_alloc_mr(struct ib_pd *ibpd, int
> >>> +max_page_list_len) {
> >>> + struct qedr_pd *pd = get_qedr_pd(ibpd);
> >>> + struct qedr_dev *dev = get_qedr_dev(ibpd->device);
> >>> + struct qedr_mr *mr;
> >>> + int rc = -ENOMEM;
> >>> +
> >>> + DP_VERBOSE(dev, QEDR_MSG_MR,
> >>> +"qedr_alloc_frmr pd = %d max_page_list_len= %d\n", pd-
> >>> pd_id,
> >>> +max_page_list_len);
> >>> +
> >>> + mr = kzalloc(sizeof(*mr), GFP_KERNEL);
> >>> + if (!mr)
> >>> + return ERR_PTR(rc);
> >>> +
> >>> + mr->dev = dev;
> >>> + mr->type = QEDR_MR_FRMR;
> >>> +
> >>> + rc = init_mr_info(dev, &mr->info, max_page_list_len, 1);
> >>> + if (rc)
> >>> + goto err0;
> >>> +
> >>> + rc = dev->ops->rdma_alloc_tid(dev->rdma_ctx, &mr->hw_mr.itid);
> >>> + if (rc) {
> >>> + DP_ERR(dev, "roce alloc tid returned an error %d\n", rc);
> >>> + goto err0;
> >>> + }
> >>> +
> >>> + /* Index only, 18 bit long, lkey = itid << 8 | key */
> >>> + mr->hw_mr.tid_type = QED_RDMA_TID_FMR;
> >>> + mr->hw_mr.key = 0;
> >>> + mr->hw_mr.pd = pd->pd_id;
> >>
> >> Do you have a real MR<->PD association in HW? If so, can you point me
> >> to the code that binds it? If not, any reason not to expose the
> local_dma_lkey?
> >>
> > Yes, we send the pd id to the FW in function qed_rdma_register_tid.
>
> Right, thanks.
>
> > In any case, if we didn't have the association in HW Wouldn't the
> > local_dma_lkey be relevant only to dma_mr ? ( code snippet above
> > refers to FMR)
>
> I was just sticking to a location in the code where you associate MR<->PD...
>
> The local_dma_lkey is a special key that spans the entire memory space and
> unlike the notorious dma_mr, its not associated with a PD.
>
> See the code in ib_alloc_pd(), if the device does not support a single device
> local dma lkey, the core allocates a dma mr associated with the pd. If your
> device has such a key, you can save a dma mr allocation for each pd in the
> system.
Managed to miss this. Our device supports such a key, we'll add support.
