On Wed, Feb 27, 2019 at 11:06:14AM +0200, Gal Pressman wrote:
> On 27-Feb-19 10:45, Leon Romanovsky wrote:
> > On Wed, Feb 27, 2019 at 10:39:30AM +0200, Gal Pressman wrote:
> >> 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?
> >
> > aren't you doing anything that your users would like to avoid - polluting dmesg?
>
> We haven't seen a case where it polluted dmesg.
> It's one error print of invalid parameter (which is very unlikely as our
> provider checks for this as well) and most applications will exit at this point
> if create QP failed so there shouldn't be any more prints.
>
> >
> >>
> >>>
> >>>
> >>>> + 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);
> >
> > Why do you need special EFA_PAGE_SIZE? Isn't PAGE_SIZE enough for you?
>
> EFA_PAGE_SIZE represents the device page size.
So why don't you do:
u32 size_in_pages = DIV_ROUND_UP(pbl->pbl_buf_size_in_bytes, EFA_PAGE_SIZE);
instead of doing it outside of efa_vmalloc_buf_to_sg()?
>
> >
> >>>> + }
> >>>> + return sglist;
> >>>> +
> >>>> +err:
> >>>> + kfree(sglist);
> >>>> + return NULL;
> >>>> +}
> >>>> +
> >>>
> >>>
> >>> Reviewed-by: Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx>
> >>
> >> Thanks Steve!
Attachment:
signature.asc
Description: PGP signature
