linux-kernel - Re: [PATCH v3 05/24] arch: introduce memremap()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 30 Jul 2015 14:11:37 -0700
From:	Dan Williams <dan.j.williams@...el.com>
To:	"Luis R. Rodriguez" <mcgrof@...e.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...nel.org>,
	"H. Peter Anvin" <hpa@...or.com>, linux-arch@...r.kernel.org,
	"Kani, Toshimitsu" <toshi.kani@...com>,
	Arnd Bergmann <arnd@...db.de>,
	"linux-nvdimm@...ts.01.org" <linux-nvdimm@...ts.01.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Russell King <rmk+kernel@....linux.org.uk>,
	Christoph Hellwig <hch@....de>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3 05/24] arch: introduce memremap()

On Thu, Jul 30, 2015 at 2:02 PM, Luis R. Rodriguez <mcgrof@...e.com> wrote:
> On Thu, Jul 30, 2015 at 12:54:07PM -0400, Dan Williams wrote:
>> diff --git a/include/linux/io.h b/include/linux/io.h
>> index fb5a99800e77..3fcf6256c088 100644
>> --- a/include/linux/io.h
>> +++ b/include/linux/io.h
>> @@ -121,4 +121,13 @@ static inline int arch_phys_wc_index(int handle)
>>  #endif
>>  #endif
>>
>> +enum {
>> +     /* See memremap() kernel-doc for usage description... */
>> +     MEMREMAP_WB = 1 << 0,
>> +     MEMREMAP_WT = 1 << 1,
>> +};
>
> Same feedback for enum nameing and also kdoc style.

I'm concerned documentation here has the possibility of getting out of
sync with the "source of truth" at the definition of memremap().  I
think it's better to have the documentation consolidated at it
implementation rather than its definition.

>
>> diff --git a/kernel/memremap.c b/kernel/memremap.c
>> new file mode 100644
>> index 000000000000..27637f42f30d
>> --- /dev/null
>> +++ b/kernel/memremap.c
>
> <-- ... -->
>
>> +/**
>> + * memremap() - remap an iomem_resource as cacheable memory
>> + * @offset: iomem resource start address
>> + * @size: size of remap
>> + * @flags: either MEMREMAP_WB or MEMREMAP_WT
>> + *
>> + * memremap() is "ioremap" for cases where it is known that the resource
>> + * being mapped does not have i/o side effects and the __iomem
>> + * annotation is not applicable.
>> + *
>> + * MEMREMAP_WB - matches the default mapping for "System RAM" on
>> + * the architecture.  This is usually a read-allocate write-back cache.
>> + * Morever, if MEMREMAP_WB is specified and the requested remap region is RAM
>> + * memremap() will bypass establishing a new mapping and instead return
>> + * a pointer into the direct map.
>> + *
>> + * MEMREMAP_WT - establish a mapping whereby writes either bypass the
>> + * cache or are written through to memory and never exist in a
>> + * cache-dirty state with respect to program visibility.  Attempts to
>> + * map "System RAM" with this mapping type will fail.
>
> Then you can extrend all this on kdoc on the enum.
>
>> + */
>> +void *memremap(resource_size_t offset, size_t size, unsigned long flags)
>> +{
>> +     int is_ram = region_intersects(offset, size, "System RAM");
>
> This could be the enum region_intersect_type, then if the region enum is
> extended you'd get a compiler error if one type was not handled.

This already does not handle the REGION_DISJOINT case explicitly, it's
implied by handling the other 2.  A compiler warning would be verbose
for not much benefit afaics.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/