* [PATCH] DSPBRIDGE: MEM_VFree() cleanup
@ 2010-02-01 17:56 Ameya Palande
2010-02-01 20:00 ` Menon, Nishanth
0 siblings, 1 reply; 3+ messages in thread
From: Ameya Palande @ 2010-02-01 17:56 UTC (permalink / raw)
To: omar.ramirez; +Cc: nm, linux-omap, deepak.chitriki
Since vfree() checks for null pointer, there is no need to check is again in
MEM_VFree(). This patch also reorganizes the function to make it more readable.
Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
---
drivers/dsp/bridge/services/mem.c | 42 +++++++++++++++----------------------
1 files changed, 17 insertions(+), 25 deletions(-)
diff --git a/drivers/dsp/bridge/services/mem.c b/drivers/dsp/bridge/services/mem.c
index dfe352d..c2887b3 100644
--- a/drivers/dsp/bridge/services/mem.c
+++ b/drivers/dsp/bridge/services/mem.c
@@ -456,35 +456,27 @@ void MEM_FlushCache(void *pMemBuf, u32 cBytes, s32 FlushType)
void MEM_VFree(IN void *pMemBuf)
{
#ifdef MEM_CHECK
- struct memInfo *pMem = (void *)((u32)pMemBuf - sizeof(struct memInfo));
+ struct memInfo *pMem;
#endif
-
- DBC_Require(pMemBuf != NULL);
-
GT_1trace(MEM_debugMask, GT_ENTER, "MEM_VFree: pMemBufs 0x%x\n",
- pMemBuf);
-
- if (pMemBuf) {
-#ifndef MEM_CHECK
- vfree(pMemBuf);
+ pMemBuf);
+#ifdef MEM_CHECK
+ if (!pMemBuf) {
+ GT_1trace(MEM_debugMask, GT_7CLASS,
+ "Invalid allocation or Buffer underflow\n");
+ return;
+ }
+ pMem = (void *)((u32)pMemBuf - sizeof(struct memInfo));
+ if (pMem && pMem->dwSignature == memInfoSign) {
+ spin_lock(&mMan.lock);
+ MLST_RemoveElem(&mMan.lst, (struct list_head *)pMem);
+ spin_unlock(&mMan.lock);
+ pMem->dwSignature = 0;
+ vfree(pMem);
+ }
#else
- if (pMem) {
- if (pMem->dwSignature == memInfoSign) {
- spin_lock(&mMan.lock);
- MLST_RemoveElem(&mMan.lst,
- (struct list_head *)pMem);
- spin_unlock(&mMan.lock);
- pMem->dwSignature = 0;
- vfree(pMem);
- } else {
- GT_1trace(MEM_debugMask, GT_7CLASS,
- "Invalid allocation or "
- "Buffer underflow at %x\n",
- (u32) pMem + sizeof(struct memInfo));
- }
- }
+ vfree(pMemBuf);
#endif
- }
}
/*
--
1.6.3.3
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] DSPBRIDGE: MEM_VFree() cleanup
2010-02-01 17:56 [PATCH] DSPBRIDGE: MEM_VFree() cleanup Ameya Palande
@ 2010-02-01 20:00 ` Menon, Nishanth
2010-02-02 18:44 ` Ameya Palande
0 siblings, 1 reply; 3+ messages in thread
From: Menon, Nishanth @ 2010-02-01 20:00 UTC (permalink / raw)
To: Ameya Palande
Cc: Ramirez Luna, Omar, linux-omap@vger.kernel.org,
Chitriki Rudramuni, Deepak
Ameya Palande said the following on 02/01/2010 07:56 PM:
> Since vfree() checks for null pointer, there is no need to check is again in
> MEM_VFree(). This patch also reorganizes the function to make it more readable.
>
> Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
> ---
> drivers/dsp/bridge/services/mem.c | 42 +++++++++++++++----------------------
> 1 files changed, 17 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/dsp/bridge/services/mem.c b/drivers/dsp/bridge/services/mem.c
> index dfe352d..c2887b3 100644
> --- a/drivers/dsp/bridge/services/mem.c
> +++ b/drivers/dsp/bridge/services/mem.c
> @@ -456,35 +456,27 @@ void MEM_FlushCache(void *pMemBuf, u32 cBytes, s32 FlushType)
> void MEM_VFree(IN void *pMemBuf)
> {
> #ifdef MEM_CHECK
> - struct memInfo *pMem = (void *)((u32)pMemBuf - sizeof(struct memInfo));
> + struct memInfo *pMem;
> #endif
> -
> - DBC_Require(pMemBuf != NULL);
does'nt this remove a warning for us?
> -
> GT_1trace(MEM_debugMask, GT_ENTER, "MEM_VFree: pMemBufs 0x%x\n",
> - pMemBuf);
> -
> - if (pMemBuf) {
> -#ifndef MEM_CHECK
> - vfree(pMemBuf);
> + pMemBuf);
probably an un-needed formattig change -> messed the diff a little bit
here
> +#ifdef MEM_CHECK
> + if (!pMemBuf) {
> + GT_1trace(MEM_debugMask, GT_7CLASS,
> + "Invalid allocation or Buffer underflow\n");
> + return;
> + }
> + pMem = (void *)((u32)pMemBuf - sizeof(struct memInfo));
> + if (pMem && pMem->dwSignature == memInfoSign) {
> + spin_lock(&mMan.lock);
> + MLST_RemoveElem(&mMan.lst, (struct list_head *)pMem);
> + spin_unlock(&mMan.lock);
> + pMem->dwSignature = 0;
^^^^^^^^^^^^^ Dumb question: signature should be locked? (yep the prev
code was as such)..
> + vfree(pMem);
> + }
> #else
> - if (pMem) {
> - if (pMem->dwSignature == memInfoSign) {
> - spin_lock(&mMan.lock);
> - MLST_RemoveElem(&mMan.lst,
> - (struct list_head *)pMem);
> - spin_unlock(&mMan.lock);
> - pMem->dwSignature = 0;
> - vfree(pMem);
> - } else {
> - GT_1trace(MEM_debugMask, GT_7CLASS,
> - "Invalid allocation or "
> - "Buffer underflow at %x\n",
> - (u32) pMem + sizeof(struct memInfo));
> - }
> - }
> + vfree(pMemBuf);
^^^^^^^^^^ it looks to me that if you put pMemBuf = pMem in the #ifdef
case, you could probably move vfree out of the #ifdef altogether..
> #endif
> - }
> }
>
> /*
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] DSPBRIDGE: MEM_VFree() cleanup
2010-02-01 20:00 ` Menon, Nishanth
@ 2010-02-02 18:44 ` Ameya Palande
0 siblings, 0 replies; 3+ messages in thread
From: Ameya Palande @ 2010-02-02 18:44 UTC (permalink / raw)
To: nm@ti.com
Cc: Ramirez Luna, Omar, linux-omap@vger.kernel.org,
Chitriki Rudramuni, Deepak
On Mon, 2010-02-01 at 21:00 +0100, ext Menon, Nishanth wrote:
> Ameya Palande said the following on 02/01/2010 07:56 PM:
> > Since vfree() checks for null pointer, there is no need to check is again in
> > MEM_VFree(). This patch also reorganizes the function to make it more readable.
> >
> > Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
> > ---
> > drivers/dsp/bridge/services/mem.c | 42 +++++++++++++++----------------------
> > 1 files changed, 17 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/dsp/bridge/services/mem.c b/drivers/dsp/bridge/services/mem.c
> > index dfe352d..c2887b3 100644
> > --- a/drivers/dsp/bridge/services/mem.c
> > +++ b/drivers/dsp/bridge/services/mem.c
> > @@ -456,35 +456,27 @@ void MEM_FlushCache(void *pMemBuf, u32 cBytes, s32 FlushType)
> > void MEM_VFree(IN void *pMemBuf)
> > {
> > #ifdef MEM_CHECK
> > - struct memInfo *pMem = (void *)((u32)pMemBuf - sizeof(struct memInfo));
> > + struct memInfo *pMem;
> > #endif
> > -
> > - DBC_Require(pMemBuf != NULL);
> does'nt this remove a warning for us?
> > -
> > GT_1trace(MEM_debugMask, GT_ENTER, "MEM_VFree: pMemBufs 0x%x\n",
> > - pMemBuf);
> > -
> > - if (pMemBuf) {
> > -#ifndef MEM_CHECK
> > - vfree(pMemBuf);
> > + pMemBuf);
> probably an un-needed formattig change -> messed the diff a little bit
> here
>
> > +#ifdef MEM_CHECK
> > + if (!pMemBuf) {
> > + GT_1trace(MEM_debugMask, GT_7CLASS,
> > + "Invalid allocation or Buffer underflow\n");
> > + return;
> > + }
> > + pMem = (void *)((u32)pMemBuf - sizeof(struct memInfo));
> > + if (pMem && pMem->dwSignature == memInfoSign) {
> > + spin_lock(&mMan.lock);
> > + MLST_RemoveElem(&mMan.lst, (struct list_head *)pMem);
> > + spin_unlock(&mMan.lock);
> > + pMem->dwSignature = 0;
> ^^^^^^^^^^^^^ Dumb question: signature should be locked? (yep the prev
> code was as such)..
> > + vfree(pMem);
> > + }
> > #else
> > - if (pMem) {
> > - if (pMem->dwSignature == memInfoSign) {
> > - spin_lock(&mMan.lock);
> > - MLST_RemoveElem(&mMan.lst,
> > - (struct list_head *)pMem);
> > - spin_unlock(&mMan.lock);
> > - pMem->dwSignature = 0;
> > - vfree(pMem);
> > - } else {
> > - GT_1trace(MEM_debugMask, GT_7CLASS,
> > - "Invalid allocation or "
> > - "Buffer underflow at %x\n",
> > - (u32) pMem + sizeof(struct memInfo));
> > - }
> > - }
> > + vfree(pMemBuf);
> ^^^^^^^^^^ it looks to me that if you put pMemBuf = pMem in the #ifdef
> case, you could probably move vfree out of the #ifdef altogether..
> > #endif
> > - }
> > }
> >
> > /*
>
> Regards,
> Nishanth Menon
NACK!
Nishanth, thanks for the review :)
I found a better solution, will send patches soon.
Cheers,
Ameya.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-02-02 18:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-01 17:56 [PATCH] DSPBRIDGE: MEM_VFree() cleanup Ameya Palande
2010-02-01 20:00 ` Menon, Nishanth
2010-02-02 18:44 ` Ameya Palande
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox