[PATCH] quick fix for chip locks

[Sean Nelson]

Here's a very quick patch to fix the missing unlock code.
Fixes missing unlock for certain chips:
  * unlock_49lf00x
     * Pm49fl002
     * Pm49fl004

* unlock_49flxxxc
     * SST49LF160C

* unlock_winbond_fwhub
     * W39V080FA
     * W39V080FA (dual mode)

Fixes missing printlock for certain chip:
  * printlock_w39v040c
     * W39V040C

Signed-off-by: Sean Nelson

---
Let's get this regression fixed ASAP.

[Sean Nelson]

On 2/18/10 3:46 PM, Sean Nelson wrote:
> Here's a very quick patch to fix the missing unlock code.
> Fixes missing unlock for certain chips:
>
> unlock_49lf00x
> Pm49fl002
> Pm49fl004
>
> unlock_49flxxxc
> SST49LF160C
>
> unlock_winbond_fwhub
> W39V080FA
> W39V080FA (dual mode)
>
> Fixes missing printlock for certain chip:
> printlock_w39v040c
> W39V040C
>
> Signed-off-by: Sean Nelson
>
> Let's get this regression fixed ASAP.

Updated patch so that unlock is done only on read/write/erase, and 
printlock returns 0.

[Carl-Daniel Hailfinger]

On 19.02.2010 01:11, Sean Nelson wrote:
> On 2/18/10 3:46 PM, Sean Nelson wrote:
> > Here's a very quick patch to fix the missing unlock code.
> > Fixes missing unlock for certain chips:
> > Fixes missing printlock for certain chip:
> >
> > Signed-off-by: Sean Nelson
> >
> > Let's get this regression fixed ASAP.
>
> Updated patch so that unlock is done only on read/write/erase, and
> printlock returns 0.

Thanks. My earlier review applies in spirit. On top of it, I noticed this:
> Index: flashchips.c
>
> --- flashchips.c	(revision 906)
> +++ flashchips.c	(working copy)
> @@ -43,18 +43,20 @@
>     * .page_size		= Page or eraseblock(?) size in bytes
>     * .tested		= Test status
>     * .probe		= Probe function
>     * .probe_timing	= Probe function delay
>     * .block_erasers[]	= Array of erase layouts and erase functions
>     * {
>     *	.eraseblocks[]	= Array of { blocksize, blockcount }
>     *	.block_erase	= Block erase function
>     * }
> * .printstatus		= Chip lock status function

AFAIK it is called printlock everywhere else.
> * .unlock		= Chip unlock function

Regards,
Carl-Daniel

[Sean Nelson]

Here's a very quick patch to fix the missing unlock code.
Fixes missing unlock for certain chips:
  * unlock_49lf00x
     * Pm49fl002
     * Pm49fl004

* unlock_49flxxxc
     * SST49LF160C

* unlock_winbond_fwhub
     * W39V080FA
     * W39V080FA (dual mode)

Fixes missing printlock for certain chip:
  * printlock_w39v040c
     * W39V040C

Signed-off-by: Sean Nelson 
---
This should fix everything.

[Sean Nelson]

On 2/18/10 4:27 PM, Sean Nelson wrote:
> Here's a very quick patch to fix the missing unlock code.
> Fixes missing unlock for certain chips:
>
> unlock_49lf00x
> Pm49fl002
> Pm49fl004
>
> unlock_49flxxxc
> SST49LF160C
>
> unlock_winbond_fwhub
> W39V080FA
> W39V080FA (dual mode)
>
> Fixes missing printlock for certain chip:
> printlock_w39v040c
> W39V040C
> Signed-off-by: Sean Nelson
>
> This should fix everything.

Explicitly unlock for write and verify.

[Michael Karcher]

Am Donnerstag, den 18.02.2010, 16:27 -0800 schrieb Sean Nelson:
> Here's a very quick patch to fix the missing unlock code.
> Fixes missing unlock for certain chips:
>
> unlock_49lf00x
> Pm49fl002
> Pm49fl004
>
> unlock_49flxxxc
> SST49LF160C
>
> unlock_winbond_fwhub
> W39V080FA
> W39V080FA (dual mode)
>
> Fixes missing printlock for certain chip:
> printlock_w39v040c
> W39V040C
>
> Signed-off-by: Sean Nelson

Acked-by: Michael Karcher

Regards,
  Michael Karcher

[Sean Nelson]

On 2/18/10 4:47 PM, Michael Karcher wrote:
> Am Donnerstag, den 18.02.2010, 16:27 -0800 schrieb Sean Nelson:
> > Here's a very quick patch to fix the missing unlock code.
> > Fixes missing unlock for certain chips:
> >
> > unlock_49lf00x
> > Pm49fl002
> > Pm49fl004
> >
> > unlock_49flxxxc
> > SST49LF160C
> >
> > unlock_winbond_fwhub
> > W39V080FA
> > W39V080FA (dual mode)
> >
> > Fixes missing printlock for certain chip:
> > printlock_w39v040c
> > W39V040C
> >
> > Signed-off-by: Sean Nelson
>
> Acked-by: Michael Karcher
>
> Regards,
>    Michael Karcher

Thanks, committed in r907.

[Carl-Daniel Hailfinger]

On 19.02.2010 00:46, Sean Nelson wrote:
> Here's a very quick patch to fix the missing unlock code.
> Fixes missing unlock for certain chips:
>
> unlock_49lf00x
> Pm49fl002
> Pm49fl004
>
> unlock_49flxxxc
> SST49LF160C
>
> unlock_winbond_fwhub
> W39V080FA
> W39V080FA (dual mode)
>
> Fixes missing printlock for certain chip:
> printlock_w39v040c
> W39V040C
>
> Signed-off-by: Sean Nelson

Thanks a lot. We desperately need this regression fix.
A few minor comments. Once you address them, this is

Acked-by: Carl-Daniel Hailfinger

W39V040C seems to only have a printlock and not any unlock. Not
something you need to fix (no regression), so feel free to ignore this
comment.
> Index: flashrom.c
>
> --- flashrom.c	(revision 906)
> +++ flashrom.c	(working copy)
> @@ -832,18 +832,19 @@
>    }
>
> if (!flash || !flash->name)
>    	return NULL;
>
> 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);
> flash->printlock(flash);

This will crash and burn (NULL pointer dereference). Please check
if (flash->printlock)
before executing that pointer.
> @@ -1130,18 +1131,20 @@
>    uint8_t *buf;
>    unsigned long numbytes;
>    FILE *image;
>    int ret = 0;
>    unsigned long size;
>
> size = flash->total_size * 1024;
>    buf = (uint8_t *) calloc(size, sizeof(char));
> flash->unlock(flash);

Same problem here.

Did you make sure that a simple probe-only execution doesn't unlock the
chip? Didn't have time to check (need to get back to my books).
Please also check that the codebase still compiles after "make distclean".

By the way, if you have the time to check all currently-unused files for
discrepancies between old functionality (that is, lock stuff) and new
functionality and you're reasonably sure that they are not needed
anymore, please send a separate patch (or rather, a list of files to
execute "svn rm" on) to kill them. I'll be happy to get rid of any dead
code.

Regards,
Carl-Daniel