Re: [PATH 1/5 -v2] acpi, IO memory pre-mapping and atomic accessing

[Bjorn Helgaas <bjorn.helgaas@hp.com>]

On Monday 14 December 2009 06:04:13 pm Huang Ying wrote:
> On Sat, 2009-12-12 at 01:36 +0800, Bjorn Helgaas wrote: 
> > The ACPI CA has functions called acpi_hw_read() and acpi_hw_write()
> > that have similar prototypes and functionality (but of course, they
> > don't work in atomic context).  It'd be nice if your new functions
> > had similar names, e.g., acpi_hw_map(), acpi_hw_unmap(),
> > acpi_hw_read_atomic(), acpi_hw_write_atomic().
> 
> acpi_hw_read/write is the 32-bit optimized version of acpi_read/write.
> So I think it is better to follow the naming convention of
> acpi_read/write.

You're right; I didn't notice that.  I agree that it's better to
follow the convention of acpi_read().

> > I think your code would be simpler if acpi_pre_map_gar() returned a
> > struct acpi_iomap pointer (from the caller's point of view, this would
> > be an opaque cookie).  Then you could just supply that cookie to
> > acpi_atomic_write(), and you wouldn't have to look it up again.  Maybe
> > you could even get rid of the list and all the fancy RCU & kref stuff
> > then, too.
> 
> The interface chosen is based on usage model, which is:
> 
> 1. In init function, all GARs needed are pre-mapped
> 2. In atomic context, pre-mapped GARs are accessed
> 3. In exit function, all GARs are post-unmapped
> 
> In 3), if struct acpi_iomap* is used as parameter for post-unmap
> function, we need to record that pointer in another list. In 2), we need
> find mapped address from GAR.

I understand that my proposal would require a slight change in your
usage model.  I am suggesting that you make it follow the same pattern
as pci_iomap(), e.g.:

	void *pci_iomap(struct pci_dev *, int bar, unsigned long len);
	unsigned int ioread32(void *);
	void pci_iounmap(struct pci_dev *, void *);

	void *acpi_map_generic_address(struct acpi_generic_address *);
	u64 acpi_read_atomic(void *);
	void acpi_unmap_generic_address(void *);

acpi_map_generic_address() would validate the GAR and do the ioremap().
If the validation or ioremap() failed, it would return a NULL pointer.

This would require the caller of acpi_map_generic_address() to hang onto
the pointer returned (just as callers of pci_iomap() must hang onto the
pointer it returns).

The pointer would be supplied to acpi_read_atomic(), so it would not
need to do acpi_check_gar() because we know it was done successfully
in acpi_map_generic_address().  It wouldn't need to look up the GAR
in the list because the list entry was passed in.  Since all the
possible failure conditions were checked in acpi_map_generic_address(),
acpi_read_atomic() doesn't need to return status and could simply
return the u64 directly.

> > > +/* In NMI handler, should set silent = 1 */
> > > +static int acpi_check_gar(struct acpi_generic_address *reg,
> > > +			  u64 *paddr, int silent)
> > > +{
> > > +	u32 width;
> > > +
> > > +	/* Handle possible alignment issues */
> > > +	memcpy(paddr, &reg->address, sizeof(*paddr));
> > > +	if (!*paddr) {
> > > +		if (!silent)
> > > +			pr_info(
> > > +			"Invalid physical address in GAR, firmware bug?\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	width = reg->bit_width;
> > > +	if ((width != 8) && (width != 16) && (width != 32) && (width != 64)) {
> > > +		if (!silent)
> > > +			pr_info(
> > > +			"Invalid bit width in GAR, firmware bug?\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> > > +	    reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
> > > +		if (!silent)
> > > +			pr_info(
> > > +			"Invalid address space type in GAR, firmware bug?\n");
> > 
> > Error messages with constant text are nearly useless because they
> > don't give much of a clue about where to look for a problem.
> > Personally, for something this, I would just return failure and
> > never print anything.  If a map fails, the caller should notice
> > and you then have a good idea of where to look.
> 
> The checking here is for bug in firmware not software. I think it is
> necessary for the user to know where the bugs may come from, and it is
> hard to express the bug in return code.

Yes, I understand that this is checking for firmware bugs.  My point
is that when a user sees this in his dmesg log:

	Invalid bit width in GAR, firmware bug?

we have no context, and even a kernel developer can't figure out what
to do.  We could ask for a copy of the FADT and DSDT, but even then,
we don't know *which* GAR structure to look at, so we'll probably have
to add some instrumentation and ask the user to reproduce the problem.

If the check were in the caller, it could at least say something like:

	ACPI: couldn't map generic address [io 0xcf8] for PCI config access

which would give us more useful information.

I guess your assumption is that *both* the caller and acpi_check_gar()
would print something, so then we'd know which GAR was bad and also
exactly what was wrong with it.  My opinion (and this is just my opinion)
is that knowing with GAR is bad is the important part.  A GAR is simple
enough that if we know which one to look at, the problem will be obvious,
so we only need the message from the caller.

Bjorn