Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
This chip is used by various Intel Motherboards.
Is it still under review and testing :
http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
http://www.spinics.net/lists/lm-sensors/msg26915.html
http://www.spinics.net/lists/lm-sensors/msg26916.html
http://www.lm-sensors.org/wiki/Devices
Thank you,
--
JSR
Hi,
On 01/23/2010 06:52 PM, Jaswinder Singh Rajput wrote:
> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
> This chip is used by various Intel Motherboards.
>
> Is it still under review and testing :
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
>
> http://www.spinics.net/lists/lm-sensors/msg26915.html
>
> http://www.spinics.net/lists/lm-sensors/msg26916.html
>
> http://www.lm-sensors.org/wiki/Devices
>
I guess this is partly my fault, I started a review but never finished
it. One of the big problems is that George did many things completely
different to how every other single hwmon driver does things. Which made
the review harder then necessary, and more over made me wonder if we
should accept the driver in that incarnation at all.
Then a lot of things happened and I never got around to doing anything
with it at all.
I just checked my Drafts folder, and I still have my unfinished review
in there. So if there is interest I can send that, note that it is
not a complete review though (there is a note in there which part
of the code is reviewed and which still needs to be reviewed).
Regards,
Hans
Hello Hans,
On Sat, Jan 23, 2010 at 11:28 PM, Hans de Goede <[email protected]> wrote:
> Hi,
>
> On 01/23/2010 06:52 PM, Jaswinder Singh Rajput wrote:
>>
>> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
>> This chip is used by various Intel Motherboards.
>>
>> Is it still under review and testing :
>>
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
>>
>> http://www.spinics.net/lists/lm-sensors/msg26915.html
>>
>> http://www.spinics.net/lists/lm-sensors/msg26916.html
>>
>> http://www.lm-sensors.org/wiki/Devices
>>
>
> I guess this is partly my fault, I started a review but never finished
> it. One of the big problems is that George did many things completely
> different to how every other single hwmon driver does things. Which made
> the review harder then necessary, and more over made me wonder if we
> should accept the driver in that incarnation at all.
>
George Joseph submitted the driver on 2008-05-29 and then he updated
the driver on 2009-10-15 for 2.6.30 and many developers are able to
test it and used it.
Are you able to compile the driver with latest kernel git.
> Then a lot of things happened and I never got around to doing anything
> with it at all.
>
Many new Intel Motherboards are coming with this chip, if we complete
this driver then it will be great.
> I just checked my Drafts folder, and I still have my unfinished review
> in there. So if there is interest I can send that, note that it is
> not a complete review though (there is a note in there which part
> of the code is reviewed and which still needs to be reviewed).
>
Please provide your review so that we can discuss about this driver
and make relevant changes to accept it.
Thank you,
--
Jaswinder Singh.
On 24/01/2010, Jaswinder Singh Rajput <[email protected]> wrote:
> Many new Intel Motherboards are coming with this chip, if we complete
> this driver then it will be great.
Really? My understanding was that the company, Andigilog, no longer
seem to exist since at least a couple of months ago (I don't recall
the exact dates their web-site was taken down without any notice, but
currently the domain has no information regarding semiconductors
whatsoever [0]). It would therefore seem somewhat strange if Intel
was still using these chips in their recent designs ? do you have some
concrete examples? Anyone has any more details regarding Andigilog
and their products?
C.
[0] http://translate.google.com/translate?hl=en&sl=ja&tl=en&u=http%3A%2F%2Fwww.andigilog.com%2F
Hello,
On Sun, Jan 24, 2010 at 11:14 AM, Constantine A. Murenin
<[email protected]> wrote:
> On 24/01/2010, Jaswinder Singh Rajput <[email protected]> wrote:
>> Many new Intel Motherboards are coming with this chip, if we complete
>> ?this driver then it will be great.
>
> Really?
Yes, http://www.intel.com/products/server/motherboard/index.htm?iid=mbd_body+sv_all
I have checked with Intel? Workstation Board WX58BP :
Now follows a summary of the probes I have just done.
Just press ENTER to continue:
Driver `to-be-written':
* Bus `SMBus I801 adapter at 2000'
Busdriver `i2c_i801', I2C address 0x2e
Chip `Andigilog aSC7621' (confidence: 5)
Driver `coretemp':
* Chip `Intel Core family thermal sensor' (confidence: 9)
Note: there is no driver for Andigilog aSC7621 yet.
Check http://www.lm-sensors.org/wiki/Devices for updates.
Do you want to overwrite /etc/sysconfig/lm_sensors? (YES/no): YES
Starting lm_sensors: loading module coretemp [ OK ]
Unloading i2c-dev... OK
You can also check with other intel motherboards.
> My understanding was that the company, Andigilog, no longer
> seem to exist since at least a couple of months ago (I don't recall
> the exact dates their web-site was taken down without any notice, but
> currently the domain has no information regarding semiconductors
> whatsoever [0]). ?It would therefore seem somewhat strange if Intel
> was still using these chips in their recent designs ? do you have some
> concrete examples? ?Anyone has any more details regarding Andigilog
> and their products?
>
http://who.is/whois/andigilog.com/
These motherboards are build in Taiwan, China. I have no idea that how
they are getting these chips but you can see them in your latest Intel
motherboards ;-)
Thanks,
--
Jaswinder Singh.
2010/1/24 Jaswinder Singh Rajput <[email protected]>:
> Hello,
>
> On Sun, Jan 24, 2010 at 11:14 AM, Constantine A. Murenin
> <[email protected]> wrote:
>> On 24/01/2010, Jaswinder Singh Rajput <[email protected]> wrote:
>>> Many new Intel Motherboards are coming with this chip, if we complete
>>> ?this driver then it will be great.
>>
>> Really?
>
> Yes, http://www.intel.com/products/server/motherboard/index.htm?iid=mbd_body+sv_all
>
>
> I have checked with Intel? Workstation Board WX58BP :
http://www.intel.com/support/motherboards/server/wx58bp/sb/CS-030316.htm
?
Spares, Parts List and Configuration Guide [PDF]
File Name: WX58BP Config Guide 1.0.pdf
Size: 76,709 bytes
Date: April 2009
File Revision: 1.0
?
The board seems old, or, at least, old enough to be designed when
Andigilog still existed. :-)
> Now follows a summary of the probes I have just done.
> Just press ENTER to continue:
>
> Driver `to-be-written':
> ?* Bus `SMBus I801 adapter at 2000'
> ? ?Busdriver `i2c_i801', I2C address 0x2e
> ? ?Chip `Andigilog aSC7621' (confidence: 5)
>
> Driver `coretemp':
> ?* Chip `Intel Core family thermal sensor' (confidence: 9)
>
> Note: there is no driver for Andigilog aSC7621 yet.
> Check http://www.lm-sensors.org/wiki/Devices for updates.
>
> Do you want to overwrite /etc/sysconfig/lm_sensors? (YES/no): YES
> Starting lm_sensors: loading module coretemp ? ? ? ? ? ? ? [ ?OK ?]
> Unloading i2c-dev... OK
>
> You can also check with other intel motherboards.
>
>> My understanding was that the company, Andigilog, no longer
>> seem to exist since at least a couple of months ago (I don't recall
>> the exact dates their web-site was taken down without any notice, but
>> currently the domain has no information regarding semiconductors
>> whatsoever [0]). ?It would therefore seem somewhat strange if Intel
>> was still using these chips in their recent designs ? do you have some
>> concrete examples? ?Anyone has any more details regarding Andigilog
>> and their products?
>>
>
> http://who.is/whois/andigilog.com/
>
> These motherboards are build in Taiwan, China. I have no idea that how
> they are getting these chips but you can see them in your latest Intel
> motherboards ;-)
Well, a better link would have been:
http://web.archive.org/web/*/http://www.andigilog.com/
:-)
C.
On Sat, 23 Jan 2010 23:22:29 +0530, Jaswinder Singh Rajput wrote:
> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
> This chip is used by various Intel Motherboards.
>
> Is it still under review and testing :
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
>
> http://www.spinics.net/lists/lm-sensors/msg26915.html
>
> http://www.spinics.net/lists/lm-sensors/msg26916.html
>
> http://www.lm-sensors.org/wiki/Devices
I've updated the wiki entry to point to the latest version of the
driver. I've also included a complete list of known requests so far,
which can be used to get in touch with potential testers.
--
Jean Delvare
Hello Jean,
On Sun, Jan 24, 2010 at 3:00 PM, Jean Delvare <[email protected]> wrote:
> On Sat, 23 Jan 2010 23:22:29 +0530, Jaswinder Singh Rajput wrote:
>> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
>> This chip is used by various Intel Motherboards.
>>
>> Is it still under review and testing :
>>
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
>>
>> http://www.spinics.net/lists/lm-sensors/msg26915.html
>>
>> http://www.spinics.net/lists/lm-sensors/msg26916.html
>>
>> http://www.lm-sensors.org/wiki/Devices
>
> I've updated the wiki entry to point to the latest version of the
> driver. I've also included a complete list of known requests so far,
> which can be used to get in touch with potential testers.
>
Thanks for updating the wiki.
Latest asc7621 driver is giving compilation errors with latest kernel
git. Do you also also have plans to update the driver so that we can
use the asc7621 driver.
I am ready for testing and debugging it.
Thank you,
--
Jaswinder Singh.
On Sun, 24 Jan 2010 17:44:11 +0530, Jaswinder Singh Rajput wrote:
> Hello Jean,
>
> On Sun, Jan 24, 2010 at 3:00 PM, Jean Delvare <[email protected]> wrote:
> > On Sat, 23 Jan 2010 23:22:29 +0530, Jaswinder Singh Rajput wrote:
> >> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
> >> This chip is used by various Intel Motherboards.
> >>
> >> Is it still under review and testing :
> >>
> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
> >>
> >> http://www.spinics.net/lists/lm-sensors/msg26915.html
> >>
> >> http://www.spinics.net/lists/lm-sensors/msg26916.html
> >>
> >> http://www.lm-sensors.org/wiki/Devices
> >
> > I've updated the wiki entry to point to the latest version of the
> > driver. I've also included a complete list of known requests so far,
> > which can be used to get in touch with potential testers.
> >
>
> Thanks for updating the wiki.
>
> Latest asc7621 driver is giving compilation errors with latest kernel
> git. Do you also also have plans to update the driver so that we can
> use the asc7621 driver.
>
> I am ready for testing and debugging it.
No, I don't have any plan. I already have too much on my plate, adding
anything would be unreasonable.
The only way I would start working on this is if someone would offer me
one of these motherboards which use the chip in question.
--
Jean Delvare
On 01/24/2010 06:05 AM, Jaswinder Singh Rajput wrote:
> Hello Hans,
>
> On Sat, Jan 23, 2010 at 11:28 PM, Hans de Goede<[email protected]> wrote:
>> Hi,
>>
>> I just checked my Drafts folder, and I still have my unfinished review
>> in there. So if there is interest I can send that, note that it is
>> not a complete review though (there is a note in there which part
>> of the code is reviewed and which still needs to be reviewed).
>>
>
> Please provide your review so that we can discuss about this driver
> and make relevant changes to accept it.
>
Ok, here is the partly review I've done:
---begin---
Hi Joseph,
[email protected] wrote:
> Here's the c file by itself.
>
> Hans, will you have any time to review the driver in the near future?
>
I believe I said I would make time for this some time ago. Given that the 2.6.29 merge window has already opened, I've now done so (made time). So that this hopefully / maybe can make 2.6.29's merge window.
I must say that this driver deviates a lot from the standard way all other hwmon drivers are written making the review somewhat cumbersome and this might also be a problem if you step down and someone else becomes the maintainer.
I've split me review in 2 parts, first some comments about how you've implemented the sysfs API:
I notice that you have added foo_label sysfs entries, while you have nothing usefull to put in them, please do not _label entries are only for when the driver knows / can provide a label in the prefered format for a human reader of the sensors output, so not "in1", but something like "ATX 12V", so please remove all _label sysfs entries and remove the corresponding "char *label" member from the asc7621_param struct.
Likewise you've added tempX_type entries, but these always return the same value, in this case they can (and should be omitted) there is only the need to know (and more importantly be able to set) the type of the sensor, if it can be of different types, as in this case the BIOS might have set it wrong, when you remove these sysfs entries, you can also remove the corresponding "value" member from the asc7621_param struct.
And second, some feedback on the code itself.
First of all in *ALL* your store functions you need to store the result of strtol in a long and strtoul in an unsigned long, storing in smaller types can cause an overflow before you do any checking / clamping when the user gives a really large value as input.
Also in *ALL* store functions please use strict_strtol (and strtoul) instead of simple.
Last in some functions you need to clamp the input from the user to the valid input range before doing further calculations to avoid overflows during the calculation. Often clamping after calculations can be too late.
Example:
Instead of:
static ssize_t store_u8(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
SETUP_STORE_data_param(dev, attr);
u8 reqval = simple_strtoul(buf, NULL, 10);
mutex_lock(&data->update_lock);
data->reg[param->msb[0]] = reqval;
write_byte(param->msb[0], reqval);
mutex_unlock(&data->update_lock);
return count;
}
Write:
static ssize_t store_u8(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
SETUP_STORE_data_param(dev, attr);
long reqval;
if (strict_strtoul(buf, 10, &reqval))
return -EINVAL;
reqval = SENSORS_LIMIT(v, 0, 255);
mutex_lock(&data->update_lock);
data->reg[param->msb[0]] = reqval;
write_byte(param->msb[0], reqval);
mutex_unlock(&data->update_lock);
return count;
}
Note that in this example all 3 described changes were made (use long, use strict_strtol, clamp before further use). *ALL* your store functions need the first 2 changes (use long, use strict_strtol), so I'm not going to make that comment for each and every store functions in the detailed comments below.
Where a store function needs earlier / different clamping I will make a comment in the detailed feedback given below.
Below are pieces of code from the driver with detailed feedback below them.
###
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; version 2 of the License.
*
I notice there is no "or at your option any later version" language here, this
is fine, but I wonder if this is on purpose, if not please add "or at your
option any later version", so that if the kernel ever (not likely) wants to
move to GPL v3 this is one less file to worry about.
###
/*
* The "chips" enum created by I2C_CLIENT_INSMOD_2 has "any_chip" as
* the first element in the enum, so it must also be first in our array.
*/
static struct asc7621_chip asc7621_chips[] = {
{
.name = "any",.chip_type = any_chip,
},
{
.name = "asc7621",.chip_type = asc7621,
.company_reg = 0x3e,.company_id = 0x61,
.verstep_reg = 0x3f,.verstep_id = 0x6c,
.addresses = normal_i2c,
},
{
.name = "asc7621a",.chip_type = asc7621a,
.company_reg = 0x3e,.company_id = 0x61,
.verstep_reg = 0x3f,.verstep_id = 0x6d,
.addresses = normal_i2c,
},
};
Your driver still uses old style i2c driver binding, please update it too the
new style. See for example:
http://lists.lm-sensors.org/pipermail/lm-sensors/2008-July/023773.html
You will probably want to remove using this when you do this, as this overlaps
with the i2c_device_id array you need to declare then.
###
static ssize_t store_u8(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
SETUP_STORE_data_param(dev, attr);
u8 reqval = simple_strtoul(buf, NULL, 10);
mutex_lock(&data->update_lock);
data->reg[param->msb[0]] = reqval;
write_byte(param->msb[0], reqval);
mutex_unlock(&data->update_lock);
return count;
}
Clamp large input values instead of allowing large values to cause overflows, see above for how this function should look (IMHO)
###
static ssize_t store_bitmask(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
SETUP_STORE_data_param(dev, attr);
u8 reqval = simple_strtoul(buf, NULL, 10);
Same as for store_u8, except the values between which to clamp depend on the mask.
###
static ssize_t show_fan16(struct device *dev,
struct device_attribute *attr, char *buf)
{
SETUP_SHOW_data_param(dev, attr);
u16 regval = (data->reg[param->msb[0]] << 8) | data->reg[param->lsb[0]]
You are using multiple values from data here, this might race with device update resulting in using an old msb with a new lsb or vica versa, so you need locking around this.
###
static ssize_t show_in10(struct device *dev, struct device_attribute *attr,
char *buf)
{
You are using multiple values from data here, you need locking around this.
###
static ssize_t store_in8(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
u8 nr;
s32 reqval;
SETUP_STORE_data_param(dev, attr);
nr = sda->index;
reqval = simple_strtoul(buf, NULL, 10);
reqval *= asc7621_in_scaling_div[nr];
reqval /= asc7621_in_scaling_mul[nr];
reqval = SENSORS_LIMIT(reqval, 0, 255);
do SENSORS_LIMIT before calculations, the multiple might cause an overflow
otherwise (and ofcourse, reqval should be a long, use strict_strtol).
###
static ssize_t show_temp10(struct device *dev,
struct device_attribute *attr, char *buf)
{
You are using multiple values from data here, you need locking around this.
###
static ssize_t show_ap2_temp(struct device *dev,
struct device_attribute *attr, char *buf)
{
You are using multiple values from data here, you need locking around this.
###
static ssize_t store_ap2_temp(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
you need to take the lock earlier, before reading auto_point1 from the cached
register array (and ofcourse, reqval should be a long, use strict_strtol).
###
static ssize_t show_pwm_enable(struct device *dev,
struct device_attribute *attr, char *buf)
{
You are using multiple values from data here, you need locking around this.
###
In store store_pwm_freq() / store_pwm_ast() / store_temp_st() the following:
if (newval == 255)
return count;
Should return -EINVAL, as you are rejecting the value (not making any changes)
###
Note to self all store / show functions are reviewed
---end---
Regards,
Hans