Re: [flashrom] [PATCH] Factor out lock functions (print, unlock, lock)

[Maciej Pijanka]

On pon, 23 lis 2009, Carl-Daniel Hailfinger wrote:
> On 20.11.2009 00:05, Maciej Pijanka wrote:
> > How about adding dummy operation to enum and something like
> >
> > case dummy_op:
> > default:
> >
> > in switch clause, first to use 0 value to dummy op, also to make more
> > explicit what
> > default really is.
>
> Adding a dummy enum which does nothing doesn't look like it would help.
> Apologies if I'm missing your point.

Its maybe a habit that didnt have much reasons to include dummy op when using
integer op enumeration. This was only my point, or i already forgot other reason.
> > Additionally i didn't saw any routine that lets check if current chip
> > has any lock abilities like
> > spi_lock(somechip, lock_query) which would return if any locks are
> > available or something that
> > code may use it to check if locking is available (regardless of
> > presence routine in chip data)
> > for this chip.
> >
> > Maybe this isn't really needed but we have routine that sets and
> > resets lock, prints info to user
> > but no routine that could poll status of lock or verify if chip has
> > locking abilities at all.
>
> Good point. Updated patch below.
>
> Chip locking has four actions you can do with it:
> Print the current locking status
> Read the current locking status
> Lock the chip
> Unlock the chip.
>
> Currently, the code usually does lock printing inside the probe
> function, and unlocking somewhere in the erase or write function. Only
> very few chips reactivate the lock after write/erase. Since many chips
> have identical probe/write/erase functions, but totally different
> locking, many such functions have been duplicated needlessly.
> With this patch, it is possible to call the chip-specific locking
> functions from probe/write/erase functions and unify lots of code.

While i like different return for fail/success/not supported i think
separate return code for unable-to-determine-if-we-failed might be good
because that way we let upper layer decide if fail in this case is disaster 
or not. Also maybe refining in this way could lead that no lock/unlock code
need to use fprintf, only use some shared buffer to leave message if any
and return code leaving decision how to print/log output to caller code, thus
making future libflashrom more flexible (ie, imagine that somebody not 
redirect stderr stream while link Xapp with libflashrom, then all printed on
stderr that he forget to catch otherwise is lost)

Otherwise code looks fine for me, i left two parts of code and comments to them
below.
> Index: flashrom-lock_refactor/spi.c
>
> --- flashrom-lock_refactor/spi.c	(Revision 769)
> +++ flashrom-lock_refactor/spi.c	(Arbeitskopie)
> @@ -467,39 +451,76 @@
>    	bpt[(status & 0x1c) >> 2]);
>  }
>
> -void spi_prettyprint_status_register(struct flashchip *flash)
> +/*
> Return 0 if successful, 1 if failed, 2 if unsupported.
> */
>
> +int spi_prettyprint_status_register(struct flashchip *flash)
>  {
>    uint8_t status;
>
> int result = 2;
>
> status = spi_read_status_register();
> printf_debug("Chip status register is %02x\n", status);
> switch (flash->manufacture_id) {
> case ST_ID:
> if (((flash->model_id & 0xff00) == 0x2000) ||
>     ((flash->model_id & 0xff00) == 0x2500))
>     ((flash->model_id & 0xff00) == 0x2500)) {
> spi_prettyprint_status_register_st_m25p(status);
> 	result = 0;
> }
> break;
> case MX_ID:
> if ((flash->model_id & 0xff00) == 0x2000)
> if ((flash->model_id & 0xff00) == 0x2000) {
> spi_prettyprint_status_register_st_m25p(status);
> 	result = 0;
> }
> break;
> case SST_ID:
> switch (flash->model_id) {
> case 0x2541:
> 	spi_prettyprint_status_register_sst25vf016(status);
> 	result = 0;
> break;
> case 0x8d:
> case 0x258d:
> 	spi_prettyprint_status_register_sst25vf040b(status);
> 	result = 0;
> break;
> default:
> 	spi_prettyprint_status_register_sst25(status);
> 	result = 0;
> break;
> }
> break;
> }
> if (result == 2)
> fprintf(stderr, "Printing lock status not supported.\n");
> return result;
>
> }

What about default case in code above to detect situation when somebody has not supported
yet spi chip and maybe give more informative warning message ?
> Index: flashrom-lock_refactor/flashrom.c
>
> --- flashrom-lock_refactor/flashrom.c	(Revision 769)
> +++ flashrom-lock_refactor/flashrom.c	(Arbeitskopie)
> @@ -509,6 +509,26 @@
>    printf("Found chip "%s %s" (%d KB, %s) at physical address 0x%lx.\n",
>           flash->vendor, flash->name, flash->total_size,
>           flashbuses_to_text(flash->bustype), base);
>
> /* Print the locking status. */
>
> if (flash->lock) {
> flash->lock(flash, lock_print);
> switch (flash->lock(flash, lock_check_unlocked)) {
> case 0:
> 	printf_debug("Chip is completely unlocked or does not "
> 		     "support locking.");
> 	break;
> case 1:
> 	printf_debug("Chip is partially/completely locked.\n");
> 	break;
> case 2:
> 	printf_debug("Chip locking status could not be "
> 		     "determined.\n");
> 	break;
> }
>
> } else {
> printf_debug("This chip has not been converted to the new chip "
> 	     "locking infrastructure yet.\n");
>
> }
>
> return flash;
>
> }

In code above i would like to get exact answer is chip unlocked or not support locking
or can't query if its locked (and this is feature of hardware not an error etc).

Best regards
Maciej