Re: [PATCH v3] net: macsec SCI assignment for ES = 0

[Jakub Kicinski]

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

On Tue, 20 Jun 2023 11:13:01 +0200
carlos.fernandez@xxxxxxxxxxxxxxxxxxxxxxx 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