Hi Hans, On Wed, 24 Jul 2019 at 10:08, Hans Verkuil <hverkuil@xxxxxxxxx> 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
