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