netdev - Re: [PATCH net-next v10 4/7] net: tn40xx: add basic Tx handling

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]

Message-ID: <2f9cf951-f357-402c-9da7-77276a9a6a63@gmx.net>
Date: Sat, 15 Jun 2024 11:40:45 +0200
From: Hans-Frieder Vogt <hfdevel@....net>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: kuba@...nel.org, netdev@...r.kernel.org, andrew@...n.ch,
 horms@...nel.org, jiri@...nulli.us, pabeni@...hat.com,
 linux@...linux.org.uk, naveenm@...vell.com, jdamato@...tly.com
Subject: Re: [PATCH net-next v10 4/7] net: tn40xx: add basic Tx handling

On 15.06.2024 11.02, FUJITA Tomonori wrote:
> On Sat, 15 Jun 2024 09:44:31 +0200
> Hans-Frieder Vogt <hfdevel@....net> wrote:
>
>>> On Tue, 11 Jun 2024 13:52:14 +0900 FUJITA Tomonori wrote:
>>>> +static void tn40_init_txd_sizes(void)
>>>> +{
>>>> +	int i, lwords;
>>>> +
>>>> +	if (tn40_txd_sizes[0].bytes)
>>>> +		return;
>>>> +
>>>> +	/* 7 - is number of lwords in txd with one phys buffer
>>>> +	 * 3 - is number of lwords used for every additional phys buffer
>>>> +	 */
>>>> +	for (i = 0; i < TN40_MAX_PBL; i++) {
>>>> +		lwords = 7 + (i * 3);
>>>> +		if (lwords & 1)
>>>> +			lwords++;	/* pad it with 1 lword */
>>>> +		tn40_txd_sizes[i].qwords = lwords >> 1;
>>>> +		tn40_txd_sizes[i].bytes = lwords << 2;
>>>> +	}
>>>> +}
>>> Since this initializes global data - you should do it in module init.
>>> Due to this you can't rely on module_pci_driver(), you gotta write
>>> the module init / exit functions by hand.
>> I would rather move tn40_txd_sizes into tn40_priv and initialize it at
>> probe-time. Just thinking what will happen if there is more than one
>> tn40 card in a computer. Then a driver-based global struct would lead
>> to
>> all sorts of problems.
> The tn40_txd_sizes array is ready-only and the same values are used
> for all TN40 cards? So no need to move it into the priv?

very good point!
But if this is anyway just a constant translation table, why not
pre-calculating it and making it a real const?
something like:

/* Sizes of tx desc (including padding if needed) as function of the SKB's
  * frag number
  * 7 - is number of lwords in txd with one phys buffer
  * 3 - is number of lwords used for every additional phys buffer
  * for (i = 0; i < TN40_MAX_PBL; i++) {
  *    lwords = 7 + (i * 3);
  *        if (lwords & 1)
  *            lwords++;    pad it with 1 lword
  *        tn40_txd_sizes[i].qwords = lwords >> 1;
  *        tn40_txd_sizes[i].bytes = lwords << 2;
  * }
  */
static struct {
     u16 bytes;
     u16 qwords;        /* qword = 64 bit */
} const tn40_txd_sizes[] = {
     { 0x20, 0x04 },
     { 0x28, 0x05 },
     { 0x38, 0x07 },
     { 0x40, 0x08 },
     { 0x50, 0x0a },
     { 0x58, 0x0b },
     { 0x68, 0x0d },
     { 0x70, 0x0e },
     { 0x80, 0x10 },
     { 0x88, 0x11 },
     { 0x98, 0x13 },
     { 0xa0, 0x14 },
     { 0xb0, 0x16 },
     { 0xb8, 0x17 },
     { 0xc8, 0x19 },
     { 0xd0, 0x1a },
     { 0xe0, 0x1c },
     { 0xe8, 0x1d },
     { 0xf8, 0x1f },
};

#define TN40_MAX_PBL (ARRAY_SIZE(tn40_txd_sizes))