* [PATCH 0/3] drm/msm: Add comm/cmdline override @ 2022-03-17 0:29 ` Rob Clark 0 siblings, 0 replies; 16+ messages in thread From: Rob Clark @ 2022-03-17 0:29 UTC (permalink / raw) To: dri-devel Cc: freedreno, linux-arm-msm, Rob Clark, Abhinav Kumar, Akhil P Oommen, Bjorn Andersson, Christian König, Dan Carpenter, Emma Anholt, Jonathan Marek, Jordan Crouse, open list, Vladimir Lypak From: Rob Clark <robdclark@chromium.org> Add a way to override comm/cmdline per-drm_file. This is useful for VM scenarios where the host process is just a proxy for the actual guest process. Rob Clark (3): drm/msm: Add support for pointer params drm/msm: Split out helper to get comm/cmdline drm/msm: Add a way to override processes comm/cmdline drivers/gpu/drm/msm/adreno/adreno_gpu.c | 46 +++++++++++++++++++++++-- drivers/gpu/drm/msm/adreno/adreno_gpu.h | 4 +-- drivers/gpu/drm/msm/msm_drv.c | 8 ++--- drivers/gpu/drm/msm/msm_gpu.c | 39 ++++++++++++--------- drivers/gpu/drm/msm/msm_gpu.h | 10 ++++-- drivers/gpu/drm/msm/msm_rd.c | 5 +-- drivers/gpu/drm/msm/msm_submitqueue.c | 2 ++ include/uapi/drm/msm_drm.h | 4 +++ 8 files changed, 90 insertions(+), 28 deletions(-) -- 2.35.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/3] drm/msm: Add comm/cmdline override @ 2022-03-17 0:29 ` Rob Clark 0 siblings, 0 replies; 16+ messages in thread From: Rob Clark @ 2022-03-17 0:29 UTC (permalink / raw) To: dri-devel Cc: Rob Clark, open list, Jonathan Marek, Emma Anholt, Akhil P Oommen, linux-arm-msm, Vladimir Lypak, Abhinav Kumar, Bjorn Andersson, Jordan Crouse, freedreno, Christian König, Dan Carpenter From: Rob Clark <robdclark@chromium.org> Add a way to override comm/cmdline per-drm_file. This is useful for VM scenarios where the host process is just a proxy for the actual guest process. Rob Clark (3): drm/msm: Add support for pointer params drm/msm: Split out helper to get comm/cmdline drm/msm: Add a way to override processes comm/cmdline drivers/gpu/drm/msm/adreno/adreno_gpu.c | 46 +++++++++++++++++++++++-- drivers/gpu/drm/msm/adreno/adreno_gpu.h | 4 +-- drivers/gpu/drm/msm/msm_drv.c | 8 ++--- drivers/gpu/drm/msm/msm_gpu.c | 39 ++++++++++++--------- drivers/gpu/drm/msm/msm_gpu.h | 10 ++++-- drivers/gpu/drm/msm/msm_rd.c | 5 +-- drivers/gpu/drm/msm/msm_submitqueue.c | 2 ++ include/uapi/drm/msm_drm.h | 4 +++ 8 files changed, 90 insertions(+), 28 deletions(-) -- 2.35.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] drm/msm: Add support for pointer params 2022-03-17 0:29 ` Rob Clark @ 2022-03-17 0:29 ` Rob Clark -1 siblings, 0 replies; 16+ messages in thread From: Rob Clark @ 2022-03-17 0:29 UTC (permalink / raw) To: dri-devel Cc: freedreno, linux-arm-msm, Rob Clark, Rob Clark, Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter, Akhil P Oommen, Jonathan Marek, Jordan Crouse, Christian König, Dan Carpenter, Bjorn Andersson, Emma Anholt, Vladimir Lypak, open list From: Rob Clark <robdclark@chromium.org> The 64b value field is already suffient to hold a pointer instead of immediate, but we also need a length field. Signed-off-by: Rob Clark <robdclark@chromium.org> --- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 12 ++++++++++-- drivers/gpu/drm/msm/adreno/adreno_gpu.h | 4 ++-- drivers/gpu/drm/msm/msm_drv.c | 8 ++++---- drivers/gpu/drm/msm/msm_gpu.h | 4 ++-- drivers/gpu/drm/msm/msm_rd.c | 5 +++-- include/uapi/drm/msm_drm.h | 2 ++ 6 files changed, 23 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c index 9efc84929be0..3d307b34854d 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c @@ -229,10 +229,14 @@ adreno_iommu_create_address_space(struct msm_gpu *gpu, } int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx, - uint32_t param, uint64_t *value) + uint32_t param, uint64_t *value, uint32_t *len) { struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); + /* No pointer params yet */ + if (*len != 0) + return -EINVAL; + switch (param) { case MSM_PARAM_GPU_ID: *value = adreno_gpu->info->revn; @@ -284,8 +288,12 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx, } int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx, - uint32_t param, uint64_t value) + uint32_t param, uint64_t value, uint32_t len) { + /* No pointer params yet */ + if (len != 0) + return -EINVAL; + switch (param) { case MSM_PARAM_SYSPROF: if (!capable(CAP_SYS_ADMIN)) diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h index 0490c5fbb780..ab3b5ef80332 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h @@ -281,9 +281,9 @@ static inline int adreno_is_a650_family(struct adreno_gpu *gpu) } int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx, - uint32_t param, uint64_t *value); + uint32_t param, uint64_t *value, uint32_t *len); int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx, - uint32_t param, uint64_t value); + uint32_t param, uint64_t value, uint32_t len); const struct firmware *adreno_request_fw(struct adreno_gpu *adreno_gpu, const char *fwname); struct drm_gem_object *adreno_fw_create_bo(struct msm_gpu *gpu, diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 780f9748aaaf..a5eed5738ac8 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -610,7 +610,7 @@ static int msm_ioctl_get_param(struct drm_device *dev, void *data, /* for now, we just have 3d pipe.. eventually this would need to * be more clever to dispatch to appropriate gpu module: */ - if (args->pipe != MSM_PIPE_3D0) + if ((args->pipe != MSM_PIPE_3D0) || (args->pad != 0)) return -EINVAL; gpu = priv->gpu; @@ -619,7 +619,7 @@ static int msm_ioctl_get_param(struct drm_device *dev, void *data, return -ENXIO; return gpu->funcs->get_param(gpu, file->driver_priv, - args->param, &args->value); + args->param, &args->value, &args->len); } static int msm_ioctl_set_param(struct drm_device *dev, void *data, @@ -629,7 +629,7 @@ static int msm_ioctl_set_param(struct drm_device *dev, void *data, struct drm_msm_param *args = data; struct msm_gpu *gpu; - if (args->pipe != MSM_PIPE_3D0) + if ((args->pipe != MSM_PIPE_3D0) || (args->pad != 0)) return -EINVAL; gpu = priv->gpu; @@ -638,7 +638,7 @@ static int msm_ioctl_set_param(struct drm_device *dev, void *data, return -ENXIO; return gpu->funcs->set_param(gpu, file->driver_priv, - args->param, args->value); + args->param, args->value, args->len); } static int msm_ioctl_gem_new(struct drm_device *dev, void *data, diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h index a84140055920..c28c2ad9f52e 100644 --- a/drivers/gpu/drm/msm/msm_gpu.h +++ b/drivers/gpu/drm/msm/msm_gpu.h @@ -44,9 +44,9 @@ struct msm_gpu_config { */ struct msm_gpu_funcs { int (*get_param)(struct msm_gpu *gpu, struct msm_file_private *ctx, - uint32_t param, uint64_t *value); + uint32_t param, uint64_t *value, uint32_t *len); int (*set_param)(struct msm_gpu *gpu, struct msm_file_private *ctx, - uint32_t param, uint64_t value); + uint32_t param, uint64_t value, uint32_t len); int (*hw_init)(struct msm_gpu *gpu); int (*pm_suspend)(struct msm_gpu *gpu); int (*pm_resume)(struct msm_gpu *gpu); diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c index 9d835331f214..a92ffde53f0b 100644 --- a/drivers/gpu/drm/msm/msm_rd.c +++ b/drivers/gpu/drm/msm/msm_rd.c @@ -180,6 +180,7 @@ static int rd_open(struct inode *inode, struct file *file) struct msm_gpu *gpu = priv->gpu; uint64_t val; uint32_t gpu_id; + uint32_t zero = 0; int ret = 0; if (!gpu) @@ -200,12 +201,12 @@ static int rd_open(struct inode *inode, struct file *file) * * Note: These particular params do not require a context */ - gpu->funcs->get_param(gpu, NULL, MSM_PARAM_GPU_ID, &val); + gpu->funcs->get_param(gpu, NULL, MSM_PARAM_GPU_ID, &val, &zero); gpu_id = val; rd_write_section(rd, RD_GPU_ID, &gpu_id, sizeof(gpu_id)); - gpu->funcs->get_param(gpu, NULL, MSM_PARAM_CHIP_ID, &val); + gpu->funcs->get_param(gpu, NULL, MSM_PARAM_CHIP_ID, &val, &zero); rd_write_section(rd, RD_CHIP_ID, &val, sizeof(val)); out: diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h index 07efc8033492..0aa1a8cb4e0d 100644 --- a/include/uapi/drm/msm_drm.h +++ b/include/uapi/drm/msm_drm.h @@ -95,6 +95,8 @@ struct drm_msm_param { __u32 pipe; /* in, MSM_PIPE_x */ __u32 param; /* in, MSM_PARAM_x */ __u64 value; /* out (get_param) or in (set_param) */ + __u32 len; /* zero for non-pointer params */ + __u32 pad; /* must be zero */ }; /* -- 2.35.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 1/3] drm/msm: Add support for pointer params @ 2022-03-17 0:29 ` Rob Clark 0 siblings, 0 replies; 16+ messages in thread From: Rob Clark @ 2022-03-17 0:29 UTC (permalink / raw) To: dri-devel Cc: Rob Clark, open list, Emma Anholt, Jonathan Marek, Akhil P Oommen, David Airlie, linux-arm-msm, Vladimir Lypak, Abhinav Kumar, Jordan Crouse, Sean Paul, Bjorn Andersson, freedreno, Christian König, Dan Carpenter From: Rob Clark <robdclark@chromium.org> The 64b value field is already suffient to hold a pointer instead of immediate, but we also need a length field. Signed-off-by: Rob Clark <robdclark@chromium.org> --- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 12 ++++++++++-- drivers/gpu/drm/msm/adreno/adreno_gpu.h | 4 ++-- drivers/gpu/drm/msm/msm_drv.c | 8 ++++---- drivers/gpu/drm/msm/msm_gpu.h | 4 ++-- drivers/gpu/drm/msm/msm_rd.c | 5 +++-- include/uapi/drm/msm_drm.h | 2 ++ 6 files changed, 23 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c index 9efc84929be0..3d307b34854d 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c @@ -229,10 +229,14 @@ adreno_iommu_create_address_space(struct msm_gpu *gpu, } int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx, - uint32_t param, uint64_t *value) + uint32_t param, uint64_t *value, uint32_t *len) { struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); + /* No pointer params yet */ + if (*len != 0) + return -EINVAL; + switch (param) { case MSM_PARAM_GPU_ID: *value = adreno_gpu->info->revn; @@ -284,8 +288,12 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx, } int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx, - uint32_t param, uint64_t value) + uint32_t param, uint64_t value, uint32_t len) { + /* No pointer params yet */ + if (len != 0) + return -EINVAL; + switch (param) { case MSM_PARAM_SYSPROF: if (!capable(CAP_SYS_ADMIN)) diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h index 0490c5fbb780..ab3b5ef80332 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h @@ -281,9 +281,9 @@ static inline int adreno_is_a650_family(struct adreno_gpu *gpu) } int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx, - uint32_t param, uint64_t *value); + uint32_t param, uint64_t *value, uint32_t *len); int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx, - uint32_t param, uint64_t value); + uint32_t param, uint64_t value, uint32_t len); const struct firmware *adreno_request_fw(struct adreno_gpu *adreno_gpu, const char *fwname); struct drm_gem_object *adreno_fw_create_bo(struct msm_gpu *gpu, diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 780f9748aaaf..a5eed5738ac8 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -610,7 +610,7 @@ static int msm_ioctl_get_param(struct drm_device *dev, void *data, /* for now, we just have 3d pipe.. eventually this would need to * be more clever to dispatch to appropriate gpu module: */ - if (args->pipe != MSM_PIPE_3D0) + if ((args->pipe != MSM_PIPE_3D0) || (args->pad != 0)) return -EINVAL; gpu = priv->gpu; @@ -619,7 +619,7 @@ static int msm_ioctl_get_param(struct drm_device *dev, void *data, return -ENXIO; return gpu->funcs->get_param(gpu, file->driver_priv, - args->param, &args->value); + args->param, &args->value, &args->len); } static int msm_ioctl_set_param(struct drm_device *dev, void *data, @@ -629,7 +629,7 @@ static int msm_ioctl_set_param(struct drm_device *dev, void *data, struct drm_msm_param *args = data; struct msm_gpu *gpu; - if (args->pipe != MSM_PIPE_3D0) + if ((args->pipe != MSM_PIPE_3D0) || (args->pad != 0)) return -EINVAL; gpu = priv->gpu; @@ -638,7 +638,7 @@ static int msm_ioctl_set_param(struct drm_device *dev, void *data, return -ENXIO; return gpu->funcs->set_param(gpu, file->driver_priv, - args->param, args->value); + args->param, args->value, args->len); } static int msm_ioctl_gem_new(struct drm_device *dev, void *data, diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h index a84140055920..c28c2ad9f52e 100644 --- a/drivers/gpu/drm/msm/msm_gpu.h +++ b/drivers/gpu/drm/msm/msm_gpu.h @@ -44,9 +44,9 @@ struct msm_gpu_config { */ struct msm_gpu_funcs { int (*get_param)(struct msm_gpu *gpu, struct msm_file_private *ctx, - uint32_t param, uint64_t *value); + uint32_t param, uint64_t *value, uint32_t *len); int (*set_param)(struct msm_gpu *gpu, struct msm_file_private *ctx, - uint32_t param, uint64_t value); + uint32_t param, uint64_t value, uint32_t len); int (*hw_init)(struct msm_gpu *gpu); int (*pm_suspend)(struct msm_gpu *gpu); int (*pm_resume)(struct msm_gpu *gpu); diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c index 9d835331f214..a92ffde53f0b 100644 --- a/drivers/gpu/drm/msm/msm_rd.c +++ b/drivers/gpu/drm/msm/msm_rd.c @@ -180,6 +180,7 @@ static int rd_open(struct inode *inode, struct file *file) struct msm_gpu *gpu = priv->gpu; uint64_t val; uint32_t gpu_id; + uint32_t zero = 0; int ret = 0; if (!gpu) @@ -200,12 +201,12 @@ static int rd_open(struct inode *inode, struct file *file) * * Note: These particular params do not require a context */ - gpu->funcs->get_param(gpu, NULL, MSM_PARAM_GPU_ID, &val); + gpu->funcs->get_param(gpu, NULL, MSM_PARAM_GPU_ID, &val, &zero); gpu_id = val; rd_write_section(rd, RD_GPU_ID, &gpu_id, sizeof(gpu_id)); - gpu->funcs->get_param(gpu, NULL, MSM_PARAM_CHIP_ID, &val); + gpu->funcs->get_param(gpu, NULL, MSM_PARAM_CHIP_ID, &val, &zero); rd_write_section(rd, RD_CHIP_ID, &val, sizeof(val)); out: diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h index 07efc8033492..0aa1a8cb4e0d 100644 --- a/include/uapi/drm/msm_drm.h +++ b/include/uapi/drm/msm_drm.h @@ -95,6 +95,8 @@ struct drm_msm_param { __u32 pipe; /* in, MSM_PIPE_x */ __u32 param; /* in, MSM_PARAM_x */ __u64 value; /* out (get_param) or in (set_param) */ + __u32 len; /* zero for non-pointer params */ + __u32 pad; /* must be zero */ }; /* -- 2.35.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] drm/msm: Split out helper to get comm/cmdline 2022-03-17 0:29 ` Rob Clark @ 2022-03-17 0:29 ` Rob Clark -1 siblings, 0 replies; 16+ messages in thread From: Rob Clark @ 2022-03-17 0:29 UTC (permalink / raw) To: dri-devel Cc: freedreno, linux-arm-msm, Rob Clark, Rob Clark, Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter, open list From: Rob Clark <robdclark@chromium.org> Deduplicate this from fault_worker and recover_worker. Signed-off-by: Rob Clark <robdclark@chromium.org> --- drivers/gpu/drm/msm/msm_gpu.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index 8fe4aee96aa9..4ec62b601adc 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -362,6 +362,20 @@ find_submit(struct msm_ringbuffer *ring, uint32_t fence) static void retire_submits(struct msm_gpu *gpu); +static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd) +{ + struct task_struct *task; + + task = get_pid_task(submit->pid, PIDTYPE_PID); + if (!task) + return; + + *comm = kstrdup(task->comm, GFP_KERNEL); + *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); + + put_task_struct(task); +} + static void recover_worker(struct kthread_work *work) { struct msm_gpu *gpu = container_of(work, struct msm_gpu, recover_work); @@ -378,18 +392,11 @@ static void recover_worker(struct kthread_work *work) submit = find_submit(cur_ring, cur_ring->memptrs->fence + 1); if (submit) { - struct task_struct *task; - /* Increment the fault counts */ submit->queue->faults++; submit->aspace->faults++; - task = get_pid_task(submit->pid, PIDTYPE_PID); - if (task) { - comm = kstrdup(task->comm, GFP_KERNEL); - cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); - put_task_struct(task); - } + get_comm_cmdline(submit, &comm, &cmd); if (comm && cmd) { DRM_DEV_ERROR(dev->dev, "%s: offending task: %s (%s)\n", @@ -478,14 +485,7 @@ static void fault_worker(struct kthread_work *work) goto resume_smmu; if (submit) { - struct task_struct *task; - - task = get_pid_task(submit->pid, PIDTYPE_PID); - if (task) { - comm = kstrdup(task->comm, GFP_KERNEL); - cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); - put_task_struct(task); - } + get_comm_cmdline(submit, &comm, &cmd); /* * When we get GPU iova faults, we can get 1000s of them, -- 2.35.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] drm/msm: Split out helper to get comm/cmdline @ 2022-03-17 0:29 ` Rob Clark 0 siblings, 0 replies; 16+ messages in thread From: Rob Clark @ 2022-03-17 0:29 UTC (permalink / raw) To: dri-devel Cc: Rob Clark, David Airlie, linux-arm-msm, Abhinav Kumar, open list, Sean Paul, freedreno From: Rob Clark <robdclark@chromium.org> Deduplicate this from fault_worker and recover_worker. Signed-off-by: Rob Clark <robdclark@chromium.org> --- drivers/gpu/drm/msm/msm_gpu.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index 8fe4aee96aa9..4ec62b601adc 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -362,6 +362,20 @@ find_submit(struct msm_ringbuffer *ring, uint32_t fence) static void retire_submits(struct msm_gpu *gpu); +static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd) +{ + struct task_struct *task; + + task = get_pid_task(submit->pid, PIDTYPE_PID); + if (!task) + return; + + *comm = kstrdup(task->comm, GFP_KERNEL); + *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); + + put_task_struct(task); +} + static void recover_worker(struct kthread_work *work) { struct msm_gpu *gpu = container_of(work, struct msm_gpu, recover_work); @@ -378,18 +392,11 @@ static void recover_worker(struct kthread_work *work) submit = find_submit(cur_ring, cur_ring->memptrs->fence + 1); if (submit) { - struct task_struct *task; - /* Increment the fault counts */ submit->queue->faults++; submit->aspace->faults++; - task = get_pid_task(submit->pid, PIDTYPE_PID); - if (task) { - comm = kstrdup(task->comm, GFP_KERNEL); - cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); - put_task_struct(task); - } + get_comm_cmdline(submit, &comm, &cmd); if (comm && cmd) { DRM_DEV_ERROR(dev->dev, "%s: offending task: %s (%s)\n", @@ -478,14 +485,7 @@ static void fault_worker(struct kthread_work *work) goto resume_smmu; if (submit) { - struct task_struct *task; - - task = get_pid_task(submit->pid, PIDTYPE_PID); - if (task) { - comm = kstrdup(task->comm, GFP_KERNEL); - cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); - put_task_struct(task); - } + get_comm_cmdline(submit, &comm, &cmd); /* * When we get GPU iova faults, we can get 1000s of them, -- 2.35.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline 2022-03-17 0:29 ` Rob Clark @ 2022-03-17 0:29 ` Rob Clark -1 siblings, 0 replies; 16+ messages in thread From: Rob Clark @ 2022-03-17 0:29 UTC (permalink / raw) To: dri-devel Cc: freedreno, linux-arm-msm, Rob Clark, Rob Clark, Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter, Akhil P Oommen, Jonathan Marek, Jordan Crouse, Emma Anholt, Dan Carpenter, open list From: Rob Clark <robdclark@chromium.org> In the cause of using the GPU via virtgpu, the host side process is really a sort of proxy, and not terribly interesting from the PoV of crash/fault logging. Add a way to override these per process so that we can see the guest process's name. Signed-off-by: Rob Clark <robdclark@chromium.org> --- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 40 +++++++++++++++++++++++-- drivers/gpu/drm/msm/msm_gpu.c | 11 +++++-- drivers/gpu/drm/msm/msm_gpu.h | 6 ++++ drivers/gpu/drm/msm/msm_submitqueue.c | 2 ++ include/uapi/drm/msm_drm.h | 2 ++ 5 files changed, 56 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c index 3d307b34854d..c68dc9c722c7 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c @@ -290,11 +290,45 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx, int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx, uint32_t param, uint64_t value, uint32_t len) { - /* No pointer params yet */ - if (len != 0) - return -EINVAL; + switch (param) { + case MSM_PARAM_COMM: + case MSM_PARAM_CMDLINE: + /* kstrdup_quotable_cmdline() limits to PAGE_SIZE, so + * that should be a reasonable upper bound + */ + if (len > PAGE_SIZE) + return -EINVAL; + break; + default: + if (len != 0) + return -EINVAL; + } switch (param) { + case MSM_PARAM_COMM: + case MSM_PARAM_CMDLINE: { + char *str, **paramp; + + str = kmalloc(len + 1, GFP_KERNEL); + if (copy_from_user(str, u64_to_user_ptr(value), len)) { + kfree(str); + return -EFAULT; + } + + /* Ensure string is null terminated: */ + str[len] = '\0'; + + if (param == MSM_PARAM_COMM) { + paramp = &ctx->comm; + } else { + paramp = &ctx->cmdline; + } + + kfree(*paramp); + *paramp = str; + + return 0; + } case MSM_PARAM_SYSPROF: if (!capable(CAP_SYS_ADMIN)) return -EPERM; diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index 4ec62b601adc..68f3f8ade76d 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu); static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd) { + struct msm_file_private *ctx = submit->queue->ctx; struct task_struct *task; + *comm = kstrdup(ctx->comm, GFP_KERNEL); + *cmd = kstrdup(ctx->cmdline, GFP_KERNEL); + task = get_pid_task(submit->pid, PIDTYPE_PID); if (!task) return; - *comm = kstrdup(task->comm, GFP_KERNEL); - *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); + if (!*comm) + *comm = kstrdup(task->comm, GFP_KERNEL); + + if (!*cmd) + *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); put_task_struct(task); } diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h index c28c2ad9f52e..2c0203fd6ce3 100644 --- a/drivers/gpu/drm/msm/msm_gpu.h +++ b/drivers/gpu/drm/msm/msm_gpu.h @@ -355,6 +355,12 @@ struct msm_file_private { */ int sysprof; + /** comm: Overridden task comm, see MSM_PARAM_COMM */ + char *comm; + + /** cmdline: Overridden task cmdline, see MSM_PARAM_CMDLINE */ + char *cmdline; + /** * elapsed: * diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c index 79b6ccd6ce64..f486a3cd4e55 100644 --- a/drivers/gpu/drm/msm/msm_submitqueue.c +++ b/drivers/gpu/drm/msm/msm_submitqueue.c @@ -61,6 +61,8 @@ void __msm_file_private_destroy(struct kref *kref) } msm_gem_address_space_put(ctx->aspace); + kfree(ctx->comm); + kfree(ctx->cmdline); kfree(ctx); } diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h index 0aa1a8cb4e0d..794ad1948497 100644 --- a/include/uapi/drm/msm_drm.h +++ b/include/uapi/drm/msm_drm.h @@ -82,6 +82,8 @@ struct drm_msm_timespec { #define MSM_PARAM_FAULTS 0x09 /* RO */ #define MSM_PARAM_SUSPENDS 0x0a /* RO */ #define MSM_PARAM_SYSPROF 0x0b /* WO: 1 preserves perfcntrs, 2 also disables suspend */ +#define MSM_PARAM_COMM 0x0c /* WO: override for task->comm */ +#define MSM_PARAM_CMDLINE 0x0d /* WO: override for task cmdline */ /* For backwards compat. The original support for preemption was based on * a single ring per priority level so # of priority levels equals the # -- 2.35.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline @ 2022-03-17 0:29 ` Rob Clark 0 siblings, 0 replies; 16+ messages in thread From: Rob Clark @ 2022-03-17 0:29 UTC (permalink / raw) To: dri-devel Cc: Rob Clark, open list, Emma Anholt, Jonathan Marek, Akhil P Oommen, David Airlie, linux-arm-msm, Abhinav Kumar, Jordan Crouse, Sean Paul, freedreno, Dan Carpenter From: Rob Clark <robdclark@chromium.org> In the cause of using the GPU via virtgpu, the host side process is really a sort of proxy, and not terribly interesting from the PoV of crash/fault logging. Add a way to override these per process so that we can see the guest process's name. Signed-off-by: Rob Clark <robdclark@chromium.org> --- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 40 +++++++++++++++++++++++-- drivers/gpu/drm/msm/msm_gpu.c | 11 +++++-- drivers/gpu/drm/msm/msm_gpu.h | 6 ++++ drivers/gpu/drm/msm/msm_submitqueue.c | 2 ++ include/uapi/drm/msm_drm.h | 2 ++ 5 files changed, 56 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c index 3d307b34854d..c68dc9c722c7 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c @@ -290,11 +290,45 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx, int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx, uint32_t param, uint64_t value, uint32_t len) { - /* No pointer params yet */ - if (len != 0) - return -EINVAL; + switch (param) { + case MSM_PARAM_COMM: + case MSM_PARAM_CMDLINE: + /* kstrdup_quotable_cmdline() limits to PAGE_SIZE, so + * that should be a reasonable upper bound + */ + if (len > PAGE_SIZE) + return -EINVAL; + break; + default: + if (len != 0) + return -EINVAL; + } switch (param) { + case MSM_PARAM_COMM: + case MSM_PARAM_CMDLINE: { + char *str, **paramp; + + str = kmalloc(len + 1, GFP_KERNEL); + if (copy_from_user(str, u64_to_user_ptr(value), len)) { + kfree(str); + return -EFAULT; + } + + /* Ensure string is null terminated: */ + str[len] = '\0'; + + if (param == MSM_PARAM_COMM) { + paramp = &ctx->comm; + } else { + paramp = &ctx->cmdline; + } + + kfree(*paramp); + *paramp = str; + + return 0; + } case MSM_PARAM_SYSPROF: if (!capable(CAP_SYS_ADMIN)) return -EPERM; diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index 4ec62b601adc..68f3f8ade76d 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu); static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd) { + struct msm_file_private *ctx = submit->queue->ctx; struct task_struct *task; + *comm = kstrdup(ctx->comm, GFP_KERNEL); + *cmd = kstrdup(ctx->cmdline, GFP_KERNEL); + task = get_pid_task(submit->pid, PIDTYPE_PID); if (!task) return; - *comm = kstrdup(task->comm, GFP_KERNEL); - *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); + if (!*comm) + *comm = kstrdup(task->comm, GFP_KERNEL); + + if (!*cmd) + *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); put_task_struct(task); } diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h index c28c2ad9f52e..2c0203fd6ce3 100644 --- a/drivers/gpu/drm/msm/msm_gpu.h +++ b/drivers/gpu/drm/msm/msm_gpu.h @@ -355,6 +355,12 @@ struct msm_file_private { */ int sysprof; + /** comm: Overridden task comm, see MSM_PARAM_COMM */ + char *comm; + + /** cmdline: Overridden task cmdline, see MSM_PARAM_CMDLINE */ + char *cmdline; + /** * elapsed: * diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c index 79b6ccd6ce64..f486a3cd4e55 100644 --- a/drivers/gpu/drm/msm/msm_submitqueue.c +++ b/drivers/gpu/drm/msm/msm_submitqueue.c @@ -61,6 +61,8 @@ void __msm_file_private_destroy(struct kref *kref) } msm_gem_address_space_put(ctx->aspace); + kfree(ctx->comm); + kfree(ctx->cmdline); kfree(ctx); } diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h index 0aa1a8cb4e0d..794ad1948497 100644 --- a/include/uapi/drm/msm_drm.h +++ b/include/uapi/drm/msm_drm.h @@ -82,6 +82,8 @@ struct drm_msm_timespec { #define MSM_PARAM_FAULTS 0x09 /* RO */ #define MSM_PARAM_SUSPENDS 0x0a /* RO */ #define MSM_PARAM_SYSPROF 0x0b /* WO: 1 preserves perfcntrs, 2 also disables suspend */ +#define MSM_PARAM_COMM 0x0c /* WO: override for task->comm */ +#define MSM_PARAM_CMDLINE 0x0d /* WO: override for task cmdline */ /* For backwards compat. The original support for preemption was based on * a single ring per priority level so # of priority levels equals the # -- 2.35.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline 2022-03-17 0:29 ` Rob Clark @ 2022-03-17 8:21 ` Dan Carpenter -1 siblings, 0 replies; 16+ messages in thread From: Dan Carpenter @ 2022-03-17 8:21 UTC (permalink / raw) To: Rob Clark Cc: dri-devel, freedreno, linux-arm-msm, Rob Clark, Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter, Akhil P Oommen, Jonathan Marek, Jordan Crouse, Emma Anholt, open list On Wed, Mar 16, 2022 at 05:29:45PM -0700, Rob Clark wrote: > switch (param) { > + case MSM_PARAM_COMM: > + case MSM_PARAM_CMDLINE: { > + char *str, **paramp; > + > + str = kmalloc(len + 1, GFP_KERNEL); if (!str) return -ENOMEM; > + if (copy_from_user(str, u64_to_user_ptr(value), len)) { > + kfree(str); > + return -EFAULT; > + } > + > + /* Ensure string is null terminated: */ > + str[len] = '\0'; > + > + if (param == MSM_PARAM_COMM) { > + paramp = &ctx->comm; > + } else { > + paramp = &ctx->cmdline; > + } > + > + kfree(*paramp); > + *paramp = str; > + > + return 0; > + } > case MSM_PARAM_SYSPROF: > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > index 4ec62b601adc..68f3f8ade76d 100644 > --- a/drivers/gpu/drm/msm/msm_gpu.c > +++ b/drivers/gpu/drm/msm/msm_gpu.c > @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu); > > static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd) > { > + struct msm_file_private *ctx = submit->queue->ctx; > struct task_struct *task; > > + *comm = kstrdup(ctx->comm, GFP_KERNEL); > + *cmd = kstrdup(ctx->cmdline, GFP_KERNEL); > + > task = get_pid_task(submit->pid, PIDTYPE_PID); > if (!task) > return; > > - *comm = kstrdup(task->comm, GFP_KERNEL); > - *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); > + if (!*comm) > + *comm = kstrdup(task->comm, GFP_KERNEL); What? If the first allocation failed, then this one is going to fail as well. Just return -ENOMEM. Or maybe this is meant to be checking for an empty string? > + > + if (!*cmd) > + *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); Same. > > put_task_struct(task); > } regards, dan carpenter ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline @ 2022-03-17 8:21 ` Dan Carpenter 0 siblings, 0 replies; 16+ messages in thread From: Dan Carpenter @ 2022-03-17 8:21 UTC (permalink / raw) To: Rob Clark Cc: Rob Clark, freedreno, Emma Anholt, Jonathan Marek, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel, Jordan Crouse, Akhil P Oommen, Sean Paul, open list On Wed, Mar 16, 2022 at 05:29:45PM -0700, Rob Clark wrote: > switch (param) { > + case MSM_PARAM_COMM: > + case MSM_PARAM_CMDLINE: { > + char *str, **paramp; > + > + str = kmalloc(len + 1, GFP_KERNEL); if (!str) return -ENOMEM; > + if (copy_from_user(str, u64_to_user_ptr(value), len)) { > + kfree(str); > + return -EFAULT; > + } > + > + /* Ensure string is null terminated: */ > + str[len] = '\0'; > + > + if (param == MSM_PARAM_COMM) { > + paramp = &ctx->comm; > + } else { > + paramp = &ctx->cmdline; > + } > + > + kfree(*paramp); > + *paramp = str; > + > + return 0; > + } > case MSM_PARAM_SYSPROF: > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > index 4ec62b601adc..68f3f8ade76d 100644 > --- a/drivers/gpu/drm/msm/msm_gpu.c > +++ b/drivers/gpu/drm/msm/msm_gpu.c > @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu); > > static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd) > { > + struct msm_file_private *ctx = submit->queue->ctx; > struct task_struct *task; > > + *comm = kstrdup(ctx->comm, GFP_KERNEL); > + *cmd = kstrdup(ctx->cmdline, GFP_KERNEL); > + > task = get_pid_task(submit->pid, PIDTYPE_PID); > if (!task) > return; > > - *comm = kstrdup(task->comm, GFP_KERNEL); > - *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); > + if (!*comm) > + *comm = kstrdup(task->comm, GFP_KERNEL); What? If the first allocation failed, then this one is going to fail as well. Just return -ENOMEM. Or maybe this is meant to be checking for an empty string? > + > + if (!*cmd) > + *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); Same. > > put_task_struct(task); > } regards, dan carpenter ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline 2022-03-17 8:21 ` Dan Carpenter @ 2022-03-17 15:03 ` Rob Clark -1 siblings, 0 replies; 16+ messages in thread From: Rob Clark @ 2022-03-17 15:03 UTC (permalink / raw) To: Dan Carpenter Cc: dri-devel, freedreno, linux-arm-msm, Rob Clark, Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter, Akhil P Oommen, Jonathan Marek, Jordan Crouse, Emma Anholt, open list On Thu, Mar 17, 2022 at 1:21 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Wed, Mar 16, 2022 at 05:29:45PM -0700, Rob Clark wrote: > > switch (param) { > > + case MSM_PARAM_COMM: > > + case MSM_PARAM_CMDLINE: { > > + char *str, **paramp; > > + > > + str = kmalloc(len + 1, GFP_KERNEL); > > if (!str) > return -ENOMEM; > > > + if (copy_from_user(str, u64_to_user_ptr(value), len)) { > > + kfree(str); > > + return -EFAULT; > > + } > > + > > + /* Ensure string is null terminated: */ > > + str[len] = '\0'; > > + > > + if (param == MSM_PARAM_COMM) { > > + paramp = &ctx->comm; > > + } else { > > + paramp = &ctx->cmdline; > > + } > > + > > + kfree(*paramp); > > + *paramp = str; > > + > > + return 0; > > + } > > case MSM_PARAM_SYSPROF: > > if (!capable(CAP_SYS_ADMIN)) > > return -EPERM; > > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > > index 4ec62b601adc..68f3f8ade76d 100644 > > --- a/drivers/gpu/drm/msm/msm_gpu.c > > +++ b/drivers/gpu/drm/msm/msm_gpu.c > > @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu); > > > > static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd) > > { > > + struct msm_file_private *ctx = submit->queue->ctx; > > struct task_struct *task; > > > > + *comm = kstrdup(ctx->comm, GFP_KERNEL); > > + *cmd = kstrdup(ctx->cmdline, GFP_KERNEL); > > + > > task = get_pid_task(submit->pid, PIDTYPE_PID); > > if (!task) > > return; > > > > - *comm = kstrdup(task->comm, GFP_KERNEL); > > - *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); > > + if (!*comm) > > + *comm = kstrdup(task->comm, GFP_KERNEL); > > What? > > If the first allocation failed, then this one is going to fail as well. > Just return -ENOMEM. Or maybe this is meant to be checking for an empty > string? fwiw, if ctx->comm is NULL, the kstrdup() will return NULL, so this isn't intended to deal with OoM, but the case that comm and/or cmdline is not overridden. BR, -R > > > + > > + if (!*cmd) > > + *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); > > Same. > > > > > put_task_struct(task); > > } > > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline @ 2022-03-17 15:03 ` Rob Clark 0 siblings, 0 replies; 16+ messages in thread From: Rob Clark @ 2022-03-17 15:03 UTC (permalink / raw) To: Dan Carpenter Cc: Rob Clark, freedreno, Emma Anholt, Jonathan Marek, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel, Jordan Crouse, Akhil P Oommen, Sean Paul, open list On Thu, Mar 17, 2022 at 1:21 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Wed, Mar 16, 2022 at 05:29:45PM -0700, Rob Clark wrote: > > switch (param) { > > + case MSM_PARAM_COMM: > > + case MSM_PARAM_CMDLINE: { > > + char *str, **paramp; > > + > > + str = kmalloc(len + 1, GFP_KERNEL); > > if (!str) > return -ENOMEM; > > > + if (copy_from_user(str, u64_to_user_ptr(value), len)) { > > + kfree(str); > > + return -EFAULT; > > + } > > + > > + /* Ensure string is null terminated: */ > > + str[len] = '\0'; > > + > > + if (param == MSM_PARAM_COMM) { > > + paramp = &ctx->comm; > > + } else { > > + paramp = &ctx->cmdline; > > + } > > + > > + kfree(*paramp); > > + *paramp = str; > > + > > + return 0; > > + } > > case MSM_PARAM_SYSPROF: > > if (!capable(CAP_SYS_ADMIN)) > > return -EPERM; > > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > > index 4ec62b601adc..68f3f8ade76d 100644 > > --- a/drivers/gpu/drm/msm/msm_gpu.c > > +++ b/drivers/gpu/drm/msm/msm_gpu.c > > @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu); > > > > static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd) > > { > > + struct msm_file_private *ctx = submit->queue->ctx; > > struct task_struct *task; > > > > + *comm = kstrdup(ctx->comm, GFP_KERNEL); > > + *cmd = kstrdup(ctx->cmdline, GFP_KERNEL); > > + > > task = get_pid_task(submit->pid, PIDTYPE_PID); > > if (!task) > > return; > > > > - *comm = kstrdup(task->comm, GFP_KERNEL); > > - *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); > > + if (!*comm) > > + *comm = kstrdup(task->comm, GFP_KERNEL); > > What? > > If the first allocation failed, then this one is going to fail as well. > Just return -ENOMEM. Or maybe this is meant to be checking for an empty > string? fwiw, if ctx->comm is NULL, the kstrdup() will return NULL, so this isn't intended to deal with OoM, but the case that comm and/or cmdline is not overridden. BR, -R > > > + > > + if (!*cmd) > > + *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); > > Same. > > > > > put_task_struct(task); > > } > > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline 2022-03-17 15:03 ` Rob Clark @ 2022-03-18 7:18 ` Dan Carpenter -1 siblings, 0 replies; 16+ messages in thread From: Dan Carpenter @ 2022-03-18 7:18 UTC (permalink / raw) To: Rob Clark Cc: dri-devel, freedreno, linux-arm-msm, Rob Clark, Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter, Akhil P Oommen, Jonathan Marek, Jordan Crouse, Emma Anholt, open list On Thu, Mar 17, 2022 at 08:03:59AM -0700, Rob Clark wrote: > > > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > > > index 4ec62b601adc..68f3f8ade76d 100644 > > > --- a/drivers/gpu/drm/msm/msm_gpu.c > > > +++ b/drivers/gpu/drm/msm/msm_gpu.c > > > @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu); > > > > > > static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd) > > > { > > > + struct msm_file_private *ctx = submit->queue->ctx; > > > struct task_struct *task; > > > > > > + *comm = kstrdup(ctx->comm, GFP_KERNEL); > > > + *cmd = kstrdup(ctx->cmdline, GFP_KERNEL); > > > + > > > task = get_pid_task(submit->pid, PIDTYPE_PID); > > > if (!task) > > > return; > > > > > > - *comm = kstrdup(task->comm, GFP_KERNEL); > > > - *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); > > > + if (!*comm) > > > + *comm = kstrdup(task->comm, GFP_KERNEL); > > > > What? > > > > If the first allocation failed, then this one is going to fail as well. > > Just return -ENOMEM. Or maybe this is meant to be checking for an empty > > string? > > fwiw, if ctx->comm is NULL, the kstrdup() will return NULL, so this > isn't intended to deal with OoM, but the case that comm and/or cmdline > is not overridden. Ah, I should have thought about that. Thanks! regards, dan carpenter ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline @ 2022-03-18 7:18 ` Dan Carpenter 0 siblings, 0 replies; 16+ messages in thread From: Dan Carpenter @ 2022-03-18 7:18 UTC (permalink / raw) To: Rob Clark Cc: Rob Clark, freedreno, Emma Anholt, Jonathan Marek, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel, Jordan Crouse, Akhil P Oommen, Sean Paul, open list On Thu, Mar 17, 2022 at 08:03:59AM -0700, Rob Clark wrote: > > > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > > > index 4ec62b601adc..68f3f8ade76d 100644 > > > --- a/drivers/gpu/drm/msm/msm_gpu.c > > > +++ b/drivers/gpu/drm/msm/msm_gpu.c > > > @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu); > > > > > > static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd) > > > { > > > + struct msm_file_private *ctx = submit->queue->ctx; > > > struct task_struct *task; > > > > > > + *comm = kstrdup(ctx->comm, GFP_KERNEL); > > > + *cmd = kstrdup(ctx->cmdline, GFP_KERNEL); > > > + > > > task = get_pid_task(submit->pid, PIDTYPE_PID); > > > if (!task) > > > return; > > > > > > - *comm = kstrdup(task->comm, GFP_KERNEL); > > > - *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); > > > + if (!*comm) > > > + *comm = kstrdup(task->comm, GFP_KERNEL); > > > > What? > > > > If the first allocation failed, then this one is going to fail as well. > > Just return -ENOMEM. Or maybe this is meant to be checking for an empty > > string? > > fwiw, if ctx->comm is NULL, the kstrdup() will return NULL, so this > isn't intended to deal with OoM, but the case that comm and/or cmdline > is not overridden. Ah, I should have thought about that. Thanks! regards, dan carpenter ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline 2022-03-18 7:18 ` Dan Carpenter @ 2022-03-18 15:06 ` Rob Clark -1 siblings, 0 replies; 16+ messages in thread From: Rob Clark @ 2022-03-18 15:06 UTC (permalink / raw) To: Dan Carpenter Cc: dri-devel, freedreno, linux-arm-msm, Rob Clark, Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter, Akhil P Oommen, Jonathan Marek, Jordan Crouse, Emma Anholt, open list On Fri, Mar 18, 2022 at 12:19 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Thu, Mar 17, 2022 at 08:03:59AM -0700, Rob Clark wrote: > > > > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > > > > index 4ec62b601adc..68f3f8ade76d 100644 > > > > --- a/drivers/gpu/drm/msm/msm_gpu.c > > > > +++ b/drivers/gpu/drm/msm/msm_gpu.c > > > > @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu); > > > > > > > > static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd) > > > > { > > > > + struct msm_file_private *ctx = submit->queue->ctx; > > > > struct task_struct *task; > > > > > > > > + *comm = kstrdup(ctx->comm, GFP_KERNEL); > > > > + *cmd = kstrdup(ctx->cmdline, GFP_KERNEL); > > > > + > > > > task = get_pid_task(submit->pid, PIDTYPE_PID); > > > > if (!task) > > > > return; > > > > > > > > - *comm = kstrdup(task->comm, GFP_KERNEL); > > > > - *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); > > > > + if (!*comm) > > > > + *comm = kstrdup(task->comm, GFP_KERNEL); > > > > > > What? > > > > > > If the first allocation failed, then this one is going to fail as well. > > > Just return -ENOMEM. Or maybe this is meant to be checking for an empty > > > string? > > > > fwiw, if ctx->comm is NULL, the kstrdup() will return NULL, so this > > isn't intended to deal with OoM, but the case that comm and/or cmdline > > is not overridden. > > Ah, I should have thought about that. Thanks! np, I realized it was fairly non-obvious so I added a comment in v2 to hopefully make it more clear BR, -R > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline @ 2022-03-18 15:06 ` Rob Clark 0 siblings, 0 replies; 16+ messages in thread From: Rob Clark @ 2022-03-18 15:06 UTC (permalink / raw) To: Dan Carpenter Cc: Rob Clark, freedreno, Emma Anholt, Jonathan Marek, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel, Jordan Crouse, Akhil P Oommen, Sean Paul, open list On Fri, Mar 18, 2022 at 12:19 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Thu, Mar 17, 2022 at 08:03:59AM -0700, Rob Clark wrote: > > > > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > > > > index 4ec62b601adc..68f3f8ade76d 100644 > > > > --- a/drivers/gpu/drm/msm/msm_gpu.c > > > > +++ b/drivers/gpu/drm/msm/msm_gpu.c > > > > @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu); > > > > > > > > static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd) > > > > { > > > > + struct msm_file_private *ctx = submit->queue->ctx; > > > > struct task_struct *task; > > > > > > > > + *comm = kstrdup(ctx->comm, GFP_KERNEL); > > > > + *cmd = kstrdup(ctx->cmdline, GFP_KERNEL); > > > > + > > > > task = get_pid_task(submit->pid, PIDTYPE_PID); > > > > if (!task) > > > > return; > > > > > > > > - *comm = kstrdup(task->comm, GFP_KERNEL); > > > > - *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL); > > > > + if (!*comm) > > > > + *comm = kstrdup(task->comm, GFP_KERNEL); > > > > > > What? > > > > > > If the first allocation failed, then this one is going to fail as well. > > > Just return -ENOMEM. Or maybe this is meant to be checking for an empty > > > string? > > > > fwiw, if ctx->comm is NULL, the kstrdup() will return NULL, so this > > isn't intended to deal with OoM, but the case that comm and/or cmdline > > is not overridden. > > Ah, I should have thought about that. Thanks! np, I realized it was fairly non-obvious so I added a comment in v2 to hopefully make it more clear BR, -R > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-03-18 15:06 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-03-17 0:29 [PATCH 0/3] drm/msm: Add comm/cmdline override Rob Clark 2022-03-17 0:29 ` Rob Clark 2022-03-17 0:29 ` [PATCH 1/3] drm/msm: Add support for pointer params Rob Clark 2022-03-17 0:29 ` Rob Clark 2022-03-17 0:29 ` [PATCH 2/3] drm/msm: Split out helper to get comm/cmdline Rob Clark 2022-03-17 0:29 ` Rob Clark 2022-03-17 0:29 ` [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline Rob Clark 2022-03-17 0:29 ` Rob Clark 2022-03-17 8:21 ` Dan Carpenter 2022-03-17 8:21 ` Dan Carpenter 2022-03-17 15:03 ` Rob Clark 2022-03-17 15:03 ` Rob Clark 2022-03-18 7:18 ` Dan Carpenter 2022-03-18 7:18 ` Dan Carpenter 2022-03-18 15:06 ` Rob Clark 2022-03-18 15:06 ` Rob Clark
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.