[PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies ipu_pm

[Vadillo, Miguel]

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 at canonical.com]
> Sent: Tuesday, July 06, 2010 6:49 PM
> To: Vadillo, Miguel
> Cc: Bryan Wu; kernel-team at lists.ubuntu.com; ogra at 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 at canonical.com'
> >> Cc: Bryan Wu; kernel-team at lists.ubuntu.com; ogra at 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 at canonical.com]
> >>> Sent: Friday, July 02, 2010 8:17 AM
> >>> To: Vadillo, Miguel
> >>> Cc: Bryan Wu; kernel-team at lists.ubuntu.com; ogra at 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 at canonical.com
> >>
> >> Regards...
> >> Miguel Vadillo
> 
> 
> --
> Tim Gardner tim.gardner at canonical.com