Allow old IoUSB devices for config.enable.usb #4634

christoph-zededa · 2025-02-20T13:06:59Z

IoUsb should not be used anymore in favor of IoUsbController and IoUsbDevice
but let's still be backwards compatible and make an intelligent guess whether a device or a controller was meant

rene · 2025-02-20T15:43:06Z

@christoph-zededa , although IoUSB is deprecated, is there any use case where IoUSB can still be used? I'm wondering if these changes can break systems using old device models + EVE with IoUsbController?

christoph-zededa · 2025-02-20T15:59:03Z

@christoph-zededa , although IoUSB is deprecated, is there any use case where IoUSB can still be used? I'm wondering if these changes can break systems using old device models + EVE with IoUsbController?

I added be643f3 to remove all remaining uses of types.IoUSB.
It should not break it, but starting the edge app will fail: https://github.com/lf-edge/eve/blob/master/pkg/pillar/cmd/domainmgr/domainmgr.go#L1481

OhmSpectator · 2025-02-21T09:05:23Z

tools/spdx-check.sh

@@ -111,6 +111,10 @@ file_to_be_checked() {
      return 1
    fi
  done
+  if grep -qi "do not edit" "$file";


Which files exactly are you referring to?

Even if a file is generated by a tool, I still think it should contain the header—it's recommended so we know the consequences of using that generating tool. While I agree that we can (and already do) ignore some generated files, we can still add headers to files that come from tools we maintain ourselves.

Could you clarify which specific files you have in mind here?

iotype_string.go

COuld we change it to "Code generated by.*stringer"

OhmSpectator · 2025-02-21T09:12:03Z

pkg/pillar/types/iotype_string.go

@@ -0,0 +1,55 @@
+// Code generated by "stringer --type IoType"; DO NOT EDIT.


Interesting, I didn't know about that tool—I've always handled it manually. While it looks useful, I'm wondering how maintainable it is. If we change an enum, will the tool be run automatically? If not, then we shouldn't really treat the output as entirely auto-generated, and we’d need to maintain it ourselves. If it is run automatically, then we can skip the header.

So, I see two options:

Automate the tool run so we don't need a header, or

Treat the file as manually maintained and add the header.

I'm wondering how maintainable it is

it throws a compile error if one makes incompatible changes

And after that, one has to go and fix it manually. So. it's not rerun automatically. So, it's manually maintained file =)

We also have lots of other auto-generated files checked in.

Like under vendors and API? Or something else?

I didn't see this custom String() function earlier. Doesn't the golang built-in String() for an enum produce the correct output?

Unfortunately not, that's why they created https://pkg.go.dev/golang.org/x/tools/cmd/stringer

Ah - I guess the protoc compiler generates such functions for protobuf enum values.

I thought the stringer example had the stringer tool run at compile time so this does not need to be checked in to github.
Thus a file with only
//go:generate stringer -type=IoType
checked in to github?

@OhmSpectator and I looked on Friday how other projects do this and lots of them check in this file, too.

OhmSpectator · 2025-02-21T09:23:47Z

pkg/pillar/types/assignableadapters.go

+		ioBundle := &aa.IoBundleList[i]
+		ioBundle.Error.removeByType(ErrIOBundleDeprecatedType{})
+
+		if ioBundle.Type == IoUSB {


Where exactly does this type originate? Could it come from the controller or from persistent storage? If it can be stored and then a user updates EVE to a newer version, might they encounter this error? Or is it only ever set by our internal code, making it impossible for an older or external value to appear?

If it can come from an older source, is there any way we could safely handle unknown types — perhaps trying to guess the correct type and logging a warning—rather than breaking the system that was previously running without issues?

It is derived from an eve-api type: PhyIoUSB = 2;

Nobody is reading these warnings ;-) - see also #4634 (comment)

Afaik the commercial users got informed about the change.

Afaik the commercial users got informed about the change.

Unfortunately, it does not prevent them from making mistakes and then requesting fixes from the engineering... We know such cases =(

I must have missed something in the past reviews/discussions and I don't recall us communicating to users that all existing models which published in the controllers (including the eden model files which we can change) need to be changed from IoTypeUsb to IoTypeUsbController.

I thought we introduced IoTypeUsbController as a more clear name, but that existing uses of IoTypeUSb would be interpreted to mean the same thing as IoTypeUsbController. If that is not the case I think we will break existing usage.
Let's set up some time on Monday to talk about this. Perhaps I'm missing something.

christoph-zededa · 2025-02-21T10:48:46Z

Alternatives:

Introduce warnings so that https://github.com/lf-edge/eve/blob/master/pkg/pillar/cmd/domainmgr/domainmgr.go#L1481 will not return an error but nevertheless will set the error and report it to the controller
Try to guess the correct type, i.e. if PciLong is set and Usbproduct and Usbaddr are not set, then it is a IoUsbController, otherwise it is a IoUsbDevice. But I am not sure if that is 100% correct.

OhmSpectator · 2025-02-21T10:58:20Z

I would like @eriknordmark to share his experience in handling such situations. I don't think it's the first time we have to introduce a potentially breaking change.

OhmSpectator · 2025-02-21T10:59:13Z

@christoph-zededa, in general, how did you come to the idea of this change? Is it related to some issue?

christoph-zededa · 2025-02-21T11:13:15Z

@christoph-zededa, in general, how did you come to the idea of this change? Is it related to some issue?

It is because of two issues I saw this week:

Somebody had an edge application that expects IoUsb and another one that expects IoUsbDevice and they were wondering what they should do in the model manifest to be able to run both.
[12.0-stable] Revert "domainmgr: do pci-reserve on start of edge app" #4633 - and they were using IoUsb and were wondering why the behavior is different on EVE 12.x vs 13.x and I think with IoUsbController the behavior would not have been different - see https://github.com/lf-edge/eve/blob/master/pkg/pillar/cmd/domainmgr/domainmgr.go#L1399

that is either IoUSBController or IoUSBDevice Signed-off-by: Christoph Ostarek <[email protected]>

to hopefully fix flaky test in the future Signed-off-by: Christoph Ostarek <[email protected]>

some older model manifests might still be using IoUSB; this code also accepts this type when checking for USB controllers that should only do the PCI reservation when starting of an edge app, because the user might set config.enable.usb to use a local keyboard Signed-off-by: Christoph Ostarek <[email protected]>

rene

LGTM

eriknordmark · 2025-02-25T09:07:54Z

pkg/pillar/hypervisor/xen.go

@@ -413,7 +413,7 @@ func (ctx xenContext) CreateDomConfig(domainName string,
 			short := types.PCILongToShort(pa.pciLong)
 			// USB controller are subject to legacy USB support from
 			// some BIOS. Use relaxed to get past that.
-			if pa.ioType == types.IoUSB {
+			if pa.ioType == types.IoUSBController {


Should this also use IsUSBController()?

Yes!
But unfortunately we had there only pciDevice which seems to become more and more an IoBundle, so I just embedded it instead: 86a12b9

eriknordmark · 2025-02-25T09:09:59Z

also make spdx checker ignore 'do-not-edit' files.

This seems to have been removed. If that was the intent please update the PR description.

pciDevice was about to become more and more a copy of types.IoBundle, instead remove all copied files and add a copy to the original types.IoBundle This also enables in the next commit to use IoBundle.IsUSBController() Signed-off-by: Christoph Ostarek <[email protected]>

this one checks for IoUSBController and IoUSB and works better with deprecated models that still use the old type Signed-off-by: Christoph Ostarek <[email protected]>

OhmSpectator · 2025-02-27T14:57:15Z

@christoph-zededa, did you manage to resolve all of Erik's doubts? =)

christoph-zededa · 2025-02-27T15:03:44Z

@christoph-zededa, did you manage to resolve all of Erik's doubts? =)

IMO on EVE side, yes.

OhmSpectator · 2025-02-27T15:05:45Z

OK, so it's ready for a new round of reviews... Got it.

github-actions bot requested review from deitch, eriknordmark, jsfakian, milan-zededa, OhmSpectator and uncleDecart February 20, 2025 13:07

christoph-zededa force-pushed the check_deprecated_iobundle_type branch 2 times, most recently from 49e5d68 to 8cc307e Compare February 20, 2025 13:18

christoph-zededa marked this pull request as ready for review February 20, 2025 14:00

christoph-zededa requested review from rene, rouming and shjala as code owners February 20, 2025 15:53

github-actions bot requested a review from rucoder February 20, 2025 15:54

OhmSpectator reviewed Feb 21, 2025

View reviewed changes

christoph-zededa force-pushed the check_deprecated_iobundle_type branch from dedad09 to 4691a87 Compare February 21, 2025 17:37

github-actions bot requested a review from OhmSpectator February 21, 2025 17:37

christoph-zededa added 2 commits February 24, 2025 18:49

pillar: replace IoUSB device type with correct one

1944f72

that is either IoUSBController or IoUSBDevice Signed-off-by: Christoph Ostarek <[email protected]>

pillar/watcher: enhance test error output

a6de54b

to hopefully fix flaky test in the future Signed-off-by: Christoph Ostarek <[email protected]>

christoph-zededa force-pushed the check_deprecated_iobundle_type branch from 4691a87 to 05bba0a Compare February 24, 2025 17:49

christoph-zededa force-pushed the check_deprecated_iobundle_type branch from 05bba0a to 63dd102 Compare February 24, 2025 17:50

christoph-zededa changed the title ~~Check deprecated iobundle type~~ Allow old IoUSB devices for config.enable.usb Feb 24, 2025

rene approved these changes Feb 25, 2025

View reviewed changes

eriknordmark reviewed Feb 25, 2025

View reviewed changes

github-actions bot requested review from eriknordmark and rene February 25, 2025 10:31

christoph-zededa added 2 commits February 25, 2025 11:45

pillar/xen: use better indicator for usb controlle

7c58026

this one checks for IoUSBController and IoUSB and works better with deprecated models that still use the old type Signed-off-by: Christoph Ostarek <[email protected]>

christoph-zededa force-pushed the check_deprecated_iobundle_type branch from 1194c7c to 7c58026 Compare February 25, 2025 10:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow old IoUSB devices for config.enable.usb #4634

Allow old IoUSB devices for config.enable.usb #4634

christoph-zededa commented Feb 20, 2025 •

edited

Loading

rene commented Feb 20, 2025

christoph-zededa commented Feb 20, 2025

OhmSpectator Feb 21, 2025

christoph-zededa Feb 21, 2025

OhmSpectator Feb 21, 2025

christoph-zededa Feb 21, 2025

OhmSpectator Feb 21, 2025

christoph-zededa Feb 21, 2025

OhmSpectator Feb 21, 2025

christoph-zededa Feb 21, 2025

OhmSpectator Feb 21, 2025

eriknordmark Feb 21, 2025

christoph-zededa Feb 21, 2025

eriknordmark Feb 24, 2025

eriknordmark Feb 24, 2025

christoph-zededa Feb 24, 2025

OhmSpectator Feb 21, 2025

christoph-zededa Feb 21, 2025

christoph-zededa Feb 21, 2025 •

edited

Loading

OhmSpectator Feb 21, 2025

eriknordmark Feb 21, 2025

christoph-zededa commented Feb 21, 2025

OhmSpectator commented Feb 21, 2025

OhmSpectator commented Feb 21, 2025

christoph-zededa commented Feb 21, 2025

rene left a comment

eriknordmark Feb 25, 2025

christoph-zededa Feb 25, 2025

eriknordmark commented Feb 25, 2025

OhmSpectator commented Feb 27, 2025

christoph-zededa commented Feb 27, 2025

OhmSpectator commented Feb 27, 2025

		@@ -0,0 +1,55 @@
		// Code generated by "stringer --type IoType"; DO NOT EDIT.

Allow old IoUSB devices for config.enable.usb #4634

Are you sure you want to change the base?

Allow old IoUSB devices for config.enable.usb #4634

Conversation

christoph-zededa commented Feb 20, 2025 • edited Loading

rene commented Feb 20, 2025

christoph-zededa commented Feb 20, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christoph-zededa Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christoph-zededa commented Feb 21, 2025

OhmSpectator commented Feb 21, 2025

OhmSpectator commented Feb 21, 2025

christoph-zededa commented Feb 21, 2025

rene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknordmark commented Feb 25, 2025

OhmSpectator commented Feb 27, 2025

christoph-zededa commented Feb 27, 2025

OhmSpectator commented Feb 27, 2025

christoph-zededa commented Feb 20, 2025 •

edited

Loading

christoph-zededa Feb 21, 2025 •

edited

Loading