Re: [PATCH net-next v3 2/2] net: macb: Add support for partial store and forward

[Simon Horman]

On Tue, May 30, 2023 at 03:51:38AM -0600, Pranavi Somisetty wrote:
> From: Maulik Jodhani <maulik.jodhani@xxxxxxxxxx>
> 
> When the receive partial store and forward mode is activated, the
> receiver will only begin to forward the packet to the external AHB
> or AXI slave when enough packet data is stored in the packet buffer.
> The amount of packet data required to activate the forwarding process
> is programmable via watermark registers which are located at the same
> address as the partial store and forward enable bits. Adding support to
> read this rx-watermark value from device-tree, to program the watermark
> registers and enable partial store and forwarding.
> 
> Signed-off-by: Maulik Jodhani <maulik.jodhani@xxxxxxxxxx>
> Signed-off-by: Pranavi Somisetty <pranavi.somisetty@xxxxxxx>

...

> @@ -4995,6 +5003,27 @@ static int macb_probe(struct platform_device *pdev)
>  
>  	bp->usrio = macb_config->usrio;
>  
> +	/* By default we set to partial store and forward mode for zynqmp.
> +	 * Disable if not set in devicetree.
> +	 */
> +	if (GEM_BFEXT(PBUF_CUTTHRU, gem_readl(bp, DCFG6))) {
> +		err = of_property_read_u16(bp->pdev->dev.of_node,
> +					   "cdns,rx-watermark",
> +					   &bp->rx_watermark);
> +
> +		if (!err) {
> +			/* Disable partial store and forward in case of error or
> +			 * invalid watermark value
> +			 */
> +			wtrmrk_rst_val = (1 << (GEM_BFEXT(RX_PBUF_ADDR, gem_readl(bp, DCFG2)))) - 1;
> +			if (bp->rx_watermark > wtrmrk_rst_val || !bp->rx_watermark) {
> +				dev_info(&bp->pdev->dev, "Invalid watermark value\n");
> +				bp->rx_watermark = 0;
> +				return -EINVAL;

Hi Pranavi,

This appears to leak resources.
Perhaps:

				err = -EINVAL;
				goto err_out_free_netdev;

> +			}
> +			bp->rx_watermark &= wtrmrk_rst_val;
> +		}
> +	}
>  	spin_lock_init(&bp->lock);
>  
>  	/* setup capabilities */