linux-kernel - Re: [PATCH v1 1/2] hwmon: (lm90): Add support for NCT7716, NCT7717 and NCT7718

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: <CAOoeyxXMrBvB=GTQUhTMETx-BLATTCwFR0wxmHxtsNm_qbgMuQ@mail.gmail.com>
Date: Wed, 15 Jan 2025 15:42:32 +0800
From: Ming Yu <a0282524688@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: tmyu0@...oton.com, jdelvare@...e.com, corbet@....net, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org, linux-hwmon@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org
Subject: Re: [PATCH v1 1/2] hwmon: (lm90): Add support for NCT7716, NCT7717
 and NCT7718

Dear Guenter,

Thank you for your comments,

Guenter Roeck <linux@...ck-us.net> 於 2025年1月10日 週五 下午11:56寫道：
>
> On 1/10/25 00:26, Ming Yu wrote:
> ...
> > @@ -2288,7 +2329,19 @@ static const char *lm90_detect_nuvoton(struct i2c_client *client, int chip_id,
> >       if (config2 < 0)
> >               return NULL;
> >
> > -     if (address == 0x4c && !(config1 & 0x2a) && !(config2 & 0xf8)) {
> > +     if (address == 0x48 && !(config1 & 0x30) && !(config2 & 0xfe) &&
>
> Why config1 & 0x30 (instead of 0x3e) ?
>

Fix it in the next patch.

> > +         convrate <= 0x08) {
> > +             if (chip_id == 0x90)
> > +                     name = "nct7717";
> > +             else if (chip_id == 0x91)
> > +                     name = "nct7716";
> > +     } else if (address == 0x49 && !(config1 & 0x30) && !(config2 & 0xfe) &&
> > +                convrate <= 0x08) {
> > +             name = "nct7716";
>
> Please also check the chip ID, and the other unused configuration register bits.
>

Fix it in the next patch.

> > +     } else if (address == 0x4c && !(config1 & 0x18) && !(config2 & 0xf8) &&
> > +                convrate <= 0x08) {
> > +             name = "nct7718";
>
> Please also check the chip ID (0x90 according to the datasheet). Why not check bit 5
> of config1 ?
>
> If there is a reason for not checking the reserved configuration register bits,
> please add a comment to the code explaining the reason.
>

Fix it in the next patch.

> > +     } else if (address == 0x4c && !(config1 & 0x2a) && !(config2 & 0xf8)) {
> >               if (chip_id == 0x01 && convrate <= 0x09) {
> >                       /* W83L771W/G */
> >                       name = "w83l771";
> > @@ -2297,6 +2350,7 @@ static const char *lm90_detect_nuvoton(struct i2c_client *client, int chip_id,
> >                       name = "w83l771";
> >               }
> >       }
> > +
> >       return name;
> >   }
> >
> > @@ -2484,6 +2538,10 @@ static int lm90_detect(struct i2c_client *client, struct i2c_board_info *info)
> >               name = lm90_detect_maxim(client, common_address, chip_id,
> >                                        config1, convrate);
> >               break;
> > +     case 0x50:      /* Nuvoton */
> > +     case 0x5c:      /* Winbond/Nuvoton */
>
> The new detection code should be implemented as separate function to avoid
> weakening the detection mechanism. I would suggest to rename the current
> lm90_detect_nuvoton() to lm90_detect_winbond() and introduce a new
> lm90_detect_nuvoton(). Alternatively, add something like lm90_detect_nuvoton_50().
>
> Given that all new chips have a chip ID register (called device ID), I would suggest
> to arrange the new code around the chip IDs. Since all chips have another chip ID
> register at address 0xfd, it would make sense to check that register as well.
> That would only require a single check since it looks like the value is the same
> for all chips. Something like
>
>         int chid = i2c_smbus_read_byte_data(client, 0xfd);
>         ...
>
>         if (chid < 0 || config2 < 0)
>                 return NULL;
>
>         if (chid != 0x50 || convrate > 0x08)
>                 return NULL;
>
>         switch (chip_id) {
>         case 0x90:
>                 ...
>         case 0x91:
>                 ...
>         default:
>                 ...
>         }
>
Okay, I will make these modifications in the next patch.


Best regards,
Ming