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?
>
>
>>
>>>
>>>>>> + }
>>>>>> + return sglist;
>>>>>> +
>>>>>> +err:
>>>>>> + kfree(sglist);
>>>>>> + return NULL;
>>>>>> +}
>>>>>> +
>>>>>
>>>>>
>>>>> Reviewed-by: Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx>
>>>>
>>>> Thanks Steve!
