Re: [PATCH v2] staging: comedi: ni_tiocmd: change mistaken use of start_src for start_arg

2016-01-13 Thread Ian Abbott

On 12/01/16 18:01, Spencer Olson wrote:

I looked through commit messages in the repository that also had Fixes:
tags that were long.  The ones I looked at were all split.  I was going
to say everything, but I just noticed that I was looking at some commits
from 2014.  I don't mind resubmitting if current tools require that or
it is better for some other reason...


Yes, you're correct.  This command shows quite a few that are presumably 
split over more than one line:


git log | grep '^ *Fixes: [0-9a-f]* (.*[^)]$' | more

--
-=( 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 v2] staging: comedi: ni_tiocmd: change mistaken use of start_src for start_arg

2016-01-13 Thread Ian Abbott

On 12/01/16 17:33, Spencer E. Olson wrote:

This fixes a bug in function ni_tio_input_inttrig().  The trigger number
should be compared to cmd->start_arg, not cmd->start_src.

Fixes: 6a760394d7eb ("staging: comedi: ni_tiocmd: clarify the
cmd->start_arg validation and use")
Cc:  # 3.17+
Signed-off-by: Spencer E. Olson 
---
  Added description suggested by Ian and Dan.  Added Fixes:, CC: tags as
  suggested by Ian.

  drivers/staging/comedi/drivers/ni_tiocmd.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_tiocmd.c 
b/drivers/staging/comedi/drivers/ni_tiocmd.c
index 437f723..823e479 100644
--- a/drivers/staging/comedi/drivers/ni_tiocmd.c
+++ b/drivers/staging/comedi/drivers/ni_tiocmd.c
@@ -92,7 +92,7 @@ static int ni_tio_input_inttrig(struct comedi_device *dev,
unsigned long flags;
int ret = 0;

-   if (trig_num != cmd->start_src)
+   if (trig_num != cmd->start_arg)
return -EINVAL;

spin_lock_irqsave(&counter->lock, flags);



Thanks!

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


Re: [PATCH v2] staging: comedi: ni_mio_common: use CR_CHAN more consistently

2016-01-13 Thread Ian Abbott

On 12/01/16 18:05, Spencer E. Olson wrote:

Generally, the CR_CHAN macro is/should be used to access the relevant bits
for channel identification in cmd->*_arg when the corresponding
cmd->*_src==TRIG_EXT, including cmd->convert_arg in this case.

This patch does not fix a bug per se, as NISTC_AI_MODE1_CONVERT_SRC() already
masks the value sufficiently, but using CR_CHAN() here makes the code clearer as
it avoids passing some irrelevant bits to NISTC_AI_MODE1_CONVERT_SRC() in the
first place.

Signed-off-by: Spencer E. Olson 
---
  drivers/staging/comedi/drivers/ni_mio_common.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c 
b/drivers/staging/comedi/drivers/ni_mio_common.c
index 6cc304a..fc4d6b0 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -2408,7 +2408,8 @@ static int ni_ai_cmd(struct comedi_device *dev, struct 
comedi_subdevice *s)
ni_stc_writew(dev, mode2, NISTC_AI_MODE2_REG);
break;
case TRIG_EXT:
-   mode1 |= NISTC_AI_MODE1_CONVERT_SRC(1 + cmd->convert_arg);
+   mode1 |= NISTC_AI_MODE1_CONVERT_SRC(1 +
+   CR_CHAN(cmd->convert_arg));
if ((cmd->convert_arg & CR_INVERT) == 0)
mode1 |= NISTC_AI_MODE1_CONVERT_POLARITY;
ni_stc_writew(dev, mode1, NISTC_AI_MODE1_REG);



Thanks!

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


Re: [PATCH v2] staging: comedi: ni_tiocmd: change mistaken use of start_src for start_arg

2016-01-13 Thread Dan Carpenter
It doesn't matter much, but I feel it's better to go long.

(The Fixes tag was my invention so I'm an authority on this).

regards,
dan carpenter

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


[PATCH] drivers: staging: media: davinci_vpfe: dm365_resizer: fixed some spelling mistakes

2016-01-13 Thread Saatvik Arya
fixed spelling mistakes which referred to OUTPUT as OUPUT

Signed-off-by: Saatvik Arya 
---
 drivers/staging/media/davinci_vpfe/dm365_resizer.c | 22 +++---
 drivers/staging/media/davinci_vpfe/dm365_resizer.h |  2 +-
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_resizer.c 
b/drivers/staging/media/davinci_vpfe/dm365_resizer.c
index acb293e..10f51f4 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_resizer.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_resizer.c
@@ -495,7 +495,7 @@ resizer_configure_in_continious_mode(struct 
vpfe_resizer_device *resizer)
int line_len;
int ret;
 
-   if (resizer->resizer_a.output != RESIZER_OUPUT_MEMORY) {
+   if (resizer->resizer_a.output != RESIZER_OUTPUT_MEMORY) {
dev_err(dev, "enable resizer - Resizer-A\n");
return -EINVAL;
}
@@ -507,7 +507,7 @@ resizer_configure_in_continious_mode(struct 
vpfe_resizer_device *resizer)
param->rsz_en[RSZ_B] = DISABLE;
param->oper_mode = RESIZER_MODE_CONTINIOUS;
 
-   if (resizer->resizer_b.output == RESIZER_OUPUT_MEMORY) {
+   if (resizer->resizer_b.output == RESIZER_OUTPUT_MEMORY) {
struct v4l2_mbus_framefmt *outformat2;
 
param->rsz_en[RSZ_B] = ENABLE;
@@ -1048,13 +1048,13 @@ static void resizer_ss_isr(struct vpfe_resizer_device 
*resizer)
if (ipipeif_sink != IPIPEIF_INPUT_MEMORY)
return;
 
-   if (resizer->resizer_a.output == RESIZER_OUPUT_MEMORY) {
+   if (resizer->resizer_a.output == RESIZER_OUTPUT_MEMORY) {
val = vpss_dma_complete_interrupt();
if (val != 0 && val != 2)
return;
}
 
-   if (resizer->resizer_a.output == RESIZER_OUPUT_MEMORY) {
+   if (resizer->resizer_a.output == RESIZER_OUTPUT_MEMORY) {
spin_lock(&video_out->dma_queue_lock);
vpfe_video_process_buffer_complete(video_out);
video_out->state = VPFE_VIDEO_BUFFER_NOT_QUEUED;
@@ -1064,7 +1064,7 @@ static void resizer_ss_isr(struct vpfe_resizer_device 
*resizer)
 
/* If resizer B is enabled */
if (pipe->output_num > 1 && resizer->resizer_b.output ==
-   RESIZER_OUPUT_MEMORY) {
+   RESIZER_OUTPUT_MEMORY) {
spin_lock(&video_out->dma_queue_lock);
vpfe_video_process_buffer_complete(video_out2);
video_out2->state = VPFE_VIDEO_BUFFER_NOT_QUEUED;
@@ -1074,7 +1074,7 @@ static void resizer_ss_isr(struct vpfe_resizer_device 
*resizer)
 
/* start HW if buffers are queued */
if (vpfe_video_is_pipe_ready(pipe) &&
-   resizer->resizer_a.output == RESIZER_OUPUT_MEMORY) {
+   resizer->resizer_a.output == RESIZER_OUTPUT_MEMORY) {
resizer_enable(resizer, 1);
vpfe_ipipe_enable(vpfe_dev, 1);
vpfe_ipipeif_enable(vpfe_dev);
@@ -1242,8 +1242,8 @@ static int resizer_do_hw_setup(struct vpfe_resizer_device 
*resizer)
struct resizer_params *param = &resizer->config;
int ret = 0;
 
-   if (resizer->resizer_a.output == RESIZER_OUPUT_MEMORY ||
-   resizer->resizer_b.output == RESIZER_OUPUT_MEMORY) {
+   if (resizer->resizer_a.output == RESIZER_OUTPUT_MEMORY ||
+   resizer->resizer_b.output == RESIZER_OUTPUT_MEMORY) {
if (ipipeif_sink == IPIPEIF_INPUT_MEMORY &&
ipipeif_source == IPIPEIF_OUTPUT_RESIZER)
ret = resizer_configure_in_single_shot_mode(resizer);
@@ -1268,7 +1268,7 @@ static int resizer_set_stream(struct v4l2_subdev *sd, int 
enable)
if (&resizer->crop_resizer.subdev != sd)
return 0;
 
-   if (resizer->resizer_a.output != RESIZER_OUPUT_MEMORY)
+   if (resizer->resizer_a.output != RESIZER_OUTPUT_MEMORY)
return 0;
 
switch (enable) {
@@ -1722,7 +1722,7 @@ static int resizer_link_setup(struct media_entity *entity,
}
if (resizer->resizer_a.output != RESIZER_OUTPUT_NONE)
return -EBUSY;
-   resizer->resizer_a.output = RESIZER_OUPUT_MEMORY;
+   resizer->resizer_a.output = RESIZER_OUTPUT_MEMORY;
break;
 
default:
@@ -1747,7 +1747,7 @@ static int resizer_link_setup(struct media_entity *entity,
}
if (resizer->resizer_b.output != RESIZER_OUTPUT_NONE)
return -EBUSY;
-   resizer->resizer_b.output = RESIZER_OUPUT_MEMORY;
+   resizer->resizer_b.output = RESIZER_OUTPUT_MEMORY;
break;
 
default:
diff --git a/drivers/staging/media/davinci_vpfe/dm365_resizer.h 
b/drivers/staging/media/davinci_vpfe/dm365_resizer.h
index 93b0f44..00e64b0 100644
--

[PATCH] drivers: staging: octeon-usb: octeon-hcd.c: fixed coding style related warnings

2016-01-13 Thread Saatvik Arya
fixed coding style warnings related to comment blocks

Signed-off-by: Saatvik Arya 
---
 drivers/staging/octeon-usb/octeon-hcd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c 
b/drivers/staging/octeon-usb/octeon-hcd.c
index 6f28717..16d4587 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -2006,7 +2006,8 @@ static void octeon_usb_urb_complete_callback(struct 
cvmx_usb_state *usb,
urb->hcpriv = NULL;
 
/* For Isochronous transactions we need to update the URB packet status
-  list from data in our private copy */
+* list from data in our private copy
+*/
if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) {
int i;
/*
-- 
2.5.0

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


[PATCH] drivers: staging: xgifb: vgatypes.h: fixed coding style warnings

2016-01-13 Thread Saatvik Arya
fixed warnings about comment block coding style

Signed-off-by: Saatvik Arya 
---
 drivers/staging/xgifb/vgatypes.h | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/xgifb/vgatypes.h b/drivers/staging/xgifb/vgatypes.h
index 61fa10f..de80e5c 100644
--- a/drivers/staging/xgifb/vgatypes.h
+++ b/drivers/staging/xgifb/vgatypes.h
@@ -27,14 +27,16 @@ struct xgi_hw_device_info {
/* of Linear VGA memory */
 
unsigned long ulVideoMemorySize; /* size, in bytes, of the
-   memory on the board */
+ * memory on the board
+ */
 
unsigned char jChipType; /* Used to Identify Graphics Chip */
 /* defined in the data structure type  */
 /* "XGI_CHIP_TYPE" */
 
unsigned char jChipRevision; /* Used to Identify Graphics
-   Chip Revision */
+ * Chip Revision
+ */
 
unsigned char ujVBChipID; /* the ID of video bridge */
  /* defined in the data structure type */
@@ -46,4 +48,3 @@ struct xgi_hw_device_info {
 /* Additional IOCTL for communication xgifb <> X driver*/
 /* If changing this, xgifb.h must also be changed (for xgifb) */
 #endif
-
-- 
2.5.0

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


[PATCH] staging: wilc1000: remove extraneous variable

2016-01-13 Thread Arnd Bergmann
Building wilc1000 with clang currently fails in the staging-next branch:

drivers/staging/wilc1000/wilc_spi.c:123:34: warning: tentative definition of 
variable with internal linkage has incomplete non-array type 'const struct 
wilc1000_ops' [-Wtentative-definition-incomplete-type]
static const struct wilc1000_ops wilc1000_spi_ops;

The reason is that wilc1000_ops was left behind after a recent cleanup,
and is completely unused and also uninitialized and const and has an
incomplete type.

Removing the variable is obviously correct, and gets rid of the warning.
No idea why gcc does not complain about it though.

Signed-off-by: Arnd Bergmann 

diff --git a/drivers/staging/wilc1000/wilc_spi.c 
b/drivers/staging/wilc1000/wilc_spi.c
index 86de50c9f7f5..b3d6541b3896 100644
--- a/drivers/staging/wilc1000/wilc_spi.c
+++ b/drivers/staging/wilc1000/wilc_spi.c
@@ -120,8 +120,6 @@ static u8 crc7(u8 crc, const u8 *buffer, u32 len)
 
 #define USE_SPI_DMA 0
 
-static const struct wilc1000_ops wilc1000_spi_ops;
-
 static int wilc_bus_probe(struct spi_device *spi)
 {
int ret, gpio;

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


[RFC 1/9] staging/android/sync: Support sync points created from dma-fences

2016-01-13 Thread John . C . Harrison
From: Maarten Lankhorst 

Debug output assumes all sync points are built on top of Android sync
points and when we start creating them from dma-fences will NULL ptr deref
unless taught about this.

v0.4: Corrected patch ownership.

v0.5: Removed redundant braces to keep style checker happy

Signed-off-by: Maarten Lankhorst 
Signed-off-by: Tvrtko Ursulin 
Cc: Maarten Lankhorst 
Cc: de...@driverdev.osuosl.org
Cc: Riley Andrews 
Cc: Greg Kroah-Hartman 
Cc: Arve Hjønnevåg 
Cc: Gustavo Padovan 
Reviewed-by: Jesse Barnes 
---
 drivers/staging/android/sync_debug.c | 45 ++--
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/android/sync_debug.c 
b/drivers/staging/android/sync_debug.c
index 91ed2c4..02a1649 100644
--- a/drivers/staging/android/sync_debug.c
+++ b/drivers/staging/android/sync_debug.c
@@ -82,36 +82,42 @@ static const char *sync_status_str(int status)
return "error";
 }
 
-static void sync_print_pt(struct seq_file *s, struct sync_pt *pt, bool fence)
+static void sync_print_pt(struct seq_file *s, struct fence *pt, bool fence)
 {
int status = 1;
-   struct sync_timeline *parent = sync_pt_parent(pt);
 
-   if (fence_is_signaled_locked(&pt->base))
-   status = pt->base.status;
+   if (fence_is_signaled_locked(pt))
+   status = pt->status;
 
seq_printf(s, "  %s%spt %s",
-  fence ? parent->name : "",
+  fence && pt->ops->get_timeline_name ?
+  pt->ops->get_timeline_name(pt) : "",
   fence ? "_" : "",
   sync_status_str(status));
 
if (status <= 0) {
struct timespec64 ts64 =
-   ktime_to_timespec64(pt->base.timestamp);
+   ktime_to_timespec64(pt->timestamp);
 
seq_printf(s, "@%lld.%09ld", (s64)ts64.tv_sec, ts64.tv_nsec);
}
 
-   if (parent->ops->timeline_value_str &&
-   parent->ops->pt_value_str) {
+   if ((!fence || pt->ops->timeline_value_str) &&
+   pt->ops->fence_value_str) {
char value[64];
+   bool success;
 
-   parent->ops->pt_value_str(pt, value, sizeof(value));
-   seq_printf(s, ": %s", value);
-   if (fence) {
-   parent->ops->timeline_value_str(parent, value,
-   sizeof(value));
-   seq_printf(s, " / %s", value);
+   pt->ops->fence_value_str(pt, value, sizeof(value));
+   success = strlen(value);
+
+   if (success)
+   seq_printf(s, ": %s", value);
+
+   if (success && fence) {
+   pt->ops->timeline_value_str(pt, value, sizeof(value));
+
+   if (strlen(value))
+   seq_printf(s, " / %s", value);
}
}
 
@@ -138,7 +144,7 @@ static void sync_print_obj(struct seq_file *s, struct 
sync_timeline *obj)
list_for_each(pos, &obj->child_list_head) {
struct sync_pt *pt =
container_of(pos, struct sync_pt, child_list);
-   sync_print_pt(s, pt, false);
+   sync_print_pt(s, &pt->base, false);
}
spin_unlock_irqrestore(&obj->child_list_lock, flags);
 }
@@ -152,13 +158,8 @@ static void sync_print_fence(struct seq_file *s, struct 
sync_fence *fence)
seq_printf(s, "[%p] %s: %s\n", fence, fence->name,
   sync_status_str(atomic_read(&fence->status)));
 
-   for (i = 0; i < fence->num_fences; ++i) {
-   struct sync_pt *pt =
-   container_of(fence->cbs[i].sync_pt,
-struct sync_pt, base);
-
-   sync_print_pt(s, pt, true);
-   }
+   for (i = 0; i < fence->num_fences; ++i)
+   sync_print_pt(s, fence->cbs[i].sync_pt, true);
 
spin_lock_irqsave(&fence->wq.lock, flags);
list_for_each_entry(pos, &fence->wq.task_list, task_list) {
-- 
1.9.1

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


[RFC 2/9] staging/android/sync: add sync_fence_create_dma

2016-01-13 Thread John . C . Harrison
From: Maarten Lankhorst 

This allows users of dma fences to create a android fence.

v0.2: Added kerneldoc. (Tvrtko Ursulin).

v0.4: Updated comments from review feedback by Maarten.

Signed-off-by: Maarten Lankhorst 
Signed-off-by: Tvrtko Ursulin 
Cc: Maarten Lankhorst 
Cc: Daniel Vetter 
Cc: Jesse Barnes 
Cc: de...@driverdev.osuosl.org
Cc: Riley Andrews 
Cc: Greg Kroah-Hartman 
Cc: Arve Hjønnevåg 
Cc: Gustavo Padovan 
Reviewed-by: Jesse Barnes 
Tested-by: Jesse Barnes 
---
 drivers/staging/android/sync.c | 13 +
 drivers/staging/android/sync.h | 10 ++
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
index f83e00c..7f0e919 100644
--- a/drivers/staging/android/sync.c
+++ b/drivers/staging/android/sync.c
@@ -188,7 +188,7 @@ static void fence_check_cb_func(struct fence *f, struct 
fence_cb *cb)
 }
 
 /* TODO: implement a create which takes more that one sync_pt */
-struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
+struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt)
 {
struct sync_fence *fence;
 
@@ -199,16 +199,21 @@ struct sync_fence *sync_fence_create(const char *name, 
struct sync_pt *pt)
fence->num_fences = 1;
atomic_set(&fence->status, 1);
 
-   fence->cbs[0].sync_pt = &pt->base;
+   fence->cbs[0].sync_pt = pt;
fence->cbs[0].fence = fence;
-   if (fence_add_callback(&pt->base, &fence->cbs[0].cb,
-  fence_check_cb_func))
+   if (fence_add_callback(pt, &fence->cbs[0].cb, fence_check_cb_func))
atomic_dec(&fence->status);
 
sync_fence_debug_add(fence);
 
return fence;
 }
+EXPORT_SYMBOL(sync_fence_create_dma);
+
+struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
+{
+   return sync_fence_create_dma(name, &pt->base);
+}
 EXPORT_SYMBOL(sync_fence_create);
 
 struct sync_fence *sync_fence_fdget(int fd)
diff --git a/drivers/staging/android/sync.h b/drivers/staging/android/sync.h
index 61f8a3a..afa0752 100644
--- a/drivers/staging/android/sync.h
+++ b/drivers/staging/android/sync.h
@@ -254,6 +254,16 @@ void sync_pt_free(struct sync_pt *pt);
  */
 struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt);
 
+/**
+ * sync_fence_create_dma() - creates a sync fence from dma-fence
+ * @name:  name of fence to create
+ * @pt:dma-fence to add to the fence
+ *
+ * Creates a fence containg @pt.  Once this is called, the fence takes
+ * ownership of @pt.
+ */
+struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt);
+
 /*
  * API for sync_fence consumers
  */
-- 
1.9.1

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


Re: [Intel-gfx] [RFC 2/9] staging/android/sync: add sync_fence_create_dma

2016-01-13 Thread Gustavo Padovan
Hi John,

2016-01-13 john.c.harri...@intel.com :

> From: Maarten Lankhorst 
> 
> This allows users of dma fences to create a android fence.
> 
> v0.2: Added kerneldoc. (Tvrtko Ursulin).
> 
> v0.4: Updated comments from review feedback by Maarten.
> 
> Signed-off-by: Maarten Lankhorst 
> Signed-off-by: Tvrtko Ursulin 
> Cc: Maarten Lankhorst 
> Cc: Daniel Vetter 
> Cc: Jesse Barnes 
> Cc: de...@driverdev.osuosl.org
> Cc: Riley Andrews 
> Cc: Greg Kroah-Hartman 
> Cc: Arve Hjønnevåg 
> Cc: Gustavo Padovan 
> Reviewed-by: Jesse Barnes 
> Tested-by: Jesse Barnes 
> ---
>  drivers/staging/android/sync.c | 13 +
>  drivers/staging/android/sync.h | 10 ++
>  2 files changed, 19 insertions(+), 4 deletions(-)

Patch 1 and 2 are already upstream in staging-next. You are missing a
rebase probably.

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


RE: [PATCH net-next] hv_netvsc: don't make assumptions on struct flow_keys layout

2016-01-13 Thread Haiyang Zhang


> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Sunday, January 10, 2016 5:26 PM
> To: vkuzn...@redhat.com
> Cc: net...@vger.kernel.org; KY Srinivasan ; Haiyang
> Zhang ; de...@linuxdriverproject.org; linux-
> ker...@vger.kernel.org; eric.duma...@gmail.com
> Subject: Re: [PATCH net-next] hv_netvsc: don't make assumptions on
> struct flow_keys layout
> 
> From: Vitaly Kuznetsov 
> Date: Thu,  7 Jan 2016 10:33:09 +0100
> 
> > Recent changes to 'struct flow_keys' (e.g commit d34af823ff40 ("net:
> Add
> >  VLAN ID to flow_keys")) introduced a performance regression in netvsc
> > driver. Is problem is, however, not the above mentioned commit but the
> > fact that netvsc_set_hash() function did some assumptions on the
> struct
> > flow_keys data layout and this is wrong. We need to extract the data
> we
> > need (src/dst addresses and ports) after the dissect.
> >
> > The issue could also be solved in a completely different way: as
> suggested
> > by Eric instead of our own homegrown netvsc_set_hash() we could use
> > skb_get_hash() which does more or less the same. Unfortunately, the
> > testing done by Simon showed that Hyper-V hosts are not happy with our
> > Jenkins hash, selecting the output queue with the current algorithm
> based
> > on Toeplitz hash works significantly better.
> >
> > Tested-by: Simon Xiao 
> > Signed-off-by: Vitaly Kuznetsov 
> 
> Stop using this Toeplitz thing and just use the proper hash the stack
> is already calculating for you.
> 
> There is no way this is faster, and the continued attempts to
> shoe-horn Toeplitz usage into this driver is resulting in incredibly
> ugly and rediculous code.
> 
> I'm not applying any patches that further the use of Toeplitz as the
> hash function in this driver.  You must use the clean,
> efficient, facilities the kernel has already for packet hashing.
> 
> If every driver did what you guys are doing, we'd be in a heap of
> trouble, and I'm simply not going to allow this to continue any
> longer.
> 
> Thanks.

I have done a comparison of the Toeplitz v.s. Jenkins Hash algorithms, 
and found that the Toeplitz provides much better distribution of the 
connections into send-indirection-table entries. See the data below -- 
showing how many TCP connections are distributed into each of the 
sixteen table entries. The Toeplitz hash distributes the connections 
almost perfectly evenly, but the Jenkins hash distributes them unevenly. 
For example, in case of 64 connections, some entries are 0 or 1, some 
other entries are 8. This could cause too many connections in one VMBus 
channel and slow down the throughput. This is consistent to our test 
which showing slower performance while using the generic skb_get_hash 
(Jenkins) than using Toeplitz hash (see perf numbers below).


#connections:32:
Toeplitz:2,2,2,2,2,1,2,2,2,2,2,3,2,2,2,2,
Jenkins:3,2,2,4,1,1,0,2,1,1,4,3,2,5,1,0,
#connections:64:
Toeplitz:4,4,5,4,4,3,4,4,4,4,4,4,4,4,4,4,
Jenkins:4,5,4,6,3,5,0,6,1,2,8,3,6,8,2,1,
#connections:128:
Toeplitz:8,8,8,8,8,7,9,8,8,8,8,8,8,8,8,8,
Jenkins:8,12,10,9,7,8,3,10,6,8,9,8,10,11,6,3,

Throughput (Gbps) comparison:
#conn   ToeplitzJenkins
32  26.623.2
64  32.123.4
128 29.124.1

For long term solution, I think we should put the Toeplitz hash as 
another option to the generic hash function in kernel... But, for the 
time being, can you accept this patch to fix the assumptions on 
struct flow_keys layout?

Thanks,
- Haiyang

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


[PATCH] Staging: comedi: fix coding style issue in comedilib.h

2016-01-13 Thread Bo YU
This is a patch to the comedilib.h file that fixes "bloch comment use
* on subsequent lines" warning found by the checkpatch.pl tool

Signed-off-by: YU Bo 
---
  drivers/staging/comedi/comedilib.h |   32 
  1 个文件被修改,插入 16 行(+),删除 16 行(-)

diff --git a/drivers/staging/comedi/comedilib.h
b/drivers/staging/comedi/comedilib.h
index 56baf85..5f0ecc5 100644
--- a/drivers/staging/comedi/comedilib.h
+++ b/drivers/staging/comedi/comedilib.h
@@ -1,20 +1,20 @@
  /*
-linux/include/comedilib.h
-header file for kcomedilib
-
-COMEDI - Linux Control and Measurement Device Interface
-Copyright (C) 1998-2001 David A. Schleef 
-
-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.  See the
-GNU General Public License for more details.
-*/
+ *linux/include/comedilib.h
+ *   header file for kcomedilib
+ *
+ *   COMEDI - Linux Control and Measurement Device Interface
+ *Copyright (C) 1998-2001 David A. Schleef 
+ *
+ *   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.  See the
+ *   GNU General Public License for more details.
+ */

  #ifndef _LINUX_COMEDILIB_H
  #define _LINUX_COMEDILIB_H
--
1.7.10.4


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


Re: [PATCH net-next] hv_netvsc: don't make assumptions on struct flow_keys layout

2016-01-13 Thread David Miller
From: Haiyang Zhang 
Date: Wed, 13 Jan 2016 23:10:57 +

> I have done a comparison of the Toeplitz v.s. Jenkins Hash algorithms, 
> and found that the Toeplitz provides much better distribution of the 
> connections into send-indirection-table entries.

This fails to take into consideration how massively more expensive
Toeplitz is to compute.

This also fails to show what the real life performance implications
are.

Just showing distributions is meaningless if it doesn't indicate
what kind of performance distrubtion A or B achieves.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] Staging: comedi: fix more characters coding style issue in comedi_pcmcia.h

2016-01-13 Thread Bo YU
This is a patch to the comedi_pcmcia.h file that fixes up a "line over
80 characters" warning  found by the checkpatch.pl tool

Signed-off-by: YU Bo 
---
  drivers/staging/comedi/comedi_pcmcia.h |3 ++-
  1 个文件被修改,插入 2 行(+),删除 1 行(-)

diff --git a/drivers/staging/comedi/comedi_pcmcia.h
b/drivers/staging/comedi/comedi_pcmcia.h
index 5d3db2b..3d076e7 100644
--- a/drivers/staging/comedi/comedi_pcmcia.h
+++ b/drivers/staging/comedi/comedi_pcmcia.h
@@ -39,7 +39,8 @@ void comedi_pcmcia_driver_unregister(struct comedi_driver *,
  struct pcmcia_driver *);

  /**
- * module_comedi_pcmcia_driver() - Helper macro for registering a
comedi PCMCIA driver
+ * module_comedi_pcmcia_driver() - Helper
+ * macro for registering a comedi PCMCIA driver
   * @__comedi_driver: comedi_driver struct
   * @__pcmcia_driver: pcmcia_driver struct
   *
--
1.7.10.4


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


[PATCH] Staging: comedi: fix more characters coding style issue in comedi_pcmcia.h

2016-01-13 Thread YU Bo

This is a patch to the comedi_pcmcia.h file that fixes up a "line over
80 characters" warning  found by the checkpatch.pl tool

Signed-off-by: YU Bo 
---
 drivers/staging/comedi/comedi_pcmcia.h |3 ++-
 1 个文件被修改,插入 2 行(+),删除 1 行(-)

diff --git a/drivers/staging/comedi/comedi_pcmcia.h 
b/drivers/staging/comedi/comedi_pcmcia.h
index 5d3db2b..3d076e7 100644
--- a/drivers/staging/comedi/comedi_pcmcia.h
+++ b/drivers/staging/comedi/comedi_pcmcia.h
@@ -39,7 +39,8 @@ void comedi_pcmcia_driver_unregister(struct comedi_driver *,
 struct pcmcia_driver *);

 /**
- * module_comedi_pcmcia_driver() - Helper macro for registering a comedi 
PCMCIA driver
+ * module_comedi_pcmcia_driver() - Helper
+ * macro for registering a comedi PCMCIA driver
  * @__comedi_driver: comedi_driver struct
  * @__pcmcia_driver: pcmcia_driver struct
  *
--
1.7.10.4


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


[PATCH] Staging: comedi.h: Fix coding style issue in comedi.h

2016-01-13 Thread YU Bo

This is a patch to the comedi.h file that fixes up warnings found by the
checkpatch.pl tool

Signed-off-by: YU Bo 
---
 drivers/staging/comedi/comedi.h |  260 +++
 1 个文件被修改,插入 154 行(+),删除 106 行(-)

diff --git a/drivers/staging/comedi/comedi.h b/drivers/staging/comedi/comedi.h
index 66edda1..d8e78af 100644
--- a/drivers/staging/comedi/comedi.h
+++ b/drivers/staging/comedi/comedi.h
@@ -1,20 +1,20 @@
 /*
-include/comedi.h (installed as /usr/include/comedi.h)
-header file for comedi
-
-COMEDI - Linux Control and Measurement Device Interface
-Copyright (C) 1998-2001 David A. Schleef 
-
-This program is free software; you can redistribute it and/or modify
-it under the terms of the GNU Lesser 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.  See the
-GNU General Public License for more details.
-*/
+ *   include/comedi.h (installed as /usr/include/comedi.h)
+ *   header file for comedi
+ *
+ *   COMEDI - Linux Control and Measurement Device Interface
+ *   Copyright (C) 1998-2001 David A. Schleef 
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU Lesser 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.  See the
+ *   GNU General Public License for more details.
+ */

 #ifndef _COMEDI_H
 #define _COMEDI_H
@@ -28,9 +28,9 @@
 #define COMEDI_MAJOR 98

 /*
-   maximum number of minor devices.  This can be increased, although
-   kernel structures are currently statically allocated, thus you
-   don't want this to be much more than you actually use.
+ *  maximum number of minor devices.  This can be increased, although
+ *  kernel structures are currently statically allocated, thus you
+ *  don't want this to be much more than you actually use.
  */
 #define COMEDI_NDEVICES 16

@@ -279,7 +279,8 @@ enum configuration_ids {
INSN_CONFIG_SET_OTHER_SRC = 2005, /* Set other source */
/* INSN_CONFIG_GET_OTHER_SRC = 2006,*//* Get other source */
/* Get size in bytes of subdevice's on-board fifos used during
-* streaming input/output */
+* streaming input/output
+*/
INSN_CONFIG_GET_HARDWARE_BUFFER_SIZE = 2006,
INSN_CONFIG_SET_COUNTER_MODE = 4097,
/* INSN_CONFIG_8254_SET_MODE is deprecated */
@@ -291,8 +292,10 @@ enum configuration_ids {
INSN_CONFIG_PWM_SET_PERIOD = 5000,  /* sets frequency */
INSN_CONFIG_PWM_GET_PERIOD = 5001,  /* gets frequency */
INSN_CONFIG_GET_PWM_STATUS = 5002,  /* is it running? */
-   /* sets H bridge: duty cycle and sign bit for a relay at the
-* same time */
+   /*
+* sets H bridge: duty cycle and sign bit for a relay at the
+* same time
+*/
INSN_CONFIG_PWM_SET_H_BRIDGE = 5003,
/* gets H bridge data: duty cycle and the sign bit */
INSN_CONFIG_PWM_GET_H_BRIDGE = 5004
@@ -521,23 +524,22 @@ struct comedi_bufinfo {
 /**/

 /*
-  8254 specific configuration.
-
-  It supports two config commands:
-
-  0 ID: INSN_CONFIG_SET_COUNTER_MODE
-  1 8254 Mode
-I8254_MODE0, I8254_MODE1, ..., I8254_MODE5
-OR'ed with:
-I8254_BCD, I8254_BINARY
-
-  0 ID: INSN_CONFIG_8254_READ_STATUS
-  1 <-- Status byte returned here.
-B7 = Output
-B6 = NULL Count
-B5 - B0 Current mode.
-
-*/
+ * 8254 specific configuration.
+ *
+ * It supports two config commands:
+ *
+ * 0 ID: INSN_CONFIG_SET_COUNTER_MODE
+ * 1 8254 Mode
+ *   I8254_MODE0, I8254_MODE1, ..., I8254_MODE5
+ *   OR'ed with:
+ *   I8254_BCD, I8254_BINARY
+ *
+ * 0 ID: INSN_CONFIG_8254_READ_STATUS
+ * 1 <-- Status byte returned here.
+ *   B7 = Output
+ *   B6 = NULL Count
+ *   B5 - B0 Current mode.
+ */

 enum i8254_mode {
I8254_MODE0 = (0 << 1),   /* Interrupt on terminal count */
@@ -545,18 +547,24 @@ enum i8254_mode {
I8254_MODE2 = (2 << 1),   /* Rate generator */
I8254_MODE3 = (3 << 1),   /* Square wave mode */
I8254_MODE4 = (4 << 1),   /* Software triggered strobe */
-   I8254_MODE5 = (5 << 1),   /* Hardware triggered strobe
-* (retriggerable) */
-   I8254_BCD = 1,  /* use binary-coded decimal instead of binary
-* (pretty useless) */
+   I8254_MODE5 = (5 << 1),   /*
+