valid_fw_version always true

[windwardly]

In the code fragment below, msbyte != 0x0 || msbyte != 0x6 || (anything) is always true, so it looks like the if statement below could be reduced to valid_fw_version.push(true);.

Is that the intention?
What is in msbyte? If I ripgrep the luwen repo looking for masked_version, I see its initial condition as fw_version & 0x00FFFFFF; and not coming across other writers. I then see below let eth_fw_version = self.eth_addrs.masked_version; let msbyte = (eth_fw_version >> 24) & 0xFF; and read that as zero out the top 8 bits, then compute the top 8 bits. Looks like masked_version was introduced by commit 53ab480 and that it has always been initialized to fw_version & 0x00FFFFFF;. What am I missing? I'm digging into all this because tt-inference-server won't start for me because something about sw and fw version. Yes, that's from tt-umd. I was looking here to get more understanding about what fw_version is.

This is from crates/luwen-if/src/chip/remote.rs as of commit ae98025

    pub(crate) fn check_ethernet_fw_version(&mut self) -> Result<Vec<bool>, PlatformError> {
        let mut valid_fw_version = Vec::with_capacity(self.eth_locations.len());
        for core in &self.eth_locations {
            let eth_fw_version = self.eth_addrs.masked_version;
            let msbyte = (eth_fw_version >> 24) & 0xFF;
            if msbyte != 0x0
                || msbyte != 0x6
                || self.noc_read32(0, core.x, core.y, self.eth_addrs.version)? & 0x00FFFFFF
                    != eth_fw_version
            {
                valid_fw_version.push(true);
            } else {
                valid_fw_version.push(false);
            }
        }

[smehtaTT]

@dkruczekTT to route to Luwen lead

[kaplanz]

I'm taking a look at the (only) caller of check_ethernet_fw_version, and it doesn't actually check the value of the Ok(valid_fw_version) in the result:

luwen/crates/luwen-if/src/chip/wormhole.rs

Lines 777 to 783 in 4528090

	if let Err(_err) = self.check_ethernet_fw_version() {
	*eth_status = WaitStatus::NotInitialized(
	EthernetPartialInitError::FwOverwritten,
	);
	} else {
	*eth_status = WaitStatus::JustFinished;
	}

I agree that msbyte should always be 0x00, so mbyte != 06 is always true, and therefore self.noc_read32 is short-circuited and never called. This actually means that the Err variant cannot be returned from check_ethernet_fw_version, so the code snippet above is equivalent to:

*eth_status = WaitStatus::JustFinished;

[warthog9]

Actually tagging this back at @smehtaTT - there's two things here. There's potentially a gotcha in luwen here as reported, but that would only affect tools, and wouldn't have any direct impact on inference server or umd. There's also the original issue of inference server and umd and fw and sw version miss-match. If inference server has a separate bug on that front I'll leave this to luwen to cover the potential gotcha (and double check with firmware) otherwise I don't want to lose that other portion of the bug if we close this as "not a bug" per-se

[windwardly]

[..] There's also the original issue of inference server and umd and fw and sw version miss-match. If inference server has a separate bug on that front I'll leave this to luwen to cover the potential gotcha (and double check with firmware) otherwise I don't want to lose that other portion of the bug if we close this as "not a bug" per-se

@warthog9, tt-umd just had a commit pushed to main, plus one other working its way through the system that I have been tracking. The bugs there have and will be fixed.

[kaplanz]

After asking around, it seems the original intention of this code was something like:

if (msbyte not in (0, 6)) or (noc_read(... fw_version) & 0x00ffffff != eth_fw_version):
    # throw an error 

Note that in this case, we'll always execute the noc_read for the firmware version (due to msbyte always being 0), however, this results in the entire firmware version number being checked.

Edit: It seems the bug here was that the first || should be an && that is evaluated separately from the noc_read:

(msbyte != 0x0 && msbyte != 0x6) || (/* noc_read */)

[kaplanz]

From a quick test, this doesn't seem to break anything on my Wormhole card, although this could be a silent failure due to check_ethernet_fw_version still not checking the Ok result.