Skip to content

Validate EEPROM after writing it #316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JohnAZoidberg wants to merge 2 commits into
base: main
Choose a base branch
from
+70 −2
Open

Validate EEPROM after writing it #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44f1501
Validate EEPROM after writing it
JohnAZoidberg Apr 14, 2026
c46e7e4
Add --validate-gpu-descriptor command
JohnAZoidberg Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions framework_lib/src/chromium_ec/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1526,6 +1526,24 @@ impl CrosEc {
return_val
}

pub fn validate_gpu_descriptor(&self, reference_desc: &[u8]) -> EcResult<bool> {
let current_desc = self.read_gpu_descriptor()?;
let max_len = core::cmp::max(current_desc.len(), reference_desc.len());
let mut valid = true;
for i in 0..max_len {
let expected = reference_desc.get(i);
let actual = current_desc.get(i);
if expected != actual {
println!(
" Offset 0x{:04X}: expected {:02X?}, got {:02X?}",
i, expected, actual
);
valid = false;
}
}
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written, validation reads header.descriptor_length bytes and compares them to reference_desc. If the reference buffer length differs from the length encoded in the header (e.g., extra trailing bytes in the file), this will always report false even when the EEPROM content matches what the header says is valid. Consider asserting reference_desc.len() == header.descriptor_length as usize (and erroring if not) or otherwise defining which length should be compared.

Suggested change
}
}
let descriptor_length = header.descriptor_length as usize;
if reference_desc.len() != descriptor_length {
return Err(EcError::DeviceError(format!(
"Reference descriptor length mismatch: expected {}, got {}",
descriptor_length,
reference_desc.len()
)));
}

Copilot uses AI. Check for mistakes.
Ok(valid)
Comment on lines +1530 to +1544
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate_gpu_descriptor() reads the EEPROM via read_gpu_descriptor() but doesn't ensure the expansion bay power rail (gpu_3v_5v_en) is enabled first. Since set_gpu_descriptor() may power-cycle the bay during writes, callers that validate immediately after writing can hit read failures. Consider either (a) handling the same power-on/power-off logic inside validation, or (b) performing validation as part of the write flow before power is turned back off.

Suggested change
let current_desc = self.read_gpu_descriptor()?;
let max_len = core::cmp::max(current_desc.len(), reference_desc.len());
let mut valid = true;
for i in 0..max_len {
let expected = reference_desc.get(i);
let actual = current_desc.get(i);
if expected != actual {
println!(
" Offset 0x{:04X}: expected {:02X?}, got {:02X?}",
i, expected, actual
);
valid = false;
}
}
Ok(valid)
let gpu_power_enabled = self.get_gpio("gpu_3v_5v_en")?;
let force_power = !gpu_power_enabled;
if force_power {
self.set_gpio("gpu_3v_5v_en", true)?;
}
let return_val = (|| -> EcResult<bool> {
let current_desc = self.read_gpu_descriptor()?;
let max_len = core::cmp::max(current_desc.len(), reference_desc.len());
let mut valid = true;
for i in 0..max_len {
let expected = reference_desc.get(i);
let actual = current_desc.get(i);
if expected != actual {
println!(
" Offset 0x{:04X}: expected {:02X?}, got {:02X?}",
i, expected, actual
);
valid = false;
}
}
Ok(valid)
})();
if force_power {
let res = self.set_gpio("gpu_3v_5v_en", false);
if let Err(err) = res {
error!("Failed to set ALW power to GPU off {:?}", err);
return if return_val.is_err() {
return_val
} else {
Err(err)
};
}
}
return_val

Copilot uses AI. Check for mistakes.
}

pub fn read_gpu_descriptor(&self) -> EcResult<Vec<u8>> {
let header = self.read_gpu_desc_header()?;
if header.magic != [0x32, 0xAC, 0x00, 0x00] {
Expand Down
7 changes: 7 additions & 0 deletions framework_lib/src/commandline/clap_std.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,10 @@ struct ClapCli {
#[arg(long)]
dump_gpu_descriptor_file: Option<std::path::PathBuf>,

/// File to validate the gpu EEPROM against
#[arg(long)]
validate_gpu_descriptor_file: Option<std::path::PathBuf>,

/// Show NVIDIA GPU information (Framework 16 only)
#[arg(long)]
nvidia: bool,
Expand Down Expand Up @@ -564,6 +568,9 @@ pub fn parse(args: &[String]) -> Cli {
dump_gpu_descriptor_file: args
.dump_gpu_descriptor_file
.map(|x| x.into_os_string().into_string().unwrap()),
validate_gpu_descriptor_file: args
.validate_gpu_descriptor_file
.map(|x| x.into_os_string().into_string().unwrap()),
Comment on lines +571 to +573
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converting PathBuf to String with .into_string().unwrap() will panic on non-UTF8 paths. Since this is user-provided input, consider keeping PathBuf in Cli or using a lossy conversion (and emitting a friendly error) to avoid crashing.

Copilot uses AI. Check for mistakes.
nvidia: args.nvidia,
host_command,
}
Expand Down
30 changes: 29 additions & 1 deletion framework_lib/src/commandline/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ pub struct Cli {
pub flash_gpu_descriptor: Option<(u8, String)>,
pub flash_gpu_descriptor_file: Option<String>,
pub dump_gpu_descriptor_file: Option<String>,
pub validate_gpu_descriptor_file: Option<String>,
pub nvidia: bool,
// UEFI only
pub allupdate: bool,
Expand Down Expand Up @@ -1807,7 +1808,15 @@ pub fn run_with_args(args: &Cli, _allupdate: bool) -> i32 {
println!(" Size: {:>20} KB", data.len() / 1024);
let res = ec.set_gpu_descriptor(&data, args.dry_run);
match res {
Ok(()) => println!("GPU Descriptor successfully written"),
Ok(()) => {
println!("GPU Descriptor successfully written");
println!("Validating GPU Descriptor...");
match ec.validate_gpu_descriptor(&data) {
Ok(true) => println!(" Validation passed"),
Ok(false) => println!(" Validation FAILED: read-back mismatch"),
Err(err) => println!(" Validation error: {:?}", err),
Comment on lines +1812 to +1817
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--validate-gpu-descriptor-file validation runs even when --dry-run is set. Since set_gpu_descriptor(..., dry_run=true) doesn't write anything, the immediate read-back validation will usually report a mismatch and/or error, which is misleading. Skip validation when args.dry_run is true (or clearly label it as validating the current EEPROM vs the file).

Suggested change
println!("GPU Descriptor successfully written");
println!("Validating GPU Descriptor...");
match ec.validate_gpu_descriptor(&data) {
Ok(true) => println!(" Validation passed"),
Ok(false) => println!(" Validation FAILED: read-back mismatch"),
Err(err) => println!(" Validation error: {:?}", err),
if args.dry_run {
println!(
"GPU Descriptor dry run successful; skipping validation because no changes were written"
);
} else {
println!("GPU Descriptor successfully written");
println!("Validating GPU Descriptor...");
match ec.validate_gpu_descriptor(&data) {
Ok(true) => println!(" Validation passed"),
Ok(false) => println!(" Validation FAILED: read-back mismatch"),
Err(err) => println!(" Validation error: {:?}", err),
}

Copilot uses AI. Check for mistakes.
}
Comment on lines +1813 to +1818
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validation is triggered after set_gpu_descriptor returns, but set_gpu_descriptor may temporarily enable gpu_3v_5v_en and then disable it again before returning. In that case validate_gpu_descriptor() will try to read the EEPROM with power off and likely fail. Consider moving validation into set_gpu_descriptor (before power is toggled off), or ensure validation also handles the same power-enable/disable flow.

Suggested change
println!("Validating GPU Descriptor...");
match ec.validate_gpu_descriptor(&data) {
Ok(true) => println!(" Validation passed"),
Ok(false) => println!(" Validation FAILED: read-back mismatch"),
Err(err) => println!(" Validation error: {:?}", err),
}
println!(
"Skipping immediate validation after write because the EEPROM may have been powered down before set_gpu_descriptor returned"
);

Copilot uses AI. Check for mistakes.
}
Err(err) => println!("GPU Descriptor write failed with error: {:?}", err),
}
}
Expand All @@ -1819,6 +1828,25 @@ pub fn run_with_args(args: &Cli, _allupdate: bool) -> i32 {
println!("Dumping to {}", dump_path);
}
dump_dgpu_eeprom(&ec, dump_path);
} else if let Some(validate_path) = &args.validate_gpu_descriptor_file {
#[cfg(feature = "uefi")]
let data: Option<Vec<u8>> = crate::fw_uefi::fs::shell_read_file(validate_path);
#[cfg(not(feature = "uefi"))]
let data = match fs::read(validate_path) {
Ok(data) => Some(data),
Err(e) => {
println!("Error {:?}", e);
None
}
};
if let Some(data) = data {
println!("Validating GPU Descriptor against {}", validate_path);
match ec.validate_gpu_descriptor(&data) {
Ok(true) => println!(" Validation passed"),
Ok(false) => println!(" Validation FAILED: read-back mismatch"),
Err(err) => println!(" Validation error: {:?}", err),
}
Comment on lines +1845 to +1848
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--validate-gpu-descriptor-file prints pass/fail but run_with_args still exits with status 0 either way, which makes it hard to use in scripts/CI. Consider returning a non-zero exit code when validation fails or errors (similar to how compare_version() returns 1 on mismatch).

Suggested change
Ok(true) => println!(" Validation passed"),
Ok(false) => println!(" Validation FAILED: read-back mismatch"),
Err(err) => println!(" Validation error: {:?}", err),
}
Ok(true) => {
println!(" Validation passed");
return 0;
}
Ok(false) => {
println!(" Validation FAILED: read-back mismatch");
return 1;
}
Err(err) => {
println!(" Validation error: {:?}", err);
return 1;
}
}
} else {
return 1;

Copilot uses AI. Check for mistakes.
}
}

0
Expand Down
9 changes: 9 additions & 0 deletions framework_lib/src/commandline/uefi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ pub fn parse(args: &[String]) -> Cli {
flash_gpu_descriptor: None,
flash_gpu_descriptor_file: None,
dump_gpu_descriptor_file: None,
validate_gpu_descriptor_file: None,
allupdate: false,
info: false,
meinfo: None,
Expand Down Expand Up @@ -859,6 +860,14 @@ pub fn parse(args: &[String]) -> Cli {
None
};
found_an_option = true;
} else if arg == "--validate-gpu-descriptor-file" {
cli.validate_gpu_descriptor_file = if args.len() > i + 1 {
Some(args[i + 1].clone())
} else {
println!("Need to provide a value for --validate_gpu_descriptor_file. PATH");
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message for a missing value uses --validate_gpu_descriptor_file, but the actual flag is --validate-gpu-descriptor-file. This will confuse users copy/pasting the flag from the error output; update the message to match the real CLI option spelling.

Suggested change
println!("Need to provide a value for --validate_gpu_descriptor_file. PATH");
println!("Need to provide a value for --validate-gpu-descriptor-file. PATH");

Copilot uses AI. Check for mistakes.
None
};
found_an_option = true;
}
}

Expand Down
6 changes: 5 additions & 1 deletion framework_tool/completions/bash/framework_tool
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ _framework_tool() {

case "${cmd}" in
framework_tool)
opts="-v -q -t -f -h --flash-gpu-descriptor --verbose --quiet --versions --version --features --esrt --device --compare-version --power --thermal --sensors --fansetduty --fansetrpm --autofanctrl --pdports --pdports-chromebook --info --meinfo --pd-info --pd-reset --pd-disable --pd-enable --dp-hdmi-info --dp-hdmi-update --audio-card-info --privacy --pd-bin --ec-bin --capsule --dump --h2o-capsule --dump-ec-flash --flash-full-ec --flash-ec --flash-ro-ec --flash-rw-ec --intrusion --inputdeck --inputdeck-mode --expansion-bay --charge-limit --charge-current-limit --charge-rate-limit --get-gpio --fp-led-level --fp-brightness --kblight --remap-key --rgbkbd --ps2-enable --tablet-mode --touchscreen-enable --stylus-battery --console --reboot-ec --ec-hib-delay --uptimeinfo --s0ix-counter --hash --driver --pd-addrs --pd-ports --test --test-retimer --boardid --force --dry-run --flash-gpu-descriptor-file --dump-gpu-descriptor-file --nvidia --host-command --generate-completions --help"
opts="-v -q -t -f -h --flash-gpu-descriptor --verbose --quiet --versions --version --features --esrt --device --compare-version --power --thermal --sensors --fansetduty --fansetrpm --autofanctrl --pdports --pdports-chromebook --info --meinfo --pd-info --pd-reset --pd-disable --pd-enable --dp-hdmi-info --dp-hdmi-update --audio-card-info --privacy --pd-bin --ec-bin --capsule --dump --h2o-capsule --dump-ec-flash --flash-full-ec --flash-ec --flash-ro-ec --flash-rw-ec --intrusion --inputdeck --inputdeck-mode --expansion-bay --charge-limit --charge-current-limit --charge-rate-limit --get-gpio --fp-led-level --fp-brightness --kblight --remap-key --rgbkbd --ps2-enable --tablet-mode --touchscreen-enable --stylus-battery --console --reboot-ec --ec-hib-delay --uptimeinfo --s0ix-counter --hash --driver --pd-addrs --pd-ports --test --test-retimer --boardid --force --dry-run --flash-gpu-descriptor-file --dump-gpu-descriptor-file --validate-gpu-descriptor-file --nvidia --host-command --generate-completions --help"
if [[ ${cur} == -* || ${COMP_CWORD} -eq 1 ]] ; then
COMPREPLY=( $(compgen -W "${opts}" -- "${cur}") )
return 0
Expand Down Expand Up @@ -201,6 +201,10 @@ _framework_tool() {
COMPREPLY=($(compgen -f "${cur}"))
return 0
;;
--validate-gpu-descriptor-file)
COMPREPLY=($(compgen -f "${cur}"))
return 0
;;
--host-command)
COMPREPLY=($(compgen -f "${cur}"))
return 0
Expand Down
1 change: 1 addition & 0 deletions framework_tool/completions/fish/framework_tool.fish
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ complete -c framework_tool -l pd-addrs -d 'Specify I2C addresses of the PD chips
complete -c framework_tool -l pd-ports -d 'Specify I2C ports of the PD chips (Advanced)' -r
complete -c framework_tool -l flash-gpu-descriptor-file -d 'File to write to the gpu EEPROM' -r -F
complete -c framework_tool -l dump-gpu-descriptor-file -d 'File to dump the gpu EEPROM to' -r -F
complete -c framework_tool -l validate-gpu-descriptor-file -d 'File to validate the gpu EEPROM against' -r -F
complete -c framework_tool -l host-command -d 'Send an EC host command. Args: <CMD_ID> <VERSION> [DATA...]' -r
complete -c framework_tool -l generate-completions -d 'Generate shell completions and print to stdout' -r -f -a "bash\t''
elvish\t''
Expand Down
1 change: 1 addition & 0 deletions framework_tool/completions/zsh/_framework_tool
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ _framework_tool() {
'*--pd-ports=[Specify I2C ports of the PD chips (Advanced)]:PD_PORTS:_default:PD_PORTS:_default:PD_PORTS:_default' \
'--flash-gpu-descriptor-file=[File to write to the gpu EEPROM]:FLASH_GPU_DESCRIPTOR_FILE:_files' \
'--dump-gpu-descriptor-file=[File to dump the gpu EEPROM to]:DUMP_GPU_DESCRIPTOR_FILE:_files' \
'--validate-gpu-descriptor-file=[File to validate the gpu EEPROM against]:VALIDATE_GPU_DESCRIPTOR_FILE:_files' \
'*--host-command=[Send an EC host command. Args\: <CMD_ID> <VERSION> \[DATA...\]]:HOST_COMMAND:_default:HOST_COMMAND:_default' \
'--generate-completions=[Generate shell completions and print to stdout]:SHELL:(bash elvish fish powershell zsh)' \
'*-v[Increase logging verbosity]' \
Expand Down