Messages in this thread | | | Subject | Re: [PATCH v8] i2c: virtio: add a virtio i2c frontend driver | From | Jie Deng <> | Date | Fri, 19 Mar 2021 15:52:05 +0800 |
| |
On 2021/3/19 13:53, Viresh Kumar wrote: > On 16-03-21, 18:35, Jie Deng wrote: >> +++ b/drivers/i2c/busses/i2c-virtio.c >> +static int virtio_i2c_send_reqs(struct virtqueue *vq, >> + struct virtio_i2c_req *reqs, >> + struct i2c_msg *msgs, int nr) >> +{ >> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr; >> + int i, outcnt, incnt, err = 0; >> + >> + for (i = 0; i < nr; i++) { >> + if (!msgs[i].len) >> + break; >> + >> + /* >> + * Only 7-bit mode supported for this moment. For the address format, >> + * Please check the Virtio I2C Specification. >> + */ >> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1); >> + >> + if (i != nr - 1) >> + reqs[i].out_hdr.flags = cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT); >> + >> + outcnt = incnt = 0; >> + sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr)); >> + sgs[outcnt++] = &out_hdr; >> + >> + reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1); > You allocate a buffer here, lets see if they are freeing properly or not (I > remember that I gave same feedback earlier as well, but anyway).
"MAY" allocate a buffer here.
> >> + if (!reqs[i].buf) >> + break; >> + >> + sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len); >> + >> + if (msgs[i].flags & I2C_M_RD) >> + sgs[outcnt + incnt++] = &msg_buf; >> + else >> + sgs[outcnt++] = &msg_buf; >> + >> + sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr)); >> + sgs[outcnt + incnt++] = &in_hdr; >> + >> + err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL); >> + if (err < 0) { >> + pr_err("failed to add msg[%d] to virtqueue.\n", i); >> + i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false); > On failure here, you freed the buffers for request "i" but not others..
Others still need to be sent and then be freed.
> >> + break; >> + } >> + } >> + >> + return i; >> +} >> + >> +static int virtio_i2c_complete_reqs(struct virtqueue *vq, >> + struct virtio_i2c_req *reqs, >> + struct i2c_msg *msgs, int nr) >> +{ >> + struct virtio_i2c_req *req; >> + unsigned int len; >> + int i, j; >> + >> + for (i = 0; i < nr; i++) { >> + req = virtqueue_get_buf(vq, &len); >> + if (!(req && req == &reqs[i])) { >> + pr_err("msg[%d]: addr=0x%x is out of order.\n", i, msgs[i].addr); >> + break; > Since you break here, what will happen to the buffer ? I thought > virtqueue_get_buf() will return a req only once and then you can't access it ?
Will refine it along with the latter loop.
> >> + } >> + >> + if (req->in_hdr.status != VIRTIO_I2C_MSG_OK) { >> + pr_err("msg[%d]: addr=0x%x backend error.\n", i, msgs[i].addr); >> + break; >> + } >> + >> + i2c_put_dma_safe_msg_buf(req->buf, &msgs[i], true); >> + } >> + >> + /* >> + * Detach all the used buffers from the vq and >> + * Release unused DMA safe buffer if any. >> + */ >> + for (j = i; j < nr; j++) { >> + req = virtqueue_get_buf(vq, &len); >> + if (req) >> + i2c_put_dma_safe_msg_buf(req->buf, &msgs[j], false); > This will come in play only if something failed in the earlier loop ? Or my > understanding incorrect ? Also this should be merged with the above for loop > itself, it is just doing part of it.
Will refine it along with the earlier loop.
> >> + } >> + >> + return i; >> +} >> + >> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) >> +{ >> + struct virtio_i2c *vi = i2c_get_adapdata(adap); >> + struct virtqueue *vq = vi->vq; >> + struct virtio_i2c_req *reqs; >> + unsigned long time_left; >> + int ret, nr; >> + >> + reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL); >> + if (!reqs) >> + return -ENOMEM; >> + >> + mutex_lock(&vi->lock); >> + >> + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num); >> + if (ret == 0) >> + goto err_unlock_free; >> + >> + nr = ret; >> + reinit_completion(&vi->completion); >> + virtqueue_kick(vq); >> + >> + time_left = wait_for_completion_timeout(&vi->completion, adap->timeout); >> + if (!time_left) { > On error here, we will surely not free the buffers, isn't it ?
Right. Will fix it. Thank you.
>> + dev_err(&adap->dev, "virtio i2c backend timeout.\n"); >> + ret = -ETIMEDOUT; >> + goto err_unlock_free; >> + } >> + >> + ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr); >> + >> +err_unlock_free: >> + mutex_unlock(&vi->lock); >> + kfree(reqs); >> + return ret; >> +}
| |