Is this a valid way to code a VHDL async reset?

I have picked up some VHDL code to maintain that has a reset written in a way I'm not familiar with. I'm not sure if it's valid, and my simulator (Modelsim) is giving warnings that I wouldn't expect. Example of the pattern:

process(clk, reset_n)
begin
  if reset_n = '0' then
    counter <= (others=>'0');
  end if;
  if rising_edge(clk) then
    case state is
    when IDLE      =>
      if signal_a = signal_b then
        state <= DATA;
      end if;
    when DATA => 
      state <= IDLE;
    when others =>
    end case;
    if state = DATA then
      counter <= counter + 1;
    else
      counter <= (others => '0');
    end if;
  end if;
end process;

Modelsim warns that state, signal_a and signal_b are "read in the process but is not in the sensitivity list". I would not expect that as they are in a clocked block and the process is sensitive to the clock.

Is this a valid coding style for an async reset? I would expect to see elsif rising_edge(clk) instead, but understand this would cause problems here with the mix of other non-reset signals (state).

Solution

This pattern likely wont behave as you expect, and also likely wont synthesise (and if it does, it wont match the code).

This code has the clock overriding the reset. If reset is asserted ('0') then the counter will be reset, but if clk is running it will the run as if reset_n is '1'. This is because processes are only triggered by a 'event' on a signal in the sensitivity list, so the reset will only occur when reset_n changes to '0'. This doesnt usually matter in VHDL as the reset branch will have higher priority than the clock. So if it is clocked, the reset branch is taken. But here, because the reset has lower priority, it is as if reset_n is a negitive edge clock.

On the actual hardware, this is different. Sensitivity lists are normally ignored and the hardware is built from the code in the process. I would expect that this will fail to build either because it will complain about no logic matching the template, or dual edge flops are not allowed.

To fix this, either make the clock branch an elsif as you suggested, and you would also need to add state to the reset to avoid an accidental clock enable on state, or move the reset to the bottom of the process, where it will override the clock assignment properly, and allow you to have a mix of reset and non-reset registers in the same process.