| Message ID | 1358247267-18089-2-git-send-email-tangchen@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
Hi Tang, One minor point. On Tue, Jan 15, 2013 at 9:54 PM, Tang Chen <tangchen@cn.fujitsu.com> wrote: > It is unsafe to return an entry pointer and release the map_entries_lock. So we should > not hold the map_entries_lock separately in firmware_map_find_entry() and > firmware_map_remove_entry(). Hold the map_entries_lock across find and remove > /sys/firmware/memmap/X operation. > > And also, users of these two functions need to be careful to hold the lock when using > these two functions. > > The suggestion is from Andrew Morton <akpm@linux-foundation.org> > > Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com> > --- > drivers/firmware/memmap.c | 25 +++++++++++++++++-------- > 1 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/drivers/firmware/memmap.c b/drivers/firmware/memmap.c > index 4211da5..940c4e9 100644 > --- a/drivers/firmware/memmap.c > +++ b/drivers/firmware/memmap.c > @@ -188,23 +188,28 @@ static inline void remove_sysfs_fw_map_entry(struct firmware_map_entry *entry) > } > > /* > - * Search memmap entry > + * firmware_map_find_entry: Search memmap entry. > + * @start: Start of the memory range. > + * @end: End of the memory range (exclusive). > + * @type: Type of the memory range. > + * > + * This function is to find the memmap entey of a given memory range. > + * The caller must hold map_entries_lock, and must not release the lock > + * until the processing of the returned entry has completed. > + * > + * Return pointer to the entry to be found on success, or NULL on failure. Why not make this completely kernel-doc compliant as you're already re-writing the comment? Thanks,
On 01/16/2013 06:26 AM, Julian Calaby wrote: > Hi Tang, > > One minor point. > >> >> /* >> - * Search memmap entry >> + * firmware_map_find_entry: Search memmap entry. >> + * @start: Start of the memory range. >> + * @end: End of the memory range (exclusive). >> + * @type: Type of the memory range. >> + * >> + * This function is to find the memmap entey of a given memory range. >> + * The caller must hold map_entries_lock, and must not release the lock >> + * until the processing of the returned entry has completed. >> + * >> + * Return pointer to the entry to be found on success, or NULL on failure. > > Why not make this completely kernel-doc compliant as you're already > re-writing the comment? Hi Julian, Thank you for reminding me this. I think I may have some more problems like this. I'll post a patch to fix as many of them as I can. :) Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/firmware/memmap.c b/drivers/firmware/memmap.c index 4211da5..940c4e9 100644 --- a/drivers/firmware/memmap.c +++ b/drivers/firmware/memmap.c @@ -150,12 +150,12 @@ static int firmware_map_add_entry(u64 start, u64 end, * firmware_map_remove_entry() - Does the real work to remove a firmware * memmap entry. * @entry: removed entry. + * + * The caller must hold map_entries_lock, and release it properly. **/ static inline void firmware_map_remove_entry(struct firmware_map_entry *entry) { - spin_lock(&map_entries_lock); list_del(&entry->list); - spin_unlock(&map_entries_lock); } /* @@ -188,23 +188,28 @@ static inline void remove_sysfs_fw_map_entry(struct firmware_map_entry *entry) } /* - * Search memmap entry + * firmware_map_find_entry: Search memmap entry. + * @start: Start of the memory range. + * @end: End of the memory range (exclusive). + * @type: Type of the memory range. + * + * This function is to find the memmap entey of a given memory range. + * The caller must hold map_entries_lock, and must not release the lock + * until the processing of the returned entry has completed. + * + * Return pointer to the entry to be found on success, or NULL on failure. */ - static struct firmware_map_entry * __meminit firmware_map_find_entry(u64 start, u64 end, const char *type) { struct firmware_map_entry *entry; - spin_lock(&map_entries_lock); list_for_each_entry(entry, &map_entries, list) if ((entry->start == start) && (entry->end == end) && (!strcmp(entry->type, type))) { - spin_unlock(&map_entries_lock); return entry; } - spin_unlock(&map_entries_lock); return NULL; } @@ -274,11 +279,15 @@ int __meminit firmware_map_remove(u64 start, u64 end, const char *type) { struct firmware_map_entry *entry; + spin_lock(&map_entries_lock); entry = firmware_map_find_entry(start, end - 1, type); - if (!entry) + if (!entry) { + spin_unlock(&map_entries_lock); return -EINVAL; + } firmware_map_remove_entry(entry); + spin_unlock(&map_entries_lock); /* remove the memmap entry */ remove_sysfs_fw_map_entry(entry);
It is unsafe to return an entry pointer and release the map_entries_lock. So we should not hold the map_entries_lock separately in firmware_map_find_entry() and firmware_map_remove_entry(). Hold the map_entries_lock across find and remove /sys/firmware/memmap/X operation. And also, users of these two functions need to be careful to hold the lock when using these two functions. The suggestion is from Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com> --- drivers/firmware/memmap.c | 25 +++++++++++++++++-------- 1 files changed, 17 insertions(+), 8 deletions(-)