[flashrom] [PATCH] sbxxx: Handle active IMCs in AMD chipsets.

[Stefan Tauner]

On Fri, 26 Jul 2013 02:26:34 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 18.07.2013 23:50 schrieb Stefan Tauner:
> > [...]
> > +int amd_imc_shutdown(struct pci_dev *dev)
> > +{
> > +	/* Try to put IMC to sleep */
> > +	int ret = imc_send_cmd(dev, 0xb4);
> > +
> > +	/* No IMC activity detectable, assume we are fine */
> > +	if (ret < 0) {
> > +		msg_pdbg2("No IMC found.\n");
> 
> The message above is printed after the message "Writes have been
> disabled for safety reasons because the IMC is active" from
> sb600_handle_imc(). AFAICS this mesage can only happen if the IMC is
> marked as present but any of the SIO port or MBOX port are zero, i.e.
> inaccessible. May I suggest the following message instead?
> "IMC inactive or unreachable.\n"

ack in general, but the wording inactive contradicts exactly the
wording you have cited... "Writes have been disabled for safety reasons
because the IMC is *active*"... so this might be worse. Also, this is
just a debug2 level message.

> > +		return 0;
> > +	}
> > +
> > +	if (ret != 0) {
> > +		msg_perr("Shutting down IMC failed.\n");
> > +		return ret;
> > +	}
> > +	msg_pdbg2("Shutting down IMC successful.\n");
> > +
> > +	if (register_shutdown(imc_resume, dev))
> > +		return 1;
> > +
> > +	return ret;
> > +}
> > +
> > +#endif
> > diff --git a/programmer.h b/programmer.h
> > index db914cb..f03eac3 100644
> > --- a/programmer.h
> > +++ b/programmer.h
> > @@ -572,6 +572,9 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> >  		 enum ich_chipset ich_generation);
> >  int via_init_spi(struct pci_dev *dev, uint32_t mmio_base);
> >  
> > +/* imc.c */
> 
> Should be amd_imc.c

yes, fixed in tested_stuff 19

> > +int amd_imc_shutdown(struct pci_dev *dev);
> > +
> >  /* it85spi.c */
> >  int it85xx_spi_init(struct superio s);
> >  
> > diff --git a/sb600spi.c b/sb600spi.c
> > index a5c00d8..e76c04a 100644
> > --- a/sb600spi.c
> > +++ b/sb600spi.c
> > @@ -194,6 +195,39 @@ static int sb600_spi_send_command(struct flashctx *flash, unsigned int writecnt,
> >  	return 0;
> >  }
> >  
> > +static int sb600_handle_imc(struct pci_dev *dev, bool amd_imc_force)
> > +{
> > +	/* Handle IMC everywhere but sb600 which does not have one. */
> > +	if (dev->device_id == 0x438d)
> > +		return 0;
> > +
> > +	/* TODO: we should not only look at IntegratedImcPresent (LPC Dev 20, Func 3, 40h) but also at
> > +	 * IMCEnable(Strap) and Override EcEnable(Strap) (sb8xx, sb9xx?, a50: Misc_Reg: 80h-87h;
> > +	 * sb7xx, sp5100: PM_Reg: B0h-B1h) etc. */
> > +	uint8_t reg = pci_read_byte(dev, 0x40);
> > +	if ((reg & (1 << 7)) == 0) {
> > +		msg_pdbg("IMC is not active.\n");
> > +		return 0;
> > +	}
> 
> This is the point where we assume the IMC is active. Given that we only
> check for the IntegratedImcPresent bit, would "IMC is not present" be a
> better message? Or do we want to keep the message and eventually look at
> the straps as well?

What does active, present, enabled mean to us? What does the respective
wording in the documentation specify exactly? We have essentially no
idea (at least I dont). So I think discussing the wording is rather
pointless :) Also, this is a debugging level message.

> > +
> > +	if (!amd_imc_force)
> > +		programmer_may_write = 0;
> > +	msg_pinfo("Writes have been disabled for safety reasons because the IMC is active\n"
> 
> It's present, but does it have to be active?

Similar to above but here the wording is more important. I am not sure
if it is better, but this might make you more happy?
	msg_pinfo("Writes have been disabled for safety reasons because the presence of the IMC\n"
		  "was detected and it could interfere with accessing flash memory. Flashrom will\n"
		  "try to disable it temporarily but even then this might not be safe:\n"

The reason why I am not sure if that is better: the longer and more
eloquent the message the less it is read and/or understood. We tend to
explain facts very thoroughly and I personally like it when apps do
that, but I think most ppl dont. :)

> > +		  "and it could interfere with accessing flash memory. Flashrom will try\n"
> > +		  "to disable it temporarily but even then this might not be safe:\n"
> > +		  "when it is reenabled and after a reboot it expects to find working code\n"
> > +		  "in the flash and it is unpredictable what happens if there is none.\n"
> > +		  "\n"
> > +		  "To be safe make sure that there is a working IMC firmware at the right\n"
> > +		  "location in the image you intend to write and do not attempt to erase.\n"
> > +		  "\n"
> > +		  "You can enforce write support with the amd_imc_force programmer option.\n");
> > +	if (amd_imc_force)
> > +		msg_pinfo("Continuing with write support because the user forced us to!\n");
> > +
> > +	return amd_imc_shutdown(dev);
> 
> Call to shutdown the IMC, done after printing that the IMC is active,
> but inside amd_imc_shutdown() we may print the message that no IMC was
> found. This is a bit contradictory in some cases.

see above

> > +}
> > +
> >  static const struct spi_programmer spi_programmer_sb600 = {
> >  	.type = SPI_CONTROLLER_SB600,
> >  	.max_data_read = 8,
> 
> I just noticed the patch has been committed an hour ago, but throwing
> away the review seemed a waste.

Yes indeed, thanks for the feedback. I am very happy that you have not
found more severe problems (yet) :)

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner