On Thu, Dec 3, 2015 at 12:35 PM, Toshi Kani <toshi.kani@xxxxxxx> wrote: > On Thu, 2015-12-03 at 11:01 -0800, Linus Torvalds wrote: >> On Thu, Dec 3, 2015 at 10:40 AM, Borislav Petkov <bp@xxxxxxxxx> wrote: >> > On Thu, Dec 03, 2015 at 11:54:19AM -0700, Toshi Kani wrote: >> > > Adding a new type for regular memory will require inspecting the >> > > codes using IORESOURCE_MEM currently, and modify them to use the new >> > > type if their target ranges are regular memory. There are many >> > > references to this type across multiple architectures and drivers, >> > > which make this inspection and testing challenging. >> > >> > What's wrong with adding a new type_flags to struct resource and not >> > touching IORESOURCE_* at all? >> >> Bah. Both of these ideas are bogus. >> >> Just add a new flag. The bits are already modifiers that you can >> *combine* to show what kind of resource it is, and we already have >> things like IORESOURCE_PREFETCH etc, that are in *addition* to the >> normal IORESOURCE_MEM bit. >> >> Just add another modifier: IORESOURCE_RAM. >> >> So it would still show up as IORESOURCE_MEM, but it would have >> additional information specifying that it's actually RAM. >> >> If somebody does something like >> >> if (res->flags == IORESOURCE_MEM) >> >> then they are already completely broken and won't work *anyway*. It's >> a bitmask, bit a set of values. > > Yes, if we can assign new modifiers, that will be quite simple. :-) I > assume we can allocate new bits from the remaining free bits as follows. > > +#define IORESOURCE_SYSTEM_RAM 0x01000000 /* System RAM */ > +#define IORESOURCE_PMEM 0x02000000 /* Persistent memory */ > #define IORESOURCE_EXCLUSIVE 0x08000000 /* Userland may not map > this resource */ > > Note, SYSTEM_RAM represents the OS memory, i.e. "System RAM", not any RAM > ranges. > > With the new modifiers, region_intersect() can check these ranges. One > caveat is that the modifiers are not very extensible for new types as they > are bit maps. region_intersect() will no longer be capable of checking any > regions with any given name. I think this is OK since this function was > introduced recently, and is only used for checking "System RAM" and > "Persistent Memory" (with this patch series). IORESOURCE_PMEM is not descriptive enough for the two different types of pmem in the kernel. How about we go with just IORESOURCE_SYSTEM_RAM for now since "is_ram()" checks are common. Let the rest continue to be checked by strcmp(). For example the nvdimm-e820 driver cares about "Persistent Memory (legacy)", while other forms of pmem may just be "reserved" and only the driver knows that it is pmem. An IORESOURCE_PMEM would not be reliable nor descriptive enough. -- 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
