* [Nouveau] [bug report] drm/nouveau/hwmon: Remove old code, add .write/.read operations
@ 2021-04-21 13:24 Dan Carpenter
2021-04-21 16:04 ` Karol Herbst
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-04-21 13:24 UTC (permalink / raw)
To: osalvador.vilardaga; +Cc: nouveau
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.
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Nouveau] [bug report] drm/nouveau/hwmon: Remove old code, add .write/.read operations
2021-04-21 13:24 [Nouveau] [bug report] drm/nouveau/hwmon: Remove old code, add .write/.read operations Dan Carpenter
@ 2021-04-21 16:04 ` Karol Herbst
2021-04-28 7:59 ` Dan Carpenter
2022-06-01 9:08 ` Dan Carpenter
0 siblings, 2 replies; 4+ messages in thread
From: Karol Herbst @ 2021-04-21 16:04 UTC (permalink / raw)
To: Dan Carpenter; +Cc: nouveau, osalvador.vilardaga
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
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Nouveau] [bug report] drm/nouveau/hwmon: Remove old code, add .write/.read operations
2021-04-21 16:04 ` Karol Herbst
@ 2021-04-28 7:59 ` Dan Carpenter
2022-06-01 9:08 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2021-04-28 7:59 UTC (permalink / raw)
To: Karol Herbst; +Cc: nouveau, osalvador.vilardaga
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?
Sorry the delayed response. I'm still not sure if this should return
-ENODEV or not. Could you please fix it and give me a reported-by tag?
regards,
dan carpenter
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Nouveau] [bug report] drm/nouveau/hwmon: Remove old code, add .write/.read operations
2021-04-21 16:04 ` Karol Herbst
2021-04-28 7:59 ` Dan Carpenter
@ 2022-06-01 9:08 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2022-06-01 9:08 UTC (permalink / raw)
To: Karol Herbst; +Cc: nouveau, osalvador.vilardaga
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-06-01 9:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-21 13:24 [Nouveau] [bug report] drm/nouveau/hwmon: Remove old code, add .write/.read operations Dan Carpenter
2021-04-21 16:04 ` Karol Herbst
2021-04-28 7:59 ` Dan Carpenter
2022-06-01 9:08 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.