Re: [bug report] drm/panthor: Add the scheduler logical block

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/04/2024 15:34, Dan Carpenter wrote:
> 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 = &ptdev->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(&ptdev->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.

Sorry, I didn't mean just a WARN_ON() - there should also be an early
return from the function, ideally with the GPU being reset in the hope
that it starts behaving again.

And you're probably right WARN_ON is a bit too strong, we should
definitely output a warning message though to point out that the
hardware isn't behaving as expected.

> 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.

Well the static checker is not wrong - but the situation where this
could happen is if the hardware (or firmware running on the hardware)
breaks the contract of the specification. I'll put it on my todo list to
take a look at handling this more gracefully, but it's likely to take a
while before it bubbles to the top. But thanks for pointing out the issue.

Steve



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux