| Message ID | c193b92d8d22ba439bb1b260d26d4b76f57d4840.1615889867.git.jie.deng@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v8] i2c: virtio: add a virtio i2c frontend driver | expand |
On 16-03-21, 18:35, Jie Deng wrote: > diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c > +struct virtio_i2c { > + struct virtio_device *vdev; > + struct completion completion; > + struct i2c_adapter *adap; > + struct mutex lock; > + struct virtqueue *vq; > +}; > +static struct i2c_adapter virtio_adapter = { > + .owner = THIS_MODULE, > + .name = "Virtio I2C Adapter", > + .class = I2C_CLASS_DEPRECATED, > + .algo = &virtio_algorithm, > +}; And FWIW, I still have my concerns about mutex-lock and I2C_CLASS_DEPRECATED, but yeah we need Wolfram to confirm on them. LGTM otherwise.
On 16.03.21 08:44, Viresh Kumar wrote: > FWIW, this limits this driver to support a single device ever. We > can't bind multiple devices to this driver now. Yeah, perhaps we will > never be required to do so, but who knows. Actually, I believe multiple devices really should be possible. The major benefit of virtio-i2c is either bridging certan real bus'es into a confined workload, or creating virtual hw testbeds w/o having to write a complete emulation (in this case, for dozens of different i2c controllers) - and having multiple i2c interfaces in one machine isn't exactly rare. --mtx -- --- Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren GPG/PGP-Schlüssel zu. --- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering info@metux.net -- +49-151-27565287
On Thu, Mar 18, 2021 at 3:42 PM Enrico Weigelt, metux IT consult <lkml@metux.net> wrote: > > On 16.03.21 08:44, Viresh Kumar wrote: > > > FWIW, this limits this driver to support a single device ever. We > > can't bind multiple devices to this driver now. Yeah, perhaps we will > > never be required to do so, but who knows. > > Actually, I believe multiple devices really should be possible. > > The major benefit of virtio-i2c is either bridging certan real bus'es > into a confined workload, or creating virtual hw testbeds w/o having to > write a complete emulation (in this case, for dozens of different i2c > controllers) - and having multiple i2c interfaces in one machine isn't > exactly rare. Allowing multiple virtio-i2c controllers in one system, and multiple i2c devices attached to each controller is clearly something that has to work. I don't actually see a limitation though. Viresh, what is the problem you see for having multiple controllers? Arnd
On 18-03-21, 15:52, Arnd Bergmann wrote: > Allowing multiple virtio-i2c controllers in one system, and multiple i2c > devices attached to each controller is clearly something that has to work. Good. > I don't actually see a limitation though. Viresh, what is the problem > you see for having multiple controllers? I thought this would be a problem in that case as we are using the global virtio_adapter here. + vi->adap = &virtio_adapter; + i2c_set_adapdata(vi->adap, vi); Multiple calls to probe() will end up updating the same pointer inside adap. + vi->adap->dev.parent = &vdev->dev; Same here, overwrite. + /* Setup ACPI node for controlled devices which will be probed through ACPI */ + ACPI_COMPANION_SET(&vi->adap->dev, ACPI_COMPANION(pdev)); + vi->adap->timeout = HZ / 10; These may be fine, but still not ideal I believe. + ret = i2c_add_adapter(vi->adap); i This should be a problem as well, we must be adding this to some sort of list, doing some RPM stuff, etc ? Jie, the solution is to allocate memory for adap at runtime in probe and remove the virtio_adapter structure completely. -- viresh
On 2021/3/19 11:54, Viresh Kumar wrote: > On 18-03-21, 15:52, Arnd Bergmann wrote: >> Allowing multiple virtio-i2c controllers in one system, and multiple i2c >> devices attached to each controller is clearly something that has to work. > Good. > >> I don't actually see a limitation though. Viresh, what is the problem >> you see for having multiple controllers? > I thought this would be a problem in that case as we are using the global > virtio_adapter here. > > + vi->adap = &virtio_adapter; > + i2c_set_adapdata(vi->adap, vi); > > Multiple calls to probe() will end up updating the same pointer inside adap. > > + vi->adap->dev.parent = &vdev->dev; > > Same here, overwrite. > > + /* Setup ACPI node for controlled devices which will be probed through ACPI */ > + ACPI_COMPANION_SET(&vi->adap->dev, ACPI_COMPANION(pdev)); > + vi->adap->timeout = HZ / 10; > > These may be fine, but still not ideal I believe. > > + ret = i2c_add_adapter(vi->adap); > i > This should be a problem as well, we must be adding this to some sort of list, > doing some RPM stuff, etc ? > > Jie, the solution is to allocate memory for adap at runtime in probe and remove > the virtio_adapter structure completely. If you want to support that. Then I think we don't need to change the following at all. > + .algo = &virtio_algorithm, > + > + return ret; > + > + vi->adap = virtio_adapter; This is strange, why are you allocating memory for adapter twice ? Once for virtio_adapter and once for vi->adap ? Either fill the fields directly for v->adap here and remove virtio_adapter or make vi->adap a pointer.
On 19-03-21, 13:31, Jie Deng wrote: > > On 2021/3/19 11:54, Viresh Kumar wrote: > > On 18-03-21, 15:52, Arnd Bergmann wrote: > > > Allowing multiple virtio-i2c controllers in one system, and multiple i2c > > > devices attached to each controller is clearly something that has to work. > > Good. > > > > > I don't actually see a limitation though. Viresh, what is the problem > > > you see for having multiple controllers? > > I thought this would be a problem in that case as we are using the global > > virtio_adapter here. > > > > + vi->adap = &virtio_adapter; > > + i2c_set_adapdata(vi->adap, vi); > > > > Multiple calls to probe() will end up updating the same pointer inside adap. > > > > + vi->adap->dev.parent = &vdev->dev; > > > > Same here, overwrite. > > > > + /* Setup ACPI node for controlled devices which will be probed through ACPI */ > > + ACPI_COMPANION_SET(&vi->adap->dev, ACPI_COMPANION(pdev)); > > + vi->adap->timeout = HZ / 10; > > > > These may be fine, but still not ideal I believe. > > > > + ret = i2c_add_adapter(vi->adap); > > i > > This should be a problem as well, we must be adding this to some sort of list, > > doing some RPM stuff, etc ? > > > > Jie, the solution is to allocate memory for adap at runtime in probe and remove > > the virtio_adapter structure completely. > > > If you want to support that. Then I think we don't need to change the > following at all. > > > + .algo = &virtio_algorithm, > > + > > + return ret; > > + > > + vi->adap = virtio_adapter; > This is strange, why are you allocating memory for adapter twice ? > Once for virtio_adapter and once for vi->adap ? Either fill the fields > directly for v->adap here and remove virtio_adapter or make vi->adap a > pointer. Yes, your previous version was partly okay but you don't need the virtio_algorithm structure to be allocated. There are only 4 fields you are updating here, just fill them directly in vi->adap. (FWIW, I also suggested the same when I said "Either fill the fields directly for v->adap here and remove virtio_adapter". ) See how drivers/i2c/busses/i2c-versatile.c and most of the other drivers have done it. -- viresh
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). > + 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.. > + 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 ? > + } > + > + 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. > + } > + > + 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 ? > + 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; > +} -- viresh
On 2021/3/19 13:40, Viresh Kumar wrote: > On 19-03-21, 13:31, Jie Deng wrote: >> On 2021/3/19 11:54, Viresh Kumar wrote: >>> On 18-03-21, 15:52, Arnd Bergmann wrote: >>>> Allowing multiple virtio-i2c controllers in one system, and multiple i2c >>>> devices attached to each controller is clearly something that has to work. >>> Good. >>> >>>> I don't actually see a limitation though. Viresh, what is the problem >>>> you see for having multiple controllers? >>> I thought this would be a problem in that case as we are using the global >>> virtio_adapter here. >>> >>> + vi->adap = &virtio_adapter; >>> + i2c_set_adapdata(vi->adap, vi); >>> >>> Multiple calls to probe() will end up updating the same pointer inside adap. >>> >>> + vi->adap->dev.parent = &vdev->dev; >>> >>> Same here, overwrite. >>> >>> + /* Setup ACPI node for controlled devices which will be probed through ACPI */ >>> + ACPI_COMPANION_SET(&vi->adap->dev, ACPI_COMPANION(pdev)); >>> + vi->adap->timeout = HZ / 10; >>> >>> These may be fine, but still not ideal I believe. >>> >>> + ret = i2c_add_adapter(vi->adap); >>> i >>> This should be a problem as well, we must be adding this to some sort of list, >>> doing some RPM stuff, etc ? >>> >>> Jie, the solution is to allocate memory for adap at runtime in probe and remove >>> the virtio_adapter structure completely. >> >> If you want to support that. Then I think we don't need to change the >> following at all. >> >>> + .algo = &virtio_algorithm, >>> + >>> + return ret; >>> + >>> + vi->adap = virtio_adapter; >> This is strange, why are you allocating memory for adapter twice ? >> Once for virtio_adapter and once for vi->adap ? Either fill the fields >> directly for v->adap here and remove virtio_adapter or make vi->adap a >> pointer. > Yes, your previous version was partly okay but you don't need the > virtio_algorithm structure to be allocated. There are only 4 fields you are > updating here, just fill them directly in vi->adap. > > (FWIW, I also suggested the same when I said > "Either fill the fields directly for v->adap here and remove virtio_adapter". > ) > > See how drivers/i2c/busses/i2c-versatile.c and most of the other drivers have > done it. I also see example drivers/i2c/busses/i2c-xiic.c. Some people might think this way is more clearer than updating each member in probe. Basically, I think it's just a matter of personal preference which doesn't solve any problems.
On 19-03-21, 14:29, Jie Deng wrote: > I also see example drivers/i2c/busses/i2c-xiic.c. Some people might think > this way is more clearer than > > updating each member in probe. Basically, I think it's just a matter of > personal preference which doesn't Memory used by one instance of struct i2c_adapter (on arm64): struct i2c_adapter { struct module * owner; /* 0 8 */ unsigned int class; /* 8 4 */ /* XXX 4 bytes hole, try to pack */ const struct i2c_algorithm * algo; /* 16 8 */ void * algo_data; /* 24 8 */ const struct i2c_lock_operations * lock_ops; /* 32 8 */ struct rt_mutex bus_lock; /* 40 32 */ /* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */ struct rt_mutex mux_lock; /* 72 32 */ int timeout; /* 104 4 */ int retries; /* 108 4 */ struct device dev; /* 112 744 */ /* XXX last struct has 7 bytes of padding */ /* --- cacheline 13 boundary (832 bytes) was 24 bytes ago --- */ long unsigned int locked_flags; /* 856 8 */ int nr; /* 864 4 */ char name[48]; /* 868 48 */ /* XXX 4 bytes hole, try to pack */ /* --- cacheline 14 boundary (896 bytes) was 24 bytes ago --- */ struct completion dev_released; /* 920 32 */ struct mutex userspace_clients_lock; /* 952 32 */ /* --- cacheline 15 boundary (960 bytes) was 24 bytes ago --- */ struct list_head userspace_clients; /* 984 16 */ struct i2c_bus_recovery_info * bus_recovery_info; /* 1000 8 */ const struct i2c_adapter_quirks * quirks; /* 1008 8 */ struct irq_domain * host_notify_domain; /* 1016 8 */ /* --- cacheline 16 boundary (1024 bytes) --- */ /* size: 1024, cachelines: 16, members: 19 */ /* sum members: 1016, holes: 2, sum holes: 8 */ /* paddings: 1, sum paddings: 7 */ }; So, this extra instance that i2c-xiic.c is creating (and that you want to create) is going to waste 1KB of memory and it will never be used. This is bad coding practice IMHO and it is not really about personal preference. -- viresh
On 2021/3/19 14:35, Viresh Kumar wrote: > On 19-03-21, 14:29, Jie Deng wrote: >> I also see example drivers/i2c/busses/i2c-xiic.c. Some people might think >> this way is more clearer than >> >> updating each member in probe. Basically, I think it's just a matter of >> personal preference which doesn't > Memory used by one instance of struct i2c_adapter (on arm64): > > struct i2c_adapter { > struct module * owner; /* 0 8 */ > unsigned int class; /* 8 4 */ > > /* XXX 4 bytes hole, try to pack */ > > const struct i2c_algorithm * algo; /* 16 8 */ > void * algo_data; /* 24 8 */ > const struct i2c_lock_operations * lock_ops; /* 32 8 */ > struct rt_mutex bus_lock; /* 40 32 */ > /* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */ > struct rt_mutex mux_lock; /* 72 32 */ > int timeout; /* 104 4 */ > int retries; /* 108 4 */ > struct device dev; /* 112 744 */ > > /* XXX last struct has 7 bytes of padding */ > > /* --- cacheline 13 boundary (832 bytes) was 24 bytes ago --- */ > long unsigned int locked_flags; /* 856 8 */ > int nr; /* 864 4 */ > char name[48]; /* 868 48 */ > > /* XXX 4 bytes hole, try to pack */ > > /* --- cacheline 14 boundary (896 bytes) was 24 bytes ago --- */ > struct completion dev_released; /* 920 32 */ > struct mutex userspace_clients_lock; /* 952 32 */ > /* --- cacheline 15 boundary (960 bytes) was 24 bytes ago --- */ > struct list_head userspace_clients; /* 984 16 */ > struct i2c_bus_recovery_info * bus_recovery_info; /* 1000 8 */ > const struct i2c_adapter_quirks * quirks; /* 1008 8 */ > struct irq_domain * host_notify_domain; /* 1016 8 */ > /* --- cacheline 16 boundary (1024 bytes) --- */ > > /* size: 1024, cachelines: 16, members: 19 */ > /* sum members: 1016, holes: 2, sum holes: 8 */ > /* paddings: 1, sum paddings: 7 */ > }; > > So, this extra instance that i2c-xiic.c is creating (and that you want to > create) is going to waste 1KB of memory and it will never be used. > > This is bad coding practice IMHO and it is not really about personal preference. I will remove that structure and update the members in probe.
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; >> +}
On Fri, Mar 19, 2021 at 7:35 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 19-03-21, 14:29, Jie Deng wrote: > > I also see example drivers/i2c/busses/i2c-xiic.c. Some people might think > > this way is more clearer than > > > > updating each member in probe. Basically, I think it's just a matter of > > personal preference which doesn't > > Memory used by one instance of struct i2c_adapter (on arm64): > > struct i2c_adapter { ... > }; > > So, this extra instance that i2c-xiic.c is creating (and that you want to > create) is going to waste 1KB of memory and it will never be used. > > This is bad coding practice IMHO and it is not really about personal preference. Agreed. At the minimum, it should have been written as an explicit memcpy() in the few drivers that have that pattern instead of a benign looking struct assignment, but even then there is nothing good about it really. Notably, the largest member by far is the 'struct device', and that by itself should be a red flag as a device is never meant to be allocated statically (this used to be common in pre-DT days, but even then was considered bad style). I suppose the i2c_add_adapter()/i2c_add_numbered_adapter() interface could be redesigned to handle this better, since every driver needs to set the same few fields. That however requires finding someone to spend the effort on coming up with a nice design and converting lots of drivers over. Arnd
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 05ebf75..cb8d0d8 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -21,6 +21,17 @@ config I2C_ALI1535 This driver can also be built as a module. If so, the module will be called i2c-ali1535. +config I2C_VIRTIO + tristate "Virtio I2C Adapter" + select VIRTIO + help + If you say yes to this option, support will be included for the virtio + I2C adapter driver. The hardware can be emulated by any device model + software according to the virtio protocol. + + This driver can also be built as a module. If so, the module + will be called i2c-virtio. + config I2C_ALI1563 tristate "ALI 1563" depends on PCI diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 615f35e..efdd3f3 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -145,4 +145,7 @@ obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o obj-$(CONFIG_SCx200_ACB) += scx200_acb.o obj-$(CONFIG_I2C_FSI) += i2c-fsi.o +# VIRTIO I2C host controller driver +obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o + ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c new file mode 100644 index 0000000..7a54bac --- /dev/null +++ b/drivers/i2c/busses/i2c-virtio.c @@ -0,0 +1,289 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Virtio I2C Bus Driver + * + * The Virtio I2C Specification: + * https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex + * + * Copyright (c) 2021 Intel Corporation. All rights reserved. + */ + +#include <linux/acpi.h> +#include <linux/completion.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/virtio.h> +#include <linux/virtio_ids.h> +#include <linux/virtio_config.h> +#include <linux/virtio_i2c.h> + +/** + * struct virtio_i2c - virtio I2C data + * @vdev: virtio device for this controller + * @completion: completion of virtio I2C message + * @adap: I2C adapter for this controller + * @i2c_lock: lock for virtqueue processing + * @vq: the virtio virtqueue for communication + */ +struct virtio_i2c { + struct virtio_device *vdev; + struct completion completion; + struct i2c_adapter *adap; + struct mutex lock; + struct virtqueue *vq; +}; + +/** + * struct virtio_i2c_req - the virtio I2C request structure + * @out_hdr: the OUT header of the virtio I2C message + * @buf: the buffer into which data is read, or from which it's written + * @in_hdr: the IN header of the virtio I2C message + */ +struct virtio_i2c_req { + struct virtio_i2c_out_hdr out_hdr ____cacheline_aligned; + uint8_t *buf ____cacheline_aligned; + struct virtio_i2c_in_hdr in_hdr ____cacheline_aligned; +}; + +static void virtio_i2c_msg_done(struct virtqueue *vq) +{ + struct virtio_i2c *vi = vq->vdev->priv; + + complete(&vi->completion); +} + +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); + 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); + 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; + } + + 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); + } + + 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) { + 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; +} + +static void virtio_i2c_del_vqs(struct virtio_device *vdev) +{ + vdev->config->reset(vdev); + vdev->config->del_vqs(vdev); +} + +static int virtio_i2c_setup_vqs(struct virtio_i2c *vi) +{ + struct virtio_device *vdev = vi->vdev; + + vi->vq = virtio_find_single_vq(vdev, virtio_i2c_msg_done, "msg"); + return PTR_ERR_OR_ZERO(vi->vq); +} + +static u32 virtio_i2c_func(struct i2c_adapter *adap) +{ + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; +} + +static struct i2c_algorithm virtio_algorithm = { + .master_xfer = virtio_i2c_xfer, + .functionality = virtio_i2c_func, +}; + +static struct i2c_adapter virtio_adapter = { + .owner = THIS_MODULE, + .name = "Virtio I2C Adapter", + .class = I2C_CLASS_DEPRECATED, + .algo = &virtio_algorithm, +}; + +static int virtio_i2c_probe(struct virtio_device *vdev) +{ + struct device *pdev = vdev->dev.parent; + struct virtio_i2c *vi; + int ret; + + vi = devm_kzalloc(&vdev->dev, sizeof(*vi), GFP_KERNEL); + if (!vi) + return -ENOMEM; + + vdev->priv = vi; + vi->vdev = vdev; + + mutex_init(&vi->lock); + init_completion(&vi->completion); + + ret = virtio_i2c_setup_vqs(vi); + if (ret) + return ret; + + vi->adap = &virtio_adapter; + i2c_set_adapdata(vi->adap, vi); + vi->adap->dev.parent = &vdev->dev; + + /* Setup ACPI node for controlled devices which will be probed through ACPI */ + ACPI_COMPANION_SET(&vi->adap->dev, ACPI_COMPANION(pdev)); + vi->adap->timeout = HZ / 10; + + ret = i2c_add_adapter(vi->adap); + if (ret) { + virtio_i2c_del_vqs(vdev); + dev_err(&vdev->dev, "failed to add virtio-i2c adapter.\n"); + } + + return ret; +} + +static void virtio_i2c_remove(struct virtio_device *vdev) +{ + struct virtio_i2c *vi = vdev->priv; + + i2c_del_adapter(vi->adap); + virtio_i2c_del_vqs(vdev); +} + +static struct virtio_device_id id_table[] = { + { VIRTIO_ID_I2C_ADPTER, VIRTIO_DEV_ANY_ID }, + {} +}; +MODULE_DEVICE_TABLE(virtio, id_table); + +static int __maybe_unused virtio_i2c_freeze(struct virtio_device *vdev) +{ + virtio_i2c_del_vqs(vdev); + return 0; +} + +static int __maybe_unused virtio_i2c_restore(struct virtio_device *vdev) +{ + return virtio_i2c_setup_vqs(vdev->priv); +} + +static struct virtio_driver virtio_i2c_driver = { + .id_table = id_table, + .probe = virtio_i2c_probe, + .remove = virtio_i2c_remove, + .driver = { + .name = "i2c_virtio", + }, +#ifdef CONFIG_PM_SLEEP + .freeze = virtio_i2c_freeze, + .restore = virtio_i2c_restore, +#endif +}; +module_virtio_driver(virtio_i2c_driver); + +MODULE_AUTHOR("Jie Deng <jie.deng@intel.com>"); +MODULE_AUTHOR("Conghui Chen <conghui.chen@intel.com>"); +MODULE_DESCRIPTION("Virtio i2c bus driver"); +MODULE_LICENSE("GPL"); diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h new file mode 100644 index 0000000..bbcfb2c --- /dev/null +++ b/include/uapi/linux/virtio_i2c.h @@ -0,0 +1,40 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */ +/* + * Definitions for virtio I2C Adpter + * + * Copyright (c) 2021 Intel Corporation. All rights reserved. + */ + +#ifndef _UAPI_LINUX_VIRTIO_I2C_H +#define _UAPI_LINUX_VIRTIO_I2C_H + +#include <linux/types.h> + +/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */ +#define VIRTIO_I2C_FLAGS_FAIL_NEXT 0x00000001 + +/** + * struct virtio_i2c_out_hdr - the virtio I2C message OUT header + * @addr: the controlled device address + * @padding: used to pad to full dword + * @flags: used for feature extensibility + */ +struct virtio_i2c_out_hdr { + __le16 addr; + __le16 padding; + __le32 flags; +}; + +/** + * struct virtio_i2c_in_hdr - the virtio I2C message IN header + * @status: the processing result from the backend + */ +struct virtio_i2c_in_hdr { + __u8 status; +}; + +/* The final status written by the device */ +#define VIRTIO_I2C_MSG_OK 0 +#define VIRTIO_I2C_MSG_ERR 1 + +#endif /* _UAPI_LINUX_VIRTIO_I2C_H */ diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h index bc1c062..6ae32db 100644 --- a/include/uapi/linux/virtio_ids.h +++ b/include/uapi/linux/virtio_ids.h @@ -54,5 +54,6 @@ #define VIRTIO_ID_FS 26 /* virtio filesystem */ #define VIRTIO_ID_PMEM 27 /* virtio pmem */ #define VIRTIO_ID_MAC80211_HWSIM 29 /* virtio mac80211-hwsim */ +#define VIRTIO_ID_I2C_ADPTER 34 /* virtio i2c adpter */ #endif /* _LINUX_VIRTIO_IDS_H */