Hi, >-----Original Message----- >From: Felipe Contreras [mailto:felipe.contre...@gmail.com] >Sent: Tuesday, February 09, 2010 5:48 PM >To: Guzman Lugo, Fernando >Cc: Ameya Palande; linux-omap@vger.kernel.org; Ramirez Luna, Omar; Menon, >Nishanth; Chitriki Rudramuni, Deepak; felipe.contre...@nokia.com; ext- >phil.2.carm...@nokia.com >Subject: Re: [PATCH] DSPBRIDGE: Prevent memory corruption in >DRV_ProcFreeDMMRes > >On Tue, Feb 9, 2010 at 10:22 PM, Guzman Lugo, Fernando <x0095...@ti.com> >wrote: >> The root problem here is that remove the DMM element should be in >PROC_UnMap but in PROC_UnReserveMemory, because apart of the problem your >are seeing about memory corruption if the application does a >PROC_ReserveMemory and then it finished unexpectedly the resource cleanup >wont be able to unreserved the memory because there is no a DMMres element >in process context struct, the same applies in the case of application >calls PROC_ReserveMemory, PROC_Map, PROC_UnMap but fails to do the >PROC_UnReserveMemory. > >No, that's a completely different issue, but also valid. > >> So the right solution should be: >> >> 1 .- Remove DRV_InsertDMMResElement function from PROC_Map and put it on >PROC_ReserveMemory. >> >> 2.- Keep DRV_UpdateDMMResElement function in PROC_Map. >> >> 3.- Remove DRV_RemoveDMMResElement function from PROC_UnMap and put it on >ROC_UnReserveMemory function. >> >> 4.- Clear dmmAllocated flag in PROC_UnMap: >> >> pDMMRes->dmmAllocated = 0; > >At this point there's an obvious question; what's the point of >reserving a memory region and not mapping it? > >I remember the answer from Hari was: some clients prefer to reserve a >big region once, and map parts of it continuously. I have my doubts >that the use-case even works with the current code-base. But assuming >it does work, your proposed changes would break it.
Resource cleanup does not support that even without my proposed changes. I just proposed a solution which fixes two issues in one patch. Moreover if this change is merged when the second issue be fixed this patch will not needed anymore, so why don't merge the patch which fixes both errors at this moment? Anyway, if you want to include this patch, the only comments we have is: >+ * PROC_UnMap will free memory allocated to p_cur_res >+ * pointer. So we need to save following data for the >+ * execution of PROC_UnReserveMemory() >+ */ >+ void *h_processor = p_cur_res->hProcessor; You could use hProcessor store in p_ctxt instead of p_cur_res, so that you don't need to save hProcessor. >+ u32 dsp_res_addr = p_cur_res->ulDSPResAddr; >+ >+ status = PROC_UnMap(p_cur_res->hProcessor, >+ (void *)p_cur_res->ulDSPAddr, p_ctxt); > >It would be much easier to merge the two functions into one. Yes, I am agreed. > >Anyway, please re-read Ameya's description carefully. > >-- >Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
