Re: [PATCH 08/12] Drivers: hv: vmbus: Implement Direct Mode for stimer0
On Sun, Feb 11, 2018 at 05:33:16PM -0700, k...@exchange.microsoft.com wrote: > @@ -116,9 +146,29 @@ static int hv_ce_set_oneshot(struct clock_event_device > *evt) > { > union hv_timer_config timer_cfg; > > + timer_cfg.as_uint64 = 0; > timer_cfg.enable = 1; > timer_cfg.auto_enable = 1; > - timer_cfg.sintx = VMBUS_MESSAGE_SINT; > + if (direct_mode_enabled) > + /* > + * When it expires, the timer will directly interrupt > + * on the specified hardware vector/IRQ. > + */ > + { > + timer_cfg.direct_mode = 1; > + timer_cfg.apic_vector = stimer0_vector; > + hv_enable_stimer0_percpu_irq(stimer0_irq); > + } > + else > + /* > + * When it expires, the timer will generate a VMbus message, > + * to be handled by the normal VMbus interrupt handler. > + */ > + { > + timer_cfg.direct_mode = 0; > + timer_cfg.sintx = VMBUS_MESSAGE_SINT; > + } > + This indenting isn't right. We should probably zero out .apic_vector if .direct_mode is zero. Or maybe it's fine. I don't know if any static analysis tools will complain... > hv_init_timer_config(HV_X64_MSR_STIMER0_CONFIG, timer_cfg.as_uint64); > > return 0; > @@ -191,6 +241,10 @@ int hv_synic_alloc(void) > INIT_LIST_HEAD(&hv_cpu->chan_list); > } > > + if (direct_mode_enabled && hv_setup_stimer0_irq( > + &stimer0_irq, &stimer0_vector, hv_stimer0_isr)) > + goto err; Can you indent it like this: if (direct_mode_enabled && hv_setup_stimer0_irq(&stimer0_irq, &stimer0_vector, hv_stimer0_isr)) goto err; [ What follows is a long rant not directed at you ] It's annoying because as soon as I see the "goto err;", I know the error handling for this function is going to be buggy... Some rules of error handling are: 1) Each function should clean up after itself instead returning partially allocated structures. 2) Each allocation function should have a matching free function. 3) Give meaningful label names based on what the label location so that we can tell what the goto does just by looking at it, such as, "goto free_some_variable". This way we can just keep a mental tally of the most recently allocated resource and verify based on the "goto free_resource;" statemetn that it frees the correct thing. We don't need to scroll to the bottom of the function. Using good names means that we should avoid do-nothing gotos because, by definition, the label name for a do-nothing goto is going to be vague. In this case the label looks like this: > + > return 0; > err: > return -ENOMEM; We allocate a bunch of stuff in this function so at first glance this looks like we leak everything but, actually, the cleanup is done in vmbus_bus_init(). This is a layering violation. Later on, we changed hv_synic_alloc() in 37cdd991fac8 ("vmbus: put related per-cpu variable together") and we started allocating: hv_cpu->clk_evt = kzalloc(... but we forgot to update the error handling because it was in the wrong place. It's a very predictable, avoidable bug if we just use proper error handling style. Anyway... Sorry for the long rant. Summary: Always distrust vague label names. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 3/9] staging: android: ion: Nuke ion_page_pool_init
ion_page_pool.c now is used to apply pool APIs for system heap, which do not need do any initial at device_initcall. Therefore ion_page_pool_init can be nuked. Signed-off-by: Yisheng Xie --- drivers/staging/android/ion/ion_page_pool.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c index f0847b3..4452e28 100644 --- a/drivers/staging/android/ion/ion_page_pool.c +++ b/drivers/staging/android/ion/ion_page_pool.c @@ -6,7 +6,6 @@ */ #include -#include #include #include @@ -158,9 +157,3 @@ void ion_page_pool_destroy(struct ion_page_pool *pool) { kfree(pool); } - -static int __init ion_page_pool_init(void) -{ - return 0; -} -device_initcall(ion_page_pool_init); -- 1.7.12.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 6/9] staging: android: ion: Remove dead code in ion_page_pool_free
ion_page_pool_add will always return 0, however ion_page_pool_free will call ion_page_pool_free_pages when ion_page_pool_add's return value is not 0, so it is a dead code which can be removed. Acked-by: Laura Abbott Signed-off-by: Yisheng Xie --- drivers/staging/android/ion/ion_page_pool.c | 6 +- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c index 4452e28..150626f 100644 --- a/drivers/staging/android/ion/ion_page_pool.c +++ b/drivers/staging/android/ion/ion_page_pool.c @@ -79,13 +79,9 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool) void ion_page_pool_free(struct ion_page_pool *pool, struct page *page) { - int ret; - BUG_ON(pool->order != compound_order(page)); - ret = ion_page_pool_add(pool, page); - if (ret) - ion_page_pool_free_pages(pool, page); + ion_page_pool_add(pool, page); } static int ion_page_pool_total(struct ion_page_pool *pool, bool high) -- 1.7.12.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 4/9] staging: android: ion: Avoid NULL point in error path
If we failed to create debugfs for ion at ion_device_create, the debug_root of ion_device will be NULL, and then when try to create debug file for shrinker of heap it will be create on the top of debugfs. If we also failed to create this the debug file, it call dentry_path to found the path of debug_root, then a NULL point will occur. Fix this by avoiding call dentry_path, but show the debug name only when failed to create debug file for shrinker. Acked-by: Laura Abbott Signed-off-by: Yisheng Xie --- drivers/staging/android/ion/ion.c | 10 +++--- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index 57e0d80..4b69372 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -564,13 +564,9 @@ void ion_device_add_heap(struct ion_heap *heap) debug_file = debugfs_create_file(debug_name, 0644, dev->debug_root, heap, &debug_shrink_fops); - if (!debug_file) { - char buf[256], *path; - - path = dentry_path(dev->debug_root, buf, 256); - pr_err("Failed to create heap shrinker debugfs at %s/%s\n", - path, debug_name); - } + if (!debug_file) + pr_err("Failed to create ion heap shrinker debugfs at %s\n", + debug_name); } dev->heap_cnt++; -- 1.7.12.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/9] staging: android: ion: Remove unused declaration ion_buffer_fault_user_mappings
ion_buffer_fault_user_mappings's definition has been removed and not be used anymore, just remove its useless declaration. Acked-by: Laura Abbott Signed-off-by: Yisheng Xie --- drivers/staging/android/ion/ion.h | 9 - 1 file changed, 9 deletions(-) diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h index a238f23..1bc443f 100644 --- a/drivers/staging/android/ion/ion.h +++ b/drivers/staging/android/ion/ion.h @@ -193,15 +193,6 @@ struct ion_heap { bool ion_buffer_cached(struct ion_buffer *buffer); /** - * ion_buffer_fault_user_mappings - fault in user mappings of this buffer - * @buffer:buffer - * - * indicates whether userspace mappings of this buffer will be faulted - * in, this can affect how buffers are allocated from the heap. - */ -bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer); - -/** * ion_device_add_heap - adds a heap to the ion device * @heap: the heap to add */ -- 1.7.12.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 0/9] staging: android: ion: Some cleanup about ion
Hi all, When I tried to review the code of ion, I find that there are many place can be cleanup, so I post this patchset. I mark it as v2, because I have post them in different patchset before[1][2] [3][4]. And now, I combine them in one patchset to make it easy to review, Meanwhile, I add some acks and rebase them to v4.15-rc1. Thanks. [1] https://lkml.org/lkml/2018/1/31/711 [2] https://lkml.org/lkml/2018/2/1/240 [3] https://lkml.org/lkml/2018/2/4/320 [4] https://lkml.org/lkml/2018/2/6/949 Yisheng Xie (9): staging: android: ion: Remove unused declaration ion_buffer_fault_user_mappings staging: android: ion: Remove unused include files for ion_page_pool.c staging: android: ion: Nuke ion_page_pool_init staging: android: ion: Avoid NULL point in error path staging: android: ion: Remove lable debugfs_done staging: android: ion: Remove dead code in ion_page_pool_free staging: android: ion: Return void instead of int staging: android: ion: Cleanup ion_page_pool_alloc_pages staging: android: ion: Combine cache and uncache pools drivers/staging/android/ion/ion.c | 20 ++- drivers/staging/android/ion/ion.h | 22 +--- drivers/staging/android/ion/ion_page_pool.c | 33 ++-- drivers/staging/android/ion/ion_system_heap.c | 76 +-- 4 files changed, 24 insertions(+), 127 deletions(-) -- 1.7.12.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 5/9] staging: android: ion: Remove lable debugfs_done
When failed to create debug_root, we will go on initail other part of ion, so we can just info this message to user and do not need a lable to jump. Acked-by: Laura Abbott Signed-off-by: Yisheng Xie --- drivers/staging/android/ion/ion.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index 4b69372..461b193 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -595,12 +595,9 @@ static int ion_device_create(void) } idev->debug_root = debugfs_create_dir("ion", NULL); - if (!idev->debug_root) { + if (!idev->debug_root) pr_err("ion: failed to create debugfs root directory.\n"); - goto debugfs_done; - } -debugfs_done: idev->buffers = RB_ROOT; mutex_init(&idev->buffer_lock); init_rwsem(&idev->lock); -- 1.7.12.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 2/9] staging: android: ion: Remove unused include files for ion_page_pool.c
After rewrite of ion_page_pool, some of its include file is no need anymore, just remove it. Signed-off-by: Yisheng Xie --- drivers/staging/android/ion/ion_page_pool.c | 4 1 file changed, 4 deletions(-) diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c index b3017f1..f0847b3 100644 --- a/drivers/staging/android/ion/ion_page_pool.c +++ b/drivers/staging/android/ion/ion_page_pool.c @@ -5,10 +5,6 @@ * Copyright (C) 2011 Google, Inc. */ -#include -#include -#include -#include #include #include #include -- 1.7.12.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 7/9] staging: android: ion: Return void instead of int
Now, nobody care about the return value of ion_page_pool_add, therefore we can just make it return void. Acked-by: Laura Abbott Signed-off-by: Yisheng Xie --- drivers/staging/android/ion/ion_page_pool.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c index 150626f..e3a6e32 100644 --- a/drivers/staging/android/ion/ion_page_pool.c +++ b/drivers/staging/android/ion/ion_page_pool.c @@ -26,7 +26,7 @@ static void ion_page_pool_free_pages(struct ion_page_pool *pool, __free_pages(page, pool->order); } -static int ion_page_pool_add(struct ion_page_pool *pool, struct page *page) +static void ion_page_pool_add(struct ion_page_pool *pool, struct page *page) { mutex_lock(&pool->mutex); if (PageHighMem(page)) { @@ -37,7 +37,6 @@ static int ion_page_pool_add(struct ion_page_pool *pool, struct page *page) pool->low_count++; } mutex_unlock(&pool->mutex); - return 0; } static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high) -- 1.7.12.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 9/9] staging: android: ion: Combine cache and uncache pools
Now we call dma_map in the dma_buf API callbacks and handle explicit caching by the dma_buf sync API, which make cache and uncache pools in the same handling flow, which can be combined. Acked-by: Sumit Semwal Signed-off-by: Yisheng Xie --- drivers/staging/android/ion/ion.c | 5 -- drivers/staging/android/ion/ion.h | 13 + drivers/staging/android/ion/ion_page_pool.c | 5 +- drivers/staging/android/ion/ion_system_heap.c | 76 +-- 4 files changed, 16 insertions(+), 83 deletions(-) diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index 461b193..c094be2 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -33,11 +33,6 @@ static struct ion_device *internal_dev; static int heap_id; -bool ion_buffer_cached(struct ion_buffer *buffer) -{ - return !!(buffer->flags & ION_FLAG_CACHED); -} - /* this function should only be called while dev->lock is held */ static void ion_buffer_add(struct ion_device *dev, struct ion_buffer *buffer) diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h index 1bc443f..ea08978 100644 --- a/drivers/staging/android/ion/ion.h +++ b/drivers/staging/android/ion/ion.h @@ -185,14 +185,6 @@ struct ion_heap { }; /** - * ion_buffer_cached - this ion buffer is cached - * @buffer:buffer - * - * indicates whether this ion buffer is cached - */ -bool ion_buffer_cached(struct ion_buffer *buffer); - -/** * ion_device_add_heap - adds a heap to the ion device * @heap: the heap to add */ @@ -302,7 +294,6 @@ size_t ion_heap_freelist_shrink(struct ion_heap *heap, * @gfp_mask: gfp_mask to use from alloc * @order: order of pages in the pool * @list: plist node for list of pools - * @cached:it's cached pool or not * * Allows you to keep a pool of pre allocated pages to use from your heap. * Keeping a pool of pages that is ready for dma, ie any cached mapping have @@ -312,7 +303,6 @@ size_t ion_heap_freelist_shrink(struct ion_heap *heap, struct ion_page_pool { int high_count; int low_count; - bool cached; struct list_head high_items; struct list_head low_items; struct mutex mutex; @@ -321,8 +311,7 @@ struct ion_page_pool { struct plist_node list; }; -struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order, - bool cached); +struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order); void ion_page_pool_destroy(struct ion_page_pool *pool); struct page *ion_page_pool_alloc(struct ion_page_pool *pool); void ion_page_pool_free(struct ion_page_pool *pool, struct page *page); diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c index 6d2caf0..db8f614 100644 --- a/drivers/staging/android/ion/ion_page_pool.c +++ b/drivers/staging/android/ion/ion_page_pool.c @@ -123,8 +123,7 @@ int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask, return freed; } -struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order, - bool cached) +struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order) { struct ion_page_pool *pool = kmalloc(sizeof(*pool), GFP_KERNEL); @@ -138,8 +137,6 @@ struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order, pool->order = order; mutex_init(&pool->mutex); plist_node_init(&pool->list, order); - if (cached) - pool->cached = true; return pool; } diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c index bc19cdd..701eb9f 100644 --- a/drivers/staging/android/ion/ion_system_heap.c +++ b/drivers/staging/android/ion/ion_system_heap.c @@ -41,31 +41,16 @@ static inline unsigned int order_to_size(int order) struct ion_system_heap { struct ion_heap heap; - struct ion_page_pool *uncached_pools[NUM_ORDERS]; - struct ion_page_pool *cached_pools[NUM_ORDERS]; + struct ion_page_pool *pools[NUM_ORDERS]; }; -/** - * The page from page-pool are all zeroed before. We need do cache - * clean for cached buffer. The uncached buffer are always non-cached - * since it's allocated. So no need for non-cached pages. - */ static struct page *alloc_buffer_page(struct ion_system_heap *heap, struct ion_buffer *buffer, unsigned long order) { - bool cached = ion_buffer_cached(buffer); - struct ion_page_pool *pool; - struct page *page; + struct ion_page_pool *pool = heap->pools[order_to_index(order)]; - if (!cached) - pool = heap->uncached_pools[order_to_index(order)]
[PATCH v2 8/9] staging: android: ion: Cleanup ion_page_pool_alloc_pages
ion_page_pool_alloc_pages calls alloc_pages to allocate pages for page pools. If alloc_pages return NULL, it will return NULL, or it will return the pages allocate from alloc_pages. So we can just return alloc_pages without any judgement. Acked-by: Sumit Semwal Signed-off-by: Yisheng Xie --- drivers/staging/android/ion/ion_page_pool.c | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c index e3a6e32..6d2caf0 100644 --- a/drivers/staging/android/ion/ion_page_pool.c +++ b/drivers/staging/android/ion/ion_page_pool.c @@ -11,13 +11,9 @@ #include "ion.h" -static void *ion_page_pool_alloc_pages(struct ion_page_pool *pool) +static inline struct page *ion_page_pool_alloc_pages(struct ion_page_pool *pool) { - struct page *page = alloc_pages(pool->gfp_mask, pool->order); - - if (!page) - return NULL; - return page; + return alloc_pages(pool->gfp_mask, pool->order); } static void ion_page_pool_free_pages(struct ion_page_pool *pool, -- 1.7.12.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/9] staging: android: ion: Some cleanup about ion
On Mon, Feb 12, 2018 at 06:43:05PM +0800, Yisheng Xie wrote: > Hi all, > > When I tried to review the code of ion, I find that there are many place > can be cleanup, so I post this patchset. > > I mark it as v2, because I have post them in different patchset before[1][2] > [3][4]. And now, I combine them in one patchset to make it easy to review, > Meanwhile, I add some acks and rebase them to v4.15-rc1. > > Thanks. > > [1] https://lkml.org/lkml/2018/1/31/711 > [2] https://lkml.org/lkml/2018/2/1/240 > [3] https://lkml.org/lkml/2018/2/4/320 > [4] https://lkml.org/lkml/2018/2/6/949 > Could you go back and reply to these threads that you have redone the patches? Otherwise, what's likely to happen is that Greg will apply as many as he can get to this series and it won't apply. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: vt6656: Remove unnecessary 'out of memory' message
This patch removes the unnecessary out of memory message fixing the following checkpatch.pl warning in usbpipe.c: WARNING: Possible unnecessary 'out of memory' message Signed-off-by: Dileep Sankhla --- Change in v2: - changed commit message --- drivers/staging/vt6656/usbpipe.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/staging/vt6656/usbpipe.c b/drivers/staging/vt6656/usbpipe.c index 273176386a51..5bbc56f8779e 100644 --- a/drivers/staging/vt6656/usbpipe.c +++ b/drivers/staging/vt6656/usbpipe.c @@ -194,9 +194,6 @@ static void vnt_submit_rx_urb_complete(struct urb *urb) if (vnt_rx_data(priv, rcb, urb->actual_length)) { rcb->skb = dev_alloc_skb(priv->rx_buf_sz); if (!rcb->skb) { - dev_dbg(&priv->usb->dev, - "Failed to re-alloc rx skb\n"); - rcb->in_use = false; return; } -- 2.15.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/9] staging: android: ion: Some cleanup about ion
Hi Dan, On 2018/2/12 19:17, Dan Carpenter wrote: > On Mon, Feb 12, 2018 at 06:43:05PM +0800, Yisheng Xie wrote: >> Hi all, >> >> When I tried to review the code of ion, I find that there are many place >> can be cleanup, so I post this patchset. >> >> I mark it as v2, because I have post them in different patchset before[1][2] >> [3][4]. And now, I combine them in one patchset to make it easy to review, >> Meanwhile, I add some acks and rebase them to v4.15-rc1. >> >> Thanks. >> >> [1] https://lkml.org/lkml/2018/1/31/711 >> [2] https://lkml.org/lkml/2018/2/1/240 >> [3] https://lkml.org/lkml/2018/2/4/320 >> [4] https://lkml.org/lkml/2018/2/6/949 >> > > Could you go back and reply to these threads that you have redone the > patches? Otherwise, what's likely to happen is that Greg will apply > as many as he can get to this series and it won't apply. Sure! thanks Yisheng > > regards, > dan carpenter > > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH resend 1/3] staging: android: ion: Remove unused declaration ion_buffer_fault_user_mappings
Hi Greg, JFYI, I have rebase this patchset to v4.15-rc1.[1] [1] https://lkml.org/lkml/2018/2/12/204 Thanks Yisheng On 2018/2/1 9:54, Yisheng Xie wrote: > ion_buffer_fault_user_mappings's definition has been removed and not be > used anymore, just remove its useless declaration. > > Signed-off-by: Yisheng Xie > --- > drivers/staging/android/ion/ion.h | 9 - > 1 file changed, 9 deletions(-) > > diff --git a/drivers/staging/android/ion/ion.h > b/drivers/staging/android/ion/ion.h > index f5f9cd6..2160c35 100644 > --- a/drivers/staging/android/ion/ion.h > +++ b/drivers/staging/android/ion/ion.h > @@ -201,15 +201,6 @@ struct ion_heap { > bool ion_buffer_cached(struct ion_buffer *buffer); > > /** > - * ion_buffer_fault_user_mappings - fault in user mappings of this buffer > - * @buffer: buffer > - * > - * indicates whether userspace mappings of this buffer will be faulted > - * in, this can affect how buffers are allocated from the heap. > - */ > -bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer); > - > -/** > * ion_device_add_heap - adds a heap to the ion device > * @heap:the heap to add > */ > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: android: ion: Avoid NULL point in error path
Hi Greg, JFYI, I have rebase this patchset to v4.15-rc1.[1] [1] https://lkml.org/lkml/2018/2/12/204 Thanks Yisheng On 2018/2/1 20:34, Yisheng Xie wrote: > If we failed to create debugfs for ion at ion_device_create, the > debug_root of ion_device will be NULL, and then when try to create debug > file for shrinker of heap it will be create on the top of debugfs. If we > also failed to create this the debug file, it call dentry_path to found > the path of debug_root, then a NULL point will occur. > > Fix this by avoiding call dentry_path, but show the debug name only when > failed to create debug file for shrinker. > > Signed-off-by: Yisheng Xie > --- > drivers/staging/android/ion/ion.c | 10 +++--- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/staging/android/ion/ion.c > b/drivers/staging/android/ion/ion.c > index f480885..3e41644 100644 > --- a/drivers/staging/android/ion/ion.c > +++ b/drivers/staging/android/ion/ion.c > @@ -570,13 +570,9 @@ void ion_device_add_heap(struct ion_heap *heap) > debug_file = debugfs_create_file( > debug_name, 0644, dev->debug_root, heap, > &debug_shrink_fops); > - if (!debug_file) { > - char buf[256], *path; > - > - path = dentry_path(dev->debug_root, buf, 256); > - pr_err("Failed to create heap shrinker debugfs at > %s/%s\n", > -path, debug_name); > - } > + if (!debug_file) > + pr_err("Failed to create ion heap shrinker debugfs at > %s\n", > +debug_name); > } > > dev->heap_cnt++; > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: android: ion: Remove dead code in ion_page_pool_free
Hi Greg, JFYI, I have rebase this patchset to v4.15-rc1.[1] [1] https://lkml.org/lkml/2018/2/12/204 Thanks Yisheng On 2018/2/5 11:26, Yisheng Xie wrote: > ion_page_pool_add will always return 0, however ion_page_pool_free will > call ion_page_pool_free_pages when ion_page_pool_add's return value is > not 0, so it is a dead code which can be removed. > > Signed-off-by: Yisheng Xie > --- > drivers/staging/android/ion/ion_page_pool.c | 6 +- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/staging/android/ion/ion_page_pool.c > b/drivers/staging/android/ion/ion_page_pool.c > index 4452e28..150626f 100644 > --- a/drivers/staging/android/ion/ion_page_pool.c > +++ b/drivers/staging/android/ion/ion_page_pool.c > @@ -79,13 +79,9 @@ struct page *ion_page_pool_alloc(struct ion_page_pool > *pool) > > void ion_page_pool_free(struct ion_page_pool *pool, struct page *page) > { > - int ret; > - > BUG_ON(pool->order != compound_order(page)); > > - ret = ion_page_pool_add(pool, page); > - if (ret) > - ion_page_pool_free_pages(pool, page); > + ion_page_pool_add(pool, page); > } > > static int ion_page_pool_total(struct ion_page_pool *pool, bool high) > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: android: ion: Cleanup ion_page_pool_alloc_pages
Hi Greg, JFYI, I have rebase this patchset to v4.15-rc1.[1] [1] https://lkml.org/lkml/2018/2/12/204 Thanks Yisheng On 2018/2/7 11:59, Yisheng Xie wrote: > ion_page_pool_alloc_pages calls alloc_pages to allocate pages for page > pools. If alloc_pages return NULL, it will return NULL, or it will > return the pages allocate from alloc_pages. So we can just return > alloc_pages without any judgement. > > Signed-off-by: Yisheng Xie > --- > drivers/staging/android/ion/ion_page_pool.c | 8 ++-- > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/android/ion/ion_page_pool.c > b/drivers/staging/android/ion/ion_page_pool.c > index e3a6e32..6d2caf0 100644 > --- a/drivers/staging/android/ion/ion_page_pool.c > +++ b/drivers/staging/android/ion/ion_page_pool.c > @@ -11,13 +11,9 @@ > > #include "ion.h" > > -static void *ion_page_pool_alloc_pages(struct ion_page_pool *pool) > +static inline struct page *ion_page_pool_alloc_pages(struct ion_page_pool > *pool) > { > - struct page *page = alloc_pages(pool->gfp_mask, pool->order); > - > - if (!page) > - return NULL; > - return page; > + return alloc_pages(pool->gfp_mask, pool->order); > } > > static void ion_page_pool_free_pages(struct ion_page_pool *pool, > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/4] staging: iio: accel: adis16201 driver cleanup
The following patch series cleans up miscellaneous code fragments and then the driver is moved from staging to mainline IIO subsytem directory. Note that the last patch to move driver is *not* generated using '-M' flag, which is used for detecting renames, since it may help reviewers to suggest more cleanups/fix while pointing inline to the patch the necessary changes. All the patches have been tested using 0-day test service with success. For all the patches: Suggested-by: Jonathan Cameron Himanshu Jha (4): staging: iio: accel: adis16201: Use SPDX identifier staging: iio: accel: Remove unnecessary comments and add suitable suffix staging: iio: accel: Use sign_extend32 and adjust a switch statement staging: iio: accel: Move adis16201 driver out of staging drivers/iio/accel/Kconfig | 12 ++ drivers/iio/accel/Makefile| 1 + drivers/iio/accel/adis16201.c | 315 drivers/staging/iio/accel/Kconfig | 12 -- drivers/staging/iio/accel/Makefile| 1 - drivers/staging/iio/accel/adis16201.c | 381 -- 6 files changed, 328 insertions(+), 394 deletions(-) create mode 100644 drivers/iio/accel/adis16201.c delete mode 100644 drivers/staging/iio/accel/adis16201.c -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
Move the adis16201 driver out of staging directory and merge to the mainline IIO directory. Signed-off-by: Himanshu Jha --- drivers/iio/accel/Kconfig | 12 ++ drivers/iio/accel/Makefile| 1 + drivers/iio/accel/adis16201.c | 315 ++ drivers/staging/iio/accel/Kconfig | 12 -- drivers/staging/iio/accel/Makefile| 1 - drivers/staging/iio/accel/adis16201.c | 315 -- 6 files changed, 328 insertions(+), 328 deletions(-) create mode 100644 drivers/iio/accel/adis16201.c delete mode 100644 drivers/staging/iio/accel/adis16201.c diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig index c6d9517..9416c6f 100644 --- a/drivers/iio/accel/Kconfig +++ b/drivers/iio/accel/Kconfig @@ -5,6 +5,18 @@ menu "Accelerometers" +config ADIS16201 +tristate "Analog Devices ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer" +depends on SPI +select IIO_ADIS_LIB +select IIO_ADIS_LIB_BUFFER if IIO_BUFFER +help + Say Y here to build support for Analog Devices adis16201 dual-axis + digital inclinometer and accelerometer. + + To compile this driver as a module, say M here: the module will + be called adis16201. + config ADXL345 tristate diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile index 368aedb..7832ec9 100644 --- a/drivers/iio/accel/Makefile +++ b/drivers/iio/accel/Makefile @@ -4,6 +4,7 @@ # # When adding new entries keep the list in alphabetical order +obj-$(CONFIG_ADIS16201) += adis16201.o obj-$(CONFIG_ADXL345) += adxl345_core.o obj-$(CONFIG_ADXL345_I2C) += adxl345_i2c.o obj-$(CONFIG_ADXL345_SPI) += adxl345_spi.o diff --git a/drivers/iio/accel/adis16201.c b/drivers/iio/accel/adis16201.c new file mode 100644 index 000..6800347 --- /dev/null +++ b/drivers/iio/accel/adis16201.c @@ -0,0 +1,315 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer + * + * Copyright 2010 Analog Devices Inc. + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +#define ADIS16201_STARTUP_DELAY_MS 220 +#define ADIS16201_FLASH_CNT_REG0x00 + +/* Data Output Register Definitions */ +#define ADIS16201_SUPPLY_OUT_REG 0x02 +#define ADIS16201_XACCL_OUT_REG0x04 +#define ADIS16201_YACCL_OUT_REG0x06 +#define ADIS16201_AUX_ADC_REG 0x08 +#define ADIS16201_TEMP_OUT_REG 0x0A +#define ADIS16201_XINCL_OUT_REG0x0C +#define ADIS16201_YINCL_OUT_REG0x0E + +/* Calibration Register Definitions */ +#define ADIS16201_XACCL_OFFS_REG 0x10 +#define ADIS16201_YACCL_OFFS_REG 0x12 +#define ADIS16201_XACCL_SCALE_REG 0x14 +#define ADIS16201_YACCL_SCALE_REG 0x16 +#define ADIS16201_XINCL_OFFS_REG 0x18 +#define ADIS16201_YINCL_OFFS_REG 0x1A +#define ADIS16201_XINCL_SCALE_REG 0x1C +#define ADIS16201_YINCL_SCALE_REG 0x1E + +/* Alarm Register Definitions */ +#define ADIS16201_ALM_MAG1_REG 0x20 +#define ADIS16201_ALM_MAG2_REG 0x22 +#define ADIS16201_ALM_SMPL1_REG0x24 +#define ADIS16201_ALM_SMPL2_REG0x26 +#define ADIS16201_ALM_CTRL_REG 0x28 + +#define ADIS16201_AUX_DAC_REG 0x30 +#define ADIS16201_GPIO_CTRL_REG0x32 +#define ADIS16201_SMPL_PRD_REG 0x36 +#define ADIS16201_AVG_CNT_REG 0x38 +#define ADIS16201_SLP_CNT_REG 0x3A + +/* MSC_CTRL */ +#define ADIS16201_MSC_CTRL_REG 0x34 +#define ADIS16201_MSC_CTRL_SELF_TEST_ENBIT(8) +#define ADIS16201_MSC_CTRL_DATA_RDY_EN BIT(2) +#define ADIS16201_MSC_CTRL_ACTIVE_HIGH BIT(1) +#define ADIS16201_MSC_CTRL_DATA_RDY_DIO1 BIT(0) + +/* DIAG_STAT */ +#define ADIS16201_DIAG_STAT_REG0x3C +#define ADIS16201_DIAG_STAT_ALARM2 BIT(9) +#define ADIS16201_DIAG_STAT_ALARM1 BIT(8) +#define ADIS16201_DIAG_STAT_SPI_FAIL_BIT 3 +#define ADIS16201_DIAG_STAT_FLASH_UPT_BIT 2 +#define ADIS16201_DIAG_STAT_POWER_HIGH_BIT 1 +#define ADIS16201_DIAG_STAT_POWER_LOW_BIT 0 + +/* GLOB_CMD */ +#define ADIS16201_GLOB_CMD_REG 0x3E +#define ADIS16201_GLOB_CMD_SW_RESETBIT(7) +#define ADIS16201_GLOB_CMD_FACTORY_CAL BIT(1) + +#define ADIS16201_ERROR_ACTIVE BIT(14) + +enum adis16201_scan { + ADIS16201_SCAN_ACC_X, + ADIS16201_SCAN_ACC_Y, + ADIS16201_SCAN_INCLI_X, + ADIS16201_SCAN_INCLI_Y, + ADIS16201_SCAN_SUPPLY, + ADIS16201_SCAN_AUX_ADC
[PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement
Use sign_extend32 function instead of manually coding it. Also, adjust a switch block to explicitly match channels and return -EINVAL as default case which improves code readability. Signed-off-by: Himanshu Jha --- drivers/staging/iio/accel/adis16201.c | 13 - 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/staging/iio/accel/adis16201.c b/drivers/staging/iio/accel/adis16201.c index 011d2c5..6800347 100644 --- a/drivers/staging/iio/accel/adis16201.c +++ b/drivers/staging/iio/accel/adis16201.c @@ -112,12 +112,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev, case IIO_CHAN_INFO_SCALE: switch (chan->type) { case IIO_VOLTAGE: - if (chan->channel == 0) { + switch (chan->channel) { + case 0: *val = 1; *val2 = 22; - } else { + break; + case 1: *val = 0; *val2 = 61; + break; + default: + return -EINVAL; } return IIO_VAL_INT_PLUS_MICRO; case IIO_TEMP: @@ -155,9 +160,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev, if (ret) return ret; - val16 &= (1 << bits) - 1; - val16 = (s16)(val16 << (16 - bits)) >> (16 - bits); - *val = val16; + *val = sign_extend32(val16, bits - 1); return IIO_VAL_INT; } -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/4] staging: iio: accel: adis16201: Use SPDX identifier
Use SPDX identifier format instead of GPLv2. Also, rearrange the headers in the alphabetical order. Signed-off-by: Himanshu Jha --- drivers/staging/iio/accel/adis16201.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/staging/iio/accel/adis16201.c b/drivers/staging/iio/accel/adis16201.c index 2ebd275..b2145c9 100644 --- a/drivers/staging/iio/accel/adis16201.c +++ b/drivers/staging/iio/accel/adis16201.c @@ -1,19 +1,18 @@ +// SPDX-License-Identifier: GPL-2.0+ /* * ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer * * Copyright 2010 Analog Devices Inc. - * - * Licensed under the GPL-2 or later. */ #include -#include #include #include +#include +#include #include #include #include -#include #include #include -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
Remove some unnecessary comments by giving appropriate name to macros. Therefore, add _REG suffix for control registers. Also, align function arguments to match open parentheses using tabs. Group the control register and register field macros together. Blank line before some returns improves code readability. Signed-off-by: Himanshu Jha --- drivers/staging/iio/accel/adis16201.c | 226 -- 1 file changed, 79 insertions(+), 147 deletions(-) diff --git a/drivers/staging/iio/accel/adis16201.c b/drivers/staging/iio/accel/adis16201.c index b2145c9..011d2c5 100644 --- a/drivers/staging/iio/accel/adis16201.c +++ b/drivers/staging/iio/accel/adis16201.c @@ -19,135 +19,63 @@ #include #include -#define ADIS16201_STARTUP_DELAY220 /* ms */ - -/* Flash memory write count */ -#define ADIS16201_FLASH_CNT 0x00 - -/* Output, power supply */ -#define ADIS16201_SUPPLY_OUT 0x02 - -/* Output, x-axis accelerometer */ -#define ADIS16201_XACCL_OUT 0x04 - -/* Output, y-axis accelerometer */ -#define ADIS16201_YACCL_OUT 0x06 - -/* Output, auxiliary ADC input */ -#define ADIS16201_AUX_ADC0x08 - -/* Output, temperature */ -#define ADIS16201_TEMP_OUT 0x0A - -/* Output, x-axis inclination */ -#define ADIS16201_XINCL_OUT 0x0C - -/* Output, y-axis inclination */ -#define ADIS16201_YINCL_OUT 0x0E - -/* Calibration, x-axis acceleration offset */ -#define ADIS16201_XACCL_OFFS 0x10 - -/* Calibration, y-axis acceleration offset */ -#define ADIS16201_YACCL_OFFS 0x12 - -/* x-axis acceleration scale factor */ -#define ADIS16201_XACCL_SCALE0x14 - -/* y-axis acceleration scale factor */ -#define ADIS16201_YACCL_SCALE0x16 - -/* Calibration, x-axis inclination offset */ -#define ADIS16201_XINCL_OFFS 0x18 - -/* Calibration, y-axis inclination offset */ -#define ADIS16201_YINCL_OFFS 0x1A - -/* x-axis inclination scale factor */ -#define ADIS16201_XINCL_SCALE0x1C - -/* y-axis inclination scale factor */ -#define ADIS16201_YINCL_SCALE0x1E - -/* Alarm 1 amplitude threshold */ -#define ADIS16201_ALM_MAG1 0x20 - -/* Alarm 2 amplitude threshold */ -#define ADIS16201_ALM_MAG2 0x22 - -/* Alarm 1, sample period */ -#define ADIS16201_ALM_SMPL1 0x24 - -/* Alarm 2, sample period */ -#define ADIS16201_ALM_SMPL2 0x26 - -/* Alarm control */ -#define ADIS16201_ALM_CTRL 0x28 - -/* Auxiliary DAC data */ -#define ADIS16201_AUX_DAC0x30 - -/* General-purpose digital input/output control */ -#define ADIS16201_GPIO_CTRL 0x32 - -/* Miscellaneous control */ -#define ADIS16201_MSC_CTRL 0x34 - -/* Internal sample period (rate) control */ -#define ADIS16201_SMPL_PRD 0x36 - -/* Operation, filter configuration */ -#define ADIS16201_AVG_CNT0x38 - -/* Operation, sleep mode control */ -#define ADIS16201_SLP_CNT0x3A - -/* Diagnostics, system status register */ -#define ADIS16201_DIAG_STAT 0x3C - -/* Operation, system command register */ -#define ADIS16201_GLOB_CMD 0x3E +#define ADIS16201_STARTUP_DELAY_MS 220 +#define ADIS16201_FLASH_CNT_REG0x00 + +/* Data Output Register Definitions */ +#define ADIS16201_SUPPLY_OUT_REG 0x02 +#define ADIS16201_XACCL_OUT_REG0x04 +#define ADIS16201_YACCL_OUT_REG0x06 +#define ADIS16201_AUX_ADC_REG 0x08 +#define ADIS16201_TEMP_OUT_REG 0x0A +#define ADIS16201_XINCL_OUT_REG0x0C +#define ADIS16201_YINCL_OUT_REG0x0E + +/* Calibration Register Definitions */ +#define ADIS16201_XACCL_OFFS_REG 0x10 +#define ADIS16201_YACCL_OFFS_REG 0x12 +#define ADIS16201_XACCL_SCALE_REG 0x14 +#define ADIS16201_YACCL_SCALE_REG 0x16 +#define ADIS16201_XINCL_OFFS_REG 0x18 +#define ADIS16201_YINCL_OFFS_REG 0x1A +#define ADIS16201_XINCL_SCALE_REG 0x1C +#define ADIS16201_YINCL_SCALE_REG 0x1E + +/* Alarm Register Definitions */ +#define ADIS16201_ALM_MAG1_REG 0x20 +#define ADIS16201_ALM_MAG2_REG 0x22 +#define ADIS16201_ALM_SMPL1_REG0x24 +#define ADIS16201_ALM_SMPL2_REG0x26 +#define ADIS16201_ALM_CTRL_REG 0x28 + +#define ADIS16201_AUX_DAC_REG 0x30 +#define ADIS16201_GPIO_CTRL_REG0x32 +#define ADIS16201_SMPL_PRD_REG 0x36 +#define ADIS16201_AVG_CNT_REG 0x38 +#define ADIS16201_SLP_CNT_REG 0x3A /* MSC_CTRL */ - -/* Self-test enable */ -#define ADIS16201_MSC_CTRL_SELF_TEST_ENBIT(8) - -/* Data-ready enable: 1 = enabled, 0 = disabled */ -#define ADIS16201_MSC_CTRL_DATA_RDY_EN BIT(2) - -/* Data-ready polarity: 1 = active high, 0 = active low */ -#define ADIS16201_MSC_CTRL_ACTIVE_HIGH BIT(1) - -/
[PATCH] staging: media: reformat line to 80 chars or less
This is a cleanup patch to fix line length issue found by checkpatch.pl script. In this patch, line 144 have been wrapped. Signed-off-by: Parthiban Nallathambi --- drivers/staging/media/imx/imx-media-capture.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c index 576bdc7e9c42..0ccabe04b0e1 100644 --- a/drivers/staging/media/imx/imx-media-capture.c +++ b/drivers/staging/media/imx/imx-media-capture.c @@ -141,7 +141,8 @@ static int capture_enum_frameintervals(struct file *file, void *fh, fie.code = cc->codes[0]; - ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval, NULL, &fie); + ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval, + NULL, &fie); if (ret) return ret; -- 2.14.3 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
On Mon, Feb 12, 2018 at 05:24:57PM +0530, Himanshu Jha wrote: > Remove some unnecessary comments by giving appropriate name to macros. > Therefore, add _REG suffix for control registers. Also, align function > arguments to match open parentheses using tabs. > Group the control register and register field macros together. > > Blank line before some returns improves code readability. > > Signed-off-by: Himanshu Jha The most obvious response is that this needs to be broken up into multiple patches. I think I liked almost all the comments... > -/* Output, power supply */ > -#define ADIS16201_SUPPLY_OUT 0x02 > +#define ADIS16201_SUPPLY_OUT_REG 0x02 To me it seems useful to know that we're talking about the power supply. Adding _REG to the name seems not terribly useful and it makes the name longer so we're going to run into the 80 character limit. I just checked and this patch does add some checkpatch warnings. But aligning the arguments is fine, deleting unused macros is fine, adding blank lines is fine. > static int adis16201_probe(struct spi_device *spi) > { > - int ret; > - struct adis *st; > struct iio_dev *indio_dev; > + struct adis *st; > + int ret; Making this reverse Christmas tree is fine. But these things should all be done in separate patches. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement
On Mon, Feb 12, 2018 at 05:24:58PM +0530, Himanshu Jha wrote: > Use sign_extend32 function instead of manually coding it. Also, adjust a ^ > switch block to explicitly match channels and return -EINVAL as default > case which improves code readability. Greg likes to say something along the lines of "when you start your sentence with "Also, " that could be a clue that it should be a separate patch.". > > Signed-off-by: Himanshu Jha > --- > drivers/staging/iio/accel/adis16201.c | 13 - > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/staging/iio/accel/adis16201.c > b/drivers/staging/iio/accel/adis16201.c > index 011d2c5..6800347 100644 > --- a/drivers/staging/iio/accel/adis16201.c > +++ b/drivers/staging/iio/accel/adis16201.c > @@ -112,12 +112,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev, > case IIO_CHAN_INFO_SCALE: > switch (chan->type) { > case IIO_VOLTAGE: > - if (chan->channel == 0) { > + switch (chan->channel) { > + case 0: > *val = 1; > *val2 = 22; > - } else { > + break; > + case 1: > *val = 0; > *val2 = 61; > + break; > + default: > + return -EINVAL; > } I don't think this improves readability. The -EINVAL is to handle a driver bug which we haven't introduced yet. Probably we would be better off printing a warning or something? But it feels weird to introduce so much code to handle a bug which would actually be pretty difficult to write. The original code is fine. > return IIO_VAL_INT_PLUS_MICRO; > case IIO_TEMP: > @@ -155,9 +160,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev, > if (ret) > return ret; > > - val16 &= (1 << bits) - 1; > - val16 = (s16)(val16 << (16 - bits)) >> (16 - bits); > - *val = val16; > + *val = sign_extend32(val16, bits - 1); Yeah. This is a nice clean up. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
I think -M is prefered for these types of diffs? Not sure. On Mon, Feb 12, 2018 at 05:24:59PM +0530, Himanshu Jha wrote: > +static int adis16201_probe(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev; > + struct adis *st; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); > + if (!indio_dev) > + return -ENOMEM; > + > + st = iio_priv(indio_dev); > + spi_set_drvdata(spi, indio_dev); > + > + indio_dev->name = spi->dev.driver->name; > + indio_dev->dev.parent = &spi->dev; > + indio_dev->info = &adis16201_info; > + > + indio_dev->channels = adis16201_channels; > + indio_dev->num_channels = ARRAY_SIZE(adis16201_channels); > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + ret = adis_init(st, indio_dev, spi, &adis16201_data); > + if (ret) > + return ret; > + > + ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL); > + if (ret) > + return ret; We should clean up the IRQ which we enabled in adis_init() instead of returning directly. > + > + /* Get the device into a sane initial state */ > + ret = adis_initial_startup(st); > + if (ret) > + goto error_cleanup_buffer_trigger; > + > + ret = iio_device_register(indio_dev); > + if (ret < 0) > + goto error_cleanup_buffer_trigger; > + > + return 0; > + > +error_cleanup_buffer_trigger: > + adis_cleanup_buffer_and_trigger(st, indio_dev); > + > + return ret; > +} Otherwise it looks fine to my not-an-iio-expert eye. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
On Mon, Feb 12, 2018 at 12:54 PM, Himanshu Jha wrote: > Move the adis16201 driver out of staging directory and merge to the > mainline IIO directory. > > Signed-off-by: Himanshu Jha > --- /dev/null > +++ b/drivers/iio/accel/adis16201.c > @@ -0,0 +1,315 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +MODULE_AUTHOR("Barry Song <21cn...@gmail.com>"); > +MODULE_DESCRIPTION("Analog Devices ADIS16201 Dual-Axis Digital Inclinometer > and Accelerometer"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("spi:adis16201"); Your MODULE_LICENSE does not match your SPDX license id. MODULE_LICENSE("GPL v2"); means SPDX GPL-2.0 MODULE_LICENSE("GPL"); means SPDX GPL-2.0+ -- Cordially Philippe Ombredanne ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
DONATION FOR less-privileged
My name is Mrs. Juliette Romualdez from Philippine. I am a sick woman who has decided to donate what I have to charity through you. You may be wondering why I chose you. But someone has to be chosen. I am 62 years old and was diagnosed for cancer about 2 years ago, I inherited a total sum of $6.150.000.00 million dollars from my late husband, This money is deposited in a reliable bank here in MANILA PHPLILPINE; Presently .This deposit was coded under a secret arrangement as a family treasure.I have informed my Attorney, about my decision in WILLING this fund to you .please get back to me immediately via my private email address ( julietteromual...@yahoo.com )Your beloved sister in Christ, Mrs. Juliette Romualdez ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
Hi Dan, On Mon, Feb 12, 2018 at 03:53:12PM +0300, Dan Carpenter wrote: > On Mon, Feb 12, 2018 at 05:24:57PM +0530, Himanshu Jha wrote: > > Remove some unnecessary comments by giving appropriate name to macros. > > Therefore, add _REG suffix for control registers. Also, align function > > arguments to match open parentheses using tabs. > > Group the control register and register field macros together. > > > > Blank line before some returns improves code readability. > > > > Signed-off-by: Himanshu Jha > > The most obvious response is that this needs to be broken up into > multiple patches. > > I think I liked almost all the comments... Please note that all the changes are suggested by Joanathan in his TODO https://marc.info/?l=linux-iio&m=151775804702998&w=2 I think it far more commenting as compared to other drivers in the mainline IIO drivers. I grouped all the relevant control registers together at one place with suitable comment. For eg: +/* Data Output Register Definitions */ The macro names are identical to the names in the datasheet if you can look and one could easily refer to the datasheet easily. Also, I grouped the control registers and their fields together make it more readable. For eg: +#define ADIS16201_MSC_CTRL_REG 0x34 +#define ADIS16201_MSC_CTRL_SELF_TEST_ENBIT(8) +#define ADIS16201_MSC_CTRL_DATA_RDY_EN BIT(2) > > -/* Output, power supply */ > > -#define ADIS16201_SUPPLY_OUT 0x02 > > +#define ADIS16201_SUPPLY_OUT_REG 0x02 > > To me it seems useful to know that we're talking about the power supply. For that purpose I already grouped data output register definitions. > Adding _REG to the name seems not terribly useful and it makes the name > longer so we're going to run into the 80 character limit. I just > checked and this patch does add some checkpatch warnings. _REG prefix is used to differentiate between registers addresses and the fields in the register as pointed in the above MSC_CTRL_REG and MSC_CTRL_SELF_TEST_EN. Yes by doing this it triggered 2 I think 80 character warning. For eg: ADIS_SUPPLY_CHAN(ADIS16201_SUPPLY_OUT_REG, ADIS16201_SCAN_SUPPLY, 0, 12), But you know with 81 characters so I neglected it because it looked more readable without breaking into lines IMHO. > But aligning the arguments is fine, deleting unused macros is fine, > adding blank lines is fine. > > > static int adis16201_probe(struct spi_device *spi) > > { > > - int ret; > > - struct adis *st; > > struct iio_dev *indio_dev; > > + struct adis *st; > > + int ret; > > Making this reverse Christmas tree is fine. But these things should all > be done in separate patches. I am sometimes confused in dividing the patches. As per your advice I should make separate patch for : 1. remove unnecessary comments. 2. add suitable suffix. 3. add tabs instead of space. 4. reverse christmas tree. But these should be done when we have *more* instances. For eg: I added a tab space in function static int adis16201_read_raw() argument to match open parentheses in this patch. But I also added tabs while removing and adding suitable suffix to the macros. So, should it also be done in a separate patch ? Since there was only one instance of adding tabs therefore I did it here without framing another patch for that purpose while saving my time to plan 'what to include/exclude in the patch ?' Also, given the above queries I was clueless as what to do first! Since sometimes it happens that the patch series doesn't apply cleanly because of incorrect ordering for framing the patches. Thanks for taking your time to review the patch series. -- Thanks Himanshu Jha ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
On Mon, Feb 12, 2018 at 03:10:56PM +0100, Philippe Ombredanne wrote: > On Mon, Feb 12, 2018 at 12:54 PM, Himanshu Jha > wrote: > > Move the adis16201 driver out of staging directory and merge to the > > mainline IIO directory. > > > > Signed-off-by: Himanshu Jha > > > > --- /dev/null > > +++ b/drivers/iio/accel/adis16201.c > > @@ -0,0 +1,315 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > > > > +MODULE_AUTHOR("Barry Song <21cn...@gmail.com>"); > > +MODULE_DESCRIPTION("Analog Devices ADIS16201 Dual-Axis Digital > > Inclinometer and Accelerometer"); > > +MODULE_LICENSE("GPL v2"); > > +MODULE_ALIAS("spi:adis16201"); > > Your MODULE_LICENSE does not match your SPDX license id. > MODULE_LICENSE("GPL v2"); means SPDX GPL-2.0 > MODULE_LICENSE("GPL"); means SPDX GPL-2.0+ > I didn't knew about that! Thanks for pointing. -- Thanks Himanshu Jha ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
On Mon, Feb 12, 2018 at 04:18:26PM +0300, Dan Carpenter wrote: > I think -M is prefered for these types of diffs? Not sure. I wrote about that in the cover letter if you missed. :) > On Mon, Feb 12, 2018 at 05:24:59PM +0530, Himanshu Jha wrote: > > +static int adis16201_probe(struct spi_device *spi) > > +{ > > + struct iio_dev *indio_dev; > > + struct adis *st; > > + int ret; > > + > > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + st = iio_priv(indio_dev); > > + spi_set_drvdata(spi, indio_dev); > > + > > + indio_dev->name = spi->dev.driver->name; > > + indio_dev->dev.parent = &spi->dev; > > + indio_dev->info = &adis16201_info; > > + > > + indio_dev->channels = adis16201_channels; > > + indio_dev->num_channels = ARRAY_SIZE(adis16201_channels); > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + > > + ret = adis_init(st, indio_dev, spi, &adis16201_data); > > + if (ret) > > + return ret; > > + > > + ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL); > > + if (ret) > > + return ret; > > We should clean up the IRQ which we enabled in adis_init() instead of > returning directly. I'm not sure about how to do that. -- Thanks Himanshu Jha ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
On Mon, Feb 12, 2018 at 08:11:57PM +0530, Himanshu Jha wrote: > On Mon, Feb 12, 2018 at 04:18:26PM +0300, Dan Carpenter wrote: > > I think -M is prefered for these types of diffs? Not sure. > > I wrote about that in the cover letter if you missed. :) > Yeah. I seldom read cover letters. > > > + ret = adis_init(st, indio_dev, spi, &adis16201_data); > > > + if (ret) > > > + return ret; > > > + > > > + ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL); > > > + if (ret) > > > + return ret; > > > > We should clean up the IRQ which we enabled in adis_init() instead of > > returning directly. > > I'm not sure about how to do that. > I believe in you that you can figure it out. :) regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: ion kernel mapping implementation
On 02/10/2018 01:43 AM, Alexey Skidanov wrote: Hi, Current ion kernel mapping implementation uses vmap() to map previously allocated buffers into kernel virtual address space. On 32 bit platforms, vmap() might fail on large enough buffers due to the limited available vmalloc space. dma_buf_kmap() should guarantee that only one page is mapped at a time. So, probably it's better to implement dma_buf_kmap() by kmap() and not by vmap()? Thanks, Alexey The short answer is yes. The long answer is that the conversion to the dma_buf APIs kept the existing Ion behavior which mapped the entire buffer. We got away with this because the in kernel mapping APIs are used very infrequently and with buffers that never triggered an exhaustion of vmalloc space. If we actually start seeing problems with this we can fix it up but I don't consider this a high priority item. Thanks, Laura ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: KASAN: use-after-free Read in remove_wait_queue
On Mon, Feb 12, 2018 at 06:11:02PM +0100, Dmitry Vyukov wrote: > The commit on which it was triggered already includes this fix. So > there must be another bug. Any chance of bisecting it? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Add requested allocation alignment
On 02/10/2018 02:17 AM, Alexey Skidanov wrote: Current ion defined allocation ioctl doesn't allow to specify the requested allocation alignment. CMA heap allocates buffers aligned on buffer size page order. Sometimes, the alignment requirement is less restrictive. In such cases, providing specific alignment may reduce the external memory fragmentation and in some cases it may avoid the allocation request failure. I really do not want to bring this back as part of the regular ABI. Having an alignment parameter that gets used for exactly one heap only leads to confusion (which is why it was removed from the ABI in the first place). The alignment came from the behavior of the DMA APIs. Do you actually need to specify any alignment from userspace or do you only need page size? Thanks, Laura To fix this, we add an alignment parameter to the allocation ioctl. Signed-off-by: Alexey Skidanov --- drivers/staging/android/ion/ion-ioctl.c | 3 ++- drivers/staging/android/ion/ion.c | 14 +- drivers/staging/android/ion/ion.h | 9 ++--- drivers/staging/android/ion/ion_carveout_heap.c | 3 ++- drivers/staging/android/ion/ion_chunk_heap.c| 3 ++- drivers/staging/android/ion/ion_cma_heap.c | 18 -- drivers/staging/android/ion/ion_system_heap.c | 6 -- drivers/staging/android/uapi/ion.h | 7 +++ 8 files changed, 40 insertions(+), 23 deletions(-) diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c index c789893..9fe022b 100644 --- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -83,7 +83,8 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) fd = ion_alloc(data.allocation.len, data.allocation.heap_id_mask, - data.allocation.flags); + data.allocation.flags, + data.allocation.align); if (fd < 0) return fd; diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index f480885..35ddc7a 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -78,7 +78,8 @@ static void ion_buffer_add(struct ion_device *dev, static struct ion_buffer *ion_buffer_create(struct ion_heap *heap, struct ion_device *dev, unsigned long len, - unsigned long flags) + unsigned long flags, + unsigned int align) { struct ion_buffer *buffer; int ret; @@ -90,14 +91,14 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap, buffer->heap = heap; buffer->flags = flags; - ret = heap->ops->allocate(heap, buffer, len, flags); + ret = heap->ops->allocate(heap, buffer, len, flags, align); if (ret) { if (!(heap->flags & ION_HEAP_FLAG_DEFER_FREE)) goto err2; ion_heap_freelist_drain(heap, 0); - ret = heap->ops->allocate(heap, buffer, len, flags); + ret = heap->ops->allocate(heap, buffer, len, flags, align); if (ret) goto err2; } @@ -390,7 +391,10 @@ static const struct dma_buf_ops dma_buf_ops = { .unmap = ion_dma_buf_kunmap, }; -int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags) +int ion_alloc(size_t len, + unsigned int heap_id_mask, + unsigned int flags, + unsigned int align) { struct ion_device *dev = internal_dev; struct ion_buffer *buffer = NULL; @@ -417,7 +421,7 @@ int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags) /* if the caller didn't specify this heap id */ if (!((1 << heap->id) & heap_id_mask)) continue; - buffer = ion_buffer_create(heap, dev, len, flags); + buffer = ion_buffer_create(heap, dev, len, flags, align); if (!IS_ERR(buffer)) break; } diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h index f5f9cd6..0c161d2 100644 --- a/drivers/staging/android/ion/ion.h +++ b/drivers/staging/android/ion/ion.h @@ -123,8 +123,10 @@ struct ion_device { */ struct ion_heap_ops { int (*allocate)(struct ion_heap *heap, - struct ion_buffer *buffer, unsigned long len, - unsigned long flags); + struct ion_buffer *buffer, + unsigned long len, + unsigned long flags, + unsigned int align); void (*free)(
Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
On Mon, Feb 12, 2018 at 08:05:22PM +0530, Himanshu Jha wrote: > But these should be done when we have *more* instances. > > For eg: > I added a tab space in function static int adis16201_read_raw() argument > to match open parentheses in this patch. But I also added tabs while > removing and adding suitable suffix to the macros. So, should it also be > done in a separate patch ? If you're changing a line of code and you fix a white space issue on that same line, then that's fine. If it's just in the same function, then do it in a separate patch. In other words, adding tabs when you're moving around macros is fine, but adding it to the arguments is unrelated. This patch was honestly pretty tricky to review. Jonathan assumes reviewers have the datasheet in front of them and I assume that that they don't. He's probably right... But especially comments like this: *val2 = 22; /* 1.22 mV */ They seem really helpful to me. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Add requested allocation alignment
On 02/12/2018 08:42 PM, Laura Abbott wrote: > On 02/10/2018 02:17 AM, Alexey Skidanov wrote: >> Current ion defined allocation ioctl doesn't allow to specify the >> requested >> allocation alignment. CMA heap allocates buffers aligned on buffer size >> page order. >> >> Sometimes, the alignment requirement is less restrictive. In such cases, >> providing specific alignment may reduce the external memory fragmentation >> and in some cases it may avoid the allocation request failure. >> > > I really do not want to bring this back as part of the regular > ABI. Yes, I know it was removed in 4.12. Having an alignment parameter that gets used for exactly > one heap only leads to confusion (which is why it was removed > from the ABI in the first place). You are correct regarding the CMA heap. But, probably it may be used by custom heap as well. > > The alignment came from the behavior of the DMA APIs. Do you > actually need to specify any alignment from userspace or do > you only need page size? Yes. If CMA gives it for free, I would suggest to let the ion user to decide > > Thanks, > Laura > Thanks, Alexey >> To fix this, we add an alignment parameter to the allocation ioctl. >> >> Signed-off-by: Alexey Skidanov >> --- >> drivers/staging/android/ion/ion-ioctl.c | 3 ++- >> drivers/staging/android/ion/ion.c | 14 +- >> drivers/staging/android/ion/ion.h | 9 ++--- >> drivers/staging/android/ion/ion_carveout_heap.c | 3 ++- >> drivers/staging/android/ion/ion_chunk_heap.c | 3 ++- >> drivers/staging/android/ion/ion_cma_heap.c | 18 -- >> drivers/staging/android/ion/ion_system_heap.c | 6 -- >> drivers/staging/android/uapi/ion.h | 7 +++ >> 8 files changed, 40 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/staging/android/ion/ion-ioctl.c >> b/drivers/staging/android/ion/ion-ioctl.c >> index c789893..9fe022b 100644 >> --- a/drivers/staging/android/ion/ion-ioctl.c >> +++ b/drivers/staging/android/ion/ion-ioctl.c >> @@ -83,7 +83,8 @@ long ion_ioctl(struct file *filp, unsigned int cmd, >> unsigned long arg) >> fd = ion_alloc(data.allocation.len, >> data.allocation.heap_id_mask, >> - data.allocation.flags); >> + data.allocation.flags, >> + data.allocation.align); >> if (fd < 0) >> return fd; >> diff --git a/drivers/staging/android/ion/ion.c >> b/drivers/staging/android/ion/ion.c >> index f480885..35ddc7a 100644 >> --- a/drivers/staging/android/ion/ion.c >> +++ b/drivers/staging/android/ion/ion.c >> @@ -78,7 +78,8 @@ static void ion_buffer_add(struct ion_device *dev, >> static struct ion_buffer *ion_buffer_create(struct ion_heap *heap, >> struct ion_device *dev, >> unsigned long len, >> - unsigned long flags) >> + unsigned long flags, >> + unsigned int align) >> { >> struct ion_buffer *buffer; >> int ret; >> @@ -90,14 +91,14 @@ static struct ion_buffer *ion_buffer_create(struct >> ion_heap *heap, >> buffer->heap = heap; >> buffer->flags = flags; >> - ret = heap->ops->allocate(heap, buffer, len, flags); >> + ret = heap->ops->allocate(heap, buffer, len, flags, align); >> if (ret) { >> if (!(heap->flags & ION_HEAP_FLAG_DEFER_FREE)) >> goto err2; >> ion_heap_freelist_drain(heap, 0); >> - ret = heap->ops->allocate(heap, buffer, len, flags); >> + ret = heap->ops->allocate(heap, buffer, len, flags, align); >> if (ret) >> goto err2; >> } >> @@ -390,7 +391,10 @@ static const struct dma_buf_ops dma_buf_ops = { >> .unmap = ion_dma_buf_kunmap, >> }; >> -int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int >> flags) >> +int ion_alloc(size_t len, >> + unsigned int heap_id_mask, >> + unsigned int flags, >> + unsigned int align) >> { >> struct ion_device *dev = internal_dev; >> struct ion_buffer *buffer = NULL; >> @@ -417,7 +421,7 @@ int ion_alloc(size_t len, unsigned int >> heap_id_mask, unsigned int flags) >> /* if the caller didn't specify this heap id */ >> if (!((1 << heap->id) & heap_id_mask)) >> continue; >> - buffer = ion_buffer_create(heap, dev, len, flags); >> + buffer = ion_buffer_create(heap, dev, len, flags, align); >> if (!IS_ERR(buffer)) >> break; >> } >> diff --git a/drivers/staging/android/ion/ion.h >> b/drivers/staging/android/ion/ion.h >> index f5f9cd6..0c161d2 100644 >> --- a/drivers/staging/android/ion/ion.h >> +++ b/drivers/staging/android/ion/ion.h >> @@ -123,8 +123,10 @@ struct ion_device { >> */ >> struct ion_heap_ops { >> int (*allocate)(struct ion_heap *heap, >> - str
Re: ion kernel mapping implementation
On 02/12/2018 08:30 PM, Laura Abbott wrote: > On 02/10/2018 01:43 AM, Alexey Skidanov wrote: >> Hi, >> >> Current ion kernel mapping implementation uses vmap() to map previously >> allocated buffers into kernel virtual address space. On 32 bit >> platforms, vmap() might fail on large enough buffers due to the limited >> available vmalloc space. >> >> dma_buf_kmap() should guarantee that only one page is mapped at a time. >> So, probably it's better to implement dma_buf_kmap() by kmap() and not >> by vmap()? >> >> Thanks, >> Alexey >> > > The short answer is yes. > > The long answer is that the conversion to the dma_buf APIs kept the > existing Ion behavior which mapped the entire buffer. We got away > with this because the in kernel mapping APIs are used very infrequently > and with buffers that never triggered an exhaustion of vmalloc > space. > > If we actually start seeing problems with this we can fix it up > but I don't consider this a high priority item. I have the patch fixing this potential bug. I would like to submit it for review, if you are ok with it. Please, just let me know. > > Thanks, > Laura Thanks, Alexey ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
On Mon, Feb 12, 2018 at 05:57:31PM +0300, Dan Carpenter wrote: > On Mon, Feb 12, 2018 at 08:05:22PM +0530, Himanshu Jha wrote: > > But these should be done when we have *more* instances. > > > > For eg: > > I added a tab space in function static int adis16201_read_raw() argument > > to match open parentheses in this patch. But I also added tabs while > > removing and adding suitable suffix to the macros. So, should it also be > > done in a separate patch ? > > If you're changing a line of code and you fix a white space issue on > that same line, then that's fine. If it's just in the same function, > then do it in a separate patch. In other words, adding tabs when you're > moving around macros is fine, but adding it to the arguments is > unrelated. I will try and divide my patches next time :) > This patch was honestly pretty tricky to review. I am sorry for that. Might be easy for IIO reviewers ;) > Jonathan assumes reviewers have the datasheet in front of them and I > assume that that they don't. He's probably right... But especially > comments like this: > > *val2 = 22; /* 1.22 mV */ They are pretty obvious as you can see from the return statements just below that which is : return IIO_VAL_INT_PLUS_MICRO; These comments are obvious because we know 'val1' will be responsible for Integer part(1.0) and 'val2' for the Micro part(22 * 10^-6 = 0.22). Isn't it ? Although I am new to IIO please correct if I'm wrong. -- Thanks Himanshu Jha ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Add requested allocation alignment
On 02/12/2018 11:11 AM, Alexey Skidanov wrote: On 02/12/2018 08:42 PM, Laura Abbott wrote: On 02/10/2018 02:17 AM, Alexey Skidanov wrote: Current ion defined allocation ioctl doesn't allow to specify the requested allocation alignment. CMA heap allocates buffers aligned on buffer size page order. Sometimes, the alignment requirement is less restrictive. In such cases, providing specific alignment may reduce the external memory fragmentation and in some cases it may avoid the allocation request failure. I really do not want to bring this back as part of the regular ABI. Yes, I know it was removed in 4.12. Having an alignment parameter that gets used for exactly one heap only leads to confusion (which is why it was removed from the ABI in the first place). You are correct regarding the CMA heap. But, probably it may be used by custom heap as well. I can think of a lot of instances where it could be used but ultimately there needs to be an actual in kernel user who wants it. The alignment came from the behavior of the DMA APIs. Do you actually need to specify any alignment from userspace or do you only need page size? Yes. If CMA gives it for free, I would suggest to let the ion user to decide I'm really not convinced changing the ABI yet again just to let the user decide is actually worth it. If we can manage it, I'd much rather see a proposal that doesn't change the ABI. Thanks, Laura Thanks, Alexey ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: ion kernel mapping implementation
On 02/12/2018 11:21 AM, Alexey Skidanov wrote: On 02/12/2018 08:30 PM, Laura Abbott wrote: On 02/10/2018 01:43 AM, Alexey Skidanov wrote: Hi, Current ion kernel mapping implementation uses vmap() to map previously allocated buffers into kernel virtual address space. On 32 bit platforms, vmap() might fail on large enough buffers due to the limited available vmalloc space. dma_buf_kmap() should guarantee that only one page is mapped at a time. So, probably it's better to implement dma_buf_kmap() by kmap() and not by vmap()? Thanks, Alexey The short answer is yes. The long answer is that the conversion to the dma_buf APIs kept the existing Ion behavior which mapped the entire buffer. We got away with this because the in kernel mapping APIs are used very infrequently and with buffers that never triggered an exhaustion of vmalloc space. If we actually start seeing problems with this we can fix it up but I don't consider this a high priority item. I have the patch fixing this potential bug. I would like to submit it for review, if you are ok with it. Please, just let me know. Yes, please submit from review and we can go from there. Thanks, Laura Thanks, Alexey ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Add requested allocation alignment
On 02/12/2018 09:52 PM, Laura Abbott wrote: > On 02/12/2018 11:11 AM, Alexey Skidanov wrote: >> >> On 02/12/2018 08:42 PM, Laura Abbott wrote: >>> On 02/10/2018 02:17 AM, Alexey Skidanov wrote: Current ion defined allocation ioctl doesn't allow to specify the requested allocation alignment. CMA heap allocates buffers aligned on buffer size page order. Sometimes, the alignment requirement is less restrictive. In such cases, providing specific alignment may reduce the external memory fragmentation and in some cases it may avoid the allocation request failure. >>> I really do not want to bring this back as part of the regular >>> ABI. >> Yes, I know it was removed in 4.12. >> Having an alignment parameter that gets used for exactly >>> one heap only leads to confusion (which is why it was removed >>> from the ABI in the first place). >> You are correct regarding the CMA heap. But, probably it may be used by >> custom heap as well. > > I can think of a lot of instances where it could be used but > ultimately there needs to be an actual in kernel user who wants > it. > >>> The alignment came from the behavior of the DMA APIs. Do you >>> actually need to specify any alignment from userspace or do >>> you only need page size? >> Yes. If CMA gives it for free, I would suggest to let the ion user to >> decide > > I'm really not convinced changing the ABI yet again just to let > the user decide is actually worth it. If we can manage it, I'd > much rather see a proposal that doesn't change the ABI. I didn't actually change the ABI - I just use the "unused" member: struct ion_allocation_data { @@ -80,7 +79,7 @@ struct ion_allocation_data { __u32 heap_id_mask; __u32 flags; __u32 fd; - __u32 unused; + __u32 align; }; As an alternative, I may add __u64 heap_specific_param - but this will change the ABI. But, probably it makes the ABI more generic? > >>> Thanks, >>> Laura >>> >> Thanks, >> Alexey >> > Thanks, Alexey ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: ion kernel mapping implementation
On 02/12/2018 10:09 PM, Laura Abbott wrote: > On 02/12/2018 11:21 AM, Alexey Skidanov wrote: >> >> >> On 02/12/2018 08:30 PM, Laura Abbott wrote: >>> On 02/10/2018 01:43 AM, Alexey Skidanov wrote: Hi, Current ion kernel mapping implementation uses vmap() to map previously allocated buffers into kernel virtual address space. On 32 bit platforms, vmap() might fail on large enough buffers due to the limited available vmalloc space. dma_buf_kmap() should guarantee that only one page is mapped at a time. So, probably it's better to implement dma_buf_kmap() by kmap() and not by vmap()? Thanks, Alexey >>> >>> The short answer is yes. >>> >>> The long answer is that the conversion to the dma_buf APIs kept the >>> existing Ion behavior which mapped the entire buffer. We got away >>> with this because the in kernel mapping APIs are used very infrequently >>> and with buffers that never triggered an exhaustion of vmalloc >>> space. >>> >>> If we actually start seeing problems with this we can fix it up >>> but I don't consider this a high priority item. >> I have the patch fixing this potential bug. I would like to submit it >> for review, if you are ok with it. Please, just let me know. > > Yes, please submit from review and we can go from there. > Ok. Will submit it. >>> >>> Thanks, >>> Laura >> >> Thanks, >> Alexey >> > Thanks, Alexey ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Restrict cache maintenance to dma mapped memory
On 02/09/2018 10:21 PM, Liam Mark wrote: The ION begin_cpu_access and end_cpu_access functions use the dma_sync_sg_for_cpu and dma_sync_sg_for_device APIs to perform cache maintenance. Currently it is possible to apply cache maintenance, via the begin_cpu_access and end_cpu_access APIs, to ION buffers which are not dma mapped. The dma sync sg APIs should not be called on sg lists which have not been dma mapped as this can result in cache maintenance being applied to the wrong address. If an sg list has not been dma mapped then its dma_address field has not been populated, some dma ops such as the swiotlb_dma_ops ops use the dma_address field to calculate the address onto which to apply cache maintenance. Fix the ION begin_cpu_access and end_cpu_access functions to only apply cache maintenance to buffers which have been dma mapped. I think this looks okay. I was initially concerned about concurrency and setting the dma_mapped flag but I think that should be handled by the caller synchronizing map/unmap/cpu_access calls (we might need to re-evaluate in the future) I would like to hold on queuing this for just a little bit until I finish working on the Ion unit test (doing this in the complete opposite order of course). I'm assuming this passed your internal tests Liam? Thanks, Laura Fixes: 2a55e7b5e544 ("staging: android: ion: Call dma_map_sg for syncing and mapping") Signed-off-by: Liam Mark --- drivers/staging/android/ion/ion.c | 16 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index f480885e346b..e5df5272823d 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -214,6 +214,7 @@ struct ion_dma_buf_attachment { struct device *dev;caller struct sg_table *table; struct list_head list; + bool dma_mapped; }; static int ion_dma_buf_attach(struct dma_buf *dmabuf, struct device *dev, @@ -235,6 +236,7 @@ static int ion_dma_buf_attach(struct dma_buf *dmabuf, struct device *dev, a->table = table; a->dev = dev; + a->dma_mapped = false; INIT_LIST_HEAD(&a->list); attachment->priv = a; @@ -272,6 +274,7 @@ static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment, direction)) return ERR_PTR(-ENOMEM); + a->dma_mapped = true; return table; } @@ -279,7 +282,10 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *table, enum dma_data_direction direction) { + struct ion_dma_buf_attachment *a = attachment->priv; + dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction); + a->dma_mapped = false; } static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma) @@ -345,8 +351,9 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, mutex_lock(&buffer->lock); list_for_each_entry(a, &buffer->attachments, list) { - dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents, - direction); + if (a->dma_mapped) + dma_sync_sg_for_cpu(a->dev, a->table->sgl, + a->table->nents, direction); } mutex_unlock(&buffer->lock); @@ -367,8 +374,9 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf, mutex_lock(&buffer->lock); list_for_each_entry(a, &buffer->attachments, list) { - dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents, - direction); + if (a->dma_mapped) + dma_sync_sg_for_device(a->dev, a->table->sgl, + a->table->nents, direction); } mutex_unlock(&buffer->lock); ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Restrict cache maintenance to dma mapped memory
On 02/09/2018 10:21 PM, Liam Mark wrote: The ION begin_cpu_access and end_cpu_access functions use the dma_sync_sg_for_cpu and dma_sync_sg_for_device APIs to perform cache maintenance. Currently it is possible to apply cache maintenance, via the begin_cpu_access and end_cpu_access APIs, to ION buffers which are not dma mapped. The dma sync sg APIs should not be called on sg lists which have not been dma mapped as this can result in cache maintenance being applied to the wrong address. If an sg list has not been dma mapped then its dma_address field has not been populated, some dma ops such as the swiotlb_dma_ops ops use the dma_address field to calculate the address onto which to apply cache maintenance. Fix the ION begin_cpu_access and end_cpu_access functions to only apply cache maintenance to buffers which have been dma mapped. I think this looks okay. I was initially concerned about concurrency and setting the dma_mapped flag but I think that should be handled by the caller synchronizing map/unmap/cpu_access calls (we might need to re-evaluate in the future) I would like to hold on queuing this for just a little bit until I finish working on the Ion unit test (doing this in the complete opposite order of course). I'm assuming this passed your internal tests Liam? Thanks, Laura Fixes: 2a55e7b5e544 ("staging: android: ion: Call dma_map_sg for syncing and mapping") Signed-off-by: Liam Mark --- drivers/staging/android/ion/ion.c | 16 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index f480885e346b..e5df5272823d 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -214,6 +214,7 @@ struct ion_dma_buf_attachment { struct device *dev;caller struct sg_table *table; struct list_head list; + bool dma_mapped; }; static int ion_dma_buf_attach(struct dma_buf *dmabuf, struct device *dev, @@ -235,6 +236,7 @@ static int ion_dma_buf_attach(struct dma_buf *dmabuf, struct device *dev, a->table = table; a->dev = dev; + a->dma_mapped = false; INIT_LIST_HEAD(&a->list); attachment->priv = a; @@ -272,6 +274,7 @@ static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment, direction)) return ERR_PTR(-ENOMEM); + a->dma_mapped = true; return table; } @@ -279,7 +282,10 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *table, enum dma_data_direction direction) { + struct ion_dma_buf_attachment *a = attachment->priv; + dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction); + a->dma_mapped = false; } static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma) @@ -345,8 +351,9 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, mutex_lock(&buffer->lock); list_for_each_entry(a, &buffer->attachments, list) { - dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents, - direction); + if (a->dma_mapped) + dma_sync_sg_for_cpu(a->dev, a->table->sgl, + a->table->nents, direction); } mutex_unlock(&buffer->lock); @@ -367,8 +374,9 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf, mutex_lock(&buffer->lock); list_for_each_entry(a, &buffer->attachments, list) { - dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents, - direction); + if (a->dma_mapped) + dma_sync_sg_for_device(a->dev, a->table->sgl, + a->table->nents, direction); } mutex_unlock(&buffer->lock); ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Add requested allocation alignment
On 02/12/2018 12:22 PM, Alexey Skidanov wrote: On 02/12/2018 09:52 PM, Laura Abbott wrote: On 02/12/2018 11:11 AM, Alexey Skidanov wrote: On 02/12/2018 08:42 PM, Laura Abbott wrote: On 02/10/2018 02:17 AM, Alexey Skidanov wrote: Current ion defined allocation ioctl doesn't allow to specify the requested allocation alignment. CMA heap allocates buffers aligned on buffer size page order. Sometimes, the alignment requirement is less restrictive. In such cases, providing specific alignment may reduce the external memory fragmentation and in some cases it may avoid the allocation request failure. I really do not want to bring this back as part of the regular ABI. Yes, I know it was removed in 4.12. Having an alignment parameter that gets used for exactly one heap only leads to confusion (which is why it was removed from the ABI in the first place). You are correct regarding the CMA heap. But, probably it may be used by custom heap as well. I can think of a lot of instances where it could be used but ultimately there needs to be an actual in kernel user who wants it. The alignment came from the behavior of the DMA APIs. Do you actually need to specify any alignment from userspace or do you only need page size? Yes. If CMA gives it for free, I would suggest to let the ion user to decide I'm really not convinced changing the ABI yet again just to let the user decide is actually worth it. If we can manage it, I'd much rather see a proposal that doesn't change the ABI. I didn't actually change the ABI - I just use the "unused" member: struct ion_allocation_data { @@ -80,7 +79,7 @@ struct ion_allocation_data { __u32 heap_id_mask; __u32 flags; __u32 fd; - __u32 unused; + __u32 align; }; Something that was previously unused is now being used. That's a change in how the structure is interpreted which is an ABI change, especially since we haven't been checking that the __unused field is set to 0. As an alternative, I may add __u64 heap_specific_param - but this will change the ABI. But, probably it makes the ABI more generic? Why does the ABI need to be more generic? If we change the ABI we're stuck with it and I'd like to approach it as the very last resort. Thanks, Laura ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/5] mtd: Simplify erase handling
Hello, This series aims at simplifying erase handling both in MTD drivers and MTD users code. Historically, the erase operation has been designed to be asynchronous, which, in theory, is a good thing since erasing a block usually takes longer that reading/writing to a flash. In practice, all drivers are implementing ->_erase() in a synchronous manner. Moreover, both drivers and users are inconsistently updating/checking the erase_info fields. In order to simplify things, let's assume ->_erase() is and will always be synchronous. This also make error code checking more consistent and allows us to get rid of a few hundred lines of code. Regards, Boris Boris Brezillon (5): mtd: Initialize ->fail_addr early in mtd_erase() mtd: Get rid of unused fields in struct erase_info mtd: Stop assuming mtd_erase() is asynchronous mtd: Unconditionally update ->fail_addr and ->addr in part_erase() mtd: Stop updating erase_info->state and calling mtd_erase_callback() drivers/mtd/chips/cfi_cmdset_0001.c | 16 +- drivers/mtd/chips/cfi_cmdset_0002.c | 26 ++--- drivers/mtd/chips/cfi_cmdset_0020.c | 3 -- drivers/mtd/chips/map_ram.c | 2 - drivers/mtd/devices/bcm47xxsflash.c | 12 + drivers/mtd/devices/block2mtd.c | 7 +-- drivers/mtd/devices/docg3.c | 16 +- drivers/mtd/devices/lart.c | 4 -- drivers/mtd/devices/mtd_dataflash.c | 4 -- drivers/mtd/devices/mtdram.c | 2 - drivers/mtd/devices/phram.c | 2 - drivers/mtd/devices/pmc551.c | 2 - drivers/mtd/devices/powernv_flash.c | 11 +--- drivers/mtd/devices/slram.c | 2 - drivers/mtd/devices/spear_smi.c | 3 -- drivers/mtd/devices/sst25l.c | 3 -- drivers/mtd/devices/st_spi_fsm.c | 4 -- drivers/mtd/ftl.c| 52 +++--- drivers/mtd/inftlmount.c | 8 ++- drivers/mtd/lpddr/lpddr2_nvm.c | 10 +--- drivers/mtd/lpddr/lpddr_cmds.c | 2 - drivers/mtd/mtdblock.c | 21 drivers/mtd/mtdchar.c| 34 +--- drivers/mtd/mtdconcat.c | 48 + drivers/mtd/mtdcore.c| 17 +++--- drivers/mtd/mtdoops.c| 20 --- drivers/mtd/mtdpart.c| 23 ++-- drivers/mtd/mtdswap.c| 34 drivers/mtd/nand/nand_base.c | 16 ++ drivers/mtd/nand/nand_bbt.c | 1 - drivers/mtd/nftlmount.c | 5 +- drivers/mtd/onenand/onenand_base.c | 17 -- drivers/mtd/rfd_ftl.c| 93 ++-- drivers/mtd/sm_ftl.c | 19 --- drivers/mtd/sm_ftl.h | 4 -- drivers/mtd/spi-nor/spi-nor.c| 3 -- drivers/mtd/tests/mtd_test.c | 5 -- drivers/mtd/tests/speedtest.c| 7 --- drivers/mtd/ubi/gluebi.c | 3 -- drivers/mtd/ubi/io.c | 36 - drivers/net/ethernet/sfc/falcon/mtd.c| 11 +--- drivers/net/ethernet/sfc/mtd.c | 11 +--- drivers/staging/goldfish/goldfish_nand.c | 3 -- fs/jffs2/erase.c | 37 ++--- include/linux/mtd/mtd.h | 19 +-- 45 files changed, 79 insertions(+), 599 deletions(-) -- 2.14.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/5] mtd: Initialize ->fail_addr early in mtd_erase()
mtd_erase() can return an error before ->fail_addr is initialized to MTD_FAIL_ADDR_UNKNOWN. Move this initialization at the very beginning of the function. Signed-off-by: Boris Brezillon --- drivers/mtd/mtdcore.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index a1c94526fb88..c87859ff338b 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -953,6 +953,8 @@ EXPORT_SYMBOL_GPL(__put_mtd_device); */ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr) { + instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN; + if (!mtd->erasesize || !mtd->_erase) return -ENOTSUPP; @@ -961,7 +963,6 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr) if (!(mtd->flags & MTD_WRITEABLE)) return -EROFS; - instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN; if (!instr->len) { instr->state = MTD_ERASE_DONE; mtd_erase_callback(instr); -- 2.14.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 5/5] mtd: Stop updating erase_info->state and calling mtd_erase_callback()
MTD users are no longer checking erase_info->state to determine if the erase operation failed or succeeded. Moreover, mtd_erase_callback() is now a NOP. We can safely get rid of all mtd_erase_callback() calls and all erase_info->state assignments. While at it, get rid of the erase_info->state field, all MTD_ERASE_XXX definitions and the mtd_erase_callback() function. Signed-off-by: Boris Brezillon --- drivers/mtd/chips/cfi_cmdset_0001.c | 16 ++-- drivers/mtd/chips/cfi_cmdset_0002.c | 26 +++--- drivers/mtd/chips/cfi_cmdset_0020.c | 3 --- drivers/mtd/chips/map_ram.c | 2 -- drivers/mtd/devices/bcm47xxsflash.c | 9 + drivers/mtd/devices/block2mtd.c | 7 +-- drivers/mtd/devices/docg3.c | 16 ++-- drivers/mtd/devices/lart.c | 4 drivers/mtd/devices/mtd_dataflash.c | 4 drivers/mtd/devices/mtdram.c | 2 -- drivers/mtd/devices/phram.c | 2 -- drivers/mtd/devices/pmc551.c | 2 -- drivers/mtd/devices/powernv_flash.c | 11 ++- drivers/mtd/devices/slram.c | 2 -- drivers/mtd/devices/spear_smi.c | 3 --- drivers/mtd/devices/sst25l.c | 3 --- drivers/mtd/devices/st_spi_fsm.c | 4 drivers/mtd/lpddr/lpddr2_nvm.c | 10 ++ drivers/mtd/lpddr/lpddr_cmds.c | 2 -- drivers/mtd/mtdconcat.c | 1 - drivers/mtd/mtdcore.c| 6 ++ drivers/mtd/mtdpart.c| 5 - drivers/mtd/nand/nand_base.c | 15 +++ drivers/mtd/onenand/onenand_base.c | 17 - drivers/mtd/spi-nor/spi-nor.c| 3 --- drivers/mtd/ubi/gluebi.c | 3 --- drivers/net/ethernet/sfc/falcon/mtd.c| 11 +-- drivers/net/ethernet/sfc/mtd.c | 11 +-- drivers/staging/goldfish/goldfish_nand.c | 3 --- include/linux/mtd/mtd.h | 9 - 30 files changed, 20 insertions(+), 192 deletions(-) diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c index 5e1b68cbcd0a..d4c07b85f18e 100644 --- a/drivers/mtd/chips/cfi_cmdset_0001.c +++ b/drivers/mtd/chips/cfi_cmdset_0001.c @@ -1993,20 +1993,8 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip, static int cfi_intelext_erase_varsize(struct mtd_info *mtd, struct erase_info *instr) { - unsigned long ofs, len; - int ret; - - ofs = instr->addr; - len = instr->len; - - ret = cfi_varsize_frob(mtd, do_erase_oneblock, ofs, len, NULL); - if (ret) - return ret; - - instr->state = MTD_ERASE_DONE; - mtd_erase_callback(instr); - - return 0; + return cfi_varsize_frob(mtd, do_erase_oneblock, instr->addr, + instr->len, NULL); } static void cfi_intelext_sync (struct mtd_info *mtd) diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index 56aa6b75213d..668e2cbc155b 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -2415,20 +2415,8 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip, static int cfi_amdstd_erase_varsize(struct mtd_info *mtd, struct erase_info *instr) { - unsigned long ofs, len; - int ret; - - ofs = instr->addr; - len = instr->len; - - ret = cfi_varsize_frob(mtd, do_erase_oneblock, ofs, len, NULL); - if (ret) - return ret; - - instr->state = MTD_ERASE_DONE; - mtd_erase_callback(instr); - - return 0; + return cfi_varsize_frob(mtd, do_erase_oneblock, instr->addr, + instr->len, NULL); } @@ -2436,7 +2424,6 @@ static int cfi_amdstd_erase_chip(struct mtd_info *mtd, struct erase_info *instr) { struct map_info *map = mtd->priv; struct cfi_private *cfi = map->fldrv_priv; - int ret = 0; if (instr->addr != 0) return -EINVAL; @@ -2444,14 +2431,7 @@ static int cfi_amdstd_erase_chip(struct mtd_info *mtd, struct erase_info *instr) if (instr->len != mtd->size) return -EINVAL; - ret = do_erase_chip(map, &cfi->chips[0]); - if (ret) - return ret; - - instr->state = MTD_ERASE_DONE; - mtd_erase_callback(instr); - - return 0; + return do_erase_chip(map, &cfi->chips[0]); } static int do_atmel_lock(struct map_info *map, struct flchip *chip, diff --git a/drivers/mtd/chips/cfi_cmdset_0020.c b/drivers/mtd/chips/cfi_cmdset_0020.c index 7d342965f392..7b7658a05036 100644 --- a/drivers/mtd/chips/cfi_cmdset_0020.c +++ b/drivers/mtd/chips/cfi_cmdset_0020.c @@ -965,9 +965,6 @@ static int cfi_staa_erase_varsize(struct mtd_info *mtd, } } - instr->state = MTD_
[PATCH 3/5] mtd: Stop assuming mtd_erase() is asynchronous
None of the mtd->_erase() implementations work in an asynchronous manner, so let's simplify MTD users that call mtd_erase(). All they need to do is check the value returned by mtd_erase() and assume that != 0 means failure. Signed-off-by: Boris Brezillon --- drivers/mtd/devices/bcm47xxsflash.c | 3 -- drivers/mtd/ftl.c | 51 drivers/mtd/inftlmount.c| 5 +- drivers/mtd/mtdblock.c | 20 drivers/mtd/mtdchar.c | 33 + drivers/mtd/mtdconcat.c | 48 ++- drivers/mtd/mtdcore.c | 8 ++-- drivers/mtd/mtdoops.c | 19 drivers/mtd/mtdpart.c | 2 - drivers/mtd/mtdswap.c | 32 - drivers/mtd/nftlmount.c | 4 +- drivers/mtd/rfd_ftl.c | 92 +++-- drivers/mtd/sm_ftl.c| 18 drivers/mtd/sm_ftl.h| 4 -- drivers/mtd/tests/mtd_test.c| 4 -- drivers/mtd/tests/speedtest.c | 6 --- drivers/mtd/ubi/io.c| 35 -- fs/jffs2/erase.c| 36 ++- include/linux/mtd/mtd.h | 2 - 19 files changed, 52 insertions(+), 370 deletions(-) diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c index e2bd81817df4..6b84947cfbea 100644 --- a/drivers/mtd/devices/bcm47xxsflash.c +++ b/drivers/mtd/devices/bcm47xxsflash.c @@ -95,9 +95,6 @@ static int bcm47xxsflash_erase(struct mtd_info *mtd, struct erase_info *erase) else erase->state = MTD_ERASE_DONE; - if (erase->callback) - erase->callback(erase); - return err; } diff --git a/drivers/mtd/ftl.c b/drivers/mtd/ftl.c index 664d206a4cbe..fcf9907e7987 100644 --- a/drivers/mtd/ftl.c +++ b/drivers/mtd/ftl.c @@ -140,12 +140,6 @@ typedef struct partition_t { #define XFER_PREPARED 0x03 #define XFER_FAILED0x04 -/**/ - - -static void ftl_erase_callback(struct erase_info *done); - - /*== Scan_header() checks to see if a memory region contains an FTL @@ -349,17 +343,19 @@ static int erase_xfer(partition_t *part, return -ENOMEM; erase->mtd = part->mbd.mtd; -erase->callback = ftl_erase_callback; erase->addr = xfer->Offset; erase->len = 1 << part->header.EraseUnitSize; -erase->priv = (u_long)part; ret = mtd_erase(part->mbd.mtd, erase); +if (!ret) { + xfer->state = XFER_ERASED; + xfer->EraseCount++; +} else { + xfer->state = XFER_FAILED; + pr_notice("ftl_cs: erase failed: err = %d\n", ret); +} -if (!ret) - xfer->EraseCount++; -else - kfree(erase); +kfree(erase); return ret; } /* erase_xfer */ @@ -371,37 +367,6 @@ static int erase_xfer(partition_t *part, ==*/ -static void ftl_erase_callback(struct erase_info *erase) -{ -partition_t *part; -struct xfer_info_t *xfer; -int i; - -/* Look up the transfer unit */ -part = (partition_t *)(erase->priv); - -for (i = 0; i < part->header.NumTransferUnits; i++) - if (part->XferInfo[i].Offset == erase->addr) break; - -if (i == part->header.NumTransferUnits) { - printk(KERN_NOTICE "ftl_cs: internal error: " - "erase lookup failed!\n"); - return; -} - -xfer = &part->XferInfo[i]; -if (erase->state == MTD_ERASE_DONE) - xfer->state = XFER_ERASED; -else { - xfer->state = XFER_FAILED; - printk(KERN_NOTICE "ftl_cs: erase failed: state = %d\n", - erase->state); -} - -kfree(erase); - -} /* ftl_erase_callback */ - static int prepare_xfer(partition_t *part, int i) { erase_unit_header_t header; diff --git a/drivers/mtd/inftlmount.c b/drivers/mtd/inftlmount.c index 8d6bb189ea8e..0f47be4834d8 100644 --- a/drivers/mtd/inftlmount.c +++ b/drivers/mtd/inftlmount.c @@ -393,9 +393,10 @@ int INFTL_formatblock(struct INFTLrecord *inftl, int block) mark only the failed block in the bbt. */ for (physblock = 0; physblock < inftl->EraseSize; physblock += instr->len, instr->addr += instr->len) { - mtd_erase(inftl->mbd.mtd, instr); + int ret; - if (instr->state == MTD_ERASE_FAILED) { + ret = mtd_erase(inftl->mbd.mtd, instr); + if (ret) { printk(KERN_WARNING "INFTL: error while formatting block %d\n", block); goto fail; diff --git a/drivers/mtd/mtdblock.c b/drivers/mtd/mtdblock.c index bb4c14f83c75..7b2b7f651181 100644 --- a/drivers/mtd/mtdblock.c +++ b/drivers/mtd/mtdblock.c @@ -55,48 +55,28 @@
[PATCH 2/5] mtd: Get rid of unused fields in struct erase_info
Some fields are not used by MTD drivers, users or core code. Moreover, those fields are not documented, so get rid of them to avoid any confusion. Signed-off-by: Boris Brezillon --- include/linux/mtd/mtd.h | 5 - 1 file changed, 5 deletions(-) diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 205ededccc60..2a407dc9beaa 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -48,14 +48,9 @@ struct erase_info { uint64_t addr; uint64_t len; uint64_t fail_addr; - u_long time; - u_long retries; - unsigned dev; - unsigned cell; void (*callback) (struct erase_info *self); u_long priv; u_char state; - struct erase_info *next; }; struct mtd_erase_region_info { -- 2.14.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 4/5] mtd: Unconditionally update ->fail_addr and ->addr in part_erase()
->fail_addr and ->addr can be updated no matter the result of parent->_erase(), we just need to remove the code doing the same thing in mtd_erase_callback() to avoid adjusting those fields twice. Note that this can be done because all MTD users have been converted to not pass an erase_info->callback() and are thus only taking the ->addr_fail and ->addr fields into account after part_erase() has returned. While we're at it, get rid of the erase_info->mtd field which was only needed to let mtd_erase_callback() get the partition device back. Signed-off-by: Boris Brezillon --- drivers/mtd/ftl.c | 1 - drivers/mtd/inftlmount.c | 3 --- drivers/mtd/mtdblock.c| 1 - drivers/mtd/mtdchar.c | 1 - drivers/mtd/mtdconcat.c | 1 - drivers/mtd/mtdoops.c | 1 - drivers/mtd/mtdpart.c | 16 drivers/mtd/mtdswap.c | 2 -- drivers/mtd/nand/nand_base.c | 1 - drivers/mtd/nand/nand_bbt.c | 1 - drivers/mtd/nftlmount.c | 1 - drivers/mtd/rfd_ftl.c | 1 - drivers/mtd/sm_ftl.c | 1 - drivers/mtd/tests/mtd_test.c | 1 - drivers/mtd/tests/speedtest.c | 1 - drivers/mtd/ubi/io.c | 1 - fs/jffs2/erase.c | 1 - include/linux/mtd/mtd.h | 3 ++- 18 files changed, 6 insertions(+), 32 deletions(-) diff --git a/drivers/mtd/ftl.c b/drivers/mtd/ftl.c index fcf9907e7987..0a6adfaec7b5 100644 --- a/drivers/mtd/ftl.c +++ b/drivers/mtd/ftl.c @@ -342,7 +342,6 @@ static int erase_xfer(partition_t *part, if (!erase) return -ENOMEM; -erase->mtd = part->mbd.mtd; erase->addr = xfer->Offset; erase->len = 1 << part->header.EraseUnitSize; diff --git a/drivers/mtd/inftlmount.c b/drivers/mtd/inftlmount.c index 0f47be4834d8..aab4f68bd36f 100644 --- a/drivers/mtd/inftlmount.c +++ b/drivers/mtd/inftlmount.c @@ -208,8 +208,6 @@ static int find_boot_record(struct INFTLrecord *inftl) if (ip->Reserved0 != ip->firstUnit) { struct erase_info *instr = &inftl->instr; - instr->mtd = inftl->mbd.mtd; - /* * Most likely this is using the * undocumented qiuck mount feature. @@ -385,7 +383,6 @@ int INFTL_formatblock(struct INFTLrecord *inftl, int block) _first_? */ /* Use async erase interface, test return code */ - instr->mtd = inftl->mbd.mtd; instr->addr = block * inftl->EraseSize; instr->len = inftl->mbd.mtd->erasesize; /* Erase one physical eraseblock at a time, even though the NAND api diff --git a/drivers/mtd/mtdblock.c b/drivers/mtd/mtdblock.c index 7b2b7f651181..a5b1933c0490 100644 --- a/drivers/mtd/mtdblock.c +++ b/drivers/mtd/mtdblock.c @@ -65,7 +65,6 @@ static int erase_write (struct mtd_info *mtd, unsigned long pos, /* * First, let's erase the flash block. */ - erase.mtd = mtd; erase.addr = pos; erase.len = len; diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c index 2beb22dd6bbb..c06b33f80e75 100644 --- a/drivers/mtd/mtdchar.c +++ b/drivers/mtd/mtdchar.c @@ -726,7 +726,6 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg) erase->addr = einfo32.start; erase->len = einfo32.length; } - erase->mtd = mtd; ret = mtd_erase(mtd, erase); kfree(erase); diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c index caa09bf6e572..93c47e56d9d8 100644 --- a/drivers/mtd/mtdconcat.c +++ b/drivers/mtd/mtdconcat.c @@ -427,7 +427,6 @@ static int concat_erase(struct mtd_info *mtd, struct erase_info *instr) erase->len = length; length -= erase->len; - erase->mtd = subdev; if ((err = mtd_erase(subdev, erase))) { /* sanity check: should never happen since * block alignment has been checked above */ diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c index 028ded59297b..9f25111fd559 100644 --- a/drivers/mtd/mtdoops.c +++ b/drivers/mtd/mtdoops.c @@ -94,7 +94,6 @@ static int mtdoops_erase_block(struct mtdoops_context *cxt, int offset) int ret; int page; - erase.mtd = mtd; erase.addr = offset; erase.len = mtd->erasesize; diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index ae1206633d9d..1c07a6f0dfe5 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -205,23 +205,15 @@ static int part_erase(struct mtd_info *mtd, struct erase_info *instr) instr->addr += part->offset; ret = part->parent->_erase(part->parent, instr); - if (ret) { - if (instr->fail_addr != MTD_FAIL_ADDR_UNKNOWN) -
Re: [PATCH] staging: android: ion: Initialize dma_address of new sg list
On Mon, 12 Feb 2018, Dan Carpenter wrote: > On Fri, Feb 09, 2018 at 10:16:56PM -0800, Liam Mark wrote: > > Fix the dup_sg_table function to initialize the dma_address of the new > > sg list entries instead of the source dma_address entries. > > > > Fixes: 17fd283f3870 ("staging: android: ion: Duplicate sg_table") > > Signed-off-by: Liam Mark > > How did you find this bug? What are the user visible effects of this > bug? I'm probably going to ask you to send a v2 patch with a new > changelog depending on the answers to these questions. I noticed the bug when reading through the code, I haven’t seen any visible effects of this bug. >From looking at the code I don’t see any obvious ways that this bug would introduce any issues with the current code base. Liam Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Add requested allocation alignment
On 02/12/2018 10:46 PM, Laura Abbott wrote: > On 02/12/2018 12:22 PM, Alexey Skidanov wrote: >> >> >> On 02/12/2018 09:52 PM, Laura Abbott wrote: >>> On 02/12/2018 11:11 AM, Alexey Skidanov wrote: On 02/12/2018 08:42 PM, Laura Abbott wrote: > On 02/10/2018 02:17 AM, Alexey Skidanov wrote: >> Current ion defined allocation ioctl doesn't allow to specify the >> requested >> allocation alignment. CMA heap allocates buffers aligned on buffer >> size >> page order. >> >> Sometimes, the alignment requirement is less restrictive. In such >> cases, >> providing specific alignment may reduce the external memory >> fragmentation >> and in some cases it may avoid the allocation request failure. >> > I really do not want to bring this back as part of the regular > ABI. Yes, I know it was removed in 4.12. Having an alignment parameter that gets used for exactly > one heap only leads to confusion (which is why it was removed > from the ABI in the first place). You are correct regarding the CMA heap. But, probably it may be used by custom heap as well. >>> >>> I can think of a lot of instances where it could be used but >>> ultimately there needs to be an actual in kernel user who wants >>> it. >>> > The alignment came from the behavior of the DMA APIs. Do you > actually need to specify any alignment from userspace or do > you only need page size? Yes. If CMA gives it for free, I would suggest to let the ion user to decide >>> >>> I'm really not convinced changing the ABI yet again just to let >>> the user decide is actually worth it. If we can manage it, I'd >>> much rather see a proposal that doesn't change the ABI. >> I didn't actually change the ABI - I just use the "unused" member: >> struct ion_allocation_data { >> @@ -80,7 +79,7 @@ struct ion_allocation_data { >> __u32 heap_id_mask; >> __u32 flags; >> __u32 fd; >> - __u32 unused; >> + __u32 align; >> }; >> > > Something that was previously unused is now being used. That's a change > in how the structure is interpreted which is an ABI change, especially > since we haven't been checking that the __unused field is set > to 0. Yes you are correct. I just realized that it isn't even backward compatible. > >> As an alternative, I may add __u64 heap_specific_param - but this will >> change the ABI. But, probably it makes the ABI more generic? > > Why does the ABI need to be more generic? If we change the ABI > we're stuck with it and I'd like to approach it as the very last > resort. > > Thanks, > Laura It seems, that there is no way to do it without some ABI change? Thanks, Alexey ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/5] mtd: Get rid of unused fields in struct erase_info
Am Montag, 12. Februar 2018, 22:03:08 CET schrieb Boris Brezillon: > Some fields are not used by MTD drivers, users or core code. Moreover, > those fields are not documented, so get rid of them to avoid any > confusion. > > Signed-off-by: Boris Brezillon > --- > include/linux/mtd/mtd.h | 5 - > 1 file changed, 5 deletions(-) > > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h > index 205ededccc60..2a407dc9beaa 100644 > --- a/include/linux/mtd/mtd.h > +++ b/include/linux/mtd/mtd.h > @@ -48,14 +48,9 @@ struct erase_info { > uint64_t addr; > uint64_t len; > uint64_t fail_addr; > - u_long time; > - u_long retries; > - unsigned dev; > - unsigned cell; > void (*callback) (struct erase_info *self); > u_long priv; > u_char state; > - struct erase_info *next; > }; > > struct mtd_erase_region_info { Reviewed-by: Richard Weinberger Thanks, //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/5] mtd: Initialize ->fail_addr early in mtd_erase()
Am Montag, 12. Februar 2018, 22:03:07 CET schrieb Boris Brezillon: > mtd_erase() can return an error before ->fail_addr is initialized to > MTD_FAIL_ADDR_UNKNOWN. Move this initialization at the very beginning > of the function. > > Signed-off-by: Boris Brezillon > --- > drivers/mtd/mtdcore.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index a1c94526fb88..c87859ff338b 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -953,6 +953,8 @@ EXPORT_SYMBOL_GPL(__put_mtd_device); > */ > int mtd_erase(struct mtd_info *mtd, struct erase_info *instr) > { > + instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN; > + > if (!mtd->erasesize || !mtd->_erase) > return -ENOTSUPP; > > @@ -961,7 +963,6 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info > *instr) if (!(mtd->flags & MTD_WRITEABLE)) > return -EROFS; > > - instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN; > if (!instr->len) { > instr->state = MTD_ERASE_DONE; > mtd_erase_callback(instr); Reviewed-by: Richard Weinberger Thanks, //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/5] mtd: Stop assuming mtd_erase() is asynchronous
Am Montag, 12. Februar 2018, 22:03:09 CET schrieb Boris Brezillon: > None of the mtd->_erase() implementations work in an asynchronous manner, > so let's simplify MTD users that call mtd_erase(). All they need to do > is check the value returned by mtd_erase() and assume that != 0 means > failure. > > Signed-off-by: Boris Brezillon Reviewed-by: Richard Weinberger Thanks, //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/5] mtd: Unconditionally update ->fail_addr and ->addr in part_erase()
Am Montag, 12. Februar 2018, 22:03:10 CET schrieb Boris Brezillon: > ->fail_addr and ->addr can be updated no matter the result of > parent->_erase(), we just need to remove the code doing the same thing > in mtd_erase_callback() to avoid adjusting those fields twice. > > Note that this can be done because all MTD users have been converted to > not pass an erase_info->callback() and are thus only taking the > ->addr_fail and ->addr fields into account after part_erase() has > returned. > > While we're at it, get rid of the erase_info->mtd field which was only > needed to let mtd_erase_callback() get the partition device back. > > Signed-off-by: Boris Brezillon Reviewed-by: Richard Weinberger Thanks, //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
usleep_range without a range
scheduling can generally be better when these values are not identical. Perhaps these ranges should be expanded. $ git grep -P -n "usleep_range\s*\(\s*([\w\.\>\-]+)\s*,\s*\1\s*\)" drivers/clk/ux500/clk-sysctrl.c:45: usleep_range(clk->enable_delay_us, clk->enable_delay_us); drivers/cpufreq/pmac64-cpufreq.c:140: usleep_range(1000, 1000); drivers/cpufreq/pmac64-cpufreq.c:239: usleep_range(1, 1); /* should be faster , to fix */ drivers/cpufreq/pmac64-cpufreq.c:284: usleep_range(500, 500); drivers/media/i2c/smiapp/smiapp-core.c:1228:usleep_range(1000, 1000); drivers/media/i2c/smiapp/smiapp-core.c:1235:usleep_range(1000, 1000); drivers/media/i2c/smiapp/smiapp-core.c:1240:usleep_range(sleep, sleep); drivers/media/i2c/smiapp/smiapp-core.c:1387:usleep_range(5000, 5000); drivers/media/i2c/smiapp/smiapp-quirk.c:205:usleep_range(2000, 2000); drivers/media/i2c/smiapp/smiapp-regs.c:279: usleep_range(2000, 2000); drivers/power/supply/ab8500_fg.c:643: usleep_range(100, 100); drivers/staging/rtl8192u/r819xU_phy.c:180: usleep_range(1000, 1000); drivers/staging/rtl8192u/r819xU_phy.c:736: usleep_range(1000, 1000); drivers/staging/rtl8192u/r819xU_phy.c:740: usleep_range(1000, 1000); sound/soc/codecs/ab8500-codec.c:1065: usleep_range(AB8500_ANC_SM_DELAY, AB8500_ANC_SM_DELAY); sound/soc/codecs/ab8500-codec.c:1068: usleep_range(AB8500_ANC_SM_DELAY, AB8500_ANC_SM_DELAY); ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/5] mtd: Stop updating erase_info->state and calling mtd_erase_callback()
Am Montag, 12. Februar 2018, 22:03:11 CET schrieb Boris Brezillon: > MTD users are no longer checking erase_info->state to determine if the > erase operation failed or succeeded. Moreover, mtd_erase_callback() is > now a NOP. > > We can safely get rid of all mtd_erase_callback() calls and all > erase_info->state assignments. While at it, get rid of the > erase_info->state field, all MTD_ERASE_XXX definitions and the > mtd_erase_callback() function. > > Signed-off-by: Boris Brezillon Reviewed-by: Richard Weinberger Thanks, //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
On Mon, Feb 12, 2018 at 3:37 PM, Himanshu Jha wrote: > On Mon, Feb 12, 2018 at 03:10:56PM +0100, Philippe Ombredanne wrote: >> On Mon, Feb 12, 2018 at 12:54 PM, Himanshu Jha >> wrote: >> > Move the adis16201 driver out of staging directory and merge to the >> > mainline IIO directory. >> > >> > Signed-off-by: Himanshu Jha >> >> >> > --- /dev/null >> > +++ b/drivers/iio/accel/adis16201.c >> > @@ -0,0 +1,315 @@ >> > +// SPDX-License-Identifier: GPL-2.0+ >> >> >> >> > +MODULE_AUTHOR("Barry Song <21cn...@gmail.com>"); >> > +MODULE_DESCRIPTION("Analog Devices ADIS16201 Dual-Axis Digital >> > Inclinometer and Accelerometer"); >> > +MODULE_LICENSE("GPL v2"); >> > +MODULE_ALIAS("spi:adis16201"); >> >> Your MODULE_LICENSE does not match your SPDX license id. >> MODULE_LICENSE("GPL v2"); means SPDX GPL-2.0 >> MODULE_LICENSE("GPL"); means SPDX GPL-2.0+ >> > > I didn't knew about that! Thanks for pointing. Sure thing. I reckon this can be a tad surprising. FWIW, the reference for the SPDX tag is in Documentation/process/license-rules.rst and for the MODULE_LICENSE macro this is in module.h It is unlikely module.h will ever evolve there as this would break all third-party module loaders/tools that rely on the documented data values and conventions -- Cordially Philippe Ombredanne ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: android: ion: Change dma_buf_kmap()/dma_buf_kmap_atomic() implementation
Current ion kernel mapping implementation uses vmap() to map previously allocated buffers into kernel virtual address space. On 32-bit platforms, vmap() might fail on large enough buffers due to the limited available vmalloc space. dma_buf_kmap() should guarantee that only one page is mapped at a time. To fix this, kmap()/kmap_atomic() is used to implement the appropriate interfaces. Signed-off-by: Alexey Skidanov --- drivers/staging/android/ion/ion.c | 97 +++ drivers/staging/android/ion/ion.h | 1 - 2 files changed, 48 insertions(+), 50 deletions(-) diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index 57e0d80..75830e3 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -27,6 +27,7 @@ #include #include #include +#include #include "ion.h" @@ -119,8 +120,6 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap, void ion_buffer_destroy(struct ion_buffer *buffer) { - if (WARN_ON(buffer->kmap_cnt > 0)) - buffer->heap->ops->unmap_kernel(buffer->heap, buffer); buffer->heap->ops->free(buffer); kfree(buffer); } @@ -140,34 +139,6 @@ static void _ion_buffer_destroy(struct ion_buffer *buffer) ion_buffer_destroy(buffer); } -static void *ion_buffer_kmap_get(struct ion_buffer *buffer) -{ - void *vaddr; - - if (buffer->kmap_cnt) { - buffer->kmap_cnt++; - return buffer->vaddr; - } - vaddr = buffer->heap->ops->map_kernel(buffer->heap, buffer); - if (WARN_ONCE(!vaddr, - "heap->ops->map_kernel should return ERR_PTR on error")) - return ERR_PTR(-EINVAL); - if (IS_ERR(vaddr)) - return vaddr; - buffer->vaddr = vaddr; - buffer->kmap_cnt++; - return vaddr; -} - -static void ion_buffer_kmap_put(struct ion_buffer *buffer) -{ - buffer->kmap_cnt--; - if (!buffer->kmap_cnt) { - buffer->heap->ops->unmap_kernel(buffer->heap, buffer); - buffer->vaddr = NULL; - } -} - static struct sg_table *dup_sg_table(struct sg_table *table) { struct sg_table *new_table; @@ -305,34 +276,68 @@ static void ion_dma_buf_release(struct dma_buf *dmabuf) _ion_buffer_destroy(buffer); } +static inline int sg_page_count(struct scatterlist *sg) +{ + return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT; +} + +static struct page *ion_dma_buf_get_page(struct sg_table *sg_table, +unsigned long offset) +{ + struct page *page; + struct scatterlist *sg; + int i; + unsigned int nr_pages; + + nr_pages = 0; + page = NULL; + /* Find the page with specified offset */ + for_each_sg(sg_table->sgl, sg, sg_table->nents, i) { + if (nr_pages + sg_page_count(sg) > offset) { + page = nth_page(sg_page(sg), offset - nr_pages); + break; + } + + nr_pages += sg_page_count(sg); + } + + return page; +} static void *ion_dma_buf_kmap(struct dma_buf *dmabuf, unsigned long offset) { struct ion_buffer *buffer = dmabuf->priv; - return buffer->vaddr + offset * PAGE_SIZE; + return kmap(ion_dma_buf_get_page(buffer->sg_table, offset)); } static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, unsigned long offset, void *ptr) { + kunmap(ptr); +} + +static void *ion_dma_buf_kmap_atomic(struct dma_buf *dmabuf, +unsigned long offset) +{ + struct ion_buffer *buffer = dmabuf->priv; + + return kmap_atomic(ion_dma_buf_get_page(buffer->sg_table, + offset)); +} + +static void ion_dma_buf_kunmap_atomic(struct dma_buf *dmabuf, + unsigned long offset, + void *ptr) +{ + kunmap_atomic(ptr); } static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, enum dma_data_direction direction) { struct ion_buffer *buffer = dmabuf->priv; - void *vaddr; struct ion_dma_buf_attachment *a; - /* -* TODO: Move this elsewhere because we don't always need a vaddr -*/ - if (buffer->heap->ops->map_kernel) { - mutex_lock(&buffer->lock); - vaddr = ion_buffer_kmap_get(buffer); - mutex_unlock(&buffer->lock); - } - mutex_lock(&buffer->lock); list_for_each_entry(a, &buffer->attachments, list) { dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents, @@ -349,12 +354,6 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf, struct ion_buffer *buffer = dmabuf->priv; struct ion_dma_buf_attachment *a; - if (bu
[PATCH] staging: rtl8723bs: make 'myid' function to follow kernel coding rules
Checkpatch.pl produced errors regarding inline keyword placement and parenthesis around returned value in 'myid'. Place inline after static keyword and remove mentioned parenthesis. Signed-off-by: Maciek Fijalkowski --- drivers/staging/rtl8723bs/include/drv_types.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/rtl8723bs/include/drv_types.h b/drivers/staging/rtl8723bs/include/drv_types.h index 32129ac8e169..16b81b1a3f33 100644 --- a/drivers/staging/rtl8723bs/include/drv_types.h +++ b/drivers/staging/rtl8723bs/include/drv_types.h @@ -692,9 +692,9 @@ int rtw_suspend_wow(struct adapter *padapter); int rtw_resume_process_wow(struct adapter *padapter); #endif -__inline static u8 *myid(struct eeprom_priv *peepriv) +static inline u8 *myid(struct eeprom_priv *peepriv) { - return (peepriv->mac_addr); + return peepriv->mac_addr; } /* HCI Related header file */ -- 2.16.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Restrict cache maintenance to dma mapped memory
On Mon, 12 Feb 2018, Laura Abbott wrote: > On 02/09/2018 10:21 PM, Liam Mark wrote: > > The ION begin_cpu_access and end_cpu_access functions use the > > dma_sync_sg_for_cpu and dma_sync_sg_for_device APIs to perform cache > > maintenance. > > > > Currently it is possible to apply cache maintenance, via the > > begin_cpu_access and end_cpu_access APIs, to ION buffers which are not > > dma mapped. > > > > The dma sync sg APIs should not be called on sg lists which have not been > > dma mapped as this can result in cache maintenance being applied to the > > wrong address. If an sg list has not been dma mapped then its dma_address > > field has not been populated, some dma ops such as the swiotlb_dma_ops ops > > use the dma_address field to calculate the address onto which to apply > > cache maintenance. > > > > Fix the ION begin_cpu_access and end_cpu_access functions to only apply > > cache maintenance to buffers which have been dma mapped. > > > I think this looks okay. I was initially concerned about concurrency and > setting the dma_mapped flag but I think that should be handled by the > caller synchronizing map/unmap/cpu_access calls (we might need to re-evaluate > in the future) I had convinced myself that concurrency wasn't a problem, but you are right it does need to be re-evaluated. For example the code could be at the point after the dma unmap call has completed but before dma_mapped has been set to false, and if userspace happened to slip in a call to begin/end cpu access cache maintenance would happen on memory which isn't dma mapped. So at least this would need to be addressed, maybe for this issue just move the setting of dma_mapped to the start of the ion_unmap_dma_buf function. I can clean this up and any other concurrency issues we can identify. > > I would like to hold on queuing this for just a little bit until I > finish working on the Ion unit test (doing this in the complete opposite > order of course). I'm assuming this passed your internal tests Liam? Yes it has passed my internal ION unit tests, though I haven't given the change to internal ION clients yet. Liam Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
staging: android: ion: potential coherency issue
Hello, Correct me if I'm wrong, but there is no user space interface, similar to the dma_buf_start_cpu_access()/dma_buf_end_cpu_access() to handle IO coherency. That is, on the platforms, where the SW is responsible for IO coherency handling, the following sequences: - write to the buffer - DMA read or - DMA write - read from the buffer may be problematic. Thanks, Alexey ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/5] mtd: Stop updating erase_info->state and calling mtd_erase_callback()
Hi Boris, Just a few comments about the form. Otherwise: Reviewed-by: Miquel Raynal > diff --git a/drivers/mtd/devices/lart.c b/drivers/mtd/devices/lart.c > index 555b94406e0b..3d6c8ffd351f 100644 > --- a/drivers/mtd/devices/lart.c > +++ b/drivers/mtd/devices/lart.c > @@ -415,7 +415,6 @@ static int flash_erase (struct mtd_info *mtd,struct > erase_info *instr) >{ > if (!erase_block (addr)) > { > - instr->state = MTD_ERASE_FAILED; >return (-EIO); > } You can also safely remove these '{' '}' > > @@ -425,9 +424,6 @@ static int flash_erase (struct mtd_info *mtd,struct > erase_info *instr) > if (addr == mtd->eraseregions[i].offset + > (mtd->eraseregions[i].erasesize * mtd->eraseregions[i].numblocks)) i++; >} > > - instr->state = MTD_ERASE_DONE; > - mtd_erase_callback(instr); > - > return (0); > } > > diff --git a/drivers/mtd/devices/mtd_dataflash.c > b/drivers/mtd/devices/mtd_dataflash.c > index 5dc8bd042cc5..aaaeaae01e1d 100644 > --- a/drivers/mtd/devices/mtd_dataflash.c > +++ b/drivers/mtd/devices/mtd_dataflash.c > @@ -220,10 +220,6 @@ static int dataflash_erase(struct mtd_info *mtd, struct > erase_info *instr) > } > mutex_unlock(&priv->lock); > > - /* Inform MTD subsystem that erase is complete */ > - instr->state = MTD_ERASE_DONE; > - mtd_erase_callback(instr); > - > return 0; > } > > diff --git a/drivers/mtd/devices/mtdram.c b/drivers/mtd/devices/mtdram.c > index 0bf4aeaf0cb8..efef43c6684b 100644 > --- a/drivers/mtd/devices/mtdram.c > +++ b/drivers/mtd/devices/mtdram.c > @@ -60,8 +60,6 @@ static int ram_erase(struct mtd_info *mtd, struct > erase_info *instr) > if (check_offs_len(mtd, instr->addr, instr->len)) > return -EINVAL; > memset((char *)mtd->priv + instr->addr, 0xff, instr->len); > - instr->state = MTD_ERASE_DONE; > - mtd_erase_callback(instr); Space ? > return 0; > } > > diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c > index 7287696a21f9..a963c88d392d 100644 > --- a/drivers/mtd/devices/phram.c > +++ b/drivers/mtd/devices/phram.c > @@ -44,8 +44,6 @@ static int phram_erase(struct mtd_info *mtd, struct > erase_info *instr) >* I don't feel at all ashamed. This kind of thing is possible anyway >* with flash, but unlikely. >*/ Not sure this comment is still relevant? Maybe you could remove it or at least change it? > - instr->state = MTD_ERASE_DONE; > - mtd_erase_callback(instr); Space ? > return 0; > } > > diff --git a/drivers/mtd/devices/pmc551.c b/drivers/mtd/devices/pmc551.c > index cadea0620cd0..5d842cbca3de 100644 > --- a/drivers/mtd/devices/pmc551.c > +++ b/drivers/mtd/devices/pmc551.c > @@ -184,12 +184,10 @@ static int pmc551_erase(struct mtd_info *mtd, struct > erase_info *instr) > } > >out: > - instr->state = MTD_ERASE_DONE; > #ifdef CONFIG_MTD_PMC551_DEBUG > printk(KERN_DEBUG "pmc551_erase() done\n"); > #endif > > - mtd_erase_callback(instr); > return 0; > } > > diff --git a/drivers/mtd/devices/powernv_flash.c > b/drivers/mtd/devices/powernv_flash.c > index 26f9feaa5d17..5f383630c16f 100644 > --- a/drivers/mtd/devices/powernv_flash.c > +++ b/drivers/mtd/devices/powernv_flash.c > @@ -175,19 +175,12 @@ static int powernv_flash_erase(struct mtd_info *mtd, > struct erase_info *erase) > { > int rc; > > - erase->state = MTD_ERASING; > - > /* todo: register our own notifier to do a true async implementation */ > rc = powernv_flash_async_op(mtd, FLASH_OP_ERASE, erase->addr, > erase->len, NULL, NULL); Are you sure this is still needed? Maybe this should go away in your first patch? > - > - if (rc) { > + if (rc) > erase->fail_addr = erase->addr; > - erase->state = MTD_ERASE_FAILED; > - } else { > - erase->state = MTD_ERASE_DONE; > - } > - mtd_erase_callback(erase); > + > return rc; > } > > diff --git a/drivers/mtd/devices/slram.c b/drivers/mtd/devices/slram.c > index 0ec85f316d24..2f05e1801047 100644 > --- a/drivers/mtd/devices/slram.c > +++ b/drivers/mtd/devices/slram.c > @@ -88,8 +88,6 @@ static int slram_erase(struct mtd_info *mtd, struct > erase_info *instr) >* I don't feel at all ashamed. This kind of thing is possible anyway >* with flash, but unlikely. >*/ Same with this comment. > - instr->state = MTD_ERASE_DONE; > - mtd_erase_callback(instr); Space ? > return(0); > } > -- Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: KASAN: use-after-free Read in remove_wait_queue
On Mon, Feb 12, 2018 at 7:31 PM, Al Viro wrote: > Any chance of bisecting it? Perhaps my fix introduced another (related) problem, I'm looking into it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel