netdev - Re: [LINUX RFC PATCH] net: macb: Add support for partial store and forward

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 13 Dec 2022 17:35:12 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Pranavi Somisetty <pranavi.somisetty@....com>
Cc:     <nicolas.ferre@...rochip.com>, <claudiu.beznea@...rochip.com>,
        <davem@...emloft.net>, <edumazet@...gle.com>, <pabeni@...hat.com>,
        <git@....com>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <michal.simek@....com>,
        <harini.katakam@....com>, <radhey.shyam.pandey@....com>
Subject: Re: [LINUX RFC PATCH] net: macb: Add support for partial store and
 forward

On Tue, 13 Dec 2022 05:12:45 -0700 Pranavi Somisetty wrote:
> From: Maulik Jodhani <maulik.jodhani@...inx.com>
> 
> - Validate FCS in receive interrupt handler if Rx checksum offloading
>   is disabled
> - Get rx-watermark value from DT

Sounds like two separate changes, please split into two patches

> @@ -1375,6 +1385,16 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
>  				 bp->rx_buffer_size, DMA_FROM_DEVICE);
>  
>  		skb->protocol = eth_type_trans(skb, bp->dev);
> +
> +		/* Validate MAC fcs if RX checsum offload disabled */
> +		if (!(bp->dev->features & NETIF_F_RXCSUM)) {

RXCSUM is for L4 (TCP/UDP) checksums, FCS is simply assumed 
to be validated by HW.

> +			if (macb_validate_hw_csum(skb)) {
> +				netdev_err(bp->dev, "incorrect FCS\n");

This can flood logs, and is likely unnecessary since we have a
dedicated statistics for crc errors (rx_crc_errors).

> +				bp->dev->stats.rx_dropped++;

CRC errors are errors not drops see the comment above struct
rtnl_link_stats64 for more info.

> +				break;
> +			}
> +		}

> @@ -3812,10 +3862,29 @@ static void macb_configure_caps(struct macb *bp,
>  				const struct macb_config *dt_conf)
>  {
>  	u32 dcfg;
> +	int retval;
>  
>  	if (dt_conf)
>  		bp->caps = dt_conf->caps;
>  
> +	/* By default we set to partial store and forward mode for zynqmp.
> +	 * Disable if not set in devicetree.
> +	 */
> +	if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
> +		retval = of_property_read_u16(bp->pdev->dev.of_node,
> +					      "rx-watermark",
> +					      &bp->rx_watermark);

is this property documented in the bindings?