Re: [PATCH] android: binder: Disable preemption while holding the global binder lock

[Arve Hjønnevåg]

On Sat, Sep 10, 2016 at 10:28 AM, Greg Kroah-Hartman
 wrote:
> On Sat, Sep 10, 2016 at 06:37:29PM +0200, Thomas Gleixner wrote:
>> On Sat, 10 Sep 2016, Peter Zijlstra wrote:
>>
>> > On Sat, Sep 10, 2016 at 09:16:59AM -0700, Christoph Hellwig wrote:
>> > > On Thu, Sep 08, 2016 at 09:12:50AM -0700, Todd Kjos wrote:
>> > > > In Android systems, the display pipeline relies on low
>> > > > latency binder transactions and is therefore sensitive to
>> > > > delays caused by contention for the global binder lock.
>> > > > Jank is siginificantly reduced by disabling preemption
>> > > > while the global binder lock is held.
>> > >
>> > > That's now how preempt_disable is supposed to use.  It is for critical
>> >
>> > not, that's supposed to be _not_. Just to be absolutely clear, this is
>> > NOT how you're supposed to use preempt_disable().
>> >
>> > > sections that use per-cpu or similar resources.
>> > >
>> > > >
>> > > > Originally-from: Riley Andrews 
>> > > > Signed-off-by: Todd Kjos 
>> >
>> > > > @@ -389,7 +390,11 @@ static int task_get_unused_fd_flags(struct
>> > > > binder_proc *proc, int flags)
>> > > >   rlim_cur = task_rlimit(proc->tsk, RLIMIT_NOFILE);
>> > > >   unlock_task_sighand(proc->tsk, );
>> > > >
>> > > > - return __alloc_fd(files, 0, rlim_cur, flags);
>> > > > + preempt_enable_no_resched();
>> > > > + ret = __alloc_fd(files, 0, rlim_cur, flags);
>> > > > + preempt_disable();
>> >
>> > And the fact that people want to use preempt_enable_no_resched() shows
>> > that they're absolutely clueless.
>> >
>> > This is so broken its not funny.
>> >
>> > NAK NAK NAK
>>
>> Indeed. Sprinkling random preempt_enabe/disable() pairs all over the place
>> documents clearly that this is tinkering and not proper software
>> engineering.
>
> I have pointed out in the other thread for this patch (the one that had
> a patch that could be applied) that the single lock in the binder code
> is the main problem here, it should be solved instead of this messing
> around with priorities.
>

While removing the single lock in the binder driver would help reduce
the problem that this patch tries to work around, it would not fix it.
The largest problems occur when a very low priority thread gets
preempted while holding the lock. When a high priority thread then
needs the same lock it can't get it. Changing the driver to use more
fine-grained locking would reduce the set of threads that can trigger
this problem, but there are processes that receive work from both high
and low priority threads and could still end up in the same situation.

A previous attempt to fix this problem, changed the lock to use
rt_mutex instead of mutex, but this apparently did not work as well as
this patch. I believe the added overhead was noticeable, and it did
not work when the preempted thread was in a different cgroup (I don't
know if this is still the case).

It would be useful to generic solution to this problem.

> So don't worry, I'm not taking this change :)
>
> thanks,
>
> greg k-h



-- 
Arve Hjønnevåg
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] hv: do not lose pending heartbeat vmbus packets

[Long Li]

From: Long Li 

The host keeps sending heartbeat packets independent of guest responding to 
them. In some situations, there might be multiple heartbeat packets pending in 
the ring buffer. Don't lose them, read them all.

Signed-off-by: Long Li 
---
 drivers/hv/hv_util.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index d5acaa2..9dc6372 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -283,10 +283,14 @@ static void heartbeat_onchannelcallback(void *context)
u8 *hbeat_txf_buf = util_heartbeat.recv_buffer;
struct icmsg_negotiate *negop = NULL;
 
-   vmbus_recvpacket(channel, hbeat_txf_buf,
-PAGE_SIZE, , );
+   while (1) {
+
+   vmbus_recvpacket(channel, hbeat_txf_buf,
+PAGE_SIZE, , );
+
+   if (!recvlen)
+   break;
 
-   if (recvlen > 0) {
icmsghdrp = (struct icmsg_hdr *)_txf_buf[
sizeof(struct vmbuspipe_hdr)];
 
-- 
1.8.5.6

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

drivers: staging: vme: Fixed some code style warnings

[Andrew Kanner]

Signed-off-by: Andrew Kanner 
---
 drivers/staging/vme/devices/vme_pio2_core.c | 12 ++--
 drivers/staging/vme/devices/vme_user.c  |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_pio2_core.c 
b/drivers/staging/vme/devices/vme_pio2_core.c
index 28a4568..8e66a52 100644
--- a/drivers/staging/vme/devices/vme_pio2_core.c
+++ b/drivers/staging/vme/devices/vme_pio2_core.c
@@ -466,23 +466,23 @@ static void __exit pio2_exit(void)
 
 /* These are required for each board */
 MODULE_PARM_DESC(bus, "Enumeration of VMEbus to which the board is connected");
-module_param_array(bus, int, _num, S_IRUGO);
+module_param_array(bus, int, _num, 0444);
 
 MODULE_PARM_DESC(base, "Base VME address for PIO2 Registers");
-module_param_array(base, long, _num, S_IRUGO);
+module_param_array(base, long, _num, 0444);
 
 MODULE_PARM_DESC(vector, "VME IRQ Vector (Lower 4 bits masked)");
-module_param_array(vector, int, _num, S_IRUGO);
+module_param_array(vector, int, _num, 0444);
 
 MODULE_PARM_DESC(level, "VME IRQ Level");
-module_param_array(level, int, _num, S_IRUGO);
+module_param_array(level, int, _num, 0444);
 
 MODULE_PARM_DESC(variant, "Last 4 characters of PIO2 board variant");
-module_param_array(variant, charp, _num, S_IRUGO);
+module_param_array(variant, charp, _num, 0444);
 
 /* This is for debugging */
 MODULE_PARM_DESC(loopback, "Enable loopback mode on all cards");
-module_param(loopback, bool, S_IRUGO);
+module_param(loopback, bool, 0444);
 
 MODULE_DESCRIPTION("GE PIO2 6U VME I/O Driver");
 MODULE_AUTHOR("Martyn Welch 

[PATCH 2/2] pci-hyperv: properly handle device eject

[Long Li]

From: Long Li 

A PCI_EJECT message can arrive at the same time we are calling 
pci_scan_child_bus in the workqueue for the previous PCI_BUS_RELATIONS message, 
in this case we could potentailly modify the bus from two places. Properly lock 
the bus access.

Signed-off-by: Long Li 
---
 drivers/pci/host/pci-hyperv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index 3c2b330..ca77009 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -1587,7 +1587,7 @@ static void hv_eject_device_work(struct work_struct *work)
pdev = pci_get_domain_bus_and_slot(hpdev->hbus->sysdata.domain, 0,
   wslot);
if (pdev) {
-   pci_stop_and_remove_bus_device(pdev);
+   pci_stop_and_remove_bus_device_locked(pdev);
pci_dev_put(pdev);
}
 
-- 
1.8.5.6

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2] pci-hyperv: properly handle pci bus remove

[Long Li]

From: Long Li 

hv_pci_devices_present is called in hv_pci_remove when we remove a PCI device 
from host (e.g. by disabling SRIOV on a device). In hv_pci_remove, the bus is 
already removed before the call, so we don't need to rescan the bus in the 
workqueue scheduled from hv_pci_devices_present. By introducing status 
hv_pcibus_removed, we can avoid this situation.

The patch fixes the following kernel panic.

[  383.853124] Workqueue: events pci_devices_present_work [pci_hyperv]
[  383.853124] task: 88007f5f8000 ti: 88007f60 task.ti:
88007f60
[  383.853124] RIP: 0010:[]  []
pci_is_pcie+0x6/0x20
[  383.853124] RSP: 0018:88007f603d38  EFLAGS: 00010206
[  383.853124] RAX: 88007f5f8000 RBX: 642f3d4854415056 RCX:
88007f603fd8
[  383.853124] RDX:  RSI:  RDI:
642f3d4854415056
[  383.853124] RBP: 88007f603d68 R08: 0246 R09:
a045eb9e
[  383.853124] R10: 88007b419a80 R11: ea0001c0ef40 R12:
880003ee1c00
[  383.853124] R13: 63702f30303a3137 R14:  R15:
0246
[  383.853124] FS:  () GS:88007b40()
knlGS:
[  383.853124] CS:  0010 DS:  ES:  CR0: 80050033
[  383.853124] CR2: 7f68b3f52350 CR3: 03546000 CR4:
000406f0
[  383.853124] DR0:  DR1:  DR2:

[  383.853124] DR3:  DR6: 0ff0 DR7:
0400
[  383.853124] Stack:
[  383.853124]  88007f603d68 8134db17 0008
880003ee1c00
[  383.853124]  63702f30303a3137 880003d8edb8 88007f603da0
8134ee2d
[  383.853124]  880003d8ed00 88007f603dd8 880075fec320
880003d8edb8
[  383.853124] Call Trace:
[  383.853124]  [] ? pci_scan_slot+0x27/0x140
[  383.853124]  [] pci_scan_child_bus+0x3d/0x150
[  383.853124]  []
pci_devices_present_work+0x3ea/0x400 [pci_hyperv]
[  383.853124]  [] process_one_work+0x17b/0x470
[  383.853124]  [] worker_thread+0x126/0x410
[  383.853124]  [] ? rescuer_thread+0x460/0x460
[  383.853124]  [] kthread+0xcf/0xe0
[  383.853124]  [] ?
kthread_create_on_node+0x140/0x140
[  383.853124]  [] ret_from_fork+0x58/0x90
[  383.853124]  [] ?
kthread_create_on_node+0x140/0x140
[  383.853124] Code: 89 e5 5d 25 f0 00 00 00 c1 f8 04 c3 66 0f 1f 84 00
00 00 00 00 66 66 66 66 90 55 0f b6 47 4a 48 89 e5 5d c3 90 66 66 66 66
90 55 <80> 7f 4a 00 48 89 e5 5d 0f 95 c0 c3 0f 1f 40 00 66 2e 0f 1f 84
[  383.853124] RIP  [] pci_is_pcie+0x6/0x20
[  383.853124]  RSP 

Signed-off-by: Long Li 
---
 drivers/pci/host/pci-hyperv.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index daa5fc3..26f049b 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -348,6 +348,7 @@ enum hv_pcibus_state {
hv_pcibus_init = 0,
hv_pcibus_probed,
hv_pcibus_installed,
+   hv_pcibus_removed,
hv_pcibus_maximum
 };
 
@@ -1481,13 +1482,24 @@ static void pci_devices_present_work(struct work_struct 
*work)
put_pcichild(hpdev, hv_pcidev_ref_initial);
}
 
-   /* Tell the core to rescan bus because there may have been changes. */
-   if (hbus->state == hv_pcibus_installed) {
+   switch (hbus->state) {
+   case hv_pcibus_installed:
+   /*
+* Tell the core to rescan bus
+* because there may have been changes.
+*/
pci_lock_rescan_remove();
pci_scan_child_bus(hbus->pci_bus);
pci_unlock_rescan_remove();
-   } else {
+   break;
+
+   case hv_pcibus_init:
+   case hv_pcibus_probed:
survey_child_resources(hbus);
+   break;
+
+   default:
+   break;
}
 
up(>enum_sem);
@@ -2163,6 +2175,7 @@ static int hv_pci_probe(struct hv_device *hdev,
hbus = kzalloc(sizeof(*hbus), GFP_KERNEL);
if (!hbus)
return -ENOMEM;
+   hbus->state = hv_pcibus_init;
 
/*
 * The PCI bus "domain" is what is called "segment" in ACPI and
@@ -2305,6 +2318,7 @@ static int hv_pci_remove(struct hv_device *hdev)
pci_stop_root_bus(hbus->pci_bus);
pci_remove_root_bus(hbus->pci_bus);
pci_unlock_rescan_remove();
+   hbus->state = hv_pcibus_removed;
}
 
ret = hv_send_resources_released(hdev);
-- 
1.8.5.6

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: comedi: comedi_pcmcia: Fixes multiple blank lines issue

[Joe Perches]

On Mon, 2016-09-12 at 19:07 +0100, Ian Abbott wrote:
> On 12/09/16 18:29, Namrata A Shettar wrote:
> > This patch fixes the checkpatch.pl  issue:
> -Please don't use multiple blank lines
[]
> diff --git a/drivers/staging/comedi/comedi_pcmcia.c
[]
> @@ -18,7 +18,6 @@
> 
>  #include 
>  #include 
> -
>  #include "comedi_pcmcia.h"
> 
>  /**
> 
> 
> But there is only one blank line, so why the warning about multiple 
> blank lines?

checkpatch doesn't emit a warning here.

Also the http://checkpatch.pl link is incorrect
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[RFCv3][PATCH 0/5] Cleanup Ion mapping/caching

[Laura Abbott]

Hi,

This is v3 on the attempt to remove the misuse of the DMA cache APIS from Ion.

As from before:
The APIs created are kernel_force_cache_clean and kernel_force_cache_invalidate.
They force a clean and invalidate of the cache, respectively. The aim was to
take the semantics of dma_sync and turn them into something that isn't
dma_sync. This series includes a nominal implementation for arm/arm64, mostly
for demonstration purposes.

The major change from v2 is that the implementations no longer leverage the
DMA abstractions. Russell King noted that dma_map and dma_unmap just 'happen'
to do the right thing but they aren't guaranteed.

I'm hoping at v3 there are no objections to the general concept but if they
exist please express them.

Thanks,
Laura

[1]http://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg49406.html

Laura Abbott (5):
  Documentation: Introduce kernel_force_cache_* APIs
  arm: Impelment ARCH_HAS_FORCE_CACHE
  arm64: Implement ARCH_HAS_FORCE_CACHE
  staging: android: ion: Convert to the kernel_force_cache APIs
  staging: ion: Add support for syncing with DMA_BUF_IOCTL_SYNC

 Documentation/cachetlb.txt  | 18 ++-
 arch/arm/include/asm/cacheflush.h   | 11 
 arch/arm/include/asm/glue-cache.h   |  2 +
 arch/arm/mm/Makefile|  2 +-
 arch/arm/mm/cache-fa.S  |  8 +++
 arch/arm/mm/cache-nop.S |  6 +++
 arch/arm/mm/cache-v4.S  | 10 
 arch/arm/mm/cache-v4wb.S|  8 +++
 arch/arm/mm/cache-v4wt.S|  8 +++
 arch/arm/mm/cache-v6.S  |  8 +++
 arch/arm/mm/cache-v7.S  | 13 +
 arch/arm/mm/cacheflush.c| 71 +
 arch/arm/mm/proc-arm920.S   |  8 +++
 arch/arm/mm/proc-arm922.S   |  8 +++
 arch/arm/mm/proc-arm925.S   |  8 +++
 arch/arm/mm/proc-arm926.S   |  8 +++
 arch/arm/mm/proc-feroceon.S | 11 
 arch/arm/mm/proc-macros.S   |  2 +
 arch/arm/mm/proc-xsc3.S |  9 
 arch/arm/mm/proc-xscale.S   |  9 
 arch/arm64/include/asm/cacheflush.h |  8 +++
 arch/arm64/mm/cache.S   | 24 +++--
 arch/arm64/mm/flush.c   | 11 
 drivers/staging/android/ion/ion.c   | 53 +++---
 drivers/staging/android/ion/ion_carveout_heap.c |  8 +--
 drivers/staging/android/ion/ion_chunk_heap.c| 12 +++--
 drivers/staging/android/ion/ion_page_pool.c |  7 +--
 drivers/staging/android/ion/ion_priv.h  | 11 
 drivers/staging/android/ion/ion_system_heap.c   |  6 +--
 include/linux/cacheflush.h  | 11 
 30 files changed, 330 insertions(+), 49 deletions(-)
 create mode 100644 arch/arm/mm/cacheflush.c
 create mode 100644 include/linux/cacheflush.h

-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[RFCv3][PATCH 5/5] staging: ion: Add support for syncing with DMA_BUF_IOCTL_SYNC

[Laura Abbott]

dma_buf added support for a userspace syncing ioctl. It is implemented
by calling dma_buf_begin_cpu_access and dma_buf_end_cpu_access. Ion
currently lacks cache operations on this code path. Add them for
compatibility with the dma_buf ioctl.

Signed-off-by: Laura Abbott 
---
 drivers/staging/android/ion/ion.c | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index c2125de..1ad8e8a 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -969,6 +969,24 @@ static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, 
unsigned long offset,
 {
 }
 
+static void ion_clean_buffer(struct ion_buffer *buffer)
+{
+   struct scatterlist *sg;
+   int i;
+
+   for_each_sg(buffer->sg_table->sgl, sg, buffer->sg_table->orig_nents, i)
+   kernel_force_cache_clean(sg_page(sg), sg->length);
+}
+
+static void ion_invalidate_buffer(struct ion_buffer *buffer)
+{
+   struct scatterlist *sg;
+   int i;
+
+   for_each_sg(buffer->sg_table->sgl, sg, buffer->sg_table->orig_nents, i)
+   kernel_force_cache_invalidate(sg_page(sg), sg->length);
+}
+
 static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
enum dma_data_direction direction)
 {
@@ -984,6 +1002,11 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf 
*dmabuf,
mutex_lock(>lock);
vaddr = ion_buffer_kmap_get(buffer);
mutex_unlock(>lock);
+
+   if (direction != DMA_TO_DEVICE) {
+   ion_invalidate_buffer(buffer);
+   }
+
return PTR_ERR_OR_ZERO(vaddr);
 }
 
@@ -996,6 +1019,12 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf 
*dmabuf,
ion_buffer_kmap_put(buffer);
mutex_unlock(>lock);
 
+   if (direction == DMA_FROM_DEVICE) {
+   ion_invalidate_buffer(buffer);
+   } else {
+   ion_clean_buffer(buffer);
+   }
+
return 0;
 }
 
@@ -1126,6 +1155,8 @@ int ion_sync_for_device(struct ion_client *client, int fd)
struct dma_buf *dmabuf;
struct ion_buffer *buffer;
 
+   WARN_ONCE(1, "This API is deprecated in favor of the dma_buf ioctl\n");
+
dmabuf = dma_buf_get(fd);
if (IS_ERR(dmabuf))
return PTR_ERR(dmabuf);
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[RFCv3][PATCH 4/5] staging: android: ion: Convert to the kernel_force_cache APIs

[Laura Abbott]

Now that there exists a proper set of cache sync APIs, move away
from the dma_sync and do less bad things.

Signed-off-by: Laura Abbott 
---
v3: Rebased to latest-next
---
 drivers/staging/android/ion/ion.c   | 22 --
 drivers/staging/android/ion/ion_carveout_heap.c |  8 +---
 drivers/staging/android/ion/ion_chunk_heap.c| 12 +++-
 drivers/staging/android/ion/ion_page_pool.c |  7 ---
 drivers/staging/android/ion/ion_priv.h  | 11 ---
 drivers/staging/android/ion/ion_system_heap.c   |  6 +++---
 6 files changed, 23 insertions(+), 43 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 396ded5..c2125de 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -37,6 +37,8 @@
 #include 
 #include 
 
+#include 
+
 #include "ion.h"
 #include "ion_priv.h"
 #include "compat_ion.h"
@@ -817,22 +819,6 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment 
*attachment,
 {
 }
 
-void ion_pages_sync_for_device(struct device *dev, struct page *page,
-  size_t size, enum dma_data_direction dir)
-{
-   struct scatterlist sg;
-
-   sg_init_table(, 1);
-   sg_set_page(, page, size, 0);
-   /*
-* This is not correct - sg_dma_address needs a dma_addr_t that is valid
-* for the targeted device, but this works on the currently targeted
-* hardware.
-*/
-   sg_dma_address() = page_to_phys(page);
-   dma_sync_sg_for_device(dev, , 1, dir);
-}
-
 struct ion_vma_list {
struct list_head list;
struct vm_area_struct *vma;
@@ -857,8 +843,8 @@ static void ion_buffer_sync_for_device(struct ion_buffer 
*buffer,
struct page *page = buffer->pages[i];
 
if (ion_buffer_page_is_dirty(page))
-   ion_pages_sync_for_device(dev, ion_buffer_page(page),
- PAGE_SIZE, dir);
+   kernel_force_cache_clean(ion_buffer_page(page),
+PAGE_SIZE);
 
ion_buffer_page_clean(buffer->pages + i);
}
diff --git a/drivers/staging/android/ion/ion_carveout_heap.c 
b/drivers/staging/android/ion/ion_carveout_heap.c
index c4f0795..af81edc 100644
--- a/drivers/staging/android/ion/ion_carveout_heap.c
+++ b/drivers/staging/android/ion/ion_carveout_heap.c
@@ -22,6 +22,9 @@
 #include 
 #include 
 #include 
+
+#include 
+
 #include "ion.h"
 #include "ion_priv.h"
 
@@ -105,8 +108,7 @@ static void ion_carveout_heap_free(struct ion_buffer 
*buffer)
ion_heap_buffer_zero(buffer);
 
if (ion_buffer_cached(buffer))
-   dma_sync_sg_for_device(NULL, table->sgl, table->nents,
-  DMA_BIDIRECTIONAL);
+   kernel_force_cache_clean(page, buffer->size);
 
ion_carveout_free(heap, paddr, buffer->size);
sg_free_table(table);
@@ -132,7 +134,7 @@ struct ion_heap *ion_carveout_heap_create(struct 
ion_platform_heap *heap_data)
page = pfn_to_page(PFN_DOWN(heap_data->base));
size = heap_data->size;
 
-   ion_pages_sync_for_device(NULL, page, size, DMA_BIDIRECTIONAL);
+   kernel_force_cache_clean(page, size);
 
ret = ion_heap_pages_zero(page, size, pgprot_writecombine(PAGE_KERNEL));
if (ret)
diff --git a/drivers/staging/android/ion/ion_chunk_heap.c 
b/drivers/staging/android/ion/ion_chunk_heap.c
index 70495dc..f6d1bae 100644
--- a/drivers/staging/android/ion/ion_chunk_heap.c
+++ b/drivers/staging/android/ion/ion_chunk_heap.c
@@ -21,6 +21,9 @@
 #include 
 #include 
 #include 
+
+#include 
+
 #include "ion.h"
 #include "ion_priv.h"
 
@@ -104,11 +107,10 @@ static void ion_chunk_heap_free(struct ion_buffer *buffer)
 
ion_heap_buffer_zero(buffer);
 
-   if (ion_buffer_cached(buffer))
-   dma_sync_sg_for_device(NULL, table->sgl, table->nents,
-  DMA_BIDIRECTIONAL);
-
for_each_sg(table->sgl, sg, table->nents, i) {
+   if (ion_buffer_cached(buffer))
+   kernel_force_cache_clean(sg_page(table->sgl),
+sg->length);
gen_pool_free(chunk_heap->pool, page_to_phys(sg_page(sg)),
  sg->length);
}
@@ -135,7 +137,7 @@ struct ion_heap *ion_chunk_heap_create(struct 
ion_platform_heap *heap_data)
page = pfn_to_page(PFN_DOWN(heap_data->base));
size = heap_data->size;
 
-   ion_pages_sync_for_device(NULL, page, size, DMA_BIDIRECTIONAL);
+   kernel_force_cache_clean(page, size);
 
ret = ion_heap_pages_zero(page, size, pgprot_writecombine(PAGE_KERNEL));
if (ret)
diff --git a/drivers/staging/android/ion/ion_page_pool.c 
b/drivers/staging/android/ion/ion_page_pool.c
index aea89c1..f289d88 100644
--- 

[RFCv3][PATCH 3/5] arm64: Implement ARCH_HAS_FORCE_CACHE

[Laura Abbott]

arm64 may need to guarantee the caches are synced. Implement versions of
the kernel_force_cache API to allow this.

Signed-off-by: Laura Abbott 
---
v3: Switch to calling cache operations directly instead of relying on
DMA mapping.
---
 arch/arm64/include/asm/cacheflush.h |  8 
 arch/arm64/mm/cache.S   | 24 
 arch/arm64/mm/flush.c   | 11 +++
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/cacheflush.h 
b/arch/arm64/include/asm/cacheflush.h
index c64268d..1134c15 100644
--- a/arch/arm64/include/asm/cacheflush.h
+++ b/arch/arm64/include/asm/cacheflush.h
@@ -87,6 +87,9 @@ extern void __dma_map_area(const void *, size_t, int);
 extern void __dma_unmap_area(const void *, size_t, int);
 extern void __dma_flush_range(const void *, const void *);
 
+extern void __force_dcache_clean(const void *, size_t);
+extern void __force_dcache_invalidate(const void *, size_t);
+
 /*
  * Copy user data from/to a page which is mapped into a different
  * processes address space.  Really, we want to allow our "user
@@ -149,4 +152,9 @@ int set_memory_rw(unsigned long addr, int numpages);
 int set_memory_x(unsigned long addr, int numpages);
 int set_memory_nx(unsigned long addr, int numpages);
 
+#define ARCH_HAS_FORCE_CACHE 1
+
+void kernel_force_cache_clean(struct page *page, size_t size);
+void kernel_force_cache_invalidate(struct page *page, size_t size);
+
 #endif
diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S
index 07d7352..e99c9a4 100644
--- a/arch/arm64/mm/cache.S
+++ b/arch/arm64/mm/cache.S
@@ -184,10 +184,6 @@ ENDPIPROC(__dma_flush_range)
  * - dir   - DMA direction
  */
 ENTRY(__dma_map_area)
-   add x1, x1, x0
-   cmp w2, #DMA_FROM_DEVICE
-   b.eq__dma_inv_range
-   b   __dma_clean_range
 ENDPIPROC(__dma_map_area)
 
 /*
@@ -202,3 +198,23 @@ ENTRY(__dma_unmap_area)
b.ne__dma_inv_range
ret
 ENDPIPROC(__dma_unmap_area)
+
+/*
+ * __force_dcache_clean(start, size)
+ * - start - kernel virtual start address
+ * - size  - size of region
+ */
+ENTRY(__force_dcache_clean)
+   add x1, x1, x0
+   b   __dma_clean_range
+ENDPROC(__force_dcache_clean)
+
+/*
+ * __force_dcache_invalidate(start, size)
+ * - start - kernel virtual start address
+ * - size  - size of region
+ */
+ENTRY(__force_dcache_invalidate)
+   add x1, x1, x0
+   b   __dma_inv_range
+ENDPROC(__force_dcache_invalidate)
diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
index 43a76b0..54ff32e 100644
--- a/arch/arm64/mm/flush.c
+++ b/arch/arm64/mm/flush.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -94,3 +95,13 @@ EXPORT_SYMBOL(flush_dcache_page);
  * Additional functions defined in assembly.
  */
 EXPORT_SYMBOL(flush_icache_range);
+
+void kernel_force_cache_clean(struct page *page, size_t size)
+{
+   __force_dcache_clean(page_address(page), size);
+}
+
+void kernel_force_cache_invalidate(struct page *page, size_t size)
+{
+   __force_dcache_invalidate(page_address(page), size);
+}
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[RFCv3][PATCH 1/5] Documentation: Introduce kernel_force_cache_* APIs

[Laura Abbott]

Some frameworks (e.g. Ion) may need to do explicit cache management
to meet performance/correctness requirements. Rather than piggy-back
on another API and hope the semantics don't change, introduce a
set of APIs to force a page to be cleaned/invalidated in the cache.

Signed-off-by: Laura Abbott 
---
 Documentation/cachetlb.txt | 18 +-
 include/linux/cacheflush.h | 11 +++
 2 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/cacheflush.h

diff --git a/Documentation/cachetlb.txt b/Documentation/cachetlb.txt
index 3f9f808..18eec7c 100644
--- a/Documentation/cachetlb.txt
+++ b/Documentation/cachetlb.txt
@@ -378,7 +378,7 @@ maps this page at its virtual address.
flush_dcache_page and update_mmu_cache. In the future, the hope
is to remove this interface completely.
 
-The final category of APIs is for I/O to deliberately aliased address
+Another set of APIs is for I/O to deliberately aliased address
 ranges inside the kernel.  Such aliases are set up by use of the
 vmap/vmalloc API.  Since kernel I/O goes via physical pages, the I/O
 subsystem assumes that the user mapping and kernel offset mapping are
@@ -401,3 +401,19 @@ I/O and invalidating it after the I/O returns.
speculatively reading data while the I/O was occurring to the
physical pages.  This is only necessary for data reads into the
vmap area.
+
+Nearly all drivers can handle cache management using the existing DMA model.
+There may be limited circumstances when a driver or framework needs to
+explicitly manage the cache; trying to force cache management into the DMA
+framework may lead to performance loss or unnecessary work. These APIs may
+be used to provide explicit coherency for memory that does not fall into
+any of the above categories. Implementers of this API must assume the
+address can be aliased. Any cache operations shall not be delayed and must
+be completed by the time the call returns.
+
+   void kernel_force_cache_clean(struct page *page, size_t size);
+   Ensures that any data in the cache by the page is written back
+and visible across all aliases.
+
+   void kernel_force_cache_invalidate(struct page *page, size_t size);
+   Invalidates the cache for the given page.
diff --git a/include/linux/cacheflush.h b/include/linux/cacheflush.h
new file mode 100644
index 000..4388846
--- /dev/null
+++ b/include/linux/cacheflush.h
@@ -0,0 +1,11 @@
+#ifndef CACHEFLUSH_H
+#define CACHEFLUSH_H
+
+#include 
+
+#ifndef ARCH_HAS_FORCE_CACHE
+static inline void kernel_force_cache_clean(struct page *page, size_t size) { }
+static inline void kernel_force_cache_invalidate(struct page *page, size_t 
size) { }
+#endif
+
+#endif
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[RFCv3][PATCH 2/5] arm: Impelment ARCH_HAS_FORCE_CACHE

[Laura Abbott]

arm64 may need to guarantee the caches are synced. Implement
versions of the kernel_force_cache API for v7. Other versions
are stubbed out and can be added as appropriate.

Signed-off-by: Laura Abbott 
---
v3: Switch to force implementations per CPU instead of relying
on dma_map/dma_unmap. v7 is the only one implemented right
now, others can be added as needed.
---
 arch/arm/include/asm/cacheflush.h | 11 ++
 arch/arm/include/asm/glue-cache.h |  2 ++
 arch/arm/mm/Makefile  |  2 +-
 arch/arm/mm/cache-fa.S|  8 +
 arch/arm/mm/cache-nop.S   |  6 
 arch/arm/mm/cache-v4.S| 10 ++
 arch/arm/mm/cache-v4wb.S  |  8 +
 arch/arm/mm/cache-v4wt.S  |  8 +
 arch/arm/mm/cache-v6.S|  8 +
 arch/arm/mm/cache-v7.S| 13 +++
 arch/arm/mm/cacheflush.c  | 71 +++
 arch/arm/mm/proc-arm920.S |  8 +
 arch/arm/mm/proc-arm922.S |  8 +
 arch/arm/mm/proc-arm925.S |  8 +
 arch/arm/mm/proc-arm926.S |  8 +
 arch/arm/mm/proc-feroceon.S   | 11 ++
 arch/arm/mm/proc-macros.S |  2 ++
 arch/arm/mm/proc-xsc3.S   |  9 +
 arch/arm/mm/proc-xscale.S |  9 +
 19 files changed, 209 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mm/cacheflush.c

diff --git a/arch/arm/include/asm/cacheflush.h 
b/arch/arm/include/asm/cacheflush.h
index 9156fc3..2d9a4d3 100644
--- a/arch/arm/include/asm/cacheflush.h
+++ b/arch/arm/include/asm/cacheflush.h
@@ -116,6 +116,9 @@ struct cpu_cache_fns {
void (*dma_unmap_area)(const void *, size_t, int);
 
void (*dma_flush_range)(const void *, const void *);
+
+   void (*force_dcache_clean)(void *, size_t);
+   void (*force_dcache_invalidate)(void *, size_t);
 };
 
 /*
@@ -133,6 +136,8 @@ extern struct cpu_cache_fns cpu_cache;
 #define __cpuc_coherent_kern_range cpu_cache.coherent_kern_range
 #define __cpuc_coherent_user_range cpu_cache.coherent_user_range
 #define __cpuc_flush_dcache_area   cpu_cache.flush_kern_dcache_area
+#define __cpuc_force_dcache_clean  cpu_cache.force_dcache_clean
+#define __cpuc_force_dcache_invalidate cpu_cache.force_dcache_invalidate
 
 /*
  * These are private to the dma-mapping API.  Do not use directly.
@@ -152,6 +157,8 @@ extern void __cpuc_flush_user_range(unsigned long, unsigned 
long, unsigned int);
 extern void __cpuc_coherent_kern_range(unsigned long, unsigned long);
 extern int  __cpuc_coherent_user_range(unsigned long, unsigned long);
 extern void __cpuc_flush_dcache_area(void *, size_t);
+extern void __cpuc_force_dcache_clean(void *, size_t);
+extern void __cpuc_force_dcache_invalidate(void *, size_t);
 
 /*
  * These are private to the dma-mapping API.  Do not use directly.
@@ -518,4 +525,8 @@ static inline void secure_flush_area(const void *addr, 
size_t size)
outer_flush_range(phys, phys + size);
 }
 
+#define ARCH_HAS_FORCE_CACHE 1
+void kernel_force_cache_clean(struct page *page, size_t size);
+void kernel_force_cache_invalidate(struct page *page, size_t size);
+
 #endif
diff --git a/arch/arm/include/asm/glue-cache.h 
b/arch/arm/include/asm/glue-cache.h
index cab07f6..232938f 100644
--- a/arch/arm/include/asm/glue-cache.h
+++ b/arch/arm/include/asm/glue-cache.h
@@ -157,6 +157,8 @@ static inline void nop_dma_unmap_area(const void *s, size_t 
l, int f) { }
 #define __cpuc_coherent_kern_range __glue(_CACHE,_coherent_kern_range)
 #define __cpuc_coherent_user_range __glue(_CACHE,_coherent_user_range)
 #define __cpuc_flush_dcache_area   __glue(_CACHE,_flush_kern_dcache_area)
+#define __cpuc_force_dcache_clean  __glue(_CACHE,_force_dcache_clean)
+#define __cpuc_force_dcache_invalidate __glue(_CACHE,_force_dcache_invalidate)
 
 #define dmac_flush_range   __glue(_CACHE,_dma_flush_range)
 #endif
diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
index 7f76d96..3afcdd0 100644
--- a/arch/arm/mm/Makefile
+++ b/arch/arm/mm/Makefile
@@ -3,7 +3,7 @@
 #
 
 obj-y  := dma-mapping.o extable.o fault.o init.o \
-  iomap.o
+  iomap.o cacheflush.o
 
 obj-$(CONFIG_MMU)  += fault-armv.o flush.o idmap.o ioremap.o \
   mmap.o pgd.o mmu.o pageattr.o
diff --git a/arch/arm/mm/cache-fa.S b/arch/arm/mm/cache-fa.S
index 2f0c588..f1fe5df 100644
--- a/arch/arm/mm/cache-fa.S
+++ b/arch/arm/mm/cache-fa.S
@@ -244,6 +244,14 @@ ENDPROC(fa_dma_unmap_area)
.globl  fa_flush_kern_cache_louis
.equfa_flush_kern_cache_louis, fa_flush_kern_cache_all
 
+ENTRY(fa_force_dcache_invalidate)
+   ret lr
+ENDPROC(fa_force_dcache_invalidate)
+
+ENTRY(fa_force_dcache_clean)
+   ret lr
+ENDPROC(fa_force_dcache_clean)
+
__INITDATA
 
@ define struct cpu_cache_fns (see  and proc-macros.S)
diff --git 

[PATCH] staging: rtl8712: fix block comments

[Shaily Sangwan]

This patch fixes the following checkpatch.pl warning:
Block comments use * on subsequent lines

Signed-off-by: Shaily Sangwan 
---
 drivers/staging/rtl8712/rtl8712_recv.h |  14 +-
 drivers/staging/rtl8712/rtl871x_cmd.h  | 264 -
 2 files changed, 139 insertions(+), 139 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl8712_recv.h 
b/drivers/staging/rtl8712/rtl8712_recv.h
index 96ca3e2..0b0c273 100644
--- a/drivers/staging/rtl8712/rtl8712_recv.h
+++ b/drivers/staging/rtl8712/rtl8712_recv.h
@@ -116,13 +116,13 @@ struct recv_buf {
 };
 
 /*
-   head  ->
-   data  ->
-   payload
-   tail  ->
-   end   ->
-   len = (unsigned int )(tail - data);
-*/
+ * head  ->
+ * data  ->
+ * payload
+ * tail  ->
+ * end   ->
+ * len = (unsigned int )(tail - data);
+ */
 struct recv_frame_hdr {
struct list_head list;
_pkt*pkt;
diff --git a/drivers/staging/rtl8712/rtl871x_cmd.h 
b/drivers/staging/rtl8712/rtl871x_cmd.h
index a2cee36..3284dcf 100644
--- a/drivers/staging/rtl8712/rtl871x_cmd.h
+++ b/drivers/staging/rtl8712/rtl871x_cmd.h
@@ -217,15 +217,15 @@ struct SetMacAddr_param {
 };
 
 /*
-Caller Ad-Hoc/AP
-
-Command -Rsp(AID == CAMID) mode
-
-This is to force fw to add an sta_data entry per driver's request.
-
-FW will write an cam entry associated with it.
-
-*/
+ * Caller Ad-Hoc/AP
+ *
+ * Command -Rsp(AID == CAMID) mode
+ *
+ * This is to force fw to add an sta_data entry per driver's request.
+ *
+ * FW will write an cam entry associated with it.
+ *
+ */
 struct set_assocsta_parm {
u8  addr[ETH_ALEN];
 };
@@ -236,27 +236,27 @@ struct set_assocsta_rsp {
 };
 
 /*
-   Caller Ad-Hoc/AP
-
-   Command mode
-
-   This is to force fw to del an sta_data entry per driver's request
-
-   FW will invalidate the cam entry associated with it.
-
-*/
+ * Caller Ad-Hoc/AP
+ *
+ * Command mode
+ *
+ * This is to force fw to del an sta_data entry per driver's request
+ *
+ * FW will invalidate the cam entry associated with it.
+ *
+ */
 struct del_assocsta_parm {
u8  addr[ETH_ALEN];
 };
 
 /*
-Caller Mode: AP/Ad-HoC(M)
-
-Notes: To notify fw that given staid has changed its power state
-
-Command Mode
-
-*/
+ * Caller Mode: AP/Ad-HoC(M)
+ *
+ * Notes: To notify fw that given staid has changed its power state
+ *
+ * Command Mode
+ *
+ */
 struct setstapwrstate_parm {
u8  staid;
u8  status;
@@ -264,25 +264,25 @@ struct setstapwrstate_parm {
 };
 
 /*
-Caller Mode: Any
-
-Notes: To setup the basic rate of RTL8711
-
-Command Mode
-
-*/
+ * Caller Mode: Any
+ *
+ * Notes: To setup the basic rate of RTL8711
+ *
+ * Command Mode
+ *
+ */
 struct setbasicrate_parm {
u8  basicrates[NumRates];
 };
 
 /*
-Caller Mode: Any
-
-Notes: To read the current basic rate
-
-Command-Rsp Mode
-
-*/
+ * Caller Mode: Any
+ *
+ * Notes: To read the current basic rate
+ *
+ * Command-Rsp Mode
+ *
+ */
 struct getbasicrate_parm {
u32 rsvd;
 };
@@ -292,13 +292,13 @@ struct getbasicrate_rsp {
 };
 
 /*
-Caller Mode: Any
-
-Notes: To setup the data rate of RTL8711
-
-Command Mode
-
-*/
+ * Caller Mode: Any
+ *
+ * Notes: To setup the data rate of RTL8711
+ *
+ * Command Mode
+ *
+ */
 struct setdatarate_parm {
u8  mac_id;
u8  datarates[NumRates];
@@ -334,13 +334,13 @@ struct SetChannelPlan_param {
 };
 
 /*
-Caller Mode: Any
-
-Notes: To read the current data rate
-
-Command-Rsp Mode
-
-*/
+ * Caller Mode: Any
+ *
+ * Notes: To read the current data rate
+ *
+ * Command-Rsp Mode
+ *
+ */
 struct getdatarate_parm {
u32 rsvd;
 
@@ -351,36 +351,36 @@ struct getdatarate_rsp {
 
 
 /*
-Caller Mode: Any
-AP: AP can use the info for the contents of beacon frame
-Infra: STA can use the info when sitesurveying
-Ad-HoC(M): Like AP
-Ad-HoC(C): Like STA
-
-
-Notes: To set the phy capability of the NIC
-
-Command Mode
-
-*/
+ * Caller Mode: Any
+ * AP: AP can use the info for the contents of beacon frame
+ * Infra: STA can use the info when sitesurveying
+ * Ad-HoC(M): Like AP
+ * Ad-HoC(C): Like STA
+ *
+ *
+ * Notes: To set the phy capability of the NIC
+ *
+ * Command Mode
+ *
+ */
 
 /*
-Caller Mode: Any
-
-Notes: To set the channel/modem/band
-This command will be used when channel/modem/band is changed.
-
-Command Mode
-
-*/
+ * Caller Mode: Any
+ *
+ * Notes: To set the channel/modem/band
+ * This command will be used when channel/modem/band is changed.
+ *
+ * Command Mode
+ *
+ */
 /*
-Caller Mode: Any
-
-Notes: To get the current setting of channel/modem/band
-
-Command-Rsp Mode
-
-*/
+ * Caller Mode: Any
+ *
+ * Notes: To get the current setting of channel/modem/band
+ *
+ * Command-Rsp 

Re: [PATCH 00/26] constify local structures

[Julia Lawall]

On Mon, 12 Sep 2016, Jarkko Sakkinen wrote:

> On Mon, Sep 12, 2016 at 04:43:58PM +0300, Felipe Balbi wrote:
> >
> > Hi,
> >
> > Jarkko Sakkinen  writes:
> > > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> > >>
> > >>
> > >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> > >>
> > >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> > >> > > Constify local structures.
> > >> > >
> > >> > > The semantic patch that makes this change is as follows:
> > >> > > (http://coccinelle.lip6.fr/)
> > >> >
> > >> > Just my two cents but:
> > >> >
> > >> > 1. You *can* use a static analysis too to find bugs or other issues.
> > >> > 2. However, you should manually do the commits and proper commit
> > >> >messages to subsystems based on your findings. And I generally think
> > >> >that if one contributes code one should also at least smoke test 
> > >> > changes
> > >> >somehow.
> > >> >
> > >> > I don't know if I'm alone with my opinion. I just think that one should
> > >> > also do the analysis part and not blindly create and submit patches.
> > >>
> > >> All of the patches are compile tested.  And the individual patches are
> > >
> > > Compile-testing is not testing. If you are not able to test a commit,
> > > you should explain why.
> >
> > Dude, Julia has been doing semantic patching for years already and
> > nobody has raised any concerns so far. There's already an expectation
> > that Coccinelle *works* and Julia's sematic patches are sound.
> >
> > Besides, adding 'const' is something that causes virtually no functional
> > changes to the point that build-testing is really all you need. Any
> > problems caused by adding 'const' to a definition will be seen by build
> > errors or warnings.
> >
> > Really, just stop with the pointless discussion and go read a bit about
> > Coccinelle and what semantic patches are giving you. The work done by
> > Julia and her peers are INRIA have measurable benefits.
> >
> > You're really making a thunderstorm in a glass of water.
>
> Hmm... I've been using coccinelle in cyclic basis for some time now.
> My comment was oversized but I didn't mean it to be impolite or attack
> of any kind for that matter.

No problem :)  Thanks for the feedback.

julia
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/26] constify local structures

[Jarkko Sakkinen]

On Mon, Sep 12, 2016 at 04:43:58PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Jarkko Sakkinen  writes:
> > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> >> 
> >> 
> >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> >> 
> >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> >> > > Constify local structures.
> >> > >
> >> > > The semantic patch that makes this change is as follows:
> >> > > (http://coccinelle.lip6.fr/)
> >> >
> >> > Just my two cents but:
> >> >
> >> > 1. You *can* use a static analysis too to find bugs or other issues.
> >> > 2. However, you should manually do the commits and proper commit
> >> >messages to subsystems based on your findings. And I generally think
> >> >that if one contributes code one should also at least smoke test 
> >> > changes
> >> >somehow.
> >> >
> >> > I don't know if I'm alone with my opinion. I just think that one should
> >> > also do the analysis part and not blindly create and submit patches.
> >> 
> >> All of the patches are compile tested.  And the individual patches are
> >
> > Compile-testing is not testing. If you are not able to test a commit,
> > you should explain why.
> 
> Dude, Julia has been doing semantic patching for years already and
> nobody has raised any concerns so far. There's already an expectation
> that Coccinelle *works* and Julia's sematic patches are sound.
> 
> Besides, adding 'const' is something that causes virtually no functional
> changes to the point that build-testing is really all you need. Any
> problems caused by adding 'const' to a definition will be seen by build
> errors or warnings.
> 
> Really, just stop with the pointless discussion and go read a bit about
> Coccinelle and what semantic patches are giving you. The work done by
> Julia and her peers are INRIA have measurable benefits.
> 
> You're really making a thunderstorm in a glass of water.

Hmm... I've been using coccinelle in cyclic basis for some time now.
My comment was oversized but I didn't mean it to be impolite or attack
of any kind for that matter.

> -- 
> balbi

/Jarkko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: squash lines for simple wrapper functions

[Jes Sorensen]

Masahiro Yamada  writes:
> Remove unneeded variables and assignments.
>
> While we are here, clean up the following as well:
>   - refactor rtl8723a_get_bcn_valid() a bit
>   - remove unneeded casts in sii164Get{Vendor,Device}ID()
>
> Signed-off-by: Masahiro Yamada 
> ---
>
>  drivers/staging/android/ion/ion.c|  6 +-
>  .../staging/lustre/lustre/obdclass/linux/linux-module.c  |  5 +
>  drivers/staging/lustre/lustre/ptlrpc/client.c|  5 +
>  drivers/staging/lustre/lustre/ptlrpc/events.c|  5 +
>  drivers/staging/rtl8723au/core/rtw_wlan_util.c   |  7 +--
>  drivers/staging/rtl8723au/hal/hal_com.c  |  6 +-
>  drivers/staging/sm750fb/ddk750_sii164.c  | 16 
> 
>  7 files changed, 10 insertions(+), 40 deletions(-)

1) Do not submit one giant patch modifying multiple drivers in one go
2) drivers/staging/rtl8723au is gone - had you followed 1), you wouldn't
   necessarily have had to respin this patch.
3) Consider if your changes, even if technically correct, actually
   improve the code (see below)

Jes

> diff --git a/drivers/staging/rtl8723au/core/rtw_wlan_util.c 
> b/drivers/staging/rtl8723au/core/rtw_wlan_util.c
> index 694cf17..7ab47f0 100644
> --- a/drivers/staging/rtl8723au/core/rtw_wlan_util.c
> +++ b/drivers/staging/rtl8723au/core/rtw_wlan_util.c
> @@ -1202,12 +1202,7 @@ unsigned int update_supported_rate23a(unsigned char 
> *ptn, unsigned int ptn_sz)
>  
>  unsigned int update_MSC_rate23a(struct ieee80211_ht_cap *pHT_caps)
>  {
> - unsigned int mask;
> -
> - mask = pHT_caps->mcs.rx_mask[0] << 12 |
> - pHT_caps->mcs.rx_mask[1] << 20;
> -
> - return mask;
> + return pHT_caps->mcs.rx_mask[0] << 12 | pHT_caps->mcs.rx_mask[1] << 20;
>  }

The use of the mask variable didn't cause any harm, and it was certainly
a lot nicer to read the way it was.

>  int support_short_GI23a(struct rtw_adapter *padapter,
> diff --git a/drivers/staging/rtl8723au/hal/hal_com.c 
> b/drivers/staging/rtl8723au/hal/hal_com.c
> index 9d7b11b..dfbeebe 100644
> --- a/drivers/staging/rtl8723au/hal/hal_com.c
> +++ b/drivers/staging/rtl8723au/hal/hal_com.c
> @@ -708,11 +708,7 @@ void rtl8723a_bcn_valid(struct rtw_adapter *padapter)
>  
>  bool rtl8723a_get_bcn_valid(struct rtw_adapter *padapter)
>  {
> - bool retval;
> -
> - retval = (rtl8723au_read8(padapter, REG_TDECTRL + 2) & BIT(0)) ? true : 
> false;
> -
> - return retval;
> + return !!(rtl8723au_read8(padapter, REG_TDECTRL + 2) & BIT(0));
>  }

One word: Yuck!

Talk about obfuscating the code for the sake of obfuscation!

>  void rtl8723a_set_beacon_interval(struct rtw_adapter *padapter, u16 interval)
> diff --git a/drivers/staging/sm750fb/ddk750_sii164.c 
> b/drivers/staging/sm750fb/ddk750_sii164.c
> index 67f36e7..28818e1 100644
> --- a/drivers/staging/sm750fb/ddk750_sii164.c
> +++ b/drivers/staging/sm750fb/ddk750_sii164.c
> @@ -36,12 +36,8 @@ static char *gDviCtrlChipName = "Silicon Image SiI 164";
>   */
>  unsigned short sii164GetVendorID(void)
>  {
> - unsigned short vendorID;
> -
> - vendorID = ((unsigned short) i2cReadReg(SII164_I2C_ADDRESS, 
> SII164_VENDOR_ID_HIGH) << 8) |
> - (unsigned short) i2cReadReg(SII164_I2C_ADDRESS, 
> SII164_VENDOR_ID_LOW);
> -
> - return vendorID;
> + return (i2cReadReg(SII164_I2C_ADDRESS, SII164_VENDOR_ID_HIGH) << 8) |
> + i2cReadReg(SII164_I2C_ADDRESS, SII164_VENDOR_ID_LOW);
>  }
>  
>  /*
> @@ -53,12 +49,8 @@ unsigned short sii164GetVendorID(void)
>   */
>  unsigned short sii164GetDeviceID(void)
>  {
> - unsigned short deviceID;
> -
> - deviceID = ((unsigned short) i2cReadReg(SII164_I2C_ADDRESS, 
> SII164_DEVICE_ID_HIGH) << 8) |
> - (unsigned short) i2cReadReg(SII164_I2C_ADDRESS, 
> SII164_DEVICE_ID_LOW);
> -
> - return deviceID;
> + return (i2cReadReg(SII164_I2C_ADDRESS, SII164_DEVICE_ID_HIGH) << 8) |
> + i2cReadReg(SII164_I2C_ADDRESS, SII164_DEVICE_ID_LOW);
>  }

Getting rid of the casts would be nice, merging them into a giant
multi-line return line certainly isn't an improvement in my book. That
said, I will leave that to the maintainer of that driver to decide what
is preferred.

Jes
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCHv3 1/3] devicetree: bindings for Ion

[Frank Rowand]

On 08/30/16 17:04, Laura Abbott wrote:
> This adds a base set of devicetree bindings for the Ion memory
> manager. This supports setting up the generic set of heaps and
> their properties.
> 
> Signed-off-by: Laura Abbott 
> ---
>  drivers/staging/android/ion/devicetree.txt | 51 
> ++
>  1 file changed, 51 insertions(+)
>  create mode 100644 drivers/staging/android/ion/devicetree.txt
> 
> diff --git a/drivers/staging/android/ion/devicetree.txt 
> b/drivers/staging/android/ion/devicetree.txt
> new file mode 100644
> index 000..16871527
> --- /dev/null
> +++ b/drivers/staging/android/ion/devicetree.txt
> @@ -0,0 +1,51 @@
> +Ion Memory Manager
> +
> +Ion is a memory manager that allows for sharing of buffers via dma-buf.
> +Ion allows for different types of allocation via an abstraction called
> +a 'heap'. A heap represents a specific type of memory. Each heap has
> +a different type. There can be multiple instances of the same heap
> +type.
> +
> +Specific heap instances are tied to heap IDs. Heap IDs are not to be 
> specified
> +in the devicetree.
> +
> +Required properties for Ion
> +
> +- compatible: "linux,ion" PLUS a compatible property for the device
> +
> +All child nodes of a linux,ion node are interpreted as heaps
> +
> +required properties for heaps
> +
> +- compatible: compatible string for a heap type PLUS a compatible property
> +for the specific instance of the heap. Current heap types
> +-- linux,ion-heap-system
> +-- linux,ion-heap-system-contig
> +-- linux,ion-heap-carveout
> +-- linux,ion-heap-chunk
> +-- linux,ion-heap-dma
> +-- linux,ion-heap-custom
> +
> +Optional properties
> +- memory-region: A phandle to a memory region. Required for DMA heap type
> +(see reserved-memory.txt for details on the reservation)
> +
> +Example:
> +
> + ion {
> + compatbile = "hisilicon,ion", "linux,ion";
> +
> + ion-system-heap {
> + compatbile = "hisilicon,system-heap", 
> "linux,ion-heap-system"
> + };
> +
> + ion-camera-region {
> + compatible = "hisilicon,camera-heap", 
> "linux,ion-heap-dma"
> + memory-region = <_region>;
> + };
> +
> + ion-fb-region {
> + compatbile = "hisilicon,fb-heap", "linux,ion-heap-dma"
> + memory-region = <_region>;
> + };
> + }
> 

This is extra complexity that does not appear to be needed.  As pointed out
in comments to earlier versions, the indirection pointing to memory regions
does not seem to be needed.  Why not just look for the ion memory regions in
the reserved-memory node?

The example in reserved-memory.txt does provide an example with the extra level
of indirection, but that is a different model where the nodes with references
to the reserved memory nodes are actually devices.  In the case of ion, the
heaps are not additional devices.

-Frank
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/26] constify local structures

[Jarkko Sakkinen]

On Mon, Sep 12, 2016 at 03:52:08PM +0200, Julia Lawall wrote:
> 
> 
> On Mon, 12 Sep 2016, Felipe Balbi wrote:
> 
> >
> > Hi,
> >
> > Jarkko Sakkinen  writes:
> > > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> > >>
> > >>
> > >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> > >>
> > >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> > >> > > Constify local structures.
> > >> > >
> > >> > > The semantic patch that makes this change is as follows:
> > >> > > (http://coccinelle.lip6.fr/)
> > >> >
> > >> > Just my two cents but:
> > >> >
> > >> > 1. You *can* use a static analysis too to find bugs or other issues.
> > >> > 2. However, you should manually do the commits and proper commit
> > >> >messages to subsystems based on your findings. And I generally think
> > >> >that if one contributes code one should also at least smoke test 
> > >> > changes
> > >> >somehow.
> > >> >
> > >> > I don't know if I'm alone with my opinion. I just think that one should
> > >> > also do the analysis part and not blindly create and submit patches.
> > >>
> > >> All of the patches are compile tested.  And the individual patches are
> > >
> > > Compile-testing is not testing. If you are not able to test a commit,
> > > you should explain why.
> >
> > Dude, Julia has been doing semantic patching for years already and
> > nobody has raised any concerns so far. There's already an expectation
> > that Coccinelle *works* and Julia's sematic patches are sound.
> >
> > Besides, adding 'const' is something that causes virtually no functional
> > changes to the point that build-testing is really all you need. Any
> > problems caused by adding 'const' to a definition will be seen by build
> > errors or warnings.
> >
> > Really, just stop with the pointless discussion and go read a bit about
> > Coccinelle and what semantic patches are giving you. The work done by
> > Julia and her peers are INRIA have measurable benefits.
> >
> > You're really making a thunderstorm in a glass of water.
> 
> Thanks for the defense, but since a lot of these patches torned out to be
> wrong, due to an incorrect parse by Coccinelle, combined with an
> unpleasantly lax compiler, Jarkko does have a point that I should have
> looked at the patches more carefully.  In any case, I have written to the
> maintainers relevant to the patches that turned out to be incorrect.

Exactly. I'm not excepting that every commit would require extensive
analysis but it would be good to quickly at least skim through commits
and see if they make sense (or ask if unsure) :) 

And I'm fine with compile testing if it is mentioned in the commit msg.

> julia

/Jarkko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: fix coding style error reported from checkpatch

[Joe Perches]

On Mon, 2016-09-12 at 21:02 +0300, Omri Arad wrote:
[]
> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c 
> b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
[]
> @@ -1976,9 +1976,9 @@ static int r871x_get_ap_info(struct net_device *dev,
>   if (pdata->length >= 32) {
>   if (copy_from_user(data, pdata->pointer, 32))
>   return -EINVAL;
> -data[32] = 0;
> + data[32] = 0;
>   } else {
> -  return -EINVAL;
> + return -EINVAL;
>   }
>   spin_lock_irqsave(&(pmlmepriv->scanned_queue.lock), irqL);
>   phead = >queue;

Please don't blindly follow checkpatch messages but look to see
how the code can be improved beyond what checkpatch emits.

Perhaps more pleasant to read would be to rewrite this block like:

if (pdata->length < 32 ||
copy_from_user(data, pdata->pointer, 32))
return -EINVAL;
data[32] = 0;

Perhaps as well the literal 32 uses here and the 33 in the declaration
of data uses should be some #define or sizeof or ARRAY_SIZE.


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: comedi: comedi_pcmcia: Fixes multiple blank lines issue

[Ian Abbott]

On 12/09/16 18:29, Namrata A Shettar wrote:

This patch fixes the checkpatch.pl  issue:
-Please don't use multiple blank lines

Signed-off-by: Namrata A Shettar >
---
 drivers/staging/comedi/comedi_pcmcia.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_pcmcia.c
b/drivers/staging/comedi/comedi_pcmcia.c
index d7072a5..ec8a0ad 100644
--- a/drivers/staging/comedi/comedi_pcmcia.c
+++ b/drivers/staging/comedi/comedi_pcmcia.c
@@ -18,7 +18,6 @@

 #include 
 #include 
-
 #include "comedi_pcmcia.h"

 /**


But there is only one blank line, so why the warning about multiple 
blank lines?


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: rtl8712: fix coding style error reported from checkpatch

[Omri Arad]

removed the following:
ERROR: code indent should use tabs where possible
WARNING: please, no spaces at the start of a line
WARNING: Statements should start on a tabstop

Signed-off-by: Omri Arad 
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c 
b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
index e205adf..475e790 100644
--- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
+++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
@@ -1976,9 +1976,9 @@ static int r871x_get_ap_info(struct net_device *dev,
if (pdata->length >= 32) {
if (copy_from_user(data, pdata->pointer, 32))
return -EINVAL;
-data[32] = 0;
+   data[32] = 0;
} else {
-return -EINVAL;
+   return -EINVAL;
}
spin_lock_irqsave(&(pmlmepriv->scanned_queue.lock), irqL);
phead = >queue;
-- 
2.5.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH resend] staging: comedi: comedi_fops: coding style fixes

[Ian Abbott]

On 12/09/16 14:18, Matias Mucciolo wrote:


From: Matias Mucciolo 



Those three lines are unnecessary, especially the initial blank line. 
I'm not sure whether Greg will ask you to fix it up.



- Fixed coding style in comedi_fops.c Symbolic to octal permission.

Signed-off-by: Matias Mucciolo 
---
 drivers/staging/comedi/comedi_fops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 1999eed..bf922ea 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -81,20 +81,20 @@ struct comedi_file {
(COMEDI_NUM_MINORS - COMEDI_NUM_BOARD_MINORS)

 static int comedi_num_legacy_minors;
-module_param(comedi_num_legacy_minors, int, S_IRUGO);
+module_param(comedi_num_legacy_minors, int, 0444);
 MODULE_PARM_DESC(comedi_num_legacy_minors,
 "number of comedi minor devices to reserve for non-auto-configured 
devices (default 0)"
);

 unsigned int comedi_default_buf_size_kb = CONFIG_COMEDI_DEFAULT_BUF_SIZE_KB;
-module_param(comedi_default_buf_size_kb, uint, S_IRUGO | S_IWUSR);
+module_param(comedi_default_buf_size_kb, uint, 0644);
 MODULE_PARM_DESC(comedi_default_buf_size_kb,
 "default asynchronous buffer size in KiB (default "
 __MODULE_STRING(CONFIG_COMEDI_DEFAULT_BUF_SIZE_KB) ")");

 unsigned int comedi_default_buf_maxsize_kb
= CONFIG_COMEDI_DEFAULT_BUF_MAXSIZE_KB;
-module_param(comedi_default_buf_maxsize_kb, uint, S_IRUGO | S_IWUSR);
+module_param(comedi_default_buf_maxsize_kb, uint, 0644);
 MODULE_PARM_DESC(comedi_default_buf_maxsize_kb,
 "default maximum size of asynchronous buffer in KiB (default "
 __MODULE_STRING(CONFIG_COMEDI_DEFAULT_BUF_MAXSIZE_KB) ")");



It looks like the octal permissions have been blessed from above 
(Linus), so this is good.


Reviewed-by: Ian Abbott 

--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4.4 123/192] [PATCH 127/135] x86/hyperv: Avoid reporting bogus NMI status for Gen2 instances

[Greg Kroah-Hartman]

4.4-stable review patch.  If anyone has any objections, please let me know.

--

[ Upstream commit 1e2ae9ec072f3b7887f456426bc2cf23b80f661a ]

Generation2 instances don't support reporting the NMI status on port 0x61,
read from there returns 'ff' and we end up reporting nonsensical PCI
error (as there is no PCI bus in these instances) on all NMIs:

NMI: PCI system error (SERR) for reason ff on CPU 0.
Dazed and confused, but trying to continue

Fix the issue by overriding x86_platform.get_nmi_reason. Use 'booted on
EFI' flag to detect Gen2 instances.

Signed-off-by: Vitaly Kuznetsov 
Cc: Alexander Shishkin 
Cc: Arnaldo Carvalho de Melo 
Cc: Cathy Avery 
Cc: Haiyang Zhang 
Cc: Jiri Olsa 
Cc: K. Y. Srinivasan 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Cc: de...@linuxdriverproject.org
Link: 
http://lkml.kernel.org/r/1460728232-31433-1-git-send-email-vkuzn...@redhat.com
Signed-off-by: Ingo Molnar 
Signed-off-by: Sasha Levin 
Signed-off-by: Greg Kroah-Hartman 
---
 arch/x86/kernel/cpu/mshyperv.c |   12 
 1 file changed, 12 insertions(+)

--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -152,6 +152,11 @@ static struct clocksource hyperv_cs = {
.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
 };
 
+static unsigned char hv_get_nmi_reason(void)
+{
+   return 0;
+}
+
 static void __init ms_hyperv_init_platform(void)
 {
/*
@@ -191,6 +196,13 @@ static void __init ms_hyperv_init_platfo
machine_ops.crash_shutdown = hv_machine_crash_shutdown;
 #endif
mark_tsc_unstable("running on Hyper-V");
+
+   /*
+* Generation 2 instances don't support reading the NMI status from
+* 0x61 port.
+*/
+   if (efi_enabled(EFI_BOOT))
+   x86_platform.get_nmi_reason = hv_get_nmi_reason;
 }
 
 const __refconst struct hypervisor_x86 x86_hyper_ms_hyperv = {


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [Outreachy kernel] [PATCH] Staging: rtl8188eu: Remove ternary operator

[Bhumika Goyal]

On Mon, Sep 12, 2016 at 8:14 PM, Jes Sorensen  wrote:
> On 09/12/16 09:58, Bhumika Goyal wrote:
>> Relational and logical operators evaluate to either true or false.
>> Explicit conversion is not needed so remove the ternary operator.
>> Done using coccinelle:
>>
>> @r@
>> expression A,B;
>> symbol true,false;
>> binary operator b = {==,!=,&&,||,>=,<=,>,<};
>> @@
>> - (A b B) ? true : false
>> + A b B
>>
>> Signed-off-by: Bhumika Goyal 
>> ---
>>  drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 8 
>>  drivers/staging/rtl8188eu/hal/phy.c   | 2 +-
>>  drivers/staging/rtl8188eu/hal/rtl8188e_dm.c   | 3 ++-
>>  3 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c 
>> b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
>> index 9544e55..b43f57be 100644
>> --- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
>> +++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
>> @@ -705,7 +705,7 @@ static int issue_probereq_ex(struct adapter *padapter,
>>   unsigned long start = jiffies;
>>
>>   do {
>> - ret = issue_probereq(padapter, pssid, da, wait_ms > 0 ? true : 
>> false);
>> + ret = issue_probereq(padapter, pssid, da, wait_ms > 0);
>>
>>   i++;
>>
>> @@ -1293,7 +1293,7 @@ int issue_nulldata(struct adapter *padapter, unsigned 
>> char *da, unsigned int pow
>>   da = pnetwork->MacAddress;
>>
>>   do {
>> - ret = _issue_nulldata(padapter, da, power_mode, wait_ms > 0 ? 
>> true : false);
>> + ret = _issue_nulldata(padapter, da, power_mode, wait_ms > 0);
>>
>>   i++;
>>
>> @@ -1420,7 +1420,7 @@ int issue_qos_nulldata(struct adapter *padapter, 
>> unsigned char *da, u16 tid, int
>>   da = pnetwork->MacAddress;
>>
>>   do {
>> - ret = _issue_qos_nulldata(padapter, da, tid, wait_ms > 0 ? 
>> true : false);
>> + ret = _issue_qos_nulldata(padapter, da, tid, wait_ms > 0);
>>
>>   i++;
>>
>> @@ -1527,7 +1527,7 @@ static int issue_deauth_ex(struct adapter *padapter, 
>> u8 *da,
>>   unsigned long start = jiffies;
>>
>>   do {
>> - ret = _issue_deauth(padapter, da, reason, wait_ms > 0 ? true : 
>> false);
>> + ret = _issue_deauth(padapter, da, reason, wait_ms > 0);
>>
>>   i++;
>
> While this part of the patch is technically correct, I would argue it
> doesn't improve the code. It would make the code more readable to pass
> in the wait_ms value and then have the decision made based on that in
> the called function.
>

Ok, I will keep the wait_ms ternary operation as it is and send a v2.
Thanks for the input.

Thanks,
Bhumika

> Cheers,
> Jes
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH resend] staging: comedi: comedi_fops: coding style fixes

[Matias Mucciolo]

Hi Ian

yes its a coding style issue(checkpatch.pl WARN)

and reference:
https://lwn.net/Articles/696229/

(sorry about the noise.)

-- 

Matias Mucciolo

Area de Infraestructura.
Piedras 737 C.A.B.A 
SUTEBA 

On Monday 12 September 2016 16:52:59 Ian Abbott wrote:
> On 12/09/16 14:18, Matias Mucciolo wrote:
> >
> > From: Matias Mucciolo 
> >
> > - Fixed coding style in comedi_fops.c Symbolic to octal permission.
> >
> > Signed-off-by: Matias Mucciolo 
> > ---
> >  drivers/staging/comedi/comedi_fops.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/comedi_fops.c 
> > b/drivers/staging/comedi/comedi_fops.c
> > index 1999eed..bf922ea 100644
> > --- a/drivers/staging/comedi/comedi_fops.c
> > +++ b/drivers/staging/comedi/comedi_fops.c
> > @@ -81,20 +81,20 @@ struct comedi_file {
> > (COMEDI_NUM_MINORS - COMEDI_NUM_BOARD_MINORS)
> >
> >  static int comedi_num_legacy_minors;
> > -module_param(comedi_num_legacy_minors, int, S_IRUGO);
> > +module_param(comedi_num_legacy_minors, int, 0444);
> >  MODULE_PARM_DESC(comedi_num_legacy_minors,
> >  "number of comedi minor devices to reserve for 
> > non-auto-configured devices (default 0)"
> > );
> >
> >  unsigned int comedi_default_buf_size_kb = 
> > CONFIG_COMEDI_DEFAULT_BUF_SIZE_KB;
> > -module_param(comedi_default_buf_size_kb, uint, S_IRUGO | S_IWUSR);
> > +module_param(comedi_default_buf_size_kb, uint, 0644);
> >  MODULE_PARM_DESC(comedi_default_buf_size_kb,
> >  "default asynchronous buffer size in KiB (default "
> >  __MODULE_STRING(CONFIG_COMEDI_DEFAULT_BUF_SIZE_KB) ")");
> >
> >  unsigned int comedi_default_buf_maxsize_kb
> > = CONFIG_COMEDI_DEFAULT_BUF_MAXSIZE_KB;
> > -module_param(comedi_default_buf_maxsize_kb, uint, S_IRUGO | S_IWUSR);
> > +module_param(comedi_default_buf_maxsize_kb, uint, 0644);
> >  MODULE_PARM_DESC(comedi_default_buf_maxsize_kb,
> >  "default maximum size of asynchronous buffer in KiB (default "
> >  __MODULE_STRING(CONFIG_COMEDI_DEFAULT_BUF_MAXSIZE_KB) ")");
> >
> 
> In my review of the earlier patch, I said I was fine with the octal 
> constants, but on reflection, I'm not sure it's worth changing them just 
> for the hell of it.  We'd probably get a patch from someone else later 
> to change it back to symbolic constants.  Is there an actual coding 
> style issue, or is this just a personal preference?
> 
> -- 
> -=( Ian Abbott @ MEV Ltd.E-mail:  )=-
> -=(  Web: http://www.mev.co.uk/  )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] android: binder: Disable preemption while holding the global binder lock

[Todd Kjos]

Thanks for the reviews. We'll come up with a different solution.

On Sat, Sep 10, 2016 at 10:28 AM, Greg Kroah-Hartman
 wrote:
> On Sat, Sep 10, 2016 at 06:37:29PM +0200, Thomas Gleixner wrote:
>> On Sat, 10 Sep 2016, Peter Zijlstra wrote:
>>
>> > On Sat, Sep 10, 2016 at 09:16:59AM -0700, Christoph Hellwig wrote:
>> > > On Thu, Sep 08, 2016 at 09:12:50AM -0700, Todd Kjos wrote:
>> > > > In Android systems, the display pipeline relies on low
>> > > > latency binder transactions and is therefore sensitive to
>> > > > delays caused by contention for the global binder lock.
>> > > > Jank is siginificantly reduced by disabling preemption
>> > > > while the global binder lock is held.
>> > >
>> > > That's now how preempt_disable is supposed to use.  It is for critical
>> >
>> > not, that's supposed to be _not_. Just to be absolutely clear, this is
>> > NOT how you're supposed to use preempt_disable().
>> >
>> > > sections that use per-cpu or similar resources.
>> > >
>> > > >
>> > > > Originally-from: Riley Andrews 
>> > > > Signed-off-by: Todd Kjos 
>> >
>> > > > @@ -389,7 +390,11 @@ static int task_get_unused_fd_flags(struct
>> > > > binder_proc *proc, int flags)
>> > > >   rlim_cur = task_rlimit(proc->tsk, RLIMIT_NOFILE);
>> > > >   unlock_task_sighand(proc->tsk, );
>> > > >
>> > > > - return __alloc_fd(files, 0, rlim_cur, flags);
>> > > > + preempt_enable_no_resched();
>> > > > + ret = __alloc_fd(files, 0, rlim_cur, flags);
>> > > > + preempt_disable();
>> >
>> > And the fact that people want to use preempt_enable_no_resched() shows
>> > that they're absolutely clueless.
>> >
>> > This is so broken its not funny.
>> >
>> > NAK NAK NAK
>>
>> Indeed. Sprinkling random preempt_enabe/disable() pairs all over the place
>> documents clearly that this is tinkering and not proper software
>> engineering.
>
> I have pointed out in the other thread for this patch (the one that had
> a patch that could be applied) that the single lock in the binder code
> is the main problem here, it should be solved instead of this messing
> around with priorities.
>
> So don't worry, I'm not taking this change :)
>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH resend] staging: comedi: comedi_fops: coding style fixes

[Ian Abbott]

On 12/09/16 14:18, Matias Mucciolo wrote:


From: Matias Mucciolo 

- Fixed coding style in comedi_fops.c Symbolic to octal permission.

Signed-off-by: Matias Mucciolo 
---
 drivers/staging/comedi/comedi_fops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 1999eed..bf922ea 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -81,20 +81,20 @@ struct comedi_file {
(COMEDI_NUM_MINORS - COMEDI_NUM_BOARD_MINORS)

 static int comedi_num_legacy_minors;
-module_param(comedi_num_legacy_minors, int, S_IRUGO);
+module_param(comedi_num_legacy_minors, int, 0444);
 MODULE_PARM_DESC(comedi_num_legacy_minors,
 "number of comedi minor devices to reserve for non-auto-configured 
devices (default 0)"
);

 unsigned int comedi_default_buf_size_kb = CONFIG_COMEDI_DEFAULT_BUF_SIZE_KB;
-module_param(comedi_default_buf_size_kb, uint, S_IRUGO | S_IWUSR);
+module_param(comedi_default_buf_size_kb, uint, 0644);
 MODULE_PARM_DESC(comedi_default_buf_size_kb,
 "default asynchronous buffer size in KiB (default "
 __MODULE_STRING(CONFIG_COMEDI_DEFAULT_BUF_SIZE_KB) ")");

 unsigned int comedi_default_buf_maxsize_kb
= CONFIG_COMEDI_DEFAULT_BUF_MAXSIZE_KB;
-module_param(comedi_default_buf_maxsize_kb, uint, S_IRUGO | S_IWUSR);
+module_param(comedi_default_buf_maxsize_kb, uint, 0644);
 MODULE_PARM_DESC(comedi_default_buf_maxsize_kb,
 "default maximum size of asynchronous buffer in KiB (default "
 __MODULE_STRING(CONFIG_COMEDI_DEFAULT_BUF_MAXSIZE_KB) ")");



In my review of the earlier patch, I said I was fine with the octal 
constants, but on reflection, I'm not sure it's worth changing them just 
for the hell of it.  We'd probably get a patch from someone else later 
to change it back to symbolic constants.  Is there an actual coding 
style issue, or is this just a personal preference?


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [Outreachy kernel] [PATCH] Staging: rtl8188eu: Remove ternary operator

[Jes Sorensen]

On 09/12/16 09:58, Bhumika Goyal wrote:
> Relational and logical operators evaluate to either true or false.
> Explicit conversion is not needed so remove the ternary operator.
> Done using coccinelle:
> 
> @r@
> expression A,B;
> symbol true,false;
> binary operator b = {==,!=,&&,||,>=,<=,>,<};
> @@
> - (A b B) ? true : false
> + A b B
> 
> Signed-off-by: Bhumika Goyal 
> ---
>  drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 8 
>  drivers/staging/rtl8188eu/hal/phy.c   | 2 +-
>  drivers/staging/rtl8188eu/hal/rtl8188e_dm.c   | 3 ++-
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c 
> b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
> index 9544e55..b43f57be 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
> @@ -705,7 +705,7 @@ static int issue_probereq_ex(struct adapter *padapter,
>   unsigned long start = jiffies;
>  
>   do {
> - ret = issue_probereq(padapter, pssid, da, wait_ms > 0 ? true : 
> false);
> + ret = issue_probereq(padapter, pssid, da, wait_ms > 0);
>  
>   i++;
>  
> @@ -1293,7 +1293,7 @@ int issue_nulldata(struct adapter *padapter, unsigned 
> char *da, unsigned int pow
>   da = pnetwork->MacAddress;
>  
>   do {
> - ret = _issue_nulldata(padapter, da, power_mode, wait_ms > 0 ? 
> true : false);
> + ret = _issue_nulldata(padapter, da, power_mode, wait_ms > 0);
>  
>   i++;
>  
> @@ -1420,7 +1420,7 @@ int issue_qos_nulldata(struct adapter *padapter, 
> unsigned char *da, u16 tid, int
>   da = pnetwork->MacAddress;
>  
>   do {
> - ret = _issue_qos_nulldata(padapter, da, tid, wait_ms > 0 ? true 
> : false);
> + ret = _issue_qos_nulldata(padapter, da, tid, wait_ms > 0);
>  
>   i++;
>  
> @@ -1527,7 +1527,7 @@ static int issue_deauth_ex(struct adapter *padapter, u8 
> *da,
>   unsigned long start = jiffies;
>  
>   do {
> - ret = _issue_deauth(padapter, da, reason, wait_ms > 0 ? true : 
> false);
> + ret = _issue_deauth(padapter, da, reason, wait_ms > 0);
>  
>   i++;

While this part of the patch is technically correct, I would argue it
doesn't improve the code. It would make the code more readable to pass
in the wait_ms value and then have the decision made based on that in
the called function.

Cheers,
Jes

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 0/2] staging: most: fix issues of core

[Christian Gromm]

This patch set is needed to fix issues of the core module of the
MOST driver.

Andrey Shvetsov (2):
  staging: most: core: show all linked channels
  staging: most: core: constify structure member

 drivers/staging/most/mostcore/core.c | 18 +-
 drivers/staging/most/mostcore/mostcore.h |  2 +-
 2 files changed, 18 insertions(+), 2 deletions(-)

-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2] staging: most: core: show all linked channels

[Christian Gromm]

This patch is needed to have all linked channels being reported by the
show() function of the attribute file add_link. Currently user space can
only read back the latest link that has been established to a certain
channel.

Signed-off-by: Andrey Shvetsov 
Signed-off-by: Christian Gromm 
---
 drivers/staging/most/mostcore/core.c | 18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/most/mostcore/core.c 
b/drivers/staging/most/mostcore/core.c
index db0606ca..bd555ec 100644
--- a/drivers/staging/most/mostcore/core.c
+++ b/drivers/staging/most/mostcore/core.c
@@ -848,7 +848,23 @@ static ssize_t show_add_link(struct most_aim_obj *aim_obj,
 struct most_aim_attribute *attr,
 char *buf)
 {
-   return snprintf(buf, PAGE_SIZE, "%s\n", aim_obj->add_link);
+   struct most_c_obj *c;
+   struct most_inst_obj *i;
+   int offs = 0;
+
+   list_for_each_entry(i, _list, list) {
+   list_for_each_entry(c, >channel_list, list) {
+   if (c->aim0.ptr == aim_obj->driver ||
+   c->aim1.ptr == aim_obj->driver) {
+   offs += snprintf(buf + offs, PAGE_SIZE - offs,
+"%s:%s\n",
+kobject_name(>kobj),
+kobject_name(>kobj));
+   }
+   }
+   }
+
+   return offs;
 }
 
 /**
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2] staging: most: core: constify structure member

[Christian Gromm]

From: Andrey Shvetsov 

This patch adds the const qualifier to the declaration of the member
name_suffix of structure most_channel_capability. It is needed since it
points to string literals.

Signed-off-by: Andrey Shvetsov 
Signed-off-by: Christian Gromm 
---
 drivers/staging/most/mostcore/mostcore.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/most/mostcore/mostcore.h 
b/drivers/staging/most/mostcore/mostcore.h
index 60e018e..e768cb8 100644
--- a/drivers/staging/most/mostcore/mostcore.h
+++ b/drivers/staging/most/mostcore/mostcore.h
@@ -112,7 +112,7 @@ struct most_channel_capability {
u16 buffer_size_packet;
u16 num_buffers_streaming;
u16 buffer_size_streaming;
-   char *name_suffix;
+   const char *name_suffix;
 };
 
 /**
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: vme/devices: vme_user.c: fix: converted decimal permissions to octal

[Ryan Swan]

Ran checkpatch.pl -f vme_user.c
Fixed: ERROR: Use 4 digit octal (0777) not decimal permissions

Signed-off-by: Ryan Swan 
---
 drivers/staging/vme/devices/vme_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vme/devices/vme_user.c 
b/drivers/staging/vme/devices/vme_user.c
index b95883b..5dd430f 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -773,7 +773,7 @@ static void __exit vme_user_exit(void)
 }
 
 MODULE_PARM_DESC(bus, "Enumeration of VMEbus to which the driver is 
connected");
-module_param_array(bus, int, _num, 0);
+module_param_array(bus, int, _num, );
 
 MODULE_DESCRIPTION("VME User Space Access Driver");
 MODULE_AUTHOR("Martyn Welch 

[PATCH] Staging: rtl8188eu: Remove ternary operator

[Bhumika Goyal]

Relational and logical operators evaluate to either true or false.
Explicit conversion is not needed so remove the ternary operator.
Done using coccinelle:

@r@
expression A,B;
symbol true,false;
binary operator b = {==,!=,&&,||,>=,<=,>,<};
@@
- (A b B) ? true : false
+ A b B

Signed-off-by: Bhumika Goyal 
---
 drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 8 
 drivers/staging/rtl8188eu/hal/phy.c   | 2 +-
 drivers/staging/rtl8188eu/hal/rtl8188e_dm.c   | 3 ++-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c 
b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
index 9544e55..b43f57be 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
@@ -705,7 +705,7 @@ static int issue_probereq_ex(struct adapter *padapter,
unsigned long start = jiffies;
 
do {
-   ret = issue_probereq(padapter, pssid, da, wait_ms > 0 ? true : 
false);
+   ret = issue_probereq(padapter, pssid, da, wait_ms > 0);
 
i++;
 
@@ -1293,7 +1293,7 @@ int issue_nulldata(struct adapter *padapter, unsigned 
char *da, unsigned int pow
da = pnetwork->MacAddress;
 
do {
-   ret = _issue_nulldata(padapter, da, power_mode, wait_ms > 0 ? 
true : false);
+   ret = _issue_nulldata(padapter, da, power_mode, wait_ms > 0);
 
i++;
 
@@ -1420,7 +1420,7 @@ int issue_qos_nulldata(struct adapter *padapter, unsigned 
char *da, u16 tid, int
da = pnetwork->MacAddress;
 
do {
-   ret = _issue_qos_nulldata(padapter, da, tid, wait_ms > 0 ? true 
: false);
+   ret = _issue_qos_nulldata(padapter, da, tid, wait_ms > 0);
 
i++;
 
@@ -1527,7 +1527,7 @@ static int issue_deauth_ex(struct adapter *padapter, u8 
*da,
unsigned long start = jiffies;
 
do {
-   ret = _issue_deauth(padapter, da, reason, wait_ms > 0 ? true : 
false);
+   ret = _issue_deauth(padapter, da, reason, wait_ms > 0);
 
i++;
 
diff --git a/drivers/staging/rtl8188eu/hal/phy.c 
b/drivers/staging/rtl8188eu/hal/phy.c
index 776e5b8..41e0cbc 100644
--- a/drivers/staging/rtl8188eu/hal/phy.c
+++ b/drivers/staging/rtl8188eu/hal/phy.c
@@ -1292,7 +1292,7 @@ void rtl88eu_phy_iq_calibrate(struct adapter *adapt, bool 
recovery)
rOFDM0_RxIQExtAnta};
bool is2t;
 
-   is2t = (dm_odm->RFType == ODM_2T2R) ? true : false;
+   is2t = dm_odm->RFType == ODM_2T2R;
 
if (!(dm_odm->SupportAbility & ODM_RF_CALIBRATION))
return;
diff --git a/drivers/staging/rtl8188eu/hal/rtl8188e_dm.c 
b/drivers/staging/rtl8188eu/hal/rtl8188e_dm.c
index 11e7246..962102d 100644
--- a/drivers/staging/rtl8188eu/hal/rtl8188e_dm.c
+++ b/drivers/staging/rtl8188eu/hal/rtl8188e_dm.c
@@ -60,7 +60,8 @@ static void Init_ODM_ComInfo_88E(struct adapter *Adapter)
 
ODM_CmnInfoInit(dm_odm, ODM_CMNINFO_CUT_VER, cut_ver);
 
-   ODM_CmnInfoInit(dm_odm, ODM_CMNINFO_MP_TEST_CHIP, 
hal_data->VersionID.ChipType == NORMAL_CHIP ? true : false);
+   ODM_CmnInfoInit(dm_odm, ODM_CMNINFO_MP_TEST_CHIP,
+   hal_data->VersionID.ChipType == NORMAL_CHIP);
 
ODM_CmnInfoInit(dm_odm, ODM_CMNINFO_PATCH_ID, hal_data->CustomerID);
ODM_CmnInfoInit(dm_odm, ODM_CMNINFO_BWIFI_TEST, 
Adapter->registrypriv.wifi_spec);
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/26] constify local structures

[Geert Uytterhoeven]

On Mon, Sep 12, 2016 at 3:43 PM, Felipe Balbi
 wrote:
> Jarkko Sakkinen  writes:
>> On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
>>> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
>>> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
>>> > > Constify local structures.
>>> > >
>>> > > The semantic patch that makes this change is as follows:
>>> > > (http://coccinelle.lip6.fr/)
>>> >
>>> > Just my two cents but:
>>> >
>>> > 1. You *can* use a static analysis too to find bugs or other issues.
>>> > 2. However, you should manually do the commits and proper commit
>>> >messages to subsystems based on your findings. And I generally think
>>> >that if one contributes code one should also at least smoke test 
>>> > changes
>>> >somehow.
>>> >
>>> > I don't know if I'm alone with my opinion. I just think that one should
>>> > also do the analysis part and not blindly create and submit patches.
>>>
>>> All of the patches are compile tested.  And the individual patches are
>>
>> Compile-testing is not testing. If you are not able to test a commit,
>> you should explain why.
>
> Dude, Julia has been doing semantic patching for years already and
> nobody has raised any concerns so far. There's already an expectation
> that Coccinelle *works* and Julia's sematic patches are sound.

+1

> Besides, adding 'const' is something that causes virtually no functional
> changes to the point that build-testing is really all you need. Any
> problems caused by adding 'const' to a definition will be seen by build
> errors or warnings.

Unfortunately in this particular case they could lead to failures that can only
be detected at runtime, when failing o write to a read-only piece of memory,
due to the casting away of the constness of the pointers later.
Fortunately this was detected during code review (doh...).

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/26] constify local structures

[Julia Lawall]

On Mon, 12 Sep 2016, Felipe Balbi wrote:

>
> Hi,
>
> Jarkko Sakkinen  writes:
> > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> >>
> >>
> >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> >>
> >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> >> > > Constify local structures.
> >> > >
> >> > > The semantic patch that makes this change is as follows:
> >> > > (http://coccinelle.lip6.fr/)
> >> >
> >> > Just my two cents but:
> >> >
> >> > 1. You *can* use a static analysis too to find bugs or other issues.
> >> > 2. However, you should manually do the commits and proper commit
> >> >messages to subsystems based on your findings. And I generally think
> >> >that if one contributes code one should also at least smoke test 
> >> > changes
> >> >somehow.
> >> >
> >> > I don't know if I'm alone with my opinion. I just think that one should
> >> > also do the analysis part and not blindly create and submit patches.
> >>
> >> All of the patches are compile tested.  And the individual patches are
> >
> > Compile-testing is not testing. If you are not able to test a commit,
> > you should explain why.
>
> Dude, Julia has been doing semantic patching for years already and
> nobody has raised any concerns so far. There's already an expectation
> that Coccinelle *works* and Julia's sematic patches are sound.
>
> Besides, adding 'const' is something that causes virtually no functional
> changes to the point that build-testing is really all you need. Any
> problems caused by adding 'const' to a definition will be seen by build
> errors or warnings.
>
> Really, just stop with the pointless discussion and go read a bit about
> Coccinelle and what semantic patches are giving you. The work done by
> Julia and her peers are INRIA have measurable benefits.
>
> You're really making a thunderstorm in a glass of water.

Thanks for the defense, but since a lot of these patches torned out to be
wrong, due to an incorrect parse by Coccinelle, combined with an
unpleasantly lax compiler, Jarkko does have a point that I should have
looked at the patches more carefully.  In any case, I have written to the
maintainers relevant to the patches that turned out to be incorrect.

julia
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/26] constify local structures

[Felipe Balbi]

Hi,

Jarkko Sakkinen  writes:
> On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
>> 
>> 
>> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
>> 
>> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
>> > > Constify local structures.
>> > >
>> > > The semantic patch that makes this change is as follows:
>> > > (http://coccinelle.lip6.fr/)
>> >
>> > Just my two cents but:
>> >
>> > 1. You *can* use a static analysis too to find bugs or other issues.
>> > 2. However, you should manually do the commits and proper commit
>> >messages to subsystems based on your findings. And I generally think
>> >that if one contributes code one should also at least smoke test changes
>> >somehow.
>> >
>> > I don't know if I'm alone with my opinion. I just think that one should
>> > also do the analysis part and not blindly create and submit patches.
>> 
>> All of the patches are compile tested.  And the individual patches are
>
> Compile-testing is not testing. If you are not able to test a commit,
> you should explain why.

Dude, Julia has been doing semantic patching for years already and
nobody has raised any concerns so far. There's already an expectation
that Coccinelle *works* and Julia's sematic patches are sound.

Besides, adding 'const' is something that causes virtually no functional
changes to the point that build-testing is really all you need. Any
problems caused by adding 'const' to a definition will be seen by build
errors or warnings.

Really, just stop with the pointless discussion and go read a bit about
Coccinelle and what semantic patches are giving you. The work done by
Julia and her peers are INRIA have measurable benefits.

You're really making a thunderstorm in a glass of water.

-- 
balbi


signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/26] constify local structures

[Julia Lawall]

On Mon, 12 Sep 2016, Jarkko Sakkinen wrote:

> On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> >
> >
> > On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> >
> > > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> > > > Constify local structures.
> > > >
> > > > The semantic patch that makes this change is as follows:
> > > > (http://coccinelle.lip6.fr/)
> > >
> > > Just my two cents but:
> > >
> > > 1. You *can* use a static analysis too to find bugs or other issues.
> > > 2. However, you should manually do the commits and proper commit
> > >messages to subsystems based on your findings. And I generally think
> > >that if one contributes code one should also at least smoke test 
> > > changes
> > >somehow.
> > >
> > > I don't know if I'm alone with my opinion. I just think that one should
> > > also do the analysis part and not blindly create and submit patches.
> >
> > All of the patches are compile tested.  And the individual patches are
>
> Compile-testing is not testing. If you are not able to test a commit,
> you should explain why.
>
> > submitted to the relevant maintainers.  The individual commit messages
> > give a more detailed explanation of the strategy used to decide that the
> > structure was constifiable.  It seemed redundant to put that in the cover
> > letter, which will not be committed anyway.
>
> I don't mean to be harsh but I do not care about your thought process
> *that much* when I review a commit (sometimes it might make sense to
> explain that but it depends on the context).
>
> I mostly only care why a particular change makes sense for this
> particular subsystem. The report given by a static analysis tool can
> be a starting point for making a commit but it's not sufficient.
> Based on the report you should look subsystems as individuals.

OK, thanks for the feedback.

julia
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH resend] staging: comedi: comedi_fops: coding style fixes

[Matias Mucciolo]

From: Matias Mucciolo 

- Fixed coding style in comedi_fops.c Symbolic to octal permission.

Signed-off-by: Matias Mucciolo 
---
 drivers/staging/comedi/comedi_fops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 1999eed..bf922ea 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -81,20 +81,20 @@ struct comedi_file {
(COMEDI_NUM_MINORS - COMEDI_NUM_BOARD_MINORS)
 
 static int comedi_num_legacy_minors;
-module_param(comedi_num_legacy_minors, int, S_IRUGO);
+module_param(comedi_num_legacy_minors, int, 0444);
 MODULE_PARM_DESC(comedi_num_legacy_minors,
 "number of comedi minor devices to reserve for 
non-auto-configured devices (default 0)"
);
 
 unsigned int comedi_default_buf_size_kb = CONFIG_COMEDI_DEFAULT_BUF_SIZE_KB;
-module_param(comedi_default_buf_size_kb, uint, S_IRUGO | S_IWUSR);
+module_param(comedi_default_buf_size_kb, uint, 0644);
 MODULE_PARM_DESC(comedi_default_buf_size_kb,
 "default asynchronous buffer size in KiB (default "
 __MODULE_STRING(CONFIG_COMEDI_DEFAULT_BUF_SIZE_KB) ")");
 
 unsigned int comedi_default_buf_maxsize_kb
= CONFIG_COMEDI_DEFAULT_BUF_MAXSIZE_KB;
-module_param(comedi_default_buf_maxsize_kb, uint, S_IRUGO | S_IWUSR);
+module_param(comedi_default_buf_maxsize_kb, uint, 0644);
 MODULE_PARM_DESC(comedi_default_buf_maxsize_kb,
 "default maximum size of asynchronous buffer in KiB (default "
 __MODULE_STRING(CONFIG_COMEDI_DEFAULT_BUF_MAXSIZE_KB) ")");
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/26] constify local structures

[Jarkko Sakkinen]

On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> 
> 
> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> 
> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> > > Constify local structures.
> > >
> > > The semantic patch that makes this change is as follows:
> > > (http://coccinelle.lip6.fr/)
> >
> > Just my two cents but:
> >
> > 1. You *can* use a static analysis too to find bugs or other issues.
> > 2. However, you should manually do the commits and proper commit
> >messages to subsystems based on your findings. And I generally think
> >that if one contributes code one should also at least smoke test changes
> >somehow.
> >
> > I don't know if I'm alone with my opinion. I just think that one should
> > also do the analysis part and not blindly create and submit patches.
> 
> All of the patches are compile tested.  And the individual patches are

Compile-testing is not testing. If you are not able to test a commit,
you should explain why.

> submitted to the relevant maintainers.  The individual commit messages
> give a more detailed explanation of the strategy used to decide that the
> structure was constifiable.  It seemed redundant to put that in the cover
> letter, which will not be committed anyway.

I don't mean to be harsh but I do not care about your thought process
*that much* when I review a commit (sometimes it might make sense to
explain that but it depends on the context).

I mostly only care why a particular change makes sense for this
particular subsystem. The report given by a static analysis tool can
be a starting point for making a commit but it's not sufficient.
Based on the report you should look subsystems as individuals.

> julia

/Jarkko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] stagging:iio:ad9834: add devicetree property support

[Lars-Peter Clausen]

Hi,

Thanks for the patch.

On 09/11/2016 12:52 PM, Gwenhael Goavec-Merou wrote:
> +static struct ad9834_platform_data *ad9834_parse_dt(struct spi_device *spi)
> +{
> + struct ad9834_platform_data *pdata;
> + struct device_node *np = spi->dev.of_node;
> +
> + pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
> +
> + if (of_property_read_u32(np, "mclk", >mclk))
> + return ERR_PTR(-ENODEV);

The input clock should be using the standard clock bindings.

> + if (of_property_read_u32(np, "freq0", >freq0))
> + return ERR_PTR(-ENODEV);
> + if (of_property_read_u32(np, "freq1", >freq1))
> + return ERR_PTR(-ENODEV);
> + if (of_property_read_u16(np, "phase0", >phase0))
> + return ERR_PTR(-ENODEV);
> + if (of_property_read_u16(np, "phase1", >phase1))
> + return ERR_PTR(-ENODEV);
> + pdata->en_div2 = of_property_read_bool(np, "en_div2");
> + pdata->en_signbit_msb_out = of_property_read_bool(np,
> + "en_signbit_msb_out");

The other attributes seem to be more runtime configuration data, rather than
hardware description. Maybe just choose a fixed default.

- Lars
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 07/21] staging: unisys: Move vbushelper.h contents to visorbus_private.h

[Greg KH]

On Fri, Sep 02, 2016 at 04:41:31PM -0400, David Kershner wrote:
> From: Bryan Thompson 
> 
> The contents of vbushelper.h are now only used by visorbus, so it no longer
> needs to be a general include file and it can be incorporated in the
> visorbus private header.
> 
> Signed-off-by: Bryan Thompson 
> Signed-off-by: David Kershner 
> Reviewed-by: Tim Sell 
> Reported-by: Greg Kroah-Hartman 
> ---
>  drivers/staging/unisys/visorbus/vbushelper.h   | 46 
> --
>  drivers/staging/unisys/visorbus/visorbus_private.h | 26 +++-
>  2 files changed, 25 insertions(+), 47 deletions(-)
>  delete mode 100644 drivers/staging/unisys/visorbus/vbushelper.h
> 
> diff --git a/drivers/staging/unisys/visorbus/vbushelper.h 
> b/drivers/staging/unisys/visorbus/vbushelper.h
> deleted file mode 100644
> index f1b6aac..000
> --- a/drivers/staging/unisys/visorbus/vbushelper.h
> +++ /dev/null
> @@ -1,46 +0,0 @@
> -/* vbushelper.h
> - *
> - * Copyright (C) 2011 - 2013 UNISYS CORPORATION
> - * All rights reserved.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or (at
> - * your option) any later version.
> - *
> - * This program is distributed in the hope that it will be useful, but
> - * WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> - * NON INFRINGEMENT.  See the GNU General Public License for more
> - * details.
> - */
> -
> -#ifndef __VBUSHELPER_H__
> -#define __VBUSHELPER_H__
> -
> -/* TARGET_HOSTNAME specified as -DTARGET_HOSTNAME=\"thename\" on the
> - * command line
> - */
> -
> -#define TARGET_HOSTNAME "linuxguest"
> -
> -static inline void bus_device_info_init(
> - struct ultra_vbus_deviceinfo *bus_device_info_ptr,
> - const char *dev_type, const char *drv_name,
> - const char *ver, const char *ver_tag)
> -{
> - memset(bus_device_info_ptr, 0, sizeof(struct ultra_vbus_deviceinfo));
> - snprintf(bus_device_info_ptr->devtype,
> -  sizeof(bus_device_info_ptr->devtype),
> -  "%s", (dev_type) ? dev_type : "unknownType");
> - snprintf(bus_device_info_ptr->drvname,
> -  sizeof(bus_device_info_ptr->drvname),
> -  "%s", (drv_name) ? drv_name : "unknownDriver");
> - snprintf(bus_device_info_ptr->infostrs,
> -  sizeof(bus_device_info_ptr->infostrs), "%s\t%s\t%s",
> -  (ver) ? ver : "unknownVer",
> -  (ver_tag) ? ver_tag : "unknownVerTag",
> -  TARGET_HOSTNAME);
> -}
> -
> -#endif
> diff --git a/drivers/staging/unisys/visorbus/visorbus_private.h 
> b/drivers/staging/unisys/visorbus/visorbus_private.h
> index 3f6ad52..0624e23 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_private.h
> +++ b/drivers/staging/unisys/visorbus/visorbus_private.h
> @@ -21,7 +21,31 @@
>  
>  #include "controlvmchannel.h"
>  #include "vbusdeviceinfo.h"
> -#include "vbushelper.h"
> +
> +/* TARGET_HOSTNAME specified as -DTARGET_HOSTNAME=\"thename\" on the
> + * command line
> + */
> +
> +#define TARGET_HOSTNAME "linuxguest"
> +
> +static inline void bus_device_info_init(

A minor nit, "inline" doesn't always do what you think it does, so I
would recommend just dropping it and letting gcc do it all for you, as
it really is going to do it that anyway.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 00/21] Clean up header files and remove unused structures

[Greg KH]

On Fri, Sep 02, 2016 at 04:41:24PM -0400, David Kershner wrote:
> This patch series starts the process of addressing the issues
> raised by Greg-KH on 08/21/2016, more patches will be coming. 
> 
> This patch series removes unused defines and structures in visorbus. 
> 
> It also combines and simplifies the headers files for the s-Par
> drivers.

This email subject doesn't say what part of the kernel it is touching,
please fix that up in future series as it makes things a bit vague,
don't you think?

Remember, what would you want to see if you had to process patches
200-400 at a time?

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: dgnc: dgnc_sysfs *_ATTR() macros convert

[Greg Kroah-Hartman]

On Fri, Sep 09, 2016 at 12:42:33PM -0300, Matias Mucciolo wrote:
> 
> Convert DRIVER_ATTR() macro with DRIVER_ATTR_RO/RW and
> DEVICE_ATTR() macro with DEVICE_ATTR_RO()

Nice, I like seeing this type of change.

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/1] Staging:dgnc:dgnc_neo: fixed 80 character line limit coding style issue

[Greg KH]

On Sat, Sep 03, 2016 at 04:16:29PM +0530, Nadim Almas wrote:
> Fixed coding style issue
> 
> Signed-off-by: Nadim Almas 
> ---
>  drivers/staging/dgnc/dgnc_neo.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
> index 0974986..f07f69c 100644
> --- a/drivers/staging/dgnc/dgnc_neo.c
> +++ b/drivers/staging/dgnc/dgnc_neo.c
> @@ -1463,8 +1463,8 @@ static void neo_copy_data_from_queue_to_uart(struct 
> channel_t *ch)
>  
>   /*
>* If DTR Toggle mode is on, turn on DTR now if not already set,
> -  * and make sure we get an event when the data transfer has 
> completed.
> +  * and make sure we get an event when the data transfer has com-
> +  * -pleted.

No need for hyphenation, just move the word to the next line.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: wlan-ng: improve code aspect in p80211req_mibset_mibget()

[Greg KH]

On Tue, Sep 06, 2016 at 07:30:50PM +0300, Claudiu Beznea wrote:
> This patch improves code aspect in p80211req_mibset_mibget() function
> by removing accolades which followed case statements. To do so,
> some data variable ware declared at the beginning of the
> function and also pstr and key variables were initialized
> only in the code which uses them.
> 
> Signed-off-by: Claudiu Beznea 
> ---
>  drivers/staging/wlan-ng/p80211req.c | 33 ++---
>  1 file changed, 18 insertions(+), 15 deletions(-)

Didn't apply to my tree for some reason, can you refresh and resend?

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] staging: sm750fb: fix block comment style and spelling issues in ddk750_chip.c

[Greg KH]

On Sun, Sep 04, 2016 at 09:04:10PM +0300, Moshe Green wrote:
> Fix the following warning types:
>  - line length
>  - block comment line * prefix
>  - trailing */ on a separate line
> found by the checkpatch.pl tool in multiple block comments.
> 
> Fix a single spelling error in a comment.
> 
> Signed-off-by: Moshe Green 
> ---
>  drivers/staging/sm750fb/ddk750_chip.c | 49 
> +++
>  1 file changed, 27 insertions(+), 22 deletions(-)

As I didn't take the first patch, this one didn't apply :(

Please resend when you send the first one again.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: sm750fb: fix line length coding style issues in ddk750_chip.c

[Greg KH]

On Sun, Sep 04, 2016 at 09:03:27PM +0300, Moshe Green wrote:
> Fix multiple line length warnings found by the checkpatch.pl tool
> in ddk750_chip.c.
> 
> Signed-off-by: Moshe Green 
> ---
>  drivers/staging/sm750fb/ddk750_chip.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/sm750fb/ddk750_chip.c 
> b/drivers/staging/sm750fb/ddk750_chip.c
> index c1356bb..76aaeaa 100644
> --- a/drivers/staging/sm750fb/ddk750_chip.c
> +++ b/drivers/staging/sm750fb/ddk750_chip.c
> @@ -71,7 +71,7 @@ static void setChipClock(unsigned int frequency)
>   pll.clockType = MXCLK_PLL;
>  
>   /*
> - * Call calcPllValue() to fill up the other fields for PLL 
> structure.
> + * Call calcPllValue() to fill the other fields of PLL structure.
>   * Sometime, the chip cannot set up the exact clock required by 
> User.
>   * Return value from calcPllValue() gives the actual possible 
> clock.

You only changed one sentance here, please fix the whole block.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: comedi: comedi_fops: coding style fixes

[Greg Kroah-Hartman]

On Thu, Sep 08, 2016 at 03:27:49PM -0300, Matias Mucciolo wrote:
> 
> - Fixed coding style in comedi_fops.c Symbolic to octal permission.
> 
> Signed-off-by: Matias Mucciolo 
> Reviewed-by: Ian Abbott 
> ---
>  drivers/staging/comedi/comedi_fops.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Does not apply to my tree at all, can you rebase it against the
staging-next branch of staging.git and resend?

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: emxx_udc: Remove unnecessary blank line

[Greg Kroah-Hartman]

On Mon, Sep 12, 2016 at 03:47:12PM +0530, Rehas Sachdeva wrote:
> This patch fixes the checkpatch.pl warning:
> Blank lines aren't necessary before a close brace '}'
> 
> Signed-off-by: Rehas Sachdeva 
> ---
>  drivers/staging/emxx_udc/emxx_udc.c | 1 -
>  1 file changed, 1 deletion(-)

You just sent this patch earlier, why send it again?

confused,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings

[Greg KH]

On Fri, Sep 09, 2016 at 08:50:35PM +0530, Nayeemahmed Badebade wrote:
> Added __acquires / __releases sparse locking annotations
> to lock_res_and_lock and unlock_res_and_lock functions in
> l_lock.c, to fix below sparse warnings:
> 
>  l_lock.c:47:22: warning: context imbalance in 'lock_res_and_lock' - wrong 
> count at exit
>  l_lock.c:62:6: warning: context imbalance in 'unlock_res_and_lock' - 
> unexpected unlock
> 
> Signed-off-by: Nayeemahmed Badebade 
> ---
>  drivers/staging/lustre/lustre/ldlm/l_lock.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/staging/lustre/lustre/ldlm/l_lock.c 
> b/drivers/staging/lustre/lustre/ldlm/l_lock.c
> index ea8840c..c4b9612 100644
> --- a/drivers/staging/lustre/lustre/ldlm/l_lock.c
> +++ b/drivers/staging/lustre/lustre/ldlm/l_lock.c
> @@ -45,6 +45,8 @@
>   * being an atomic operation.
>   */
>  struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock)
> + __acquires(>l_lock)
> + __acquires(lock->l_resource)

Hm, these are tricky, I don't want to take this type of change without
an ack from the lustre developers...

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCHv3 0/3] Devicetree bindings for Ion

[Greg Kroah-Hartman]

On Tue, Aug 30, 2016 at 05:04:26PM -0700, Laura Abbott wrote:
> Hi,
> 
> This is a long overdue resend and slight update from the last version[1] of
> Ion devicetree bindings.
> 
> The goal here is to keep the Ion bindings minimalist. I experimented with
> dropping all but a dummy devicetree node and just matching on the machine
> name in the platform file. This ends up being a nightmare for the DMA (i.e. 
> CMA)
> heap type. That heap requires a device structure to do its allocation and
> setting up a device structure properly isn't pretty. I have other ideas for
> working with that heap if this gets NAKed.
> 
> I've thought about the idea of a devicetree overlay for specifying more
> platform configuration but that a) requires Android actually load the overlay
> at the right time in the framework and b) opens up an entirely new can of
> worms.
> 
> In conclusion, if we assume that Ion platform support is something anyone
> actually wants, this is still the least bad and intrusive idea I've come up
> with. There exists hisilicon Ion code but it came in without being fully 
> acked.
> I've converted it over as an example of how it might look.
> 
> As always, feedback appreciated.

Give a total lack of feeback, I've now applied these patches :)

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Fix checkpatch.pl error in lowmemkiller.c

[Greg Kroah-Hartman]

On Fri, Sep 02, 2016 at 10:28:48PM +0530, Chinmay Nivsarkar wrote:
> This patch fixes checkpatch.pl error WARNING: line over 80 characters #95:
>  FILE: drivers/staging/android/lowmemorykiller.c:95:
> 
> Signed-off-by: Chinmay Nivsarkar 
> ---
>  drivers/staging/android/lowmemorykiller.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/android/lowmemorykiller.c
> b/drivers/staging/android/lowmemorykiller.c
> index 45a1b4e..41d64b8 100644
> --- a/drivers/staging/android/lowmemorykiller.c
> +++ b/drivers/staging/android/lowmemorykiller.c
> @@ -92,8 +92,8 @@ static unsigned long lowmem_scan(struct shrinker *s,
> struct shrink_control *sc)
>  int array_size = ARRAY_SIZE(lowmem_adj);
>  int other_free = global_page_state(NR_FREE_PAGES) - totalreserve_pages;
>  int other_file = global_node_page_state(NR_FILE_PAGES) -
> -global_node_page_state(NR_SHMEM) -
> -total_swapcache_pages();
> +global_node_page_state(NR_SHMEM) -
> +total_swapcache_pages();
> 
>  if (lowmem_adj_size < array_size)
>  array_size = lowmem_adj_size;
> -- 
> 2.9.3

I don't see how this patch solves the checkpatch message you refer to
here, do you?

confused,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: emxx_udc: Remove unnecessary blank line

[Rehas Sachdeva]

This patch fixes the checkpatch.pl warning:
Blank lines aren't necessary before a close brace '}'

Signed-off-by: Rehas Sachdeva 
---
 drivers/staging/emxx_udc/emxx_udc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/emxx_udc/emxx_udc.c 
b/drivers/staging/emxx_udc/emxx_udc.c
index f4d9000..f6233ec 100644
--- a/drivers/staging/emxx_udc/emxx_udc.c
+++ b/drivers/staging/emxx_udc/emxx_udc.c
@@ -131,7 +131,6 @@ static void _nbu2ss_dump_register(struct nbu2ss_udc *udc)
reg_data =  _nbu2ss_readl(
(u32 *)IO_ADDRESS(USB_BASE_ADDRESS + i + 12));
dev_dbg(>dev, " %08x\n", (int)reg_data);
-
}
 
spin_lock(>lock);
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging/android: mark sync_timeline_create() static

[Greg KH]

On Mon, Sep 05, 2016 at 08:40:25AM +0800, Baoyou Xie wrote:
> 
> 
> On 4 September 2016 at 23:41, Sudip Mukherjee 
> wrote:
> 
> On Sat, Sep 03, 2016 at 02:34:13PM +0800, Baoyou Xie wrote:
> > We get 1 warning when building kernel with W=1:
> > drivers/staging/android/sw_sync.c:56:23: warning: no previous prototype
> for 'sync_timeline_create' [-Wmissing-prototypes]
> >
> > In fact, this function is only used in the file in which it is
> > declared and don't need a declaration, but can be made static.
> > so this patch marks this function with 'static'.
> >
> > Signed-off-by: Baoyou Xie 
> > ---
> >  drivers/staging/android/sw_sync.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> which tree are you using?
> 
> 
> I using master tree with 4.8-rc1

That's a quick way to ensure that your patches will not apply, as that
tree does not contain any of the patches submitted since then.

Please always work against linux-next or my staging-next tree for
staging patches.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ks7010: mark symbols static where possible

[Greg KH]

On Thu, Sep 08, 2016 at 07:37:24PM +0800, Baoyou Xie wrote:
> We get 3 warnings when building kernel with W=1:
> drivers/staging/ks7010/ks7010_sdio.c:90:6: warning: no previous prototype for 
> 'ks_wlan_hw_sleep_doze_request' [-Wmissing-prototypes]
> drivers/staging/ks7010/ks7010_sdio.c:121:6: warning: no previous prototype 
> for 'ks_wlan_hw_sleep_wakeup_request' [-Wmissing-prototypes]
> drivers/staging/ks7010/ks7010_sdio.c:174:5: warning: no previous prototype 
> for '_ks_wlan_hw_power_save' [-Wmissing-prototypes]
> 
> In fact, these functions are only used in the file in which they are
> declared and don't need a declaration, but can be made static.
> so this patch marks these functions with 'static'.
> 
> Signed-off-by: Baoyou Xie 
> ---
>  drivers/staging/ks7010/ks7010_sdio.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Does not apply to my tree :(
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Fix checkpatch.pl error in drivers:staging:ks7010:ks_wlan_net.c

[Greg Kroah-Hartman]

On Sun, Sep 11, 2016 at 02:37:07PM +0530, Chinmay Nivsarkar wrote:
> Fixes error foo * bar should be foo *bar in line #843
> 
> Signed-off-by: Chinmay Nivsarkar 
> ---
>  drivers/staging/ks7010/ks_wlan_net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Does not apply to my tree :(
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ks7010: mark symbols static where possible

[Greg KH]

On Sun, Sep 04, 2016 at 02:38:39PM +0800, Baoyou Xie wrote:
> We get 2 warnings when building kernel with W=1:
> drivers/staging/ks7010/ks_hostif.c:72:6: warning: no previous prototype for 
> 'ks_wlan_hw_wakeup_task' [-Wmissing-prototypes]
> drivers/staging/ks7010/ks_hostif.c:1508:6: warning: no previous prototype for 
> 'hostif_infrastructure_set2_request' [-Wmissing-prototypes]
> 
> In fact, these functions are only used in the file in which they are
> declared and don't need a declaration, but can be made static.
> so this patch marks these functions with 'static'.
> 
> Signed-off-by: Baoyou Xie 
> ---
>  drivers/staging/ks7010/ks_hostif.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

This patch is already in my tree :(
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/5] staging: ks7010: Fix coding style warning in ks7010_sdio.c

[Greg KH]

On Sat, Sep 03, 2016 at 10:35:48PM +0530, Sabitha George wrote:
> This patch fixes the coding style warning ' braces {} are not
> necessary for single statement blocks' found by checkpatch.pl
> 
> Signed-off-by: Sabitha George 
> ---
>  drivers/staging/ks7010/ks7010_sdio.c | 41 
> +++-
>  1 file changed, 17 insertions(+), 24 deletions(-)

This patch doesn't apply, so please refresh this series against my
latest tree and resend.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/5] staging: ks7010: Fix warnings on printk() usage

[Greg KH]

On Sat, Sep 03, 2016 at 10:51:49PM +0530, Sabitha George wrote:
> This patch fixes the following warnings on ks7010_sdio.c
> 1. printk() should include KERN_ facility level
> 2. Prefer [subsystem eg: netdev]_err([subsystem]dev, ...
> then dev_err(dev, ... then pr_err(...  to printk(KERN_ERR ...
> 
> Signed-off-by: Sabitha George 
> ---
>  drivers/staging/ks7010/ks7010_sdio.c | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
> b/drivers/staging/ks7010/ks7010_sdio.c
> index 67b01a6..d45bacd 100644
> --- a/drivers/staging/ks7010/ks7010_sdio.c
> +++ b/drivers/staging/ks7010/ks7010_sdio.c
> @@ -499,8 +499,8 @@ static void ks7010_rw_function(struct work_struct *work)
>   DPRINTK(4, "wait after WAKEUP \n");
>  /*   
> queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,>ks_wlan_hw.rw_wq,
>   (priv->last_wakeup + ((30*HZ)/1000) - jiffies));*/
> - printk("wake: %lu %lu\n", priv->last_wakeup + (30 * HZ) / 1000,
> -jiffies);
> + pr_info("wake: %lu %lu\n", priv->last_wakeup + (30 * HZ) / 1000,
> + jiffies);
>   msleep(30);
>   }
>  
> @@ -1010,11 +1010,11 @@ static int ks7010_sdio_probe(struct sdio_func *func,
>   /* private memory allocate */
>   netdev = alloc_etherdev(sizeof(*priv));
>   if (netdev == NULL) {
> - printk(KERN_ERR "ks7010 : Unable to alloc new net device\n");
> + pr_err("ks7010 : Unable to alloc new net device\n");
>   goto error_release_irq;
>   }
>   if (dev_alloc_name(netdev, "wlan%d") < 0) {
> - printk(KERN_ERR "ks7010 :  Couldn't get name!\n");
> + pr_err("ks7010 :  Couldn't get name!\n");
>   goto error_free_netdev;
>   }
>  
> @@ -1054,8 +1054,7 @@ static int ks7010_sdio_probe(struct sdio_func *func,
>   /* Upload firmware */
>   ret = ks7010_upload_firmware(priv, card);   /* firmware load */
>   if (ret) {
> - printk(KERN_ERR
> -"ks7010: firmware load failed !! retern code = %d\n",
> + pr_err("ks7010: firmware load failed !! retern code = %d\n",
>  ret);

This is a driver, so please use dev_err() where ever possible (I think
it can be used in two of the above places, maybe all three.)

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/26] constify local structures

[Julia Lawall]

On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:

> On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> > Constify local structures.
> >
> > The semantic patch that makes this change is as follows:
> > (http://coccinelle.lip6.fr/)
>
> Just my two cents but:
>
> 1. You *can* use a static analysis too to find bugs or other issues.
> 2. However, you should manually do the commits and proper commit
>messages to subsystems based on your findings. And I generally think
>that if one contributes code one should also at least smoke test changes
>somehow.
>
> I don't know if I'm alone with my opinion. I just think that one should
> also do the analysis part and not blindly create and submit patches.

All of the patches are compile tested.  And the individual patches are
submitted to the relevant maintainers.  The individual commit messages
give a more detailed explanation of the strategy used to decide that the
structure was constifiable.  It seemed redundant to put that in the cover
letter, which will not be committed anyway.

julia

>
> Anyway, I'll apply the TPM change at some point. As I said they were
> for better. Thanks.
>
> /Jarkko
>
> > // 
> > // The first rule ignores some cases that posed problems
> > @r disable optional_qualifier@
> > identifier s != {peri_clk_data,threshold_attr,tracer_flags,tracer};
> > identifier i != {s5k5baf_cis_rect,smtcfb_fix};
> > position p;
> > @@
> > static struct s i@p = { ... };
> >
> > @lstruct@
> > identifier r.s;
> > @@
> > struct s { ... };
> >
> > @used depends on lstruct@
> > identifier r.i;
> > @@
> > i
> >
> > @bad1@
> > expression e;
> > identifier r.i;
> > assignment operator a;
> > @@
> >  (<+...i...+>) a e
> >
> > @bad2@
> > identifier r.i;
> > @@
> >  &(<+...i...+>)
> >
> > @bad3@
> > identifier r.i;
> > declarer d;
> > @@
> >  d(...,<+...i...+>,...);
> >
> > @bad4@
> > identifier r.i;
> > type T;
> > T[] e;
> > identifier f;
> > position p;
> > @@
> >
> > f@p(...,
> > (
> >   (<+...i...+>)
> > &
> >   e
> > )
> > ,...)
> >
> > @bad4a@
> > identifier r.i;
> > type T;
> > T *e;
> > identifier f;
> > position p;
> > @@
> >
> > f@p(...,
> > (
> >   (<+...i...+>)
> > &
> >   e
> > )
> > ,...)
> >
> > @ok5@
> > expression *e;
> > identifier r.i;
> > position p;
> > @@
> > e =@p i
> >
> > @bad5@
> > expression *e;
> > identifier r.i;
> > position p != ok5.p;
> > @@
> > e =@p (<+...i...+>)
> >
> > @rr depends on used && !bad1 && !bad2 && !bad3 && !bad4 && !bad4a && !bad5@
> > identifier s,r.i;
> > position r.p;
> > @@
> >
> > static
> > +const
> >  struct s i@p = { ... };
> >
> > @depends on used && !bad1 && !bad2 && !bad3 && !bad4 && !bad4a && !bad5
> >  disable optional_qualifier@
> > identifier rr.s,r.i;
> > @@
> >
> > static
> > +const
> >  struct s i;
> > // 
> >
> > ---
> >
> >  drivers/acpi/acpi_apd.c  |8 +++---
> >  drivers/char/tpm/tpm-interface.c |   10 
> >  drivers/char/tpm/tpm-sysfs.c |2 -
> >  drivers/cpufreq/intel_pstate.c   |8 +++---
> >  drivers/infiniband/hw/i40iw/i40iw_uk.c   |6 ++---
> >  drivers/media/i2c/tvp514x.c  |2 -
> >  drivers/media/pci/ddbridge/ddbridge-core.c   |   18 +++
> >  drivers/media/pci/ngene/ngene-cards.c|   14 ++--
> >  drivers/media/pci/smipcie/smipcie-main.c |8 +++---
> >  drivers/misc/sgi-xp/xpc_uv.c |2 -
> >  drivers/net/arcnet/com20020-pci.c|   10 
> >  drivers/net/can/c_can/c_can_pci.c|4 +--
> >  drivers/net/can/sja1000/plx_pci.c|   20 
> > -
> >  drivers/net/ethernet/mellanox/mlx4/main.c|4 +--
> >  drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c |2 -
> >  drivers/net/ethernet/renesas/sh_eth.c|   14 ++--
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c |2 -
> >  drivers/net/wireless/ath/dfs_pattern_detector.c  |2 -
> >  drivers/net/wireless/intel/iwlegacy/3945.c   |4 +--
> >  drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c  |2 -
> >  drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c  |2 -
> >  drivers/net/wireless/realtek/rtlwifi/rtl8192de/sw.c  |2 -
> >  drivers/net/wireless/realtek/rtlwifi/rtl8192ee/sw.c  |2 -
> >  drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c  |2 -
> >  drivers/net/wireless/realtek/rtlwifi/rtl8723ae/sw.c  |2 -
> >  drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c  |2 -
> >  drivers/net/wireless/realtek/rtlwifi/rtl8821ae/sw.c  |2 -
> >  drivers/platform/chrome/chromeos_laptop.c|   22 
> > +--
> >  drivers/platform/x86/intel_scu_ipc.c |6 ++---
> >  drivers/platform/x86/intel_telemetry_debugfs.c   |2 -
> >  drivers/scsi/esas2r/esas2r_flash.c   |2 -
> >  

[PATCH] [media] pulse8-cec: avoid uninitialized data use

[Arnd Bergmann]

Building with -Wmaybe-uninitialized reveals the use on an uninitialized
variable containing the physical address of the device whenever
firmware before version 2 is used:

drivers/staging/media/pulse8-cec/pulse8-cec.c: In function 'pulse8_connect':
drivers/staging/media/pulse8-cec/pulse8-cec.c:447:2: error: 'pa' may be used 
uninitialized in this function [-Werror=maybe-uninitialized]

This sets the address to CEC_PHYS_ADDR_INVALID in this case, so we don't
try to write back the uninitialized data to the device.

Signed-off-by: Arnd Bergmann 
Fixes: e28a6c8b3fcc ("[media] pulse8-cec: sync configuration with adapter")
---
 drivers/staging/media/pulse8-cec/pulse8-cec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/pulse8-cec/pulse8-cec.c 
b/drivers/staging/media/pulse8-cec/pulse8-cec.c
index 1158ba9f828f..64fffc709416 100644
--- a/drivers/staging/media/pulse8-cec/pulse8-cec.c
+++ b/drivers/staging/media/pulse8-cec/pulse8-cec.c
@@ -342,8 +342,10 @@ static int pulse8_setup(struct pulse8 *pulse8, struct 
serio *serio,
return err;
pulse8->vers = (data[0] << 8) | data[1];
dev_info(pulse8->dev, "Firmware version %04x\n", pulse8->vers);
-   if (pulse8->vers < 2)
+   if (pulse8->vers < 2) {
+   *pa = CEC_PHYS_ADDR_INVALID;
return 0;
+   }
 
cmd[0] = MSGCODE_GET_BUILDDATE;
err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 4);
-- 
2.9.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/6] rtl8723au: remove declaration of unimplemented functions

[Greg Kroah-Hartman]

On Fri, Sep 02, 2016 at 02:57:44PM +0200, Luca Ceresoli wrote:
> These functions have been declared without any implementation since
> the first commit (364e30ebd2dbaccba430c603da03e68746eb932a) and there
> has been no mention of them in following commits.
> 
> Signed-off-by: Luca Ceresoli 
> Cc: Larry Finger 
> Cc: Jes Sorensen 
> Cc: Greg Kroah-Hartman 
> Cc: linux-wirel...@vger.kernel.org
> Cc: de...@driverdev.osuosl.org
> Cc: linux-ker...@vger.kernel.org
> 
> ---
> 
> Changes v1 -> v2:
> - improve the commit message.
> ---
>  drivers/staging/rtl8723au/include/recv_osdep.h | 3 ---
>  1 file changed, 3 deletions(-)

File is no longer in my tree.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 1/6] staging: rtl8723au: remove unimplemented functions declaration

[Greg Kroah-Hartman]

On Tue, Sep 06, 2016 at 06:36:52PM +0200, Luca Ceresoli wrote:
> These functions have been declared without any implementation since
> the first commit (364e30ebd2dbaccba430c603da03e68746eb932a) and there
> has been no mention of them in following commits.
> 
> Signed-off-by: Luca Ceresoli 
> Cc: Larry Finger 
> Cc: Jes Sorensen 
> Cc: Greg Kroah-Hartman 
> Cc: linux-wirel...@vger.kernel.org
> Cc: de...@driverdev.osuosl.org
> Cc: linux-ker...@vger.kernel.org
> 
> ---
> 
> Changes v2 -> v3:
> - add "staging: " prefix to commit message title.
> 
> Changes v1 -> v2:
> - improve the commit message.
> ---
>  drivers/staging/rtl8723au/include/recv_osdep.h | 3 ---

File is no longer in my tree :(
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] rtlbt: Add RTL8822BE Bluetooth device

[Marcel Holtmann]

Hi Larry,

> The RTL8822BE is a new Realtek wifi and BT device. Support for the BT
> part is hereby added.
> 
> As this device is similar to most of the other Realtek BT devices, the
> changes are minimal. The main difference is that the 8822BE needs a
> configuration file for enabling and disabling features. Thus code is
> added to select and load this configuration file. Although not needed
> at the moment, hooks are added for the other devices that might need
> such configuration files.
> 
> One additional change is to the routine that tests that the project
> ID contained in the firmware matches the hardware. As the project IDs
> are not sequential, continuing to use the position in the array as the
> expected value of the ID would require adding extra unused entries in
> the table, and any subsequant rearrangment of the array would break the
> code. To fix these problems, the array elements now contain both the
> hardware ID and the expected value for the project ID.
> 
> Signed-off-by: 陆朱伟 
> Signed-off-by: Larry Finger 
> ---
> drivers/bluetooth/btrtl.c | 104 --
> drivers/bluetooth/btrtl.h |   5 +++
> 2 files changed, 97 insertions(+), 12 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: vme/devices: vme_user.c: fix: converted decimal permissions to octal

[Greg KH]

On Sun, Sep 11, 2016 at 03:40:30PM -0400, Ryan Swan wrote:
> From: ryan 

If your email client is ok (and yours seems to be), you don't need this
line as it would rename the author of the patch.  This name needs to
match the signed-off-by: line exactly.

> 
> Ran checkpatch.pl -f vme_user.c
> Fixed: ERROR: Use 4 digit octal (0777) not decimal permissions
> 
> Signed-off-by: Ryan Swan 
> ---
>  drivers/staging/vme/devices/vme_user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Third time's a charm?

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel