[PATCH v2 0/3] staging: tidspbridge: bugfixes

[PATCH v2 0/3] staging: tidspbridge: bugfixes

[Ionut Nicu]

Changes since v1:

* Split the mgr_enum_node_info patch into two patches:
one that fixes the issue and one that reorganizes the
code.

Ionut Nicu (3):
  staging: tidspbridge: fix mgr_enum_node_info
  staging: tidspbridge: mgr_enum_node_info cleanup
  staging: tidspbridge: fix kernel oops in bridge_io_get_proc_load

 drivers/staging/tidspbridge/core/io_sm.c |   78 +++++++++++------------------
 drivers/staging/tidspbridge/rmgr/mgr.c   |   50 +++++++------------
 2 files changed, 48 insertions(+), 80 deletions(-)

-- 
1.7.2.3

[PATCH v2 1/3] staging: tidspbridge: fix mgr_enum_node_info

[Ionut Nicu]

The current code was always returning a non-zero status value
to userspace applications when this ioctl was called.

The error code was ENODATA, which isn't actually an error,
it's always returned by dcd_enumerate_object() when it hits the
end of list.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Ionut Nicu <ionut.nicu@mindbit.ro>
---
 drivers/staging/tidspbridge/rmgr/mgr.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/mgr.c b/drivers/staging/tidspbridge/rmgr/mgr.c
index 0ea89a1..2eab6a5 100644
--- a/drivers/staging/tidspbridge/rmgr/mgr.c
+++ b/drivers/staging/tidspbridge/rmgr/mgr.c
@@ -169,6 +169,11 @@ int mgr_enum_node_info(u32 node_id, struct dsp_ndbprops *pndb_props,
 
 		}
 	}
+
+	/* the last status is not 0, but neither an error */
+	if (status > 0)
+		status = 0;
+
 	if (!status) {
 		if (node_id > (node_index - 1)) {
 			status = -EINVAL;
-- 
1.7.2.3

Re: [PATCH v2 1/3] staging: tidspbridge: fix mgr_enum_node_info

[Felipe Contreras]

On Fri, Nov 5, 2010 at 5:15 PM, Ionut Nicu <ionut.nicu@gmail.com> wrote:
> The current code was always returning a non-zero status value
> to userspace applications when this ioctl was called.
>
> The error code was ENODATA, which isn't actually an error,
> it's always returned by dcd_enumerate_object() when it hits the
> end of list.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Ionut Nicu <ionut.nicu@mindbit.ro>

You should leave the original author (me).

-- 
Felipe Contreras

Re: [PATCH v2 1/3] staging: tidspbridge: fix mgr_enum_node_info

[Greg KH]

On Fri, Nov 05, 2010 at 06:01:47PM +0200, Felipe Contreras wrote:
> On Fri, Nov 5, 2010 at 5:15 PM, Ionut Nicu <ionut.nicu@gmail.com> wrote:
> > The current code was always returning a non-zero status value
> > to userspace applications when this ioctl was called.
> >
> > The error code was ENODATA, which isn't actually an error,
> > it's always returned by dcd_enumerate_object() when it hits the
> > end of list.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > Signed-off-by: Ionut Nicu <ionut.nicu@mindbit.ro>
> 
> You should leave the original author (me).

Ick, yes, that is very bad.

Ionut, please go re-read Documentation/SubmittingPatches for how to
properly attribute the original author of the patch.  Please resend all
of these patches after ensuring that you have the proper authors
credited.

thanks,

greg k-h

Re: [PATCH v2 1/3] staging: tidspbridge: fix mgr_enum_node_info

[Ionut Nicu]

On Fri, 2010-11-05 at 09:09 -0700, Greg KH wrote:
> On Fri, Nov 05, 2010 at 06:01:47PM +0200, Felipe Contreras wrote:
> > On Fri, Nov 5, 2010 at 5:15 PM, Ionut Nicu <ionut.nicu@gmail.com> wrote:
> > > The current code was always returning a non-zero status value
> > > to userspace applications when this ioctl was called.
> > >
> > > The error code was ENODATA, which isn't actually an error,
> > > it's always returned by dcd_enumerate_object() when it hits the
> > > end of list.
> > >
> > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > > Signed-off-by: Ionut Nicu <ionut.nicu@mindbit.ro>
> > 
> > You should leave the original author (me).
> 
> Ick, yes, that is very bad.
> 
> Ionut, please go re-read Documentation/SubmittingPatches for how to
> properly attribute the original author of the patch.  Please resend all
> of these patches after ensuring that you have the proper authors
> credited.
> 

Oops. Sorry about that. I put my sign off on this patch because the fix
was also in v1 of this series, but it was combined with patch 3/3.

I definitely don't want to take credit for other people's work, so I
will re-submit this series.

Thanks,
Ionut. 

Re: [PATCH v2 1/3] staging: tidspbridge: fix mgr_enum_node_info

[Greg KH]

On Fri, Nov 05, 2010 at 06:17:55PM +0200, Ionut Nicu wrote:
> On Fri, 2010-11-05 at 09:09 -0700, Greg KH wrote:
> > On Fri, Nov 05, 2010 at 06:01:47PM +0200, Felipe Contreras wrote:
> > > On Fri, Nov 5, 2010 at 5:15 PM, Ionut Nicu <ionut.nicu@gmail.com> wrote:
> > > > The current code was always returning a non-zero status value
> > > > to userspace applications when this ioctl was called.
> > > >
> > > > The error code was ENODATA, which isn't actually an error,
> > > > it's always returned by dcd_enumerate_object() when it hits the
> > > > end of list.
> > > >
> > > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > > > Signed-off-by: Ionut Nicu <ionut.nicu@mindbit.ro>
> > > 
> > > You should leave the original author (me).
> > 
> > Ick, yes, that is very bad.
> > 
> > Ionut, please go re-read Documentation/SubmittingPatches for how to
> > properly attribute the original author of the patch.  Please resend all
> > of these patches after ensuring that you have the proper authors
> > credited.
> > 
> 
> Oops. Sorry about that. I put my sign off on this patch because the fix
> was also in v1 of this series, but it was combined with patch 3/3.
> 
> I definitely don't want to take credit for other people's work, so I
> will re-submit this series.

I now have to ask if your other series also all came from you, did it?
If not, please resend it as well.

thanks,

greg k-h

[PATCH v2 2/3] staging: tidspbridge: mgr_enum_node_info cleanup

[Ionut Nicu]

Reorganized mgr_enum_node_info code to increase its
readability.

Signed-off-by: Ionut Nicu <ionut.nicu@mindbit.ro>
---
 drivers/staging/tidspbridge/rmgr/mgr.c |   51 ++++++++++----------------------
 1 files changed, 16 insertions(+), 35 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/mgr.c b/drivers/staging/tidspbridge/rmgr/mgr.c
index 2eab6a5..16410a5 100644
--- a/drivers/staging/tidspbridge/rmgr/mgr.c
+++ b/drivers/staging/tidspbridge/rmgr/mgr.c
@@ -134,8 +134,7 @@ int mgr_enum_node_info(u32 node_id, struct dsp_ndbprops *pndb_props,
 			      u32 undb_props_size, u32 *pu_num_nodes)
 {
 	int status = 0;
-	struct dsp_uuid node_uuid, temp_uuid;
-	u32 temp_index = 0;
+	struct dsp_uuid node_uuid;
 	u32 node_index = 0;
 	struct dcd_genericobj gen_obj;
 	struct mgr_object *pmgr_obj = NULL;
@@ -149,24 +148,27 @@ int mgr_enum_node_info(u32 node_id, struct dsp_ndbprops *pndb_props,
 	*pu_num_nodes = 0;
 	/* Get the Manager Object from the driver data */
 	if (!drv_datap || !drv_datap->mgr_object) {
-		status = -ENODATA;
 		pr_err("%s: Failed to retrieve the object handle\n", __func__);
-		goto func_cont;
-	} else {
-		pmgr_obj = drv_datap->mgr_object;
+		return -ENODATA;
 	}
+	pmgr_obj = drv_datap->mgr_object;
 
 	DBC_ASSERT(pmgr_obj);
 	/* Forever loop till we hit failed or no more items in the
 	 * Enumeration. We will exit the loop other than 0; */
-	while (status == 0) {
-		status = dcd_enumerate_object(temp_index++, DSP_DCDNODETYPE,
-					      &temp_uuid);
-		if (status == 0) {
-			node_index++;
-			if (node_id == (node_index - 1))
-				node_uuid = temp_uuid;
-
+	while (!status) {
+		status = dcd_enumerate_object(node_index++, DSP_DCDNODETYPE,
+				&node_uuid);
+		if (status)
+			break;
+		*pu_num_nodes = node_index;
+		if (node_id == (node_index - 1)) {
+			status = dcd_get_object_def(pmgr_obj->hdcd_mgr,
+					&node_uuid, DSP_DCDNODETYPE, &gen_obj);
+			if (status)
+				break;
+			/* Get the Obj def */
+			*pndb_props = gen_obj.obj_data.node_obj.ndb_props;
 		}
 	}
 
@@ -174,27 +176,6 @@ int mgr_enum_node_info(u32 node_id, struct dsp_ndbprops *pndb_props,
 	if (status > 0)
 		status = 0;
 
-	if (!status) {
-		if (node_id > (node_index - 1)) {
-			status = -EINVAL;
-		} else {
-			status = dcd_get_object_def(pmgr_obj->hdcd_mgr,
-						    (struct dsp_uuid *)
-						    &node_uuid, DSP_DCDNODETYPE,
-						    &gen_obj);
-			if (!status) {
-				/* Get the Obj def */
-				*pndb_props =
-				    gen_obj.obj_data.node_obj.ndb_props;
-				*pu_num_nodes = node_index;
-			}
-		}
-	}
-
-func_cont:
-	DBC_ENSURE((!status && *pu_num_nodes > 0) ||
-		   (status && *pu_num_nodes == 0));
-
 	return status;
 }
 
-- 
1.7.2.3

[PATCH v2 3/3] staging: tidspbridge: fix kernel oops in bridge_io_get_proc_load

[Ionut Nicu]

The DSP shared memory area gets initialized only when
a COFF file is loaded.

If bridge_io_get_proc_load is called before loading a base
image into the DSP, the shared_mem member of the io manager
will be NULL, resulting in a kernel oops when it's dereferenced.

Also made some coding style changes to bridge_io_create.

Signed-off-by: Ionut Nicu <ionut.nicu@mindbit.ro>
---
 drivers/staging/tidspbridge/core/io_sm.c |   78 +++++++++++------------------
 1 files changed, 30 insertions(+), 48 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/io_sm.c b/drivers/staging/tidspbridge/core/io_sm.c
index de2cc3b..bd407b6 100644
--- a/drivers/staging/tidspbridge/core/io_sm.c
+++ b/drivers/staging/tidspbridge/core/io_sm.c
@@ -164,57 +164,41 @@ int bridge_io_create(struct io_mgr **io_man,
 			    struct dev_object *hdev_obj,
 			    const struct io_attrs *mgr_attrts)
 {
-	int status = 0;
 	struct io_mgr *pio_mgr = NULL;
-	struct shm *shared_mem = NULL;
 	struct bridge_dev_context *hbridge_context = NULL;
 	struct cfg_devnode *dev_node_obj;
 	struct chnl_mgr *hchnl_mgr;
 	u8 dev_type;
 
 	/* Check requirements */
-	if (!io_man || !mgr_attrts || mgr_attrts->word_size == 0) {
-		status = -EFAULT;
-		goto func_end;
-	}
+	if (!io_man || !mgr_attrts || mgr_attrts->word_size == 0)
+		return -EFAULT;
+
+	*io_man = NULL;
+
 	dev_get_chnl_mgr(hdev_obj, &hchnl_mgr);
-	if (!hchnl_mgr || hchnl_mgr->hio_mgr) {
-		status = -EFAULT;
-		goto func_end;
-	}
+	if (!hchnl_mgr || hchnl_mgr->hio_mgr)
+		return -EFAULT;
+
 	/*
 	 * Message manager will be created when a file is loaded, since
 	 * size of message buffer in shared memory is configurable in
 	 * the base image.
 	 */
 	dev_get_bridge_context(hdev_obj, &hbridge_context);
-	if (!hbridge_context) {
-		status = -EFAULT;
-		goto func_end;
-	}
+	if (!hbridge_context)
+		return -EFAULT;
+
 	dev_get_dev_type(hdev_obj, &dev_type);
-	/*
-	 * DSP shared memory area will get set properly when
-	 * a program is loaded. They are unknown until a COFF file is
-	 * loaded. I chose the value -1 because it was less likely to be
-	 * a valid address than 0.
-	 */
-	shared_mem = (struct shm *)-1;
 
 	/* Allocate IO manager object */
 	pio_mgr = kzalloc(sizeof(struct io_mgr), GFP_KERNEL);
-	if (pio_mgr == NULL) {
-		status = -ENOMEM;
-		goto func_end;
-	}
+	if (!pio_mgr)
+		return -ENOMEM;
 
 	/* Initialize chnl_mgr object */
-#if defined(CONFIG_TIDSPBRIDGE_BACKTRACE) || defined(CONFIG_TIDSPBRIDGE_DEBUG)
-	pio_mgr->pmsg = NULL;
-#endif
 	pio_mgr->hchnl_mgr = hchnl_mgr;
 	pio_mgr->word_size = mgr_attrts->word_size;
-	pio_mgr->shared_mem = shared_mem;
 
 	if (dev_type == DSP_UNIT) {
 		/* Create an IO DPC */
@@ -226,29 +210,24 @@ int bridge_io_create(struct io_mgr **io_man,
 
 		spin_lock_init(&pio_mgr->dpc_lock);
 
-		status = dev_get_dev_node(hdev_obj, &dev_node_obj);
+		if (dev_get_dev_node(hdev_obj, &dev_node_obj)) {
+			bridge_io_destroy(pio_mgr);
+			return -EIO;
+		}
 	}
 
-	if (!status) {
-		pio_mgr->hbridge_context = hbridge_context;
-		pio_mgr->shared_irq = mgr_attrts->irq_shared;
-		if (dsp_wdt_init())
-			status = -EPERM;
-	} else {
-		status = -EIO;
-	}
-func_end:
-	if (status) {
-		/* Cleanup */
+	pio_mgr->hbridge_context = hbridge_context;
+	pio_mgr->shared_irq = mgr_attrts->irq_shared;
+	if (dsp_wdt_init()) {
 		bridge_io_destroy(pio_mgr);
-		if (io_man)
-			*io_man = NULL;
-	} else {
-		/* Return IO manager object to caller... */
-		hchnl_mgr->hio_mgr = pio_mgr;
-		*io_man = pio_mgr;
+		return -EPERM;
 	}
-	return status;
+
+	/* Return IO manager object to caller... */
+	hchnl_mgr->hio_mgr = pio_mgr;
+	*io_man = pio_mgr;
+
+	return 0;
 }
 
 /*
@@ -1532,6 +1511,9 @@ int io_sh_msetting(struct io_mgr *hio_mgr, u8 desc, void *pargs)
 int bridge_io_get_proc_load(struct io_mgr *hio_mgr,
 				struct dsp_procloadstat *proc_lstat)
 {
+	if (!hio_mgr->shared_mem)
+		return -EFAULT;
+
 	proc_lstat->curr_load =
 			hio_mgr->shared_mem->load_mon_info.curr_dsp_load;
 	proc_lstat->predicted_load =
-- 
1.7.2.3

Re: [PATCH v2 0/3] staging: tidspbridge: bugfixes

[Greg KH]

On Fri, Nov 05, 2010 at 05:15:45PM +0200, Ionut Nicu wrote:
> Changes since v1:
> 
> * Split the mgr_enum_node_info patch into two patches:
> one that fixes the issue and one that reorganizes the
> code.

Are these for the .37 or .38 kernel?

thanks,

greg k-h

Re: [PATCH v2 0/3] staging: tidspbridge: bugfixes

[Ionut Nicu]

Hi,

On Fri, 2010-11-05 at 08:43 -0700, Greg KH wrote:
> On Fri, Nov 05, 2010 at 05:15:45PM +0200, Ionut Nicu wrote:
> > Changes since v1:
> > 
> > * Split the mgr_enum_node_info patch into two patches:
> > one that fixes the issue and one that reorganizes the
> > code.
> 
> Are these for the .37 or .38 kernel?
> 

They're bug fixes, so I think they should get into .37. It's up to you
to decide.

Thanks,
Ionut.

Re: [PATCH v2 0/3] staging: tidspbridge: bugfixes

[Felipe Contreras]

On Fri, Nov 5, 2010 at 6:00 PM, Ionut Nicu <ionut.nicu@mindbit.ro> wrote:
> On Fri, 2010-11-05 at 08:43 -0700, Greg KH wrote:
>> On Fri, Nov 05, 2010 at 05:15:45PM +0200, Ionut Nicu wrote:
>> > Changes since v1:
>> >
>> > * Split the mgr_enum_node_info patch into two patches:
>> > one that fixes the issue and one that reorganizes the
>> > code.
>>
>> Are these for the .37 or .38 kernel?
>
> They're bug fixes, so I think they should get into .37. It's up to you
> to decide.

Other than patch #2, I agree it would be better on .37.

-- 
Felipe Contreras