Received-SPF: pass (google.com: domain of linux-kernel+bounces-160553-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 26 Apr 2024 20:13:34 +0200
From: Andrew Lunn <andrew@lunn.ch>
To: Parthiban.Veerasooran@microchip.com
Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
	pabeni@redhat.com, horms@kernel.org, saeedm@nvidia.com,
	anthony.l.nguyen@intel.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, corbet@lwn.net,
	linux-doc@vger.kernel.org, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	devicetree@vger.kernel.org, Horatiu.Vultur@microchip.com,
	ruanjinjie@huawei.com, Steen.Hegelund@microchip.com,
	vladimir.oltean@nxp.com, UNGLinuxDriver@microchip.com,
	Thorsten.Kummermehr@microchip.com, Pier.Beruto@onsemi.com,
	Selvamani.Rajagopal@onsemi.com, Nicolas.Ferre@microchip.com,
	benjamin.bigler@bernformulastudent.ch
Subject: Re: [PATCH net-next v4 09/12] net: ethernet: oa_tc6: implement
 receive path to receive rx ethernet frames
Message-ID: <669541fd-abc5-4461-a92f-af5a09c1e1e6@lunn.ch>
References: <20240418125648.372526-1-Parthiban.Veerasooran@microchip.com>
 <20240418125648.372526-10-Parthiban.Veerasooran@microchip.com>
 <574fec4d-5a23-490a-ba12-c40432ebe4b8@lunn.ch>
 <6e529720-3caa-4dc4-b9be-bc6674806ba5@microchip.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6e529720-3caa-4dc4-b9be-bc6674806ba5@microchip.com>

On Fri, Apr 26, 2024 at 01:45:20PM +0000, Parthiban.Veerasooran@microchip.com wrote:
> Hi Andrew,
> 
> On 24/04/24 5:38 am, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> >> +static int oa_tc6_allocate_rx_skb(struct oa_tc6 *tc6)
> >> +{
> >> +     tc6->rx_skb = netdev_alloc_skb(tc6->netdev, tc6->netdev->mtu + ETH_HLEN +
> >> +                                    ETH_FCS_LEN + NET_IP_ALIGN);
> >> +     if (!tc6->rx_skb) {
> >> +             tc6->netdev->stats.rx_dropped++;
> >> +             return -ENOMEM;
> >> +     }
> >> +     skb_reserve(tc6->rx_skb, NET_IP_ALIGN);
> > 
> > I think you can use netdev_alloc_skb_ip_align() here.
> Ah OK, then do you mean we can rewrite the function 
> oa_tc6_allocate_rx_skb() as below?
> 
> static int oa_tc6_allocate_rx_skb(struct oa_tc6 *tc6)
> {
> 	tc6->rx_skb = netdev_alloc_skb_ip_align(tc6->netdev, tc6->netdev->mtu + 
> ETH_HLEN + ETH_FCS_LEN);
> 	if (tc6->rx_skb)
> 		return 0;
> 
> 	tc6->netdev->stats.rx_dropped++;
> 	return -ENOMEM;
> }

Looks about right. But i did say 'I think', meaning i'm not too sure
about this.

I generally don't review code actually moving packets around. It is
what developers focus on, test heavily, and so is generally O.K. It is
the code around the edges which often needs improvements prompted by
review, ethtool, PHY handling, statistics.

	Andrew