| Message ID | 20240705085937.1644229-2-huangjunxian6@hisilicon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA/hns: Bugfixes | expand |
On Fri, Jul 05, 2024 at 04:59:29PM +0800, Junxian Huang wrote: > 8 bytes is the only supported length of atomic. Return an error if > it is not. > > Fixes: 384f88185112 ("RDMA/hns: Add atomic support") > Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com> > --- > drivers/infiniband/hw/hns/hns_roce_device.h | 2 ++ > drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 19 +++++++++++++++---- > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h > index ff0b3f68ee3a..05005079258c 100644 > --- a/drivers/infiniband/hw/hns/hns_roce_device.h > +++ b/drivers/infiniband/hw/hns/hns_roce_device.h > @@ -91,6 +91,8 @@ > /* Configure to HW for PAGE_SIZE larger than 4KB */ > #define PG_SHIFT_OFFSET (PAGE_SHIFT - 12) > > +#define ATOMIC_WR_LEN 8 > + > #define HNS_ROCE_IDX_QUE_ENTRY_SZ 4 > #define SRQ_DB_REG 0x230 > > diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c > index 4287818a737f..a5d746a5cc68 100644 > --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c > +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c > @@ -164,15 +164,23 @@ static void set_frmr_seg(struct hns_roce_v2_rc_send_wqe *rc_sq_wqe, > hr_reg_clear(fseg, FRMR_BLK_MODE); > } > > -static void set_atomic_seg(const struct ib_send_wr *wr, > - struct hns_roce_v2_rc_send_wqe *rc_sq_wqe, > - unsigned int valid_num_sge) > +static int set_atomic_seg(struct hns_roce_dev *hr_dev, > + const struct ib_send_wr *wr, > + struct hns_roce_v2_rc_send_wqe *rc_sq_wqe, > + unsigned int valid_num_sge, u32 msg_len) > { > struct hns_roce_v2_wqe_data_seg *dseg = > (void *)rc_sq_wqe + sizeof(struct hns_roce_v2_rc_send_wqe); > struct hns_roce_wqe_atomic_seg *aseg = > (void *)dseg + sizeof(struct hns_roce_v2_wqe_data_seg); > > + if (msg_len != ATOMIC_WR_LEN) { > + ibdev_err_ratelimited(&hr_dev->ib_dev, > + "invalid atomic wr len, len = %u.\n", > + msg_len); > + return -EINVAL; 1. Please don't add prints in data-path. 2. You most likely need to add this check before calling to set_atomic_seg(). 3. You shouldn't continue to process the WQE if the length is invalid. Need to return from set_rc_wqe() and not continue. 4. I wonder if it is right place to put this limitation and can't be enforced much earlier. Thanks
On 2024/7/7 16:24, Leon Romanovsky wrote: > On Fri, Jul 05, 2024 at 04:59:29PM +0800, Junxian Huang wrote: >> 8 bytes is the only supported length of atomic. Return an error if >> it is not. >> >> Fixes: 384f88185112 ("RDMA/hns: Add atomic support") >> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com> >> --- >> drivers/infiniband/hw/hns/hns_roce_device.h | 2 ++ >> drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 19 +++++++++++++++---- >> 2 files changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h >> index ff0b3f68ee3a..05005079258c 100644 >> --- a/drivers/infiniband/hw/hns/hns_roce_device.h >> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h >> @@ -91,6 +91,8 @@ >> /* Configure to HW for PAGE_SIZE larger than 4KB */ >> #define PG_SHIFT_OFFSET (PAGE_SHIFT - 12) >> >> +#define ATOMIC_WR_LEN 8 >> + >> #define HNS_ROCE_IDX_QUE_ENTRY_SZ 4 >> #define SRQ_DB_REG 0x230 >> >> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c >> index 4287818a737f..a5d746a5cc68 100644 >> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c >> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c >> @@ -164,15 +164,23 @@ static void set_frmr_seg(struct hns_roce_v2_rc_send_wqe *rc_sq_wqe, >> hr_reg_clear(fseg, FRMR_BLK_MODE); >> } >> >> -static void set_atomic_seg(const struct ib_send_wr *wr, >> - struct hns_roce_v2_rc_send_wqe *rc_sq_wqe, >> - unsigned int valid_num_sge) >> +static int set_atomic_seg(struct hns_roce_dev *hr_dev, >> + const struct ib_send_wr *wr, >> + struct hns_roce_v2_rc_send_wqe *rc_sq_wqe, >> + unsigned int valid_num_sge, u32 msg_len) >> { >> struct hns_roce_v2_wqe_data_seg *dseg = >> (void *)rc_sq_wqe + sizeof(struct hns_roce_v2_rc_send_wqe); >> struct hns_roce_wqe_atomic_seg *aseg = >> (void *)dseg + sizeof(struct hns_roce_v2_wqe_data_seg); >> >> + if (msg_len != ATOMIC_WR_LEN) { >> + ibdev_err_ratelimited(&hr_dev->ib_dev, >> + "invalid atomic wr len, len = %u.\n", >> + msg_len); >> + return -EINVAL; > > 1. Please don't add prints in data-path. > 2. You most likely need to add this check before calling to set_atomic_seg(). > 3. You shouldn't continue to process the WQE if the length is invalid. > Need to return from set_rc_wqe() and not continue. > 4. I wonder if it is right place to put this limitation and can't be > enforced much earlier. > > Thanks > Thanks. 1 & 3 will be fixed. And for 2 & 4, I don't see any place more appropriate, so I'll just add this check in set_rc_wqe(). Junxian
diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h index ff0b3f68ee3a..05005079258c 100644 --- a/drivers/infiniband/hw/hns/hns_roce_device.h +++ b/drivers/infiniband/hw/hns/hns_roce_device.h @@ -91,6 +91,8 @@ /* Configure to HW for PAGE_SIZE larger than 4KB */ #define PG_SHIFT_OFFSET (PAGE_SHIFT - 12) +#define ATOMIC_WR_LEN 8 + #define HNS_ROCE_IDX_QUE_ENTRY_SZ 4 #define SRQ_DB_REG 0x230 diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c index 4287818a737f..a5d746a5cc68 100644 --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c @@ -164,15 +164,23 @@ static void set_frmr_seg(struct hns_roce_v2_rc_send_wqe *rc_sq_wqe, hr_reg_clear(fseg, FRMR_BLK_MODE); } -static void set_atomic_seg(const struct ib_send_wr *wr, - struct hns_roce_v2_rc_send_wqe *rc_sq_wqe, - unsigned int valid_num_sge) +static int set_atomic_seg(struct hns_roce_dev *hr_dev, + const struct ib_send_wr *wr, + struct hns_roce_v2_rc_send_wqe *rc_sq_wqe, + unsigned int valid_num_sge, u32 msg_len) { struct hns_roce_v2_wqe_data_seg *dseg = (void *)rc_sq_wqe + sizeof(struct hns_roce_v2_rc_send_wqe); struct hns_roce_wqe_atomic_seg *aseg = (void *)dseg + sizeof(struct hns_roce_v2_wqe_data_seg); + if (msg_len != ATOMIC_WR_LEN) { + ibdev_err_ratelimited(&hr_dev->ib_dev, + "invalid atomic wr len, len = %u.\n", + msg_len); + return -EINVAL; + } + set_data_seg_v2(dseg, wr->sg_list); if (wr->opcode == IB_WR_ATOMIC_CMP_AND_SWP) { @@ -185,6 +193,8 @@ static void set_atomic_seg(const struct ib_send_wr *wr, } hr_reg_write(rc_sq_wqe, RC_SEND_WQE_SGE_NUM, valid_num_sge); + + return 0; } static int fill_ext_sge_inl_data(struct hns_roce_qp *qp, @@ -592,7 +602,8 @@ static inline int set_rc_wqe(struct hns_roce_qp *qp, if (wr->opcode == IB_WR_ATOMIC_CMP_AND_SWP || wr->opcode == IB_WR_ATOMIC_FETCH_AND_ADD) - set_atomic_seg(wr, rc_sq_wqe, valid_num_sge); + ret = set_atomic_seg(hr_dev, wr, rc_sq_wqe, valid_num_sge, + msg_len); else if (wr->opcode != IB_WR_REG_MR) ret = set_rwqe_data_seg(&qp->ibqp, wr, rc_sq_wqe, &curr_idx, valid_num_sge);
8 bytes is the only supported length of atomic. Return an error if it is not. Fixes: 384f88185112 ("RDMA/hns: Add atomic support") Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com> --- drivers/infiniband/hw/hns/hns_roce_device.h | 2 ++ drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 19 +++++++++++++++---- 2 files changed, 17 insertions(+), 4 deletions(-)