Em Mon, 06 Nov 2017 23:36:36 +0000
Alan <alan@xxxxxxxxxxxxxxx> escreveu:
> This isn't the ideal final solution but it stops the main problem for now
> where an open (often from udev) races the device initialization and we try
> and load the firmware twice at the same time. This needless to say doesn't
> usually end well.
What we do on most drivers is that video_register_device() is called
only after all hardware init.
That's usually enough to avoid race conditions with udev, although
a mutex is also common in order to avoid some other race conditions
between open/close - with can happen with multiple opens.
>
> Signed-off-by: Alan Cox <alan@xxxxxxxxxxxxxxx>
> ---
> .../media/atomisp/pci/atomisp2/atomisp_fops.c | 12 ++++++++++++
> .../media/atomisp/pci/atomisp2/atomisp_internal.h | 5 +++++
> .../media/atomisp/pci/atomisp2/atomisp_v4l2.c | 6 ++++++
> 3 files changed, 23 insertions(+)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
> index dd7596d8763d..b82c53cee32c 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
> @@ -771,6 +771,18 @@ static int atomisp_open(struct file *file)
>
> dev_dbg(isp->dev, "open device %s\n", vdev->name);
>
> + /* Ensure that if we are still loading we block. Once the loading
> + is over we can proceed. We can't blindly hold the lock until
> + that occurs as if the load fails we'll deadlock the unload */
> + rt_mutex_lock(&isp->loading);
> + /* Revisit this with a better check once the code structure is
> + cleaned up a bit more FIXME */
> + if (!isp->ready) {
> + rt_mutex_unlock(&isp->loading);
> + return -ENXIO;
> + }
> + rt_mutex_unlock(&isp->loading);
> +
> rt_mutex_lock(&isp->mutex);
>
> acc_node = !strncmp(vdev->name, "ATOMISP ISP ACC",
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
> index 52a6f8002048..808d79c840d4 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
> @@ -252,6 +252,11 @@ struct atomisp_device {
> /* Purpose of mutex is to protect and serialize use of isp data
> * structures and css API calls. */
> struct rt_mutex mutex;
> + /* This mutex ensures that we don't allow an open to succeed while
> + * the initialization process is incomplete */
> + struct rt_mutex loading;
> + /* Set once the ISP is ready to allow opens */
> + bool ready;
> /*
> * Serialise streamoff: mutex is dropped during streamoff to
> * cancel the watchdog queue. MUST be acquired BEFORE
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
> index 3c260f8b52e2..350e298bc3a6 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
> @@ -1220,6 +1220,7 @@ static int atomisp_pci_probe(struct pci_dev *dev,
> isp->saved_regs.ispmmadr = start;
>
> rt_mutex_init(&isp->mutex);
> + rt_mutex_init(&isp->loading);
> mutex_init(&isp->streamoff_mutex);
> spin_lock_init(&isp->lock);
>
> @@ -1393,6 +1394,8 @@ static int atomisp_pci_probe(struct pci_dev *dev,
> csi_afe_trim);
> }
>
> + rt_mutex_lock(&isp->loading);
> +
> err = atomisp_initialize_modules(isp);
> if (err < 0) {
> dev_err(&dev->dev, "atomisp_initialize_modules (%d)\n", err);
> @@ -1450,6 +1453,8 @@ static int atomisp_pci_probe(struct pci_dev *dev,
> release_firmware(isp->firmware);
> isp->firmware = NULL;
> isp->css_env.isp_css_fw.data = NULL;
> + isp->ready = true;
> + rt_mutex_unlock(&isp->loading);
>
> atomisp_drvfs_init(&atomisp_pci_driver, isp);
>
> @@ -1468,6 +1473,7 @@ static int atomisp_pci_probe(struct pci_dev *dev,
> register_entities_fail:
> atomisp_uninitialize_modules(isp);
> initialize_modules_fail:
> + rt_mutex_unlock(&isp->loading);
> pm_qos_remove_request(&isp->pm_qos);
> atomisp_msi_irq_uninit(isp, dev);
> enable_msi_fail:
>
Thanks,
Mauro
