[PATCH 1/3] atomisp: Fix up the open v load race

[PATCH 1/3] atomisp: Fix up the open v load race

[Alan]

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.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---
 .../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:

[PATCH 2/3] atomisp: fix vfree of bogus data on unload

[Alan]

We load the firmware once, set pointers to it and then at some point release
it. We should not be doing a vfree() on the pointers into the firmware.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---
 .../atomisp/pci/atomisp2/css2400/sh_css_firmware.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
index 8158ea40d069..f181bd8fcee2 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
@@ -288,8 +288,6 @@ void sh_css_unload_firmware(void)
 		for (i = 0; i < sh_css_num_binaries; i++) {
 			if (fw_minibuffer[i].name)
 				kfree((void *)fw_minibuffer[i].name);
-			if (fw_minibuffer[i].buffer)
-				vfree((void *)fw_minibuffer[i].buffer);
 		}
 		kfree(fw_minibuffer);
 		fw_minibuffer = NULL;

[PATCH 3/3] atomisp: hmm gives a bogus warning on unload

[Alan]

The problem is that we allocated a dummy page to ensure that 0 aka NULL
isn't returned as an ISP pointer into the hmm space. The free routine rather
sensibly checks for bogus NULL frees but is tripped by hmm_cleanup freeing
the dummy.

Split the routine so that we can keep the protection check and avoid the
bogus warning on a cleanup

Signed-off-by: Alan Cox <alan@linux.intel.com>
---
 .../staging/media/atomisp/pci/atomisp2/hmm/hmm.c   |   21 +++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
index a1c81c12718c..dfb9bf54b5d2 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
@@ -42,6 +42,8 @@ static ia_css_ptr dummy_ptr;
 static bool hmm_initialized;
 struct _hmm_mem_stat hmm_mem_stat;
 
+static void hmm_do_free(ia_css_ptr virt);
+
 /*
  * p: private
  * s: shared
@@ -211,7 +213,7 @@ void hmm_cleanup(void)
 	sysfs_remove_group(&atomisp_dev->kobj, atomisp_attribute_group);
 
 	/* free dummy memory first */
-	hmm_free(dummy_ptr);
+	hmm_do_free(dummy_ptr);
 	dummy_ptr = 0;
 
 	hmm_bo_device_exit(&bo_device);
@@ -268,13 +270,10 @@ ia_css_ptr hmm_alloc(size_t bytes, enum hmm_bo_type type,
 	return 0;
 }
 
-void hmm_free(ia_css_ptr virt)
+static void hmm_do_free(ia_css_ptr virt)
 {
-	struct hmm_buffer_object *bo;
-
-	WARN_ON(!virt);
-
-	bo = hmm_bo_device_search_start(&bo_device, (unsigned int)virt);
+	struct hmm_buffer_object *bo =
+		hmm_bo_device_search_start(&bo_device, (unsigned int)virt);
 
 	if (!bo) {
 		dev_err(atomisp_dev,
@@ -290,6 +289,14 @@ void hmm_free(ia_css_ptr virt)
 	hmm_bo_unref(bo);
 }
 
+
+void hmm_free(ia_css_ptr virt)
+{
+	WARN_ON(!virt);
+
+	hmm_do_free(virt);
+}
+
 static inline int hmm_check_bo(struct hmm_buffer_object *bo, unsigned int ptr)
 {
 	if (!bo) {

Re: [PATCH 2/3] atomisp: fix vfree of bogus data on unload

[Sakari Ailus]

Hi Alan,

On Mon, Nov 06, 2017 at 11:36:45PM +0000, Alan wrote:
> We load the firmware once, set pointers to it and then at some point release
> it. We should not be doing a vfree() on the pointers into the firmware.
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> ---
>  .../atomisp/pci/atomisp2/css2400/sh_css_firmware.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> index 8158ea40d069..f181bd8fcee2 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> @@ -288,8 +288,6 @@ void sh_css_unload_firmware(void)
>  		for (i = 0; i < sh_css_num_binaries; i++) {
>  			if (fw_minibuffer[i].name)
>  				kfree((void *)fw_minibuffer[i].name);
> -			if (fw_minibuffer[i].buffer)
> -				vfree((void *)fw_minibuffer[i].buffer);

You shouldn't end up here if the firmware is just loaded once. If multiple
times, then yes.

The memory appears to have been allocated using kmalloc() in some cases.
How about kvfree(), or changing that kmalloc() to vmalloc()?

>  		}
>  		kfree(fw_minibuffer);
>  		fw_minibuffer = NULL;
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 2/3] atomisp: fix vfree of bogus data on unload

[Alan Cox]

On Tue, 14 Nov 2017 00:05:48 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> wrote:

> Hi Alan,
> 
> On Mon, Nov 06, 2017 at 11:36:45PM +0000, Alan wrote:
> > We load the firmware once, set pointers to it and then at some point release
> > it. We should not be doing a vfree() on the pointers into the firmware.
> > 
> > Signed-off-by: Alan Cox <alan@linux.intel.com>
> > ---
> >  .../atomisp/pci/atomisp2/css2400/sh_css_firmware.c |    2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> > index 8158ea40d069..f181bd8fcee2 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> > @@ -288,8 +288,6 @@ void sh_css_unload_firmware(void)
> >  		for (i = 0; i < sh_css_num_binaries; i++) {
> >  			if (fw_minibuffer[i].name)
> >  				kfree((void *)fw_minibuffer[i].name);
> > -			if (fw_minibuffer[i].buffer)
> > -				vfree((void *)fw_minibuffer[i].buffer);  
> 
> You shouldn't end up here if the firmware is just loaded once. If multiple
> times, then yes.

You end up there when unloading the module.

> The memory appears to have been allocated using kmalloc() in some cases.
> How about kvfree(), or changing that kmalloc() to vmalloc()

I'll take a deeper look at what is going on.

Alan

Re: [PATCH 2/3] atomisp: fix vfree of bogus data on unload

[Sakari Ailus]

On Tue, Nov 14, 2017 at 12:16:01AM +0000, Alan Cox wrote:
> On Tue, 14 Nov 2017 00:05:48 +0200
> Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> 
> > Hi Alan,
> > 
> > On Mon, Nov 06, 2017 at 11:36:45PM +0000, Alan wrote:
> > > We load the firmware once, set pointers to it and then at some point release
> > > it. We should not be doing a vfree() on the pointers into the firmware.
> > > 
> > > Signed-off-by: Alan Cox <alan@linux.intel.com>
> > > ---
> > >  .../atomisp/pci/atomisp2/css2400/sh_css_firmware.c |    2 --
> > >  1 file changed, 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> > > index 8158ea40d069..f181bd8fcee2 100644
> > > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> > > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> > > @@ -288,8 +288,6 @@ void sh_css_unload_firmware(void)
> > >  		for (i = 0; i < sh_css_num_binaries; i++) {
> > >  			if (fw_minibuffer[i].name)
> > >  				kfree((void *)fw_minibuffer[i].name);
> > > -			if (fw_minibuffer[i].buffer)
> > > -				vfree((void *)fw_minibuffer[i].buffer);  
> > 
> > You shouldn't end up here if the firmware is just loaded once. If multiple
> > times, then yes.
> 
> You end up there when unloading the module.

Ah, that's for sure indeed. I thought loading would be already a challenge.
:-)

> 
> > The memory appears to have been allocated using kmalloc() in some cases.
> > How about kvfree(), or changing that kmalloc() to vmalloc()
> 
> I'll take a deeper look at what is going on.

Look for minibuffer in sh_css_load_blob_info(). The buffer field of the
struct is allocated using kmalloc, I wonder if changing that to vmalloc
would just address this.

The buffer is elsewhere allocated using vmalloc. I suspect that some of the
cleanup patches changed how this works but missed changing the other one.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 1/3] atomisp: Fix up the open v load race

[Mauro Carvalho Chehab]

Em Mon, 06 Nov 2017 23:36:36 +0000
Alan <alan@linux.intel.com> 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@linux.intel.com>
> ---
>  .../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

Re: [PATCH 1/3] atomisp: Fix up the open v load race

[Alan Cox]

On Tue, 12 Dec 2017 09:03:50 -0200
Mauro Carvalho Chehab <mchehab@s-opensource.com> wrote:

> Em Mon, 06 Nov 2017 23:36:36 +0000
> Alan <alan@linux.intel.com> 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.

There are a whole bunch of other ordering issues to deal with in the
atomisp case beyond this - another one is that the camera probe can race
the ISP probe.

Quite a lot of the registration code needs fixing, but I'm prioritizing
stabilizing the code first, and trying to get the Cherrytrail version
going.

Alan