Re: [PATCH 1/1] hwmon: Driver for temperature sensors on SATA drives

[Linus Walleij]

Hi Guenther,

needless to say I am a big fan of this patch, so:
Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>

It's a nice addition with the SCT command, I never
figured that part out. Also nice how you register the
scsi class interface I never saw that before, it makes it
a very neat plug-in.

The comments are more discussion points on how to
(maybe) take it further after this.

On Mon, Dec 9, 2019 at 6:21 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:

> If the drive supports SCT transport and reports temperature limits,
> those are reported as well.

If I understand the patch correctly it will prefer to use
SCT transport to read the temperature, and only fall back
to the SMART attributes if this is not working, so I guess the
commit message should state the heuristics used here.

> +++ b/Documentation/hwmon/satatemp.rst

Excellent doc.

> + * If the SCT Command Transport feature set is not available, drive temperatures
> + * may be readable through SMART attributes. Since SMART attributes are not well
> + * defined, this method is only used as fallback mechanism.

So this maybe cut/paste to commit message as well so people understand
the commit fully.

> +       for (i = 0; i < ATA_MAX_SMART_ATTRS; i++) {
> +               u8 *attr = buf + i * 12;
> +               int id = attr[2];
> +
> +               if (!id)
> +                       continue;
> +
> +               if (id == SMART_TEMP_PROP_190) {
> +                       temp_raw = attr[7];
> +                       have_temp = true;
> +               }
> +               if (id == SMART_TEMP_PROP_194) {
> +                       temp_raw = attr[7];
> +                       have_temp = true;
> +                       break;
> +               }
> +       }
> +
> +       if (have_temp) {
> +               *temp = temp_raw * 1000;
> +               return 0;
> +       }

This looks like it will work fine, I had some heuristics to determine
the vendor-specific max/min temperatures in property 194 in my
patch, but I can certainly add that back in later.

> +static const struct hwmon_channel_info *satatemp_info[] = {
> +       HWMON_CHANNEL_INFO(chip,
> +                          HWMON_C_REGISTER_TZ),

I suppose this means I will also have a temperature zone as
I want :D

When I read the comments from the previous thread I got the
impression the SCSI people wanted me to use something like
the SCT transport and the hook in the SMART thing in the
libata back-end specifically for [S]ATA in response to the
SCT read log command.

In  drivers/ata/libata-scsi.c I suppose.

I guess one thing doesn't exclude the other though.

We can attempt to move the code for [S]ATA over to libata
at some point and respond to the SCT read log command
from within the library in that case.

I don't understand if that means the SCT read log also works
on some SCSI drives, or if it is just a slot-in thing for
ATA translation that has no meaning on SCSI drives.
But that can be resolved by people who want to use this
for SCSI drives and not by us.

Yours,
Linus Walleij