Dave Airlie
2023-Dec-22 04:31 UTC
[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 <airlied at redhat.com> --- .../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, &argv, 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, &argv, 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(&disp->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(&disp->rm.objcom, ctrl, sizeof(*ctrl)); - if (IS_ERR(ctrl)) - return PTR_ERR(ctrl); + ret = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, &ctrl, sizeof(*ctrl)); + if (ret) { + nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); + return ret; + } lvl = ctrl->brightness; nvkm_gsp_rm_ctrl_done(&disp->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(&disp->rm.objcom, ctrl, sizeof(*ctrl)); - if (IS_ERR(ctrl)) - return (void *)ctrl; + ret = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, &ctrl, sizeof(*ctrl)); + if (ret) { + nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); + return ERR_PTR(ret); + } list_for_each_entry(conn, &disp->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(&disp->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(&disp->rm.objcom, ctrl, sizeof(*ctrl)); - if (IS_ERR(ctrl)) - return PTR_ERR(ctrl); + ret = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, &ctrl, sizeof(*ctrl)); + if (ret) { + nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); + return ret; + } for (or = 0; or < ARRAY_SIZE(ctrl->sorAssignListWithTag); or++) { if (ctrl->sorAssignListWithTag[or].displayMask & BIT(outp->index)) { @@ -727,6 +733,7 @@ static int r535_disp_head_displayid(struct nvkm_disp *disp, int head, u32 *displayid) { NV0073_CTRL_SYSTEM_GET_ACTIVE_PARAMS *ctrl; + int ret; ctrl = nvkm_gsp_rm_ctrl_get(&disp->rm.objcom, NV0073_CTRL_CMD_SYSTEM_GET_ACTIVE, sizeof(*ctrl)); @@ -736,9 +743,11 @@ r535_disp_head_displayid(struct nvkm_disp *disp, int head, u32 *displayid) ctrl->subDeviceInstance = 0; ctrl->head = head; - ctrl = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, ctrl, sizeof(*ctrl)); - if (IS_ERR(ctrl)) - return PTR_ERR(ctrl); + ret = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, &ctrl, sizeof(*ctrl)); + if (ret) { + nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); + return ret; + } *displayid = ctrl->displayId; nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); @@ -772,9 +781,11 @@ r535_outp_inherit(struct nvkm_outp *outp) ctrl->subDeviceInstance = 0; ctrl->displayId = displayid; - ctrl = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, ctrl, sizeof(*ctrl)); - if (IS_ERR(ctrl)) + ret = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, &ctrl, sizeof(*ctrl)); + if (ret) { + nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); return NULL; + } id = ctrl->index; proto = ctrl->protocol; @@ -825,6 +836,7 @@ r535_outp_dfp_get_info(struct nvkm_outp *outp) { NV0073_CTRL_DFP_GET_INFO_PARAMS *ctrl; struct nvkm_disp *disp = outp->disp; + int ret; ctrl = nvkm_gsp_rm_ctrl_get(&disp->rm.objcom, NV0073_CTRL_CMD_DFP_GET_INFO, sizeof(*ctrl)); if (IS_ERR(ctrl)) @@ -832,9 +844,11 @@ r535_outp_dfp_get_info(struct nvkm_outp *outp) ctrl->displayId = BIT(outp->index); - ctrl = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, ctrl, sizeof(*ctrl)); - if (IS_ERR(ctrl)) - return PTR_ERR(ctrl); + ret = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, &ctrl, sizeof(*ctrl)); + if (ret) { + nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); + return ret; + } nvkm_debug(&disp->engine.subdev, "DFP %08x: flags:%08x flags2:%08x\n", ctrl->displayId, ctrl->flags, ctrl->flags2); @@ -858,9 +872,11 @@ r535_outp_detect(struct nvkm_outp *outp) ctrl->subDeviceInstance = 0; ctrl->displayMask = BIT(outp->index); - ctrl = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, ctrl, sizeof(*ctrl)); - if (IS_ERR(ctrl)) - return PTR_ERR(ctrl); + ret = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, &ctrl, sizeof(*ctrl)); + if (ret) { + nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); + return ret; + } if (ctrl->displayMask & BIT(outp->index)) { ret = r535_outp_dfp_get_info(outp); @@ -895,6 +911,7 @@ r535_dp_mst_id_get(struct nvkm_outp *outp, u32 *pid) { NV0073_CTRL_CMD_DP_TOPOLOGY_ALLOCATE_DISPLAYID_PARAMS *ctrl; struct nvkm_disp *disp = outp->disp; + int ret; ctrl = nvkm_gsp_rm_ctrl_get(&disp->rm.objcom, NV0073_CTRL_CMD_DP_TOPOLOGY_ALLOCATE_DISPLAYID, @@ -904,9 +921,11 @@ r535_dp_mst_id_get(struct nvkm_outp *outp, u32 *pid) ctrl->subDeviceInstance = 0; ctrl->displayId = BIT(outp->index); - ctrl = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, ctrl, sizeof(*ctrl)); - if (IS_ERR(ctrl)) - return PTR_ERR(ctrl); + ret = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, &ctrl, sizeof(*ctrl)); + if (ret) { + nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); + return ret; + } *pid = ctrl->displayIdAssigned; nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); @@ -964,9 +983,11 @@ r535_dp_train_target(struct nvkm_outp *outp, u8 target, bool mst, u8 link_nr, u8 !(outp->dp.dpcd[DPCD_RC03] & DPCD_RC03_TPS4_SUPPORTED)) ctrl->cmd |= NVDEF(NV0073_CTRL, DP_CMD, POST_LT_ADJ_REQ_GRANTED, YES); - ctrl = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, ctrl, sizeof(*ctrl)); - if (IS_ERR(ctrl)) - return PTR_ERR(ctrl); + ret = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, &ctrl, sizeof(*ctrl)); + if (ret) { + nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); + return ret; + } ret = ctrl->err ? -EIO : 0; nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); @@ -1036,9 +1057,11 @@ r535_dp_aux_xfer(struct nvkm_outp *outp, u8 type, u32 addr, u8 *data, u8 *psize) ctrl->size = !ctrl->bAddrOnly ? (size - 1) : 0; memcpy(ctrl->data, data, size); - ctrl = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, ctrl, sizeof(*ctrl)); - if (IS_ERR(ctrl)) + ret = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, &ctrl, sizeof(*ctrl)); + if (ret) { + nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); return PTR_ERR(ctrl); + } memcpy(data, ctrl->data, size); *psize = ctrl->size; @@ -1111,10 +1134,13 @@ r535_tmds_edid_get(struct nvkm_outp *outp, u8 *data, u16 *psize) ctrl->subDeviceInstance = 0; ctrl->displayId = BIT(outp->index); - ctrl = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, ctrl, sizeof(*ctrl)); - if (IS_ERR(ctrl)) - return PTR_ERR(ctrl); + ret = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, &ctrl, sizeof(*ctrl)); + if (ret) { + nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); + return ret; + } + ret = -E2BIG; if (ctrl->bufferSize <= *psize) { memcpy(data, ctrl->edidBuffer, ctrl->bufferSize); *psize = ctrl->bufferSize; @@ -1153,9 +1179,11 @@ r535_outp_new(struct nvkm_disp *disp, u32 id) ctrl->subDeviceInstance = 0; ctrl->displayId = BIT(id); - ctrl = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, ctrl, sizeof(*ctrl)); - if (IS_ERR(ctrl)) - return PTR_ERR(ctrl); + ret = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, &ctrl, sizeof(*ctrl)); + if (ret) { + nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); + return ret; + } switch (ctrl->type) { case NV0073_CTRL_SPECIFIC_OR_TYPE_NONE: @@ -1229,9 +1257,11 @@ r535_outp_new(struct nvkm_disp *disp, u32 id) ctrl->sorIndex = ~0; - ctrl = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, ctrl, sizeof(*ctrl)); - if (IS_ERR(ctrl)) - return PTR_ERR(ctrl); + ret = nvkm_gsp_rm_ctrl_push(&disp->rm.objcom, &ctrl, sizeof(*ctrl)); + if (ret) { + nvkm_gsp_rm_ctrl_done(&disp->rm.objcom, ctrl); + return ret; + } switch (NVVAL_GET(ctrl->maxLinkRate, NV0073_CTRL_CMD, DP_GET_CAPS, MAX_LINK_RATE)) { case NV0073_CTRL_CMD_DP_GET_CAPS_MAX_LINK_RATE_1_62: diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c index 54c1fbccc013..e2810fd1a36f 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c @@ -599,13 +599,13 @@ r535_gsp_rpc_rm_alloc_push(struct nvkm_gsp_object *object, void *argv, u32 repc) if (rpc->status) { ret = ERR_PTR(r535_rpc_status_to_errno(rpc->status)); - if (ret != -EAGAIN) + if (PTR_ERR(ret) != -EAGAIN) nvkm_error(&gsp->subdev, "RM_ALLOC: 0x%x\n", rpc->status); } else { ret = repc ? rpc->params : NULL; } - if (IS_ERR_OR_NULL(ret)) + if (ret) nvkm_gsp_rpc_done(gsp, rpc); return ret; @@ -639,30 +639,34 @@ r535_gsp_rpc_rm_ctrl_done(struct nvkm_gsp_object *object, void *repv) { rpc_gsp_rm_control_v03_00 *rpc = container_of(repv, typeof(*rpc), params); + if (!repv) + return; nvkm_gsp_rpc_done(object->client->gsp, rpc); } -static void * -r535_gsp_rpc_rm_ctrl_push(struct nvkm_gsp_object *object, void *argv, u32 repc) +static int +r535_gsp_rpc_rm_ctrl_push(struct nvkm_gsp_object *object, void **argv, u32 repc) { - rpc_gsp_rm_control_v03_00 *rpc = container_of(argv, typeof(*rpc), params); + rpc_gsp_rm_control_v03_00 *rpc = container_of((*argv), typeof(*rpc), params); struct nvkm_gsp *gsp = object->client->gsp; - void *ret; + int ret = 0; rpc = nvkm_gsp_rpc_push(gsp, rpc, true, repc); - if (IS_ERR_OR_NULL(rpc)) - return rpc; + if (IS_ERR_OR_NULL(rpc)) { + *argv = NULL; + return PTR_ERR(rpc); + } if (rpc->status) { - ret = ERR_PTR(r535_rpc_status_to_errno(rpc->status)); + ret = r535_rpc_status_to_errno(rpc->status); if (ret != -EAGAIN) nvkm_error(&gsp->subdev, "cli:0x%08x obj:0x%08x ctrl cmd:0x%08x failed: 0x%08x\n", object->client->object.handle, object->handle, rpc->cmd, rpc->status); - } else { - ret = repc ? rpc->params : NULL; } - if (IS_ERR_OR_NULL(ret)) + if (repc) + *argv = rpc->params; + else nvkm_gsp_rpc_done(gsp, rpc); return ret; @@ -860,9 +864,11 @@ r535_gsp_intr_get_table(struct nvkm_gsp *gsp) if (IS_ERR(ctrl)) return PTR_ERR(ctrl); - ctrl = nvkm_gsp_rm_ctrl_push(&gsp->internal.device.subdevice, ctrl, sizeof(*ctrl)); - if (WARN_ON(IS_ERR(ctrl))) - return PTR_ERR(ctrl); + ret = nvkm_gsp_rm_ctrl_push(&gsp->internal.device.subdevice, &ctrl, sizeof(*ctrl)); + if (WARN_ON(ret)) { + nvkm_gsp_rm_ctrl_done(&gsp->internal.device.subdevice, ctrl); + return ret; + } for (unsigned i = 0; i < ctrl->tableLen; i++) { enum nvkm_subdev_type type; -- 2.43.0
Dan Carpenter
2024-Jan-03 14:46 UTC
[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-lkp at 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 <lkp at intel.com> | Reported-by: Dan Carpenter <dan.carpenter at linaro.org> | Closes: https://lore.kernel.org/r/202312271917.55xuDMdc-lkp at 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(&gsp->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