On 04-Jan-19 05:51, Jason Gunthorpe wrote:
>> +
>> +static int qp_mmap_entries_setup(struct efa_qp *qp,
>> + struct efa_dev *dev,
>> + struct efa_ucontext *ucontext,
>> + struct efa_com_create_qp_params *params,
>> + struct efa_ibv_create_qp_resp *resp)
>> +{
>> + struct efa_mmap_entry *rq_db_entry;
>> + struct efa_mmap_entry *sq_db_entry;
>> + struct efa_mmap_entry *rq_entry;
>> + struct efa_mmap_entry *sq_entry;
>> +
>> + sq_db_entry = kzalloc(sizeof(*sq_db_entry), GFP_KERNEL);
>> + sq_entry = kzalloc(sizeof(*sq_entry), GFP_KERNEL);
>> + if (!sq_db_entry || !sq_entry) {
>> + dev->stats.sw_stats.mmap_entry_alloc_err++;
>> + goto err_alloc;
>> + }
>> +
>> + if (qp->rq_size) {
>> + rq_entry = kzalloc(sizeof(*rq_entry), GFP_KERNEL);
>> + rq_db_entry = kzalloc(sizeof(*rq_db_entry), GFP_KERNEL);
>> + if (!rq_entry || !rq_db_entry) {
>> + dev->stats.sw_stats.mmap_entry_alloc_err++;
>> + goto err_alloc_rq;
>> + }
>> +
>> + rq_db_entry->obj = qp;
>> + rq_entry->obj = qp;
>> +
>> + rq_entry->address = virt_to_phys(qp->rq_cpu_addr);
>
> virt_to_phys cannot be called on addresses returned by dma_alloc_coherent:
ACK.
>
>> + qp->rq_cpu_addr = dma_zalloc_coherent(&dev->pdev->dev,
>> + qp->rq_size,
>> + &qp->rq_dma_addr,
>> + GFP_KERNEL);
>
> And this whole mmap_entries data structure looks like a big confusing
> mess to me, I think it should just be trivial usage of xarray if I
> understand what it is trying to do. (and doing remove during mmap
> seems really wrong to me, mmap cookies should exist as long as the
> owning object exists..)
The motivation here is that the user calls create_qp/cq, the driver allocates
the DMA buffers and returns an mmap key.
The user should use the same key to mmap the buffers that the driver allocated
for the qp/cq.
The mmap_entries are a way to identify mmap_key to DMA buffers, objects are
added to the list on creation and removed on mmap call (when the queues are
mapped and the key -> object translation is no longer needed).
>
> Also you can't mmap to user space dma_coherent memory, so this entire
> thing needs reworking to use non-coherent memory and proper barriers
> like all the other drivers.
ACK.
>
> Also everything in __efa_mmap is old-style, it needs to use the new
> rdma_user_mmap_io/page() interfaces.
Thanks, wasn't aware of these interfaces.
>
> .. and I just wanted to know if CQ was done sensibly :(
>
>
> Oh, and:
>
>> +int efa_post_send(struct ib_qp *ibqp,
>> + const struct ib_send_wr *wr,
>> + const struct ib_send_wr **bad_wr)
>> +{
>> + pr_warn("Function not supported\n");
>> + return -EOPNOTSUPP;
>> +}
>
> Drivers that don't support something are to just set the function
> pointer to NULL, not do stubs like this.
Will be removed following my RFC for non kverbs providers.
