| Message ID | 4C33C0E2.7070801@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
Hi Tim, Your patch is correct I was not taking care of all the memory leak scenarios for that version of the code, thanks. We also made a review of our code for the next release and we are covering and handling those scenarios properly. We are following your suggestions for declaring the sem handle statically and also the declaration of locally used functions and variables as static. Regards... Miguel Vadillo Cel. 214-587-2910 > -----Original Message----- > From: Tim Gardner [mailto:tim.gardner@canonical.com] > Sent: Tuesday, July 06, 2010 6:49 PM > To: Vadillo, Miguel > Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@canonical.com; Jan, > Sebastien; Hunt, Paul; Shah, Bhavin; Liu, Stanley; Gutierrez, Juan > Subject: Re: [PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies > ipu_pm > > Miguel, > > I think there are still memory leaks in your patch. How about the > attached version. > > rtg > > On 07/06/2010 01:45 PM, Vadillo, Miguel wrote: > > Tim, Bryan, > > > > Please find attached the patch that solves the error with the return > values in the ipu_pm_setup function. > > Regarding the semaphore and the static declaration both changes will > be included in the next release. > > > > Please let us know what you think. As always your feedback is > welcome. > > > > Regards... > > Miguel Vadillo > > Cel. 214-587-2910 > > > > > >> -----Original Message----- > >> From: Vadillo, Miguel > >> Sent: Friday, July 02, 2010 10:39 AM > >> To: 'tim.gardner@canonical.com' > >> Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@canonical.com; Jan, > >> Sebastien; Hunt, Paul; Shah, Bhavin; Liu, Stanley; Gutierrez, Juan > >> Subject: RE: [PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies > >> ipu_pm > >> > >>> -----Original Message----- > >>> From: Tim Gardner [mailto:tim.gardner@canonical.com] > >>> Sent: Friday, July 02, 2010 8:17 AM > >>> To: Vadillo, Miguel > >>> Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@canonical.com; Jan, > >>> Sebastien; Hunt, Paul; Shah, Bhavin; Liu, Stanley; Gutierrez, Juan > >>> Subject: Re: [PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies > >>> ipu_pm > >>> > >>> On 07/01/2010 09:53 AM, Vadillo, Miguel wrote: > >>> > >>>>>> /* > >>>>>> + Function for setup ipu_pm module > >>>>>> + * > >>>>>> + */ > >>>>>> +int ipu_pm_setup(void *notify_driver_handle) > >>>>>> +{ > >>>>>> + u32 i = 0; > >>>>>> + ipu_pm_notifydrv_handle = notify_driver_handle; > >>>>>> + /* Get the shared RCB */ > >>>>>> + rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR, > >>>>>> + sizeof(struct sms)); > >>>>>> + > >>>>>> + pm_event = kzalloc(sizeof(struct pm_event) * NUMBER_PM_EVENTS, > >>>>>> + GFP_KERNEL); > >>>>>> + > >>>>>> + /* Each event has it own sem */ > >>>>>> + for (i = 0; i< NUMBER_PM_EVENTS; i++) { > >>>>>> + pm_event[i].sem_handle = kzalloc(sizeof(struct > >> semaphore), > >>>>>> + GFP_KERNEL); > >>>>>> + sema_init(pm_event[i].sem_handle, 0); > >>>>>> + pm_event[i].event_type = i; > >>>>>> + } > >>>>>> + return 0; > >>>>>> +} > >>>>>> +EXPORT_SYMBOL(ipu_pm_setup); > >>>>>> + > >>>>> > >>>>> Why bother returning a status code if its just a constant? How about > >>>>> checking for failures when calling ioremap() and kzalloc() ? > >>>> > >>>> This part is now fixed and will be available for the next release > too. > >>> >We are checking the kzalloc. Also the ioremap is not done anymore > >>> since we are requesting a heap from the share memory. > >>> > >>> So, you're saying this is all different in 2.6.35 (which I assume is > the > >>> 'next release') ? That could be awhile. > >>> > >>> As far as I can tell from looking at the repositories at > >>> git://dev.omapzoom.org/pub/scm/integration you wrote these patches > just > >>> for the Ubuntu branch. Since we may have to live with this 2.6.34 > based > >>> kernel for a few weeks more, please take the time to write correct > code > >>> and resend this patch. At least put some BUG_ON()s in it so we catch > >>> allocation failures. > >>> > >> > >> > >> Ok in that case I will rework the patches following your suggestions > and > >> resend them ASAP, probably next week. > >> > >>> rtg > >>> -- > >>> Tim Gardner tim.gardner@canonical.com > >> > >> Regards... > >> Miguel Vadillo > > > -- > Tim Gardner tim.gardner@canonical.com
applied On 07/06/2010 06:47 PM, Vadillo, Miguel wrote: > Hi Tim, > > Your patch is correct I was not taking care of all the memory leak scenarios for that version of the code, thanks. > > We also made a review of our code for the next release and we are covering and handling those scenarios properly. We are following your suggestions for declaring the sem handle statically and also the declaration of locally used functions and variables as static. > > Regards... > Miguel Vadillo > Cel. 214-587-2910 > > >> -----Original Message----- >> From: Tim Gardner [mailto:tim.gardner@canonical.com] >> Sent: Tuesday, July 06, 2010 6:49 PM >> To: Vadillo, Miguel >> Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@canonical.com; Jan, >> Sebastien; Hunt, Paul; Shah, Bhavin; Liu, Stanley; Gutierrez, Juan >> Subject: Re: [PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies >> ipu_pm >> >> Miguel, >> >> I think there are still memory leaks in your patch. How about the >> attached version. >> >> rtg >> >> On 07/06/2010 01:45 PM, Vadillo, Miguel wrote: >>> Tim, Bryan, >>> >>> Please find attached the patch that solves the error with the return >> values in the ipu_pm_setup function. >>> Regarding the semaphore and the static declaration both changes will >> be included in the next release. >>> >>> Please let us know what you think. As always your feedback is >> welcome. >>> >>> Regards... >>> Miguel Vadillo >>> Cel. 214-587-2910 >>> >>> >>>> -----Original Message----- >>>> From: Vadillo, Miguel >>>> Sent: Friday, July 02, 2010 10:39 AM >>>> To: 'tim.gardner@canonical.com' >>>> Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@canonical.com; Jan, >>>> Sebastien; Hunt, Paul; Shah, Bhavin; Liu, Stanley; Gutierrez, Juan >>>> Subject: RE: [PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies >>>> ipu_pm >>>> >>>>> -----Original Message----- >>>>> From: Tim Gardner [mailto:tim.gardner@canonical.com] >>>>> Sent: Friday, July 02, 2010 8:17 AM >>>>> To: Vadillo, Miguel >>>>> Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@canonical.com; Jan, >>>>> Sebastien; Hunt, Paul; Shah, Bhavin; Liu, Stanley; Gutierrez, Juan >>>>> Subject: Re: [PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies >>>>> ipu_pm >>>>> >>>>> On 07/01/2010 09:53 AM, Vadillo, Miguel wrote: >>>>> >>>>>>>> /* >>>>>>>> + Function for setup ipu_pm module >>>>>>>> + * >>>>>>>> + */ >>>>>>>> +int ipu_pm_setup(void *notify_driver_handle) >>>>>>>> +{ >>>>>>>> + u32 i = 0; >>>>>>>> + ipu_pm_notifydrv_handle = notify_driver_handle; >>>>>>>> + /* Get the shared RCB */ >>>>>>>> + rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR, >>>>>>>> + sizeof(struct sms)); >>>>>>>> + >>>>>>>> + pm_event = kzalloc(sizeof(struct pm_event) * NUMBER_PM_EVENTS, >>>>>>>> + GFP_KERNEL); >>>>>>>> + >>>>>>>> + /* Each event has it own sem */ >>>>>>>> + for (i = 0; i< NUMBER_PM_EVENTS; i++) { >>>>>>>> + pm_event[i].sem_handle = kzalloc(sizeof(struct >>>> semaphore), >>>>>>>> + GFP_KERNEL); >>>>>>>> + sema_init(pm_event[i].sem_handle, 0); >>>>>>>> + pm_event[i].event_type = i; >>>>>>>> + } >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> +EXPORT_SYMBOL(ipu_pm_setup); >>>>>>>> + >>>>>>> >>>>>>> Why bother returning a status code if its just a constant? How about >>>>>>> checking for failures when calling ioremap() and kzalloc() ? >>>>>> >>>>>> This part is now fixed and will be available for the next release >> too. >>>>> >We are checking the kzalloc. Also the ioremap is not done anymore >>>>> since we are requesting a heap from the share memory. >>>>> >>>>> So, you're saying this is all different in 2.6.35 (which I assume is >> the >>>>> 'next release') ? That could be awhile. >>>>> >>>>> As far as I can tell from looking at the repositories at >>>>> git://dev.omapzoom.org/pub/scm/integration you wrote these patches >> just >>>>> for the Ubuntu branch. Since we may have to live with this 2.6.34 >> based >>>>> kernel for a few weeks more, please take the time to write correct >> code >>>>> and resend this patch. At least put some BUG_ON()s in it so we catch >>>>> allocation failures. >>>>> >>>> >>>> >>>> Ok in that case I will rework the patches following your suggestions >> and >>>> resend them ASAP, probably next week. >>>> >>>>> rtg >>>>> -- >>>>> Tim Gardner tim.gardner@canonical.com >>>> >>>> Regards... >>>> Miguel Vadillo >> >> >> -- >> Tim Gardner tim.gardner@canonical.com
From 3f7786199d1ee79871a3fcd7540bce09feaf1454 Mon Sep 17 00:00:00 2001 From: Tim Gardner <tim.gardner@canonical.com> Date: Tue, 6 Jul 2010 14:56:00 -0600 Subject: [PATCH] SYSLINK:IPU-PM: solve circular dependencies ipu_pm Changes to fix circular dependencies when compiling ipu_pm in modules mode. Signed-off-by: Miguel Vadillo <vadillo@ti.com> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- arch/arm/plat-omap/clock.c | 1 + drivers/dsp/syslink/ipu_pm/ipu_pm.c | 90 ++++++++++++++++++++++++- drivers/dsp/syslink/ipu_pm/ipu_pm.h | 10 ++- drivers/dsp/syslink/multicore_ipc/platform.c | 35 ++-------- 4 files changed, 101 insertions(+), 35 deletions(-) diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c index 701e4ea..104eaec 100644 --- a/arch/arm/plat-omap/clock.c +++ b/arch/arm/plat-omap/clock.c @@ -336,6 +336,7 @@ struct clk *omap_clk_get_by_name(const char *name) return ret; } +EXPORT_SYMBOL(omap_clk_get_by_name); /* * Low level helpers diff --git a/drivers/dsp/syslink/ipu_pm/ipu_pm.c b/drivers/dsp/syslink/ipu_pm/ipu_pm.c index 3806f5a..81d078e 100644 --- a/drivers/dsp/syslink/ipu_pm/ipu_pm.c +++ b/drivers/dsp/syslink/ipu_pm/ipu_pm.c @@ -85,6 +85,9 @@ int ipu_timer_list[NUM_IPU_TIMERS] = { struct omap_dm_timer *p_gpt; struct clk *p_i2c_clk; +struct sms *rcb_table; +void *ipu_pm_notifydrv_handle; +struct pm_event *pm_event; /** ============================================================================ * Forward declarations of internal functions @@ -221,7 +224,7 @@ void ipu_pm_callback(short int procId, /* send the ACK to DUCATI*/ return_val = notify_sendevent( - platform_notifydrv_handle, + ipu_pm_notifydrv_handle, SYS_M3,/*DUCATI_PROC*/ PM_RESOURCE,/*PWR_MGMT_EVENT*/ payload, @@ -280,7 +283,7 @@ int ipu_pm_notifications(enum pm_event_type event_type) pm_msg.fields.parm = PM_SUCCESS; /* send the ACK to DUCATI*/ return_val = notify_sendevent( - platform_notifydrv_handle, + ipu_pm_notifydrv_handle, SYS_M3,/*DUCATI_PROC*/ PM_NOTIFICATION,/*PWR_MGMT_EVENT*/ (unsigned int)pm_msg.whole, @@ -301,7 +304,7 @@ int ipu_pm_notifications(enum pm_event_type event_type) pm_msg.fields.parm = PM_SUCCESS; /* send the ACK to DUCATI*/ return_val = notify_sendevent( - platform_notifydrv_handle, + ipu_pm_notifydrv_handle, SYS_M3,/*DUCATI_PROC*/ PM_NOTIFICATION,/*PWR_MGMT_EVENT*/ (unsigned int)pm_msg.whole, @@ -322,7 +325,7 @@ int ipu_pm_notifications(enum pm_event_type event_type) pm_msg.fields.parm = PM_SUCCESS; /* send the ACK to DUCATI*/ return_val = notify_sendevent( - platform_notifydrv_handle, + ipu_pm_notifydrv_handle, SYS_M3,/*DUCATI_PROC*/ PM_NOTIFICATION,/*PWR_MGMT_EVENT*/ (unsigned int)pm_msg.whole, @@ -343,6 +346,83 @@ int ipu_pm_notifications(enum pm_event_type event_type) } EXPORT_SYMBOL(ipu_pm_notifications); +static void ipu_pm_free_events(struct pm_event *pm_event) +{ + if (pm_event) { + u32 i; + for (i = 0; i < NUMBER_PM_EVENTS; i++) { + kfree(pm_event[i].sem_handle); + pm_event[i].sem_handle = NULL; + pm_event[i].event_type = 0; + } + } +} + +static int ipu_pm_setup_events(struct pm_event *pm_event) +{ + u32 i; + for (i = 0; i < NUMBER_PM_EVENTS; i++) { + pm_event[i].sem_handle = kzalloc(sizeof(struct semaphore), + GFP_KERNEL); + if (WARN_ON(!pm_event[i].sem_handle)) + goto error; + sema_init(pm_event[i].sem_handle, 0); + pm_event[i].event_type = i; + } + + return 0; +error: + ipu_pm_free_events(pm_event); + return -ENOMEM; +} + +/* + Function for setup ipu_pm module + * + */ +int ipu_pm_setup(void *notify_driver_handle) +{ + ipu_pm_notifydrv_handle = notify_driver_handle; + + /* Get the shared RCB */ + rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR, + sizeof(struct sms)); + if (WARN_ON(!rcb_table)) + goto error; + + pm_event = kzalloc(sizeof(struct pm_event) * NUMBER_PM_EVENTS, + GFP_KERNEL); + if (WARN_ON(!pm_event)) + goto error; + + if (WARN_ON(ipu_pm_setup_events(pm_event) < 0)) + goto error; + + return 0; + +error: + ipu_pm_finish(); + return -ENOMEM; + +} +EXPORT_SYMBOL(ipu_pm_setup); + +/* + Function for finish ipu_pm module + * + */ +void ipu_pm_finish(void) +{ + ipu_pm_free_events(pm_event); + kfree(pm_event); + pm_event = NULL; + if (rcb_table) + iounmap(rcb_table); + rcb_table = NULL; + ipu_pm_notifydrv_handle = NULL; +} +EXPORT_SYMBOL(ipu_pm_finish); + /* Function for get sdma channels from PRCM * @@ -518,3 +598,5 @@ inline void ipu_pm_rel_i2c_bus(unsigned rcb_num) pm_i2c_bus_counter--; } +MODULE_LICENSE("GPL"); + diff --git a/drivers/dsp/syslink/ipu_pm/ipu_pm.h b/drivers/dsp/syslink/ipu_pm/ipu_pm.h index e9106fb..713e269 100644 --- a/drivers/dsp/syslink/ipu_pm/ipu_pm.h +++ b/drivers/dsp/syslink/ipu_pm/ipu_pm.h @@ -185,10 +185,6 @@ struct rcb_block { }; -extern struct sms *rcb_table; -extern void *platform_notifydrv_handle; -extern struct pm_event *pm_event; - struct sms { unsigned rat; struct rcb_block rcb[RCB_MAX]; @@ -214,5 +210,11 @@ void ipu_pm_notify_callback(short int procId, /* Function for send PM Notifications */ int ipu_pm_notifications(enum pm_event_type event_type); +/* Function for setup ipu_pm module */ +int ipu_pm_setup(void *notify_driver_handle); + +/* Function for finish ipu_pm module */ +void ipu_pm_finish(void); + #endif diff --git a/drivers/dsp/syslink/multicore_ipc/platform.c b/drivers/dsp/syslink/multicore_ipc/platform.c index d1c62e7..7490f76 100644 --- a/drivers/dsp/syslink/multicore_ipc/platform.c +++ b/drivers/dsp/syslink/multicore_ipc/platform.c @@ -233,9 +233,6 @@ */ void *platform_notifydrv_handle; -struct pm_event *pm_event; -struct sms *rcb_table; - /* Handles for SysM3 */ void *platform_nsrn_gate_handle_sysm3; void *platform_nsrn_handle_sysm3; @@ -982,22 +979,9 @@ void platform_start_callback(void *arg) goto pm_register_fail; } - /* Get the shared RCB */ - rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR, - sizeof(struct sms)); - - pm_event = - kzalloc(sizeof(struct pm_event) - * NUMBER_PM_EVENTS, GFP_KERNEL); - - /* Each event has it own sem */ - for (i = 0; i < NUMBER_PM_EVENTS; i++) { - pm_event[i].sem_handle = - kzalloc(sizeof(struct semaphore), - GFP_KERNEL); - sema_init(pm_event[i].sem_handle, 0); - pm_event[i].event_type = i; - } + if (ipu_pm_setup(platform_notifydrv_handle) < 0) + goto pm_setup_fail; + } /* END PM */ @@ -1315,6 +1299,9 @@ proc_invalid_id: goto exit; pm_register_fail: printk(KERN_ERR "pm register events failed"); + goto exit; +pm_setup_fail: + printk(KERN_ERR "ipu_pm_setup() failed"); exit: return; } @@ -1334,7 +1321,6 @@ void platform_stop_callback(void *arg) u16 proc_id = (u32) arg; int index = 0; u32 nread = 0; - u32 i = 0; if (proc_id == multiproc_get_id("SysM3")) index = SMHEAP_SRINDEX_SYSM3; @@ -1460,13 +1446,8 @@ void platform_stop_callback(void *arg) (void *)NULL); if (status < 0) printk(KERN_INFO "ERROR UNREGISTERING PM EVENT\n"); - for (i = 0; i < NUMBER_PM_EVENTS; i++) { - kfree(pm_event[i].sem_handle); - pm_event[i].event_type = 0; - } - kfree(pm_event); - /* Release the shared RCB */ - iounmap(rcb_table); + + ipu_pm_finish(); } /* END PM */ -- 1.7.0.4