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:   Tue, 13 Dec 2022 17:35:12 -0800
From:   Jakub Kicinski <kuba@kernel.org>
To:     Pranavi Somisetty <pranavi.somisetty@amd.com>
Cc:     <nicolas.ferre@microchip.com>, <claudiu.beznea@microchip.com>,
        <davem@davemloft.net>, <edumazet@google.com>, <pabeni@redhat.com>,
        <git@amd.com>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <michal.simek@amd.com>,
        <harini.katakam@amd.com>, <radhey.shyam.pandey@amd.com>
Subject: Re: [LINUX RFC PATCH] net: macb: Add support for partial store and
 forward
Message-ID: <20221213173512.7902e7df@kernel.org>
In-Reply-To: <20221213121245.13981-1-pranavi.somisetty@amd.com>
References: <20221213121245.13981-1-pranavi.somisetty@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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?