[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
[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_ccb.c:277 pvr_kccb_send_cmd_reserved_powered() warn: odd binop '0x0 & 0xf' drivers/gpu/drm/imagination/pvr_ccb.c 268 WRITE_ONCE(pvr_dev->kccb.rtn[old_write_offset], 269ROGUE_FWIF_KCCB_RTN_SLOT_NO_RESPONSE); 270 } 271 mb(); /* memory barrier */ 272 WRITE_ONCE(ctrl->write_offset, new_write_offset); 273 pvr_dev->kccb.reserved_count--; 274 275 /* Kick MTS */ 276 pvr_fw_mts_schedule(pvr_dev, --> 277 PVR_FWIF_DM_GP & ~ROGUE_CR_MTS_SCHEDULE_DM_CLRMSK); ^^ PVR_FWIF_DM_GP is zero. 278 279 out_unlock: 280 mutex_unlock(_ccb->lock); 281 } regards, dan carpenter
