On 27-Feb-19 16:05, Steve Wise wrote:
>
>
>> -----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.
Will change to debug.
>
>>
>>>
>>>
>>>> + 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!
>
