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] [day] [month] [year] [list]

Message-ID: <059d01db6656$07f4dcb0$17de9610$@trustnetic.com>
Date: Tue, 14 Jan 2025 15:29:21 +0800
From: Jiawen Wu <jiawenwu@...stnetic.com>
To: "'Przemek Kitszel'" <przemyslaw.kitszel@...el.com>
Cc: <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

> > +	/* index to unique seq id for each mbox message */
> > +	send_hdr->index = wx->swfw_index;
> > +	send_cmd = send_hdr->cmd;
> > +
> > +	dword_len = length >> 2;
> > +	/* write data to SW-FW mbox array */
> > +	for (i = 0; i < dword_len; i++) {
> > +		wr32a(wx, WX_SW2FW_MBOX, i, (__force u32)cpu_to_le32(buffer[i]));
> > +		/* write flush */
> > +		rd32a(wx, WX_SW2FW_MBOX, i);
> 
> do you really need to flush all registers?

Yes, hardware requires this.

> 
> > +	}
> > +
> > +	/* generate interrupt to notify FW */
> > +	wr32m(wx, WX_SW2FW_MBOX_CMD, WX_SW2FW_MBOX_CMD_VLD, 0);
> > +	wr32m(wx, WX_SW2FW_MBOX_CMD, WX_SW2FW_MBOX_CMD_VLD, WX_SW2FW_MBOX_CMD_VLD);
> > +
> > +	dword_len = hdr_size >> 2;
> > +
> > +	/* 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)

Indeed need.

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

FW structures are big endian, and we agreed to transfer in the mailbox registers
according to 32-bit little endian. However, buffers contain 8 bits or 16 bits field
which is composed using CPU byte order. So we use cpu_to_le32() and le32_to_cpu()
in this driver.