[PATCH] media input infrastructure:tw686x: Fix of possible inconsistent memory deallocation and/or race condition by implementation of custom video_device_release function in tw686x driver

[Mark Balantzyan]

Possible inconsistent memory deallocation and/or race conditions were 
detected specifically with respect to remaining open handles to the video 
device handled by the tw686x driver. This patch addresses this by 
implementing a revised independent instance of the video_device_release 
function to free the remaining resources and memory where the last open 
handle(s) is/were closed.


Signed-off-by: Mark Balantzyan 
---
 drivers/media/pci/tw686x/tw686x-video.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
b/drivers/media/pci/tw686x/tw686x-video.c

index 3a06c000..741e15ac 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -1151,6 +1151,13 @@ void tw686x_video_irq(struct tw686x_dev *dev, 
unsigned long requests,

}
 }

+void video_device_release(tw686x_dev *dev) {
+   for (int pb = 0; pb < 2; pb++) {
+   dev->dma_ops->free(dev->video_channels,pb);
+   }
+   kfree(dev);
+}
+
 void tw686x_video_free(struct tw686x_dev *dev)
 {
unsigned int ch, pb;
@@ -1160,9 +1167,9 @@ void tw686x_video_free(struct tw686x_dev *dev)

video_unregister_device(vc->device);

-   if (dev->dma_ops->free)
-   for (pb = 0; pb < 2; pb++)
-   dev->dma_ops->free(vc, pb);
+   if (dev->dma_ops->free) {
+   video_device_release(dev);
+   }
}
 }

--
2.17.1

tw686x driver (again, sorry; I respect it as a good driver!)

[Mark Balantzyan]

Hi Hans,

I recall us agreeing that a custom function to free the last resources 
attached to the video device would be preferable. So may I clarify, in 
your words, what bug I may be fixing? I please need a description to 
report to patchwork and to my mentor..


Also I’m getting confused reactions about my patches from Ezequiel and 
that may owe to my lack of direction on the fixing.


Thank you very much,
Mark

Re: [PATCH] media input infrastructure:tw686x: Added custom function for video device release functionality in tw686x driver

[Ezequiel Garcia]

Hi Mark,

On Fri, 26 Jul 2019 at 17:41, Mark Balantzyan  wrote:
>
> Signed-off-by: Mark Balantzyan 
> Reported-by: kbuild test robot 
> ---
> This patch adds a custom function for releasing the video device for the 
> tw686x video device driver.
>
>  drivers/media/pci/tw686x/tw686x-video.c | 23 ++-
>  1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
> b/drivers/media/pci/tw686x/tw686x-video.c
> index 3a06c000..cabc4f89 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -1151,18 +1151,32 @@ void tw686x_video_irq(struct tw686x_dev *dev, 
> unsigned long requests,
> }
>  }
>
> +void tw686x_video_device_release(struct tw686x_video_channel *vc) {
> +

Who's calling this ^ ?

Regards,
Eze

Re: tw686x driver (continued)

[Ezequiel Garcia]

Hi Mark,

On Wed, 24 Jul 2019 at 09:25, Mark Balançian  wrote:
>
> Hi Ezequiel,
>
> (sorry didn't include linux-media in first email)
> I'm not sure yet if I have my supervisor's permission to declare our
> tool as open source, but I'll tell you the possible code paths that I
> think may be leading our tool to think what it's thinking.
>
> First off, it detects a write access to desc->virt without locks inside
> tw686x_memcpy_data_free, after it is called in the calling chain
> tw686x_probe -> allocate an interrupt line -> tw686x_video_init ->
> tw686x_set_format -> tw686x_memcpy_dma_free. Further,
> spin_lock_init(&dev->lock) (line 319 of tw686x-core.c) isn't
> correspondingly closed in the function. Is this intended?
>

Yes, it is intended.

> Second, there is a possibility according to how I have traced a call
> chain that tw686x_init is going to the error: label since
> tw686x_memcpy_dma_free is getting called inside another possible calling
> chain, going tw686x_init -> tw686x_video_free (error: label) ->
> dma_ops->free (i.e. tw686x_memcpy_dma_free). I would assume this would
> not be intended either.
>

I'm not sure I understand what you think it's not intended, sorry.

Re: tw686x driver (continued)

[Ezequiel Garcia]

Hi Hans,

On Wed, 24 Jul 2019 at 10:08, Hans Verkuil  wrote:
>
> On 7/24/19 2:25 PM, Mark Balançian wrote:
> > Hi Ezequiel,
> >
> > (sorry didn't include linux-media in first email)
> > I'm not sure yet if I have my supervisor's permission to declare our
> > tool as open source, but I'll tell you the possible code paths that I
> > think may be leading our tool to think what it's thinking.
> >
> > First off, it detects a write access to desc->virt without locks inside
> > tw686x_memcpy_data_free, after it is called in the calling chain
> > tw686x_probe -> allocate an interrupt line -> tw686x_video_init ->
> > tw686x_set_format -> tw686x_memcpy_dma_free.
>
> Until the video device has been registered there is no need for locking
> since there is no possibility of concurrent access.
>
>  Further,
> > spin_lock_init(&dev->lock) (line 319 of tw686x-core.c) isn't
> > correspondingly closed in the function. Is this intended?
>
> Huh? spin_lock_init doesn't take a lock. It just initializes internal
> data.
>
> >
> > Second, there is a possibility according to how I have traced a call
> > chain that tw686x_init is going to the error: label since
>
> tw686x_init? I assume you mean tw686x_video_init?
>
> > tw686x_memcpy_dma_free is getting called inside another possible calling
> > chain, going tw686x_init -> tw686x_video_free (error: label) ->
> > dma_ops->free (i.e. tw686x_memcpy_dma_free). I would assume this would
> > not be intended either.
>
> What tw686x_video_free() does really should be done in the release function
> of the video_device: vdev->release is currently set to video_device_release,
> but that should be a custom function that calls dev->dma_ops->free.
>

IIUC, v4l2_device_release is called when the last reference to the device
goes away. That, in turn, will call v4l2_dev->release(), which
is set to tw686x_dev_release.

Therefore, tw686x_dev_release is called when the last open handle
was closed, and it's where we free the last resources.

If there's any race with the consistent memory allocation/free,
it seems tw686x_dev_release would be an appropriate place
to free them.

Thanks,
Ezequiel

[PATCH] media input infrastructure:tw686x: Added custom function for video device release functionality in tw686x driver

[Mark Balantzyan]

Signed-off-by: Mark Balantzyan 
Reported-by: kbuild test robot 
---
This patch adds a custom function for releasing the video device for the tw686x 
video device driver.

 drivers/media/pci/tw686x/tw686x-video.c | 23 ++-
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
b/drivers/media/pci/tw686x/tw686x-video.c
index 3a06c000..cabc4f89 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -1151,18 +1151,32 @@ void tw686x_video_irq(struct tw686x_dev *dev, unsigned 
long requests,
}
 }
 
+void tw686x_video_device_release(struct tw686x_video_channel *vc) {
+
+   struct tw686x_dev *dev = vc->dev;
+
+   unsigned int ch;
+
+   for (ch = 0; ch < max_channels(dev); ch++) {
+   struct tw686x_video_channel *vc = &dev->video_channels[ch];
+
+   }
+   
+   dev->dma_ops->free(vc,0);
+   
+   video_device_release((struct video_device*)dev);
+
+}
+
 void tw686x_video_free(struct tw686x_dev *dev)
 {
-   unsigned int ch, pb;
+   unsigned int ch;
 
for (ch = 0; ch < max_channels(dev); ch++) {
struct tw686x_video_channel *vc = &dev->video_channels[ch];
 
video_unregister_device(vc->device);
 
-   if (dev->dma_ops->free)
-   for (pb = 0; pb < 2; pb++)
-   dev->dma_ops->free(vc, pb);
}
 }
 
@@ -1277,7 +1291,6 @@ int tw686x_video_init(struct tw686x_dev *dev)
snprintf(vdev->name, sizeof(vdev->name), "%s video", dev->name);
vdev->fops = &tw686x_video_fops;
vdev->ioctl_ops = &tw686x_video_ioctl_ops;
-   vdev->release = video_device_release;
vdev->v4l2_dev = &dev->v4l2_dev;
vdev->queue = &vc->vidq;
vdev->tvnorms = V4L2_STD_525_60 | V4L2_STD_625_50;
-- 
2.17.1

[PATCH] Added custom function for video device release functionality in tw686x driver

[Mark Balantzyan]

Signed-off-by: Mark Balantzyan 
Reported-by: kbuild test robot 
---
This patch adds a custom function in tw686x-video.c to provide a release 
mechanism for the driver for the tw686x video device.

 drivers/media/pci/tw686x/tw686x-video.c | 23 ++-
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
b/drivers/media/pci/tw686x/tw686x-video.c
index 3a06c000..cabc4f89 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -1151,18 +1151,32 @@ void tw686x_video_irq(struct tw686x_dev *dev, unsigned 
long requests,
}
 }
 
+void tw686x_video_device_release(struct tw686x_video_channel *vc) {
+
+   struct tw686x_dev *dev = vc->dev;
+
+   unsigned int ch;
+
+   for (ch = 0; ch < max_channels(dev); ch++) {
+   struct tw686x_video_channel *vc = &dev->video_channels[ch];
+
+   }
+   
+   dev->dma_ops->free(vc,0);
+   
+   video_device_release((struct video_device*)dev);
+
+}
+
 void tw686x_video_free(struct tw686x_dev *dev)
 {
-   unsigned int ch, pb;
+   unsigned int ch;
 
for (ch = 0; ch < max_channels(dev); ch++) {
struct tw686x_video_channel *vc = &dev->video_channels[ch];
 
video_unregister_device(vc->device);
 
-   if (dev->dma_ops->free)
-   for (pb = 0; pb < 2; pb++)
-   dev->dma_ops->free(vc, pb);
}
 }
 
@@ -1277,7 +1291,6 @@ int tw686x_video_init(struct tw686x_dev *dev)
snprintf(vdev->name, sizeof(vdev->name), "%s video", dev->name);
vdev->fops = &tw686x_video_fops;
vdev->ioctl_ops = &tw686x_video_ioctl_ops;
-   vdev->release = video_device_release;
vdev->v4l2_dev = &dev->v4l2_dev;
vdev->queue = &vc->vidq;
vdev->tvnorms = V4L2_STD_525_60 | V4L2_STD_625_50;
-- 
2.17.1

Re: [PATCH] media input infrastructure:tw686x: Added Added custom function to set vdev->release in tw686x driver

[kbuild test robot]

Hi Mark,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.3-rc1 next-20190726]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Mark-Balantzyan/media-input-infrastructure-tw686x-Added-Added-custom-function-to-set-vdev-release-in-tw686x-driver/20190727-005525
base:   git://linuxtv.org/media_tree.git master
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 7.4.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot 

All error/warnings (new ones prefixed by >>):

   drivers/media/pci/tw686x/tw686x-video.c: In function 
'tw686x_video_device_release':
   drivers/media/pci/tw686x/tw686x-video.c:1156:14: warning: statement with no 
effect [-Wunused-value]
 dev->dma_ops->free;
 ^~
   drivers/media/pci/tw686x/tw686x-video.c:1154:32: warning: unused variable 
'vc' [-Wunused-variable]
  struct tw686x_video_channel *vc = &dev->video_channels[ch];
   ^~
   drivers/media/pci/tw686x/tw686x-video.c: In function 'tw686x_video_free':
   drivers/media/pci/tw686x/tw686x-video.c:1164:19: warning: unused variable 
'pb' [-Wunused-variable]
 unsigned int ch, pb;
  ^~
   drivers/media/pci/tw686x/tw686x-video.c: In function 
'tw686x_video_device_release':
>> drivers/media/pci/tw686x/tw686x-video.c:1162:1: warning: ISO C90 forbids 
>> mixed declarations and code [-Wdeclaration-after-statement]
void tw686x_video_free(struct tw686x_dev *dev)
^~~~
   drivers/media/pci/tw686x/tw686x-video.c: In function 'tw686x_video_init':
>> drivers/media/pci/tw686x/tw686x-video.c:1285:17: error: assignment from 
>> incompatible pointer type [-Werror=incompatible-pointer-types]
  vdev->release = tw686x_video_device_release;
^
   drivers/media/pci/tw686x/tw686x-video.c: In function 
'tw686x_video_device_release':
>> drivers/media/pci/tw686x/tw686x-video.c:1321:1: error: expected declaration 
>> or statement at end of input
}
^
   drivers/media/pci/tw686x/tw686x-video.c:1151:19: warning: unused variable 
'pb' [-Wunused-variable]
 unsigned int ch, pb;
  ^~
   At top level:
   drivers/media/pci/tw686x/tw686x-video.c:1174:5: warning: 'tw686x_video_init' 
defined but not used [-Wunused-function]
int tw686x_video_init(struct tw686x_dev *dev)
^
   cc1: some warnings being treated as errors

vim +1285 drivers/media/pci/tw686x/tw686x-video.c

  1146  
  1147  
  1148  
  1149  void tw686x_video_device_release(struct tw686x_video_channel *vc) {
  1150  struct tw686x_dev *dev = vc->dev;
  1151  unsigned int ch, pb;
  1152  
  1153  for (ch = 0; ch < max_channels(dev); ch++) {
> 1154  struct tw686x_video_channel *vc = 
> &dev->video_channels[ch];
  1155  
  1156  dev->dma_ops->free;
  1157  
  1158  video_device_release((struct video_device*)dev);
  1159  
  1160  }
  1161  
> 1162  void tw686x_video_free(struct tw686x_dev *dev)
  1163  {
> 1164  unsigned int ch, pb;
  1165  
  1166  for (ch = 0; ch < max_channels(dev); ch++) {
  1167  struct tw686x_video_channel *vc = 
&dev->video_channels[ch];
  1168  
  1169  video_unregister_device(vc->device);
  1170  
  1171  }
  1172  }
  1173  
  1174  int tw686x_video_init(struct tw686x_dev *dev)
  1175  {
  1176  unsigned int ch, val;
  1177  int err;
  1178  
  1179  if (dev->dma_mode == TW686X_DMA_MODE_MEMCPY)
  1180  dev->dma_ops = &memcpy_dma_ops;
  1181  else if (dev->dma_mode == TW686X_DMA_MODE_CONTIG)
  1182  dev->dma_ops = &contig_dma_ops;
  1183  else if (dev->dma_mode == TW686X_DMA_MODE_SG)
  1184  dev->dma_ops = &sg_dma_ops;
  1185  else
  1186  return -EINVAL;
  1187  
  1188  err = v4l2_device_register(&dev->pci_dev->dev, &dev->v4l2_dev);
  1189  if (err)
  1190  return err;
  1191  
  1192  if (dev->dma_ops->setup) {
  1193  err = dev->dma_ops->setup(dev);
  1194  if (err)
  1195  return err;
  1196  }
  1197  
  1198  /* Initialize vc->dev and vc->ch fo

Re: [PATCH] media input infrastructure:tw686x: Added Added custom function to set vdev->release in tw686x driver

[Mark Balançian]

Hi Ezequiel,

I just test built the kernel with my patch included and things run fine. 
To guide me in my assignment, could you please clarify what Hans Verkuil 
had been mentioning would be a good thing to code, he said in one of our 
previous emails:


What tw686x_video_free() does really should be done in the release function
of the video_device: vdev->release is currently set to video_device_release,
but that should be a custom function that calls dev->dma_ops->free.

But in order to provide a thorough explanation that makes sense, as you 
mentioned and that I'd like to do, does the custom function and edits I 
made provide the functionality Hans mentioned, as per how it is seen in 
the code below?:


(Please note I didn't provide a description this time as the purpose of 
this email is to get a better idea of how to make one)


From 6d38673ca7d2206b21deb7d28971f52ee8453346 Mon Sep 17 00:00:00 2001
From: Mark Balantzyan 
Date: Wed, 24 Jul 2019 14:01:30 -0700
Subject: [PATCH] media input infrastructure:tw686x: Added custom 
function to provide dev->dma_ops->free for vdev->release in 
tw686x_video_init() in tw686x driver


---
 drivers/media/pci/tw686x/tw686x-video.c | 24 +++-
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
b/drivers/media/pci/tw686x/tw686x-video.c

index 3a06c000..1b875a7c 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -1151,18 +1151,32 @@ void tw686x_video_irq(struct tw686x_dev *dev, 
unsigned long requests,

 }
 }

+void tw686x_video_device_release(struct tw686x_video_channel *vc) {
+
+    struct tw686x_dev *dev = vc->dev;
+
+    unsigned int ch;
+
+    for (ch = 0; ch < max_channels(dev); ch++) {
+        struct tw686x_video_channel *vc = &dev->video_channels[ch];
+
+    }
+
+    dev->dma_ops->free;
+
+    video_device_release((struct video_device*)dev);
+
+}
+
 void tw686x_video_free(struct tw686x_dev *dev)
 {
-    unsigned int ch, pb;
+    unsigned int ch;

 for (ch = 0; ch < max_channels(dev); ch++) {
     struct tw686x_video_channel *vc = &dev->video_channels[ch];

     video_unregister_device(vc->device);

-        if (dev->dma_ops->free)
-            for (pb = 0; pb < 2; pb++)
-                dev->dma_ops->free(vc, pb);
 }
 }

@@ -1277,7 +1291,7 @@ int tw686x_video_init(struct tw686x_dev *dev)
     snprintf(vdev->name, sizeof(vdev->name), "%s video", dev->name);
     vdev->fops = &tw686x_video_fops;
     vdev->ioctl_ops = &tw686x_video_ioctl_ops;
-        vdev->release = video_device_release;
+        vdev->release = tw686x_video_device_release;
     vdev->v4l2_dev = &dev->v4l2_dev;
     vdev->queue = &vc->vidq;
     vdev->tvnorms = V4L2_STD_525_60 | V4L2_STD_625_50;
--
2.17.1

Re: [PATCH] media input infrastructure:tw686x: Added Added custom function to set vdev->release in tw686x driver

[Ezequiel Garcia]

Hi Mark,

On Wed, 24 Jul 2019 at 12:10, Mark Balantzyan  wrote:
>

This commit needs to be thoroughly explained in order to make sense.

> Signed-off-by: Mark Balantzyan 
> ---
> This patch adds a custom function to release video device in assignment to 
> vdev->release member in tw686x driver.
>
>  drivers/media/pci/tw686x/tw686x-video.c | 20 
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
> b/drivers/media/pci/tw686x/tw686x-video.c
> index 3a06c000..3631d0f5 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -1151,6 +1151,21 @@ void tw686x_video_irq(struct tw686x_dev *dev, unsigned 
> long requests,
> }
>  }
>
> +
> +
> +void tw686x_video_device_release(struct tw686x_video_channel *vc) {
> +   struct tw686x_dev *dev = vc->dev;
> +   unsigned int ch, pb;
> +
> +   for (ch = 0; ch < max_channels(dev); ch++) {
> +   struct tw686x_video_channel *vc = &dev->video_channels[ch];
> +
> +   dev->dma_ops->free;
> +

While I certainly appreciate your intention, you should at least test build
your patch.

You might want to read Documentation/process/ and get some insight on
upstreaming good practices.

Thanks,
Eze

> +   video_device_release((struct video_device*)dev);
> +
> +}
> +
>  void tw686x_video_free(struct tw686x_dev *dev)
>  {
> unsigned int ch, pb;
> @@ -1160,9 +1175,6 @@ void tw686x_video_free(struct tw686x_dev *dev)
>
> video_unregister_device(vc->device);
>
> -   if (dev->dma_ops->free)
> -   for (pb = 0; pb < 2; pb++)
> -   dev->dma_ops->free(vc, pb);
> }
>  }
>
> @@ -1277,7 +1289,7 @@ int tw686x_video_init(struct tw686x_dev *dev)
> snprintf(vdev->name, sizeof(vdev->name), "%s video", 
> dev->name);
> vdev->fops = &tw686x_video_fops;
> vdev->ioctl_ops = &tw686x_video_ioctl_ops;
> -   vdev->release = video_device_release;
> +   vdev->release = tw686x_video_device_release;
> vdev->v4l2_dev = &dev->v4l2_dev;
> vdev->queue = &vc->vidq;
> vdev->tvnorms = V4L2_STD_525_60 | V4L2_STD_625_50;
> --
> 2.17.1
>

[PATCH] media input infrastructure:tw686x: Added Added custom function to set vdev->release in tw686x driver

[Mark Balantzyan]

Signed-off-by: Mark Balantzyan 
---
This patch adds a custom function to release video device in assignment to 
vdev->release member in tw686x driver.

 drivers/media/pci/tw686x/tw686x-video.c | 20 
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
b/drivers/media/pci/tw686x/tw686x-video.c
index 3a06c000..3631d0f5 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -1151,6 +1151,21 @@ void tw686x_video_irq(struct tw686x_dev *dev, unsigned 
long requests,
}
 }
 
+
+
+void tw686x_video_device_release(struct tw686x_video_channel *vc) {
+   struct tw686x_dev *dev = vc->dev;
+   unsigned int ch, pb;
+
+   for (ch = 0; ch < max_channels(dev); ch++) {
+   struct tw686x_video_channel *vc = &dev->video_channels[ch];
+
+   dev->dma_ops->free;
+   
+   video_device_release((struct video_device*)dev);
+
+}
+
 void tw686x_video_free(struct tw686x_dev *dev)
 {
unsigned int ch, pb;
@@ -1160,9 +1175,6 @@ void tw686x_video_free(struct tw686x_dev *dev)
 
video_unregister_device(vc->device);
 
-   if (dev->dma_ops->free)
-   for (pb = 0; pb < 2; pb++)
-   dev->dma_ops->free(vc, pb);
}
 }
 
@@ -1277,7 +1289,7 @@ int tw686x_video_init(struct tw686x_dev *dev)
snprintf(vdev->name, sizeof(vdev->name), "%s video", dev->name);
vdev->fops = &tw686x_video_fops;
vdev->ioctl_ops = &tw686x_video_ioctl_ops;
-   vdev->release = video_device_release;
+   vdev->release = tw686x_video_device_release;
vdev->v4l2_dev = &dev->v4l2_dev;
vdev->queue = &vc->vidq;
vdev->tvnorms = V4L2_STD_525_60 | V4L2_STD_625_50;
-- 
2.17.1

Re: tw686x driver (continued)

[Hans Verkuil]

On 7/24/19 3:31 PM, Mark Balançian wrote:
> On Jul 24, 2019, at 6:08 AM, Hans Verkuil  > wrote:
>>
>> What tw686x_video_free() does really should be done in the release function
>> of the video_device: vdev->release is currently set to video_device_release,
>> but that should be a custom function that calls dev->dma_ops->free.
> 
> Hello all,
> 
> There just appears some possible race condition as detected by the tool my 
> supervisor has given me access to. I have to write a patch for
> the linux kernel as an assignment. By the above, does this mean I may please 
> write the custom function that calls dev->dma_ops->free to
> replace the vdev->release = video_device_release line?

Certainly. That's definitely a sensible change.

Regards,

Hans

Re: tw686x driver (continued)

[Hans Verkuil]

On 7/24/19 2:25 PM, Mark Balançian wrote:
> Hi Ezequiel,
> 
> (sorry didn't include linux-media in first email)
> I'm not sure yet if I have my supervisor's permission to declare our 
> tool as open source, but I'll tell you the possible code paths that I 
> think may be leading our tool to think what it's thinking.
> 
> First off, it detects a write access to desc->virt without locks inside 
> tw686x_memcpy_data_free, after it is called in the calling chain 
> tw686x_probe -> allocate an interrupt line -> tw686x_video_init -> 
> tw686x_set_format -> tw686x_memcpy_dma_free.

Until the video device has been registered there is no need for locking
since there is no possibility of concurrent access.

 Further,
> spin_lock_init(&dev->lock) (line 319 of tw686x-core.c) isn't 
> correspondingly closed in the function. Is this intended?

Huh? spin_lock_init doesn't take a lock. It just initializes internal
data.

> 
> Second, there is a possibility according to how I have traced a call 
> chain that tw686x_init is going to the error: label since 

tw686x_init? I assume you mean tw686x_video_init?

> tw686x_memcpy_dma_free is getting called inside another possible calling 
> chain, going tw686x_init -> tw686x_video_free (error: label) -> 
> dma_ops->free (i.e. tw686x_memcpy_dma_free). I would assume this would 
> not be intended either.

What tw686x_video_free() does really should be done in the release function
of the video_device: vdev->release is currently set to video_device_release,
but that should be a custom function that calls dev->dma_ops->free.

> In addition, our tool detects a read access without locks to desc->virt 
> inside tw686x_audio_irq (line 72 of tw686x-audio.c). Not sure what you 
> make of that, but I'd be keen on hearing about that as well from you.

No locks should be needed for that.

It is still not clear what your actual problem is: do you have crashes
sometimes? Is there really something wrong?

Regards,

Hans

Re: tw686x driver (continued)

[Mark Balançian]

TBH though, I’m not sure about the tw686x_init  going to the error: label 
though, so am curious as to your thoughts on this as well. It was just 
something I thought might be occurring due to the fact that the error trace in 
our tool ends in tw686x_memcpy_dma_free (i.e. dma_ops->free).

Please see the other indications I made toward the codes of tw686x-core.c and 
tw686x-audio.c in the message below.

Thank you,
Mark

> On Jul 24, 2019, at 5:25 AM, Mark Balançian  wrote:
> 
> Hi Ezequiel,
> 
> (sorry didn't include linux-media in first email)
> I'm not sure yet if I have my supervisor's permission to declare our tool as 
> open source, but I'll tell you the possible code paths that I think may be 
> leading our tool to think what it's thinking.
> 
> First off, it detects a write access to desc->virt without locks inside 
> tw686x_memcpy_data_free, after it is called in the calling chain tw686x_probe 
> -> allocate an interrupt line -> tw686x_video_init -> tw686x_set_format -> 
> tw686x_memcpy_dma_free. Further, spin_lock_init(&dev->lock) (line 319 of 
> tw686x-core.c) isn't correspondingly closed in the function. Is this intended?
> 
> Second, there is a possibility according to how I have traced a call chain 
> that tw686x_init is going to the error: label since tw686x_memcpy_dma_free is 
> getting called inside another possible calling chain, going tw686x_init -> 
> tw686x_video_free (error: label) -> dma_ops->free (i.e. 
> tw686x_memcpy_dma_free). I would assume this would not be intended either.
> 
> In addition, our tool detects a read access without locks to desc->virt 
> inside tw686x_audio_irq (line 72 of tw686x-audio.c). Not sure what you make 
> of that, but I'd be keen on hearing about that as well from you.
> 
> Thank you in advance,
> 
> Mark

Re: tw686x driver (continued)

[Mark Balançian]

Hi Ezequiel,

(sorry didn't include linux-media in first email)
I'm not sure yet if I have my supervisor's permission to declare our 
tool as open source, but I'll tell you the possible code paths that I 
think may be leading our tool to think what it's thinking.


First off, it detects a write access to desc->virt without locks inside 
tw686x_memcpy_data_free, after it is called in the calling chain 
tw686x_probe -> allocate an interrupt line -> tw686x_video_init -> 
tw686x_set_format -> tw686x_memcpy_dma_free. Further, 
spin_lock_init(&dev->lock) (line 319 of tw686x-core.c) isn't 
correspondingly closed in the function. Is this intended?


Second, there is a possibility according to how I have traced a call 
chain that tw686x_init is going to the error: label since 
tw686x_memcpy_dma_free is getting called inside another possible calling 
chain, going tw686x_init -> tw686x_video_free (error: label) -> 
dma_ops->free (i.e. tw686x_memcpy_dma_free). I would assume this would 
not be intended either.


In addition, our tool detects a read access without locks to desc->virt 
inside tw686x_audio_irq (line 72 of tw686x-audio.c). Not sure what you 
make of that, but I'd be keen on hearing about that as well from you.


Thank you in advance,

Mark

Re: tw686x driver (continued)

[Ezequiel Garcia]

On Tue, 23 Jul 2019 at 12:55, Mark Balançian  wrote:
>
> On Jul 23, 2019, at 8:17 AM, Ezequiel Garcia  
> wrote:
>
>
> On Tue, 23 Jul 2019 at 12:02, Mark Balançian  wrote:
>
>
> I see. I guess then my issue would be help in seeing how 
> tw686x_memcpy_dma_free alone does any required interrupt handling, since 
> there are also functions tw686x_irq and tw686x_audio_irq for interrupt 
> handling as well? However, in my analysis, they were called after 
> tw686x_memcpy_dma_free.
>
>
> What are you trying to do? and what is exactly not working?
>
> PS: Don't drop linux-media from the Cc list, and please don't top-post.
>
> Thanks,
> Eze
>
>
> I don’t know what top-posting is, but I take it that it means I should write 
> my email below the previous? Anyways, I’m working with a linux driver 
> verification team to detect race conditions in the kernel. Using our tool, we 
> detected a possible race condition with the tw686x driver.

Can you describe how is this race condition possible ? E.g. what are
the possible code paths and what would be the problem?

Also, is the tool open source?

> Even if there’s the slightest thing I’d like to please patch it as I really 
> need this for my enrolled program.
>

Sure, if there's anything to patch, let's patch it!

> In any case, if interrupt handing isn’t given dedicated functions that are 
> called before tw686x_memcpy_dma_free, I wouldn’t mind writing them and 
> including them in a patch.
>

I can't understand what you mean. Can you describe what is the issue
you are seeing in the driver?

Thanks,
Ezequiel

tw686x driver (continued)

[Mark Balançian]

Hi Ezequiel,

I don’t know what top-posting is and I had to delete the continuation as 
linux-media kept flagging my email as a virus, but I take it that it 
means I should write my email below the previous? Anyways, I’m working 
with a linux driver verification team to detect race conditions in the 
kernel. Using our tool, we detected a possible race condition with the 
tw686x driver. Even if there’s the slightest thing I’d like to please 
patch it as I really need this for my enrolled program.


In any case, if interrupt handing isn’t given dedicated functions that 
are called before tw686x_memcpy_dma_free, I wouldn’t mind writing them 
and including them in a patch.


Thank you,
Mark

Re: Question about TW686X driver

[Ezequiel Garcia]

On Tue, 23 Jul 2019 at 12:02, Mark Balançian  wrote:
>
> I see. I guess then my issue would be help in seeing how 
> tw686x_memcpy_dma_free alone does any required interrupt handling, since 
> there are also functions tw686x_irq and tw686x_audio_irq for interrupt 
> handling as well? However, in my analysis, they were called after 
> tw686x_memcpy_dma_free.
>

What are you trying to do? and what is exactly not working?

PS: Don't drop linux-media from the Cc list, and please don't top-post.

Thanks,
Eze

Re: Question about TW686X driver

[Ezequiel Garcia]

Hey Mark,

I'm glad you are having fun with the driver.

On Tue, 23 Jul 2019 at 11:45, Mark Balançian  wrote:
>
> Hello all,
>
> My name is Mark and I am working on contributing to the open-source Linux 
> project.
>
> I would please like to know more about the TW686X driver. In particular, when 
> running concurrency tests on the driver, it seems like there is no evidence 
> of interrupt handling/calling before tw686x_memcpy_dma_free(), which could be 
> problematic. Am I missing something? I would highly appreciate your 
> explanation for educational purposes.
>

Not necesarily, as the code explains:

/*
 * We can call this even when alloc_dma failed for the given channel
 */
static void tw686x_memcpy_dma_free(struct tw686x_video_channel *vc,
   unsigned int pb)

Are you having any specific issue ?

Thanks,
Ezequiel

Re: tw686x driver

[Hans Verkuil]

On 03/10/2016 08:16 AM, Krzysztof Hałasa wrote:
> Hans Verkuil  writes:
> 
>> Heck, if you prefer your driver can be added to staging first, then 
>> Ezequiel's
>> driver commit can directly refer to the staging driver as being derived from 
>> it.
> 
> Ok, I guess it's fair enough for me. Would you like me to send a patch
> with paths changed to staging/?

Yes please!

> However I'm not sure if Greg will like it - staging was meant for code
> not otherwise ready for mainline. Not a place for alternate drivers.

As I said before, it's for anything that is not considered ready for mainline 
for
whatever reason. In pretty sure Greg is only too happy to see an 'almost ready 
for
mainline' driver in staging instead of the usual crap :-)

Thanks!

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil  writes:

> Heck, if you prefer your driver can be added to staging first, then Ezequiel's
> driver commit can directly refer to the staging driver as being derived from 
> it.

Ok, I guess it's fair enough for me. Would you like me to send a patch
with paths changed to staging/?


However I'm not sure if Greg will like it - staging was meant for code
not otherwise ready for mainline. Not a place for alternate drivers.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil  writes:

> Sorry, I meant V4L2_FIELD_INTERLACED support. Very few applications support
> FIELD_TOP/BOTTOM, let alone SEQ_BT.

Well, that's doable, though not in SG mode. It still doesn't require
memcpy() of uncompressed video.

> I don't get it. Getting your driver in staging is much better for you since
> your code is in there and can be compiled for those who want to. I'm not
> going to add your driver and then replace it with Ezequiel's version.

Then simply add my driver and don't replace it.
Face it: Ezequiel's driver adds the audio support, and I guess he can
add this audio code without breaking the existing driver.
I also have (old) audio code for this driver, but it has only been
tested without an actual audio input, so it's not ready for deployment.
I simply don't use audio at the moment.

Otherwise, "his driver" is a regression - it removes critical
functionality, in exchange giving only the V4L2_FIELD_INTERLACED, which
can be easily implemented without breaking the rest.

> Heck, if you prefer your driver can be added to staging first, then Ezequiel's
> driver commit can directly refer to the staging driver as being
> derived from it.

Well, I will have to think about it. Though I wonder - if you do that,
perhaps my next request should be to swap them.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw686x driver

[Hans Verkuil]

On 03/04/2016 01:37 PM, Krzysztof Hałasa wrote:
> Hans Verkuil  writes:
> 
>> I have two drivers with different feature sets. Only one can be active
>> at a time. I have to make a choice which one I'll take and Ezequiel's
>> version has functionality (audio, interlaced support) which matches best
>> with existing v4l applications and the typical use cases. I'm not going
>> to have two drivers for the same hw in the media subsystem since only
>> one can be active anyway. My decision, although Mauro can of course decide
>> otherwise.
> 
> (BTW my driver supports interlace)

Sorry, I meant V4L2_FIELD_INTERLACED support. Very few applications support
FIELD_TOP/BOTTOM, let alone SEQ_BT.

> 
>> I am OK with adding your driver to staging in the hope that someone will
>> merged the functionalities of the two to make a new and better driver.
> 
> Then I don't really understand why there can be two drivers for the same
> hw in the tree, but one has to be in "staging".
> Staging isn't meant for this. My driver perfectly qualifies for being
> merged in the non-staging media directory - doesn't it?
> 
> You are right, there can be two drivers in the tree for the same hw,
> examples are known. You don't have to make a choice here, though you are
> free to do so.
> 
>> My goal is to provide the end-user with the best experience, and this is
>> IMHO the best option given the hand I've been dealt.
> 
> Then, if the moral side of the story can't be maintained, at least do it
> legally as required by copyright laws (and the GPL license as well).
> Doing so is not a "pollution" of git history, but your responsibility as
> a maintainer.
> 
> 
> To be honest, I still can't understand why are you afraid of adding
> Ezequiel's changes on top of my driver properly. While probably far from
> being a pretty changeset, it would make it legal, and this is the thing
> that the author, I suppose, is entitled to.
> Adding some "link" to a mail archive(?) is not a substitute.
> 

I don't get it. Getting your driver in staging is much better for you since
your code is in there and can be compiled for those who want to. I'm not
going to add your driver and then replace it with Ezequiel's version.

Heck, if you prefer your driver can be added to staging first, then Ezequiel's
driver commit can directly refer to the staging driver as being derived from it.

I'm not going to put both drivers in drivers/media. The functionalities of
these drivers should be merged. If I put both drivers in drivers/media then
that will never happen given human nature.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil  writes:

> I have two drivers with different feature sets. Only one can be active
> at a time. I have to make a choice which one I'll take and Ezequiel's
> version has functionality (audio, interlaced support) which matches best
> with existing v4l applications and the typical use cases. I'm not going
> to have two drivers for the same hw in the media subsystem since only
> one can be active anyway. My decision, although Mauro can of course decide
> otherwise.

(BTW my driver supports interlace)

> I am OK with adding your driver to staging in the hope that someone will
> merged the functionalities of the two to make a new and better driver.

Then I don't really understand why there can be two drivers for the same
hw in the tree, but one has to be in "staging".
Staging isn't meant for this. My driver perfectly qualifies for being
merged in the non-staging media directory - doesn't it?

You are right, there can be two drivers in the tree for the same hw,
examples are known. You don't have to make a choice here, though you are
free to do so.

> My goal is to provide the end-user with the best experience, and this is
> IMHO the best option given the hand I've been dealt.

Then, if the moral side of the story can't be maintained, at least do it
legally as required by copyright laws (and the GPL license as well).
Doing so is not a "pollution" of git history, but your responsibility as
a maintainer.


To be honest, I still can't understand why are you afraid of adding
Ezequiel's changes on top of my driver properly. While probably far from
being a pretty changeset, it would make it legal, and this is the thing
that the author, I suppose, is entitled to.
Adding some "link" to a mail archive(?) is not a substitute.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw686x driver

[Hans Verkuil]

On 03/04/2016 07:11 AM, Krzysztof Hałasa wrote:
> Hans Verkuil  writes:
> 
>>> Staging is meant for completely different situation - for immature,
>>> incomplete code. It has nothing to do with the case.
>>
>> It can be for anything that prevents it from being mainlined. It was (still 
>> is?)
>> used for mature android drivers, for example.
> 
> What is preventing my driver from being mainlined?

I have two drivers with different feature sets. Only one can be active
at a time. I have to make a choice which one I'll take and Ezequiel's
version has functionality (audio, interlaced support) which matches best
with existing v4l applications and the typical use cases. I'm not going
to have two drivers for the same hw in the media subsystem since only
one can be active anyway. My decision, although Mauro can of course decide
otherwise.

I am OK with adding your driver to staging in the hope that someone will
merged the functionalities of the two to make a new and better driver.

Whether that means that Ezequiel's code is merged into yours or vice versa,
I really don't care.

My goal is to provide the end-user with the best experience, and this is
IMHO the best option given the hand I've been dealt.

I ordered a tw6869-based PCIe card so I can do testing myself once it has
arrived.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil  writes:

>> Staging is meant for completely different situation - for immature,
>> incomplete code. It has nothing to do with the case.
>
> It can be for anything that prevents it from being mainlined. It was (still 
> is?)
> used for mature android drivers, for example.

What is preventing my driver from being mainlined?
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw686x driver

[Hans Verkuil]

On 03/03/16 15:22, Krzysztof Hałasa wrote:
> Hans Verkuil  writes:
> 
>> There is no point whatsoever in committing a driver and then replacing it
>> with another which has a different feature set. I'm not going to do
>> that.
> 
> Sure, that's why I haven't asked you to do it.
> Now there is no another driver, as Ezequiel stated - there is just one
> driver.
> 
> The point is clear, showing who exactly wrote what.
> 
>> One option that might be much more interesting is to add your driver to
>> staging with a TODO stating that the field support should be added to
>> the mainline driver.
> 
> Field mode is one thing. What's a bit more important is that Ezequiel's
> changes take away the SG DMA, and basically all DMA. The chip has to use
> DMA, but his driver then simply memcpy() all the data to userspace
> buffers. This doesn't work on low-power machines.
> 
> Staging is meant for completely different situation - for immature,
> incomplete code. It has nothing to do with the case.

It can be for anything that prevents it from being mainlined. It was (still is?)
used for mature android drivers, for example.

> 
>> I'm not sure if Mauro would go for it, but I think this is a fair option.
> 
> I don't expect the situation to be fair to me, anymore.
> 
> I also don't want to pursue the legal stuff, copyright laws etc.,
> but a quick glance at the COPYING file at the root of the kernel sources
> may be helpful:
> 
>> 2. You may modify your copy or copies of the Program or any portion
>> of it, thus forming a work based on the Program, and copy and
>> distribute such modifications or work under the terms of Section 1
>> above, provided that you also meet all of these conditions:
>>
>> a) You must cause the modified files to carry prominent notices
>> stating that you changed the files and the date of any change.
> 
> I don't even ask for that much - I only ask that the single set of
> changes from Ezequiel has this very information. This is BTW one of the
> reasons we switched to git.

Ezequiel, can you make a v4 and add a link to the original patch posted by
Krzysztof that you based your code on?

I think that takes care of the provenance.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil  writes:

> There is no point whatsoever in committing a driver and then replacing it
> with another which has a different feature set. I'm not going to do
> that.

Sure, that's why I haven't asked you to do it.
Now there is no another driver, as Ezequiel stated - there is just one
driver.

The point is clear, showing who exactly wrote what.

> One option that might be much more interesting is to add your driver to
> staging with a TODO stating that the field support should be added to
> the mainline driver.

Field mode is one thing. What's a bit more important is that Ezequiel's
changes take away the SG DMA, and basically all DMA. The chip has to use
DMA, but his driver then simply memcpy() all the data to userspace
buffers. This doesn't work on low-power machines.

Staging is meant for completely different situation - for immature,
incomplete code. It has nothing to do with the case.

> I'm not sure if Mauro would go for it, but I think this is a fair option.

I don't expect the situation to be fair to me, anymore.

I also don't want to pursue the legal stuff, copyright laws etc.,
but a quick glance at the COPYING file at the root of the kernel sources
may be helpful:

> 2. You may modify your copy or copies of the Program or any portion
> of it, thus forming a work based on the Program, and copy and
> distribute such modifications or work under the terms of Section 1
> above, provided that you also meet all of these conditions:
>
> a) You must cause the modified files to carry prominent notices
> stating that you changed the files and the date of any change.

I don't even ask for that much - I only ask that the single set of
changes from Ezequiel has this very information. This is BTW one of the
reasons we switched to git.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw686x driver

[Hans Verkuil]

On 03/03/16 13:41, Krzysztof Hałasa wrote:
> Hans Verkuil  writes:
> 
>> When a driver is merged for the first time in the kernel it is always as
>> a single change, i.e. you don't include the development history as that
>> would pollute the kernel's history.
> 
> We don't generally add new drivers with long patch series, that's right.
> That's because there is usually no reason to do so. This situation is
> different, there is a very good reason.
> 
> I'm not asking for doing this with a long patch set. A single patch from
> me + single patch containing the Ezequiel changes would suffice.
> 
>> Those earlier versions have never
>> been tested/reviewed to the same extent as the final version
> 
> This is not a real problem, given the code will be changed immediately
> again.
> 
>> and adding
>> them could break git bisect.
> 
> Not really. You can apply the version based on current media tree,
> I have posted it a week ago or so. This doesn't require any effort.
> 
> BTW applying the thing in two consecutive patches (the old version +
> changes) doesn't break git bisect at all. It only breaks the compiling,
> and only in the very case when git bisect happens to stop between the
> first and second patch, and the driver is configured in. Very unlikely,
> though the remedy is very easy as shown in "man git-bisect".
> This is a common thing when you do git bisect, though the related
> patches are usually much more distant and thus the probability that the
> kernel won't compile is much much greater.
> 
> Alternatively one could apply the original version to an older kernel and
> merge the product. Less trivial and I don't know why one would want to
> do so, given #1.
> 
> One could also disable the CONFIG_VIDEO_TW686X in Kconfig (at the
> original patch). Possibilities are endless.
> 
> We have to face it: there is absolutely no problem with adding the
> driver with two patches, either using my original patch or the updated
> one. And it doesn't require any effort, just a couple of git commands.
> 
>> It is a quite unusual situation and the only way I could make it worse would
>> by not merging anything.
> 
> Not really.
> There is a very easy way out. You can just add the driver properly with
> two patches.
> 
> 
> Though I don't know why do you think my driver alone doesn't qualify to
> be merged (without subsequent Ezequiel's changes). It's functional
> and stable, and I have been using it (in various versions) for many
> months. It's much more mature than many other drivers which make it to
> the kernel regularly.
> 
> It is definitely _not_ my driver that is problematic.
> 

There is no point whatsoever in committing a driver and then replacing it
with another which has a different feature set. I'm not going to do that.

One option that might be much more interesting is to add your driver to
staging with a TODO stating that the field support should be added to
the mainline driver. So the code is there and having it in staging makes
it a bit easier to do the migration. And if nothing is done about it
after 1-2 years, then it is removed again since that indicates that there
is not enough interest in the features specific to your driver version.

I'm not sure if Mauro would go for it, but I think this is a fair option.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil  writes:

> When a driver is merged for the first time in the kernel it is always as
> a single change, i.e. you don't include the development history as that
> would pollute the kernel's history.

We don't generally add new drivers with long patch series, that's right.
That's because there is usually no reason to do so. This situation is
different, there is a very good reason.

I'm not asking for doing this with a long patch set. A single patch from
me + single patch containing the Ezequiel changes would suffice.

> Those earlier versions have never
> been tested/reviewed to the same extent as the final version

This is not a real problem, given the code will be changed immediately
again.

> and adding
> them could break git bisect.

Not really. You can apply the version based on current media tree,
I have posted it a week ago or so. This doesn't require any effort.

BTW applying the thing in two consecutive patches (the old version +
changes) doesn't break git bisect at all. It only breaks the compiling,
and only in the very case when git bisect happens to stop between the
first and second patch, and the driver is configured in. Very unlikely,
though the remedy is very easy as shown in "man git-bisect".
This is a common thing when you do git bisect, though the related
patches are usually much more distant and thus the probability that the
kernel won't compile is much much greater.

Alternatively one could apply the original version to an older kernel and
merge the product. Less trivial and I don't know why one would want to
do so, given #1.

One could also disable the CONFIG_VIDEO_TW686X in Kconfig (at the
original patch). Possibilities are endless.

We have to face it: there is absolutely no problem with adding the
driver with two patches, either using my original patch or the updated
one. And it doesn't require any effort, just a couple of git commands.

> It is a quite unusual situation and the only way I could make it worse would
> by not merging anything.

Not really.
There is a very easy way out. You can just add the driver properly with
two patches.


Though I don't know why do you think my driver alone doesn't qualify to
be merged (without subsequent Ezequiel's changes). It's functional
and stable, and I have been using it (in various versions) for many
months. It's much more mature than many other drivers which make it to
the kernel regularly.

It is definitely _not_ my driver that is problematic.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw686x driver

[Hans Verkuil]

On 03/03/2016 07:51 AM, Krzysztof Hałasa wrote:
> Hi Hans,
> 
> Hans Verkuil  writes:
> 
>> So lessons learned:
>>
>> Krzysztof, next time don't wait many months before posting a new version 
>> fixing
>> requested changes.
> 
> Actually, this is not how it happened.
> 
> On July 3, 2015 I posted the original driver:
> http://www.spinics.net/lists/linux-media/msg91474.html
> 
> Ezequiel reviewed the code on 6 Jul 2015:
> http://www.spinics.net/lists/linux-media/msg91516.html
> 
> On 27 Jul 2015, as you can easily find in your own mail archive, he
> informed me that he's working on the driver and that he's going to post
> updated patches himself. This was holidays time for me and I stated
> that I have to suspend my work till the end of August.
> 
> I asked him to add his changes on top of my code several times (and this
> is really easy with git). This never happened, and on 14 Aug 2015 he
> posted:
> 
>> Problem is I've re-written the driver, taking yours as a starting point
>> and reference.
> 
>> In other words, I don't have a proper git branch with a history, starting
>> from the patch you submitted. Instead, I would be submitting a new
>> patch for Hans and Mauro to review.
> 
> Maybe I got the meaning of this wrong, and he wasn't writing about
> rewriting the driver "from scratch". Yes, after receiving this mail
> I stopped my development, waiting for his driver to show up. That's
> where the "many months" are. Yes, Ezequiel waited for "many months" with
> his version - not me.
> 
> Only after he has posted "his" driver, I could find out what the
> "rewriting" meant.

Thank you for the clarification.

> Don't get me wrong, I was never opposed to him improving my driver.
> I only requested that his contributions are clearly shown, in a form
> of a patch or a patch set (or a git tree etc.), and so are my own.
> I really can't understand why his code can't be transparently applied
> over my original patch (or the updated one, which compiles and works
> fine with recent media tree).
> 
> Is it too much to ask?

When a driver is merged for the first time in the kernel it is always as
a single change, i.e. you don't include the development history as that
would pollute the kernel's history. Those earlier versions have never
been tested/reviewed to the same extent as the final version and adding
them could break git bisect. So as a general rule this isn't done.

> The lesson I learned is thus this instead:
> - don't publish code because it can be hijacked, twisted and you'll
> have to fight for even getting your authorship back. Forget about proper
> attribution and history.

Your code is attributed in the source code and your patches are all archived
in patchwork and on mailinglist archives.

And should you decide to make patches for the merged driver adding back the
lost functionality, then feel free to yell loudly at Ezequiel in the commit
log (within reason, of course :-) ). Heck, I might add some lines of my own
to that.

It is a quite unusual situation and the only way I could make it worse would
by not merging anything.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw686x driver

[Krzysztof Hałasa]

Hi Hans,

Hans Verkuil  writes:

> So lessons learned:
>
> Krzysztof, next time don't wait many months before posting a new version 
> fixing
> requested changes.

Actually, this is not how it happened.

On July 3, 2015 I posted the original driver:
http://www.spinics.net/lists/linux-media/msg91474.html

Ezequiel reviewed the code on 6 Jul 2015:
http://www.spinics.net/lists/linux-media/msg91516.html

On 27 Jul 2015, as you can easily find in your own mail archive, he
informed me that he's working on the driver and that he's going to post
updated patches himself. This was holidays time for me and I stated
that I have to suspend my work till the end of August.

I asked him to add his changes on top of my code several times (and this
is really easy with git). This never happened, and on 14 Aug 2015 he
posted:

> Problem is I've re-written the driver, taking yours as a starting point
> and reference.

> In other words, I don't have a proper git branch with a history, starting
> from the patch you submitted. Instead, I would be submitting a new
> patch for Hans and Mauro to review.

Maybe I got the meaning of this wrong, and he wasn't writing about
rewriting the driver "from scratch". Yes, after receiving this mail
I stopped my development, waiting for his driver to show up. That's
where the "many months" are. Yes, Ezequiel waited for "many months" with
his version - not me.

Only after he has posted "his" driver, I could find out what the
"rewriting" meant.

Don't get me wrong, I was never opposed to him improving my driver.
I only requested that his contributions are clearly shown, in a form
of a patch or a patch set (or a git tree etc.), and so are my own.
I really can't understand why his code can't be transparently applied
over my original patch (or the updated one, which compiles and works
fine with recent media tree).

Is it too much to ask?


The lesson I learned is thus this instead:
- don't publish code because it can be hijacked, twisted and you'll
have to fight for even getting your authorship back. Forget about proper
attribution and history.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html