Re: [PATCH v3 1/3] resource: Add @flags to region_intersects()

[Toshi Kani]

On Tue, 2015-12-01 at 09:19 -0800, Linus Torvalds wrote:
> On Tue, Dec 1, 2015 at 9:13 AM, Borislav Petkov <bp@xxxxxxxxx> wrote:
> > 
> > Oh sure, I didn't mean you. I was simply questioning that whole
> > identify-resource-by-its-name approach. And that came with:
> > 
> > 67cf13ceed89 ("x86: optimize resource lookups for ioremap")
> > 
> > I just think it is silly and that we should be identifying resource
> > things in a more robust way.
> 
> I could easily imagine just adding a IORESOURCE_RAM flag (or SYSMEM or
> whatever). That sounds sane. I agree that comparing the string is
> ugly.
> 
> > Btw, the ->name thing in struct resource has been there since a *long*
> > time
> 
> It's pretty much always been there.  It is indeed meant for things
> like /proc/iomem etc, and as a debug aid when printing conflicts,
> yadda yadda. Just showing the numbers is usually useless for figuring
> out exactly *what* something conflicts with.

I agree that regular memory should have its own type, which separates
itself from MMIO.  By looking at how IORESOURCE types are used, this change
has the following challenges, and I am sure I missed some more.

1. Large number of IORESOURCE_MEM usage
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.

http://lxr.free-electrons.com/ident?i=IORESOURCE_MEM

2. Lack of free flags bit in resource
The flags bits are defined in include/linux/ioport.h.  The flags are
defined as unsigned long, which is 32-bit in 32-bit config.  The most of
the bits have been assigned already.  Bus-specific bits for IORESOURCE_MEM
have been assigned mostly as well (line 82).

3. Interaction with pnp subsystem
The same IORESOURCE types and bus-specific flags are used by the pnp
subsystem.  pnp_mem objects represent IORESOURCE_MEM type listed by
pnp_dev.  Adding a new IORESOURCE type likely requires adding a new object
type and its interfaces to pnp.

4. I/O resource names represent allocation types
While IORESOURCE types represent hardware types and capabilities, the
string names represent resource allocation types and usages.  For instance,
regular memory is allocated for the OS as "System RAM", kdump as "Crash
kernel", FW as "ACPI Tables", and so on.  Hence, a new type representing
"System RAM" needs to be usage based, which is different from the current
IORESOURCE types.

I think this work will require a separate patch series at least.  For this
patch series, supporting error injections to NVDIMM, I propose that we make
the change suggested by Dan:

"We could define 'const char *system_ram = "System RAM"' somewhere andthen
do pointer comparisons to cut down on the thrash of adding newflags to
'struct resource'?"

Let me know if you have any suggestions/concerns. 

Thanks,
-Toshi

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/