Hello,
This was earlier sent as part of a patch series [1] adding support for GPIO/I2C
virtio devices. The device specific patches would require some rework and
possibly several versions, and so this series separates out the generic
independent patches into a series of their own.
This series makes some of the generic code independent of the disk device, since
it can be used for other device types later on.
Rebased over staging branch from today.
V5->V6:
- Separated into a patch series of their own.
- Updated commit log of 1st patch to cover all changes.
- Rename make_virtio_mmio_node_simple() as make_virtio_mmio_node().
- New patch 3/3, separated code from device specific patch.
--
Viresh
Viresh Kumar (3):
libxl: arm: Create alloc_virtio_mmio_params()
libxl: arm: Split make_virtio_mmio_node()
libxl: arm: make creation of iommu node independent of disk device
tools/libs/light/libxl_arm.c | 83 +++++++++++++++++++++++++-----------
1 file changed, 57 insertions(+), 26 deletions(-)
--
2.31.1.272.g89b43f80a514
--
Viresh
[1] https://lore.kernel.org/all/cover.1661159474.git.viresh.kumar@linaro.org/
Hello,
Here is another version of the rust bindings, based of the master branch.
Pushed here:
https://github.com/vireshk/libgpiod v8
V7->V8:
- Several updates to cargo.toml files, like license, version, etc.
- Removed Sync support for chip and gpiosim.
- Implemented, in a separate patch, iterator support for Events.
- Fixed missing SAFETY comments.
- Fixed build for 32 bit systems.
- Use errno::Errno.
- Removed Clone derive for many structures, that store raw pointers.
- line setting helpers return the object back, so another helper can be called
directly on them. Also made all helpers public and used the same in tests and
example for single configurations.
- Enums for gpiosim constants.
- New examples to demonstrate parallelism and event handling.
- Separated out HTE tests and marked as #[ignore] now.
- Updated commit subjects.
- Other minor changes.
V6->V7:
- Don't let buffer read new events if the earlier events are still referenced.
- BufferIntenal is gone now, to make the above work.
- Update example and tests too for the same.
V5->V6:
- Updates according to the new line-settings interface.
- New file, line_settings.rs.
- Renamed 'enum Setting' as 'SettingVal' to avoid conflicting names, as we also
have 'struct Settings' now.
- Support for HTE clock type.
- Implement 'Eq' for public structure/enums (reported by build).
- Remove 'SettingKindMap' and 'SettingMap' as they aren't required anymore.
- Updated tests based on new interface.
V4->V5:
- Arrange as workspace with crates for libgpiod-sys, libgpiod, gpiosim.
- Use static libgpiod and libgpiosim libraries instead of rebuilding again.
- Arrange in modules instead of flattened approach.
- New enums like Setting and SettingKind and new types based on them SettingMap
and SettingKindMap.
- New property independent helpers for line_config, like set_prop_default().
- Improved tests/examples, new example for gpiowatch.
- Add pre-built bindings for gpiosim too.
- Many other changes.
V3->V4:
- Rebased on top of new changes, and made changes accordingly.
- Added rust integration tests with gpiosim.
- Found a kernel bug with tests, sent a patch for that to LKML.
V2->V3:
- Remove naming redundancy, users just need to do this now
use libgpiod:{Chip, Direction, LineConfig} now (Bartosz);
- Fix lifetime issues between event-buffer and edge-event modules, the event
buffer is released after the last edge-event reference is dropped (Bartosz).
- Allow edge-event to be copied, and freed later (Bartosz).
- Add two separate rust crates, sys and wrapper (Gerard).
- Null-terminate the strings passed to libgpiod (Wedson).
- Drop unnecessary checks to validate string returned from chip:name/label/path.
- Fix SAFETY comments (Wedson).
- Drop unnecessary clone() instances (Bartosz).
V1->V2:
- Added examples (I tested everything except gpiomon.rs, didn't have right
hardware/mock device to test).
- Build rust bindings as part of Make, update documentation.
Thanks.
--
Viresh
Viresh Kumar (9):
bindings: rust: Add libgpiod-sys rust crate
bindings: rust: Add pre generated bindings for libgpiod-sys
bindings: rust: Add gpiosim crate
bindings: rust: Add pre generated bindings for gpiosim
bindings: rust: Add libgpiod crate
bindings: rust: Add examples to libgpiod crate
bindings: rust: Add tests for libgpiod crate
bindings: rust: Integrate building of bindings with make
bindings: rust: Implement iterator for edge events
.gitignore | 5 +
README | 8 +-
TODO | 8 -
bindings/Makefile.am | 6 +
bindings/rust/Cargo.toml | 7 +
bindings/rust/Makefile.am | 18 +
bindings/rust/gpiosim/Cargo.toml | 24 +
bindings/rust/gpiosim/README.md | 11 +
bindings/rust/gpiosim/build.rs | 43 +
bindings/rust/gpiosim/src/bindings.rs | 180 +++
bindings/rust/gpiosim/src/lib.rs | 79 ++
bindings/rust/gpiosim/src/sim.rs | 331 +++++
bindings/rust/libgpiod-sys/Cargo.toml | 22 +
bindings/rust/libgpiod-sys/README.md | 11 +
bindings/rust/libgpiod-sys/build.rs | 41 +
bindings/rust/libgpiod-sys/src/bindings.rs | 1173 +++++++++++++++++
bindings/rust/libgpiod-sys/src/lib.rs | 13 +
bindings/rust/libgpiod/Cargo.toml | 23 +
.../rust/libgpiod/examples/gpio_events.rs | 89 ++
.../examples/gpio_threaded_info_events.rs | 133 ++
bindings/rust/libgpiod/examples/gpiodetect.rs | 31 +
bindings/rust/libgpiod/examples/gpiofind.rs | 37 +
bindings/rust/libgpiod/examples/gpioget.rs | 46 +
bindings/rust/libgpiod/examples/gpioinfo.rs | 98 ++
bindings/rust/libgpiod/examples/gpiomon.rs | 74 ++
bindings/rust/libgpiod/examples/gpioset.rs | 64 +
bindings/rust/libgpiod/examples/gpiowatch.rs | 54 +
bindings/rust/libgpiod/src/chip.rs | 317 +++++
bindings/rust/libgpiod/src/edge_event.rs | 110 ++
bindings/rust/libgpiod/src/event_buffer.rs | 179 +++
bindings/rust/libgpiod/src/info_event.rs | 69 +
bindings/rust/libgpiod/src/lib.rs | 480 +++++++
bindings/rust/libgpiod/src/line_config.rs | 135 ++
bindings/rust/libgpiod/src/line_info.rs | 162 +++
bindings/rust/libgpiod/src/line_request.rs | 227 ++++
bindings/rust/libgpiod/src/line_settings.rs | 297 +++++
bindings/rust/libgpiod/src/request_config.rs | 95 ++
bindings/rust/libgpiod/tests/chip.rs | 99 ++
bindings/rust/libgpiod/tests/common/config.rs | 143 ++
bindings/rust/libgpiod/tests/common/mod.rs | 10 +
bindings/rust/libgpiod/tests/edge_event.rs | 299 +++++
bindings/rust/libgpiod/tests/info_event.rs | 167 +++
bindings/rust/libgpiod/tests/line_config.rs | 96 ++
bindings/rust/libgpiod/tests/line_info.rs | 276 ++++
bindings/rust/libgpiod/tests/line_request.rs | 510 +++++++
bindings/rust/libgpiod/tests/line_settings.rs | 204 +++
.../rust/libgpiod/tests/request_config.rs | 39 +
configure.ac | 16 +
48 files changed, 6548 insertions(+), 11 deletions(-)
create mode 100644 bindings/rust/Cargo.toml
create mode 100644 bindings/rust/Makefile.am
create mode 100644 bindings/rust/gpiosim/Cargo.toml
create mode 100644 bindings/rust/gpiosim/README.md
create mode 100644 bindings/rust/gpiosim/build.rs
create mode 100644 bindings/rust/gpiosim/src/bindings.rs
create mode 100644 bindings/rust/gpiosim/src/lib.rs
create mode 100644 bindings/rust/gpiosim/src/sim.rs
create mode 100644 bindings/rust/libgpiod-sys/Cargo.toml
create mode 100644 bindings/rust/libgpiod-sys/README.md
create mode 100644 bindings/rust/libgpiod-sys/build.rs
create mode 100644 bindings/rust/libgpiod-sys/src/bindings.rs
create mode 100644 bindings/rust/libgpiod-sys/src/lib.rs
create mode 100644 bindings/rust/libgpiod/Cargo.toml
create mode 100644 bindings/rust/libgpiod/examples/gpio_events.rs
create mode 100644 bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs
create mode 100644 bindings/rust/libgpiod/examples/gpiodetect.rs
create mode 100644 bindings/rust/libgpiod/examples/gpiofind.rs
create mode 100644 bindings/rust/libgpiod/examples/gpioget.rs
create mode 100644 bindings/rust/libgpiod/examples/gpioinfo.rs
create mode 100644 bindings/rust/libgpiod/examples/gpiomon.rs
create mode 100644 bindings/rust/libgpiod/examples/gpioset.rs
create mode 100644 bindings/rust/libgpiod/examples/gpiowatch.rs
create mode 100644 bindings/rust/libgpiod/src/chip.rs
create mode 100644 bindings/rust/libgpiod/src/edge_event.rs
create mode 100644 bindings/rust/libgpiod/src/event_buffer.rs
create mode 100644 bindings/rust/libgpiod/src/info_event.rs
create mode 100644 bindings/rust/libgpiod/src/lib.rs
create mode 100644 bindings/rust/libgpiod/src/line_config.rs
create mode 100644 bindings/rust/libgpiod/src/line_info.rs
create mode 100644 bindings/rust/libgpiod/src/line_request.rs
create mode 100644 bindings/rust/libgpiod/src/line_settings.rs
create mode 100644 bindings/rust/libgpiod/src/request_config.rs
create mode 100644 bindings/rust/libgpiod/tests/chip.rs
create mode 100644 bindings/rust/libgpiod/tests/common/config.rs
create mode 100644 bindings/rust/libgpiod/tests/common/mod.rs
create mode 100644 bindings/rust/libgpiod/tests/edge_event.rs
create mode 100644 bindings/rust/libgpiod/tests/info_event.rs
create mode 100644 bindings/rust/libgpiod/tests/line_config.rs
create mode 100644 bindings/rust/libgpiod/tests/line_info.rs
create mode 100644 bindings/rust/libgpiod/tests/line_request.rs
create mode 100644 bindings/rust/libgpiod/tests/line_settings.rs
create mode 100644 bindings/rust/libgpiod/tests/request_config.rs
--
2.31.1.272.g89b43f80a514
() b
On Thu, Aug 25, 2022 at 3:44 PM Harald Mommer
<harald.mommer(a)opensynergy.com> wrote:
>
> - CAN Control
>
> - "ip link set up can0" starts the virtual CAN controller,
> - "ip link set up can0" stops the virtual CAN controller
>
> - CAN RX
>
> Receive CAN frames. CAN frames can be standard or extended, classic or
> CAN FD. Classic CAN RTR frames are supported.
>
> - CAN TX
>
> Send CAN frames. CAN frames can be standard or extended, classic or
> CAN FD. Classic CAN RTR frames are supported.
>
> - CAN Event indication (BusOff)
>
> The bus off handling is considered code complete but until now bus off
> handling is largely untested.
>
> Signed-off-by: Harald Mommer <hmo(a)opensynergy.com>
This looks nice overall, but as you say there is still some work needed in all
the details. I've done a rough first pass at reviewing it, but I have
no specific
understanding of CAN, so these are mostly generic comments about
coding style or network drivers.
> drivers/net/can/Kconfig | 1 +
> drivers/net/can/Makefile | 1 +
> drivers/net/can/virtio_can/Kconfig | 12 +
> drivers/net/can/virtio_can/Makefile | 5 +
> drivers/net/can/virtio_can/virtio_can.c | 1176 +++++++++++++++++++++++
> include/uapi/linux/virtio_can.h | 69 ++
Since the driver is just one file, you probably don't need the subdirectory.
> +struct virtio_can_tx {
> + struct list_head list;
> + int prio; /* Currently always 0 "normal priority" */
> + int putidx;
> + struct virtio_can_tx_out tx_out;
> + struct virtio_can_tx_in tx_in;
> +};
Having a linked list of these appears to add a little extra complexity.
If they are always processed in sequence, using an array would be
much simpler, as you just need to remember the index.
> +#ifdef DEBUG
> +static void __attribute__((unused))
> +virtio_can_hexdump(const void *data, size_t length, size_t base)
> +{
> +#define VIRTIO_CAN_MAX_BYTES_PER_LINE 16u
This seems to duplicate print_hex_dump(), maybe just use that?
> +
> + while (!virtqueue_get_buf(vq, &len) && !virtqueue_is_broken(vq))
> + cpu_relax();
> +
> + mutex_unlock(&priv->ctrl_lock);
A busy loop is probably not what you want here. Maybe just
wait_for_completion() until the callback happens?
> + /* Push loopback echo. Will be looped back on TX interrupt/TX NAPI */
> + can_put_echo_skb(skb, dev, can_tx_msg->putidx, 0);
> +
> + err = virtqueue_add_sgs(vq, sgs, 1u, 1u, can_tx_msg, GFP_ATOMIC);
> + if (err != 0) {
> + list_del(&can_tx_msg->list);
> + virtio_can_free_tx_idx(priv, can_tx_msg->prio,
> + can_tx_msg->putidx);
> + netif_stop_queue(dev);
> + spin_unlock_irqrestore(&priv->tx_lock, flags);
> + kfree(can_tx_msg);
> + if (err == -ENOSPC)
> + netdev_info(dev, "TX: Stop queue, no space left\n");
> + else
> + netdev_warn(dev, "TX: Stop queue, reason = %d\n", err);
> + return NETDEV_TX_BUSY;
> + }
> +
> + if (!virtqueue_kick(vq))
> + netdev_err(dev, "%s(): Kick failed\n", __func__);
> +
> + spin_unlock_irqrestore(&priv->tx_lock, flags);
There should not be a need for a spinlock or disabling interrupts
in the xmit function. What exactly are you protecting against here?
As a further optimization, you may want to use the xmit_more()
function, as the virtqueue kick is fairly expensive and can be
batched here.
> + kfree(can_tx_msg);
> +
> + /* Flow control */
> + if (netif_queue_stopped(dev)) {
> + netdev_info(dev, "TX ACK: Wake up stopped queue\n");
> + netif_wake_queue(dev);
> + }
You may want to add netdev_sent_queue()/netdev_completed_queue()
based BQL flow control here as well, so you don't have to rely on the
queue filling up completely.
> +static int virtio_can_probe(struct virtio_device *vdev)
> +{
> + struct net_device *dev;
> + struct virtio_can_priv *priv;
> + int err;
> + unsigned int echo_skb_max;
> + unsigned int idx;
> + u16 lo_tx = VIRTIO_CAN_ECHO_SKB_MAX;
> +
> + BUG_ON(!vdev);
Not a useful debug check, just remove the BUG_ON(!vdev), here and elsewhere
> +
> + echo_skb_max = lo_tx;
> + dev = alloc_candev(sizeof(struct virtio_can_priv), echo_skb_max);
> + if (!dev)
> + return -ENOMEM;
> +
> + priv = netdev_priv(dev);
> +
> + dev_info(&vdev->dev, "echo_skb_max = %u\n", priv->can.echo_skb_max);
Also remove the prints, I assume this is left over from
initial debugging.
> + priv->can.do_set_mode = virtio_can_set_mode;
> + priv->can.state = CAN_STATE_STOPPED;
> + /* Set Virtio CAN supported operations */
> + priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;
> + if (virtio_has_feature(vdev, VIRTIO_CAN_F_CAN_FD)) {
> + dev_info(&vdev->dev, "CAN FD is supported\n");
> + } else {
> + dev_info(&vdev->dev, "CAN FD not supported\n");
> + }
Same here. There should be a way to see CAN FD support as an interactive
user, but there is no point printing it to the console.
> +
> + register_virtio_can_dev(dev);
> +
> + /* Initialize virtqueues */
> + err = virtio_can_find_vqs(priv);
> + if (err != 0)
> + goto on_failure;
Should the register_virtio_can_dev() be done here? I would expect this to be
the last thing after setting up the queues.
> +static struct virtio_driver virtio_can_driver = {
> + .feature_table = features,
> + .feature_table_size = ARRAY_SIZE(features),
> + .feature_table_legacy = NULL,
> + .feature_table_size_legacy = 0u,
> + .driver.name = KBUILD_MODNAME,
> + .driver.owner = THIS_MODULE,
> + .id_table = virtio_can_id_table,
> + .validate = virtio_can_validate,
> + .probe = virtio_can_probe,
> + .remove = virtio_can_remove,
> + .config_changed = NULL,
> +#ifdef CONFIG_PM_SLEEP
> + .freeze = virtio_can_freeze,
> + .restore = virtio_can_restore,
> +#endif
You can remove the #ifdef here and above, and replace that with the
pm_sleep_ptr() macro in the assignment.
> diff --git a/include/uapi/linux/virtio_can.h b/include/uapi/linux/virtio_can.h
> new file mode 100644
> index 000000000000..0ca75c7a98ee
> --- /dev/null
> +++ b/include/uapi/linux/virtio_can.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Copyright (C) 2021 OpenSynergy GmbH
> + */
> +#ifndef _LINUX_VIRTIO_VIRTIO_CAN_H
> +#define _LINUX_VIRTIO_VIRTIO_CAN_H
> +
> +#include <linux/types.h>
> +#include <linux/virtio_types.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>
Maybe a link to the specification here? I assume the definitions in this file
are all lifted from that document, rather than specific to the driver, right?
Arnd
Hi All,
There have been discussions about virtio-camera before and more recently
I've heard the term virtio-sensor used. I think using "sensor" eludes to
the fact that there are a whole class of devices that provide some sort
of 2d plane view of the world (cameras, fingerprint readers, LIDAR?)
that would benefit in being consumed by a workload in a standard
non-bespoke way.
Why not virtio-video?
=====================
There is already a specification and various implementations of
virtio-video in various states of up-streaming. It is tempting to think
of a camera as a simplified subset of processing video streams. However
while virtio-video allows the consumption and display of various video
formats it offers no direct control of the source itself.
Complex control plane
=====================
Modern cameras are more than a simple CCD recording photons. Aside from
controlling things like f-stop/exposure/position there are also more
complex computational photography aspects. The camera soc might be
capable of doing edge or object detection or even facial and feature
recognition. Cameras are no longer simple webcams and have long since
gone past the relatively simple API that V4L present (c.f. libcamera).
Cloud native
============
One of the drivers for these virtio devices is the concept of cloud
native development. That is developing your workload in the cloud and
feeding it data through standardised VirtIO interfaces. Once you are
happy with its behaviour you can take that workload and run the same
binaries in your edge device but this time with data being provided by a
real sensor which is exposed via the same VirtIO interface.
Competing Requirements?
=======================
I've heard about use cases across a wide range of deployment scenarios
including:
Virtualised Mobile Devices
Here the backend containing the vendors secret sauce exists in its own
isolated VM with access to the real camera HW and exports virtio-camera
to a standardised main OS.
Desktop Virtualisation
Here the aim is to expose host camera devices (such as webcams) to
guest system which would be an otherwise sandboxed VM that needs access
to the system camera for a particular task.
Automotive
Cars are rapidly growing cameras both as driver aids and for more
advanced use cases such as self driving. The cloud native case is
particularly strong here as a lot of validation and iteration will be
taking place in the relatively limitless resources of the cloud before
being run in the carefully controlled and isolated safety critical
environs of the car itself.
Do these use-cases have competing demands? Can a solution be found that
satisfies all of them?
So for the next Stratos sync-up call I'd like to discuss virtio-camera
and if there is enough interest to specify a work package to define and
upstream the device. I'm casting a wide net for people who are
interested in the topic so we can talk through the issues and see if we
can arrive at consensus for a minimal viable product.
To help with that I would welcome people submitting ahead of time any:
- use-cases and user stories
- concrete requirements
and also any:
- previous work and RFCs
Please forward this email to anyone else who you think might find this
discussion relevant.
The next Stratos meeting will be on the 14th October, 15:00 UTC / 16:00
BST @ https://meet.google.com/rct-xgqm-woi
The meeting notes will be:
https://linaro.atlassian.net/wiki/spaces/STR/pages/28771778789/2022-10-14+P…
And as ever the project jumping off page is at:
https://linaro.atlassian.net/wiki/spaces/STR/overview
Thanks,
--
Alex Bennée