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

[Dan Carpenter <dan.carpenter@oracle.com>]

We never did fix this bug in the end...  I'm not really the right person
to fix this.

regards,
dan carpenter

On Wed, Apr 21, 2021 at 06:04:45PM +0200, Karol Herbst wrote:
> On Wed, Apr 21, 2021 at 3:57 PM Dan Carpenter <dan.carpenter@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