On Wed, Feb 27, 2019 at 01:39:25PM +0200, Gal Pressman wrote:
> On 27-Feb-19 11:37, Leon Romanovsky wrote:
> > 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()?
>
> I'm using 'size_in_pages' both in pbl_indirect_initialize and in
> efa_vmalloc_buf_to_sg so it's calculated once in the outer function. Do you
> suggest to make the calculation twice?
I see it now, Thanks
>
> >
> >
> >>
> >>>
> >>>>>> + }
> >>>>>> + return sglist;
> >>>>>> +
> >>>>>> +err:
> >>>>>> + kfree(sglist);
> >>>>>> + return NULL;
> >>>>>> +}
> >>>>>> +
> >>>>>
> >>>>>
> >>>>> Reviewed-by: Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx>
> >>>>
> >>>> Thanks Steve!
Attachment:
signature.asc
Description: PGP signature
