[PATCH 0/5] staging: tidspbridge: for 3.9

[PATCH 0/5] staging: tidspbridge: for 3.9

[Omar Ramirez Luna]

Patches for staging-next, fixing comments and suggestions provided
by Chen Gang.

There is an additional scm patch, that removes hardcoded defines
related to direct register handling for SCM, it was dependent on
changes that already made it to mainline.

Omar Ramirez Luna (5):
  staging: tidspbridge: fix potential array out of bounds write
  staging: tidspbridge: fix memory corruption on long string names
  staging: tidspbridge: fix uninitialized variable sym_name
  staging: tidspbridge: use scm functions to set boot address and mode
  staging: tidspbridge: remove unused code to handle iva_img

 drivers/staging/tidspbridge/core/tiomap3430.c      |   34 ++++---------------
 .../staging/tidspbridge/include/dspbridge/proc.h   |    2 -
 drivers/staging/tidspbridge/rmgr/dbdcd.c           |    3 +-
 drivers/staging/tidspbridge/rmgr/drv_interface.c   |    1 -
 drivers/staging/tidspbridge/rmgr/nldr.c            |    6 ++-
 drivers/staging/tidspbridge/rmgr/node.c            |   12 +++---
 drivers/staging/tidspbridge/rmgr/proc.c            |    6 +---
 7 files changed, 19 insertions(+), 45 deletions(-)

-- 
1.7.4.4

[PATCH 1/5] staging: tidspbridge: fix potential array out of bounds write

[Omar Ramirez Luna]

The name of the firmware (drv_datap->base_img) could potentially
become equal to 255 valid characters (size of exec_file), this
will result in an out of bounds write, given that the 255 chars
along with a '\0' terminator will be copied into an array of
255 chars.

Produce an error on this cases, because the driver expects the NULL
ending to be among the 255 char limit.

Reported-by: Chen Gang <gang.chen@asianux.com>
Signed-off-by: Omar Ramirez Luna <omar.ramirez@copitl.com>
---
 drivers/staging/tidspbridge/rmgr/proc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/proc.c b/drivers/staging/tidspbridge/rmgr/proc.c
index 5e43938..ac016ed 100644
--- a/drivers/staging/tidspbridge/rmgr/proc.c
+++ b/drivers/staging/tidspbridge/rmgr/proc.c
@@ -394,7 +394,7 @@ static int get_exec_file(struct cfg_devnode *dev_node_obj,
 		if (!drv_datap || !drv_datap->base_img)
 			return -EFAULT;
 
-		if (strlen(drv_datap->base_img) > size)
+		if (strlen(drv_datap->base_img) >= size)
 			return -EINVAL;
 
 		strcpy(exec_file, drv_datap->base_img);
-- 
1.7.4.4

[PATCH 2/5] staging: tidspbridge: fix memory corruption on long string names

[Omar Ramirez Luna]

The value allocated doesn't match the one that is meant to be
stored, resulting in corruption of memory for longer strings
that can't be held in such space.

Fix by allocating the correct byte value for the string meant to
be stored.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@copitl.com>
---
 drivers/staging/tidspbridge/rmgr/dbdcd.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/dbdcd.c b/drivers/staging/tidspbridge/rmgr/dbdcd.c
index 9d52c3c..3d2a26f 100644
--- a/drivers/staging/tidspbridge/rmgr/dbdcd.c
+++ b/drivers/staging/tidspbridge/rmgr/dbdcd.c
@@ -852,8 +852,7 @@ int dcd_register_object(struct dsp_uuid *uuid_obj,
 				goto func_end;
 			}
 
-			dcd_key->path = kmalloc(strlen(sz_reg_key) + 1,
-								GFP_KERNEL);
+			dcd_key->path = kmalloc(dw_path_size, GFP_KERNEL);
 
 			if (!dcd_key->path) {
 				kfree(dcd_key);
-- 
1.7.4.4

[PATCH 3/5] staging: tidspbridge: fix uninitialized variable sym_name

[Omar Ramirez Luna]

On both counts, sym_name could be printed uninitialized, this
is solved by moving the pr_* statement to be triggered if the
value is assigned.

Reported-by: Chen Gang <gang.chen@asianux.com>
Signed-off-by: Omar Ramirez Luna <omar.ramirez@copitl.com>
---
 drivers/staging/tidspbridge/rmgr/nldr.c |    6 ++++--
 drivers/staging/tidspbridge/rmgr/node.c |   12 ++++++------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/nldr.c b/drivers/staging/tidspbridge/rmgr/nldr.c
index 6309221..ca38050 100644
--- a/drivers/staging/tidspbridge/rmgr/nldr.c
+++ b/drivers/staging/tidspbridge/rmgr/nldr.c
@@ -1802,8 +1802,6 @@ int nldr_find_addr(struct nldr_nodeobject *nldr_node, u32 sym_addr,
 	bool status1 = false;
 	s32 i = 0;
 	struct lib_node root = { NULL, 0, NULL };
-	pr_debug("%s(0x%x, 0x%x, 0x%x, 0x%x,  %s)\n", __func__, (u32) nldr_node,
-			sym_addr, offset_range, (u32) offset_output, sym_name);
 
 	if (nldr_node->dynamic && *nldr_node->phase_split) {
 		switch (nldr_node->phase) {
@@ -1852,6 +1850,10 @@ int nldr_find_addr(struct nldr_nodeobject *nldr_node, u32 sym_addr,
 		pr_debug("%s: Address 0x%x not found in range %d.\n",
 					__func__, sym_addr, offset_range);
 		status = -ESPIPE;
+	} else {
+		pr_debug("%s(0x%x, 0x%x, 0x%x, 0x%x,  %s)\n",
+			 __func__, (u32) nldr_node, sym_addr, offset_range,
+			 (u32) offset_output, sym_name);
 	}
 
 	return status;
diff --git a/drivers/staging/tidspbridge/rmgr/node.c b/drivers/staging/tidspbridge/rmgr/node.c
index 737f4a9..87dfa92 100644
--- a/drivers/staging/tidspbridge/rmgr/node.c
+++ b/drivers/staging/tidspbridge/rmgr/node.c
@@ -3012,16 +3012,16 @@ int node_find_addr(struct node_mgr *node_mgr, u32 sym_addr,
 	struct node_object *node_obj;
 	int status = -ENOENT;
 
-	pr_debug("%s(0x%x, 0x%x, 0x%x, 0x%x,  %s)\n", __func__,
-			(unsigned int) node_mgr,
-			sym_addr, offset_range,
-			(unsigned int) sym_addr_output, sym_name);
-
 	list_for_each_entry(node_obj, &node_mgr->node_list, list_elem) {
 		status = nldr_find_addr(node_obj->nldr_node_obj, sym_addr,
 			offset_range, sym_addr_output, sym_name);
-		if (!status)
+		if (!status) {
+			pr_debug("%s(0x%x, 0x%x, 0x%x, 0x%x, %s)\n", __func__,
+				 (unsigned int) node_mgr,
+				 sym_addr, offset_range,
+				 (unsigned int) sym_addr_output, sym_name);
 			break;
+		}
 	}
 
 	return status;
-- 
1.7.4.4

[PATCH 4/5] staging: tidspbridge: use scm functions to set boot address and mode

[Omar Ramirez Luna]

Instead of ioremapping SCM registers, use the correspondent layer
to write into them.

This allows us to get rid of a layer violation, since the registers
are no longer touched by driver code.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@copitl.com>
---
 drivers/staging/tidspbridge/core/tiomap3430.c |   34 +++++-------------------
 1 files changed, 7 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/tiomap3430.c b/drivers/staging/tidspbridge/core/tiomap3430.c
index f619fb3..b770b22 100644
--- a/drivers/staging/tidspbridge/core/tiomap3430.c
+++ b/drivers/staging/tidspbridge/core/tiomap3430.c
@@ -70,14 +70,9 @@
 #define PAGES_II_LVL_TABLE   512
 #define PHYS_TO_PAGE(phys)      pfn_to_page((phys) >> PAGE_SHIFT)
 
-/*
- * This is a totally ugly layer violation, but needed until
- * omap_ctrl_set_dsp_boot*() are provided.
- */
-#define OMAP3_IVA2_BOOTMOD_IDLE 1
-#define OMAP2_CONTROL_GENERAL 0x270
-#define OMAP343X_CONTROL_IVA2_BOOTADDR (OMAP2_CONTROL_GENERAL + 0x0190)
-#define OMAP343X_CONTROL_IVA2_BOOTMOD (OMAP2_CONTROL_GENERAL + 0x0194)
+/* IVA Boot modes */
+#define DIRECT		0
+#define IDLE		1
 
 /* Forward Declarations: */
 static int bridge_brd_monitor(struct bridge_dev_context *dev_ctxt);
@@ -423,29 +418,14 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
 
 		/* Assert RST1 i.e only the RST only for DSP megacell */
 		if (!status) {
-			/*
-			 * XXX: OMAP343X_CTRL_BASE ioremapping  MUST be removed once ctrl
-			 * function is made available.
-			 */
-			void __iomem *ctrl = ioremap(0x48002000, SZ_4K);
-			if (!ctrl) {
-				iounmap(sync_addr);
-				return -ENOMEM;
-			}
-
 			(*pdata->dsp_prm_rmw_bits)(OMAP3430_RST1_IVA2_MASK,
 					OMAP3430_RST1_IVA2_MASK, OMAP3430_IVA2_MOD,
 					OMAP2_RM_RSTCTRL);
-			/* Mask address with 1K for compatibility */
-			__raw_writel(dsp_addr & OMAP3_IVA2_BOOTADDR_MASK,
-					ctrl + OMAP343X_CONTROL_IVA2_BOOTADDR);
-			/*
-			 * Set bootmode to self loop if dsp_debug flag is true
-			 */
-			__raw_writel((dsp_debug) ? OMAP3_IVA2_BOOTMOD_IDLE : 0,
-					ctrl + OMAP343X_CONTROL_IVA2_BOOTMOD);
 
-			iounmap(ctrl);
+			/* Mask address with 1K for compatibility */
+			pdata->set_bootaddr(dsp_addr &
+						OMAP3_IVA2_BOOTADDR_MASK);
+			pdata->set_bootmode(dsp_debug ? IDLE : DIRECT);
 		}
 	}
 	if (!status) {
-- 
1.7.4.4

[PATCH 5/5] staging: tidspbridge: remove unused code to handle iva_img

[Omar Ramirez Luna]

There is no way to specify the value of iva_img and since this code
is not being used, remove it.

This analysis resulted from a report by
Chen Gang <gang.chen@asianux.com>, mentioning that the existing code
was wrongly specifying the size to be copied.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@copitl.com>
---
 .../staging/tidspbridge/include/dspbridge/proc.h   |    2 --
 drivers/staging/tidspbridge/rmgr/drv_interface.c   |    1 -
 drivers/staging/tidspbridge/rmgr/proc.c            |    4 ----
 3 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/tidspbridge/include/dspbridge/proc.h b/drivers/staging/tidspbridge/include/dspbridge/proc.h
index 851b356..774a3f6 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/proc.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/proc.h
@@ -23,8 +23,6 @@
 #include <dspbridge/devdefs.h>
 #include <dspbridge/drv.h>
 
-extern char *iva_img;
-
 /*
  *  ======== proc_attach ========
  *  Purpose:
diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index e6f31d8..df0f37e 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -65,7 +65,6 @@ static struct class *bridge_class;
 static u32 driver_context;
 static s32 driver_major;
 static char *base_img;
-char *iva_img;
 static s32 shm_size = 0x500000;	/* 5 MB */
 static int tc_wordswapon;	/* Default value is always false */
 #ifdef CONFIG_TIDSPBRIDGE_RECOVERY
diff --git a/drivers/staging/tidspbridge/rmgr/proc.c b/drivers/staging/tidspbridge/rmgr/proc.c
index ac016ed..e1bdf6e 100644
--- a/drivers/staging/tidspbridge/rmgr/proc.c
+++ b/drivers/staging/tidspbridge/rmgr/proc.c
@@ -382,7 +382,6 @@ static int get_exec_file(struct cfg_devnode *dev_node_obj,
 				u32 size, char *exec_file)
 {
 	u8 dev_type;
-	s32 len;
 	struct drv_data *drv_datap = dev_get_drvdata(bridge);
 
 	dev_get_dev_type(hdev_obj, (u8 *) &dev_type);
@@ -398,9 +397,6 @@ static int get_exec_file(struct cfg_devnode *dev_node_obj,
 			return -EINVAL;
 
 		strcpy(exec_file, drv_datap->base_img);
-	} else if (dev_type == IVA_UNIT && iva_img) {
-		len = strlen(iva_img);
-		strncpy(exec_file, iva_img, len + 1);
 	} else {
 		return -ENOENT;
 	}
-- 
1.7.4.4

Re: [PATCH 0/5] staging: tidspbridge: for 3.9

[Greg Kroah-Hartman]

On Thu, Jan 10, 2013 at 03:36:57AM -0600, Omar Ramirez Luna wrote:
> Patches for staging-next, fixing comments and suggestions provided
> by Chen Gang.
> 
> There is an additional scm patch, that removes hardcoded defines
> related to direct register handling for SCM, it was dependent on
> changes that already made it to mainline.

What is the status on getting this out of the staging tree?  What needs
to be done still?

thanks,

greg k-h

Re: [PATCH 0/5] staging: tidspbridge: for 3.9

[Tony Lindgren]

* Greg Kroah-Hartman <gregkh@linuxfoundation.org> [130117 16:51]:
> On Thu, Jan 10, 2013 at 03:36:57AM -0600, Omar Ramirez Luna wrote:
> > Patches for staging-next, fixing comments and suggestions provided
> > by Chen Gang.
> > 
> > There is an additional scm patch, that removes hardcoded defines
> > related to direct register handling for SCM, it was dependent on
> > changes that already made it to mainline.
> 
> What is the status on getting this out of the staging tree?  What needs
> to be done still?

Omar, please correct me if I'm wrong. This thing should be reimplemented
using remoteproc AFAIK.

But I doubt that anybody is working on making it happen?

Regards,

Tony

Re: [PATCH 0/5] staging: tidspbridge: for 3.9

[Omar Ramirez Luna]

Hi Greg,

On Thu, Jan 17, 2013 at 6:47 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Jan 10, 2013 at 03:36:57AM -0600, Omar Ramirez Luna wrote:
>> Patches for staging-next, fixing comments and suggestions provided
>> by Chen Gang.
>>
>> There is an additional scm patch, that removes hardcoded defines
>> related to direct register handling for SCM, it was dependent on
>> changes that already made it to mainline.
>
> What is the status on getting this out of the staging tree?

I'm currently working to do this.

>  What needs
> to be done still?

- There is a tasklet that I'm working to remove.
- Some portions could be fitted into remoteproc (as Tony mentions).
- Migration to generic iommu framework.
- Need to rework patches to use request_firmware.
- And a thorough cleanup to get rid of if-else nesting.

At some point I should be able to work on all the items (since I have
the time), but any help is appreciated.

Cheers,

Omar

Re: [PATCH 0/5] staging: tidspbridge: for 3.9

[Omar Ramirez Luna]

Hi Tony,

On Thu, Jan 17, 2013 at 8:01 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [130117 16:51]:
>> On Thu, Jan 10, 2013 at 03:36:57AM -0600, Omar Ramirez Luna wrote:
>> > Patches for staging-next, fixing comments and suggestions provided
>> > by Chen Gang.
>> >
>> > There is an additional scm patch, that removes hardcoded defines
>> > related to direct register handling for SCM, it was dependent on
>> > changes that already made it to mainline.
>>
>> What is the status on getting this out of the staging tree?  What needs
>> to be done still?
>
> Omar, please correct me if I'm wrong. This thing should be reimplemented
> using remoteproc AFAIK.

tidspbridge might be able to use remoteproc, however it can't get rid
of all the infrastructure to support the SN interface, given that the
SN and firmware are distributed as binaries.

OTOH, remoteproc can also implement support for omap3 dsp, but the
major drawback is that we will lose the distributed codecs along with
gst-dsp support.

> But I doubt that anybody is working on making it happen?

I'm not aware of anybody working on remoteproc for omap3 (eventually I
could pick it up) but I'm more focused on getting tidspbridge out of
staging right now.

Cheers,

Omar

Re: [PATCH 0/5] staging: tidspbridge: for 3.9

[Greg Kroah-Hartman]

On Sun, Jan 20, 2013 at 05:45:16PM -0600, Omar Ramirez Luna wrote:
> Hi Greg,
> 
> On Thu, Jan 17, 2013 at 6:47 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Jan 10, 2013 at 03:36:57AM -0600, Omar Ramirez Luna wrote:
> >> Patches for staging-next, fixing comments and suggestions provided
> >> by Chen Gang.
> >>
> >> There is an additional scm patch, that removes hardcoded defines
> >> related to direct register handling for SCM, it was dependent on
> >> changes that already made it to mainline.
> >
> > What is the status on getting this out of the staging tree?
> 
> I'm currently working to do this.
> 
> >  What needs
> > to be done still?
> 
> - There is a tasklet that I'm working to remove.
> - Some portions could be fitted into remoteproc (as Tony mentions).
> - Migration to generic iommu framework.
> - Need to rework patches to use request_firmware.
> - And a thorough cleanup to get rid of if-else nesting.

That looks good, care to update the TODO file for the driver in the
kernel to reflect this?

thanks,

greg k-h

Re: [PATCH 0/5] staging: tidspbridge: for 3.9

[Omar Ramirez Luna]

On Sun, Jan 20, 2013 at 5:51 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> That looks good, care to update the TODO file for the driver in the
> kernel to reflect this?

I'll update it.

Cheers,

Omar

Re: [PATCH 0/5] staging: tidspbridge: for 3.9

[Dan Carpenter]

On Thu, Jan 17, 2013 at 04:47:46PM -0800, Greg Kroah-Hartman wrote:
> On Thu, Jan 10, 2013 at 03:36:57AM -0600, Omar Ramirez Luna wrote:
> > Patches for staging-next, fixing comments and suggestions provided
> > by Chen Gang.
> > 
> > There is an additional scm patch, that removes hardcoded defines
> > related to direct register handling for SCM, it was dependent on
> > changes that already made it to mainline.
> 
> What is the status on getting this out of the staging tree?  What needs
> to be done still?
> 

Why can't tidspbridge be built as a module?

The Kconfig help files aren't very informative.

regards,
dan carpenter

Re: [PATCH 0/5] staging: tidspbridge: for 3.9

[Dan Carpenter]

On Mon, Jan 21, 2013 at 10:25:24AM +0300, Dan Carpenter wrote:
> On Thu, Jan 17, 2013 at 04:47:46PM -0800, Greg Kroah-Hartman wrote:
> > On Thu, Jan 10, 2013 at 03:36:57AM -0600, Omar Ramirez Luna wrote:
> > > Patches for staging-next, fixing comments and suggestions provided
> > > by Chen Gang.
> > > 
> > > There is an additional scm patch, that removes hardcoded defines
> > > related to direct register handling for SCM, it was dependent on
> > > changes that already made it to mainline.
> > 
> > What is the status on getting this out of the staging tree?  What needs
> > to be done still?
> > 
> 
> Why can't tidspbridge be built as a module?
> 
> The Kconfig help files aren't very informative.

Huh.  Ignore me.  I don't know what I was looking at before.  Both
the things I said were wrong.

Sorry for the noise.

regards,
dan carpenter