Re: [PATCH net-next 1/6] netdevsim: psp: reset spi on key rotation and check for exhaustion on alloc

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Daniel Zahka wrote:
> The PSP spec states that the lower 31b of the SPI need to be
> non-zero. Though not in the spec, I think it is reasonable to reset
> the lower 31b of the spi space after a key rotation, and to also
> decline to generate session keys when the lower 31b saturate.

Since this is already a 6 patch series, these two independent changes
could be separate patches.

Agreed on both points btw.
 
> Assisted-by: Claude:claude-opus-4.7
> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
> ---
>  drivers/net/netdevsim/psp.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/netdevsim/psp.c b/drivers/net/netdevsim/psp.c
> index 6936ecb8173e..5073bda60883 100644
> --- a/drivers/net/netdevsim/psp.c
> +++ b/drivers/net/netdevsim/psp.c
> @@ -132,14 +132,14 @@ nsim_rx_spi_alloc(struct psp_dev *psd, u32 version,
>  		  struct netlink_ext_ack *extack)
>  {
>  	struct netdevsim *ns = psd->drv_priv;
> -	unsigned int new;
>  	int i;
>  
> -	new = ++ns->psp.spi & PSP_SPI_KEY_ID;
> -	if (psd->generation & 1)
> -		new |= PSP_SPI_KEY_PHASE;
> +	if ((ns->psp.spi ^ (ns->psp.spi + 1)) & PSP_SPI_KEY_PHASE) {
> +		NL_SET_ERR_MSG(extack, "SPI space exhausted");
> +		return -ENOSPC;
> +	}

Can this all be more readable without the use of XORs? This is not hot
path code that needs to be optimized.

        if (ns->psp.spi & PSP_SPI_KEY_ID == INT32_MAX) {
                NL_SET_ERR_MSG(extack, "SPI space exhausted");
                return -ENOSPC;
        }
  
> -	assoc->spi = cpu_to_be32(new);
> +	assoc->spi = cpu_to_be32(++ns->psp.spi);
>  	assoc->key[0] = psd->generation;
>  	for (i = 1; i < PSP_MAX_KEY; i++)
>  		assoc->key[i] = ns->psp.spi + i;
> @@ -162,6 +162,10 @@ static int nsim_assoc_add(struct psp_dev *psd, struct psp_assoc *pas,
>  
>  static int nsim_key_rotate(struct psp_dev *psd, struct netlink_ext_ack *extack)
>  {
> +	struct netdevsim *ns = psd->drv_priv;
> +
> +	ns->psp.spi = (ns->psp.spi & PSP_SPI_KEY_PHASE) ^ PSP_SPI_KEY_PHASE;
> +

        /* Flip key phase and reset SPI to 0 within that space
         * (will be pre-incremented, as 0 is an invalid SPI)
         */
        if (ns->psp.spi & PSP_SPI_KEY_PHASE)
                ns->psp.spi = 0;
        else
                ns->psp.spi = PSP_SPI_KEY_PHASE;

Even while making the code more self evident I still feel it needs a
comment to explain all that is going on.

Maybe it can be more self-evident.

Or maybe you find the XOR just as readable already.

Your call. A comment might be helpful either way.


>  	return 0;
>  }
>  
> 
> -- 
> 2.52.0
>