linux-kernel - Re: [PATCH 1/1] thermal: of: improve of-thermal sensor registration API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20141118125029.GA15239@developer>
Date:	Tue, 18 Nov 2014 08:50:31 -0400
From:	Eduardo Valentin <edubezval@...il.com>
To:	Lukasz Majewski <l.majewski@...sung.com>
Cc:	Linux PM <linux-pm@...r.kernel.org>,
	Caesar Wang <caesar.wang@...k-chips.com>,
	Wei Ni <wni@...dia.com>,
	Mikko Perttunen <mikko.perttunen@...si.fi>,
	Alexandre Courbot <gnurou@...il.com>,
	devicetree@...r.kernel.org, Grant Likely <grant.likely@...aro.org>,
	Guenter Roeck <linux@...ck-us.net>,
	Jean Delvare <jdelvare@...e.de>, linux-kernel@...r.kernel.org,
	linux-tegra@...r.kernel.org, lm-sensors@...sensors.org,
	Rob Herring <robh+dt@...nel.org>,
	Stephen Warren <swarren@...dotorg.org>,
	Thierry Reding <thierry.reding@...il.com>,
	Zhang Rui <rui.zhang@...el.com>
Subject: Re: [PATCH 1/1] thermal: of: improve of-thermal sensor registration
 API


Hey Lukasz,

On Tue, Nov 18, 2014 at 08:38:57AM +0100, Lukasz Majewski wrote:
> Hi Eduardo,
> 
> In the mail topic we have PATCH 1/1 but I think that it should be PATCH
> v3 1/1.
> 

Yeah, sent it without checking that. Fixing in V4, no issues.

<big cut>

> > @@ -107,10 +106,7 @@ static int of_thermal_get_temp(struct
> > thermal_zone_device *tz, {
> >  	struct __thermal_zone *data = tz->devdata;
> >  
> > -	if (!data->get_temp)
> > -		return -EINVAL;
> 
> To be consistent, I think that we should keep the above check [1]. 
> 
> 	if (!data->ops->get_temp)
> 		return -EINVAL;
> 
> The same check is done with get_trend callback.
> 

OK. I agree, and disagree, :-). Now that you mention here, I will resend
with your request applied. The reasoning is to, yes, keep the
consistency. However, not to be the same as .get_trend, but in fact, to
keep same behavior as the code as it is currently. The thing is
.get_temp is required field, while .get_trend is not. So, checking for
required fields in the registration makes more sense than checking it
only when the field is needed.

However, as I mentioned, to keep the same behavior, before and after the
patch, it makes sense we keep the checks as they are. I will send v4
with this amendment.


> > -
> > -	return data->get_temp(data->sensor_data, temp);
> > +	return data->ops->get_temp(data->sensor_data, temp);
> >  }
> >  
> >  static int of_thermal_get_trend(struct thermal_zone_device *tz, int
> > trip, @@ -120,10 +116,10 @@ static int of_thermal_get_trend(struct
> > thermal_zone_device *tz, int trip, long dev_trend;
> >  	int r;
> >  
> > -	if (!data->get_trend)
> > +	if (!data->ops->get_trend)
> >  		return -EINVAL;
> >  
> > -	r = data->get_trend(data->sensor_data, &dev_trend);
> > +	r = data->ops->get_trend(data->sensor_data, &dev_trend);
> >  	if (r)
> >  		return r;
> >  
> > @@ -324,8 +320,7 @@ static struct thermal_zone_device_ops
> > of_thermal_ops = { static struct thermal_zone_device *
> >  thermal_zone_of_add_sensor(struct device_node *zone,
> >  			   struct device_node *sensor, void *data,
> > -			   int (*get_temp)(void *, long *),
> > -			   int (*get_trend)(void *, long *))
> > +			   const struct thermal_zone_of_device_ops
> > *ops) {
> >  	struct thermal_zone_device *tzd;
> >  	struct __thermal_zone *tz;
> > @@ -336,9 +331,11 @@ thermal_zone_of_add_sensor(struct device_node
> > *zone, 
> >  	tz = tzd->devdata;
> >  
> > +	if (!(ops && ops->get_temp))
> > +		return ERR_PTR(-EINVAL);
> 
> IMHO, here we should only check:
> 	if (!ops)
> 		return ERR_PTR(-EINVAL);
> 
> 	And check if specific callbacks are available in other
> 	functions (like [1])
> 


OK. For the sake of this change only, I agree. However, I might be
sending patches on top of this one to keep the checks of required fields in the
registration itself.


Cheers,
> >  }
> 
> Despite this minor comments, feel free to add :-)
> 
> Reviewed-by: Lukasz Majewski <l.majewski@...sung.com>

OK. Thanks.

> 
> -- 
> Best regards,
> 
> Lukasz Majewski
> 
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group


Eduardo Valentin

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)