re: mlxsw: spectrum: Adjust headroom buffers for 8x ports

re: mlxsw: spectrum: Adjust headroom buffers for 8x ports

[Colin Ian King]

Hi

Static analysis with Coverity has detected an issue that relies on the
machine endianness to work. The commit in question is:

commit 60833d54d56c21e7538296eb2e00e104768fd047
Author: Ido Schimmel <idosch@mellanox.com>
Date:   Tue Jun 16 10:14:58 2020 +0300

    mlxsw: spectrum: Adjust headroom buffers for 8x ports

in line:
    mlxsw_sp_port_headroom_8x_adjust(mlxsw_sp_port, (u16 *) &buffsize);


The cast of the u32 buffsize to (u16 *) to scale buffsize in the call to
to mlxsw_sp_port_headroom_8x_adjust() will behave differently on big
endian architectures to that of little endian architectures.  I'm not
sure if this is intentional or not.

One solution is to either make buffsize a u16, but I am concerned this
may be incorrect as the buffsize is assigned from the call
mlxsw_sp_span_buffsize_get() and this returns a u32 so we may have
overflow issues. Probably better to make
mlxsw_sp_port_headroom_8x_adjust handle u32 integers and to return the
adjusted value rather than modifying it by pass-by-reference.

Colin

Re: mlxsw: spectrum: Adjust headroom buffers for 8x ports

[Ido Schimmel]

On Wed, Jun 17, 2020 at 06:15:35PM +0100, Colin Ian King wrote:
> Hi
> 
> Static analysis with Coverity has detected an issue that relies on the
> machine endianness to work. The commit in question is:
> 
> commit 60833d54d56c21e7538296eb2e00e104768fd047
> Author: Ido Schimmel <idosch@mellanox.com>
> Date:   Tue Jun 16 10:14:58 2020 +0300
> 
>     mlxsw: spectrum: Adjust headroom buffers for 8x ports
> 
> in line:
>     mlxsw_sp_port_headroom_8x_adjust(mlxsw_sp_port, (u16 *) &buffsize);
> 
> 
> The cast of the u32 buffsize to (u16 *) to scale buffsize in the call to
> to mlxsw_sp_port_headroom_8x_adjust() will behave differently on big
> endian architectures to that of little endian architectures.  I'm not
> sure if this is intentional or not.

Not intentional. The hardware interface is quite weird. The buffer size
can be configured via two registers. One takes size as 24 bits and the
second as 16 bits. Either way, the max size is much lower than 2^16-1.

> One solution is to either make buffsize a u16, but I am concerned this
> may be incorrect as the buffsize is assigned from the call
> mlxsw_sp_span_buffsize_get() and this returns a u32 so we may have
> overflow issues. Probably better to make
> mlxsw_sp_port_headroom_8x_adjust handle u32 integers and to return the
> adjusted value rather than modifying it by pass-by-reference.

Thanks for the report, will fix.

> 
> Colin
>