Re: [PATCH] Staging: TIDSPBRIDGE: Remove UUID helper
On 01.12.2013 21:06, Joe Perches wrote:
On Sun, 2013-12-01 at 19:07 +0200, Ivaylo DImitrov wrote:
From: Ivaylo Dimitrov
Custom uuid helper function is needed only in rmgr/dbdcd.c and doesn't
need to be exported. It can also be made way simpler by using sscanf.
[]
diff --git a/drivers/staging/tidspbridge/rmgr/dbdcd.c
b/drivers/staging/tidspbridge/rmgr/dbdcd.c
[]
@@ -74,6 +74,40 @@ static int get_dep_lib_info(struct dcd_manager *hdcd_mgr,
enum nldr_phase phase);
/*
+ * dcd_uuid_from_string
+ * Purpose:
+ * Converts an ANSI string to a dsp_uuid.
+ * Parameters:
+ * sz_uuid:Pointer to a string that represents a dsp_uuid object.
+ * uuid_obj: Pointer to a dsp_uuid object.
+ * Returns:
+ * Requires:
+ * uuid_obj & sz_uuid are non-NULL values.
+ * Ensures:
+ * Details:
+ * We assume the string representation of a UUID has the following format:
+ * "12345678_1234_1234_1234_123456789abc".
+ */
+static void dcd_uuid_from_string(char *sz_uuid, struct dsp_uuid *uuid_obj)
+{
+ char c;
+ u64 t;
+
+ /*
+* sscanf implementation cannot deal with hh format modifier
+* if the converted value doesn't fit in u32. So, convert the
+* last six bytes to u64 and memcpy what is needed
+*/
+ sscanf(sz_uuid, "%8x%c%4hx%c%4hx%c%2hhx%2hhx%c%llx",
+ &uuid_obj->data1, &c, &uuid_obj->data2, &c,
+ &uuid_obj->data3, &c, &uuid_obj->data4,
+ &uuid_obj->data5, &c, &t);
+
+ t = cpu_to_be64(t);
+ memcpy(&uuid_obj->data6[0], ((char*)&t) + 2, 6);
+}
It'd probably be better to return true or false on
successful conversion, use a temporary struct dsp_uuid,
check the sscanf return is 10 and only copy to uuid_obj
on success.
Something like:
static bool dcd_uuid_from_string(char *sz_uuid, struct dsp_uuid *uuid_obj)
{
char c;
u64 t;
struct dsp_uuid tmp;
/*
* sscanf implementation cannot deal with hh format modifier
* if the converted value doesn't fit in u32. So, convert the
* last six bytes to u64 and memcpy what is needed
*/
if (sscanf(sz_uuid, "%8x%c%4hx%c%4hx%c%2hhx%2hhx%c%llx",
&tmp.data1, &c, &tmp.data2, &c,
&tmp.data3, &c, &tmp.data4,
&tmp.data5, &c, &t) != 10)
return false;
t = cpu_to_be64(t);
memcpy(&tmp.data6[0], ((char*)&t) + 2, 6);
*uuid_obj = tmp;
return true;
}
If there is to be a return value from that function, it is better to be
int (-EINVAL/0), IMO. And I am not sure that makes much of a sense, as
(afaik) uuids are read from baseimage.dof and codec nodes, if those are
broken I suspect wrong uuids will be our least problem.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: TIDSPBRIDGE: Remove UUID helper
Hi Greg, On 01.12.2013 19:07, Ivaylo DImitrov wrote: From: Ivaylo Dimitrov Custom uuid helper function is needed only in rmgr/dbdcd.c and doesn't need to be exported. It can also be made way simpler by using sscanf. Signed-off-by: Ivaylo Dimitrov --- drivers/staging/tidspbridge/Makefile |2 +- drivers/staging/tidspbridge/gen/uuidutil.c | 85 .../tidspbridge/include/dspbridge/uuidutil.h | 18 drivers/staging/tidspbridge/rmgr/dbdcd.c | 42 +- 4 files changed, 39 insertions(+), 108 deletions(-) delete mode 100644 drivers/staging/tidspbridge/gen/uuidutil.c I guess the initial mail somehow didn't make it through your spam filter: https://lkml.org/lkml/2013/12/1/70 Regards, Ivo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: TIDSPBRIDGE: Remove UUID helper
My other patch (the one that fixes the security issue) was already applied, albeit it was sent after this one, see https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/commit/?h=staging-linus&id=559c71fe5dc3bf2ecc55afb336145db7f0abf810 , that is why I think there is some problem with the mail itself. Sure I will wait :) regards, Ivo On 06.12.2013 09:32, Dan Carpenter wrote: On Fri, Dec 06, 2013 at 08:05:38AM +0200, Ivajlo Dimitrov wrote: Hi Greg, On 01.12.2013 19:07, Ivaylo DImitrov wrote: From: Ivaylo Dimitrov Custom uuid helper function is needed only in rmgr/dbdcd.c and doesn't need to be exported. It can also be made way simpler by using sscanf. Signed-off-by: Ivaylo Dimitrov --- drivers/staging/tidspbridge/Makefile |2 +- drivers/staging/tidspbridge/gen/uuidutil.c | 85 .../tidspbridge/include/dspbridge/uuidutil.h | 18 drivers/staging/tidspbridge/rmgr/dbdcd.c | 42 +- 4 files changed, 39 insertions(+), 108 deletions(-) delete mode 100644 drivers/staging/tidspbridge/gen/uuidutil.c I guess the initial mail somehow didn't make it through your spam filter: https://lkml.org/lkml/2013/12/1/70 It's too early to start resending. Wait for another week at least. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: TIDSPBRIDGE: Remove UUID helper
On 06.12.2013 17:10, gre...@linuxfoundation.org wrote: On Fri, Dec 06, 2013 at 08:05:38AM +0200, Ivajlo Dimitrov wrote: Hi Greg, On 01.12.2013 19:07, Ivaylo DImitrov wrote: From: Ivaylo Dimitrov Custom uuid helper function is needed only in rmgr/dbdcd.c and doesn't need to be exported. It can also be made way simpler by using sscanf. Signed-off-by: Ivaylo Dimitrov --- drivers/staging/tidspbridge/Makefile |2 +- drivers/staging/tidspbridge/gen/uuidutil.c | 85 .../tidspbridge/include/dspbridge/uuidutil.h | 18 drivers/staging/tidspbridge/rmgr/dbdcd.c | 42 +- 4 files changed, 39 insertions(+), 108 deletions(-) delete mode 100644 drivers/staging/tidspbridge/gen/uuidutil.c I guess the initial mail somehow didn't make it through your spam filter: https://lkml.org/lkml/2013/12/1/70 It did, but I thought that people asked for it to be changed in the thread afterwards, so I was expecting an updated version from you. Care to fix things up and resend it? thanks, greg k-h Sure, the change I was asked for is trivial, but I didn't get the reason why it is needed. Neither there is a reply to my follow-up comment [0]. Sorry, I am pretty much new on LKML and could miss things that are supposed to be clear from the start, but my impression is that when someone says "it is better", he/she should explain why it is better or at least what is wrong with the patch he/she wants to be changed. However, I don't want to enter some arguing loop, so if you think I should change the code as per Joe's comment, just confirm it and I'll do it. Thanks, Ivo [0] https://lkml.org/lkml/2013/12/1/113 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: TIDSPBRIDGE: Use vm_iomap_memory for mmap-ing instead of remap_pfn_range
On 08.12.2013 01:49, Steven Luo wrote:
This patch causes problems with DSP codecs on OMAP3 devices running
Android -- specifically, when the decoder is cleaning up after itself,
munmap() of the mapped area fails, leading to a memory leak which
eventually crashes the system.
As far as I can tell, the code with this patch applied reduces to
(ignoring checks and such)
remap_pfn_range(vma, vma->vm_start,
(pdata->phys_mempool_base >> PAGE_SHIFT) + vma->vm_pgoff,
vma->vm_end - vma->vm_start,
vma->vm_page_prot);
whereas the original was
- status = remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
-vma->vm_end - vma->vm_start,
-vma->vm_page_prot);
We're subtracting (pdata->phys_mempool_base >> PAGE_SHIFT) from
vma->vm_pgoff before calling vm_iomap_memory() to address the issue --
if that's satisfactory to everyone involved, I can submit the following
patch.
-Steven Luo
(please cc, not subscribed)
From: Steven Luo
Date: Sat, 7 Dec 2013 02:11:20 -0800
Subject: [PATCH] tidspbridge: fix last patch to map same region of physical
memory as before
Commit 559c71fe5dc3 ("Staging: TIDSPBRIDGE: Use vm_iomap_memory for
mmap-ing instead of remap_pfn_range") had the effect of inadvertently
shifting the start of the physical memory area mapped by
pdata->phys_mempool_base. Correct this by subtracting that shift before
calling vm_iomap_memory() and adding it back afterwards.
Reported-by: Dheeraj CVR
Signed-off-by: Steven Luo
---
drivers/staging/tidspbridge/rmgr/drv_interface.c | 29 +++---
1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c
b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index 83cc3a5..d7f7d04 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -258,6 +258,9 @@ err:
/* This function maps kernel space memory to user space memory. */
static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
{
+ unsigned long base_pgoff;
+ int status;
+
struct omap_dsp_platform_data *pdata =
omap_dspbridge_dev->dev.platform_data;
@@ -269,9 +272,29 @@ static int bridge_mmap(struct file *filp, struct
vm_area_struct *vma)
vma->vm_start, vma->vm_end, vma->vm_page_prot,
vma->vm_flags);
- return vm_iomap_memory(vma,
- pdata->phys_mempool_base,
- pdata->phys_mempool_size);
+ /*
+* vm_iomap_memory() expects vma->vm_pgoff to be expressed as an offset
+* from the start of the physical memory pool, but we're called with
+* a pfn (physical page number) stored there instead.
+*
+* To avoid duplicating lots of tricky overflow checking logic,
+* temporarily convert vma->vm_pgoff to the offset vm_iomap_memory()
+* expects, but restore the original value once the mapping has been
+* created.
+*/
+ base_pgoff = pdata->phys_mempool_base >> PAGE_SHIFT;
+ if (vma->vm_pgoff < base_pgoff)
+ return -EINVAL;
+ vma->vm_pgoff -= base_pgoff;
+
+ status = vm_iomap_memory(vma,
+pdata->phys_mempool_base,
+pdata->phys_mempool_size);
+
+ /* Restore the original value of vma->vm_pgoff */
+ vma->vm_pgoff += base_pgoff;
+
+ return status;
}
static const struct file_operations bridge_fops = {
Tested on Nokia N900 with Maemo 5 and Harmattan codec nodes
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: TIDSPBRIDGE: Use vm_iomap_memory for mmap-ing instead of remap_pfn_range
On 08.12.2013 01:49, Steven Luo wrote: This patch causes problems with DSP codecs on OMAP3 devices running Android -- specifically, when the decoder is cleaning up after itself, munmap() of the mapped area fails, leading to a memory leak which eventually crashes the system. As far as I can tell, the code with this patch applied reduces to (ignoring checks and such) remap_pfn_range(vma, vma->vm_start, (pdata->phys_mempool_base >> PAGE_SHIFT) + vma->vm_pgoff, vma->vm_end - vma->vm_start, vma->vm_page_prot); whereas the original was - status = remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, -vma->vm_end - vma->vm_start, -vma->vm_page_prot); We're subtracting (pdata->phys_mempool_base >> PAGE_SHIFT) from vma->vm_pgoff before calling vm_iomap_memory() to address the issue -- if that's satisfactory to everyone involved, I can submit the following patch. Hi, I can pick your changes and re-send the original patch with them incorporated if there are no objections. Are you fine with that? Regards, Ivo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
