Andrzej Pietrasiewicz
February 18, 2019
Share this post:
Reading time:
tl;dr: Automate your gadget creation. A look at how to implement USB gadget devices on Linux machines which have the necessary UDC hardware, automate the manual configfs process via declarative gadget "schemes", and use systemd for gadget composition at boot time.
In order to understand what is a USB gadget we need to have a look at a broader picture. In USB there are two distinct roles: a host and a device. The purpose of USB is to extend the host with some functionalities provided by devices: be it a mass storage device, an Ethernet card on USB, a sound card or the like. On a given USB bus there can be only one host and many (up to 127) devices. The bus is host-centric, which means that all the activities happening on it are decided and directed by the host.
One way of implementing a USB device is to have a machine running Linux, equipped with a special piece of hardware called USB Device Controller (UDC), and appropriate software running on it. It is exactly this case we will be talking about in this post.
The question you likely ask yourself is whether your machine has a UDC. In case of desktop PCs you most probably need a dedicated add-on card, which is not a very popular thing: there are not so many users who might want to convert their desktop PC into a USB device. But such boards do exist. The hardware supported by the Linux kernel can be found in drivers/usb/gadget/udc/Kconfig (line 306 in v5.0-rc6). In the embedded world a UDC is very often a part of your system-on-chip (SoC), but beware: merely having a UDC inside SoC does not mean that it is actually connected to anything on the board.
For example Raspberry Pi Zero's SoC does contain a UDC and it is connected to one of the micro USB sockets on the board. Other examples of suitably equipped boards are Odroid U3 and XU3, or Beagle Bone Black. If you don't have access to any hardware of this kind, fear not! You still can play (to some extent) with USB gadgets using an emulated UDC which is a part of the dummy_hcd kernel module. The dummy_hcd combines an emulated host controller with an emulated device controller, so your machine acts as both a USB host and a device. More on dummy_hcd will be in another blogpost which I'm going to write soon.
Oh, and one more thing: you can often come across the term OTG, which stands for on-the-go device and refers to chips which are capable of being either a host, or a device. For the purpose of this post we will be using the term UDC.
The Linux kernel provides drivers for various UDCs. But merely being able to drive a UDC is not enough to fully implement a USB device. What is missing is actual functionality, for example mass storage or Ethernet over USB. And here comes what USB standard says: a USB device can provide more than one functionality over a single USB cable at a time. A set of such functionalities is called a configuration. In fact the standard allows more than one configuration - only one can be active at a time, though - but devices providing more than one are rarely seen in practice.
In the Linux kernel an implementation of a USB device is called a USB gadget. This implementation of gadgets is nicely layered: there is a so called composite layer, which contains code common for all USB functionalities and allows composing gadgets out of several functionalities. The composite layer talks to the UDC driver. On top of the composite driver there are USB functionalities (such as mass storage or Ethernet), which are called USB functions. We will be focusing on the composite layer and functions.
The traditional approach to gadgets composition was to create a kernel module for a given composition of a gadget. And even if you wanted only a slightly different set of USB functions in your gadget, you had to create another kernel module. Around late 2012 a new approach started appearing. The idea was to decouple information about gadget composition from code and only provide building blocks out of which the user composes their gadget at runtime. Very much in the spirit of "mechanism, not policy" philosophy. The interface chosen for userspace interaction was configfs (by default can be found in /sys/kernel/config). After about two years, all USB functions available in the Linux kernel had been converted to use the new interface.
The usage pattern is like this: the user creates a separate directory per each gadget they want to have, gives their gadget a personality by specifying vendor id, product id and USB strings (visible e.g. after running lsusb -v as root), then under that directory creates the configurations they want and instantiates USB functions they want (both by creating respective directories) and finally associates functions to configurations with symbolic links. At this point gadget's composition is already in memory, but is not bound to any UDC. To activate the gadget one must write UDC name to the UDC attribute in the gadget's configfs directory - the gadget then becomes bound to this particular UDC (and the UDC cannot be used by more than one gadget). Available UDC names are in /sys/class/udc. Only after a gadget is bound to a UDC can it be successfully enumerated by the USB host.
A working, minimal example of ECM (Ethernet) on an Odroid U3, which leaves some attributes at their default values:
# go to configfs directory for USB gadgets
CONFIGFS_ROOT=/sys/kernel/config # adapt to your machine
cd "${CONFIGFS_ROOT}"/usb_gadget
# create gadget directory and enter it
mkdir g1
cd g1
# USB ids
echo 0x1d6b > idVendor
echo 0x104 > idProduct
# USB strings, optional
mkdir strings/0x409 # US English, others rarely seen
echo "Collabora" > strings/0x409/manufacturer
echo "ECM" > strings/0x409/product
# create the (only) configuration
mkdir configs/c.1 # dot and number mandatory
# create the (only) function
mkdir functions/ecm.usb0 # .
# assign function to configuration
ln -s functions/ecm.usb0/ configs/c.1/
# bind!
echo 12480000.hsotg > UDC # ls /sys/class/udc to see available UDCs
Please note that your vendor id is assigned for a fee by USB Implementors Forum (USB IF) - this refers to products you want to put on the market. For your own tinkering you can choose whatever you like. However, these ids (vendor and product) can be used by the host to decide which host-side driver to use to talk to your device. 0x1d6b is for Linux Foundation and 0x0104 is for Ethernet Gagdet. If your USB host sees such ids it assumes it needs the cdc_ether host-side driver.
If your device is connected to a Linux host, then you shoud see output similar to the below in host's dmesg:
usb 3-1.2.1.4.4: New USB device found, idVendor=1d6b, idProduct=0104 usb 3-1.2.1.4.4: New USB device strings: Mfr=1, Product=2, SerialNumber=3 usb 3-1.2.1.4.4: Product: ECM usb 3-1.2.1.4.4: Manufacturer: Collabora cdc_ether 3-1.2.1.4.4:1.0 usb0: register 'cdc_ether' at usb-0000:3c:00.0-1.2.1.4.4, CDC Ethernet Device, d2:c2:2d:b7:8e:6b
Note the Product and Manufacturer strings which are exactly what has been written to configfs.
A new usb interface should appear at the host side...
ifconfig -a
usb0: flags=4099<UP,BROADCAST,MULTICAST> mtu 1500
ether d2:c2:2d:b7:8e:6b txqueuelen 1000 (Ethernet)
RX packets 0 bytes 0 (0.0 B)
RX errors 0 dropped 0 overruns 0 frame 0
TX packets 1 bytes 90 (90.0 B)
TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0
# why not configure it?
ifconfig usb0 192.168.1.2 up
...and at the device:
ifconfig -a
usb0: flags=4098<BROADCAST,MULTICAST> mtu 1500
ether f2:40:e6:d3:01:2c txqueuelen 1000 (Ethernet)
RX packets 0 bytes 0 (0.0 B)
RX errors 0 dropped 0 overruns 0 frame 0
TX packets 0 bytes 0 (0.0 B)
TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0
# why not configure this one as well?
ifconfig usb0 192.168.1.3 up
# and ping the host?
ping 192.168.1.2
PING 192.168.1.2 (192.168.1.2) 56(84) bytes of data.
64 bytes from 192.168.1.2: icmp_seq=1 ttl=64 time=1.40 ms
Similarly, the device can be pinged from the host:
ping 192.168.1.3 PING 192.168.1.3 (192.168.1.3) 56(84) bytes of data. 64 bytes from 192.168.1.3: icmp_seq=1 ttl=64 time=1.06 ms
Poking around configfs is not difficult, but you need to know where to look, what to look for, what kind of directories to create and what values to write to particular files. And what to symlink from where. Creating a gadget containing just one function takes about 15-20 shell commands, which of course can be scripted. But it seems not a very nice approach. You can instead use an opensource tool called gt (https://github.com/kopasiak/gt) (requires https://github.com/libusbgx/libusbgx), which supports so called gadget schemes: instead of describing creating of your gadget procedurally (explicit shell commands) you describe it declaratively in a configuration file and the tool knows how to parse the file and do all the necessary configfs manipulation. This way modprobe g_ether can be changed to gt load my_ether.scheme, which is a very comparable amount of work :)
A scheme corresponding to the above gadget is like this (let's call it ecm.scheme):
attrs :
{
idVendor = 0x1D6B;
idProduct = 0x104;
};
strings = (
{
lang = 0x409;
manufacturer = "Collabora";
product = "ECM";
}
);
functions :
{
ecm_usb0 :
{
instance = "usb0";
type = "ecm";
};
};
configs = (
{
id = 1;
name = "c";
functions = (
{
name = "ecm.usb0";
function = "ecm_usb0";
} );
} );
Now at the device side you can simply:
gt load ecm.scheme g1 # load the scheme and name the gadget 'g1'
and achieve the same gadget composition as with the above shell commands.
systemd is here. I'm not going to discuss whether it is a good thing or a bad thing. Instead, I want to share with you how you can have systemd compose your gadget, for example at system boot time. A typical example is to have your Ethernet connection over USB up and running and you want that controllable by systemd, so that you for example can systemctl enable/disable your gadget.
The obvious event triggering our gadget creation is the appearance of a UDC in the system. https://github.com/systemd/systemd/issues/11587 points to a pull request, which adds a new udev rule for systemd:
SUBSYSTEM=="udc", ACTION=="add", TAG+="systemd", ENV{SYSTEMD_WANTS}+="usb-gadget.target"
The rule triggers reaching the usb-gadget.target, whose purpose is to mark the point when UDC is available and allow other units depend on it:
[Unit] Description=Hardware activated USB gadget Documentation=man:systemd.special(7)
Now that we have a target to depend on, we can create a service which will be started once the target is reached. Let's call it usb-gadget.service.
[Unit] Description=Load USB gadget scheme Requires=sys-kernel-config.mount After=sys-kernel-config.mount [Service] ExecStart=/bin/gt load ecm.scheme ecm RemainAfterExit=yes ExecStop=/bin/gt rm -rf ecm Type=simple [Install] WantedBy=usb-gadget.target
Such a service can be controlled with systemctl:
systemctl enable usb-gadget.service systemctl disable usb-gadget.service
This way the usb-gadget.target can be reached with or without actually composing the gadget, or a different gadget can be chosen by the system administrator for each purpose.
The selection of USB functions available for composition is quite large (20), but you still might need something else, for example some custom USB protocol. Or, even not so custom (such as e.g. ptp), but a protocol which is not likely to reach upstream kernel. What to do? FunctionFS to the rescue! I will talk about that in the next installment of this post, so stay tuned.
Continue reading (Modern USB gadget on Linux & how to integrate it with systemd - Part 2)…
25/04/2024
While I managed to land support for two extensions, implementing control flow re-convergence in NVK did not go as planned. This is the story…
14/03/2024
In continuation with our series about Kernel Integration we'll go into more detail about how regression detection, processing, and tracking…
21/02/2024
Now included in our Debian images & available via our GitLab, you can build a complete, working BL31 (Boot Loader stage 3.1), and replace…
19/02/2024
Back in 2022, after a series of issues were found in its design, I made the call to rework some of WirePlumber's fundamentals in order to…
08/02/2024
Continuing our Kernel Integration series, we're excited to introduce DRM-CI, a groundbreaking solution that enables developers to test their…
23/01/2024
This is the fourth and final part in a series on persian-rug, a Rust crate for interconnected objects. We've touched on the two big limitations:…
Whether writing a line of code or shaping a longer-term strategic software development plan, we'll help you navigate the ever-evolving world of Open Source.
한국어 버전의 Collabora.com 보기
Acesse Collabora.com em Português
Comments (21)
Diego Sueiro:
Jul 01, 2019 at 07:06 AM
Very interesting article.
I'm trying to force the "dev_addr" and "host_addr" in the gadget scheme but I'm not able to do so.
I added the options (dev_addr = "2e:2d:11:43:db:f7"; host_addr = "72:39:99:c0:06:24";) both after "type = "ecm";" and "function = "ecm_usb0";" lines but no success. When using gt load it allways configures with random MAC addr.
Do you know how can I achieve this?
Reply to this comment
Reply to this comment
Andrzej Pietrasiewicz:
Jul 22, 2019 at 03:54 PM
Hi, you need to put dev_addr and host_addr in the "attrs" section. This is required by libusbgx, which is used internally by gt. Please see https://github.com/libusbgx/libusbgx/blob/master/doc/gadget_schemes.txt for details.
Reply to this comment
Reply to this comment
Sergei:
Apr 20, 2022 at 05:01 PM
Will this schema work if gadgets statically linked with kernel?
Reply to this comment
Reply to this comment
JOHN YANCEY:
Dec 27, 2022 at 08:07 PM
Is it possible to add the hid report_desc to a function declaration using a gadget scheme?
I am looking for the equivalent method of the echo -ne below:
mkdir functions/hid.usb0 # HID keyboard function
echo 1 > functions/hid.usb0/protocol # Keyboard protocol
echo 1 > functions/hid.usb0/subclass # Keyboard device subclass
echo 8 > functions/hid.usb0/report_length # Keyboard HID report length
echo -ne \\x05\\x01\\x09\\x06\\xa1\\x01\\x05\\x07\\x19\\xe0\\x29\\xe7\\x15\\x00\\x25\\x01\\x75\\x01\\x95\\x08\\x81\\x02\\x95\\x01\\x75\\x08\\x81\\x03\\x95\\x05\\x75\\x01\\x05\\x08\\x19\\x01\\x29\\x05\\x91\\x02\\x95\\x01\\x75\\x03\\x91\\x03\\x95\\x06\\x75\\x08\\x15\\x00\\x25\\x65\\x05\\x07\\x19\\x00\\x29\\x65\\x81\\x00\\xc0 > functions/hid.usb0/report_desc
Thank you,
John
Reply to this comment
Reply to this comment
Andrzej Pietrasiewicz:
Jan 11, 2023 at 10:26 AM
As far as I can tell, gadget scheme should support report_desc = [ ... ] kind of syntax.
Reply to this comment
Reply to this comment
JOHN YANCEY:
Jan 13, 2023 at 07:00 PM
Andrzej, Thank you for the reply.
When I try to load my scheme with:
functions :
{
hid_usb0 :
{
instance = "usb0";
type = "hid";
attrs = {
protocol = 1;
subclass = 1;
report_length = 8;
report_desc = [ 0x05, 0x01, 0x09, 0x06, 0xa1, 0x01, 0x05, 0x07, 0x19, 0xe0, 0x29, 0xe7, 0x15, 0x00, 0x25, 0x01, 0x75, 0x01, 0x95, 0x08, 0x81, 0x02, 0x95, 0x01, 0x75, 0x08, 0x81, 0x03, 0x95, 0x05, 0x75, 0x01, 0x05, 0x08, 0x19, 0x01, 0x29, 0x05, 0x91, 0x02, 0x95, 0x01, 0x75, 0x03, 0x91, 0x03, 0x95, 0x06, 0x75, 0x08, 0x15, 0x00, 0x25, 0x65, 0x05, 0x07, 0x19, 0x00, 0x29, 0x65, 0x81, 0x00, 0xc0 ];
}
};
hid_usb1 :
{
instance = "usb1";
type = "hid";
attrs = {
protocol = 2;
subclass = 1;
report_length = 8;
report_desc = [ 0x05, 0x01, 0x09, 0x02, 0xA1, 0x01, 0x09, 0x01, 0xA1, 0x00, 0x05, 0x09, 0x19, 0x01, 0x29, 0x03, 0x15, 0x00, 0x25, 0x01, 0x95, 0x03, 0x75, 0x01, 0x81, 0x02, 0x95, 0x01, 0x75, 0x05, 0x81, 0x01, 0x05, 0x01, 0x09, 0x30, 0x09, 0x31, 0x15, 0x81, 0x25, 0x7F, 0x75, 0x08, 0x95, 0x02, 0x81, 0x06, 0xC0, 0xC0 ];
}
};
};
I get this error:
Error on import gadget
Error: USBG_ERROR_INVALID_TYPE : One of attributes has incompatible type.
If I comment out the two report_desc lines, the gt load succeeds.
Do I have the syntax right?
John
Reply to this comment
Reply to this comment
Wilson:
Oct 25, 2023 at 09:47 AM
HI John,
You can try with this:
attrs = {
protocol = 1;
subclass = 1;
report_length = 8;
report_desc = ( 0x5, 0x1, 0x9, 0x6, 0xA1, 0x1, 0x5, 0x7, 0x19, 0xE0, 0x29, 0xE7, 0x15, 0x0, 0x25, 0x1, 0x75, 0x1, 0x95, 0x8, 0x81, 0x2, 0x95, 0x1, 0x75, 0x8, 0x81, 0x3, 0x95, 0x5, 0x75, 0x1, 0x5, 0x8, 0x19, 0x1, 0x29, 0x5, 0x91, 0x2, 0x95, 0x1, 0x75, 0x3, 0x91, 0x3, 0x95, 0x6, 0x75, 0x8, 0x15, 0x0, 0x25, 0x65, 0x5, 0x7, 0x19, 0x0, 0x29, 0x65, 0x81, 0x0, 0xC0 );
}
Reply to this comment
Reply to this comment
John Yancey:
Mar 30, 2024 at 01:58 PM
Wilson,
Thank you very much! That did it.
John
Reply to this comment
Reply to this comment
sam:
Feb 08, 2023 at 06:01 PM
could you please let me know how to get trigger for usb-gadget.target when UDC is connected ? I am not getting any events and usb-gadget.target is not called
is it already working in systemd ?
Reply to this comment
Reply to this comment
Sergei:
Feb 09, 2023 at 06:55 AM
Device is different beast when we are talking about events. You can check it with "udevadm monitor".
So I check device state using sysfs /sys/class/udc//state
You even can monitor this file with poll event POLLPRI
Reply to this comment
Reply to this comment
sam:
Apr 05, 2023 at 08:10 AM
I am able to get the UDC event ins raspberry pi 3b+ , Here as soon as I add the event in 99-systemd.rule , I get the event and calling the ffs service.
root@raspberrypi:/home/pi# dmesg | grep "usb-gadget\|udc"
[ 29.087630] kobject: 'udc_core' ((ptrval)): kobject_add_internal: parent: 'module', set: 'module'
[ 29.087665] kobject: 'holders' ((ptrval)): kobject_add_internal: parent: 'udc_core', set: ''
[ 29.087897] kobject: 'notes' ((ptrval)): kobject_add_internal: parent: 'udc_core', set: ''
[ 29.087916] kobject: 'udc_core' ((ptrval)): kobject_uevent_env
[ 29.087931] kobject: 'udc_core' ((ptrval)): fill_kobj_path: path = '/module/udc_core'
[ 29.088022] kobject: 'udc' ((ptrval)): kobject_add_internal: parent: 'class', set: 'class'
[ 29.088037] kobject: 'udc' ((ptrval)): kobject_uevent_env
[ 29.088048] kobject: 'udc' ((ptrval)): fill_kobj_path: path = '/class/udc'
[ 29.416434] kobject: 'udc' ((ptrval)): kobject_add_internal: parent: '3f980000.usb', set: '(null)'
[ 29.416447] kobject: '3f980000.usb' ((ptrval)): kobject_add_internal: parent: 'udc', set: 'devices'
[ 29.416558] kobject: '3f980000.usb' ((ptrval)): fill_kobj_path: path = '/devices/platform/soc/3f980000.usb/udc/3f980000.usb'
[ 29.918976] systemd-udevd[138]: seq 1211 queued, 'add' 'udc'
[ 29.961589] systemd-udevd[152]: created db file '/run/udev/data/+udc:3f980000.usb' for '/devices/platform/soc/3f980000.usb/udc/3f980000.usb'
[ 29.975309] systemd[1]: sys-devices-platform-soc-3f980000.usb-udc-3f980000.usb.device: Changed dead -> plugged
[ 29.975378] systemd[1]: usb-gadget.target: Trying to enqueue job usb-gadget.target/start/fail
[ 29.975525] systemd[1]: usb-gadget.target: Installed new job usb-gadget.target/start as 122
[ 29.975567] systemd[1]: usb-gadget.target: Enqueued job usb-gadget.target/start as 122
[ 29.976139] systemd[1]: usb-gadget.target changed dead -> active
[ 29.976173] systemd[1]: usb-gadget.target: Job usb-gadget.target/start finished, result=done
[ 42.444545] systemd[816]: sys-devices-platform-soc-3f980000.usb-udc-3f980000.usb.device: Changed dead -> plugged
Reply to this comment
Reply to this comment
Paul:
Feb 21, 2023 at 10:25 AM
Whenever I try to use either gt or manually mkdir ffs.??? (? isn't an actual path I'm trying) in function/ I always get a device/resource busy message. I can mkdir in function of other device classes, ecm etc. Function fs is enabled in my kernel. Additionally manually making the ffs mountpoint doesn't seem to have any impact. Anyone have any ideas why my mkdir is failing?
thanks for your time.
Reply to this comment
Reply to this comment
sam:
Apr 05, 2023 at 07:58 AM
your question is not clear, please give the exact step you are doing
FYI. if you are making dir in already enabled UDC, it will give resource busy. you need to disable UDC (ehco "" > /sys/kernel/usb_gadget/g1/UDC)first and then
make dir in function
Reply to this comment
Reply to this comment
Terence Darwen:
Jun 15, 2023 at 08:31 PM
Hi, I appreciate the article. It makes it more clear to me how UDC's work in Linux. I'm currently attempting to port an Embedded Linux project that uses UDC so that Linux can emulate a mouse/keyboard when plugged into a laptop or PC.
In my case, I notice ffs_func_setup() in drivers/usb/gadget/function/f_fs.c gets called during device setup. However, I'm not sure how. Do you have any idea under what circumstances this is called? I see it's set as a callback in ffs_alloc() but unsure how exactly it is called (i.e. what exactly calls it).
Any info you might be able to provide would be appreciated.
Reply to this comment
Reply to this comment
Andrzej Pietrasiewicz:
Jun 16, 2023 at 08:59 AM
If you need a keyboard/mouse, why don't you try HID gadget/function? You can easily test with a legacy gadget and once it works you can re-create a similar configuration via ConfigFS.
Reply to this comment
Reply to this comment
Terence Darwen:
Jun 27, 2023 at 03:28 PM
Hi Andrzej - Thanks for the reply. Not sure why, but I didn't get an email reminder of your post until now (6/27/23).
Apologies for my lack of knowledge on this topic, I've just started digging into it. By "HID gadget/function" I take it you mean the USB Hid/gadget driver: https://docs.kernel.org/usb/gadget_hid.html ?
When you say I can easily test with a legacy gadget, do you know of an example somewhere?
Thanks in advance,
Terence
Reply to this comment
Reply to this comment
Andrzej Pietrasiewicz:
Jul 03, 2023 at 09:37 AM
Yes, that. You can optionally use a pre-composed hid gadget drivers/usb/gadget/legacy/hid.c if that helps (instead of composing with configfs). If what you need is a HID device then chances are a HID function is all you need. You only want to use FunctionFS if you want a (completely) custom function which does not conform to any known (and supported by the gadget side) USB class.
Reply to this comment
Reply to this comment
Terence Darwen:
Jul 07, 2023 at 02:16 PM
Okay, this makes sense. I'll check this out. Thanks for the help with this.
Reply to this comment
Reply to this comment
Sagi:
Aug 18, 2023 at 12:20 AM
Hi Andrzej,
I'm trying to set up my Ubuntu 20.04 machine to act like a USB storage when connected via the USB C port on demand.
I saw your article here, and I tried follow it, with some adjustments I read in other places, but I just can't get it working (currently I'm stuck - I have my "/sys/kernel/config/usb_gadget/g1" , but one source claimed I need to add "mass_storage.lun.0" folder there, but I keep getting "cannot create directory ‘/sys/kernel/config/usb_gadget/g1/mass_storage.lun.0/’: Operation not permitted", despite having sudo privileges...)
Before I continue and dig (or damage :) )my machine too much - do you think the above article can help me achieve my goal? Have you ever tried doing what I'm trying to do - can you maybe point me to the right direction?
thanks in advance,
Sagi
Reply to this comment
Reply to this comment
Sagi:
Aug 18, 2023 at 08:31 PM
update - got the permission error to work by using sudo 'bash -c ...', but now, I fail to enable the gadget - when I try to update the UDC file I get bash: line 1: echo: write error: Device or resource busy....
(also, my /sys/class/udc/ is empty...)
still not sure if I'm even on the right path to create OTG flash drive on the PC....
Reply to this comment
Reply to this comment
Andrzej Pietrasiewicz:
Aug 18, 2023 at 09:18 PM
There you go. No UDC in /sys/class/udc, no UDC to bind to. Your system is apparently missing appropriate hardware. If you want to just play with gadgets you can use dummy_hcd. https://www.collabora.com/news-and-blog/blog/2019/06/24/using-dummy-hcd/
Reply to this comment
Reply to this comment
Add a Comment