> -----Original Message-----
> From: Gal Pressman <galpress@xxxxxxxxxx>
> Sent: Wednesday, February 27, 2019 2:40 AM
> To: Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx>; Jason Gunthorpe
> <jgg@xxxxxxxx>; Doug Ledford <dledford@xxxxxxxxxx>
> Cc: Yossi Leybovich <sleybo@xxxxxxxxxx>; Alexander Matushevsky
> <matua@xxxxxxxxxx>; Leah Shalev <shalevl@xxxxxxxxxx>; Dave Goodell
> <goodell@xxxxxxxxxx>; Brian Barrett <bbarrett@xxxxxxxxxx>; linux-
> rdma@xxxxxxxxxxxxxxx; Sean Hefty <sean.hefty@xxxxxxxxx>; Dennis
> Dalessandro <dennis.dalessandro@xxxxxxxxx>; Leon Romanovsky
> <leon@xxxxxxxxxx>; Christoph Hellwig <hch@xxxxxxxxxxxxx>; Parav Pandit
> <parav@xxxxxxxxxxxx>; Sagi Grimberg <sagi@xxxxxxxxxxx>
> Subject: Re: [PATCH rdma-next v2 09/11] RDMA/efa: Add EFA verbs
> implementation
>
> On 26-Feb-19 23:43, Steve Wise wrote:
> >> +int efa_query_port(struct ib_device *ibdev, u8 port,
> >> + struct ib_port_attr *props)
> >> +{
> >> + struct efa_dev *dev = to_edev(ibdev);
> >> +
> >> + memset(props, 0, sizeof(*props));
> >> +
> >> + props->lid = 0;
> >> + props->lmc = 1;
> >> + props->sm_lid = 0;
> >> + props->sm_sl = 0;
> >> +
> >> + props->state = IB_PORT_ACTIVE;
> >> + props->phys_state = 5;
> >> + props->port_cap_flags = 0;
> >> + props->gid_tbl_len = 1;
> >> + props->pkey_tbl_len = 1;
> >> + props->bad_pkey_cntr = 0;
> >> + props->qkey_viol_cntr = 0;
> >> + props->active_speed = IB_SPEED_EDR;
> >> + props->active_width = IB_WIDTH_4X;
> >> + props->max_mtu = ib_mtu_int_to_enum(dev->mtu);
> >> + props->active_mtu = ib_mtu_int_to_enum(dev->mtu);
> >> + props->max_msg_sz = dev->mtu;
> >> + props->max_vl_num = 1;
> >> +
> >
> >
> > Since you memset() props to all zeros, should you bother with
> > initializing the zero fields?
>
> Will remove.
>
> >
> >
> >> + return 0;
> >> +}
> >> +
> >> +static int efa_qp_validate_cap(struct efa_dev *dev,
> >> + struct ib_qp_init_attr *init_attr)
> >> +{
> >> + if (init_attr->cap.max_send_wr > dev->dev_attr.max_sq_depth) {
> >> + efa_err(&dev->ibdev.dev,
> >> + "qp: requested send wr[%u] exceeds the
> max[%u]\n",
> >> + init_attr->cap.max_send_wr,
> >> + dev->dev_attr.max_sq_depth);
> >> + return -EINVAL;
> >> + }
> >> + if (init_attr->cap.max_recv_wr > dev->dev_attr.max_rq_depth) {
> >> + efa_err(&dev->ibdev.dev,
> >> + "qp: requested receive wr[%u] exceeds the
> max[%u]\n",
> >> + init_attr->cap.max_recv_wr,
> >> + dev->dev_attr.max_rq_depth);
> >> + return -EINVAL;
> >> + }
> >> + if (init_attr->cap.max_send_sge > dev->dev_attr.max_sq_sge) {
> >> + efa_err(&dev->ibdev.dev,
> >> + "qp: requested sge send[%u] exceeds the
> max[%u]\n",
> >> + init_attr->cap.max_send_sge, dev-
> >dev_attr.max_sq_sge);
> >> + return -EINVAL;
> >> + }
> >> + if (init_attr->cap.max_recv_sge > dev->dev_attr.max_rq_sge) {
> >> + efa_err(&dev->ibdev.dev,
> >> + "qp: requested sge recv[%u] exceeds the
> max[%u]\n",
> >> + init_attr->cap.max_recv_sge, dev-
> >dev_attr.max_rq_sge);
> >> + return -EINVAL;
> >> + }
> >> + if (init_attr->cap.max_inline_data > dev->dev_attr.inline_buf_size) {
> >> + efa_err(&dev->ibdev.dev,
> >> + "requested inline data[%u] exceeds the max[%u]\n",
> >> + init_attr->cap.max_inline_data,
> >> + dev->dev_attr.inline_buf_size);
> >> + return -EINVAL;
> >> + }
> >> +
> >
> >
> > Should all these efa_err() calls really be efa_dbg()s? That's a lot of
> > log polluting for user errors.
>
> Most users don't really enable debug and we want them to have an
> indication of
> what happened. is efa_warn() better?
I don't know what the preference is for *warn vs *err vs *info. But I think in general, these sorts of errors aren't logged except for debug purposes.
>
> >
> >
> >> + return 0;
> >> +}
> >> +
> >> +static struct scatterlist *efa_vmalloc_buf_to_sg(u64 *buf, int page_cnt)
> >> +{
> >> + struct scatterlist *sglist;
> >> + struct page *pg;
> >> + int i;
> >> +
> >> + sglist = kcalloc(page_cnt, sizeof(*sglist), GFP_KERNEL);
> >> + if (!sglist)
> >> + return NULL;
> >> + sg_init_table(sglist, page_cnt);
> >> + for (i = 0; i < page_cnt; i++) {
> >> + pg = vmalloc_to_page(buf);
> >> + if (!pg)
> >> + goto err;
> >> + WARN_ON_ONCE(PageHighMem(pg));
> >
> > Is this WARN_ON_ONCE() really an error that needs to be handled?
>
> AFAIK, there is no way we can actually get a higemem page here.
> The WARN is here from early dev days, it should probably be removed.
>
> >
> >
> >> + sg_set_page(&sglist[i], pg, EFA_PAGE_SIZE, 0);
> >> + buf = (u64 *)((u8 *)buf + EFA_PAGE_SIZE);
> >> + }
> >> + return sglist;
> >> +
> >> +err:
> >> + kfree(sglist);
> >> + return NULL;
> >> +}
> >> +
> >
> >
> > Reviewed-by: Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx>
>
> Thanks Steve!
