usb: dfu: rework of dfu class implementation #30562

mrrosen · 2020-12-09T18:16:01Z

To improve the usability and flexibility of the USB DFU class, some fixes and changes were implemented:

Added separate Kconfig variables for idVendor and idProduct when running in DFU mode as required by USB DFU 1.1 specification (Ch 2) since keeping the same values for these fields can lead to host OS caching of descriptors and not properly updating the configuration descriptor once in DFU mode.
Leverage Zephyr's devicetree-defined flash partitions to also define the alternates for USB DFU, allowing for flashing of images and data to more than just the image-1 used by mcuboot, making the class much more dynamic. It also enforces the read-only property of the partitions to prevent writing to partitions that arent supposed to be written to (this is actually a more general problem in the flash APIs right now ignoring this property)
Fix a few other minor issues, adding timeout timer for appDETACH, moved the time when the descriptor table changes in DFU mode, etc.

Minor refactor of USB DFU class implementation, including: . Support for different idVendor and idProduct than during runtime as required by the USB DFU 1.1 specification . Support for all partitions to be available as alternates . Change to DFU descriptors only after USB Reset . Timeout timer added to appDETACH state as required by USB DFU 1.1 specification Signed-off-by: Michael Rosen <[email protected]>

jfischer-no

Please separate it into smaller self-contained commits, describe the reason for the changes in the commits message. Please do not make style fixes.

jfischer-no · 2020-12-09T21:47:01Z

subsys/usb/class/dfu/CMakeLists.txt

+ endif()
+endif()
+
+if(CONFIG_USB_DEVICE_DFU_VID EQUAL 0x2FE3)


NACK for CONFIG_USB_DEVICE_DFU_VID, this is no required and not "considered necessary"

jfischer-no · 2020-12-09T21:49:04Z

subsys/usb/class/dfu/CMakeLists.txt

+This value is only for testing and MUST be configured for USB products."
+ )
+
+ if(CONFIG_USB_DEVICE_DFU_PID EQUAL 0x101)


Also NACK for CONFIG_USB_DEVICE_DFU_PID, "considered necessary" is not a must. Which OS did you observe the problems with?

Windows 10, dfu-util 0.9

The exact behavior is that the tool cannot find the device in DFU mode after USB reset because of OS-level caching of descriptors (from that Ch2 of the specification, its actually considered "necessary" to change the VID/PID). If you try again, the tool is able to preform DFU operations since the device is doing the correct thing.

jfischer-no · 2020-12-09T21:52:01Z

subsys/usb/class/dfu/Kconfig

+
+config USB_DEVICE_DFU_PID
+ hex "USB DFU Product ID"
+ default 0x0101


You can not assign 0x0101 here. There is a list of assigned PID, samples/subsys/usb/usb_pid.Kconfig. However, I do not think this is necessary at all.

Default PID is assigned in subsys/usb/Kconfig, thats what this is based off of. While a different PID between runtime mode and DFU mode is required by the specification and does have issues on Windows 10 (see other comments), we can have a single VID. I was just providing for maximum flexibility by creating new Kconfig variables for DFU mode descriptors.

The one from subsys/usb/Kconfig is leftover before samples/subsys/usb/usb_pid.Kconfig was inserted. Please assign the value 0xffff for now.

jfischer-no · 2020-12-09T21:56:29Z

subsys/usb/class/dfu/usb_dfu.c

 struct usb_string_desription {
 struct usb_string_descriptor lang_descr;
 struct usb_mfr_descriptor {
 uint8_t bLength;
 uint8_t bDescriptorType;
 uint8_t bString[USB_BSTRING_LENGTH(
- CONFIG_USB_DEVICE_MANUFACTURER)];


Please do not make style fixes along with other changes. These were not necessary here either.

jfischer-no · 2020-12-09T22:03:52Z

subsys/usb/class/dfu/usb_dfu.c

+ DT_FOREACH_CHILD(DT_DRV_INST(n), READONLY)
+
+/**
+ * @brief Helper function to check if given partition is read-only


I see no benefit in using Doxygen for internal functions.

Some (not all) internal helper functions in this file had Doxygen comments, should they be added for new helper functions then or not?

Please not for internal functions, I do not know why people do it.

jfischer-no · 2020-12-09T22:42:51Z

subsys/usb/class/dfu/usb_dfu.c

+#define DT_EXPAND_CAT(a, b) DT_CAT(a, b)
+
+#define DEFINE_IF(part) \
+ struct usb_if_descriptor DT_EXPAND_CAT(if, DT_FIXED_PARTITION_ID(part));


Convert to using DT should be done in separate commit (b). However why must all partitions (mcuboot?, image-0, image-1, image-scratch?, storage?) be exposed? I can not think of a good reason for this. This should remain limited for image-0 or image-0 + image-1.

mcuboot is probably never needed, however, its possible any number of additional partitions for file systems and other storage might want to be accessed or preflashed along with an OS image. Thats the motivation behind this change; and exposing all partitions is a simpler solution than trying to figure out which a user would want to expose. An alternative would be to add to device tree to add a flag to determine which partitions want to be exposed, but I didnt adding all that complexity was necessary.

Well this implementation depends on image manager and mcuboot. Not only is it not necessary to expose all partitions, it is also confusing for the user and probably not desired by the developer.

The problem with generating descriptors for all partitions is that it bloats binary with code that will never be accessed in most cases, for example when user only wants to dfu the image-1 or image-2.

You may, for example, be out of flash on mcuboot partition, when trying to link DFU to mcuboot.

True that it does some additional data to the image that would not be used (the changes to the actual code arent significant in terms of code size). However, the additional descriptors really dont cost much memory (9 bytes for interface descriptor, 2 + 2 * strlen(partition-name) bytes for the string descriptors; so assuming a generous 5 extra partitions not used at all by DFU and a length of 15 characters, thats 5*(9 + 2 + 2 * 15) = 205 bytes, which is certainly not zero but compared to whats being added to the image when enabling USB and whats usually required for the image to use mcuboot with USB (32-64KiB) its not a very significant change. If this is a major concern, I would instead propose adding to device tree a marker for which partitions are desired to be accessed over DFU instead of hard limiting it to just image-1 or image-0 in single slot mode.
I agree there are some dependencies on the image manager than I did not entirely remove. Since there is alot of discussion around this topic; I think it would be better to split this PR between the parts fixing issues and the parts implementing the partitions rework (I will split up the commits as @jfischer-no suggested), or is it preferred each fix/commit be a separate PR?

Yes, do separate commits.
Maybe we should add 'upgreadable' property to elements of 'fixed-partitions' and generate the descriptors only for these partitions that are marked as upgredable, dfuable or whatever property there will be.
The ~200 does not seem much but the space in mcuboot partitions starts to get tight and if we can shave something off easily, why not do it?

jfischer-no · 2020-12-09T22:44:45Z

subsys/usb/class/dfu/usb_dfu.c

@@ -368,7 +393,11 @@ static void dfu_flash_write(uint8_t *data, size_t len)
 LOG_DBG("flash write done");
 dfu_data.state = dfuMANIFEST_SYNC;
 dfu_reset_counters();
- if (boot_request_upgrade(false)) {
+ /* TODO: Reexamine if there is a nicer way of handling mcuboot


Do TODO or remove this comment.

jfischer-no · 2020-12-09T22:50:54Z

subsys/usb/class/dfu/usb_dfu.c

@@ -533,18 +568,10 @@ static int dfu_class_handle_req(struct usb_setup_packet *pSetup,
 }

 if (len) {
- const struct flash_area *fa;


This change should be done in separate commit (c)

jfischer-no · 2020-12-09T22:52:05Z

subsys/usb/class/dfu/usb_dfu.c

- return -EIO;
- }
+ /* Begin detach timeout timer */
+ timeout = MIN(pSetup->wValue, CONFIG_USB_DFU_DETACH_TIMEOUT);


Should be done in separate commit (d)

jfischer-no · 2020-12-09T22:53:29Z

subsys/usb/class/dfu/usb_dfu.c

 if (dfu_data.state == appDETACH) {
 dfu_data.state = dfuIDLE;
+
+ /* Set the DFU mode descriptors to be used after reset */
+ dfu_config.usb_device_description =


This change should be done in separate commit (e)

nvlsianpu

I blocked merge until I understood what is changing here.

de-nordic · 2020-12-10T11:28:52Z

subsys/usb/class/dfu/usb_dfu.c

+#define READONLY(part) \
+ [DT_FIXED_PARTITION_ID(part)] = DT_PROP(part, read_only),
+
+#define READONLY_FOREACH_PARTITION(n) \
+ DT_FOREACH_CHILD(DT_DRV_INST(n), READONLY)
+


Unless the read-only property is really implemented, I see no reason for these to be here.
The idea is OK, but if it is TODO, it should get separate PR with proposed implementation.

de-nordic

I think that it current state, this should be RFC.

nvlsianpu · 2020-12-10T12:09:06Z

subsys/usb/class/dfu/usb_dfu.c

+ /* TODO: Reexamine if there is a nicer way of handling mcuboot
+ * interface
+ */
+ if (dfu_data.fa->fa_id == FLASH_AREA_ID(image_1) &&


What is user-case for this change? Currently boot_request_upgrade() is no-op if image_1 dosen't exist (so USB DFU is build in the MCUBoot for this case). It is the operation for dual-bank mode. Dual-XIP support is in progress - but mean also no-op for this function.

nvlsianpu · 2020-12-10T12:28:59Z

subsys/usb/class/dfu/usb_dfu.c

 dfu_data.status = errWRITE;
 dfu_data.state = dfuERROR;
- LOG_ERR("This area can not be overwritten");
+ LOG_ERR("DFU_DNLOAD area is readonly");


typo DFU_DNLOAD -> DFU_DOWNLOAD

All the other print messages use DFU_DNLOAD, should they all be DF_DOWNLOAD?

DFU_DNLOAD is a DFU class request. Maybe:

Suggested change

LOG_ERR("DFU_DNLOAD area is readonly");

LOG_ERR("DFU_DNLOAD write area is readonly");

subsys/usb/class/dfu/usb_dfu.c

nvlsianpu · 2020-12-10T12:55:27Z

This is not a minor rework.

mrrosen · 2020-12-10T16:04:48Z

As mentioned above, there is a lot more discussion that needs to happen around the partition mapping; however there are a number of minor fixes also in this PR. So, I think its best if we split the PR between those actually minor fixes and the move to using the device tree partition map for USB DFU. As also mentioned, is it preferred by the code owners if those fixes are each a small PR or one PR?

mrrosen · 2020-12-10T19:06:48Z

All the other fixes have been moved to their own PR: #30611
How best should be handle the discussion in this thread for opening up DFU to more partitions? Continue this PR, open a new PR, open an issue for discussion?

nvlsianpu · 2020-12-11T11:01:50Z

My opinion is that rest can be continued here.

mrrosen · 2021-02-05T20:46:31Z

After a bit of thought on this (and coming back to it), I think there are two ways to go about adding the additional functionality while providing the control needed to minimize the number of partitions exposed, both leveraging devicetree:

Add to the partition table a few new labels to the partitions themselves (like what @de-nordic suggested) and have something like dfu-access with values "readonly" (can only be uploaded, like "image-0" now), "readwrite" (can be uploaded/downloaded, like "image-1" now), "hidden" (isnt accessible via dfu at all, like everything else now, so would be the default). To accommodate downloads to partitions that arent images (for example, loading prebuilt filesystems), another label should be added to handle what should happen during manifest, for mcuboot images, this is a call to boot_request_upgrade, so something like "dfu-manifest-action" with values "none" (default), "mcuboot", and allowing for expansion later if other post-processing steps are needed for other bootloaders/partitions.

partitions {
		compatible = "fixed-partitions";
		#address-cells = <1>;
		#size-cells = <1>;

		boot_partition: partition@0 {
			label = "mcuboot";
			reg = <0x00000000 0xc000>;
		};
		slot0_partition: partition@c000 {
			label = "image-0";
			reg = <0x0000C000 0x32000>;
			dfu-access = "readonly";
		};
		slot1_partition: partition@3e000 {
			label = "image-1";
			reg = <0x0003E000 0x32000>;
			dfu-access = "readwrite";
			dfu-manifest-action = "mcuboot";
		};
		scratch_partition: partition@70000 {
			label = "image-scratch";
			reg = <0x00070000 0xa000>;
		};
		storage_partition: partition@7a000 {
			label = "storage";
			reg = <0x0007a000 0x00006000>;
			dfu-access = "readwrite";
		};
	};

Instead of adding these to the partition table device tree, instead do something like what the USB audio class does and have the USB DFU class have its own set of device tress configurations settings as a child of the USB controller (maybe all USB classes should be this way, so descriptor tables/class configuration come more from device tree instead of all from Kconfig?). In this system, there are similar tags to the partitions but is instead but in one place associated with the DFU class and not mixed in with the partition table:

&usbd {
	dfu {
		label = "usb-dfu";
		compatible = "usb-dfu";

		alt_image_0 {
			partition = &slot0_partition;
			read-only;
		};
		alt_image_1 {
			partition = &slot1_partition;
			manifest-action = "mcuboot";
		};
		alt_storage {
			partition = &storage_partition;
		};
	};
};

jfischer-no · 2021-02-05T21:42:01Z

Instead of adding these to the partition table device tree, instead do something like what the USB audio class does and have the USB DFU class have its own set of device tress configurations settings as a child of the USB controller (maybe all USB classes should be this way, so descriptor tables/class configuration come more from device tree instead of all from Kconfig?).

No, I do not think it is a good idea. DT for audio is temporary solution and please do not use it as an example. And we do not obtain "descriptor tables" from from Kconfig.
I think in the opposite direction. Maybe it should not come preconfigured from the USB DFU implementation which partition is exposed, but the application (developer/user) should determine which partitions should be used. Before the USB start (usb_enable()) the partitions would be registered in the USB DFU core, possibly with appropriate flags (R|RW). But that would consume a little more flash.

mrrosen · 2021-02-05T21:57:20Z

@jfischer-no Do you mean for applications to each make their own version of the secondary descriptor table and set a pointer for the dfu implementation to use at runtime? With helper macros that would certainly be one way to do it, but now any application wanting to use DFU would have to include this macro (unless there is a default one in the DFU implementation folder).

mrrosen · 2021-02-09T17:01:09Z

@jfischer-no If its supposed to be user configurable (in code rather than DTS), it gets a bit messy to do while still ensuring the descriptors end up in the readonly section and dont consume RAM; since the descriptors need to change based on what partitions the user selects. As I mentioned above, we can generate them all using macros (not complete):

typedef void (*dfu_manifest_action_t)(uint8_t);
struct _dfu_partition {
  uint8_t area;
  uint8_t perm;
  dfu_manifest_action_t manifest_action;
};

#define USB_DFU_PARTITION_LIST0(v) {.area = FLASH_AREA_ID(v),
#define USB_DFU_PARTITION_LIST1(v) .perm = (v),
#define USB_DFU_PARTITION_LIST2(v) .manifest_action = (v)},
#define USB_DFU_PARTITION_LIST3(V) USB_DFU_PARTITION_LIST0(v)
#define USB_DFU_PARTITION_LIST4(v) USB_DFU_PARTITION_LIST1(v)
...
#define USB_DFU_PARTITION_LIST(i, v) UTIL_CAT(USB_DFU_PARTITION_LIST, i)(v)

#define USB_DFU_DESCRIPTOR_DEF0(v) struct usb_if_descriptor if0;
#define USB_DFU_DESCRIPTOR_DEF1(v) EMPTY
#define USB_DFU_DESCRIPTOR_DEF2(v) EMPTY
#define USB_DFU_DESCRIPTOR_DEF3(v) struct usb_if_descriptor if1;
...
#define USB_DFU_DESCRIPTOR_DEF(i, v) UTIL_CAT(USB_DFU_DESCRIPTOR_DEF, i)(v)

#define USB_DFU_DESCRIPTOR_INIT0(v) .if0 = { \
			.bLength = sizeof(struct usb_if_descriptor), \
			.bDescriptorType = USB_INTERFACE_DESC, \
			.bInterfaceNumber = 0, \
			.bAlternateSetting = 0, \
			.bNumEndpoints = 0, \
			.bInterfaceClass = DFU_DEVICE_CLASS, \
			.bInterfaceSubClass = DFU_SUBCLASS, \
			.bInterfaceProtocol = DFU_MODE_PROTOCOL, \
			.iInterface = 4, \
		},
#define USB_DFU_DESCRIPTOR_INIT1(v) EMPTY
#define USB_DFU_DESCRIPTOR_INIT2(v) EMPTY
#define USB_DFU_DESCRIPTOR_INIT3(v) .if1 = { \
			.bLength = sizeof(struct usb_if_descriptor), \
			.bDescriptorType = USB_INTERFACE_DESC, \
			.bInterfaceNumber = 0, \
			.bAlternateSetting = 1, \
			.bNumEndpoints = 0, \
			.bInterfaceClass = DFU_DEVICE_CLASS, \
			.bInterfaceSubClass = DFU_SUBCLASS, \
			.bInterfaceProtocol = DFU_MODE_PROTOCOL, \
			.iInterface = 5, \
		},
...
#define USB_DFU_DESCRIPTOR_INIT(i, v) UTIL_CAT(USB_DFU_DESCRIPTOR_INIT, i)(v)

#define USB_DFU_PARTITIONS(...) \
  struct _dfu_partition _dfu_partitions[] = { \
    FOR_EACH_IDX(USB_DFU_PARTITION_LIST, (,), LIST_DROP_EMPTY(__VA_ARGS__)) \
  }; \
  struct struct dev_dfu_mode_descriptor { \
    struct usb_device_descriptor device_descriptor; \
	  struct usb_cfg_descriptor cfg_descr; \
	  struct usb_sec_dfu_config { \
      FOR_EACH_IDX(USB_DFU_DESCRIPTOR_DEF, (;), LIST_DROP_EMPTY(__VA_ARGS__)) \
      struct dfu_runtime_descriptor dfu_descr; \
    } __packed sec_dfu_cfg; \
  } __packed; \
  USBD_DEVICE_DESCR_DEFINE(secondary) \
  struct dev_dfu_mode_descriptor dfu_mode_desc = { \
    .device_descriptor = { \
      ... \
    }, \
    .cfg_descr = { \
      ... \
    }, \
    .sec_dfu_cfg = {
      FOR_EACH_IDX(USB_DFU_DESCRIPTOR_INIT, (,), LIST_DROP_EMPTY(__VA_ARGS__)) \
      .dfu_descr = { \
        ... \
      }, \
    }, \
  }; \
  ... // Same for string descriptors
  
USB_DFU_PARTITIONS(image_0, DFU_READ, NULL,
                   image_1, DFU_READ | DFU_WRITE, DFU_MANIFEST_MCUBOOT,
                   storage, DFU_READ | DFU_WRITE, NULL);

Another thing to consider with this solution is that any errors generated from a bad partition name or other mistake might be difficult to decipher. But without moving the descriptor into RAM, Im not sure of a better way to handle this. There might be better ways of using the FOR_EACH and other macros to make this cleaner but I couldnt come up with a different way.

Alternatively, a custom linker section/scripts could be used to setup the descriptors in memory from many different definitions like how device initialization works (I havent looked into this approach in detail).

jfischer-no · 2021-02-09T18:29:09Z

@jfischer-no If its supposed to be user configurable (in code rather than DTS), it gets a bit messy to do while still ensuring the descriptors end up in the readonly section and dont consume RAM; since the descriptors need to change based on what partitions the user selects. As I mentioned above, we can generate them all using macros (not complete):

Generally, all descriptors are placed in RAM because they are changed at runtime. This is how the stack works. Excuse me, but I do not see any urgent need to change anything in DFU implementation at the moment, especially to add new features, or change how we handle descriptors in current USB stack just because of new feature for DFU. I am in the process of reworking the USB device stack. The handling of the descriptors will change and the application will have more control over them, after that also the USB DFU implementation will be reworked and more abstracted.

mrrosen · 2021-02-09T18:52:47Z

@jfischer-no I didnt notice the descriptors were in RAM, so I apologize for that misunderstanding. The only urgency here was that this is a feature I need for my project (expanded access to other partitions via DFU), and thought it would be a nice feature to upstream in a generic way to enable anyone else who might need it (as well as fix the issues we found when trying to use DFU on Windows that were part of the original PR).

I can just use my original technique for my project and we can look at expanding this functionality with the new device stack later; so can close this PR.

jfischer-no · 2021-02-09T19:36:23Z

The only urgency here was that this is a feature I need for my project (expanded access to other partitions via DFU), and thought it would be a nice feature to upstream in a generic way to enable anyone else who might need it (as well as fix the issues we found when trying to use DFU on Windows that were part of the original PR).

@mrrosen Could you please open a feature request so that it does not get lost?
Thanks for the patch which fixed the problem on Windows OS.

mrrosen · 2021-02-09T19:43:41Z

The only urgency here was that this is a feature I need for my project (expanded access to other partitions via DFU), and thought it would be a nice feature to upstream in a generic way to enable anyone else who might need it (as well as fix the issues we found when trying to use DFU on Windows that were part of the original PR).

@mrrosen Could you please open a feature request so that it does not get lost?
Thanks for the patch which fixed the problem on Windows OS.

#32135

This feature can be now tracked here instead.

mrrosen requested review from finikorg and jfischer-no as code owners December 9, 2020 18:16

mrrosen force-pushed the usb-dfu-partitions branch 5 times, most recently from b88eab0 to 9b33788 Compare December 9, 2020 19:43

mrrosen force-pushed the usb-dfu-partitions branch from 9b33788 to 6a2cff8 Compare December 9, 2020 19:46

jfischer-no requested changes Dec 9, 2020

View reviewed changes

jfischer-no requested review from nvlsianpu and de-nordic December 9, 2020 22:58

jfischer-no added area: DFU Device Firmware Upgrade area: USB Universal Serial Bus labels Dec 9, 2020

nvlsianpu requested changes Dec 10, 2020

View reviewed changes

de-nordic reviewed Dec 10, 2020

View reviewed changes

de-nordic requested changes Dec 10, 2020

View reviewed changes

nvlsianpu reviewed Dec 10, 2020

View reviewed changes

nvlsianpu mentioned this pull request Dec 10, 2020

flash_map: Add a flash_area_?() API for check read-only property #30587

Open

nvlsianpu reviewed Dec 10, 2020

View reviewed changes

subsys/usb/class/dfu/usb_dfu.c Show resolved Hide resolved

nvlsianpu changed the title ~~usb: dfu: minor rework of dfu class implementation~~ usb: dfu: rework of dfu class implementation Dec 10, 2020

mrrosen mentioned this pull request Dec 10, 2020

usb: dfu: refactor dfu class implementation #30611

Merged

zephyrbot requested a review from jfischer-no January 8, 2021 21:21

zephyrbot assigned jfischer-no Jan 8, 2021

mrrosen closed this Feb 9, 2021

mrrosen mentioned this pull request Feb 9, 2021

Expand USB DFU to include more partitions #32135

Open

de-nordic mentioned this pull request Feb 12, 2021

Common DFU partition enumeration API #32257

Closed

	LOG_ERR("DFU_DNLOAD area is readonly");
	LOG_ERR("DFU_DNLOAD write area is readonly");

usb: dfu: rework of dfu class implementation #30562

usb: dfu: rework of dfu class implementation #30562

Conversation

mrrosen commented Dec 9, 2020

jfischer-no left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrrosen Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvlsianpu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

de-nordic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvlsianpu commented Dec 10, 2020

mrrosen commented Dec 10, 2020

mrrosen commented Dec 10, 2020

nvlsianpu commented Dec 11, 2020

mrrosen commented Feb 5, 2021 • edited Loading

jfischer-no commented Feb 5, 2021

mrrosen commented Feb 5, 2021

mrrosen commented Feb 9, 2021

jfischer-no commented Feb 9, 2021

mrrosen commented Feb 9, 2021

jfischer-no commented Feb 9, 2021

mrrosen commented Feb 9, 2021

mrrosen Dec 9, 2020 •

edited

Loading

mrrosen commented Feb 5, 2021 •

edited

Loading