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
