| Message ID | 1559222731-16715-16-git-send-email-maxg@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce new API for T10-PI offload | expand |
On Thu, May 30, 2019 at 04:25:26PM +0300, Max Gurtovoy wrote: > Protect the case that a ULP tries to allocate a QP with signature > enabled flag while the LLD doesn't support this feature. > > Signed-off-by: Max Gurtovoy <maxg@mellanox.com> Oh, ok. I think this should be folded into the previous patch. > + if ((qp_init_attr->rwq_ind_tbl && > + (qp_init_attr->recv_cq || > + qp_init_attr->srq || qp_init_attr->cap.max_recv_wr || > + qp_init_attr->cap.max_recv_sge)) || > + ((qp_init_attr->create_flags & IB_QP_CREATE_SIGNATURE_EN) && > + !(device->attrs.device_cap_flags & IB_DEVICE_SIGNATURE_HANDOVER))) > return ERR_PTR(-EINVAL); This looks almost unreadable. Just make the signature check a separate conditional.
On 6/4/2019 10:48 AM, Christoph Hellwig wrote: > On Thu, May 30, 2019 at 04:25:26PM +0300, Max Gurtovoy wrote: >> Protect the case that a ULP tries to allocate a QP with signature >> enabled flag while the LLD doesn't support this feature. >> >> Signed-off-by: Max Gurtovoy <maxg@mellanox.com> > Oh, ok. I think this should be folded into the previous patch. I'm fine with this squash. > >> + if ((qp_init_attr->rwq_ind_tbl && >> + (qp_init_attr->recv_cq || >> + qp_init_attr->srq || qp_init_attr->cap.max_recv_wr || >> + qp_init_attr->cap.max_recv_sge)) || >> + ((qp_init_attr->create_flags & IB_QP_CREATE_SIGNATURE_EN) && >> + !(device->attrs.device_cap_flags & IB_DEVICE_SIGNATURE_HANDOVER))) >> return ERR_PTR(-EINVAL); > This looks almost unreadable. Just make the signature check a separate > conditional. Yup we'll fix that.
> - if (qp_init_attr->rwq_ind_tbl && > - (qp_init_attr->recv_cq || > - qp_init_attr->srq || qp_init_attr->cap.max_recv_wr || > - qp_init_attr->cap.max_recv_sge)) > + if ((qp_init_attr->rwq_ind_tbl && > + (qp_init_attr->recv_cq || > + qp_init_attr->srq || qp_init_attr->cap.max_recv_wr || > + qp_init_attr->cap.max_recv_sge)) || > + ((qp_init_attr->create_flags & IB_QP_CREATE_SIGNATURE_EN) && > + !(device->attrs.device_cap_flags & IB_DEVICE_SIGNATURE_HANDOVER))) Wouldn't it make sense to also change the qp create flag and the device cap to be PI_EN/PI_HANDOVER while we're at it?
>> - if (qp_init_attr->rwq_ind_tbl && >> - (qp_init_attr->recv_cq || >> - qp_init_attr->srq || qp_init_attr->cap.max_recv_wr || >> - qp_init_attr->cap.max_recv_sge)) >> + if ((qp_init_attr->rwq_ind_tbl && >> + (qp_init_attr->recv_cq || >> + qp_init_attr->srq || qp_init_attr->cap.max_recv_wr || >> + qp_init_attr->cap.max_recv_sge)) || >> + ((qp_init_attr->create_flags & IB_QP_CREATE_SIGNATURE_EN) && >> + !(device->attrs.device_cap_flags & >> IB_DEVICE_SIGNATURE_HANDOVER))) > > Wouldn't it make sense to also change the qp create flag and the device > cap to be PI_EN/PI_HANDOVER while we're at it? Or INTEGRITY_EN/INTEGRITY_HANDOVER?
On 6/5/2019 10:40 PM, Sagi Grimberg wrote: > >>> - if (qp_init_attr->rwq_ind_tbl && >>> - (qp_init_attr->recv_cq || >>> - qp_init_attr->srq || qp_init_attr->cap.max_recv_wr || >>> - qp_init_attr->cap.max_recv_sge)) >>> + if ((qp_init_attr->rwq_ind_tbl && >>> + (qp_init_attr->recv_cq || >>> + qp_init_attr->srq || qp_init_attr->cap.max_recv_wr || >>> + qp_init_attr->cap.max_recv_sge)) || >>> + ((qp_init_attr->create_flags & IB_QP_CREATE_SIGNATURE_EN) && >>> + !(device->attrs.device_cap_flags & >>> IB_DEVICE_SIGNATURE_HANDOVER))) >> >> Wouldn't it make sense to also change the qp create flag and the device >> cap to be PI_EN/PI_HANDOVER while we're at it? We're already standing on 20 patches in this series, so if Jason will agree I'll do this renaming in a separate commit or we can stay with the current naming. > > Or INTEGRITY_EN/INTEGRITY_HANDOVER?
>>>> - if (qp_init_attr->rwq_ind_tbl && >>>> - (qp_init_attr->recv_cq || >>>> - qp_init_attr->srq || qp_init_attr->cap.max_recv_wr || >>>> - qp_init_attr->cap.max_recv_sge)) >>>> + if ((qp_init_attr->rwq_ind_tbl && >>>> + (qp_init_attr->recv_cq || >>>> + qp_init_attr->srq || qp_init_attr->cap.max_recv_wr || >>>> + qp_init_attr->cap.max_recv_sge)) || >>>> + ((qp_init_attr->create_flags & IB_QP_CREATE_SIGNATURE_EN) && >>>> + !(device->attrs.device_cap_flags & >>>> IB_DEVICE_SIGNATURE_HANDOVER))) >>> >>> Wouldn't it make sense to also change the qp create flag and the device >>> cap to be PI_EN/PI_HANDOVER while we're at it? > > We're already standing on 20 patches in this series, so if Jason will > agree I'll do this renaming in a separate commit or we can stay with the > current naming. This is a good chance to clean the naming here. Ideally we can also cleanup the naming on the call-sites and get rid of the signature name everywhere (since we ended up doing only the pi subset), but that indeed can be a followup. This can come as a separate patch.
On Thu, Jun 06, 2019 at 01:05:54AM +0300, Max Gurtovoy wrote: > > On 6/5/2019 10:40 PM, Sagi Grimberg wrote: > > > > > > - if (qp_init_attr->rwq_ind_tbl && > > > > - (qp_init_attr->recv_cq || > > > > - qp_init_attr->srq || qp_init_attr->cap.max_recv_wr || > > > > - qp_init_attr->cap.max_recv_sge)) > > > > + if ((qp_init_attr->rwq_ind_tbl && > > > > + (qp_init_attr->recv_cq || > > > > + qp_init_attr->srq || qp_init_attr->cap.max_recv_wr || > > > > + qp_init_attr->cap.max_recv_sge)) || > > > > + ((qp_init_attr->create_flags & IB_QP_CREATE_SIGNATURE_EN) && > > > > + !(device->attrs.device_cap_flags & > > > > IB_DEVICE_SIGNATURE_HANDOVER))) > > > > > > Wouldn't it make sense to also change the qp create flag and the device > > > cap to be PI_EN/PI_HANDOVER while we're at it? > > We're already standing on 20 patches in this series, so if Jason will agree > I'll do this renaming in a separate commit or we can stay with the current > naming. Given the CH and Sagi have done the hard work to review the large series, I am fine to take 2x patches if they are fine to review it Jason
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c index 936498c3f9cb..4e933ea70159 100644 --- a/drivers/infiniband/core/verbs.c +++ b/drivers/infiniband/core/verbs.c @@ -1152,10 +1152,12 @@ struct ib_qp *ib_create_qp_user(struct ib_pd *pd, struct ib_qp *qp; int ret; - if (qp_init_attr->rwq_ind_tbl && - (qp_init_attr->recv_cq || - qp_init_attr->srq || qp_init_attr->cap.max_recv_wr || - qp_init_attr->cap.max_recv_sge)) + if ((qp_init_attr->rwq_ind_tbl && + (qp_init_attr->recv_cq || + qp_init_attr->srq || qp_init_attr->cap.max_recv_wr || + qp_init_attr->cap.max_recv_sge)) || + ((qp_init_attr->create_flags & IB_QP_CREATE_SIGNATURE_EN) && + !(device->attrs.device_cap_flags & IB_DEVICE_SIGNATURE_HANDOVER))) return ERR_PTR(-EINVAL); /*
Protect the case that a ULP tries to allocate a QP with signature enabled flag while the LLD doesn't support this feature. Signed-off-by: Max Gurtovoy <maxg@mellanox.com> --- drivers/infiniband/core/verbs.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)