netdev - Re: [PATCH net-next v2 2/3] net: ethernet: ti: Register the RPMsg driver as network device

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: Sun, 2 Jun 2024 17:54:08 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Siddharth Vadapalli <s-vadapalli@...com>
Cc: Yojana Mallik <y-mallik@...com>, schnelle@...ux.ibm.com,
	wsa+renesas@...g-engineering.com, diogo.ivo@...mens.com,
	rdunlap@...radead.org, horms@...nel.org, vigneshr@...com,
	rogerq@...com, danishanwar@...com, pabeni@...hat.com,
	kuba@...nel.org, edumazet@...gle.com, davem@...emloft.net,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, srk@...com,
	rogerq@...nel.org
Subject: Re: [PATCH net-next v2 2/3] net: ethernet: ti: Register the RPMsg
 driver as network device

> > +#define ICVE_MIN_PACKET_SIZE ETH_ZLEN
> > +#define ICVE_MAX_PACKET_SIZE 1540 //(ETH_FRAME_LEN + ETH_FCS_LEN)
> 
> Is the commented portion above required?

I would actually say the comment part is better, since it gives an
idea where the number comes from. However, 1514 + 4 != 1540. So there
is something missing here.

> >  struct icve_port {
> > +	struct icve_shared_mem *tx_buffer; /* Write buffer for data to be consumed remote side */
> > +	struct icve_shared_mem *rx_buffer; /* Read buffer for data to be consumed by this driver */
> > +	struct timer_list rx_timer;
> >  	struct icve_common *common;
> > -} __packed;
> 
> Is the "__packed" attribute no longer required, or was it overlooked?

Why is packed even needed? This is not a message structure to be
passed over the RPC is it?

I think this is the second time code has been added in one patch, and
then removed in the next. That is bad practice and suggests the
overall code quality is not good. Please do some internal reviews.

	Andrew