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

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 13 Dec 2022 05:12:45 -0700 Pranavi Somisetty wrote:
> From: Maulik Jodhani <maulik.jodhani@xilinx.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?