Re: [PATCH v1 2/3] drm/panel: st7701: Add support for SPI for configuration
Hi Hironori, 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/Hironori-KIKUCHI/dt-bindings-display-st7701-Add-Anbernic-RG28XX-panel/20240618-161849 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next patch link: https://lore.kernel.org/r/20240618081515.1215552-3-kikuchan98%40gmail.com patch subject: [PATCH v1 2/3] drm/panel: st7701: Add support for SPI for configuration config: i386-randconfig-141-20240621 (https://download.01.org/0day-ci/archive/20240621/202406210841.q17g5isz-...@intel.com/config) compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0 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/202406210841.q17g5isz-...@intel.com/ smatch warnings: drivers/gpu/drm/panel/panel-sitronix-st7701.c:1076 st7701_dsi_probe() warn: passing zero to 'dev_err_probe' vim +/dev_err_probe +1076 drivers/gpu/drm/panel/panel-sitronix-st7701.c 433b7d46874577 Hironori KIKUCHI 2024-06-18 1062 static int st7701_dsi_probe(struct mipi_dsi_device *dsi) 433b7d46874577 Hironori KIKUCHI 2024-06-18 1063 { 433b7d46874577 Hironori KIKUCHI 2024-06-18 1064struct st7701 *st7701; 433b7d46874577 Hironori KIKUCHI 2024-06-18 1065int err; 433b7d46874577 Hironori KIKUCHI 2024-06-18 1066 433b7d46874577 Hironori KIKUCHI 2024-06-18 1067err = st7701_probe(>dev, DRM_MODE_CONNECTOR_DSI); 433b7d46874577 Hironori KIKUCHI 2024-06-18 1068if (err) 433b7d46874577 Hironori KIKUCHI 2024-06-18 1069return err; 433b7d46874577 Hironori KIKUCHI 2024-06-18 1070 433b7d46874577 Hironori KIKUCHI 2024-06-18 1071st7701 = dev_get_drvdata(>dev); 849b2e3ff96982 Jagan Teki 2019-01-25 1072st7701->dsi = dsi; 433b7d46874577 Hironori KIKUCHI 2024-06-18 1073st7701->write_command = st7701_dsi_write; 849b2e3ff96982 Jagan Teki 2019-01-25 1074 433b7d46874577 Hironori KIKUCHI 2024-06-18 1075if (!st7701->desc->lanes) 433b7d46874577 Hironori KIKUCHI 2024-06-18 @1076return dev_err_probe(>dev, err, "This panel is not for MIPI DSI\n"); ^^^ -EINVAL? 433b7d46874577 Hironori KIKUCHI 2024-06-18 1077 433b7d46874577 Hironori KIKUCHI 2024-06-18 1078dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST | 433b7d46874577 Hironori KIKUCHI 2024-06-18 1079 MIPI_DSI_MODE_LPM | MIPI_DSI_CLOCK_NON_CONTINUOUS; 433b7d46874577 Hironori KIKUCHI 2024-06-18 1080dsi->format = st7701->desc->format; 433b7d46874577 Hironori KIKUCHI 2024-06-18 1081dsi->lanes = st7701->desc->lanes; 433b7d46874577 Hironori KIKUCHI 2024-06-18 1082 433b7d46874577 Hironori KIKUCHI 2024-06-18 1083 mipi_dsi_set_drvdata(dsi, st7701); 433b7d46874577 Hironori KIKUCHI 2024-06-18 1084 433b7d46874577 Hironori KIKUCHI 2024-06-18 1085err = mipi_dsi_attach(dsi); 433b7d46874577 Hironori KIKUCHI 2024-06-18 1086if (err) 433b7d46874577 Hironori KIKUCHI 2024-06-18 1087return dev_err_probe(>dev, err, "Failed to init MIPI DSI\n"); c62102165dd792 Marek Vasut 2022-10-15 1088 c62102165dd792 Marek Vasut 2022-10-15 1089return 0; 433b7d46874577 Hironori KIKUCHI 2024-06-18 1090 } -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
[bug report] drm: atmel-hlcdc: add driver ops to differentiate HLCDC and XLCDC IP
Hello Manikandan Muralidharan, Commit aa71584b323a ("drm: atmel-hlcdc: add driver ops to differentiate HLCDC and XLCDC IP") from Apr 24, 2024 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c:573 atmel_hlcdc_plane_update_buffers() error: uninitialized symbol 'sr'. drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c 556 static void atmel_hlcdc_plane_update_buffers(struct atmel_hlcdc_plane *plane, 557 struct atmel_hlcdc_plane_state *state) 558 { 559 const struct atmel_hlcdc_layer_desc *desc = plane->layer.desc; 560 struct atmel_hlcdc_dc *dc = plane->base.dev->dev_private; 561 struct drm_framebuffer *fb = state->base.fb; 562 u32 sr; 563 int i; 564 565 if (!dc->desc->is_xlcdc) 566 sr = atmel_hlcdc_layer_read_reg(>layer, ATMEL_HLCDC_LAYER_CHSR); sr is uninitialized on else path. 567 568 for (i = 0; i < state->nplanes; i++) { 569 struct drm_gem_dma_object *gem = drm_fb_dma_get_gem_obj(fb, i); 570 571 state->dscrs[i]->addr = gem->dma_addr + state->offsets[i]; 572 --> 573 dc->desc->ops->lcdc_update_buffers(plane, state, sr, i); ^^ Uninitialized. 574 575 if (desc->layout.xstride[i]) 576 atmel_hlcdc_layer_write_cfg(>layer, 577 desc->layout.xstride[i], 578 state->xstride[i]); 579 580 if (desc->layout.pstride[i]) 581 atmel_hlcdc_layer_write_cfg(>layer, 582 desc->layout.pstride[i], 583 state->pstride[i]); 584 } 585 } regards, dan carpenter
[PATCH] drm/amd/display: Clean up indenting in dm_dp_mst_is_port_support_mode()
This code works, but it's not aligned correctly. Add a couple missing tabs. Signed-off-by: Dan Carpenter --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c index 48118447c8d9..5d4f831b1e55 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c @@ -1691,7 +1691,7 @@ enum dc_status dm_dp_mst_is_port_support_mode( if (aconnector->mst_output_port->passthrough_aux) { if (bw_range.min_kbps > end_to_end_bw_in_kbps) { DRM_DEBUG_DRIVER("DSC passthrough. Max dsc compression can't fit into end-to-end bw\n"); - return DC_FAIL_BANDWIDTH_VALIDATE; + return DC_FAIL_BANDWIDTH_VALIDATE; } } else { /*dsc bitstream decoded at the dp last link*/ @@ -1756,7 +1756,7 @@ enum dc_status dm_dp_mst_is_port_support_mode( if (branch_max_throughput_mps != 0 && ((stream->timing.pix_clk_100hz / 10) > branch_max_throughput_mps * 1000)) { DRM_DEBUG_DRIVER("DSC is required but max throughput mps fails"); - return DC_FAIL_BANDWIDTH_VALIDATE; + return DC_FAIL_BANDWIDTH_VALIDATE; } } else { DRM_DEBUG_DRIVER("DSC is required but can't find common dsc config."); -- 2.43.0
[PATCH] drm/amdgpu/kfd: Add unlock() on error path to add_queue_mes()
We recently added locking to add_queue_mes() but this error path was overlooked. Add an unlock to the error path. Fixes: 1802b042a343 ("drm/amdgpu/kfd: remove is_hws_hang and is_resetting") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c index d2fceb6f9802..4f48507418d2 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c @@ -230,6 +230,7 @@ static int add_queue_mes(struct device_queue_manager *dqm, struct queue *q, if (queue_type < 0) { dev_err(adev->dev, "Queue type not supported with MES, queue:%d\n", q->properties.type); + up_read(>reset_domain->sem); return -EINVAL; } queue_input.queue_type = (uint32_t)queue_type; -- 2.43.0
Re: [PATCH] drm/nouveau: Use kmemdup_array() instead of kmemdup()
On Mon, Jun 17, 2024 at 05:55:33PM +0200, Danilo Krummrich wrote: > On 6/17/24 11:33, Dan Carpenter wrote: > > Use kmemdup_array() because we're allocating an array. > > > > The main difference between kmemdup() and kmemdup_array() is that the > > kmemdup_array() function has integer overflow checking built it. The > > "args->in_sync.count" variable is a u32 so integer overflows would only > > be a concern on 32bit systems. Fortunately, however, the u_memcpya() > > function has integer overflow checking which means that it is not an > > issue. > > > > Still using kmemdup_array() is more appropriate and makes auditing the > > code easier. > > Indeed, we shouldn't rely on the previous check here, good catch. > > > > > Signed-off-by: Dan Carpenter > > --- > > drivers/gpu/drm/nouveau/nouveau_sched.c | 16 > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.c > > b/drivers/gpu/drm/nouveau/nouveau_sched.c > > index 32fa2e273965..53d8b0584a56 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_sched.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_sched.c > > @@ -45,10 +45,10 @@ nouveau_job_init(struct nouveau_job *job, > > if (job->sync) > > return -EINVAL; > > - job->in_sync.data = kmemdup(args->in_sync.s, > > -sizeof(*args->in_sync.s) * > > -args->in_sync.count, > > -GFP_KERNEL); > > + job->in_sync.data = kmemdup_array(args->in_sync.s, > > + args->in_sync.count, > > + sizeof(*args->in_sync.s), > > + GFP_KERNEL); > > if (!job->in_sync.data) > > return -ENOMEM; > > Not sure if this is what we want for kmemdup_array(). It just saturates the > size. This doesn't prevent accessing the array out of bounds later on. I mean, > it's rather unlikely to get such a huge amount of physically contiguous memory > anyways, but wouldn't it be cleaner to let kmemdup_array() return > ERR_PTR(-EOVERFLOW) on overflow, just like memdup_array_user()[1]? > > AFAICS, there's just two users of kmemdup_array(), hence it should be an easy > change. :-) > > [1] https://elixir.bootlin.com/linux/latest/source/include/linux/string.h#L30 > We can't change the return values. kmemdup_array() has to match kmemdup(). <-- returns NULL memdup_array_user() has to match memdup_user(). <-- returns error pointer regards, dan carpenter
Re: [PATCH v4 08/12] drm/ttm: Add a virtual base class for graphics memory backup
Hi Thomas, 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/Thomas-Hellstr-m/drm-ttm-Allow-TTM-LRU-list-nodes-of-different-types/20240614-182911 base: https://gitlab.freedesktop.org/drm/xe/kernel.git drm-xe-next patch link: https://lore.kernel.org/r/20240614102548.4364-9-thomas.hellstrom%40linux.intel.com patch subject: [PATCH v4 08/12] drm/ttm: Add a virtual base class for graphics memory backup config: x86_64-randconfig-161-20240617 (https://download.01.org/0day-ci/archive/20240617/202406170559.wddkfeiu-...@intel.com/config) compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0 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/202406170559.wddkfeiu-...@intel.com/ smatch warnings: drivers/gpu/drm/ttm/ttm_backup_shmem.c:130 ttm_backup_shmem_create() error: dereferencing freed memory 'sbackup' vim +/sbackup +130 drivers/gpu/drm/ttm/ttm_backup_shmem.c 827540d42dec01 Thomas Hellström 2024-06-14 119 struct ttm_backup *ttm_backup_shmem_create(loff_t size) 827540d42dec01 Thomas Hellström 2024-06-14 120 { 827540d42dec01 Thomas Hellström 2024-06-14 121 struct ttm_backup_shmem *sbackup = 827540d42dec01 Thomas Hellström 2024-06-14 122 kzalloc(sizeof(*sbackup), GFP_KERNEL | __GFP_ACCOUNT); 827540d42dec01 Thomas Hellström 2024-06-14 123 827540d42dec01 Thomas Hellström 2024-06-14 124 if (!sbackup) 827540d42dec01 Thomas Hellström 2024-06-14 125 return ERR_PTR(-ENOMEM); 827540d42dec01 Thomas Hellström 2024-06-14 126 827540d42dec01 Thomas Hellström 2024-06-14 127 sbackup->filp = shmem_file_setup("ttm shmem backup", size, 0); 827540d42dec01 Thomas Hellström 2024-06-14 128 if (IS_ERR(sbackup->filp)) { 827540d42dec01 Thomas Hellström 2024-06-14 129 kfree(sbackup); ^^^ Freed 827540d42dec01 Thomas Hellström 2024-06-14 @130 return ERR_CAST(sbackup->filp); ^ Use after free -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
[PATCH] drm/nouveau: Use kmemdup_array() instead of kmemdup()
Use kmemdup_array() because we're allocating an array. The main difference between kmemdup() and kmemdup_array() is that the kmemdup_array() function has integer overflow checking built it. The "args->in_sync.count" variable is a u32 so integer overflows would only be a concern on 32bit systems. Fortunately, however, the u_memcpya() function has integer overflow checking which means that it is not an issue. Still using kmemdup_array() is more appropriate and makes auditing the code easier. Signed-off-by: Dan Carpenter --- drivers/gpu/drm/nouveau/nouveau_sched.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.c b/drivers/gpu/drm/nouveau/nouveau_sched.c index 32fa2e273965..53d8b0584a56 100644 --- a/drivers/gpu/drm/nouveau/nouveau_sched.c +++ b/drivers/gpu/drm/nouveau/nouveau_sched.c @@ -45,10 +45,10 @@ nouveau_job_init(struct nouveau_job *job, if (job->sync) return -EINVAL; - job->in_sync.data = kmemdup(args->in_sync.s, -sizeof(*args->in_sync.s) * -args->in_sync.count, -GFP_KERNEL); + job->in_sync.data = kmemdup_array(args->in_sync.s, + args->in_sync.count, + sizeof(*args->in_sync.s), + GFP_KERNEL); if (!job->in_sync.data) return -ENOMEM; } @@ -60,10 +60,10 @@ nouveau_job_init(struct nouveau_job *job, goto err_free_in_sync; } - job->out_sync.data = kmemdup(args->out_sync.s, - sizeof(*args->out_sync.s) * - args->out_sync.count, - GFP_KERNEL); + job->out_sync.data = kmemdup_array(args->out_sync.s, + args->out_sync.count, + sizeof(*args->out_sync.s), + GFP_KERNEL); if (!job->out_sync.data) { ret = -ENOMEM; goto err_free_in_sync; -- 2.43.0
[PATCH v2] drm/bridge: it6505: remove unnecessary NULL checks
Smatch complains correctly that the NULL checking isn't consistent: drivers/gpu/drm/bridge/ite-it6505.c:2583 it6505_poweron() error: we previously assumed 'pdata->pwr18' could be null (see line 2569) These the ->pwr18 pointer is allocated during probe using the devm_regulator_get() function. If CONFIG_REGULATOR is disabled then, yes, it can return NULL but in that case regulator_enable() and disable() functions are no-ops so passing a NULL pointer is okay. Smatch is correct that the NULL checks are inconsistent but the fix is to delete the unnecessary NULL checking. Do the same for "pdata->ovdd" as well. Signed-off-by: Dan Carpenter --- v2: In the first patch, I added a NULL check but that wasn't necessary or correct. drivers/gpu/drm/bridge/ite-it6505.c | 43 - 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c index 3f68c82888c2..d89d1bb9a8ec 100644 --- a/drivers/gpu/drm/bridge/ite-it6505.c +++ b/drivers/gpu/drm/bridge/ite-it6505.c @@ -2566,24 +2566,21 @@ static int it6505_poweron(struct it6505 *it6505) return 0; } - if (pdata->pwr18) { - err = regulator_enable(pdata->pwr18); - if (err) { - DRM_DEV_DEBUG_DRIVER(dev, "Failed to enable VDD18: %d", -err); - return err; - } + err = regulator_enable(pdata->pwr18); + if (err) { + DRM_DEV_DEBUG_DRIVER(dev, "Failed to enable VDD18: %d", +err); + return err; } - if (pdata->ovdd) { - /* time interval between IVDD and OVDD at least be 1ms */ - usleep_range(1000, 2000); - err = regulator_enable(pdata->ovdd); - if (err) { - regulator_disable(pdata->pwr18); - return err; - } + /* time interval between IVDD and OVDD at least be 1ms */ + usleep_range(1000, 2000); + err = regulator_enable(pdata->ovdd); + if (err) { + regulator_disable(pdata->pwr18); + return err; } + /* time interval between OVDD and SYSRSTN at least be 10ms */ if (pdata->gpiod_reset) { usleep_range(1, 2); @@ -2618,17 +2615,13 @@ static int it6505_poweroff(struct it6505 *it6505) if (pdata->gpiod_reset) gpiod_set_value_cansleep(pdata->gpiod_reset, 0); - if (pdata->pwr18) { - err = regulator_disable(pdata->pwr18); - if (err) - return err; - } + err = regulator_disable(pdata->pwr18); + if (err) + return err; - if (pdata->ovdd) { - err = regulator_disable(pdata->ovdd); - if (err) - return err; - } + err = regulator_disable(pdata->ovdd); + if (err) + return err; it6505->powered = false; it6505->sink_count = 0; -- 2.39.2
Re: [PATCH] drm/bridge: it6505: Fix potential NULL dereference
On Sun, Jun 09, 2024 at 10:38:39PM +0300, Dmitry Baryshkov wrote: > On Sat, Jun 08, 2024 at 05:21:08PM +0300, Dan Carpenter wrote: > > Smatch complains correctly that the NULL checking isn't consistent: > > > > drivers/gpu/drm/bridge/ite-it6505.c:2583 it6505_poweron() > > error: we previously assumed 'pdata->pwr18' could be null > > (see line 2569) > > > > Add a NULL check to prevent a NULL dereference on the error path. > > > > Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver") > > Signed-off-by: Dan Carpenter > > --- > > drivers/gpu/drm/bridge/ite-it6505.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c > > b/drivers/gpu/drm/bridge/ite-it6505.c > > index 3f68c82888c2..4f01fadaec0f 100644 > > --- a/drivers/gpu/drm/bridge/ite-it6505.c > > +++ b/drivers/gpu/drm/bridge/ite-it6505.c > > @@ -2580,7 +2580,8 @@ static int it6505_poweron(struct it6505 *it6505) > > usleep_range(1000, 2000); > > err = regulator_enable(pdata->ovdd); > > if (err) { > > - regulator_disable(pdata->pwr18); > > + if (pdata->pwr18) > > + regulator_disable(pdata->pwr18); > > Wait... I wat too quick to R-B it. The driver uses devm_regulator_get(), > which always returns non-NULL result. So all `if (pdata->pwr18)` and > `if (pdata->ovdd)` checks in the driver are useless. Could you please > send a patch, removing them? > Sure. Will do. regards, dan carpenter
[bug report] drm/xe/bo: Simplify xe_bo_lock()
Hello Thomas Hellström, Commit 08a4f00e62bc ("drm/xe/bo: Simplify xe_bo_lock()") from Sep 8, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/xe/xe_vm.c:2095 vm_bind_ioctl_ops_create() error: we previously assumed 'bo' could be null (see line 2067) drivers/gpu/drm/xe/xe_vm.c 2061 static struct drm_gpuva_ops * 2062 vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_bo *bo, 2063 u64 bo_offset_or_userptr, u64 addr, u64 range, 2064 u32 operation, u32 flags, 2065 u32 prefetch_region, u16 pat_index) 2066 { 2067 struct drm_gem_object *obj = bo ? >ttm.base : NULL; ^^ So far as I can see bo is always valid. No need to check here. 2068 struct drm_gpuva_ops *ops; 2069 struct drm_gpuva_op *__op; 2070 struct drm_gpuvm_bo *vm_bo; 2071 int err; 2072 2073 lockdep_assert_held_write(>lock); 2074 2075 vm_dbg(>xe->drm, 2076"op=%d, addr=0x%016llx, range=0x%016llx, bo_offset_or_userptr=0x%016llx", 2077operation, (ULL)addr, (ULL)range, 2078(ULL)bo_offset_or_userptr); 2079 2080 switch (operation) { 2081 case DRM_XE_VM_BIND_OP_MAP: 2082 case DRM_XE_VM_BIND_OP_MAP_USERPTR: 2083 ops = drm_gpuvm_sm_map_ops_create(>gpuvm, addr, range, 2084 obj, bo_offset_or_userptr); 2085 break; 2086 case DRM_XE_VM_BIND_OP_UNMAP: 2087 ops = drm_gpuvm_sm_unmap_ops_create(>gpuvm, addr, range); 2088 break; 2089 case DRM_XE_VM_BIND_OP_PREFETCH: 2090 ops = drm_gpuvm_prefetch_ops_create(>gpuvm, addr, range); 2091 break; 2092 case DRM_XE_VM_BIND_OP_UNMAP_ALL: 2093 xe_assert(vm->xe, bo); 2094 --> 2095 err = xe_bo_lock(bo, true); ^^ Unchecked dereference here. 2096 if (err) 2097 return ERR_PTR(err); 2098 2099 vm_bo = drm_gpuvm_bo_obtain(>gpuvm, obj); 2100 if (IS_ERR(vm_bo)) { 2101 xe_bo_unlock(bo); 2102 return ERR_CAST(vm_bo); 2103 } 2104 2105 ops = drm_gpuvm_bo_unmap_ops_create(vm_bo); 2106 drm_gpuvm_bo_put(vm_bo); 2107 xe_bo_unlock(bo); 2108 break; 2109 default: 2110 drm_warn(>xe->drm, "NOT POSSIBLE"); 2111 ops = ERR_PTR(-EINVAL); 2112 } 2113 if (IS_ERR(ops)) 2114 return ops; 2115 2116 drm_gpuva_for_each_op(__op, ops) { 2117 struct xe_vma_op *op = gpuva_op_to_vma_op(__op); 2118 2119 if (__op->op == DRM_GPUVA_OP_MAP) { 2120 op->map.immediate = 2121 flags & DRM_XE_VM_BIND_FLAG_IMMEDIATE; 2122 op->map.read_only = 2123 flags & DRM_XE_VM_BIND_FLAG_READONLY; 2124 op->map.is_null = flags & DRM_XE_VM_BIND_FLAG_NULL; 2125 op->map.dumpable = flags & DRM_XE_VM_BIND_FLAG_DUMPABLE; 2126 op->map.pat_index = pat_index; 2127 } else if (__op->op == DRM_GPUVA_OP_PREFETCH) { 2128 op->prefetch.region = prefetch_region; 2129 } 2130 2131 print_op(vm->xe, __op); 2132 } 2133 2134 return ops; 2135 } regards, dan carpenter
[bug report] drm/xe: Introduce a new DRM driver for Intel GPUs
), 673 xe_vma_size(vma), ); 674 else 675 xe_res_first_sg(xe_bo_sg(bo), xe_vma_bo_offset(vma), ^^ More unchecked dereferences 676 xe_vma_size(vma), ); 677 } else { 678 curs.size = xe_vma_size(vma); 679 } 680 681 ret = xe_pt_walk_range(>base, pt->level, xe_vma_start(vma), 682xe_vma_end(vma), _walk.base); 683 684 *num_entries = xe_walk.wupd.num_used_entries; 685 return ret; 686 } regards, dan carpenter
[PATCH] drm/bridge: it6505: Fix potential NULL dereference
Smatch complains correctly that the NULL checking isn't consistent: drivers/gpu/drm/bridge/ite-it6505.c:2583 it6505_poweron() error: we previously assumed 'pdata->pwr18' could be null (see line 2569) Add a NULL check to prevent a NULL dereference on the error path. Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/bridge/ite-it6505.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c index 3f68c82888c2..4f01fadaec0f 100644 --- a/drivers/gpu/drm/bridge/ite-it6505.c +++ b/drivers/gpu/drm/bridge/ite-it6505.c @@ -2580,7 +2580,8 @@ static int it6505_poweron(struct it6505 *it6505) usleep_range(1000, 2000); err = regulator_enable(pdata->ovdd); if (err) { - regulator_disable(pdata->pwr18); + if (pdata->pwr18) + regulator_disable(pdata->pwr18); return err; } } -- 2.43.0
[bug report] drm/meson: fix unbind path if HDMI fails to bind
} 283 } 284 285 /* 286 * Remove early framebuffers (ie. simplefb). The framebuffer can be 287 * located anywhere in RAM 288 */ 289 ret = drm_aperture_remove_framebuffers(_driver); 290 if (ret) 291 goto free_drm; 292 293 ret = drmm_mode_config_init(drm); 294 if (ret) 295 goto free_drm; 296 drm->mode_config.max_width = 3840; 297 drm->mode_config.max_height = 2160; 298 drm->mode_config.funcs = _mode_config_funcs; 299 drm->mode_config.helper_private= _mode_config_helpers; 300 301 /* Hardware Initialization */ 302 303 meson_vpu_init(priv); 304 meson_venc_init(priv); 305 meson_vpp_init(priv); 306 meson_viu_init(priv); 307 if (priv->afbcd.ops) { 308 ret = priv->afbcd.ops->init(priv); 309 if (ret) 310 goto free_drm; 311 } 312 313 /* Encoder Initialization */ 314 315 ret = meson_encoder_cvbs_probe(priv); 316 if (ret) 317 goto exit_afbcd; 318 319 if (has_components) { 320 ret = component_bind_all(dev, drm); 321 if (ret) { 322 dev_err(drm->dev, "Couldn't bind all components\n"); 323 /* Do not try to unbind */ 324 has_components = false; 325 goto exit_afbcd; 326 } 327 } 328 329 ret = meson_encoder_hdmi_probe(priv); 330 if (ret) 331 goto exit_afbcd; 332 333 if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) { 334 ret = meson_encoder_dsi_probe(priv); 335 if (ret) 336 goto exit_afbcd; 337 } 338 339 ret = meson_plane_create(priv); 340 if (ret) 341 goto exit_afbcd; 342 343 ret = meson_overlay_create(priv); 344 if (ret) 345 goto exit_afbcd; 346 347 ret = meson_crtc_create(priv); 348 if (ret) 349 goto exit_afbcd; 350 351 ret = request_irq(priv->vsync_irq, meson_irq, 0, drm->driver->name, drm); 352 if (ret) 353 goto exit_afbcd; 354 355 drm_mode_config_reset(drm); 356 357 drm_kms_helper_poll_init(drm); 358 359 platform_set_drvdata(pdev, priv); 360 361 ret = drm_dev_register(drm, 0); 362 if (ret) 363 goto uninstall_irq; 364 365 drm_fbdev_dma_setup(drm, 32); 366 367 return 0; 368 369 uninstall_irq: 370 free_irq(priv->vsync_irq, drm); 371 exit_afbcd: 372 if (priv->afbcd.ops) 373 priv->afbcd.ops->exit(priv); 374 free_drm: 375 drm_dev_put(drm); 376 --> 377 meson_encoder_dsi_remove(priv); 378 meson_encoder_hdmi_remove(priv); 379 meson_encoder_cvbs_remove(priv); 380 381 if (has_components) 382 component_unbind_all(dev, drm); 383 384 return ret; 385 } regards, dan carpenter
[PATCH] backlight: lm3509_bl: Fix NULL vs IS_ERR() check in register() function
The devm_backlight_device_register() doesn't return NULL, it returns error pointers. Update the error checking to match. Fixes: b72755f5b577 ("backlight: Add new lm3509 backlight driver") Signed-off-by: Dan Carpenter --- drivers/video/backlight/lm3509_bl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/video/backlight/lm3509_bl.c b/drivers/video/backlight/lm3509_bl.c index ab57f79ffe23..c93cdedff5ad 100644 --- a/drivers/video/backlight/lm3509_bl.c +++ b/drivers/video/backlight/lm3509_bl.c @@ -114,9 +114,10 @@ lm3509_backlight_register(struct device *dev, const char *name_suffix, } bd = devm_backlight_device_register(dev, label, dev, data, ops, ); - if (bd) - backlight_update_status(bd); + if (IS_ERR(bd)) + return bd; + backlight_update_status(bd); return bd; } -- 2.43.0
Re: [PATCH v2 3/3] drm/panic: Add a kmsg panic screen
Hi Jocelyn, kernel test robot noticed the following build warnings: url: https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-panic-only-draw-the-foreground-color-in-drm_panic_blit/20240603-181247 base: 86266829ea755f737762ebda614c59b136c8feac patch link: https://lore.kernel.org/r/20240603095343.39588-4-jfalempe%40redhat.com patch subject: [PATCH v2 3/3] drm/panic: Add a kmsg panic screen config: i386-randconfig-141-20240604 (https://download.01.org/0day-ci/archive/20240604/202406041051.kuvqtzcd-...@intel.com/config) compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0 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/202406041051.kuvqtzcd-...@intel.com/ smatch warnings: drivers/gpu/drm/drm_panic.c:531 draw_panic_static_kmsg() warn: variable dereferenced before check 'font' (see line 529) vim +/font +531 drivers/gpu/drm/drm_panic.c c259bba1e69ff2 Jocelyn Falempe 2024-06-03 519 static void draw_panic_static_kmsg(struct drm_scanout_buffer *sb) c259bba1e69ff2 Jocelyn Falempe 2024-06-03 520 { c259bba1e69ff2 Jocelyn Falempe 2024-06-03 521 u32 fg_color = convert_from_xrgb(CONFIG_DRM_PANIC_FOREGROUND_COLOR, sb->format->format); c259bba1e69ff2 Jocelyn Falempe 2024-06-03 522 u32 bg_color = convert_from_xrgb(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format); c259bba1e69ff2 Jocelyn Falempe 2024-06-03 523 const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL); c259bba1e69ff2 Jocelyn Falempe 2024-06-03 524 struct drm_rect r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height); c259bba1e69ff2 Jocelyn Falempe 2024-06-03 525 struct kmsg_dump_iter iter; c259bba1e69ff2 Jocelyn Falempe 2024-06-03 526 char kmsg_buf[512]; c259bba1e69ff2 Jocelyn Falempe 2024-06-03 527 size_t kmsg_len; c259bba1e69ff2 Jocelyn Falempe 2024-06-03 528 struct drm_panic_line line; c259bba1e69ff2 Jocelyn Falempe 2024-06-03 @529 int yoffset = sb->height - font->height - (sb->height % font->height) / 2; Unchecked dereferences c259bba1e69ff2 Jocelyn Falempe 2024-06-03 530 c259bba1e69ff2 Jocelyn Falempe 2024-06-03 @531 if (!font) Checked too late c259bba1e69ff2 Jocelyn Falempe 2024-06-03 532 return; c259bba1e69ff2 Jocelyn Falempe 2024-06-03 533 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Re: [PATCH v3 7/9] staging: media: atmel: use for_each_endpoint_of_node()
On Thu, May 30, 2024 at 02:06:22AM +, Kuninori Morimoto wrote: > We already have for_each_endpoint_of_node(), don't use > of_graph_get_next_endpoint() directly. Replace it. > > Signed-off-by: Kuninori Morimoto > Reviewed-by: Laurent Pinchart > --- > .../staging/media/deprecated/atmel/atmel-sama5d2-isc.c| 8 ++-- > .../staging/media/deprecated/atmel/atmel-sama7g5-isc.c| 8 ++-- > 2 files changed, 4 insertions(+), 12 deletions(-) > > diff --git a/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c > b/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c > index 31b2b48085c59..3b28a232418a9 100644 > --- a/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c > +++ b/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c > @@ -333,20 +333,16 @@ static const u32 > isc_sama5d2_gamma_table[][GAMMA_ENTRIES] = { > static int isc_parse_dt(struct device *dev, struct isc_device *isc) > { > struct device_node *np = dev->of_node; > - struct device_node *epn = NULL; > + struct device_node *epn; > struct isc_subdev_entity *subdev_entity; > unsigned int flags; > int ret; > > INIT_LIST_HEAD(>subdev_entities); > > - while (1) { > + for_each_endpoint_of_node(np, epn) { > struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 }; > > - epn = of_graph_get_next_endpoint(np, epn); > - if (!epn) > - return 0; > - > ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn), >_epn); > if (ret) { This introduces a Smatch warning because now "ret" is uninitialized if the for_each_endpoint_of_node() list is empty. Is that something which is possible? I've been meaning to make a list of loops which always iterate at least one time. for_each_cpu() etc. regards, dan carpenter
Re: [PATCH v3 4/9] media: platform: microchip: use for_each_endpoint_of_node()
On Thu, May 30, 2024 at 02:06:05AM +, Kuninori Morimoto wrote: > diff --git a/drivers/media/platform/microchip/microchip-sama5d2-isc.c > b/drivers/media/platform/microchip/microchip-sama5d2-isc.c > index 5ac149cf3647f..60b6d922d764e 100644 > --- a/drivers/media/platform/microchip/microchip-sama5d2-isc.c > +++ b/drivers/media/platform/microchip/microchip-sama5d2-isc.c > @@ -353,33 +353,29 @@ static const u32 > isc_sama5d2_gamma_table[][GAMMA_ENTRIES] = { > static int isc_parse_dt(struct device *dev, struct isc_device *isc) > { > struct device_node *np = dev->of_node; > - struct device_node *epn = NULL; > + struct device_node *epn; > struct isc_subdev_entity *subdev_entity; > unsigned int flags; > - int ret; > > INIT_LIST_HEAD(>subdev_entities); > > - while (1) { > + for_each_endpoint_of_node(np, epn) { > struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 }; > - > - epn = of_graph_get_next_endpoint(np, epn); > - if (!epn) > - return 0; > + int ret; > > ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn), >_epn); > if (ret) { > - ret = -EINVAL; > + of_node_put(epn); > dev_err(dev, "Could not parse the endpoint\n"); > - break; > + return -EINVAL; > } > > subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity), >GFP_KERNEL); > if (!subdev_entity) { > - ret = -ENOMEM; > - break; > + of_node_put(epn); > + return -ENOMEM; > } > subdev_entity->epn = epn; This code is an example of what Laurent was talking about. We're taking storing "subdev_entity->epn = epn" but then not incrementing the refcount. Perhaps it's not necessary? The difference between this and _scoped() would be if we stored epn and then returned. I feel like that's less common. We could detect that sort of thing using static analysis if it turned out to be an issue. regards, dan carpenter
Re: [PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node()
On Wed, May 29, 2024 at 05:52:53PM +0300, Laurent Pinchart wrote: > On Wed, May 29, 2024 at 05:34:41PM +0300, Dan Carpenter wrote: > > On Wed, May 29, 2024 at 03:40:47AM +0300, Laurent Pinchart wrote: > > > > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct > > > > device *dev, > > > > } > > > > > > > > /* Iterate through each output port to discover topology */ > > > > - while ((ep = of_graph_get_next_endpoint(parent, ep))) { > > > > + for_each_endpoint_of_node(parent, ep) { > > > > /* > > > > * Legacy binding mixes input/output ports under the > > > > * same parent. So, skip the input ports if we are > > > > dealing > > > > > > I think there's a bug below. The loop contains > > > > > > ret = of_coresight_parse_endpoint(dev, ep, pdata); > > > if (ret) > > > return ret; > > > > > > which leaks the reference to ep. This is not introduced by this patch, > > > > Someone should create for_each_endpoint_of_node_scoped(). > > > > #define for_each_endpoint_of_node_scoped(parent, child) \ > > for (struct device_node *child __free(device_node) = \ > > of_graph_get_next_endpoint(parent, NULL); child != NULL; \ > > child = of_graph_get_next_endpoint(parent, child)) > > I was thinking about that too :-) I wondered if we should then bother > taking and releasing references, given that references to the children > can't be leaked out of the loop. My reasoning was that the parent > device_node is guaranteed to be valid throughout the loop, so borrowing > references to children instead of creating new ones within the loop > should be fine. This assumes that endpoints and ports can't vanish while > the parent is there. Thinking further about it, it may not be a safe > assumption for the future. As we anyway use functions internally that > create new references, we can as well handle them correctly. > > Using this new macro, the loop body would need to call of_node_get() if > it wants to get a reference out of the loop. The child pointer is declared local to just the loop so you'd need create a different function scoped variable. If it's not local to the loop then we'd end up taking a reference on each iteration and never releasing anything except on error paths. > That's the right thing to > do, and I think it would be less error-prone than having to drop > references when exiting from the loop as we do today. It would still be > nice if we could have an API that allows catching this missing > of_node_get() automatically, but I don't see a simple way to do so at > the moment. That's an interesting point. If we did "function_scope_var = ep;" here then we'd need to take a second reference as you say. With other cleanup stuff like kfree() it's very hard to miss it if we forget to call "no_free_ptr()" because it's on the success path. It leads to an immediate crash in testing. But here it's just ref counting so possibly we might miss that sort of bug. regards, dan carpenter
Re: [PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node()
On Wed, May 29, 2024 at 03:40:47AM +0300, Laurent Pinchart wrote: > > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device > > *dev, > > } > > > > /* Iterate through each output port to discover topology */ > > - while ((ep = of_graph_get_next_endpoint(parent, ep))) { > > + for_each_endpoint_of_node(parent, ep) { > > /* > > * Legacy binding mixes input/output ports under the > > * same parent. So, skip the input ports if we are dealing > > I think there's a bug below. The loop contains > > ret = of_coresight_parse_endpoint(dev, ep, pdata); > if (ret) > return ret; > > which leaks the reference to ep. This is not introduced by this patch, Someone should create for_each_endpoint_of_node_scoped(). #define for_each_endpoint_of_node_scoped(parent, child) \ for (struct device_node *child __free(device_node) = \ of_graph_get_next_endpoint(parent, NULL); child != NULL; \ child = of_graph_get_next_endpoint(parent, child)) regards, dan carpenter
[PATCH] drm/amdgpu: delete unnecessary check
The "ret" variable is zero. No need to check. Signed-off-by: Dan Carpenter --- drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c index a037e8fba29f..4d50fb039509 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c @@ -2807,7 +2807,7 @@ static void amdgpu_ras_do_page_retirement(struct work_struct *work) static void amdgpu_ras_poison_creation_handler(struct amdgpu_device *adev, uint32_t timeout_ms) { - int ret = 0; + int ret; struct ras_ecc_log_info *ecc_log; struct ras_query_if info; uint32_t timeout = timeout_ms; @@ -2836,8 +2836,7 @@ static void amdgpu_ras_poison_creation_handler(struct amdgpu_device *adev, return; } - if (!ret) - schedule_delayed_work(>page_retirement_dwork, 0); + schedule_delayed_work(>page_retirement_dwork, 0); } static int amdgpu_ras_poison_consumption_handler(struct amdgpu_device *adev, -- 2.43.0
[PATCH] drm/amd/pm: Fix error code in vega10_hwmgr_backend_init()
Return -EINVAL on error instead of success. Also on the success path, return a literal zero instead of "return result;" Fixes: e098bc9612c2 ("drm/amd/pm: optimize the power related source code layout") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c index 37c915d7723c..9b9f8615070a 100644 --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c @@ -924,7 +924,7 @@ static int vega10_hwmgr_backend_init(struct pp_hwmgr *hwmgr) data->total_active_cus = adev->gfx.cu_info.number; if (!hwmgr->not_vf) - return result; + return -EINVAL; /* Setup default Overdrive Fan control settings */ data->odn_fan_table.target_fan_speed = @@ -947,7 +947,7 @@ static int vega10_hwmgr_backend_init(struct pp_hwmgr *hwmgr) "Mem Channel Index Exceeded maximum!", return -EINVAL); - return result; + return 0; } static int vega10_init_sclk_threshold(struct pp_hwmgr *hwmgr) -- 2.43.0
Re: [PATCH] staging: fb_tinylcd Alignment to open parenthesis
On Fri, May 03, 2024 at 04:20:24PM +, Ashok Kumar wrote: > > Is there a list of exceptions to the checkpatch information that we can > ignore in general. > For Greg's subsystems ignore the warning about extra parentheses. You can search on lore for if a patch has been patch has been sent before. Otherwise ignore checkpatch if it tells you to do something that makes the code less readable. regards, dan carpenter
Re: [PATCH] staging: fb_tinylcd Alignment to open parenthesis
On Thu, May 02, 2024 at 06:52:16PM -0700, Ashok Kumar wrote: > Corrected coding style CHECK: Alignment should match open parenthesis > The original author was aligned it deliberately to improve readability. Just ignore checkpatch on this. regards, dan carpenter
Re: [PATCH] staging: fbtft:fixes unnecessary parentheses
On Sun, Apr 28, 2024 at 08:19:08PM +0800, pipishuo wrote: > This patch fixes the checks reported by checkpatch.pl > for unnecessary parentheses > > Signed-off-by: pipishuo <1289151...@qq.com> > --- > drivers/staging/fbtft/fb_ili9320.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/fbtft/fb_ili9320.c > b/drivers/staging/fbtft/fb_ili9320.c > index 0be7c2d51548..045275a9bc0b 100644 > --- a/drivers/staging/fbtft/fb_ili9320.c > +++ b/drivers/staging/fbtft/fb_ili9320.c > @@ -37,7 +37,8 @@ static int init_display(struct fbtft_par *par) > devcode = read_devicecode(par); > fbtft_par_dbg(DEBUG_INIT_DISPLAY, par, "Device code: 0x%04X\n", > devcode); > - if ((devcode != 0x) && (devcode != 0x9320)) > + if (devcode != 0x && > + devcode != 0x9320) Nah, just leave it. It's a personal preference for Greg. https://lore.kernel.org/all/?q=init_display%20devcode regards, dan carpenter
[PATCH] drm/amdgpu: Fix signedness bug in sdma_v4_0_process_trap_irq()
The "instance" variable needs to be signed for the error handling to work. Fixes: b34ddc71267a ("drm/amdgpu: add error handle to avoid out-of-bounds") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c index 101038395c3b..772604feb6ac 100644 --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c @@ -2017,7 +2017,7 @@ static int sdma_v4_0_process_trap_irq(struct amdgpu_device *adev, struct amdgpu_irq_src *source, struct amdgpu_iv_entry *entry) { - uint32_t instance; + int instance; DRM_DEBUG("IH: SDMA trap\n"); instance = sdma_v4_0_irq_id_to_seq(entry->client_id); -- 2.43.0
[PATCH] drm/amd/display: re-indent dpp401_dscl_program_isharp()
Smatch complains because some lines are indented more than they should be. I went a bit crazy re-indenting this. ;) The comments were not useful except as a marker of things which are left to implement so I deleted most of them except for the TODO. I introduced a "data" pointer so that I could replace "scl_data->dscl_prog_data." with just "data->" and shorten the lines a bit. It's more readable without the line breaks. I also tried to align it so you can see what is changing on each line. Signed-off-by: Dan Carpenter --- .../display/dc/dpp/dcn401/dcn401_dpp_dscl.c | 93 ++- 1 file changed, 30 insertions(+), 63 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/dpp/dcn401/dcn401_dpp_dscl.c b/drivers/gpu/drm/amd/display/dc/dpp/dcn401/dcn401_dpp_dscl.c index c20376083441..696ccf96b847 100644 --- a/drivers/gpu/drm/amd/display/dc/dpp/dcn401/dcn401_dpp_dscl.c +++ b/drivers/gpu/drm/amd/display/dc/dpp/dcn401/dcn401_dpp_dscl.c @@ -779,75 +779,42 @@ static void dpp401_dscl_program_isharp(struct dpp *dpp_base, const struct scaler_data *scl_data) { struct dcn401_dpp *dpp = TO_DCN401_DPP(dpp_base); + const struct dscl_prog_data *data; if (memcmp(>scl_data, scl_data, sizeof(*scl_data)) == 0) return; PERF_TRACE(); dpp->scl_data = *scl_data; - // ISHARP_EN - REG_SET(ISHARP_MODE, 0, - ISHARP_EN, scl_data->dscl_prog_data.isharp_en); - // ISHARP_NOISEDET_EN - REG_SET(ISHARP_MODE, 0, - ISHARP_NOISEDET_EN, scl_data->dscl_prog_data.isharp_noise_det.enable); - // ISHARP_NOISEDET_MODE - REG_SET(ISHARP_MODE, 0, - ISHARP_NOISEDET_MODE, scl_data->dscl_prog_data.isharp_noise_det.mode); - // ISHARP_NOISEDET_UTHRE - REG_SET(ISHARP_NOISEDET_THRESHOLD, 0, - ISHARP_NOISEDET_UTHRE, scl_data->dscl_prog_data.isharp_noise_det.uthreshold); - // ISHARP_NOISEDET_DTHRE - REG_SET(ISHARP_NOISEDET_THRESHOLD, 0, - ISHARP_NOISEDET_DTHRE, scl_data->dscl_prog_data.isharp_noise_det.dthreshold); - REG_SET(ISHARP_MODE, 0, - ISHARP_NOISEDET_MODE, scl_data->dscl_prog_data.isharp_noise_det.mode); - // ISHARP_NOISEDET_UTHRE - REG_SET(ISHARP_NOISEDET_THRESHOLD, 0, - ISHARP_NOISEDET_UTHRE, scl_data->dscl_prog_data.isharp_noise_det.uthreshold); - // ISHARP_NOISEDET_DTHRE - REG_SET(ISHARP_NOISEDET_THRESHOLD, 0, - ISHARP_NOISEDET_DTHRE, scl_data->dscl_prog_data.isharp_noise_det.dthreshold); - // ISHARP_NOISEDET_PWL_START_IN - REG_SET(ISHARP_NOISE_GAIN_PWL, 0, - ISHARP_NOISEDET_PWL_START_IN, scl_data->dscl_prog_data.isharp_noise_det.pwl_start_in); - // ISHARP_NOISEDET_PWL_END_IN - REG_SET(ISHARP_NOISE_GAIN_PWL, 0, - ISHARP_NOISEDET_PWL_END_IN, scl_data->dscl_prog_data.isharp_noise_det.pwl_end_in); - // ISHARP_NOISEDET_PWL_SLOPE - REG_SET(ISHARP_NOISE_GAIN_PWL, 0, - ISHARP_NOISEDET_PWL_SLOPE, scl_data->dscl_prog_data.isharp_noise_det.pwl_slope); - // ISHARP_LBA_MODE - REG_SET(ISHARP_MODE, 0, - ISHARP_LBA_MODE, scl_data->dscl_prog_data.isharp_lba.mode); - // TODO: ISHARP_LBA: IN_SEG, BASE_SEG, SLOPE_SEG - // ISHARP_FMT_MODE - REG_SET(ISHARP_MODE, 0, - ISHARP_FMT_MODE, scl_data->dscl_prog_data.isharp_fmt.mode); - // ISHARP_FMT_NORM - REG_SET(ISHARP_MODE, 0, - ISHARP_FMT_NORM, scl_data->dscl_prog_data.isharp_fmt.norm); - // ISHARP_DELTA_LUT - dpp401_dscl_set_isharp_filter(dpp, scl_data->dscl_prog_data.isharp_delta); - // ISHARP_NLDELTA_SCLIP_EN_P - REG_SET(ISHARP_NLDELTA_SOFT_CLIP, 0, - ISHARP_NLDELTA_SCLIP_EN_P, scl_data->dscl_prog_data.isharp_nldelta_sclip.enable_p); - // ISHARP_NLDELTA_SCLIP_PIVOT_P - REG_SET(ISHARP_NLDELTA_SOFT_CLIP, 0, - ISHARP_NLDELTA_SCLIP_PIVOT_P, scl_data->dscl_prog_data.isharp_nldelta_sclip.pivot_p); - // ISHARP_NLDELTA_SCLIP_SLOPE_P - REG_SET(ISHARP_NLDELTA_SOFT_CLIP, 0, - ISHARP_NLDELTA_SCLIP_SLOPE_P, scl_data->dscl_prog_data.isharp_nldelta_sclip.slope_p); - // ISHARP_NLDELTA_SCLIP_EN_N - REG_SET(I
[PATCH v2] drm/amd/display: re-indent dc_power_down_on_boot()
These lines are indented too far. Clean the whitespace. Signed-off-by: Dan Carpenter --- v2: Delete another blank line (checkpatch.pl --strict). drivers/gpu/drm/amd/display/dc/core/dc.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c index 3e16041bf4f9..5a0835f884a8 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c @@ -5192,11 +5192,9 @@ void dc_interrupt_ack(struct dc *dc, enum dc_irq_source src) void dc_power_down_on_boot(struct dc *dc) { if (dc->ctx->dce_environment != DCE_ENV_VIRTUAL_HW && - dc->hwss.power_down_on_boot) { - - if (dc->caps.ips_support) - dc_exit_ips_for_hw_access(dc); - + dc->hwss.power_down_on_boot) { + if (dc->caps.ips_support) + dc_exit_ips_for_hw_access(dc); dc->hwss.power_down_on_boot(dc); } } -- 2.43.0
Re: [PATCH] drm/amd/display: re-indent dc_power_down_on_boot()
On Wed, Apr 24, 2024 at 03:33:11PM +0200, Christian König wrote: > Am 24.04.24 um 15:20 schrieb Dan Carpenter: > > On Wed, Apr 24, 2024 at 03:11:08PM +0200, Christian König wrote: > > > Am 24.04.24 um 13:41 schrieb Dan Carpenter: > > > > These lines are indented too far. Clean the whitespace. > > > > > > > > Signed-off-by: Dan Carpenter > > > > --- > > > >drivers/gpu/drm/amd/display/dc/core/dc.c | 7 +++ > > > >1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c > > > > b/drivers/gpu/drm/amd/display/dc/core/dc.c > > > > index 8eefba757da4..f64d7229eb6c 100644 > > > > --- a/drivers/gpu/drm/amd/display/dc/core/dc.c > > > > +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c > > > > @@ -5043,11 +5043,10 @@ void dc_interrupt_ack(struct dc *dc, enum > > > > dc_irq_source src) > > > >void dc_power_down_on_boot(struct dc *dc) > > > >{ > > > > if (dc->ctx->dce_environment != DCE_ENV_VIRTUAL_HW && > > > > - dc->hwss.power_down_on_boot) { > > > > - > > > > - if (dc->caps.ips_support) > > > > - dc_exit_ips_for_hw_access(dc); > > > > + dc->hwss.power_down_on_boot) { > > > > + if (dc->caps.ips_support) > > > > + dc_exit_ips_for_hw_access(dc); > > > Well while at it can't the two ifs be merged here? > > > > > > (I don't know this code to well, but it looks like it). > > > > > I'm sorry, I don't see what you're saying. > > The indentation was so messed up that I though the call to > power_down_on_boot() was after both ifs, but it is still inside the first. > > So your patch is actually right, sorry for the noise. Okay, but let me send a v2 anyway to delete the extra blank line. regards, dan carpenter
Re: [PATCH] drm/amd/display: re-indent dc_power_down_on_boot()
On Wed, Apr 24, 2024 at 03:11:08PM +0200, Christian König wrote: > Am 24.04.24 um 13:41 schrieb Dan Carpenter: > > These lines are indented too far. Clean the whitespace. > > > > Signed-off-by: Dan Carpenter > > --- > > drivers/gpu/drm/amd/display/dc/core/dc.c | 7 +++ > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c > > b/drivers/gpu/drm/amd/display/dc/core/dc.c > > index 8eefba757da4..f64d7229eb6c 100644 > > --- a/drivers/gpu/drm/amd/display/dc/core/dc.c > > +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c > > @@ -5043,11 +5043,10 @@ void dc_interrupt_ack(struct dc *dc, enum > > dc_irq_source src) > > void dc_power_down_on_boot(struct dc *dc) > > { > > if (dc->ctx->dce_environment != DCE_ENV_VIRTUAL_HW && > > - dc->hwss.power_down_on_boot) { > > - > > - if (dc->caps.ips_support) > > - dc_exit_ips_for_hw_access(dc); > > + dc->hwss.power_down_on_boot) { > > + if (dc->caps.ips_support) > > + dc_exit_ips_for_hw_access(dc); > > Well while at it can't the two ifs be merged here? > > (I don't know this code to well, but it looks like it). > I'm sorry, I don't see what you're saying. I probably should have deleted the other blank line as well, though. It introduces a checkpatch.pl --strict warning. regards, dan carpenter
[PATCH] drm/amd/display: re-indent dc_power_down_on_boot()
These lines are indented too far. Clean the whitespace. Signed-off-by: Dan Carpenter --- drivers/gpu/drm/amd/display/dc/core/dc.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c index 8eefba757da4..f64d7229eb6c 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c @@ -5043,11 +5043,10 @@ void dc_interrupt_ack(struct dc *dc, enum dc_irq_source src) void dc_power_down_on_boot(struct dc *dc) { if (dc->ctx->dce_environment != DCE_ENV_VIRTUAL_HW && - dc->hwss.power_down_on_boot) { - - if (dc->caps.ips_support) - dc_exit_ips_for_hw_access(dc); + dc->hwss.power_down_on_boot) { + if (dc->caps.ips_support) + dc_exit_ips_for_hw_access(dc); dc->hwss.power_down_on_boot(dc); } } -- 2.43.0
Re: [PATCH] staging:fbtft:fb_ili9320 Removed redundant Parentheses
0) Fix your From address. 1) Look at how other people write subjects. git log --oneline drivers/staging/fbtft/fb_ili9320.c On Fri, Apr 19, 2024 at 11:04:21AM -0700, A wrote: > >From 51e98164e314a2d1d834d2a9baea21a9823650bb Mon Sep 17 00:00:00 2001 > From: Ashok Kumar > Date: Fri, 19 Apr 2024 10:32:48 -0700 > Subject: [PATCH] staging:fbtft:fb_ili9320 Removed redundant > Parentheses 2) This should not be part of the email. > > Adhere to Linux kernel coding style. > Reported by checkpatch > > CHECK: Unnecessary parentheses around 'devcode != 0x' > + if ((devcode != 0x) && (devcode != 0x9320)) > 3) Just leave this as-is. Ignore checkpatch in this case. Greg likes parens. > Signed-off-by: Ashok Kumar > --- > drivers/staging/fbtft/fb_ili9320.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/fbtft/fb_ili9320.c > b/drivers/staging/fbtft/fb_ili9320.c > index 0be7c2d51548..409b54cc562e 100644 > --- a/drivers/staging/fbtft/fb_ili9320.c > +++ b/drivers/staging/fbtft/fb_ili9320.c > @@ -37,7 +37,7 @@ static int init_display(struct fbtft_par *par) > devcode = read_devicecode(par); > fbtft_par_dbg(DEBUG_INIT_DISPLAY, par, "Device code: > 0x%04X\n", 4) This patch is corrupt and will not apply. Read the first two paragraphs of Documentation/process/email-clients.rst. regards, dan carpenter
[bug report] drm/nouveau/disp/r535: initial support
Hello Nouveau Devs, Commit 9e999023 ("drm/nouveau/disp/r535: initial support") from Sep 19, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c:1482 r535_disp_oneinit() error: potential NULL/IS_ERR bug 'ctrl' drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c:1582 r535_disp_oneinit() error: potential NULL/IS_ERR bug 'ctrl' drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c:1596 r535_disp_oneinit() error: potential NULL/IS_ERR bug 'ctrl' drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c:1625 r535_disp_oneinit() error: potential NULL/IS_ERR bug 'ctrl' drivers/gpu/drm/nouveau/nvkm/engine/fifo/r535.c:519 r535_fifo_ectx_size() error: potential NULL/IS_ERR bug 'ctrl' drivers/gpu/drm/nouveau/nvkm/engine/fifo/r535.c:556 r535_fifo_runl_ctor() error: potential NULL/IS_ERR bug 'ctrl' drivers/gpu/drm/nouveau/nvkm/engine/fifo/r535.c:631 r535_fifo_runl_ctor() error: potential NULL/IS_ERR bug 'ctrl' drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c 1432 static int 1433 r535_disp_oneinit(struct nvkm_disp *disp) 1434 { 1435 struct nvkm_device *device = disp->engine.subdev.device; 1436 struct nvkm_gsp *gsp = device->gsp; 1437 NV2080_CTRL_INTERNAL_DISPLAY_WRITE_INST_MEM_PARAMS *ctrl; 1438 int ret, i; 1439 1440 /* RAMIN. */ 1441 ret = nvkm_gpuobj_new(device, 0x1, 0x1, false, NULL, >inst); 1442 if (ret) 1443 return ret; 1444 1445 if (WARN_ON(nvkm_memory_target(disp->inst->memory) != NVKM_MEM_TARGET_VRAM)) 1446 return -EINVAL; 1447 1448 ctrl = nvkm_gsp_rm_ctrl_get(>internal.device.subdevice, 1449 NV2080_CTRL_CMD_INTERNAL_DISPLAY_WRITE_INST_MEM, 1450 sizeof(*ctrl)); 1451 if (IS_ERR(ctrl)) 1452 return PTR_ERR(ctrl); 1453 1454 ctrl->instMemPhysAddr = nvkm_memory_addr(disp->inst->memory); 1455 ctrl->instMemSize = nvkm_memory_size(disp->inst->memory); 1456 ctrl->instMemAddrSpace = ADDR_FBMEM; 1457 ctrl->instMemCpuCacheAttr = NV_MEMORY_WRITECOMBINED; 1458 1459 ret = nvkm_gsp_rm_ctrl_wr(>internal.device.subdevice, ctrl); 1460 if (ret) 1461 return ret; 1462 1463 /* OBJs. */ 1464 ret = nvkm_gsp_client_device_ctor(gsp, >rm.client, >rm.device); 1465 if (ret) 1466 return ret; 1467 1468 ret = nvkm_gsp_rm_alloc(>rm.device.object, 0x0073, NV04_DISPLAY_COMMON, 0, 1469 >rm.objcom); 1470 if (ret) 1471 return ret; 1472 1473 { 1474 NV2080_CTRL_INTERNAL_DISPLAY_GET_STATIC_INFO_PARAMS *ctrl; 1475 1476 ctrl = nvkm_gsp_rm_ctrl_rd(>internal.device.subdevice, 1477 NV2080_CTRL_CMD_INTERNAL_DISPLAY_GET_STATIC_INFO, 1478sizeof(*ctrl)); 1479 if (IS_ERR(ctrl)) The problem here is that r535_gsp_rpc_push() returns a mix of error pointers and NULL. I've written a blog about how mixing error pointers and NULL normally works and I think there is some kind of similar logic here but I forget what it is... https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/ 1480 return PTR_ERR(ctrl); 1481 --> 1482 disp->wndw.mask = ctrl->windowPresentMask; ^^ Potential NULL dereference. 1483 disp->wndw.nr = fls(disp->wndw.mask); 1484 nvkm_gsp_rm_ctrl_done(>internal.device.subdevice, ctrl); 1485 } 1486 1487 /* */ 1488 { 1489 #if defined(CONFIG_ACPI) && defined(CONFIG_X86) 1490 NV2080_CTRL_INTERNAL_INIT_BRIGHTC_STATE_LOAD_PARAMS *ctrl; 1491 struct nvkm_gsp_object *subdevice = >rm.client.gsp->internal.device.subdevice; 1492 1493 ctrl = nvkm_gsp_rm_ctrl_get(subdevice, 1494 NV2080_CTRL_CMD_INTERNAL_INIT_BRIGHTC_STATE_LOAD, 1495 sizeof(*ctrl)); 1496 if (IS_ERR(ctrl)) regards, dan carpenter
Re: [PATCH] treewide: Fix common grammar mistake "the the"
On Thu, Apr 11, 2024 at 05:04:40PM +0200, Thorsten Blum wrote: > Use `find . -type f -exec sed -i 's/\/the/g' {} +` to find all > occurrences of "the the" and replace them with a single "the". > > Changes only comments and documentation - no code changes. > > Signed-off-by: Thorsten Blum > --- It's tricky to know which tree a patch like this would go through. We used to have a trivial tree for this stuff but I guess that didn't work. It's possible that it could go through linux-doc, but probably it has to go as a set of patches through each of the trees in the CC list. regards, dan carpenter
[bug report] drm: bridge: cdns-mhdp8546: Fix possible null pointer dereference
Hello Aleksandr Mishin, Commit 935a92a1c400 ("drm: bridge: cdns-mhdp8546: Fix possible null pointer dereference") from Apr 8, 2024 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:2074 cdns_mhdp_atomic_enable() warn: inconsistent returns '>link_mutex'. drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c 1986 static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge, 1987 struct drm_bridge_state *bridge_state) 1988 { 1989 struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge); 1990 struct drm_atomic_state *state = bridge_state->base.state; 1991 struct cdns_mhdp_bridge_state *mhdp_state; 1992 struct drm_crtc_state *crtc_state; 1993 struct drm_connector *connector; 1994 struct drm_connector_state *conn_state; 1995 struct drm_bridge_state *new_state; 1996 const struct drm_display_mode *mode; 1997 u32 resp; 1998 int ret; 1999 2000 dev_dbg(mhdp->dev, "bridge enable\n"); 2001 2002 mutex_lock(>link_mutex); ^^ Holding a lock 2003 2004 if (mhdp->plugged && !mhdp->link_up) { 2005 ret = cdns_mhdp_link_up(mhdp); 2006 if (ret < 0) 2007 goto out; 2008 } 2009 2010 if (mhdp->info && mhdp->info->ops && mhdp->info->ops->enable) 2011 mhdp->info->ops->enable(mhdp); 2012 2013 /* Enable VIF clock for stream 0 */ 2014 ret = cdns_mhdp_reg_read(mhdp, CDNS_DPTX_CAR, ); 2015 if (ret < 0) { 2016 dev_err(mhdp->dev, "Failed to read CDNS_DPTX_CAR %d\n", ret); 2017 goto out; 2018 } 2019 2020 cdns_mhdp_reg_write(mhdp, CDNS_DPTX_CAR, 2021 resp | CDNS_VIF_CLK_EN | CDNS_VIF_CLK_RSTN); 2022 2023 connector = drm_atomic_get_new_connector_for_encoder(state, 2024 bridge->encoder); 2025 if (WARN_ON(!connector)) 2026 goto out; 2027 2028 conn_state = drm_atomic_get_new_connector_state(state, connector); 2029 if (WARN_ON(!conn_state)) 2030 goto out; 2031 2032 if (mhdp->hdcp_supported && 2033 mhdp->hw_state == MHDP_HW_READY && 2034 conn_state->content_protection == 2035 DRM_MODE_CONTENT_PROTECTION_DESIRED) { 2036 mutex_unlock(>link_mutex); 2037 cdns_mhdp_hdcp_enable(mhdp, conn_state->hdcp_content_type); 2038 mutex_lock(>link_mutex); 2039 } 2040 2041 crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); 2042 if (WARN_ON(!crtc_state)) 2043 goto out; 2044 2045 mode = _state->adjusted_mode; 2046 2047 new_state = drm_atomic_get_new_bridge_state(state, bridge); 2048 if (WARN_ON(!new_state)) 2049 goto out; 2050 2051 if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes, 2052 mhdp->link.rate)) { 2053 ret = -EINVAL; 2054 goto out; 2055 } 2056 2057 cdns_mhdp_sst_enable(mhdp, mode); 2058 2059 mhdp_state = to_cdns_mhdp_bridge_state(new_state); 2060 2061 mhdp_state->current_mode = drm_mode_duplicate(bridge->dev, mode); 2062 if (!mhdp_state->current_mode) 2063 return; ^^^ Needs to unlock before returning. 2064 2065 drm_mode_set_name(mhdp_state->current_mode); 2066 2067 dev_dbg(mhdp->dev, "%s: Enabling mode %s\n", __func__, mode->name); 2068 2069 mhdp->bridge_enabled = true; 2070 2071 out: 2072 mutex_unlock(>link_mutex); 2073 if (ret < 0) --> 2074 schedule_work(>modeset_retry_work); 2075 } regards, dan carpenter
Re: [bug report] drm/panthor: Add the scheduler logical block
On Wed, Apr 10, 2024 at 03:11:52PM +0100, Steven Price wrote: > On 08/04/2024 08:35, Dan Carpenter wrote: > > Hello Boris Brezillon, > > > > Commit de8548813824 ("drm/panthor: Add the scheduler logical block") > > from Feb 29, 2024 (linux-next), leads to the following Smatch static > > checker warning: > > > > drivers/gpu/drm/panthor/panthor_sched.c:1153 > > csg_slot_sync_state_locked() > > error: uninitialized symbol 'new_state'. > > > > drivers/gpu/drm/panthor/panthor_sched.c > > 1123 static void > > 1124 csg_slot_sync_state_locked(struct panthor_device *ptdev, u32 > > csg_id) > > 1125 { > > 1126 struct panthor_csg_slot *csg_slot = > > >scheduler->csg_slots[csg_id]; > > 1127 struct panthor_fw_csg_iface *csg_iface; > > 1128 struct panthor_group *group; > > 1129 enum panthor_group_state new_state, old_state; > > 1130 > > 1131 lockdep_assert_held(>scheduler->lock); > > 1132 > > 1133 csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id); > > 1134 group = csg_slot->group; > > 1135 > > 1136 if (!group) > > 1137 return; > > 1138 > > 1139 old_state = group->state; > > 1140 switch (csg_iface->output->ack & CSG_STATE_MASK) { > > ^^ > > This mask is 0x7. Should it be 0x3? That would silence the static > > checker warning. > > The mask is correct - it's effectively a hardware register (well it's > read/written by the firmware on the hardware). States 4-7 are officially > "reserved" and Should Not Happen™! > > I guess a "default:" case with a WARN_ON() would be the right solution. > > Steve A WARN_ON() won't silence the warning. Plus WARN_ON() is not free in terms of memory usage. And they're kind of controversial these days to be honest. One solution would be to just ignore the static checker warning. These are a one time thing, and if people have questions in the future, they can just search lore for this thread. regards, dan carpenter
[PATCH] drm/panthor: clean up some types in panthor_sched_suspend()
These variables should be u32 instead of u64 because they're only storing u32 values. Also static checkers complain when we do: suspended_slots &= ~upd_ctx.timedout_mask; In this code "suspended_slots" is a u64 and "upd_ctx.timedout_mask". The mask clears out the top 32 bits which would likely be a bug if anything were stored there. Signed-off-by: Dan Carpenter --- drivers/gpu/drm/panthor/panthor_sched.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c index d4bc652b34d5..b3a51a6de523 100644 --- a/drivers/gpu/drm/panthor/panthor_sched.c +++ b/drivers/gpu/drm/panthor/panthor_sched.c @@ -2546,7 +2546,7 @@ void panthor_sched_suspend(struct panthor_device *ptdev) { struct panthor_scheduler *sched = ptdev->scheduler; struct panthor_csg_slots_upd_ctx upd_ctx; - u64 suspended_slots, faulty_slots; + u32 suspended_slots, faulty_slots; struct panthor_group *group; u32 i; -- 2.43.0
[bug report] drm/panthor: Add the scheduler logical block
Hello Boris Brezillon, Commit de8548813824 ("drm/panthor: Add the scheduler logical block") from Feb 29, 2024 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/panthor/panthor_sched.c:1153 csg_slot_sync_state_locked() error: uninitialized symbol 'new_state'. drivers/gpu/drm/panthor/panthor_sched.c 1123 static void 1124 csg_slot_sync_state_locked(struct panthor_device *ptdev, u32 csg_id) 1125 { 1126 struct panthor_csg_slot *csg_slot = >scheduler->csg_slots[csg_id]; 1127 struct panthor_fw_csg_iface *csg_iface; 1128 struct panthor_group *group; 1129 enum panthor_group_state new_state, old_state; 1130 1131 lockdep_assert_held(>scheduler->lock); 1132 1133 csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id); 1134 group = csg_slot->group; 1135 1136 if (!group) 1137 return; 1138 1139 old_state = group->state; 1140 switch (csg_iface->output->ack & CSG_STATE_MASK) { ^^ This mask is 0x7. Should it be 0x3? That would silence the static checker warning. 1141 case CSG_STATE_START: 1142 case CSG_STATE_RESUME: 1143 new_state = PANTHOR_CS_GROUP_ACTIVE; 1144 break; 1145 case CSG_STATE_TERMINATE: 1146 new_state = PANTHOR_CS_GROUP_TERMINATED; 1147 break; 1148 case CSG_STATE_SUSPEND: 1149 new_state = PANTHOR_CS_GROUP_SUSPENDED; 1150 break; 1151 } 1152 --> 1153 if (old_state == new_state) 1154 return; 1155 1156 if (new_state == PANTHOR_CS_GROUP_SUSPENDED) 1157 csg_slot_sync_queues_state_locked(ptdev, csg_id); 1158 1159 if (old_state == PANTHOR_CS_GROUP_ACTIVE) { 1160 u32 i; 1161 1162 /* Reset the queue slots so we start from a clean 1163 * state when starting/resuming a new group on this 1164 * CSG slot. No wait needed here, and no ringbell 1165 * either, since the CS slot will only be re-used 1166 * on the next CSG start operation. 1167 */ 1168 for (i = 0; i < group->queue_count; i++) { 1169 if (group->queues[i]) 1170 cs_slot_reset_locked(ptdev, csg_id, i); 1171 } 1172 } 1173 1174 group->state = new_state; 1175 } regards, dan carpenter
Re: [PATCH 7/7] drm/udl: Remove struct udl_connector
Hi Thomas, 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/Thomas-Zimmermann/drm-edid-Implement-drm_probe_ddc-with-drm_edid_probe_custom/20240404-231057 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20240404150857.5520-8-tzimmermann%40suse.de patch subject: [PATCH 7/7] drm/udl: Remove struct udl_connector config: parisc-randconfig-r071-20240405 (https://download.01.org/0day-ci/archive/20240405/202404051359.y6aguwfi-...@intel.com/config) compiler: hppa-linux-gcc (GCC) 13.2.0 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/202404051359.y6aguwfi-...@intel.com/ smatch warnings: drivers/gpu/drm/udl/udl_modeset.c:527 udl_modeset_init() warn: passing a valid pointer to 'PTR_ERR' vim +/PTR_ERR +527 drivers/gpu/drm/udl/udl_modeset.c 72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06 518 encoder = >encoder; 72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06 519 ret = drm_encoder_init(dev, encoder, _encoder_funcs, DRM_MODE_ENCODER_DAC, NULL); 72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06 520 if (ret) 72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06 521 return ret; 72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06 522 encoder->possible_crtcs = drm_crtc_mask(crtc); 5320918b9a87865 Dave Airlie 2010-12-15 523 a80d9e00c8195dc Thomas Zimmermann 2024-04-04 524 connector = >connector; a80d9e00c8195dc Thomas Zimmermann 2024-04-04 525 ret = drm_connector_init(dev, connector, _connector_funcs, DRM_MODE_CONNECTOR_VGA); a80d9e00c8195dc Thomas Zimmermann 2024-04-04 526 if (ret) fe5b7c86d6068ac Daniel Vetter 2020-03-23 @527 return PTR_ERR(connector); ^^^ This needs to be updated to "return ret;" -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
[PATCH] drm: xlnx: db: fix a memory leak in probe
Free "dp" before returning. Fixes: be318d01a903 ("drm: xlnx: dp: Reset DisplayPort IP") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c index 5a40aa1d4283..8a15d18a65a6 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -1716,7 +1716,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub) ret = zynqmp_dp_reset(dp, true); if (ret < 0) - return ret; + goto err_free; ret = zynqmp_dp_reset(dp, false); if (ret < 0) -- 2.43.0
Re: [PATCH v5 02/10] PCI: Deprecate iomap-table functions
Hi Philipp, 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/Philipp-Stanner/PCI-Add-new-set-of-devres-functions/20240403-160932 base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next patch link: https://lore.kernel.org/r/20240403080712.13986-5-pstanner%40redhat.com patch subject: [PATCH v5 02/10] PCI: Deprecate iomap-table functions config: i386-randconfig-141-20240404 (https://download.01.org/0day-ci/archive/20240404/202404040920.qixhnemu-...@intel.com/config) compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0 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/202404040920.qixhnemu-...@intel.com/ smatch warnings: drivers/pci/devres.c:897 pcim_iomap_regions_request_all() error: we previously assumed 'legacy_iomap_table' could be null (see line 894) vim +/legacy_iomap_table +897 drivers/pci/devres.c acc2364fe66106 Philipp Stanner 2024-01-31 865 int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask, acc2364fe66106 Philipp Stanner 2024-01-31 866 const char *name) acc2364fe66106 Philipp Stanner 2024-01-31 867 { 34e90b966504f3 Philipp Stanner 2024-04-03 868 short bar; 34e90b966504f3 Philipp Stanner 2024-04-03 869 int ret; 34e90b966504f3 Philipp Stanner 2024-04-03 870 void __iomem **legacy_iomap_table; acc2364fe66106 Philipp Stanner 2024-01-31 871 34e90b966504f3 Philipp Stanner 2024-04-03 872 ret = pcim_request_all_regions(pdev, name); 34e90b966504f3 Philipp Stanner 2024-04-03 873 if (ret != 0) 34e90b966504f3 Philipp Stanner 2024-04-03 874 return ret; acc2364fe66106 Philipp Stanner 2024-01-31 875 34e90b966504f3 Philipp Stanner 2024-04-03 876 for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) { 34e90b966504f3 Philipp Stanner 2024-04-03 877 if (!mask_contains_bar(mask, bar)) 34e90b966504f3 Philipp Stanner 2024-04-03 878 continue; 34e90b966504f3 Philipp Stanner 2024-04-03 879 if (!pcim_iomap(pdev, bar, 0)) 34e90b966504f3 Philipp Stanner 2024-04-03 880 goto err; 34e90b966504f3 Philipp Stanner 2024-04-03 881 } 34e90b966504f3 Philipp Stanner 2024-04-03 882 34e90b966504f3 Philipp Stanner 2024-04-03 883 return 0; 34e90b966504f3 Philipp Stanner 2024-04-03 884 34e90b966504f3 Philipp Stanner 2024-04-03 885 err: 34e90b966504f3 Philipp Stanner 2024-04-03 886 /* 34e90b966504f3 Philipp Stanner 2024-04-03 887 * Here it gets tricky: pcim_iomap() above has most likely 34e90b966504f3 Philipp Stanner 2024-04-03 888 * failed because it got an OOM when trying to create the 34e90b966504f3 Philipp Stanner 2024-04-03 889 * legacy-table. 34e90b966504f3 Philipp Stanner 2024-04-03 890 * We check here if that has happened. If not, pcim_iomap() 34e90b966504f3 Philipp Stanner 2024-04-03 891 * must have failed because of EINVAL. 34e90b966504f3 Philipp Stanner 2024-04-03 892 */ 34e90b966504f3 Philipp Stanner 2024-04-03 893 legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev); 34e90b966504f3 Philipp Stanner 2024-04-03 @894 ret = legacy_iomap_table ? -EINVAL : -ENOMEM; ^^ Check for NULL 34e90b966504f3 Philipp Stanner 2024-04-03 895 34e90b966504f3 Philipp Stanner 2024-04-03 896 while (--bar >= 0) 34e90b966504f3 Philipp Stanner 2024-04-03 @897 pcim_iounmap(pdev, legacy_iomap_table[bar]); ^^ Unchecked dereference 34e90b966504f3 Philipp Stanner 2024-04-03 898 34e90b966504f3 Philipp Stanner 2024-04-03 899 pcim_release_all_regions(pdev); 34e90b966504f3 Philipp Stanner 2024-04-03 900 34e90b966504f3 Philipp Stanner 2024-04-03 901 return ret; acc2364fe66106 Philipp Stanner 2024-01-31 902 } -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Re: [PATCH v3] drm/panthor: Fix couple of NULL vs IS_ERR() bugs
On Tue, Apr 02, 2024 at 05:19:25PM +0200, Boris Brezillon wrote: > On Tue, 2 Apr 2024 17:44:18 +0300 > Dan Carpenter wrote: > > > On Tue, Apr 02, 2024 at 04:38:38PM +0200, Boris Brezillon wrote: > > > On Tue, 2 Apr 2024 07:14:11 -0700 > > > Harshit Mogalapalli wrote: > > > > > > > Currently panthor_vm_get_heap_pool() returns both ERR_PTR() and > > > > NULL(when create is false and if there is no poool attached to the > > > > > >^ pool > > > > > > > VM) > > > > - Change the function to return error pointers, when pool is > > > > NULL return -ENOENT > > > > - Also handle the callers to check for IS_ERR() on failure. > > > > > > > > Fixes: 4bdca1150792 ("drm/panthor: Add the driver frontend block") > > > > > > I would explain that the code was correct, but the documentation didn't > > > match the function behavior, otherwise it feels a bit weird to have a > > > Fixes tag here. > > > > The code wasn't correct, it returned a mix of error pointers and NULL. > > AFAICT, this is allowed, otherwise why would we have IS_ERR_OR_NULL(). Yep. I have written a blog about this: https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/ > The fact smatch can't see through panthor_vm_get_heap_pool() and detect > that the return value is different for create=false/true doesn't mean > the code was wrong. I'm certainly not saying this is a good thing to > have a function that encodes the error case with two different kind of > return value, but I wouldn't qualify it as a bug either. What's > incorrect though, is the fact the documentation doesn't match the code. > > > So it needs a Fixes tag. > > I didn't say we should drop the Fixes tag, but the bug being fixed here > is a mismatch between the doc and the implementation, the code itself > was correct, and the behavior is actually unchanged with this patch > applied, it's just done in a less confusing way. Oh. Sorry, I haven't been following this thread closely and I misread the code as well. You're right that the code works. In this case, I would say actually that it does not need a Fixes tag because it's not a bug. It's just a cleanup. Sorry for the noise. regards, dan carpenter
Re: [PATCH v3] drm/panthor: Fix couple of NULL vs IS_ERR() bugs
On Tue, Apr 02, 2024 at 04:38:38PM +0200, Boris Brezillon wrote: > On Tue, 2 Apr 2024 07:14:11 -0700 > Harshit Mogalapalli wrote: > > > Currently panthor_vm_get_heap_pool() returns both ERR_PTR() and > > NULL(when create is false and if there is no poool attached to the > >^ pool > > > VM) > > - Change the function to return error pointers, when pool is > > NULL return -ENOENT > > - Also handle the callers to check for IS_ERR() on failure. > > > > Fixes: 4bdca1150792 ("drm/panthor: Add the driver frontend block") > > I would explain that the code was correct, but the documentation didn't > match the function behavior, otherwise it feels a bit weird to have a > Fixes tag here. The code wasn't correct, it returned a mix of error pointers and NULL. So it needs a Fixes tag. regards, dan carpenter
[PATCH] drm/panthor: Fix a couple -ENOMEM error codes
These error paths forgot to set the error code to -ENOMEM. Fixes: 647810ec2476 ("drm/panthor: Add the MMU/VM logical block") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/panthor/panthor_mmu.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c index fdd35249169f..a26b40aab261 100644 --- a/drivers/gpu/drm/panthor/panthor_mmu.c +++ b/drivers/gpu/drm/panthor/panthor_mmu.c @@ -1264,8 +1264,10 @@ static int panthor_vm_prepare_map_op_ctx(struct panthor_vm_op_ctx *op_ctx, op_ctx->rsvd_page_tables.pages = kcalloc(pt_count, sizeof(*op_ctx->rsvd_page_tables.pages), GFP_KERNEL); - if (!op_ctx->rsvd_page_tables.pages) + if (!op_ctx->rsvd_page_tables.pages) { + ret = -ENOMEM; goto err_cleanup; + } ret = kmem_cache_alloc_bulk(pt_cache, GFP_KERNEL, pt_count, op_ctx->rsvd_page_tables.pages); @@ -1318,8 +1320,10 @@ static int panthor_vm_prepare_unmap_op_ctx(struct panthor_vm_op_ctx *op_ctx, op_ctx->rsvd_page_tables.pages = kcalloc(pt_count, sizeof(*op_ctx->rsvd_page_tables.pages), GFP_KERNEL); - if (!op_ctx->rsvd_page_tables.pages) + if (!op_ctx->rsvd_page_tables.pages) { + ret = -ENOMEM; goto err_cleanup; + } ret = kmem_cache_alloc_bulk(pt_cache, GFP_KERNEL, pt_count, op_ctx->rsvd_page_tables.pages); -- 2.43.0
[PATCH] drm/panthor: Fix off by one in panthor_fw_get_cs_iface()
The ->iface.streams[csg_slot][] array has MAX_CS_PER_CSG elements so this > comparison needs to be >= to prevent an out of bounds access. Fixes: 2718d91816ee ("drm/panthor: Add the FW logical block") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/panthor/panthor_fw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c index 33c87a59834e..181395e2859a 100644 --- a/drivers/gpu/drm/panthor/panthor_fw.c +++ b/drivers/gpu/drm/panthor/panthor_fw.c @@ -308,7 +308,7 @@ panthor_fw_get_csg_iface(struct panthor_device *ptdev, u32 csg_slot) struct panthor_fw_cs_iface * panthor_fw_get_cs_iface(struct panthor_device *ptdev, u32 csg_slot, u32 cs_slot) { - if (drm_WARN_ON(>base, csg_slot >= MAX_CSGS || cs_slot > MAX_CS_PER_CSG)) + if (drm_WARN_ON(>base, csg_slot >= MAX_CSGS || cs_slot >= MAX_CS_PER_CSG)) return NULL; return >fw->iface.streams[csg_slot][cs_slot]; -- 2.43.0
[PATCH] drm/panthor: Fix error code in panthor_gpu_init()
This code accidentally returns zero/success on error because of a typo. It should be "irq" instead of "ret". The other thing is that if platform_get_irq_byname() were to return zero then the error code would be cmplicated. Fortunately, it does not so we can just change <= to < 0. Fixes: 5cd894e258c4 ("drm/panthor: Add the GPU logical block") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/panthor/panthor_gpu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c index 0f7c962440d3..5251d8764e7d 100644 --- a/drivers/gpu/drm/panthor/panthor_gpu.c +++ b/drivers/gpu/drm/panthor/panthor_gpu.c @@ -211,8 +211,8 @@ int panthor_gpu_init(struct panthor_device *ptdev) return ret; irq = platform_get_irq_byname(to_platform_device(ptdev->base.dev), "gpu"); - if (irq <= 0) - return ret; + if (irq < 0) + return irq; ret = panthor_request_gpu_irq(ptdev, >gpu->irq, irq, GPU_INTERRUPTS_MASK); if (ret) -- 2.43.0
Re: [PATCH 4/8] media: platform: ti: use for_each_endpoint_of_node()
On Mon, Mar 25, 2024 at 03:05:27AM +, Kuninori Morimoto wrote: > We already have for_each_endpoint_of_node(), don't use > of_graph_get_next_endpoint() directly. Replace it. > > Signed-off-by: Kuninori Morimoto > --- > drivers/media/platform/ti/am437x/am437x-vpfe.c | 8 +++- > drivers/media/platform/ti/davinci/vpif_capture.c | 11 +-- > 2 files changed, 8 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/platform/ti/am437x/am437x-vpfe.c > b/drivers/media/platform/ti/am437x/am437x-vpfe.c > index 77e12457d149..4f185a0d42b3 100644 > --- a/drivers/media/platform/ti/am437x/am437x-vpfe.c > +++ b/drivers/media/platform/ti/am437x/am437x-vpfe.c > @@ -2306,14 +2306,10 @@ vpfe_get_pdata(struct vpfe_device *vpfe) > if (!pdata) > return NULL; > > - for (i = 0; ; i++) { > + for_each_endpoint_of_node(dev->of_node, endpoint) { > struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 }; > struct device_node *rem; > > - endpoint = of_graph_get_next_endpoint(dev->of_node, endpoint); > - if (!endpoint) > - break; > - > sdinfo = >sub_devs[i]; ^ "i" is uninitialized now. Also in the initializer it has "struct device_node *endpoint = NULL;" which is unnecessary now. And at the end it has: of_node_put(endpoint); return pdata; Since endpoint is NULL this was always a pointless no-op but now it's more obvious, so lets delete that. regards, dan carpenter
Re: [PATCH v5 08/11] devm-helpers: Add resource managed version of debugfs directory create function
On Sat, Mar 23, 2024 at 10:10:40PM +0100, Christophe JAILLET wrote: > > static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev) > > { > > - pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL); > > + pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL); > > + if (IS_ERR(pvt->dbgfs_dir)) > > + return PTR_ERR(pvt->dbgfs_dir); > > Not sure if the test and error handling should be added here. Yep. debugfs_create_dir() is not supposed to be checked here. It breaks the driver if CONFIG_DEBUGFS is disabled. I have written a blog about this: https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/ regards, dan carpenter
[PATCH] backlight: mp3309c: fix signedness bug in mp3309c_parse_fwnode()
The "num_levels" variable is used to store error codes from device_property_count_u32() so it needs to be signed. This doesn't cause an issue at runtime because devm_kcalloc() won't allocate negative sizes. However, it's still worth fixing. Fixes: b54c828bdba9 ("backlight: mp3309c: Make use of device properties") Signed-off-by: Dan Carpenter --- drivers/video/backlight/mp3309c.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/video/backlight/mp3309c.c b/drivers/video/backlight/mp3309c.c index c80a1481e742..4e98e60417d2 100644 --- a/drivers/video/backlight/mp3309c.c +++ b/drivers/video/backlight/mp3309c.c @@ -205,8 +205,9 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip, struct mp3309c_platform_data *pdata) { int ret, i; - unsigned int num_levels, tmp_value; + unsigned int tmp_value; struct device *dev = chip->dev; + int num_levels; if (!dev_fwnode(dev)) return dev_err_probe(dev, -ENODEV, "failed to get firmware node\n"); -- 2.43.0
[PATCH] drm/amd/display: delete unnecessary check in dcn35_set_long_vblank()
"timing" is "_ctx[i]->stream->timing" where ->timing is not the first struct member of ->stream. So it's the address which points into the middle of a struct. It can't be NULL so delete the NULL check. Signed-off-by: Dan Carpenter --- drivers/gpu/drm/amd/display/dc/hwss/dcn35/dcn35_hwseq.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn35/dcn35_hwseq.c b/drivers/gpu/drm/amd/display/dc/hwss/dcn35/dcn35_hwseq.c index 2e8ec58a16eb..87cfd9f1cec9 100644 --- a/drivers/gpu/drm/amd/display/dc/hwss/dcn35/dcn35_hwseq.c +++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn35/dcn35_hwseq.c @@ -1411,10 +1411,7 @@ void dcn35_set_long_vblank(struct pipe_ctx **pipe_ctx, if (pipe_ctx[i]->stream) { struct dc_crtc_timing *timing = _ctx[i]->stream->timing; - if (timing) - params.vertical_blank_start = timing->v_total - timing->v_front_porch; - else - params.vertical_blank_start = 0; + params.vertical_blank_start = timing->v_total - timing->v_front_porch; if ((pipe_ctx[i]->stream_res.tg != NULL) && pipe_ctx[i]->stream_res.tg->funcs && pipe_ctx[i]->stream_res.tg->funcs->set_long_vtotal) -- 2.43.0
Re: [PATCH 3/6] backlight/omap1-bl: Replace FB_BLANK_ states with simple on/off
On Thu, Mar 14, 2024 at 09:16:15AM +0100, Thomas Zimmermann wrote: > Hi > > Am 13.03.24 um 19:00 schrieb Sam Ravnborg: > > Hi Thomas, > > > > On Wed, Mar 13, 2024 at 04:45:02PM +0100, Thomas Zimmermann wrote: > > > The backlight is on for fb_blank eq FB_BLANK_UNBLANK, or off for > > > any other value in fb_blank. But the field fb_blank in struct > > > backlight_properties is deprecated and should not be used any > > > longer. > > > > > > Replace the test for fb_blank in omap's backlight code with a > > > simple boolean parameter and push the test into the update_status > > > helper. Instead of reading fb_blank directly, decode the backlight > > > device's status with backlight_is_blank(). > > > > > > Signed-off-by: Thomas Zimmermann > > My biased opinion is that the approach in this patch is a little bit > > better: > > https://lore.kernel.org/lkml/20230107-sam-video-backlight-drop-fb_blank-v1-13-1bd9bafb3...@ravnborg.org/ > > > > I never came around resending this series it seems. > > Oh, that series has been around for over a year. I don't care about which > patches go in as long as they remove the dependency on . I saw > that Dan has already r-b'ed the current patchset, but if you prefer I'll > adopt yours. I hadn't seen Sam's patch. It's a little bit more daring, but it's really nice code and I trust him. regards, dan carpenter
Re: [PATCH 0/6] backlight: Remove struct backlight_properties.fb_blank
I was only going to comment on the staging bits but, heck, I reviewed the whole series. Reviewed-by: Dan Carpenter regards, dan carpenter
Re: [PATCH 1/6] auxdisplay/ht16k33: Replace use of fb_blank with backlight helper
On Wed, Mar 13, 2024 at 04:45:00PM +0100, Thomas Zimmermann wrote: > Replace the use of struct backlight_properties.fb_blank with a > call to backlight_get_brightness(). The helper implement the same > logic as the driver's function. If you end up resending there is a typo s/implement/implements/. regards, dan carpenter
Re: [PATCH 5/6] staging/fbtft: Remove reference to fb_blank
On Wed, Mar 13, 2024 at 04:45:04PM +0100, Thomas Zimmermann wrote: > The field fb_blank in struct backlight_properties is deprecated and > should not be used. Don't output its value in the driver's debug print. > > Signed-off-by: Thomas Zimmermann > --- > drivers/staging/fbtft/fb_ssd1351.c | 4 +--- > drivers/staging/fbtft/fbtft-core.c | 5 ++--- Reviewed-by: Dan Carpenter regards, dan carpenter
Re: [PATCH 1/6] auxdisplay/ht16k33: Replace use of fb_blank with backlight helper
On Wed, Mar 13, 2024 at 05:08:08PM +0100, Miguel Ojeda wrote: > > - int brightness = bl->props.brightness; > > + int brightness = backlight_get_brightness(bl); > > This can be `const` now (or even removed and have the call embedded below). > Is there an advantage to making this const? regards, dan carpenter
Re: [PATCH 00/14] Add support for suppressing warning backtraces
Thanks! Acked-by: Dan Carpenter regards, dan carpenter
[drm-intel:for-linux-next-gt 1/4] drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:2680 guc_context_policy_init_v70() warn: variable dereferenced before check 'ce' (see line 2663)
tree: git://anongit.freedesktop.org/drm-intel for-linux-next-gt head: 7ad6a8fae597af7fae5193efc73276609337c360 commit: cec82816d0d018f178b9b7f88fe4bf80d66954e9 [1/4] drm/i915/guc: Use context hints for GT frequency config: i386-randconfig-141-20240309 (https://download.01.org/0day-ci/archive/20240310/202403101225.7ahejhzj-...@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 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/202403101225.7ahejhzj-...@intel.com/ New smatch warnings: drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:2680 guc_context_policy_init_v70() warn: variable dereferenced before check 'ce' (see line 2663) vim +/ce +2680 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 774ce1510e6ccb Daniele Ceraolo Spurio 2022-07-18 2661 static int guc_context_policy_init_v70(struct intel_context *ce, bool loop) 2584b3549f4c40 John Harrison 2022-04-12 2662 { 2584b3549f4c40 John Harrison 2022-04-12 @2663 struct intel_engine_cs *engine = ce->engine; ^^ Unchecked dereference 3f2f20da79b208 Andi Shyti 2023-12-29 2664 struct intel_guc *guc = gt_to_guc(engine->gt); 2584b3549f4c40 John Harrison 2022-04-12 2665 struct context_policy policy; 2584b3549f4c40 John Harrison 2022-04-12 2666 u32 execution_quantum; 2584b3549f4c40 John Harrison 2022-04-12 2667 u32 preemption_timeout; cec82816d0d018 Vinay Belgaumkar 2024-03-05 2668 u32 slpc_ctx_freq_req = 0; 2584b3549f4c40 John Harrison 2022-04-12 2669 unsigned long flags; 2584b3549f4c40 John Harrison 2022-04-12 2670 int ret; 3a4bfa091c46e9 Rahul Kumar Singh 2021-07-26 2671 7935785240508c John Harrison 2021-07-26 2672 /* NB: For both of these, zero means disabled. */ 568944af44e753 John Harrison 2022-10-06 2673 GEM_BUG_ON(overflows_type(engine->props.timeslice_duration_ms * 1000, 568944af44e753 John Harrison 2022-10-06 2674 execution_quantum)); 568944af44e753 John Harrison 2022-10-06 2675 GEM_BUG_ON(overflows_type(engine->props.preempt_timeout_ms * 1000, 568944af44e753 John Harrison 2022-10-06 2676 preemption_timeout)); 2584b3549f4c40 John Harrison 2022-04-12 2677 execution_quantum = engine->props.timeslice_duration_ms * 1000; 2584b3549f4c40 John Harrison 2022-04-12 2678 preemption_timeout = engine->props.preempt_timeout_ms * 1000; 2584b3549f4c40 John Harrison 2022-04-12 2679 cec82816d0d018 Vinay Belgaumkar 2024-03-05 @2680 if (ce && (ce->flags & BIT(CONTEXT_LOW_LATENCY))) ^^ NULL check is too late. cec82816d0d018 Vinay Belgaumkar 2024-03-05 2681 slpc_ctx_freq_req |= SLPC_CTX_FREQ_REQ_IS_COMPUTE; cec82816d0d018 Vinay Belgaumkar 2024-03-05 2682 2584b3549f4c40 John Harrison 2022-04-12 2683 __guc_context_policy_start_klv(, ce->guc_id.id); 2584b3549f4c40 John Harrison 2022-04-12 2684 2584b3549f4c40 John Harrison 2022-04-12 2685 __guc_context_policy_add_priority(, ce->guc_state.prio); 2584b3549f4c40 John Harrison 2022-04-12 2686 __guc_context_policy_add_execution_quantum(, execution_quantum); 2584b3549f4c40 John Harrison 2022-04-12 2687 __guc_context_policy_add_preemption_timeout(, preemption_timeout); cec82816d0d018 Vinay Belgaumkar 2024-03-05 2688 __guc_context_policy_add_slpc_ctx_freq_req(, slpc_ctx_freq_req); 2584b3549f4c40 John Harrison 2022-04-12 2689 2584b3549f4c40 John Harrison 2022-04-12 2690 if (engine->flags & I915_ENGINE_WANT_FORCED_PREEMPTION) 2584b3549f4c40 John Harrison 2022-04-12 2691 __guc_context_policy_add_preempt_to_idle(, 1); 2584b3549f4c40 John Harrison 2022-04-12 2692 2584b3549f4c40 John Harrison 2022-04-12 2693 ret = __guc_context_set_context_policies(guc, , loop); 2584b3549f4c40 John Harrison 2022-04-12 2694 2584b3549f4c40 John Harrison 2022-04-12 2695 spin_lock_irqsave(>guc_state.lock, flags); 6c82c75230b87d Daniele Ceraolo Spurio 2022-07-27 2696 if (ret != 0) 2584b3549f4c40 John Harrison 2022-04-12 2697 set_context_policy_required(ce); 2584b3549f4c40 John Harrison 2022-04-12 2698 else 2584b
Re: [PATCH] soc: qcom: pmic_glink_altmode: Use common error handling code in pmic_glink_altmode_probe()
On Thu, Feb 29, 2024 at 12:26:49AM +0100, Konrad Dybcio wrote: > > > On 2/28/24 19:05, Markus Elfring wrote: > > From: Markus Elfring > > Date: Wed, 28 Feb 2024 18:45:13 +0100 > > > > Add a jump target so that a bit of exception handling can be better reused > > at the end of this function implementation. > > > > This issue was transformed by using the Coccinelle software. > > > > Signed-off-by: Markus Elfring > > --- > > (+CC Peter) > > Hmm.. this looks very similar to the problem that __free solves > with .. > > I know no better, but wouldn't the same mechanism, down to the > usage of DEFINE_FREE work fine for _put-like functions? Jonathan Cameron has created something like this: https://lore.kernel.org/all/20240225142714.286440-1-ji...@kernel.org/ It hasn't been merged yet and it would need a bit of adjusting for this use case but it's basically what you want. regards, dan carpenter
Re: [PATCH] staging: fbtft: Fix "space prohibited before that close parenthesis ')'" error reported by checkpatch
On Wed, Feb 21, 2024 at 06:54:04PM +0900, sawara0...@gmail.com wrote: > From: Kyoji Ogasawara > > Since whitespace is prohibited before that close parenthesis ')' in a > conditional statements, remove it and fix checkpatch.pl error "space > prohibited before that > close parenthesis ')'". > > Signed-off-by: Kyoji Ogasawara This breaks the build. You could do a search for it. https://lore.kernel.org/all/?q=define_fbtft_write_reg regards, dan carpenter
Re: [PATCH v2] udmabuf: Fix a potential (and unlikely) access to unallocated memory
On Mon, Feb 19, 2024 at 06:59:02PM +0100, Christophe JAILLET wrote: > Le 19/02/2024 à 09:37, Dan Carpenter a écrit : > > On Sun, Feb 18, 2024 at 06:46:44PM +0100, Christophe JAILLET wrote: > > > If 'list_limit' is set to a very high value, 'lsize' computation could > > > overflow if 'head.count' is big enough. > > > > > > > The "list_limit" is set via module parameter so if you set that high > > enough to lead to an integer overflow then you kind of deserve what > > you get. > > > > This patch is nice for kernel hardening and making the code easier to > > read/audit but the real world security impact is negligible. > > Agreed. > > That is what I meant by "and unlikely". > Maybe the commit message could be more explicit if needed. > > Let me know if ok as-is or if I should try to re-word the description. No, it's fine. But in the future if there is an integer overflow then lets mention in the commit message who it affects or what the impact is. regards, dan carpenter
Re: [PATCH v2] udmabuf: Fix a potential (and unlikely) access to unallocated memory
On Sun, Feb 18, 2024 at 06:46:44PM +0100, Christophe JAILLET wrote: > If 'list_limit' is set to a very high value, 'lsize' computation could > overflow if 'head.count' is big enough. > The "list_limit" is set via module parameter so if you set that high enough to lead to an integer overflow then you kind of deserve what you get. This patch is nice for kernel hardening and making the code easier to read/audit but the real world security impact is negligible. regards, dan carpenter
Re: [PATCH 16/28] drm/i915/color: Create a transfer function color pipeline
Hi Uma, 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/Uma-Shankar/drm-color-pipeline-base-work/20240213-144544 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20240213064835.139464-17-uma.shankar%40intel.com patch subject: [PATCH 16/28] drm/i915/color: Create a transfer function color pipeline config: i386-randconfig-141-20240217 (https://download.01.org/0day-ci/archive/20240218/202402180310.gmdixajx-...@intel.com/config) compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18) 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/202402180310.gmdixajx-...@intel.com/ New smatch warnings: drivers/gpu/drm/i915/display/intel_color.c:3867 intel_plane_tf_pipeline_init() error: 'colorop' dereferencing possible ERR_PTR() vim +/colorop +3867 drivers/gpu/drm/i915/display/intel_color.c 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3852 int intel_plane_tf_pipeline_init(struct drm_plane *plane, struct drm_prop_enum_list *list) 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3853 { 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3854 struct intel_plane *intel_plane = to_intel_plane(plane); 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3855 struct intel_plane_colorop *colorop; 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3856 struct drm_device *dev = plane->dev; 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3857 struct drm_i915_private *i915 = to_i915(dev); 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3858 int ret; 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3859 struct drm_colorop *prev_op; 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3860 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3861 colorop = intel_plane_colorop_create(CB_PLANE_PRE_CSC_LUT); No error checking 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3862 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3863 ret = drm_colorop_init(dev, >base, plane, DRM_COLOROP_1D_LUT); 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3864 if (ret) 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3865 return ret; 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3866 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 @3867 list->type = colorop->base.base.id; 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3868 list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", colorop->base.base.id); 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3869 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3870 /* TODO: handle failures and clean up*/ 5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3871 prev_op = >base; -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
[PATCH] drm/nouveau/mmu/r535: uninitialized variable in r535_bar_new_()
If gf100_bar_new_() fails then "bar" is not initialized. Fixes: 5bf0257136a2 ("drm/nouveau/mmu/r535: initial support") Signed-off-by: Dan Carpenter --- It looks like this was intended to handle a failure from the "rm" func but "rm" can't actually fail so it's easier to write the error handling for the code as-is. drivers/gpu/drm/nouveau/nvkm/subdev/bar/r535.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/bar/r535.c b/drivers/gpu/drm/nouveau/nvkm/subdev/bar/r535.c index 4135690326f4..3a30bea30e36 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/bar/r535.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/bar/r535.c @@ -168,12 +168,11 @@ r535_bar_new_(const struct nvkm_bar_func *hw, struct nvkm_device *device, rm->flush = r535_bar_flush; ret = gf100_bar_new_(rm, device, type, inst, ); - *pbar = bar; if (ret) { - if (!bar) - kfree(rm); + kfree(rm); return ret; } + *pbar = bar; bar->flushBAR2PhysMode = ioremap(device->func->resource_addr(device, 3), PAGE_SIZE); if (!bar->flushBAR2PhysMode) -- 2.43.0
[PATCH] drm/imx/dcss: fix resource size calculation
The resource is inclusive of the ->start and ->end addresses so this calculation is not correct. It should be "res->end - res->start + 1". Use the resource_size() to do the calculation. Fixes: 90393c9b5408 ("drm/imx/dcss: request memory region") Signed-off-by: Dan Carpenter --- >From static analysis. Not tested. drivers/gpu/drm/imx/dcss/dcss-dev.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/gpu/drm/imx/dcss/dcss-dev.c b/drivers/gpu/drm/imx/dcss/dcss-dev.c index 597e9b7bd4bf..7fd0c4c14205 100644 --- a/drivers/gpu/drm/imx/dcss/dcss-dev.c +++ b/drivers/gpu/drm/imx/dcss/dcss-dev.c @@ -167,7 +167,6 @@ struct dcss_dev *dcss_dev_create(struct device *dev, bool hdmi_output) struct resource *res; struct dcss_dev *dcss; const struct dcss_type_data *devtype; - resource_size_t res_len; devtype = of_device_get_match_data(dev); if (!devtype) { @@ -181,8 +180,7 @@ struct dcss_dev *dcss_dev_create(struct device *dev, bool hdmi_output) return ERR_PTR(-EINVAL); } - res_len = res->end - res->start; - if (!devm_request_mem_region(dev, res->start, res_len, "dcss")) { + if (!devm_request_mem_region(dev, res->start, resource_size(res), "dcss")) { dev_err(dev, "cannot request memory region\n"); return ERR_PTR(-EBUSY); } -- 2.43.0
[PATCH] drm/amd/display: Fix && vs || typos
These ANDs should be ORs or it will lead to a NULL dereference. Fixes: fb5a3d037082 ("drm/amd/display: Add NULL test for 'timing generator' in 'dcn21_set_pipe()'") Fixes: 886571d217d7 ("drm/amd/display: Fix 'panel_cntl' could be null in 'dcn21_set_backlight_level()'") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c b/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c index 5c7f380a84f9..7252f5f781f0 100644 --- a/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c +++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c @@ -211,7 +211,7 @@ void dcn21_set_pipe(struct pipe_ctx *pipe_ctx) struct dmcu *dmcu = pipe_ctx->stream->ctx->dc->res_pool->dmcu; uint32_t otg_inst; - if (!abm && !tg && !panel_cntl) + if (!abm || !tg || !panel_cntl) return; otg_inst = tg->inst; @@ -245,7 +245,7 @@ bool dcn21_set_backlight_level(struct pipe_ctx *pipe_ctx, struct panel_cntl *panel_cntl = pipe_ctx->stream->link->panel_cntl; uint32_t otg_inst; - if (!abm && !tg && !panel_cntl) + if (!abm || !tg || !panel_cntl) return false; otg_inst = tg->inst; -- 2.43.0
[bug report] drm/bridge: imx: add driver for HDMI TX Parallel Video Interface
Hello Lucas Stach, This is a semi-automatic email about new static checker warnings. drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:81 imx8mp_hdmi_pvi_bridge_enable() warn: variable dereferenced before check 'bridge_state' (see line 54) drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c 53 { 54 struct drm_atomic_state *state = bridge_state->base.state; ^^ bridge_state is dereferenced here. 55 struct imx8mp_hdmi_pvi *pvi = to_imx8mp_hdmi_pvi(bridge); 56 struct drm_connector_state *conn_state; 57 const struct drm_display_mode *mode; 58 struct drm_crtc_state *crtc_state; 59 struct drm_connector *connector; 60 u32 bus_flags, val; 61 62 connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder); 63 conn_state = drm_atomic_get_new_connector_state(state, connector); 64 crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); 65 66 if (WARN_ON(pm_runtime_resume_and_get(pvi->dev))) 67 return; 68 69 mode = _state->adjusted_mode; 70 71 val = FIELD_PREP(PVI_CTRL_MODE_MASK, PVI_CTRL_MODE_LCDIF) | PVI_CTRL_EN; 72 73 if (mode->flags & DRM_MODE_FLAG_PVSYNC) 74 val |= PVI_CTRL_OP_VSYNC_POL | PVI_CTRL_INP_VSYNC_POL; 75 76 if (mode->flags & DRM_MODE_FLAG_PHSYNC) 77 val |= PVI_CTRL_OP_HSYNC_POL | PVI_CTRL_INP_HSYNC_POL; 78 79 if (pvi->next_bridge->timings) 80 bus_flags = pvi->next_bridge->timings->input_bus_flags; 81 else if (bridge_state) This check for NULL is too late. Hopefully it can it be removed? 82 bus_flags = bridge_state->input_bus_cfg.flags; 83 regards, dan carpenter
[PATCH] drm/imagination: Fix an IS_ERR vs NULL bug in pvr_context_create()
The pvr_vm_context_lookup() function returns NULL on error (not error pointers). Update the check accordingly. Fixes: d2d79d29bb98 ("drm/imagination: Implement context creation/destruction ioctls") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/imagination/pvr_context.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/imagination/pvr_context.c b/drivers/gpu/drm/imagination/pvr_context.c index eded5e955cc0..e38d3578fc09 100644 --- a/drivers/gpu/drm/imagination/pvr_context.c +++ b/drivers/gpu/drm/imagination/pvr_context.c @@ -315,8 +315,8 @@ int pvr_context_create(struct pvr_file *pvr_file, struct drm_pvr_ioctl_create_co goto err_free_ctx; ctx->vm_ctx = pvr_vm_context_lookup(pvr_file, args->vm_context_handle); - if (IS_ERR(ctx->vm_ctx)) { - err = PTR_ERR(ctx->vm_ctx); + if (!ctx->vm_ctx) { + err = -EINVAL; goto err_free_ctx; } -- 2.43.0
Re: [PATCH next 10/11] block: Use a boolean expression instead of max() on booleans
On Mon, Jan 29, 2024 at 09:22:40AM +, David Laight wrote: > From: Jani Nikula > > Sent: 29 January 2024 09:08 > > > > On Sun, 28 Jan 2024, David Laight wrote: > > > blk_stack_limits() contains: > > > t->zoned = max(t->zoned, b->zoned); > > > These are bool, so it is just a bitwise or. > > > > Should be a logical or, really. And || in code. > > Not really, bitwise is fine for bool (especially for 'or') > and generates better code. For | vs || the type doesn't make a difference... It makes a difference for AND. "0x1 & 0x10" vs "0x1 && 0x10". regards, dan carpenter
[PATCH] drm/i915/gvt: Fix uninitialized variable in handle_mmio()
This code prints the wrong variable in the warning message. It should print "i" instead of "info->offset". On the first iteration "info" is uninitialized leading to a crash and on subsequent iterations it prints the previous offset instead of the current one. Fixes: e0f74ed4634d ("i915/gvt: Separate the MMIO tracking table from GVT-g") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/i915/gvt/handlers.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c index 90f6c1ece57d..efcb00472be2 100644 --- a/drivers/gpu/drm/i915/gvt/handlers.c +++ b/drivers/gpu/drm/i915/gvt/handlers.c @@ -2849,8 +2849,7 @@ static int handle_mmio(struct intel_gvt_mmio_table_iter *iter, u32 offset, for (i = start; i < end; i += 4) { p = intel_gvt_find_mmio_info(gvt, i); if (p) { - WARN(1, "dup mmio definition offset %x\n", - info->offset); + WARN(1, "dup mmio definition offset %x\n", i); /* We return -EEXIST here to make GVT-g load fail. * So duplicated MMIO can be found as soon as -- 2.43.0
[bug report] drm/amdkfd: Export DMABufs from KFD using GEM handles
Hello Felix Kuehling, The patch 1819200166ce: "drm/amdkfd: Export DMABufs from KFD using GEM handles" from Aug 24, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/dma-buf/dma-buf.c:729 dma_buf_get() warn: fd used after fd_install() 'fd' drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 809 static int kfd_mem_export_dmabuf(struct kgd_mem *mem) 810 { 811 if (!mem->dmabuf) { 812 struct amdgpu_device *bo_adev; 813 struct dma_buf *dmabuf; 814 int r, fd; 815 816 bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev); 817 r = drm_gem_prime_handle_to_fd(_adev->ddev, bo_adev->kfd.client.file, 818 mem->gem_handle, 819 mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ? 820 DRM_RDWR : 0, ); ^^^ The drm_gem_prime_handle_to_fd() function does an fd_install() and returns the result as "fd". 821 if (r) 822 return r; 823 dmabuf = dma_buf_get(fd); ^^ Then we do another fget() inside dma_buf_get(). I'm not an expert, but this looks wrong. We can't assume that the dmabuf here is the same one from drm_gem_prime_handle_to_fd() because the user could change it after the fd_install(). I suspect drm_gem_prime_handle_to_fd() should pass the dmabuf back instead. We had several CVEs similar to this such as CVE-2022-1998. 824 close_fd(fd); 825 if (WARN_ON_ONCE(IS_ERR(dmabuf))) 826 return PTR_ERR(dmabuf); 827 mem->dmabuf = dmabuf; 828 } 829 830 return 0; 831 } regards, dan carpenter
Re: [PATCH][next] drm/nouveau/fifo/gk104: remove redundant variable ret
Let's CC Felix on this one because he might know the answer. All day long I spend looking at code like this: net/core/dev.c:724 dev_fill_forward_path() info: returning a literal zero is cleaner net/core/dev.c:732 dev_fill_forward_path() info: returning a literal zero is cleaner net/core/dev.c 696 int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr, 697struct net_device_path_stack *stack) 698 { 699 const struct net_device *last_dev; 700 struct net_device_path_ctx ctx = { 701 .dev= dev, 702 }; 703 struct net_device_path *path; 704 int ret = 0; 705 706 memcpy(ctx.daddr, daddr, sizeof(ctx.daddr)); 707 stack->num_paths = 0; 708 while (ctx.dev && ctx.dev->netdev_ops->ndo_fill_forward_path) { 709 last_dev = ctx.dev; 710 path = dev_fwd_path(stack); 711 if (!path) 712 return -1; 713 714 memset(path, 0, sizeof(struct net_device_path)); 715 ret = ctx.dev->netdev_ops->ndo_fill_forward_path(, path); 716 if (ret < 0) This if condition might trick you into thinking that ->ndo_fill_forward_path() can return non-zero positive numbers, but it can't. It returns zero on success or negative error codes on failure. Smatch is doing cross function analysis so we know this. 717 return -1; 718 719 if (WARN_ON_ONCE(last_dev == ctx.dev)) 720 return -1; 721 } 722 723 if (!ctx.dev) 724 return ret; Is this intentional or not? Who knows? If this were an obvious bug, I could fix it right away but ambiguous stuff like this takes way more time to deal with. 725 726 path = dev_fwd_path(stack); 727 if (!path) 728 return -1; 729 path->type = DEV_PATH_ETHERNET; 730 path->dev = ctx.dev; 731 732 return ret; Obviously this is intentional, but if you were tricked by the checking earlier then you might assume that ret is some positive value from the last iteration through the loop. "return 0;" is so much clearer. 733 } regards, dan carpetner
Re: [PATCH][next] drm/nouveau/fifo/gk104: remove redundant variable ret
On Tue, Jan 23, 2024 at 12:04:23AM +0100, Danilo Krummrich wrote: > On 1/16/24 13:31, Dan Carpenter wrote: > > On Tue, Jan 16, 2024 at 11:16:09AM +, Colin Ian King wrote: > > > The variable ret is being assigned a value but it isn't being > > > read afterwards. The assignment is redundant and so ret can be > > > removed. > > > > > > Cleans up clang scan build warning: > > > warning: Although the value stored to 'ret' is used in the enclosing > > > expression, the value is never actually read from 'ret' > > > [deadcode.DeadStores] > > > > > > Signed-off-by: Colin Ian King > > > --- > > > drivers/gpu/drm/nouveau/nvif/fifo.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/nouveau/nvif/fifo.c > > > b/drivers/gpu/drm/nouveau/nvif/fifo.c > > > index a463289962b2..e96de14ce87e 100644 > > > --- a/drivers/gpu/drm/nouveau/nvif/fifo.c > > > +++ b/drivers/gpu/drm/nouveau/nvif/fifo.c > > > @@ -73,9 +73,9 @@ u64 > > > nvif_fifo_runlist(struct nvif_device *device, u64 engine) > > > { > > > u64 runm = 0; > > > - int ret, i; > > > + int i; > > > - if ((ret = nvif_fifo_runlists(device))) > > > + if (nvif_fifo_runlists(device)) > > > return runm; > > > > Could we return a literal zero here? Otherwise, I'm surprised this > > doesn't trigger a static checker warning. > > Why do you think so? Conditionally, runm is used later on as well. I don't > think the checker should complain about keeping the value single source. > > If you agree, want to offer your RB? If you look at v6.7 then probably 300 patches were from static analysis. The syzbot gets credit for 63 bugs and those bugs are more important because those are real life bugs. But static analysis is a huge in terms of just quantity. One of the most common bugs that static checkers complain about is missing error codes. It's a super common bug. Returning success instead of failure almost always results in NULL dereference or a use after free or some kind of crash. Fortunately, error paths seldom affect real life users. My published Smatch checks only complain about: if (ret) return ret; if (failure) return ret; I have a different check that I haven't published but I wish that I could which looks like: if (!ret) return ret; Here is a bug that check found recently. https://lore.kernel.org/all/9c81251b-bc87-4ca3-bb86-843dc85e5145@moroto.mountain/ I have a different unpublished check for every time ret is zero and we do: return ret; But I only review those warnings for specific code. Perhaps, I could make a warning for: if (failure) return ret; I'm sure I tried this in the past and it had more false positives than when we have an "if (ret) return ret;" like in the first example, but I can't remember. I could experiment with that a bit... To me, if "return ret;" and "return 0;" are the same, then "return 0;" is obviously more clear and looks more intentional. When I was looking at the code here, I had to consider the context. Especially when the patch was dealing with the "ret" variable it seemed suspicous. But "return 0;" is unamibuous. I don't have a problem with this patch, it's correct. But I really do think that "return 0;" is clearer than "return ret;" regards, dan carpenter
Re: [PATCH][next] drm/nouveau/fifo/gk104: remove redundant variable ret
On Tue, Jan 16, 2024 at 11:16:09AM +, Colin Ian King wrote: > The variable ret is being assigned a value but it isn't being > read afterwards. The assignment is redundant and so ret can be > removed. > > Cleans up clang scan build warning: > warning: Although the value stored to 'ret' is used in the enclosing > expression, the value is never actually read from 'ret' > [deadcode.DeadStores] > > Signed-off-by: Colin Ian King > --- > drivers/gpu/drm/nouveau/nvif/fifo.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nvif/fifo.c > b/drivers/gpu/drm/nouveau/nvif/fifo.c > index a463289962b2..e96de14ce87e 100644 > --- a/drivers/gpu/drm/nouveau/nvif/fifo.c > +++ b/drivers/gpu/drm/nouveau/nvif/fifo.c > @@ -73,9 +73,9 @@ u64 > nvif_fifo_runlist(struct nvif_device *device, u64 engine) > { > u64 runm = 0; > - int ret, i; > + int i; > > - if ((ret = nvif_fifo_runlists(device))) > + if (nvif_fifo_runlists(device)) > return runm; Could we return a literal zero here? Otherwise, I'm surprised this doesn't trigger a static checker warning. regards, dan carpenter
[PATCH] drm/nouveau/disp/r535: fix error code in r535_dp_aux_xfer()
This code was shuffled around but the return wasn't updated. It should return "ret" instead of "ctrl". Fixes: 4ae3a20102b2 ("nouveau/gsp: don't free ctrl messages on errors") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c b/drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c index 6a0a4d3b8902..027867c2a8c5 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c @@ -1080,7 +1080,7 @@ r535_dp_aux_xfer(struct nvkm_outp *outp, u8 type, u32 addr, u8 *data, u8 *psize) ret = nvkm_gsp_rm_ctrl_push(>rm.objcom, , sizeof(*ctrl)); if (ret) { nvkm_gsp_rm_ctrl_done(>rm.objcom, ctrl); - return PTR_ERR(ctrl); + return ret; } memcpy(data, ctrl->data, size); -- 2.43.0
[PATCH] drm/amdgpu: fix return value in aca_bank_hwip_is_matched()
The aca_bank_hwip_is_matched() function is type bool. This error path return -EINVAL which is cast to true, but it should return false instead. Fixes: 22a4fa4709e3 ("drm/amdgpu: implement RAS ACA driver framework") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c index 6074a529caf7..1d3ae7c241e5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c @@ -182,7 +182,7 @@ static bool aca_bank_hwip_is_matched(struct aca_bank *bank, enum aca_hwip_type t u64 ipid; if (!bank || type == ACA_HWIP_TYPE_UNKNOW) - return -EINVAL; + return false; hwip = _hwid_mcatypes[type]; if (!hwip->hwid) -- 2.43.0
[bug report] backlight: hx8357: Convert to agnostic GPIO API
Hello Andy Shevchenko, The patch 7d84a63a39b7: "backlight: hx8357: Convert to agnostic GPIO API" from Dec 7, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/video/backlight/hx8357.c:612 hx8357_probe() error: potential NULL/IS_ERR bug 'lcd->im_pins' drivers/video/backlight/hx8357.c 580 static int hx8357_probe(struct spi_device *spi) 581 { 582 struct device *dev = >dev; 583 struct lcd_device *lcdev; 584 struct hx8357_data *lcd; 585 const struct of_device_id *match; 586 int i, ret; 587 588 lcd = devm_kzalloc(>dev, sizeof(*lcd), GFP_KERNEL); 589 if (!lcd) 590 return -ENOMEM; 591 592 ret = spi_setup(spi); 593 if (ret < 0) { 594 dev_err(>dev, "SPI setup failed.\n"); 595 return ret; 596 } 597 598 lcd->spi = spi; 599 600 match = of_match_device(hx8357_dt_ids, >dev); 601 if (!match || !match->data) 602 return -EINVAL; 603 604 lcd->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); 605 if (IS_ERR(lcd->reset)) 606 return dev_err_probe(dev, PTR_ERR(lcd->reset), "failed to request reset GPIO\n"); 607 gpiod_set_consumer_name(lcd->reset, "hx8357-reset"); 608 609 lcd->im_pins = devm_gpiod_get_array_optional(dev, "im", GPIOD_OUT_LOW); 610 if (IS_ERR(lcd->im_pins)) 611 return dev_err_probe(dev, PTR_ERR(lcd->im_pins), "failed to request im GPIOs\n"); --> 612 if (lcd->im_pins->ndescs < HX8357_NUM_IM_PINS) I think _optional() functions can return NULL. 613 return dev_err_probe(dev, -EINVAL, "not enough im GPIOs\n"); 614 615 for (i = 0; i < HX8357_NUM_IM_PINS; i++) 616 gpiod_set_consumer_name(lcd->im_pins->desc[i], "im_pins"); 617 618 lcdev = devm_lcd_device_register(>dev, "mxsfb", >dev, lcd, 619 _ops); 620 if (IS_ERR(lcdev)) { 621 ret = PTR_ERR(lcdev); 622 return ret; 623 } 624 spi_set_drvdata(spi, lcdev); 625 626 hx8357_lcd_reset(lcdev); 627 628 ret = ((int (*)(struct lcd_device *))match->data)(lcdev); 629 if (ret) { 630 dev_err(>dev, "Couldn't initialize panel\n"); 631 return ret; 632 } 633 634 dev_info(>dev, "Panel probed\n"); 635 636 return 0; 637 } regards, dan carpenter
[PATCH] drm/xe: clean up type of GUC_HXG_MSG_0_ORIGIN
The GUC_HXG_MSG_0_ORIGIN definition should be unsigned. Currently it is defined as INT_MIN. This doesn't cause a problem currently but it's still worth cleaning up. Signed-off-by: Dan Carpenter --- drivers/gpu/drm/xe/abi/guc_messages_abi.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/xe/abi/guc_messages_abi.h b/drivers/gpu/drm/xe/abi/guc_messages_abi.h index 3d199016cf88..c04606872e48 100644 --- a/drivers/gpu/drm/xe/abi/guc_messages_abi.h +++ b/drivers/gpu/drm/xe/abi/guc_messages_abi.h @@ -40,7 +40,7 @@ */ #define GUC_HXG_MSG_MIN_LEN1u -#define GUC_HXG_MSG_0_ORIGIN (0x1 << 31) +#define GUC_HXG_MSG_0_ORIGIN (0x1U << 31) #define GUC_HXG_ORIGIN_HOST 0u #define GUC_HXG_ORIGIN_GUC 1u #define GUC_HXG_MSG_0_TYPE (0x7 << 28) -- 2.42.0
[bug report] drm/amdgpu/vpe: enable vpe dpm
Hello Peyton Lee, The patch 5f82a0c90cca: "drm/amdgpu/vpe: enable vpe dpm" from Dec 12, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c:62 vpe_u1_8_from_fraction() warn: unsigned 'numerator' is never less than zero. drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c:63 vpe_u1_8_from_fraction() warn: unsigned 'denominator' is never less than zero. drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c 60 static uint16_t vpe_u1_8_from_fraction(uint16_t numerator, uint16_t denominator) 61 { --> 62 bool arg1_negative = numerator < 0; 63 bool arg2_negative = denominator < 0; uint16_t can't be negative. 64 65 uint16_t arg1_value = (uint16_t)(arg1_negative ? -numerator : numerator); 66 uint16_t arg2_value = (uint16_t)(arg2_negative ? -denominator : denominator); 67 68 uint16_t remainder; 69 regards, dan carpenter
[PATCH] drm/xe/device: clean up on error in probe()
This error path should clean up before returning. Smatch detected this bug: drivers/gpu/drm/xe/xe_device.c:487 xe_device_probe() warn: missing unwind goto? Fixes: 4cb12b71923b ("drm/xe/xe2: Determine bios enablement for flat ccs on igfx") Signed-off-by: Dan Carpenter --- Speeking of static analysis, someone should probably run Sparse on this driver. There are some missing annotations. drivers/gpu/drm/xe/xe_mmio.h:65:72: warning: incorrect type in argument 1 (different address spaces) drivers/gpu/drm/xe/xe_mmio.h:65:72:expected void const volatile [noderef] __iomem *addr drivers/gpu/drm/xe/xe_mmio.h:65:72:got void * drivers/gpu/drm/xe/xe_device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c index d9ae77fe7382..b8d8da546670 100644 --- a/drivers/gpu/drm/xe/xe_device.c +++ b/drivers/gpu/drm/xe/xe_device.c @@ -484,7 +484,7 @@ int xe_device_probe(struct xe_device *xe) err = xe_device_set_has_flat_ccs(xe); if (err) - return err; + goto err_irq_shutdown; err = xe_mmio_probe_vram(xe); if (err) -- 2.42.0
[bug report] drm/xe: Introduce a new DRM driver for Intel GPUs
Hello Matthew Brost, The patch dd08ebf6c352: "drm/xe: Introduce a new DRM driver for Intel GPUs" from Mar 30, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/xe/xe_bo.c:2246 xe_bo_dumb_create() warn: potential integer overflow from user '((args->width * cpp)) + (((64)) - 1)' drivers/gpu/drm/xe/xe_bo.c 2234 int xe_bo_dumb_create(struct drm_file *file_priv, 2235 struct drm_device *dev, 2236 struct drm_mode_create_dumb *args) 2237 { 2238 struct xe_device *xe = to_xe_device(dev); 2239 struct xe_bo *bo; 2240 uint32_t handle; 2241 int cpp = DIV_ROUND_UP(args->bpp, 8); 2242 int err; 2243 u32 page_size = max_t(u32, PAGE_SIZE, 2244 xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ? SZ_64K : SZ_4K); 2245 --> 2246 args->pitch = ALIGN(args->width * cpp, 64); drm_mode_create_dumb() guarantees that "args->width * cpp" can't overflow but if we pick "args->width * cpp" set to U32_MAX - 63 or above then the ALIGN() can overflow to zero. I should have picked INT_MAX as the limit in drm_mode_create_dumb()... 2247 args->size = ALIGN(mul_u32_u32(args->pitch, args->height), 2248page_size); 2249 2250 bo = xe_bo_create_user(xe, NULL, NULL, args->size, 2251DRM_XE_GEM_CPU_CACHING_WC, 2252ttm_bo_type_device, 2253 XE_BO_CREATE_VRAM_IF_DGFX(xe_device_get_root_tile(xe)) | 2254XE_BO_CREATE_USER_BIT | XE_BO_SCANOUT_BIT | 2255XE_BO_NEEDS_CPU_ACCESS); 2256 if (IS_ERR(bo)) 2257 return PTR_ERR(bo); 2258 2259 err = drm_gem_handle_create(file_priv, >ttm.base, ); 2260 /* drop reference from allocate - handle holds it now */ 2261 drm_gem_object_put(>ttm.base); 2262 if (!err) 2263 args->handle = handle; 2264 return err; 2265 } regards, dan carpenter
[bug report] drm/xe: shift wrapping in xe_gem_create_ioctl()
Hello Matthew Brost, The patch dd08ebf6c352: "drm/xe: Introduce a new DRM driver for Intel GPUs" from Mar 30, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/xe/xe_bo.c:1916 xe_gem_create_ioctl() warn: potential integer overflow from user 'args->placement <<' drivers/gpu/drm/xe/xe_bo.c 1869 int xe_gem_create_ioctl(struct drm_device *dev, void *data, 1870 struct drm_file *file) 1871 { 1872 struct xe_device *xe = to_xe_device(dev); 1873 struct xe_file *xef = to_xe_file(file); 1874 struct drm_xe_gem_create *args = data; 1875 struct xe_vm *vm = NULL; 1876 struct xe_bo *bo; 1877 unsigned int bo_flags; 1878 u32 handle; 1879 int err; 1880 1881 if (XE_IOCTL_DBG(xe, args->extensions) || 1882 XE_IOCTL_DBG(xe, args->pad[0] || args->pad[1] || args->pad[2]) || 1883 XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1])) 1884 return -EINVAL; 1885 1886 /* at least one valid memory placement must be specified */ 1887 if (XE_IOCTL_DBG(xe, (args->placement & ~xe->info.mem_region_mask) || 1888 !args->placement)) 1889 return -EINVAL; 1890 1891 if (XE_IOCTL_DBG(xe, args->flags & 1892 ~(DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING | 1893DRM_XE_GEM_CREATE_FLAG_SCANOUT | 1894DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM))) 1895 return -EINVAL; 1896 1897 if (XE_IOCTL_DBG(xe, args->handle)) 1898 return -EINVAL; 1899 1900 if (XE_IOCTL_DBG(xe, !args->size)) 1901 return -EINVAL; 1902 1903 if (XE_IOCTL_DBG(xe, args->size > SIZE_MAX)) 1904 return -EINVAL; 1905 1906 if (XE_IOCTL_DBG(xe, args->size & ~PAGE_MASK)) 1907 return -EINVAL; 1908 1909 bo_flags = 0; 1910 if (args->flags & DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING) 1911 bo_flags |= XE_BO_DEFER_BACKING; 1912 1913 if (args->flags & DRM_XE_GEM_CREATE_FLAG_SCANOUT) 1914 bo_flags |= XE_BO_SCANOUT_BIT; 1915 --> 1916 bo_flags |= args->placement << (ffs(XE_BO_CREATE_SYSTEM_BIT) - 1); ^^^ Potential shift wrapping. 1917 1918 if (args->flags & DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM) { 1919 if (XE_IOCTL_DBG(xe, !(bo_flags & XE_BO_CREATE_VRAM_MASK))) 1920 return -EINVAL; 1921 1922 bo_flags |= XE_BO_NEEDS_CPU_ACCESS; 1923 } 1924 regards, dan carpenter
[PATCH] drm/xe/selftests: Fix an error pointer dereference bug
Check if "bo" is an error pointer before calling xe_bo_lock() on it. Fixes: d6abc18d6693 ("drm/xe/xe2: Modify xe_bo_test for system memory") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/xe/tests/xe_bo.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/xe/tests/xe_bo.c b/drivers/gpu/drm/xe/tests/xe_bo.c index 412b2e7ce40c..3436fd9cf2b2 100644 --- a/drivers/gpu/drm/xe/tests/xe_bo.c +++ b/drivers/gpu/drm/xe/tests/xe_bo.c @@ -125,14 +125,13 @@ static void ccs_test_run_tile(struct xe_device *xe, struct xe_tile *tile, bo = xe_bo_create_user(xe, NULL, NULL, SZ_1M, DRM_XE_GEM_CPU_CACHING_WC, ttm_bo_type_device, bo_flags); - - xe_bo_lock(bo, false); - if (IS_ERR(bo)) { KUNIT_FAIL(test, "Failed to create bo.\n"); return; } + xe_bo_lock(bo, false); + kunit_info(test, "Verifying that CCS data is cleared on creation.\n"); ret = ccs_test_migrate(tile, bo, false, 0ULL, 0xdeadbeefdeadbeefULL, test); -- 2.42.0
[PATCH] drm/xe: unlock on error path in xe_vm_add_compute_exec_queue()
Drop the ">lock" before returning. Fixes: 24f947d58fe5 ("drm/xe: Use DRM GPUVM helpers for external- and evicted objects") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/xe/xe_vm.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c index 9180f2d2d71d..4aa7979fe6bf 100644 --- a/drivers/gpu/drm/xe/xe_vm.c +++ b/drivers/gpu/drm/xe/xe_vm.c @@ -332,13 +332,13 @@ int xe_vm_add_compute_exec_queue(struct xe_vm *vm, struct xe_exec_queue *q) down_write(>lock); err = drm_gpuvm_exec_lock(_exec); if (err) - return err; + goto out_up_write; pfence = xe_preempt_fence_create(q, q->compute.context, ++q->compute.seqno); if (!pfence) { err = -ENOMEM; - goto out_unlock; + goto out_fini; } list_add(>compute.link, >preempt.exec_queues); @@ -361,8 +361,9 @@ int xe_vm_add_compute_exec_queue(struct xe_vm *vm, struct xe_exec_queue *q) up_read(>userptr.notifier_lock); -out_unlock: +out_fini: drm_exec_fini(exec); +out_up_write: up_write(>lock); return err; -- 2.42.0
Re: [PATCH 1/3] drm: property: One function call less in drm_property_create() after error detection
On Wed, Jan 03, 2024 at 06:18:13PM +0100, Michel Dänzer wrote: > On 2024-01-03 17:24, Markus Elfring wrote: > > > >> Out of curiosity, what exactly did Coccinelle report? > > > > Some SmPL scripts from my own selection tend to point questionable > > implementation details out. > > That doesn't answer my question. > > Without seeing the actual Coccinelle report, I'll assume that it didn't > actually call for this change. This isn't one of the Coccinelle scripts which ship with the kernel, it's something that Markus wrote himself. regards, dan carpenter
[bug report] drm/xe: missing access_ok() in ioctl
Hello Matthew Brost, The patch dd08ebf6c352: "drm/xe: Introduce a new DRM driver for Intel GPUs" from Mar 30, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/xe/xe_exec.c:163 xe_exec_ioctl() warn: calling '__copy_from_user()' without access_ok() drivers/gpu/drm/xe/xe_exec.c 102 int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file) 103 { 104 struct xe_device *xe = to_xe_device(dev); 105 struct xe_file *xef = to_xe_file(file); 106 struct drm_xe_exec *args = data; 107 struct drm_xe_sync __user *syncs_user = u64_to_user_ptr(args->syncs); 108 u64 __user *addresses_user = u64_to_user_ptr(args->address); 109 struct xe_exec_queue *q; 110 struct xe_sync_entry *syncs = NULL; 111 u64 addresses[XE_HW_ENGINE_MAX_INSTANCE]; 112 struct drm_gpuvm_exec vm_exec = {.extra.fn = xe_exec_fn}; 113 struct drm_exec *exec = _exec.exec; 114 u32 i, num_syncs = 0; 115 struct xe_sched_job *job; 116 struct dma_fence *rebind_fence; 117 struct xe_vm *vm; 118 bool write_locked; 119 ktime_t end = 0; 120 int err = 0; 121 122 if (XE_IOCTL_DBG(xe, args->extensions) || 123 XE_IOCTL_DBG(xe, args->pad[0] || args->pad[1] || args->pad[2]) || 124 XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1])) 125 return -EINVAL; 126 127 q = xe_exec_queue_lookup(xef, args->exec_queue_id); 128 if (XE_IOCTL_DBG(xe, !q)) 129 return -ENOENT; 130 131 if (XE_IOCTL_DBG(xe, q->flags & EXEC_QUEUE_FLAG_VM)) 132 return -EINVAL; 133 134 if (XE_IOCTL_DBG(xe, args->num_batch_buffer && 135 q->width != args->num_batch_buffer)) 136 return -EINVAL; 137 138 if (XE_IOCTL_DBG(xe, q->flags & EXEC_QUEUE_FLAG_BANNED)) { 139 err = -ECANCELED; 140 goto err_exec_queue; 141 } 142 143 if (args->num_syncs) { 144 syncs = kcalloc(args->num_syncs, sizeof(*syncs), GFP_KERNEL); 145 if (!syncs) { 146 err = -ENOMEM; 147 goto err_exec_queue; 148 } 149 } 150 151 vm = q->vm; 152 153 for (i = 0; i < args->num_syncs; i++) { 154 err = xe_sync_entry_parse(xe, xef, [num_syncs++], 155 _user[i], SYNC_PARSE_FLAG_EXEC | 156 (xe_vm_in_lr_mode(vm) ? 157SYNC_PARSE_FLAG_LR_MODE : 0)); 158 if (err) 159 goto err_syncs; 160 } 161 162 if (xe_exec_queue_is_parallel(q)) { --> 163 err = __copy_from_user(addresses, addresses_user, sizeof(u64) * 164q->width); It's been a long time since I've ever reported one of these. The difference between copy_from_user() and __copy_from_user() is that the underscore version means we called access_ok(addresses_user, sizeof(u64) * q->width) already. In olden days if we were doing multiple copies then it was nice to avoid doing duplicate checks on the fast path. I don't think we have called access_ok() here. At one point Linus said that if we don't call access_ok() in the same function then it should just automatically considered a bug. We used to have a lot of issues with this back in the day... I don't think anyone took Linus too seriously on this though. 165 if (err) { 166 err = -EFAULT; 167 goto err_syncs; 168 } 169 } 170 171 retry: 172 if (!xe_vm_in_lr_mode(vm) && xe_vm_userptr_check_repin(vm)) { 173 err = down_write_killable(>lock); 174 write_locked = true; 175 } else { regards, dan carpenter
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
[bug report] drm/xe: Introduce a new DRM driver for Intel GPUs
Hello Matthew Brost, The patch dd08ebf6c352: "drm/xe: Introduce a new DRM driver for Intel GPUs" from Mar 30, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/xe/tests/xe_bo.c:298 evict_test_run_tile() error: double unlocked 'external->ttm.base.resv' (orig line 220) drivers/gpu/drm/xe/tests/xe_bo.c 294 295 xe_bo_lock(external, false); 296 xe_bo_unpin_external(external); 297 xe_bo_unlock(external); This is a false positive, but the API is ugly... xe_bo_lock() is an interruptible lock if the second argument is true. Why not just create a xe_bo_lock_interruptible()? This has several advantages: 1) More readable and you could delete the comments explaining how to use it because it's so obvious. 2) Less typing because you wouldn't have to write ", false" so much. 3) You could mark the _interruptible version as must check and the other version could be a void function. 4) Smatch could parse it automatically. I can write some custom code to parse the existing function but it's a small hassle for me. regards, dan carpenter
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
Re: [PATCH 2/2] drm/imagination: Fix error path in pvr_vm_create_context
Thanks so much. Reviewed-by: Dan Carpenter regards, dan carpenter
Re: [PATCH v4 06/10] drm/panel: Add Synaptics R63353 panel driver
Hi Dario, 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/Dario-Binacchi/drm-bridge-Fix-bridge-disable-logic/20231205-185455 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20231205105341.4100896-7-dario.binacchi%40amarulasolutions.com patch subject: [PATCH v4 06/10] drm/panel: Add Synaptics R63353 panel driver config: i386-randconfig-r071-20231206 (https://download.01.org/0day-ci/archive/20231207/202312070214.eyi9t4eq-...@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce: (https://download.01.org/0day-ci/archive/20231207/202312070214.eyi9t4eq-...@intel.com/reproduce) 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/202312070214.eyi9t4eq-...@intel.com/ smatch warnings: drivers/gpu/drm/panel/panel-synaptics-r63353.c:132 r63353_panel_activate() warn: missing unwind goto? vim +132 drivers/gpu/drm/panel/panel-synaptics-r63353.c 64f91a53613aea Michael Trimarchi 2023-12-05 107 static int r63353_panel_activate(struct r63353_panel *rpanel) 64f91a53613aea Michael Trimarchi 2023-12-05 108 { 64f91a53613aea Michael Trimarchi 2023-12-05 109struct mipi_dsi_device *dsi = rpanel->dsi; 64f91a53613aea Michael Trimarchi 2023-12-05 110struct device *dev = >dev; 64f91a53613aea Michael Trimarchi 2023-12-05 111int i, ret; 64f91a53613aea Michael Trimarchi 2023-12-05 112 64f91a53613aea Michael Trimarchi 2023-12-05 113ret = mipi_dsi_dcs_soft_reset(dsi); 64f91a53613aea Michael Trimarchi 2023-12-05 114if (ret < 0) { 64f91a53613aea Michael Trimarchi 2023-12-05 115dev_err(dev, "Failed to do Software Reset (%d)\n", ret); 64f91a53613aea Michael Trimarchi 2023-12-05 116goto fail; 64f91a53613aea Michael Trimarchi 2023-12-05 117} 64f91a53613aea Michael Trimarchi 2023-12-05 118 64f91a53613aea Michael Trimarchi 2023-12-05 119usleep_range(15000, 17000); 64f91a53613aea Michael Trimarchi 2023-12-05 120 64f91a53613aea Michael Trimarchi 2023-12-05 121ret = mipi_dsi_dcs_enter_sleep_mode(dsi); 64f91a53613aea Michael Trimarchi 2023-12-05 122if (ret < 0) { 64f91a53613aea Michael Trimarchi 2023-12-05 123dev_err(dev, "Failed to enter sleep mode (%d)\n", ret); 64f91a53613aea Michael Trimarchi 2023-12-05 124goto fail; 64f91a53613aea Michael Trimarchi 2023-12-05 125} 64f91a53613aea Michael Trimarchi 2023-12-05 126 64f91a53613aea Michael Trimarchi 2023-12-05 127for (i = 0; i < rpanel->pdata->init_length; i++) { 64f91a53613aea Michael Trimarchi 2023-12-05 128const struct r63353_instr *instr = >pdata->init[i]; 64f91a53613aea Michael Trimarchi 2023-12-05 129 64f91a53613aea Michael Trimarchi 2023-12-05 130ret = mipi_dsi_dcs_write_buffer(dsi, instr->data, instr->len); 64f91a53613aea Michael Trimarchi 2023-12-05 131if (ret < 0) 64f91a53613aea Michael Trimarchi 2023-12-05 @132return ret; goto fail? 64f91a53613aea Michael Trimarchi 2023-12-05 133} 64f91a53613aea Michael Trimarchi 2023-12-05 134 64f91a53613aea Michael Trimarchi 2023-12-05 135msleep(120); 64f91a53613aea Michael Trimarchi 2023-12-05 136 64f91a53613aea Michael Trimarchi 2023-12-05 137ret = mipi_dsi_dcs_exit_sleep_mode(dsi); 64f91a53613aea Michael Trimarchi 2023-12-05 138if (ret < 0) { 64f91a53613aea Michael Trimarchi 2023-12-05 139dev_err(dev, "Failed to exit sleep mode (%d)\n", ret); 64f91a53613aea Michael Trimarchi 2023-12-05 140goto fail; 64f91a53613aea Michael Trimarchi 2023-12-05 141} 64f91a53613aea Michael Trimarchi 2023-12-05 142 64f91a53613aea Michael Trimarchi 2023-12-05 143usleep_range(5000, 1); 64f91a53613aea Michael Trimarchi 2023-12-05 144 64f91a53613aea Michael Trimarchi 2023-12-05 145ret = mipi_dsi_dcs_set_display_on(dsi); 64f91a53613aea Michael Trimarchi 2023-12-05 146if (ret < 0) { 64f91a53613aea Michael Trimarchi 2023-12-05 147dev_err(dev, "Failed to set display ON (%d)\n", ret); 64f91a53613aea Michael Trimarchi 2023-12-05 148goto fail; 64f91a53613aea Michael Trimarchi 2023-12-05 149} 64f91a53613aea Michael Trimarchi 2023-12-05 150 64f91a53613aea Michael Trimarchi 2023-12-05 151return 0; 64f91a53613aea Michael Trimarchi 2023-12-05 152 64f91a53613aea Michael Trimarchi 2023-12-05 153 fail: 64f91a53613aea Michael Trimarchi 2023-12-05 154 gpiod_
[PATCH] drm/bridge: nxp-ptn3460: simplify some error checking
The i2c_master_send/recv() functions return negative error codes or they return "len" on success. So the error handling here can be written as just normal checks for "if (ret < 0) return ret;". No need to complicate things. Btw, in this code the "len" parameter can never be zero, but even if it were, then I feel like this would still be the best way to write it. Fixes: 914437992876 ("drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking") Signed-off-by: Dan Carpenter --- This is not really a bug fix but I added a Fixes tag because I don't want people to pull my other commit without also applying this. drivers/gpu/drm/bridge/nxp-ptn3460.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c index 9b7eb8c669c1..7c0076e49953 100644 --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c @@ -54,15 +54,15 @@ static int ptn3460_read_bytes(struct ptn3460_bridge *ptn_bridge, char addr, int ret; ret = i2c_master_send(ptn_bridge->client, , 1); - if (ret <= 0) { + if (ret < 0) { DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); - return ret ?: -EIO; + return ret; } ret = i2c_master_recv(ptn_bridge->client, buf, len); - if (ret != len) { + if (ret < 0) { DRM_ERROR("Failed to recv i2c data, ret=%d\n", ret); - return ret < 0 ? ret : -EIO; + return ret; } return 0; @@ -78,9 +78,9 @@ static int ptn3460_write_byte(struct ptn3460_bridge *ptn_bridge, char addr, buf[1] = val; ret = i2c_master_send(ptn_bridge->client, buf, ARRAY_SIZE(buf)); - if (ret != ARRAY_SIZE(buf)) { + if (ret < 0) { DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); - return ret < 0 ? ret : -EIO; + return ret; } return 0; -- 2.42.0
[PATCH] drm/imagination: Move dereference after NULL check in pvr_mmu_backing_page_init()
This code dereferences "page->pvr_dev" and then checked for NULL on the next line. Re-order it to avoid a potential NULL pointer dereference. Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/imagination/pvr_mmu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c index c8562bfc0dcd..4fe70610ed94 100644 --- a/drivers/gpu/drm/imagination/pvr_mmu.c +++ b/drivers/gpu/drm/imagination/pvr_mmu.c @@ -316,12 +316,14 @@ pvr_mmu_backing_page_init(struct pvr_mmu_backing_page *page, static void pvr_mmu_backing_page_fini(struct pvr_mmu_backing_page *page) { - struct device *dev = from_pvr_device(page->pvr_dev)->dev; + struct device *dev; /* Do nothing if no allocation is present. */ if (!page->pvr_dev) return; + dev = from_pvr_device(page->pvr_dev)->dev; + dma_unmap_page(dev, page->dma_addr, PVR_MMU_BACKING_PAGE_SIZE, DMA_TO_DEVICE); -- 2.42.0
[bug report] drm/imagination: Implement MIPS firmware processor and MMU support
Hello Sarah Walker, The patch 927f3e0253c1: "drm/imagination: Implement MIPS firmware processor and MMU support" from Nov 22, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/imagination/pvr_vm_mips.c:204 pvr_vm_mips_map() warn: 'pfn' can be negative (type promoted to high) drivers/gpu/drm/imagination/pvr_vm_mips.c 144 int 145 pvr_vm_mips_map(struct pvr_device *pvr_dev, struct pvr_fw_object *fw_obj) 146 { 147 struct pvr_fw_device *fw_dev = _dev->fw_dev; 148 struct pvr_fw_mips_data *mips_data = fw_dev->processor_data.mips_data; 149 struct pvr_gem_object *pvr_obj = fw_obj->gem; 150 const u64 start = fw_obj->fw_mm_node.start; 151 const u64 size = fw_obj->fw_mm_node.size; 152 u64 end; 153 u32 cache_policy; 154 u32 pte_flags; 155 u32 start_pfn; 156 u32 end_pfn; 157 s32 pfn; pfn is s32 but start_pfn is u32. 158 int err; 159 160 if (check_add_overflow(start, size - 1, )) 161 return -EINVAL; 162 163 if (start < ROGUE_FW_HEAP_BASE || 164 start >= ROGUE_FW_HEAP_BASE + fw_dev->fw_heap_info.raw_size || 165 end < ROGUE_FW_HEAP_BASE || 166 end >= ROGUE_FW_HEAP_BASE + fw_dev->fw_heap_info.raw_size || 167 (start & ROGUE_MIPSFW_PAGE_MASK_4K) || 168 ((end + 1) & ROGUE_MIPSFW_PAGE_MASK_4K)) 169 return -EINVAL; 170 171 start_pfn = (start & fw_dev->fw_heap_info.offset_mask) >> ROGUE_MIPSFW_LOG2_PAGE_SIZE_4K; Can start_pfn be zero? 172 end_pfn = (end & fw_dev->fw_heap_info.offset_mask) >> ROGUE_MIPSFW_LOG2_PAGE_SIZE_4K; 173 174 if (pvr_obj->flags & PVR_BO_FW_FLAGS_DEVICE_UNCACHED) 175 cache_policy = ROGUE_MIPSFW_UNCACHED_CACHE_POLICY; 176 else 177 cache_policy = mips_data->cache_policy; 178 179 pte_flags = get_mips_pte_flags(true, true, cache_policy); 180 181 for (pfn = start_pfn; pfn <= end_pfn; pfn++) { 182 dma_addr_t dma_addr; 183 u32 pte; 184 185 err = pvr_fw_object_get_dma_addr(fw_obj, 186 (pfn - start_pfn) << 187 ROGUE_MIPSFW_LOG2_PAGE_SIZE_4K, 188 _addr); 189 if (err) 190 goto err_unmap_pages; 191 192 pte = ((dma_addr >> ROGUE_MIPSFW_LOG2_PAGE_SIZE_4K) 193<< ROGUE_MIPSFW_ENTRYLO_PFN_SHIFT) & mips_data->pfn_mask; 194 pte |= pte_flags; 195 196 WRITE_ONCE(mips_data->pt[pfn], pte); 197 } 198 199 pvr_mmu_flush_request_all(pvr_dev); 200 201 return 0; 202 203 err_unmap_pages: --> 204 for (; pfn >= start_pfn; pfn--) If start_pfn can be zero then this is an endless loop. I've seen this code in other places as well. This loop is slightly off as well. It should decrement pfn on the first iteration. There are hack arounds for using unsigned iterators: while (pfn-- > start_pfn) WRITE_ONCE(mips_data->pt[pfn], 0); Personally I would be tempted to make things signed... Here are some links to various rants I have wrote: https://staticthinking.wordpress.com/2022/06/01/unsigned-int-i-is-stupid/ https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/ You didn't ask for rants... No one ever asks for rants... 205 WRITE_ONCE(mips_data->pt[pfn], 0); 206 207 pvr_mmu_flush_request_all(pvr_dev); 208 WARN_ON(pvr_mmu_flush_exec(pvr_dev, true)); 209 210 return err; 211 } regards, dan carpenter
[bug report] drm/imagination: Implement firmware infrastructure and META FW support
Hello Sarah Walker, The patch cc1aeedb98ad: "drm/imagination: Implement firmware infrastructure and META FW support" from Nov 22, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/imagination/pvr_vm.c:631 pvr_vm_create_context() error: 'vm_ctx->mmu_ctx' dereferencing possible ERR_PTR() drivers/gpu/drm/imagination/pvr_vm.c 597 vm_ctx = kzalloc(sizeof(*vm_ctx), GFP_KERNEL); 598 if (!vm_ctx) 599 return ERR_PTR(-ENOMEM); 600 601 drm_gem_private_object_init(_dev->base, _ctx->dummy_gem, 0); 602 603 vm_ctx->pvr_dev = pvr_dev; 604 kref_init(_ctx->ref_count); 605 mutex_init(_ctx->lock); 606 607 drm_gpuvm_init(_ctx->gpuvm_mgr, 608is_userspace_context ? "PowerVR-user-VM" : "PowerVR-FW-VM", 6090, _dev->base, _ctx->dummy_gem, 6100, 1ULL << device_addr_bits, 0, 0, _vm_gpuva_ops); 611 612 vm_ctx->mmu_ctx = pvr_mmu_context_create(pvr_dev); 613 err = PTR_ERR_OR_ZERO(_ctx->mmu_ctx); ^ s/&//. The address is never an error pointer so this will always return 0. Fixing this will silence the static checker but there are some other issues as well. 614 if (err) { 615 vm_ctx->mmu_ctx = NULL; Setting vm_ctx->mmu_ctx is not sufficient. 616 goto err_put_ctx; 617 } 618 619 if (is_userspace_context) { 620 err = pvr_fw_object_create(pvr_dev, sizeof(struct rogue_fwif_fwmemcontext), 621 PVR_BO_FW_FLAGS_DEVICE_UNCACHED, 622fw_mem_context_init, vm_ctx, _ctx->fw_mem_ctx_obj); 623 624 if (err) 625 goto err_page_table_destroy; 626 } 627 628 return vm_ctx; 629 630 err_page_table_destroy: --> 631 pvr_mmu_context_destroy(vm_ctx->mmu_ctx); This will lead to a double free. Delete. 632 633 err_put_ctx: 634 pvr_vm_context_put(vm_ctx); The pvr_vm_context_put() function does: kref_put(_ctx->ref_count, pvr_vm_context_release); I really think that with kref free functions the way to do it is to wait until the last possible momement when everything has been allocated before we set up the kref release code. Here the pvr_vm_context_release() will call: pvr_mmu_context_destroy(vm_ctx->mmu_ctx); We already did that on line 631 as mentioned so it's a double free. But also imagine if vm_ctx->mmu_ctx is NULL, then it will lead to a NULL dereference. The pvr_vm_context_release() function has several WARN() functions that trigger if not everything is set up. It's complicated to review. So I kind of always think that people should manually kfree() things in their allocation functions and then do a kref_init() at the end. 635 636 return ERR_PTR(err); 637 } regards, dan carpenter
[bug report] drm/imagination: Implement firmware infrastructure and META FW support
Hello Sarah Walker, The patch cc1aeedb98ad: "drm/imagination: Implement firmware infrastructure and META FW support" from Nov 22, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/imagination/pvr_fw_startstop.c:210 pvr_fw_stop() warn: odd mask '0x & 0x' drivers/gpu/drm/imagination/pvr_fw_startstop.c:213 pvr_fw_stop() warn: odd mask '0x & 0x' drivers/gpu/drm/imagination/pvr_fw_startstop.c:216 pvr_fw_stop() warn: odd mask '0x & 0x' drivers/gpu/drm/imagination/pvr_fw_startstop.c:219 pvr_fw_stop() warn: odd mask '0x & 0x' drivers/gpu/drm/imagination/pvr_fw_startstop.c 187 int 188 pvr_fw_stop(struct pvr_device *pvr_dev) 189 { 190 const u32 sidekick_idle_mask = ROGUE_CR_SIDEKICK_IDLE_MASKFULL & 191 ~(ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN | 192 ROGUE_CR_SIDEKICK_IDLE_SOCIF_EN | 193 ROGUE_CR_SIDEKICK_IDLE_HOSTIF_EN); 194 bool skip_garten_idle = false; 195 u32 reg_value; 196 int err; 197 198 /* 199 * Wait for Sidekick/Jones to signal IDLE except for the Garten Wrapper. 200 * For cores with the LAYOUT_MARS feature, SIDEKICK would have been 201 * powered down by the FW. 202 */ 203 err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE, sidekick_idle_mask, 204 sidekick_idle_mask, POLL_TIMEOUT_USEC); 205 if (err) 206 return err; 207 208 /* Unset MTS DM association with threads. */ 209 pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_INTCTX_THREAD0_DM_ASSOC, --> 210ROGUE_CR_MTS_INTCTX_THREAD0_DM_ASSOC_MASKFULL & 211 ROGUE_CR_MTS_INTCTX_THREAD0_DM_ASSOC_DM_ASSOC_CLRMSK); What's the point of these masks? They don't overlap so they just equal zero. 212 pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_BGCTX_THREAD0_DM_ASSOC, 213ROGUE_CR_MTS_BGCTX_THREAD0_DM_ASSOC_MASKFULL & 214 ROGUE_CR_MTS_BGCTX_THREAD0_DM_ASSOC_DM_ASSOC_CLRMSK); 215 pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC, 216ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC_MASKFULL & 217 ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC_DM_ASSOC_CLRMSK); 218 pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC, 219ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC_MASKFULL & 220 ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC_DM_ASSOC_CLRMSK); 221 222 /* Extra Idle checks. */ 223 err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_BIF_STATUS_MMU, 0, regards, dan carpenter
[PATCH] drm/msm/dp: Fix platform_get_irq() check
The platform_get_irq() function returns negative error codes. It never returns zero. Fix the check accordingly. Fixes: 82c2a5751227 ("drm/msm/dp: tie dp_display_irq_handler() with dp driver") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/msm/dp/dp_display.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 61b7103498a7..d80cb3d14c6b 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1166,9 +1166,9 @@ static int dp_display_request_irq(struct dp_display_private *dp) struct platform_device *pdev = dp->dp_display.pdev; dp->irq = platform_get_irq(pdev, 0); - if (!dp->irq) { + if (dp->irq < 0) { DRM_ERROR("failed to get irq\n"); - return -EINVAL; + return dp->irq; } rc = devm_request_irq(>dev, dp->irq, dp_display_irq_handler, -- 2.42.0
Re: [PATCH] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking
On Tue, Dec 05, 2023 at 03:04:49PM +0100, Robert Foss wrote: > On Tue, Dec 5, 2023, 15:01 Dan Carpenter wrote: > > > On Tue, Dec 05, 2023 at 02:48:26PM +0100, Robert Foss wrote: > > > On Mon, 4 Dec 2023 15:29:00 +0300, Dan Carpenter wrote: > > > > The i2c_master_send/recv() functions return negative error codes or the > > > > number of bytes that were able to be sent/received. This code has > > > > two problems. 1) Instead of checking if all the bytes were sent or > > > > received, it checks that at least one byte was sent or received. > > > > 2) If there was a partial send/receive then we should return a negative > > > > error code but this code returns success. > > > > > > > > [...] > > > > > > Applied, thanks! > > > > > > [1/1] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking > > > https://cgit.freedesktop.org/drm/drm-misc/commit/?id=914437992876 > > > > > > > Wait. That was unexpected. Neil's review comments were correct. I was > > planning to send a v2 patch which was just a cleanup. > > > > Sorry Dan, I was too quick on the draw. Can you send a fixup and I'll apply > it too? > Sure. I will do that. regards, dan carpenter
Re: [PATCH] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking
On Tue, Dec 05, 2023 at 02:48:26PM +0100, Robert Foss wrote: > On Mon, 4 Dec 2023 15:29:00 +0300, Dan Carpenter wrote: > > The i2c_master_send/recv() functions return negative error codes or the > > number of bytes that were able to be sent/received. This code has > > two problems. 1) Instead of checking if all the bytes were sent or > > received, it checks that at least one byte was sent or received. > > 2) If there was a partial send/receive then we should return a negative > > error code but this code returns success. > > > > [...] > > Applied, thanks! > > [1/1] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking > https://cgit.freedesktop.org/drm/drm-misc/commit/?id=914437992876 > Wait. That was unexpected. Neil's review comments were correct. I was planning to send a v2 patch which was just a cleanup. regards, dan carpenter
Re: Kunit drm_test_check_plane_state: EXPECTATION FAILED at drivers/gpu/drm/tests/drm_plane_helper_test.c:123
On Tue, Dec 05, 2023 at 09:37:05AM +0100, Maxime Ripard wrote: > Hi Naresh, > > Thanks for the report > > On Mon, Dec 04, 2023 at 11:05:36PM +0530, Naresh Kamboju wrote: > > The Kunit drm_plane_helper failed on all devices running Linux next-20231204 > > > > ## Test Regressions (compared to next-20231201) > > * qemu-armv7, kunit and > > * x86, kunit > > - drm_test_check_invalid_plane_state_downscaling_invalid > > - drm_test_check_invalid_plane_state_drm_plane_helper > > - drm_test_check_invalid_plane_state_drm_test_check_invalid_plane_state > > - drm_test_check_invalid_plane_state_positioning_invalid > > - drm_test_check_invalid_plane_state_upscaling_invalid > > - drm_test_check_plane_state_clipping_rotate_reflect > > - drm_test_check_plane_state_clipping_simple > > - drm_test_check_plane_state_downscaling > > - drm_test_check_plane_state_drm_test_check_plane_state > > - drm_test_check_plane_state_positioning_simple > > - drm_test_check_plane_state_rounding1 > > - drm_test_check_plane_state_rounding2 > > - drm_test_check_plane_state_rounding3 > > - drm_test_check_plane_state_rounding4 > > - drm_test_check_plane_state_upscaling > > I found the source of failure to be f1e75da5364e ("drm/atomic: Loosen FB > atomic checks"). > > Fortunately for us, it's already been reverted yesterday for some > unrelated reason, so it should be fixed in next-20231205 onward. Sorry, that's a bummer that these patches were reverted. :( The whole episode was a bit unfortunate... Qualcom has been working on those patches for a year. They must not be using kunit testing as part of their QC... It's some kind of communication failure on our part. Hopefully we can get this all sorted out and re-apply the patches soon. regards, dan carpenter
Re: [PATCH] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking
On Mon, Dec 04, 2023 at 02:53:05PM +0100, Neil Armstrong wrote: > On 04/12/2023 13:29, Dan Carpenter wrote: > > The i2c_master_send/recv() functions return negative error codes or the > > number of bytes that were able to be sent/received. This code has > > two problems. 1) Instead of checking if all the bytes were sent or > > received, it checks that at least one byte was sent or received. > > 2) If there was a partial send/receive then we should return a negative > > error code but this code returns success. > > > > Fixes: a9fe713d7d45 ("drm/bridge: Add PTN3460 bridge driver") > > Cc: sta...@vger.kernel.org > > Signed-off-by: Dan Carpenter > > --- > > This is from static analysis and code review. It's always a concern > > when you add stricter error handling that something will break. > > > > drivers/gpu/drm/bridge/nxp-ptn3460.c | 10 +- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c > > b/drivers/gpu/drm/bridge/nxp-ptn3460.c > > index d81920227a8a..9b7eb8c669c1 100644 > > --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c > > +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c > > @@ -56,13 +56,13 @@ static int ptn3460_read_bytes(struct ptn3460_bridge > > *ptn_bridge, char addr, > > ret = i2c_master_send(ptn_bridge->client, , 1); > > if (ret <= 0) { > > DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); > > - return ret; > > + return ret ?: -EIO; > > } > > ret = i2c_master_recv(ptn_bridge->client, buf, len); > > - if (ret <= 0) { > > + if (ret != len) { > > This is impossible, i2c_transfer_buffer_flags() returns len as-is if no > error, so > ret can only be negative or equal to len. The original code is right. It works, but it's not "right". The <= 0 could be changed to < 0. The "len" variable is EDID_LENGTH (128). regards, dan carpenter
[bug report] drm/amdgpu: Workaround to skip kiq ring test during ras gpu recovery
Hello Stanley.Yang, The patch b1338a8e71ac: "drm/amdgpu: Workaround to skip kiq ring test during ras gpu recovery" from Oct 17, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c:604 amdgpu_get_xgmi_hive() warn: sleeping in atomic context drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c 591 struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct amdgpu_device *adev) 592 { 593 struct amdgpu_hive_info *hive = NULL; 594 int ret; 595 596 if (!adev->gmc.xgmi.hive_id) 597 return NULL; 598 599 if (adev->hive) { 600 kobject_get(>hive->kobj); 601 return adev->hive; 602 } 603 --> 604 mutex_lock(_mutex); ^^^ Shhh The mutexes are sleeping. 605 606 list_for_each_entry(hive, _hive_list, node) { The caller is amdgpu_gfx_disable_kcq(): drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c 516 spin_lock(>ring_lock); ^^^ Holding a spin lock. 517 if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size * 518 adev->gfx.num_compute_rings)) { 519 spin_unlock(>ring_lock); 520 return -ENOMEM; 521 } 522 523 for (i = 0; i < adev->gfx.num_compute_rings; i++) { 524 j = i + xcc_id * adev->gfx.num_compute_rings; 525 kiq->pmf->kiq_unmap_queues(kiq_ring, 526 >gfx.compute_ring[j], 527 RESET_QUEUES, 0, 0); 528 } 529 530 /** 531 * This is workaround: only skip kiq_ring test 532 * during ras recovery in suspend stage for gfx9.4.3 533 */ 534 hive = amdgpu_get_xgmi_hive(adev); ^^ Can't call a sleeping function when holding a spin_lock. 535 if (hive) { 536 hive_ras_recovery = atomic_read(>ras_recovery); 537 amdgpu_put_xgmi_hive(hive); 538 } 539 540 ras = amdgpu_ras_get_context(adev); 541 if ((amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 3)) && 542 ras && (atomic_read(>in_recovery) || hive_ras_recovery)) { 543 spin_unlock(>ring_lock); 544 return 0; 545 } 546 547 if (kiq_ring->sched.ready && !adev->job_hang) 548 r = amdgpu_ring_test_helper(kiq_ring); 549 spin_unlock(>ring_lock); regards, dan carpenter