netdev - Re: [PATCH net-next v3 2/2] 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, 30 May 2023 21:22:47 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Pranavi Somisetty <pranavi.somisetty@....com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, nicolas.ferre@...rochip.com,
	claudiu.beznea@...rochip.com, git@....com, michal.simek@....com,
	harini.katakam@....com, radhey.shyam.pandey@....com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH net-next v3 2/2] net: macb: Add support for partial store
 and forward

On Tue, May 30, 2023 at 03:51:38AM -0600, Pranavi Somisetty wrote:
> From: Maulik Jodhani <maulik.jodhani@...inx.com>
> 
> 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@...inx.com>
> Signed-off-by: Pranavi Somisetty <pranavi.somisetty@....com>

...

> @@ -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 */