Re: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3268 bytes --]

On Fri, Jul 11, 2014 at 02:46:35AM +0000, Tc, Jenny wrote:
> > From: Sebastian Reichel [mailto:sre@kernel.org]
> > Sent: Tuesday, July 08, 2014 9:26 PM
> > To: Tc, Jenny
> > Cc: linux-kernel@vger.kernel.org; Dmitry Eremin-Solenikov; Pavel Machek; Stephen
> > Rothwell; Anton Vorontsov; David Woodhouse; David Cohen; Pallala, Ramakrishna
> > Subject: Re: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
> > 
> > On Tue, Jul 08, 2014 at 06:07:29AM +0000, Tc, Jenny wrote:
> > > > > +static int get_tempzone(struct psy_pse_chrg_prof *pse_mod_bprof,
> > > > > +		int temp)
> > > > > +{
> > > > > +	int i = 0;
> > > > > +	int temp_range_cnt;
> > > > > +
> > > > > +	temp_range_cnt =  min_t(u16, pse_mod_bprof->temp_mon_ranges,
> > > > > +					BATT_TEMP_NR_RNG);
> > > > > +	if ((temp < pse_mod_bprof->temp_low_lim) ||
> > > > > +		(temp > pse_mod_bprof->temp_mon_range[0].temp_up_lim))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	for (i = 0; i < temp_range_cnt; ++i)
> > > > > +		if (temp > pse_mod_bprof->temp_mon_range[i].temp_up_lim)
> > > > > +			break;
> > > > > +	return i-1;
> > > > > +}
> > > >
> > > > pse_mod_bprof->temp_mon_ranges > BATT_TEMP_NR_RNG is not allowed, so
> > > > I suggest to print an error and return some error code.
> > > >
> > > min_t takes care of the upper bound. The algorithm process
> > > BATT_TEMP_NR_RNG even if the actual number of zones are greater than this.
> > 
> > Right, the function will not fail, but the zone information table is truncated. I would
> > expect at least warning about that. I think it doesn't hurt to have a small function,
> > which validates the zone data as good as possible. Using incorrect temperature
> > zones is a safety thread and we should try our best to avoid exploding batteries ;)
> > 
> > Maybe something like that:
> > 
> > static bool check_tempzones(struct psy_pse_chrg_prof *pse_mod_bprof) {
> >     int i = 0;
> >     int last_temp = ;
> > 
> >     /* check size */
> >     if (BATT_TEMP_NR_RNG > pse_mod_bprof->temp_mon_ranges)
> >         return false;
> 
> This is in a way good to have, OK to implement the same.
> 
> But KO with below suggestion. This doesn't guarantee safety. IMHO
> Safety is 1/0 - SAFE or NOT SAFE. No half safety.

We won't be able to handle every possible case, but we can try our
best to avoid problems. One (possibly useless) safety check too much
is better than one missing check.

> To ensure complete safety, measures should be taken at the entry
> point- where data is read from external source. Since the
> algorithm gets the data from internal kernel component
> (power_supply_charger.c), it trust the data. Since the data is
> originated from battery identification driver, the safety should
> be ensured at that level.

I think some basic checks of kernel's platform data help to avoid
potential problems during development phase.

> >     /* check zone order */
> >     for (i = 0; i < pse_mod_bprof->temp_mon_ranges; i++) {
> >         if (last_temp < pse_mod_bprof->temp_mon_range[i].temp_up_lim)
> >             return false;
> >         last_temp = pse_mod_bprof->temp_mon_range[i].temp_up_lim;
> >     }
> > 
> >     return true;
> > }

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]