Re: [flashrom] [RFC] Kill --force

[Michael Karcher]

Am Dienstag, den 13.04.2010, 04:16 +0200 schrieb Carl-Daniel Hailfinger:
> First step:
> Remove any suggestions to use --force for probe/read from flashrom
> output.

Good idea, best would be to have dummy chips added. So chips that can't
be probed, written or erased, but read by mmap. Like dummy512k and so
on.
> Don't talk about "success" or "Found chip" if the chip is forced.

Good idea.
> Add a new internal programmer parameter boardmismatch=force. This
> overrides any mismatch detection from cbtable/image comparisons.

OK.
> Add a new internal programmer parameter laptop=force_I_want_a_brick.

I don't like "force_I_want_a_brick", but I don't have any better idea.
Something like "warranty=void" sounds appealing on the first thought,
but does not really carry the information that the laptop system is the
problem.
> Adjust the documentation for --force.

Excellent. Having documentation/code drift is a serious problem.
> -Force write without checking whether the ROM image file is really meant
> -to be used on this board.
> -.sp
> -Note: This check only works while coreboot is running, and only for those
> -boards where the coreboot code supports it.
> +Force something undefined. DO NOT USE!

Maybe you should write down here what force does. You had it in the
introduction text.
> +If flashrom detects that the image you want to write and the current board
> +do not match, it will refuse to write the image unless you specify
> +.sp
> +.B "flashrom -p internal:boardmismatch=force"
> +.sp
> +Note: The board match check only works while coreboot is running, and only for
> +those boards where the coreboot code supports it.
> +.sp

Write it the other way around, like

On systems running coreboot system with support for it, flashrom checks
whether the image is meant for your board...

So that the reader knows about the limitation that this checking only
works on coreboot systems before he feels safe that flashrom checks.
> +Do not use flashrom on laptops! The embedded controller (EC) present in many
> +laptops interacts badly with any flash attempts. This is a hardware limitation
> +and flashrom will attempt to detect it and abort immediately for safety reasons.

I am more like "Using flashrom on laptops is dangerous and may easily
make your hardware unusable!" instead of "Do not use flashrom on
laptops!" I think that having people making the choice not to use
flashrom on laptops because they know it's dangerous is superior to just
tell them not to do it.
> if (chip_to_probe) {
> for (flash = flashchips; flash && flash->name; flash++)
> 	if (!strcmp(flash->name, chip_to_probe))
> 		break;
> if (!flash || !flash->name) {
> 	fprintf(stderr, "Error: Unknown chip specified.\n");
> 	exit(1);
> }
> /* Clean up after the check. */
> flash = NULL;
> }

do we need this new code block...
> if (force && read_it && chip_to_probe) {
> 	printf("Force read (-f -r -c) requested, forcing chip probe success:\n");
> 	printf("Force read (-f -r -c) requested, pretending the chip is there:\n");

good message change.
> 	flashes[0] = probe_flash(flashchips, 1);
> 	if (!flashes[0]) {
> 		printf("flashrom does not support a flash chip named '%s'.\n", chip_to_probe);

if we have this?
> printf("Found chip "%s %s" (%d KB, %s) at physical address 0x%lx.\n",
> printf("%s chip "%s %s" (%d KB, %s) at physical address 0x%lx.\n",
>       force ? "Assuming" : "Found",

Assuming -> Forced?
> if (is_laptop) {
> msg_perr("========================================================================\n"
> 	 "WARNING! You seem to be running flashrom on a laptop.\n"
> 	 "Laptops, notebooks and netbooks are difficult to support and we recommend\n"
> 	 "to use the vendor flashing utility. The embedded controller (EC) in these\n"
> 	 "machines often interacts badly with flashing.\n"
> 	 "See http://www.flashrom.org/Laptops for details.\n\n"
> 	 "If flash is shared with the EC, erase is guaranteed to brick your laptop\n"
> 	 "and write may brick your laptop.\n"
> 	 "Read and probe may irritate your EC and cause fan failure, backlight\n"
> 	 "failure and sudden poweroff.\n"
> 	 "You have been warned.\n"
> 	 "========================================================================\n");
> if (force_laptop) {
> 	msg_perr("Proceeding anyway because user specified "
> 		 "laptop=force_I_want_a_brick.\n");
> } else {
> 	msg_perr("Aborting.\n");
> 	exit(1);
> }
> }

I understand your motivation of not documenting the force parameter
here. But I don't like the "security by obscurity" approach in general.
If we provide the forcing parameter, it should be documented somewhere.
Leave the undocumented stuff to closed-source software.

No real showstoppers, so

Acked-by: Michael Karcher

Regards,
  Michael Karcher