[Nouveau] [bug report] drm/nouveau/hwmon: Remove old code, add .write/.read operations

[Karol Herbst]

On Wed, Apr 21, 2021 at 3:57 PM Dan Carpenter <dan.carpenter at oracle.com> wrote:
>
> Hello Oscar Salvador,
>
> The patch bfb96e4c344e: "drm/nouveau/hwmon: Remove old code, add
> .write/.read operations" from May 18, 2017, leads to the following
> static checker warning:
>
>     drivers/gpu/drm/nouveau/nouveau_hwmon.c:507 nouveau_in_read()
>     warn: check sign expansion for '-19'
>     drivers/gpu/drm/nouveau/nouveau_hwmon.c:510 nouveau_in_read()
>     warn: check sign expansion for '-19'
>
> drivers/gpu/drm/nouveau/nouveau_hwmon.c
>    488  static int
>    489  nouveau_in_read(struct device *dev, u32 attr, int channel, long *val)
>    490  {
>    491          struct drm_device *drm_dev = dev_get_drvdata(dev);
>    492          struct nouveau_drm *drm = nouveau_drm(drm_dev);
>    493          struct nvkm_volt *volt = nvxx_volt(&drm->client.device);
>    494          int ret;
>    495
>    496          if (!volt)
>    497                  return -EOPNOTSUPP;
>    498
>    499          switch (attr) {
>    500          case hwmon_in_input:
>    501                  if (drm_dev->switch_power_state != DRM_SWITCH_POWER_ON)
>    502                          return -EINVAL;
>    503                  ret = nvkm_volt_get(volt);
>    504                  *val = ret < 0 ? ret : (ret / 1000);
>    505                  break;
>    506          case hwmon_in_min:
>    507                  *val = volt->min_uv > 0 ? (volt->min_uv / 1000) : -ENODEV;
>
> This is trying to set "*val = -ENODEV" but because "volt->min_uv" is
> unsigned int it actually sets it to "*val = (unsigned int)-ENODEV".
>
> It's weird to me that this code doesn't return -ENODEV instead of
> setting *val to it.
>

ohh.. that might actually be a left over from the conversion we've
done in the past. Thanks for pointing it out. Do you want to write the
patch as well?

>    508                  break;
>    509          case hwmon_in_max:
>    510                  *val = volt->max_uv > 0 ? (volt->max_uv / 1000) : -ENODEV;
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>    511                  break;
>    512          default:
>    513                  return -EOPNOTSUPP;
>    514          }
>    515
>    516          return 0;
>    517  }
>
> regards,
> dan carpenter
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
>