Re: [PATCH 08/11] nouveau/gsp: don't free ctrl messages on errors
On Thu, Jan 04, 2024 at 10:41:50AM +1000, Dave Airlie wrote: > On Thu, 4 Jan 2024 at 00:47, Dan Carpenter wrote: > > > > Hi Dave, > > > > kernel test robot noticed the following build warnings: > > > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > > > url: > > https://github.com/intel-lab-lkp/linux/commits/Dave-Airlie/nouveau-gsp-drop-some-acpi-related-debug/20231222-180432 > > base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next > > patch link: > > https://lore.kernel.org/r/20231222043308.3090089-9-airlied%40gmail.com > > patch subject: [PATCH 08/11] nouveau/gsp: don't free ctrl messages on errors > > config: powerpc-randconfig-r071-20231226 > > (https://download.01.org/0day-ci/archive/20231227/202312271917.55xudmdc-...@intel.com/config) > > compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project > > d3ef86708241a3bee902615c190dead1638c4e09) > > > > If you fix the issue in a separate patch/commit (i.e. not just a new > > version of > > the same patch/commit), kindly add following tags > > | Reported-by: kernel test robot > > | Reported-by: Dan Carpenter > > | Closes: https://lore.kernel.org/r/202312271917.55xudmdc-...@intel.com/ > > This is a false positive, I think the code is operating like I'd > expect, we maybe could restructure it to avoid this warning? > > The idea is you send an rpc msg, if there's a reply you get a reply, > if no reply you get NULL and if an error you get an error. > > So in the case you get an error or NULL you just want to return 0 for > the NULL as it's successful, and error otherwise. > > Would using PTR_ERR_OR_ZERO make smatch happy? (even if it's not > really what we want). Hm... You're using the API correctly. Linus has complained about this warning before but in new code over 90% of the warnings are correct. It's a high quality warning. I looked around for an explanation to see what the NULL meant but couldn't find it documented in the code. The NULL vs error pointer comes from a function pointer and it's not always clear where the documentation should be with a function pointer. So perhaps I missed it. Let's not use PTR_ERR_OR_ZERO. Perhaps I should introduce a PTR_ERR_OR_NULL() macro to silence this warning. But most of the code which does this correctly is in fs/ and they probably are like Linus and would be surprised to learn that people get it wrong... regards, dan carpenter
Re: [PATCH 08/11] nouveau/gsp: don't free ctrl messages on errors
On Thu, 4 Jan 2024 at 00:47, Dan Carpenter wrote:
>
> Hi Dave,
>
> kernel test robot noticed the following build warnings:
>
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:
> https://github.com/intel-lab-lkp/linux/commits/Dave-Airlie/nouveau-gsp-drop-some-acpi-related-debug/20231222-180432
> base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
> patch link:
> https://lore.kernel.org/r/20231222043308.3090089-9-airlied%40gmail.com
> patch subject: [PATCH 08/11] nouveau/gsp: don't free ctrl messages on errors
> config: powerpc-randconfig-r071-20231226
> (https://download.01.org/0day-ci/archive/20231227/202312271917.55xudmdc-...@intel.com/config)
> compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project
> d3ef86708241a3bee902615c190dead1638c4e09)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version
> of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot
> | Reported-by: Dan Carpenter
> | Closes: https://lore.kernel.org/r/202312271917.55xudmdc-...@intel.com/
This is a false positive, I think the code is operating like I'd
expect, we maybe could restructure it to avoid this warning?
The idea is you send an rpc msg, if there's a reply you get a reply,
if no reply you get NULL and if an error you get an error.
So in the case you get an error or NULL you just want to return 0 for
the NULL as it's successful, and error otherwise.
Would using PTR_ERR_OR_ZERO make smatch happy? (even if it's not
really what we want).
Dave.
>
> New smatch warnings:
> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:659
> r535_gsp_rpc_rm_ctrl_push() warn: passing zero to 'PTR_ERR'
> drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c:1063 r535_dp_aux_xfer() warn:
> passing a valid pointer to 'PTR_ERR'
>
> Old smatch warnings:
> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1887 nvkm_gsp_radix3_sg()
> error: uninitialized symbol 'addr'.
>
> vim +/PTR_ERR +659 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
>
> af265ee961627a Dave Airlie 2023-12-22 649 static int
> af265ee961627a Dave Airlie 2023-12-22 650 r535_gsp_rpc_rm_ctrl_push(struct
> nvkm_gsp_object *object, void **argv, u32 repc)
> 4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 651 {
> af265ee961627a Dave Airlie 2023-12-22 652 rpc_gsp_rm_control_v03_00
> *rpc = container_of((*argv), typeof(*rpc), params);
> 4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 653 struct nvkm_gsp *gsp =
> object->client->gsp;
> af265ee961627a Dave Airlie 2023-12-22 654 int ret = 0;
> 4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 655
> 4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 656 rpc = nvkm_gsp_rpc_push(gsp,
> rpc, true, repc);
> af265ee961627a Dave Airlie 2023-12-22 657 if (IS_ERR_OR_NULL(rpc)) {
> af265ee961627a Dave Airlie 2023-12-22 658 *argv = NULL;
> af265ee961627a Dave Airlie 2023-12-22 @659 return PTR_ERR(rpc);
>
> If nvkm_gsp_rpc_push() returns NULL (probably a failure) then this
> returns PTR_ERR(NULL) which is zero/success.
>
> af265ee961627a Dave Airlie 2023-12-22 660 }
> 4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 661
> 4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 662 if (rpc->status) {
> af265ee961627a Dave Airlie 2023-12-22 663 ret =
> r535_rpc_status_to_errno(rpc->status);
> 555bb9c29a45be Dave Airlie 2023-12-22 664 if (ret != -EAGAIN)
> 4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 665
> nvkm_error(>subdev, "cli:0x%08x obj:0x%08x ctrl cmd:0x%08x failed:
> 0x%08x\n",
> 4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 666
> object->client->object.handle, object->handle, rpc->cmd, rpc->status);
> 4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 667 }
> 4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 668
> af265ee961627a Dave Airlie 2023-12-22 669 if (repc)
> af265ee961627a Dave Airlie 2023-12-22 670 *argv = rpc->params;
> af265ee961627a Dave Airlie 2023-12-22 671 else
> 4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 672
> nvkm_gsp_rpc_done(gsp, rpc);
> 4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 673
> 4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 674 return ret;
> 4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 675 }
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
Re: [PATCH 08/11] nouveau/gsp: don't free ctrl messages on errors
Hi Dave,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Dave-Airlie/nouveau-gsp-drop-some-acpi-related-debug/20231222-180432
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:
https://lore.kernel.org/r/20231222043308.3090089-9-airlied%40gmail.com
patch subject: [PATCH 08/11] nouveau/gsp: don't free ctrl messages on errors
config: powerpc-randconfig-r071-20231226
(https://download.01.org/0day-ci/archive/20231227/202312271917.55xudmdc-...@intel.com/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project
d3ef86708241a3bee902615c190dead1638c4e09)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Closes: https://lore.kernel.org/r/202312271917.55xudmdc-...@intel.com/
New smatch warnings:
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:659 r535_gsp_rpc_rm_ctrl_push()
warn: passing zero to 'PTR_ERR'
drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c:1063 r535_dp_aux_xfer() warn:
passing a valid pointer to 'PTR_ERR'
Old smatch warnings:
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1887 nvkm_gsp_radix3_sg() error:
uninitialized symbol 'addr'.
vim +/PTR_ERR +659 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
af265ee961627a Dave Airlie 2023-12-22 649 static int
af265ee961627a Dave Airlie 2023-12-22 650 r535_gsp_rpc_rm_ctrl_push(struct
nvkm_gsp_object *object, void **argv, u32 repc)
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 651 {
af265ee961627a Dave Airlie 2023-12-22 652 rpc_gsp_rm_control_v03_00 *rpc
= container_of((*argv), typeof(*rpc), params);
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 653 struct nvkm_gsp *gsp =
object->client->gsp;
af265ee961627a Dave Airlie 2023-12-22 654 int ret = 0;
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 655
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 656 rpc = nvkm_gsp_rpc_push(gsp,
rpc, true, repc);
af265ee961627a Dave Airlie 2023-12-22 657 if (IS_ERR_OR_NULL(rpc)) {
af265ee961627a Dave Airlie 2023-12-22 658 *argv = NULL;
af265ee961627a Dave Airlie 2023-12-22 @659 return PTR_ERR(rpc);
If nvkm_gsp_rpc_push() returns NULL (probably a failure) then this
returns PTR_ERR(NULL) which is zero/success.
af265ee961627a Dave Airlie 2023-12-22 660 }
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 661
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 662 if (rpc->status) {
af265ee961627a Dave Airlie 2023-12-22 663 ret =
r535_rpc_status_to_errno(rpc->status);
555bb9c29a45be Dave Airlie 2023-12-22 664 if (ret != -EAGAIN)
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 665
nvkm_error(>subdev, "cli:0x%08x obj:0x%08x ctrl cmd:0x%08x failed:
0x%08x\n",
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 666
object->client->object.handle, object->handle, rpc->cmd, rpc->status);
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 667 }
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 668
af265ee961627a Dave Airlie 2023-12-22 669 if (repc)
af265ee961627a Dave Airlie 2023-12-22 670 *argv = rpc->params;
af265ee961627a Dave Airlie 2023-12-22 671 else
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 672 nvkm_gsp_rpc_done(gsp,
rpc);
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 673
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 674 return ret;
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 675 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
[PATCH 08/11] nouveau/gsp: don't free ctrl messages on errors
It looks like for some messages the upper layers need to get access to the
results of the message so we can interpret it.
Rework the ctrl push interface to not free things and cleanup properly
whereever it errors out.
Requested-by: Lyude
Signed-off-by: Dave Airlie
---
.../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 17 +--
.../gpu/drm/nouveau/nvkm/engine/disp/r535.c | 108 +++---
.../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c| 36 +++---
3 files changed, 100 insertions(+), 61 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
index 2fa0445d8928..d1437c08645f 100644
--- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
+++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
@@ -187,7 +187,7 @@ struct nvkm_gsp {
void (*rpc_done)(struct nvkm_gsp *gsp, void *repv);
void *(*rm_ctrl_get)(struct nvkm_gsp_object *, u32 cmd, u32
argc);
- void *(*rm_ctrl_push)(struct nvkm_gsp_object *, void *argv, u32
repc);
+ int (*rm_ctrl_push)(struct nvkm_gsp_object *, void **argv, u32
repc);
void (*rm_ctrl_done)(struct nvkm_gsp_object *, void *repv);
void *(*rm_alloc_get)(struct nvkm_gsp_object *, u32 oclass, u32
argc);
@@ -265,7 +265,7 @@ nvkm_gsp_rm_ctrl_get(struct nvkm_gsp_object *object, u32
cmd, u32 argc)
return object->client->gsp->rm->rm_ctrl_get(object, cmd, argc);
}
-static inline void *
+static inline int
nvkm_gsp_rm_ctrl_push(struct nvkm_gsp_object *object, void *argv, u32 repc)
{
return object->client->gsp->rm->rm_ctrl_push(object, argv, repc);
@@ -275,21 +275,24 @@ static inline void *
nvkm_gsp_rm_ctrl_rd(struct nvkm_gsp_object *object, u32 cmd, u32 repc)
{
void *argv = nvkm_gsp_rm_ctrl_get(object, cmd, repc);
+ int ret;
if (IS_ERR(argv))
return argv;
- return nvkm_gsp_rm_ctrl_push(object, argv, repc);
+ ret = nvkm_gsp_rm_ctrl_push(object, , repc);
+ if (ret)
+ return ERR_PTR(ret);
+ return argv;
}
static inline int
nvkm_gsp_rm_ctrl_wr(struct nvkm_gsp_object *object, void *argv)
{
- void *repv = nvkm_gsp_rm_ctrl_push(object, argv, 0);
-
- if (IS_ERR(repv))
- return PTR_ERR(repv);
+ int ret = nvkm_gsp_rm_ctrl_push(object, , 0);
+ if (ret)
+ return ret;
return 0;
}
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c
b/drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c
index 1c8c4cca0957..1b4f988df7ed 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c
@@ -282,7 +282,7 @@ r535_sor_bl_get(struct nvkm_ior *sor)
{
struct nvkm_disp *disp = sor->disp;
NV0073_CTRL_SPECIFIC_BACKLIGHT_BRIGHTNESS_PARAMS *ctrl;
- int lvl;
+ int ret, lvl;
ctrl = nvkm_gsp_rm_ctrl_get(>rm.objcom,
NV0073_CTRL_CMD_SPECIFIC_GET_BACKLIGHT_BRIGHTNESS,
@@ -292,9 +292,11 @@ r535_sor_bl_get(struct nvkm_ior *sor)
ctrl->displayId = BIT(sor->asy.outp->index);
- ctrl = nvkm_gsp_rm_ctrl_push(>rm.objcom, ctrl, sizeof(*ctrl));
- if (IS_ERR(ctrl))
- return PTR_ERR(ctrl);
+ ret = nvkm_gsp_rm_ctrl_push(>rm.objcom, , sizeof(*ctrl));
+ if (ret) {
+ nvkm_gsp_rm_ctrl_done(>rm.objcom, ctrl);
+ return ret;
+ }
lvl = ctrl->brightness;
nvkm_gsp_rm_ctrl_done(>rm.objcom, ctrl);
@@ -649,9 +651,11 @@ r535_conn_new(struct nvkm_disp *disp, u32 id)
ctrl->subDeviceInstance = 0;
ctrl->displayId = BIT(id);
- ctrl = nvkm_gsp_rm_ctrl_push(>rm.objcom, ctrl, sizeof(*ctrl));
- if (IS_ERR(ctrl))
- return (void *)ctrl;
+ ret = nvkm_gsp_rm_ctrl_push(>rm.objcom, , sizeof(*ctrl));
+ if (ret) {
+ nvkm_gsp_rm_ctrl_done(>rm.objcom, ctrl);
+ return ERR_PTR(ret);
+ }
list_for_each_entry(conn, >conns, head) {
if (conn->index == ctrl->data[0].index) {
@@ -686,7 +690,7 @@ r535_outp_acquire(struct nvkm_outp *outp, bool hda)
struct nvkm_disp *disp = outp->disp;
struct nvkm_ior *ior;
NV0073_CTRL_DFP_ASSIGN_SOR_PARAMS *ctrl;
- int or;
+ int ret, or;
ctrl = nvkm_gsp_rm_ctrl_get(>rm.objcom,
NV0073_CTRL_CMD_DFP_ASSIGN_SOR,
sizeof(*ctrl));
@@ -699,9 +703,11 @@ r535_outp_acquire(struct nvkm_outp *outp, bool hda)
if (hda)
ctrl->flags |= NVDEF(NV0073_CTRL, DFP_ASSIGN_SOR_FLAGS, AUDIO,
OPTIMAL);
- ctrl = nvkm_gsp_rm_ctrl_push(>rm.objcom, ctrl, sizeof(*ctrl));
- if (IS_ERR(ctrl))
- return PTR_ERR(ctrl);
+ ret = nvkm_gsp_rm_ctrl_push(>rm.objcom, , sizeof(*ctrl));
+ if (ret) {
+ nvkm_gsp_rm_ctrl_done(>rm.objcom, ctrl);
+
