netdev - Re: [PATCH net-next v2 1/2] net: txgbe: Add basic support for new AML devices

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: <30f42cba-3fff-4137-9aa8-b210ee205423@lunn.ch>
Date: Mon, 13 Jan 2025 18:13:50 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: Jiawen Wu <jiawenwu@...stnetic.com>, mengyuanlou@...-swift.com,
	andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, linux@...linux.org.uk,
	horms@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 1/2] net: txgbe: Add basic support for new
 AML devices

> > +	u32 dword_len;
> > +	u16 buf_len;
> > +	u8 send_cmd;
> > +	u32 i, bi;
> > +
> > +	/* wait max to 50ms to get lock */
> > +	WARN_ON(in_interrupt());
> 
> the comment does not belong here (@timeout is a param, not a const=50ms)
> the warning would be better left to be triggered by lockdep
> (sleeping in atomic context is reported then)

I actually think it is DEBUG_ATOMIC_SLEEP, which is not part of
lockdep.

Also, i've seen recommendations of not using in_interrupt() because
that is not sufficient. There are cases this is false, and you still
cannot sleep.

might_sleep() is the correct thing to put here.

> 
> > +	while (test_and_set_bit(WX_STATE_SWFW_BUSY, wx->state)) {
> > +		timeout--;
> > +		if (!timeout)
> > +			return -ETIMEDOUT;
> 
> it is rather
> ETIME 62 Timer expired
> not
> ETIMEDOUT 110 Connection timed out

ETIMEDOUT is the preferred error. See for example
include/linux/iopoll.h

Ideally, this code should be implemented using one of the macros in
that file.

> > +		wr32a(wx, WX_SW2FW_MBOX, i, (__force u32)cpu_to_le32(buffer[i]));
> > +		/* write flush */
> > +		rd32a(wx, WX_SW2FW_MBOX, i);

> > +	/* polling reply from FW */
> > +	timeout = 50;
> > +	do {
> > +		timeout--;
> > +		usleep_range(1000, 2000);
> > +
> > +		/* read hdr */
> > +		for (bi = 0; bi < dword_len; bi++)
> > +			buffer[bi] = rd32a(wx, WX_FW2SW_MBOX, bi);
> 
> no need for le32_to_cpu()?
> (if so, reexamine whole patch)

When i see (__force u32) i start to wounder if the design is correct
as well.

> > + *  wx_host_interface_command - Issue command to manageability block
> > + *  @wx: pointer to the HW structure
> > + *  @buffer: contains the command to write and where the return status will
> > + *   be placed
> > + *  @length: length of buffer, must be multiple of 4 bytes
> > + *  @timeout: time in ms to wait for command completion
> > + *  @return_data: read and return data from the buffer (true) or not (false)
> > + *   Needed because FW structures are big endian and decoding of
> 
> In other places you were using cpu_to_le32(), this comment seems to
> contradict that

It would be good the use the __be32 and __le32 annotation. sparse will
then help spot such errors.

	Andrew