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