Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 21 Jun 2023 17:34:29 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     carlos.fernandez@technica-engineering.de
Cc:     davem@davemloft.net, edumazet@google.com, pabeni@redhat.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Sabrina Dubroca <sd@queasysnail.net>
Subject: Re: [PATCH v3] net: macsec SCI assignment for ES = 0
Message-ID: <20230621173429.18348fc8@kernel.org>
In-Reply-To: <20230620091301.21981-1-carlos.fernandez@technica-engineering.de>
References: <20230620091301.21981-1-carlos.fernandez@technica-engineering.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

A few nit picks and questions, when you repost please make sure to CC 
Sabrina Dubroca <sd@queasysnail.net>

On Tue, 20 Jun 2023 11:13:01 +0200
carlos.fernandez@technica-engineering.de wrote:
> -static sci_t macsec_frame_sci(struct macsec_eth_header *hdr, bool sci_present)
> +static sci_t macsec_frame_sci(struct macsec_eth_header *hdr, bool sci_present,
> +			      struct macsec_rxh_data *rxd)
>  {
> +	struct macsec_dev *macsec_device;
>  	sci_t sci;
>  
> -	if (sci_present)
> +	if (sci_present) {
>  		memcpy(&sci, hdr->secure_channel_id,
> -		       sizeof(hdr->secure_channel_id));
> -	else
> +			sizeof(hdr->secure_channel_id));

the alignment of sizeof() was correct, don't change it

> +	} else if (0 == (hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC))) {

Just
	} else if (hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC)) {

> +		list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
> +			struct macsec_rx_sc *rx_sc;
> +			struct macsec_secy *secy = &macsec_device->secy;

You should reorder these two declaration, networking likes local
variable declaration lines longest to shortest.

> +			for_each_rxsc(secy, rx_sc) {
> +				rx_sc = rx_sc ? macsec_rxsc_get(rx_sc) : NULL;
> +				if (rx_sc && rx_sc->active)
> +					return rx_sc->sci;
> +			}

I haven't looked in detail but are you possibly returning rx_sc->sci
here just to ...

> +		}
> +		/* If not found, use MAC in hdr as default*/
>  		sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> -
> +	} else {
> +		sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> +	}
>  	return sci;
>  }
>  
> @@ -1150,11 +1165,12 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
>  
>  	macsec_skb_cb(skb)->has_sci = !!(hdr->tci_an & MACSEC_TCI_SC);
>  	macsec_skb_cb(skb)->assoc_num = hdr->tci_an & MACSEC_AN_MASK;
> -	sci = macsec_frame_sci(hdr, macsec_skb_cb(skb)->has_sci);
>  
>  	rcu_read_lock();
>  	rxd = macsec_data_rcu(skb->dev);
>  
> +	sci = macsec_frame_sci(hdr, macsec_skb_cb(skb)->has_sci, rxd);
> +
>  	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
>  		struct macsec_rx_sc *sc = find_rx_sc(&macsec->secy, sci);

... look up the rx_sc based on the sci? 
-- 
pw-bot: cr