* [bug report] drm/amdgpu: add ring buffer information in devcoredump
@ 2024-03-15 15:16 Dan Carpenter
2024-03-15 19:08 ` Khatri, Sunil
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2024-03-15 15:16 UTC (permalink / raw)
To: sunil.khatri; +Cc: amd-gfx
Hello Sunil Khatri,
Commit 42742cc541bb ("drm/amdgpu: add ring buffer information in
devcoredump") from Mar 11, 2024 (linux-next), leads to the following
Smatch static checker warning:
drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c:219 amdgpu_devcoredump_read()
error: we previously assumed 'coredump->adev' could be null (see line 206)
drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
171 static ssize_t
172 amdgpu_devcoredump_read(char *buffer, loff_t offset, size_t count,
173 void *data, size_t datalen)
174 {
175 struct drm_printer p;
176 struct amdgpu_coredump_info *coredump = data;
177 struct drm_print_iterator iter;
178 int i;
179
180 iter.data = buffer;
181 iter.offset = 0;
182 iter.start = offset;
183 iter.remain = count;
184
185 p = drm_coredump_printer(&iter);
186
187 drm_printf(&p, "**** AMDGPU Device Coredump ****\n");
188 drm_printf(&p, "version: " AMDGPU_COREDUMP_VERSION "\n");
189 drm_printf(&p, "kernel: " UTS_RELEASE "\n");
190 drm_printf(&p, "module: " KBUILD_MODNAME "\n");
191 drm_printf(&p, "time: %lld.%09ld\n", coredump->reset_time.tv_sec,
192 coredump->reset_time.tv_nsec);
193
194 if (coredump->reset_task_info.pid)
195 drm_printf(&p, "process_name: %s PID: %d\n",
196 coredump->reset_task_info.process_name,
197 coredump->reset_task_info.pid);
198
199 if (coredump->ring) {
200 drm_printf(&p, "\nRing timed out details\n");
201 drm_printf(&p, "IP Type: %d Ring Name: %s\n",
202 coredump->ring->funcs->type,
203 coredump->ring->name);
204 }
205
206 if (coredump->adev) {
^^^^^^^^^^^^^^
Check for NULL
207 struct amdgpu_vm_fault_info *fault_info =
208 &coredump->adev->vm_manager.fault_info;
209
210 drm_printf(&p, "\n[%s] Page fault observed\n",
211 fault_info->vmhub ? "mmhub" : "gfxhub");
212 drm_printf(&p, "Faulty page starting at address: 0x%016llx\n",
213 fault_info->addr);
214 drm_printf(&p, "Protection fault status register: 0x%x\n\n",
215 fault_info->status);
216 }
217
218 drm_printf(&p, "Ring buffer information\n");
--> 219 for (int i = 0; i < coredump->adev->num_rings; i++) {
^^^^^^^^^^^^^^
Unchecked dereference
220 int j = 0;
221 struct amdgpu_ring *ring = coredump->adev->rings[i];
222
223 drm_printf(&p, "ring name: %s\n", ring->name);
224 drm_printf(&p, "Rptr: 0x%llx Wptr: 0x%llx RB mask: %x\n",
225 amdgpu_ring_get_rptr(ring),
226 amdgpu_ring_get_wptr(ring),
227 ring->buf_mask);
228 drm_printf(&p, "Ring size in dwords: %d\n",
229 ring->ring_size / 4);
230 drm_printf(&p, "Ring contents\n");
231 drm_printf(&p, "Offset \t Value\n");
232
233 while (j < ring->ring_size) {
234 drm_printf(&p, "0x%x \t 0x%x\n", j, ring->ring[j/4]);
235 j += 4;
236 }
237 }
238
239 if (coredump->reset_vram_lost)
240 drm_printf(&p, "VRAM is lost due to GPU reset!\n");
241 if (coredump->adev->reset_info.num_regs) {
^^^^^^^^^^^^^^
Here too
242 drm_printf(&p, "AMDGPU register dumps:\nOffset: Value:\n");
243
244 for (i = 0; i < coredump->adev->reset_info.num_regs; i++)
245 drm_printf(&p, "0x%08x: 0x%08x\n",
246 coredump->adev->reset_info.reset_dump_reg_list[i],
247 coredump->adev->reset_info.reset_dump_reg_value[i]);
248 }
249
250 return count - iter.remain;
251 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] drm/amdgpu: add ring buffer information in devcoredump
2024-03-15 15:16 [bug report] drm/amdgpu: add ring buffer information in devcoredump Dan Carpenter
@ 2024-03-15 19:08 ` Khatri, Sunil
2024-03-16 9:12 ` Dan Carpenter
2024-03-18 10:32 ` Christian König
0 siblings, 2 replies; 5+ messages in thread
From: Khatri, Sunil @ 2024-03-15 19:08 UTC (permalink / raw)
To: Dan Carpenter, sunil.khatri, christian.koenig, Alex Deucher; +Cc: amd-gfx
[-- Attachment #1: Type: text/plain, Size: 5573 bytes --]
Thanks for pointing these. I do have some doubt and i raised inline.
On 3/15/2024 8:46 PM, Dan Carpenter wrote:
> Hello Sunil Khatri,
>
> Commit 42742cc541bb ("drm/amdgpu: add ring buffer information in
> devcoredump") from Mar 11, 2024 (linux-next), leads to the following
> Smatch static checker warning:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c:219 amdgpu_devcoredump_read()
> error: we previously assumed 'coredump->adev' could be null (see line 206)
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
> 171 static ssize_t
> 172 amdgpu_devcoredump_read(char *buffer, loff_t offset, size_t count,
> 173 void *data, size_t datalen)
> 174 {
> 175 struct drm_printer p;
> 176 struct amdgpu_coredump_info *coredump = data;
> 177 struct drm_print_iterator iter;
> 178 int i;
> 179
> 180 iter.data = buffer;
> 181 iter.offset = 0;
> 182 iter.start = offset;
> 183 iter.remain = count;
> 184
> 185 p = drm_coredump_printer(&iter);
> 186
> 187 drm_printf(&p, "**** AMDGPU Device Coredump ****\n");
> 188 drm_printf(&p, "version: " AMDGPU_COREDUMP_VERSION "\n");
> 189 drm_printf(&p, "kernel: " UTS_RELEASE "\n");
> 190 drm_printf(&p, "module: " KBUILD_MODNAME "\n");
> 191 drm_printf(&p, "time: %lld.%09ld\n", coredump->reset_time.tv_sec,
> 192 coredump->reset_time.tv_nsec);
> 193
> 194 if (coredump->reset_task_info.pid)
> 195 drm_printf(&p, "process_name: %s PID: %d\n",
> 196 coredump->reset_task_info.process_name,
> 197 coredump->reset_task_info.pid);
> 198
> 199 if (coredump->ring) {
> 200 drm_printf(&p, "\nRing timed out details\n");
> 201 drm_printf(&p, "IP Type: %d Ring Name: %s\n",
> 202 coredump->ring->funcs->type,
> 203 coredump->ring->name);
> 204 }
> 205
> 206 if (coredump->adev) {
> ^^^^^^^^^^^^^^
> Check for NULL
This is the check for NULL. Is there any issue here ?
>
> 207 struct amdgpu_vm_fault_info *fault_info =
> 208 &coredump->adev->vm_manager.fault_info;
> 209
> 210 drm_printf(&p, "\n[%s] Page fault observed\n",
> 211 fault_info->vmhub ? "mmhub" : "gfxhub");
> 212 drm_printf(&p, "Faulty page starting at address: 0x%016llx\n",
> 213 fault_info->addr);
> 214 drm_printf(&p, "Protection fault status register: 0x%x\n\n",
> 215 fault_info->status);
> 216 }
> 217
> 218 drm_printf(&p, "Ring buffer information\n");
> --> 219 for (int i = 0; i < coredump->adev->num_rings; i++) {
> ^^^^^^^^^^^^^^
> Unchecked dereference
Agree
>
> 220 int j = 0;
> 221 struct amdgpu_ring *ring = coredump->adev->rings[i];
> 222
> 223 drm_printf(&p, "ring name: %s\n", ring->name);
> 224 drm_printf(&p, "Rptr: 0x%llx Wptr: 0x%llx RB mask: %x\n",
> 225 amdgpu_ring_get_rptr(ring),
> 226 amdgpu_ring_get_wptr(ring),
> 227 ring->buf_mask);
> 228 drm_printf(&p, "Ring size in dwords: %d\n",
> 229 ring->ring_size / 4);
> 230 drm_printf(&p, "Ring contents\n");
> 231 drm_printf(&p, "Offset \t Value\n");
> 232
> 233 while (j < ring->ring_size) {
> 234 drm_printf(&p, "0x%x \t 0x%x\n", j, ring->ring[j/4]);
> 235 j += 4;
> 236 }
> 237 }
> 238
> 239 if (coredump->reset_vram_lost)
> 240 drm_printf(&p, "VRAM is lost due to GPU reset!\n");
> 241 if (coredump->adev->reset_info.num_regs) {
> ^^^^^^^^^^^^^^
> Here too
Agree.
>
> 242 drm_printf(&p, "AMDGPU register dumps:\nOffset: Value:\n");
> 243
> 244 for (i = 0; i < coredump->adev->reset_info.num_regs; i++)
> 245 drm_printf(&p, "0x%08x: 0x%08x\n",
> 246 coredump->adev->reset_info.reset_dump_reg_list[i],
> 247 coredump->adev->reset_info.reset_dump_reg_value[i]);
> 248 }
> 249
> 250 return count - iter.remain;
> 251 }
Although adev is a global structure and never in the code it is being
checked for NULL as it wont be NULL until the driver is unloaded. I can
add a check for adev in the beginning of the function
amdgpu_devcoredump_read for completeness of the tool but still not very
sure of it.
Christian and Alex Do you agree with my understanding the adev does
really need a validation for NULL. I dint see throughout the code adev
to be validated for NULL. Do you recommend to add a check for NULL for
adev in the above mentioned function/places.
Regards Sunil
>
> regards,
> dan carpenter
[-- Attachment #2: Type: text/html, Size: 7012 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] drm/amdgpu: add ring buffer information in devcoredump
2024-03-15 19:08 ` Khatri, Sunil
@ 2024-03-16 9:12 ` Dan Carpenter
2024-03-18 6:00 ` Khatri, Sunil
2024-03-18 10:32 ` Christian König
1 sibling, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2024-03-16 9:12 UTC (permalink / raw)
To: Khatri, Sunil; +Cc: sunil.khatri, christian.koenig, Alex Deucher, amd-gfx
The static checker is just complaining about NULL checking that doesn't
make sense. It raises the question, can the pointer be NULL or not?
Based on your comments and from reviewing the code, I do not think it
can be NULL. Thus the correct thing is to remove the unnecessary NULL
check.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [bug report] drm/amdgpu: add ring buffer information in devcoredump
2024-03-16 9:12 ` Dan Carpenter
@ 2024-03-18 6:00 ` Khatri, Sunil
0 siblings, 0 replies; 5+ messages in thread
From: Khatri, Sunil @ 2024-03-18 6:00 UTC (permalink / raw)
To: Dan Carpenter
Cc: Koenig, Christian, Deucher, Alexander,
amd-gfx@lists.freedesktop.org
[AMD Official Use Only - General]
Got it. Thanks for reported that. Sent the patch for review.
Regards
Sunil khatri
-----Original Message-----
From: Dan Carpenter <dan.carpenter@linaro.org>
Sent: Saturday, March 16, 2024 2:42 PM
To: Khatri, Sunil <Sunil.Khatri@amd.com>
Cc: Khatri, Sunil <Sunil.Khatri@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [bug report] drm/amdgpu: add ring buffer information in devcoredump
The static checker is just complaining about NULL checking that doesn't make sense. It raises the question, can the pointer be NULL or not?
Based on your comments and from reviewing the code, I do not think it can be NULL. Thus the correct thing is to remove the unnecessary NULL check.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] drm/amdgpu: add ring buffer information in devcoredump
2024-03-15 19:08 ` Khatri, Sunil
2024-03-16 9:12 ` Dan Carpenter
@ 2024-03-18 10:32 ` Christian König
1 sibling, 0 replies; 5+ messages in thread
From: Christian König @ 2024-03-18 10:32 UTC (permalink / raw)
To: Khatri, Sunil, Dan Carpenter, sunil.khatri, christian.koenig,
Alex Deucher
Cc: amd-gfx
[-- Attachment #1: Type: text/plain, Size: 5942 bytes --]
Am 15.03.24 um 20:08 schrieb Khatri, Sunil:
>
> Thanks for pointing these. I do have some doubt and i raised inline.
>
> On 3/15/2024 8:46 PM, Dan Carpenter wrote:
>> Hello Sunil Khatri,
>>
>> Commit 42742cc541bb ("drm/amdgpu: add ring buffer information in
>> devcoredump") from Mar 11, 2024 (linux-next), leads to the following
>> Smatch static checker warning:
>>
>> drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c:219 amdgpu_devcoredump_read()
>> error: we previously assumed 'coredump->adev' could be null (see line 206)
>>
>> drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
>> 171 static ssize_t
>> 172 amdgpu_devcoredump_read(char *buffer, loff_t offset, size_t count,
>> 173 void *data, size_t datalen)
>> 174 {
>> 175 struct drm_printer p;
>> 176 struct amdgpu_coredump_info *coredump = data;
>> 177 struct drm_print_iterator iter;
>> 178 int i;
>> 179
>> 180 iter.data = buffer;
>> 181 iter.offset = 0;
>> 182 iter.start = offset;
>> 183 iter.remain = count;
>> 184
>> 185 p = drm_coredump_printer(&iter);
>> 186
>> 187 drm_printf(&p, "**** AMDGPU Device Coredump ****\n");
>> 188 drm_printf(&p, "version: " AMDGPU_COREDUMP_VERSION "\n");
>> 189 drm_printf(&p, "kernel: " UTS_RELEASE "\n");
>> 190 drm_printf(&p, "module: " KBUILD_MODNAME "\n");
>> 191 drm_printf(&p, "time: %lld.%09ld\n", coredump->reset_time.tv_sec,
>> 192 coredump->reset_time.tv_nsec);
>> 193
>> 194 if (coredump->reset_task_info.pid)
>> 195 drm_printf(&p, "process_name: %s PID: %d\n",
>> 196 coredump->reset_task_info.process_name,
>> 197 coredump->reset_task_info.pid);
>> 198
>> 199 if (coredump->ring) {
>> 200 drm_printf(&p, "\nRing timed out details\n");
>> 201 drm_printf(&p, "IP Type: %d Ring Name: %s\n",
>> 202 coredump->ring->funcs->type,
>> 203 coredump->ring->name);
>> 204 }
>> 205
>> 206 if (coredump->adev) {
>> ^^^^^^^^^^^^^^
>> Check for NULL
> This is the check for NULL. Is there any issue here ?
>> 207 struct amdgpu_vm_fault_info *fault_info =
>> 208 &coredump->adev->vm_manager.fault_info;
>> 209
>> 210 drm_printf(&p, "\n[%s] Page fault observed\n",
>> 211 fault_info->vmhub ? "mmhub" : "gfxhub");
>> 212 drm_printf(&p, "Faulty page starting at address: 0x%016llx\n",
>> 213 fault_info->addr);
>> 214 drm_printf(&p, "Protection fault status register: 0x%x\n\n",
>> 215 fault_info->status);
>> 216 }
>> 217
>> 218 drm_printf(&p, "Ring buffer information\n");
>> --> 219 for (int i = 0; i < coredump->adev->num_rings; i++) {
>> ^^^^^^^^^^^^^^
>> Unchecked dereference
> Agree
>> 220 int j = 0;
>> 221 struct amdgpu_ring *ring = coredump->adev->rings[i];
>> 222
>> 223 drm_printf(&p, "ring name: %s\n", ring->name);
>> 224 drm_printf(&p, "Rptr: 0x%llx Wptr: 0x%llx RB mask: %x\n",
>> 225 amdgpu_ring_get_rptr(ring),
>> 226 amdgpu_ring_get_wptr(ring),
>> 227 ring->buf_mask);
>> 228 drm_printf(&p, "Ring size in dwords: %d\n",
>> 229 ring->ring_size / 4);
>> 230 drm_printf(&p, "Ring contents\n");
>> 231 drm_printf(&p, "Offset \t Value\n");
>> 232
>> 233 while (j < ring->ring_size) {
>> 234 drm_printf(&p, "0x%x \t 0x%x\n", j, ring->ring[j/4]);
>> 235 j += 4;
>> 236 }
>> 237 }
>> 238
>> 239 if (coredump->reset_vram_lost)
>> 240 drm_printf(&p, "VRAM is lost due to GPU reset!\n");
>> 241 if (coredump->adev->reset_info.num_regs) {
>> ^^^^^^^^^^^^^^
>> Here too
> Agree.
>> 242 drm_printf(&p, "AMDGPU register dumps:\nOffset: Value:\n");
>> 243
>> 244 for (i = 0; i < coredump->adev->reset_info.num_regs; i++)
>> 245 drm_printf(&p, "0x%08x: 0x%08x\n",
>> 246 coredump->adev->reset_info.reset_dump_reg_list[i],
>> 247 coredump->adev->reset_info.reset_dump_reg_value[i]);
>> 248 }
>> 249
>> 250 return count - iter.remain;
>> 251 }
>
>
> Although adev is a global structure and never in the code it is being
> checked for NULL as it wont be NULL until the driver is unloaded. I
> can add a check for adev in the beginning of the function
> amdgpu_devcoredump_read for completeness of the tool but still not
> very sure of it.
>
> Christian and Alex Do you agree with my understanding the adev does
> really need a validation for NULL. I dint see throughout the code adev
> to be validated for NULL. Do you recommend to add a check for NULL for
> adev in the above mentioned function/places.
>
No, that doesn't make sense. adev is mandatory to be around for the core
dump to be valid and accessible.
Without a device you don't have a core dump in the first place.
Regards,
Christian.
> Regards Sunil
>
>> regards,
>> dan carpenter
[-- Attachment #2: Type: text/html, Size: 7457 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-03-18 10:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-15 15:16 [bug report] drm/amdgpu: add ring buffer information in devcoredump Dan Carpenter
2024-03-15 19:08 ` Khatri, Sunil
2024-03-16 9:12 ` Dan Carpenter
2024-03-18 6:00 ` Khatri, Sunil
2024-03-18 10:32 ` Christian König
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox