On Thursday 17 December 2009 01:33:07 am Huang Ying wrote: > On Thu, 2009-12-17 at 01:54 +0800, Bjorn Helgaas wrote: > > On Tuesday 15 December 2009 06:23:15 pm Huang Ying wrote: > > > On Wed, 2009-12-16 at 01:47 +0800, Bjorn Helgaas wrote: > > > > On Monday 14 December 2009 06:04:13 pm Huang Ying wrote: > > > > > On Sat, 2009-12-12 at 01:36 +0800, Bjorn Helgaas wrote: > > > > > > 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. > > > > > > The usage model is different. Please take a look at > > > __apei_exec_read_register(). We need to get virtual address from > > > physical address in GAR. And many small-size GARs in ERST or EINJ may > > > share same page, so some kind of virtual space optimization is > > > necessary. > > > > The sharing could be done with a reference count in the map/unmap > > path. > > > > I don't have time to fully understand your patch series, but I suspect > > the issue is that you're interpreting opcodes, and those contain or > > reference generic_address structures, and you think it's easier just > > pass around the GAR pointer than it is to remember a (GAR, mapped-GAR) > > association made by pre_map_gar() and use the mapped-GAR when executing > > the "read" opcode. > > So we need the (GAR, mapped-GAR) association anyway. Variable > acpi_iomaps in atomicio.c is such a association. When executing the > "read" opcode, we still need get mapped-GAR from the GAR. So I think > your proposal and my implementation is similar, with different naming > convention and you want to put the association in APEI code, and I want > to put it in ACPI code. Well, yes, they're similar in the sense that both do remember the (GAR, mapped-GAR) association. Your implementation remembers it in the acpi_iomaps list, and I proposed having the caller keep it. The advantage I see with having the caller keep it is that it would make atomicio.c MUCH simpler -- no list, no lookup, no repeated validation, no RCU, probably no kref stuff. Obviously this would come at the cost of some increased complexity in the callers. And I think it's worth something to use the pattern of pci_iomap(), because that's a well-known Linux design pattern. Anyway, just my opinion; feel free to ignore. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
