8 bit sequential multiplier using add and shift

I'm designing an 8-bit signed sequential multiplier using Verilog. The inputs are clk (clock), rst (reset), a (8 bit multiplier), b (8 bit multiplicand), and the outputs are p (product) and rdy (ready signal, indicating multiplication is over). For negative inputs, I do a sign extension and save it in the 15 bit register variables multiplier and multiplicand. Here's my code:

module seq_mult (p, rdy, clk, reset, a, b);
   input clk, reset;
   input [7:0] a, b;
   output [15:0] p;
   output rdy;
   
   reg [15:0] p;
   reg [15:0] multiplier;
   reg [15:0] multiplicand;
   reg rdy;
   reg [4:0] ctr;

always @(posedge clk or posedge reset) begin
    if (reset) 
    begin
    rdy     <= 0;
    p   <= 0;
    ctr     <= 0;
    multiplier <= {{8{a[7]}}, a};
    multiplicand <= {{8{b[7]}}, b};
    end 
    else 
    begin 
      if(ctr < 16) 
          begin
          if(multiplier[ctr]==1)
              begin
              multiplicand = multiplicand<<ctr;
              p <= p + multiplicand;
              end
          ctr <= ctr+1;
          end
       else 
           begin
           rdy <= 1;
           end
    end
  end //End of always block
    
endmodule

And here's my testbench:

`timescale 1ns/1ns
`define width 8
`define TESTFILE "test_in.dat"

module seq_mult_tb () ;
    reg signed [`width-1:0] a, b;
    reg             clk, reset;

    wire signed [2*`width-1:0] p;
    wire           rdy;

    integer total, err;
    integer i, s, fp, numtests;

    // Golden reference - can be automatically generated in this case
    // otherwise store and read from a file
    wire signed [2*`width-1:0] ans = a*b;

    // Device under test - always use named mapping of signals to ports
    seq_mult dut( .clk(clk),
        .reset(reset),
        .a(a),
        .b(b),
        .p(p),
        .rdy(rdy));

    // Set up 10ns clock
    always #5 clk = !clk;

    // A task to automatically run till the rdy signal comes back from DUT
    task apply_and_check;
        input [`width-1:0] ain;
        input [`width-1:0] bin;
        begin
            // Set the inputs
            a = ain;
            b = bin;
            // Reset the DUT for one clock cycle
            reset = 1;
            @(posedge clk);
            // Remove reset 
            #1 reset = 0;

            // Loop until the DUT indicates 'rdy'
            while (rdy == 0) begin
                @(posedge clk); // Wait for one clock cycle
            end
            if (p == ans) begin
                $display($time, " Passed %d * %d = %d", a, b, p);
            end else begin
                $display($time, " Fail %d * %d: %d instead of %d", a, b, p, ans);
                err = err + 1;
            end
            total = total + 1;
        end
    endtask // apply_and_check

    initial begin
        // Initialize the clock 
        clk = 1;
        // Counters to track progress
        total = 0;
        err = 0;

        // Get all inputs from file: 1st line has number of inputs
        fp = $fopen(`TESTFILE, "r");
        s = $fscanf(fp, "%d\n", numtests);
        // Sequences of values pumped through DUT 
        for (i=0; i<numtests; i=i+1) begin
            s = $fscanf(fp, "%d %d\n", a, b);
            apply_and_check(a, b);
        end
        if (err > 0) begin
            $display("FAIL %d out of %d", err, total);
        end else begin
            $display("PASS %d tests", total);
        end
        $finish;
    end

endmodule // seq_mult_tb

I also created a file called test_in.dat in which the test cases are stored (first line indicates number of test cases):

Now the problem is: the code works for only the first two inputs and for the last two inputs. For the remaining inputs, I get a different number than is expected. Can someone point out any logical error in my code that is causing this? Or if there's a much simpler strategy for doing the same, please let me know of that as well.

Solution

multiplicand is shifted to the left by ctr in each iteration if multiplier[ctr] is 1.

But ctr already includes the previous shift amounts, so you are shifting too far.

You should just shift multiplicand by 1 in every iteration unconditionally:

multiplicand <= multiplicand << 1;
if (multiplier[ctr] == 1)
begin
    p <= p + multiplicand;
end
ctr <= ctr + 1;

You should also use nonblocking assignment for multiplicand. You might need to move the shifting to after adding it to p.