Jean Delvare wrote:
> Hi Hans,
>
> On Tue, 21 Oct 2008 16:24:43 +0200, Hans de Goede wrote:
>> The f71882fg driver did some io to ioports it hadn't reserved yet in its
>> find (detect) function, this patches moves this io to the probe function
>> where these ports are reserved and this io belongs.
>>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>
> Review:
>
<snip>
>> @@ -1507,12 +1507,31 @@
>> mutex_init(&data->update_lock);
>> platform_set_drvdata(pdev, data);
>>
>> + start_reg = f71882fg_read8(data, F71882FG_REG_START);
>> + if (!(start_reg & 0x03)) {
>> + printk(KERN_WARNING DRVNAME
>> + ": Hardware monitoring not activated\n");
>
> Please use dev_info() instead.
>
Done.
>> + goto exit_free;
>
> You're returning with err = 0, which means success. So when you unload
> the driver, the f71882fg_remove() function will be called and you'll
> double-free your private data structure. No good.
>
> I think you should simply return -ENODEV here.
>
Done.
>> + }
>> +
>> + /* If it is a 71862 and the fan / pwm part is enabled sanity check
>> + the pwm settings */
>> + if (data->type == f71862fg && (start_reg & 0x02)) {
>> + u8 reg = f71882fg_read8(data, F71882FG_REG_PWM_ENABLE);
>> + if ((reg & 0x15) != 0x15) {
>> + printk(KERN_ERR DRVNAME
>> + ": Invalid (reserved) pwm settings: 0x%02x\n",
>> + (unsigned int)reg);
>
> Here again please use dev_err().
>
Done.
>> + err = -EIO;
>
> This isn't actually an I/O error, I think I'd return -ENODEV as well.
Done.
> Or, ideally, you could simply disable the pwm features of the driver
> but still let it bind. After all, voltage, temperature and fan features
> are still useful, aren't they?
>
This is a should never happen case, those bits I'm testing are reserved and
should always be 1, the reason I'm testing them is because the 71862 only has
PWM auto fan control, where as the 71882 has both PWM and RPM modes, the code
paths used by both check these bits to see if the chip is in RPM or PWM mode,
so if these *reserved* bits have the wrong value, the RPM mode code path can be
entered on the 71862.
Even more troublesome is that as the 71862 clearly is a stripped down 71882 the
chip itself may behave funny if programmed to be in the removed RPM modes. So I
rather just bail completely when this register shows the should never happen
readings. If this message gets triggered ever (and reported) we can revisit this.
I've attached an updated version of the patch.
Regards,
Hans
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: f71882fg-no-io-to-unreserverd-ports-v2.patch
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20081021/ff8162dc/attachment.pl
