media: atomisp: Fix locking around asd->streaming read/write

For reading / writing the asd->streaming enum the following rules
should be followed:

1. Writers of streaming must hold both isp->mutex and isp->lock.
2. Readers of streaming need to hold only one of the two locks.

Not all writers where properly taking both locks this fixes this.

In the case of the readers, many readers depend on their caller
to hold isp->mutex, add asserts for this

And in the case of atomisp_css_get_dis_stat() it is called with
isp->mutex held, so there is no need to take the spinlock just
for reading the streaming value.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 97ef02e4e7a6fa531acf8eb63a6e077db736e840..c7f825e38921d1bf3a08351bd90d77767f591a92 100644 (file)
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -899,6 +899,8 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, int error,
        struct v4l2_control ctrl;
        bool reset_wdt_timer = false;
 
+       lockdep_assert_held(&isp->mutex);
+
        if (
            buf_type != IA_CSS_BUFFER_TYPE_METADATA &&
            buf_type != IA_CSS_BUFFER_TYPE_3A_STATISTICS &&
@@ -1298,6 +1300,9 @@ static void __atomisp_css_recover(struct atomisp_device *isp, bool isp_timeout)
        bool stream_restart[MAX_STREAM_NUM] = {0};
        bool depth_mode = false;
        int i, ret, depth_cnt = 0;
+       unsigned long flags;
+
+       lockdep_assert_held(&isp->mutex);
 
        atomisp_css_irq_enable(isp, IA_CSS_IRQ_INFO_CSS_RECEIVER_SOF, false);
 
@@ -1320,7 +1325,9 @@ static void __atomisp_css_recover(struct atomisp_device *isp, bool isp_timeout)
 
                stream_restart[asd->index] = true;
 
+               spin_lock_irqsave(&isp->lock, flags);
                asd->streaming = ATOMISP_DEVICE_STREAMING_STOPPING;
+               spin_unlock_irqrestore(&isp->lock, flags);
 
                /* stream off sensor */
                ret = v4l2_subdev_call(
@@ -1335,7 +1342,9 @@ static void __atomisp_css_recover(struct atomisp_device *isp, bool isp_timeout)
                css_pipe_id = atomisp_get_css_pipe_id(asd);
                atomisp_css_stop(asd, css_pipe_id, true);
 
+               spin_lock_irqsave(&isp->lock, flags);
                asd->streaming = ATOMISP_DEVICE_STREAMING_DISABLED;
+               spin_unlock_irqrestore(&isp->lock, flags);
 
                asd->preview_exp_id = 1;
                asd->postview_exp_id = 1;
@@ -1376,11 +1385,14 @@ static void __atomisp_css_recover(struct atomisp_device *isp, bool isp_timeout)
                                                   IA_CSS_INPUT_MODE_BUFFERED_SENSOR);
 
                css_pipe_id = atomisp_get_css_pipe_id(asd);
-               if (atomisp_css_start(asd, css_pipe_id, true))
+               if (atomisp_css_start(asd, css_pipe_id, true)) {
                        dev_warn(isp->dev,
                                 "start SP failed, so do not set streaming to be enable!\n");
-               else
+               } else {
+                       spin_lock_irqsave(&isp->lock, flags);
                        asd->streaming = ATOMISP_DEVICE_STREAMING_ENABLED;
+                       spin_unlock_irqrestore(&isp->lock, flags);
+               }
 
                atomisp_csi2_configure(asd);
        }
@@ -1608,6 +1620,8 @@ void atomisp_css_flush(struct atomisp_device *isp)
 {
        int i;
 
+       lockdep_assert_held(&isp->mutex);
+
        if (!atomisp_streaming_count(isp))
                return;
 
@@ -4046,6 +4060,8 @@ void atomisp_handle_parameter_and_buffer(struct atomisp_video_pipe *pipe)
        unsigned long irqflags;
        bool need_to_enqueue_buffer = false;
 
+       lockdep_assert_held(&asd->isp->mutex);
+
        if (!asd) {
                dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
                        __func__, pipe->vdev.name);
@@ -4139,6 +4155,8 @@ int atomisp_set_parameters(struct video_device *vdev,
        struct atomisp_css_params *css_param = &asd->params.css_param;
        int ret;
 
+       lockdep_assert_held(&asd->isp->mutex);
+
        if (!asd) {
                dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
                        __func__, vdev->name);
@@ -5537,6 +5555,8 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
        struct v4l2_subdev_fh fh;
        int ret;
 
+       lockdep_assert_held(&isp->mutex);
+
        if (!asd) {
                dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
                        __func__, vdev->name);
@@ -6159,6 +6179,8 @@ int atomisp_offline_capture_configure(struct atomisp_sub_device *asd,
 {
        struct v4l2_ctrl *c;
 
+       lockdep_assert_held(&asd->isp->mutex);
+
        /*
        * In case of M10MO ZSL capture case, we need to issue a separate
        * capture request to M10MO which will output captured jpeg image
@@ -6433,6 +6455,8 @@ int atomisp_exp_id_capture(struct atomisp_sub_device *asd, int *exp_id)
        int value = *exp_id;
        int ret;
 
+       lockdep_assert_held(&isp->mutex);
+
        ret = __is_raw_buffer_locked(asd, value);
        if (ret) {
                dev_err(isp->dev, "%s exp_id %d invalid %d.\n", __func__, value, ret);
@@ -6454,6 +6478,8 @@ int atomisp_exp_id_unlock(struct atomisp_sub_device *asd, int *exp_id)
        int value = *exp_id;
        int ret;
 
+       lockdep_assert_held(&isp->mutex);
+
        ret = __clear_raw_buffer_bitmap(asd, value);
        if (ret) {
                dev_err(isp->dev, "%s exp_id %d invalid %d.\n", __func__, value, ret);
@@ -6489,6 +6515,8 @@ int atomisp_inject_a_fake_event(struct atomisp_sub_device *asd, int *event)
        if (!event || asd->streaming != ATOMISP_DEVICE_STREAMING_ENABLED)
                return -EINVAL;
 
+       lockdep_assert_held(&asd->isp->mutex);
+
        dev_dbg(asd->isp->dev, "%s: trying to inject a fake event 0x%x\n",
                __func__, *event);
 

diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
index cda0b5eba16dba2aa40f03d5a912e1c540459705..15ef31b0c601d3b71fb8f508ffba0507a94bc293 100644 (file)
--- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
@@ -3626,6 +3626,8 @@ int atomisp_css_get_dis_stat(struct atomisp_sub_device *asd,
        struct atomisp_dis_buf *dis_buf;
        unsigned long flags;
 
+       lockdep_assert_held(&isp->mutex);
+
        if (!asd->params.dvs_stat->hor_prod.odd_real ||
            !asd->params.dvs_stat->hor_prod.odd_imag ||
            !asd->params.dvs_stat->hor_prod.even_real ||
@@ -3637,12 +3639,8 @@ int atomisp_css_get_dis_stat(struct atomisp_sub_device *asd,
                return -EINVAL;
 
        /* isp needs to be streaming to get DIS statistics */
-       spin_lock_irqsave(&isp->lock, flags);
-       if (asd->streaming != ATOMISP_DEVICE_STREAMING_ENABLED) {
-               spin_unlock_irqrestore(&isp->lock, flags);
+       if (asd->streaming != ATOMISP_DEVICE_STREAMING_ENABLED)
                return -EINVAL;
-       }
-       spin_unlock_irqrestore(&isp->lock, flags);
 
        if (atomisp_compare_dvs_grid(asd, &stats->dvs2_stat.grid_info) != 0)
                /* If the grid info in the argument differs from the current
@@ -3801,6 +3799,8 @@ int atomisp_css_isr_thread(struct atomisp_device *isp,
        bool reset_wdt_timer[MAX_STREAM_NUM] = {false};
        int i;
 
+       lockdep_assert_held(&isp->mutex);
+
        while (!ia_css_dequeue_psys_event(&current_event.event)) {
                if (current_event.event.type ==
                    IA_CSS_EVENT_TYPE_FW_ASSERT) {

diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index 57587d739c4b950c868f3fbbf2b8ac0c1a7695cd..e1b213ba46865cc2568192f3400f589c08737d8c 100644 (file)
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -813,6 +813,7 @@ static int atomisp_release(struct file *file)
        struct v4l2_requestbuffers req;
        struct v4l2_subdev_fh fh;
        struct v4l2_rect clear_compose = {0};
+       unsigned long flags;
        int ret = 0;
 
        v4l2_fh_init(&fh.vfh, vdev);
@@ -878,7 +879,9 @@ static int atomisp_release(struct file *file)
 
        /* clear the asd field to show this camera is not used */
        isp->inputs[asd->input_curr].asd = NULL;
+       spin_lock_irqsave(&isp->lock, flags);
        asd->streaming = ATOMISP_DEVICE_STREAMING_DISABLED;
+       spin_unlock_irqrestore(&isp->lock, flags);
 
        if (atomisp_dev_users(isp))
                goto done;

diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index 759575cbd3561365672541014aeb37e1d0725864..b2c362ef7199499c8a1e8d692a816d8d31a3a114 100644 (file)
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -266,7 +266,7 @@ struct atomisp_device {
 
        atomic_t wdt_work_queued;
 
-       spinlock_t lock; /* Just for streaming below */
+       spinlock_t lock; /* Protects asd[i].streaming */
 
        bool need_gfx_throttle;
 

diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 4016ac4fffe063490ae7d2cc2b567ee84fa8f9b7..21af5feca386b74029c4524aa5303723f20726a3 100644 (file)
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1837,7 +1837,9 @@ static int atomisp_streamon(struct file *file, void *fh,
        if (ret)
                goto out;
 
+       spin_lock_irqsave(&isp->lock, irqflags);
        asd->streaming = ATOMISP_DEVICE_STREAMING_ENABLED;
+       spin_unlock_irqrestore(&isp->lock, irqflags);
        atomic_set(&asd->sof_count, -1);
        atomic_set(&asd->sequence, -1);
        atomic_set(&asd->sequence_temp, -1);
@@ -1910,7 +1912,9 @@ start_sensor:
        ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
                               video, s_stream, 1);
        if (ret) {
+               spin_lock_irqsave(&isp->lock, irqflags);
                asd->streaming = ATOMISP_DEVICE_STREAMING_DISABLED;
+               spin_unlock_irqrestore(&isp->lock, irqflags);
                ret = -EINVAL;
                goto out;
        }

diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index 4a43677015098c4b8dcc3f43bd8c2b1e9c992e50..88bf693f4c50ad6cd513b651851fea1aa3fc696c 100644 (file)
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -874,12 +874,18 @@ static int s_ctrl(struct v4l2_ctrl *ctrl)
 {
        struct atomisp_sub_device *asd = container_of(
                                             ctrl->handler, struct atomisp_sub_device, ctrl_handler);
+       unsigned int streaming;
+       unsigned long flags;
 
        switch (ctrl->id) {
        case V4L2_CID_RUN_MODE:
                return __atomisp_update_run_mode(asd);
        case V4L2_CID_DEPTH_MODE:
-               if (asd->streaming != ATOMISP_DEVICE_STREAMING_DISABLED) {
+               /* Use spinlock instead of mutex to avoid possible locking issues */
+               spin_lock_irqsave(&asd->isp->lock, flags);
+               streaming = asd->streaming;
+               spin_unlock_irqrestore(&asd->isp->lock, flags);
+               if (streaming != ATOMISP_DEVICE_STREAMING_DISABLED) {
                        dev_err(asd->isp->dev,
                                "ISP is streaming, it is not supported to change the depth mode\n");
                        return -EINVAL;

diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.h b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
index eaf7678804075e3490c97f1d5fdd605770c11f73..b44f060b0bb5206fa3d940aa003fef944d340603 100644 (file)
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
@@ -330,7 +330,11 @@ struct atomisp_sub_device {
        atomic_t sequence;      /* Sequence value that is assigned to buffer. */
        atomic_t sequence_temp;
 
-       unsigned int streaming; /* Hold both mutex and lock to change this */
+       /*
+        * Writers of streaming must hold both isp->mutex and isp->lock.
+        * Readers of streaming need to hold only one of the two locks.
+        */
+       unsigned int streaming;
        bool stream_prepared; /* whether css stream is created */
 
        /* subdev index: will be used to show which subdev is holding the