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

[Steven Price]

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