Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <65427400.5d0a0220.41c58.0ded@mx.google.com>
Date:   Wed, 1 Nov 2023 16:51:26 +0100
From:   Christian Marangi <ansuelsmth@gmail.com>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Robert Marko <robimarko@gmail.com>
Subject: Re: [net-next PATCH v2 1/2] net: phy: aquantia: add firmware load
 support
References: <20231101123608.11157-1-ansuelsmth@gmail.com>
 <4b536ad3-2112-4f28-90e4-586b5745be20@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4b536ad3-2112-4f28-90e4-586b5745be20@lunn.ch>
Precedence: bulk

On Wed, Nov 01, 2023 at 02:13:05PM +0100, Andrew Lunn wrote:
> On Wed, Nov 01, 2023 at 01:36:07PM +0100, Christian Marangi wrote:
> > From: Robert Marko <robimarko@gmail.com>
> > 
> > Aquantia PHY-s require firmware to be loaded before they start operating.
> > It can be automatically loaded in case when there is a SPI-NOR connected
> > to Aquantia PHY-s or can be loaded from the host via MDIO.
> > 
> > This patch adds support for loading the firmware via MDIO as in most cases
> > there is no SPI-NOR being used to save on cost.
> > Firmware loading code itself is ported from mainline U-boot with cleanups.
> > 
> > The firmware has mixed values both in big and little endian.
> > PHY core itself is big-endian but it expects values to be in little-endian.
> > The firmware is little-endian but CRC-16 value for it is stored at the end
> > of firmware in big-endian.
> > 
> > It seems the PHY does the conversion internally from firmware that is
> > little-endian to the PHY that is big-endian on using the mailbox
> > but mailbox returns a big-endian CRC-16 to verify the written data
> > integrity.
> > 
> > Co-developed-by: Christian Marangi <ansuelsmth@gmail.com>
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> > Changes v2:
> > - Move out of RFC
> 
> Actually, since we are in the merge window, RFC would be correct.
>

My bad!

> > - Address sanity check for offsets
> > - Add additional comments on firmware load check
> > - Fix some typo
> > - Capitalize CRC in comments
> > - Rename load_sysfs to load_fs
> > 
> >  drivers/net/phy/Kconfig         |   1 +
> >  drivers/net/phy/aquantia_main.c | 304 ++++++++++++++++++++++++++++++++
> >  2 files changed, 305 insertions(+)
> > 
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 421d2b62918f..46c7194efcea 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -98,6 +98,7 @@ config ADIN1100_PHY
> >  
> >  config AQUANTIA_PHY
> >  	tristate "Aquantia PHYs"
> > +	select CRC_CCITT
> >  	help
> >  	  Currently supports the Aquantia AQ1202, AQ2104, AQR105, AQR405
> >  
> > diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
> > index 334a6904ca5a..0f1b8d75cca0 100644
> > --- a/drivers/net/phy/aquantia_main.c
> > +++ b/drivers/net/phy/aquantia_main.c
> > @@ -12,6 +12,10 @@
> >  #include <linux/delay.h>
> >  #include <linux/bitfield.h>
> >  #include <linux/phy.h>
> > +#include <linux/of.h>
> > +#include <linux/firmware.h>
> > +#include <linux/crc-ccitt.h>
> > +#include <linux/nvmem-consumer.h>
> >  
> >  #include "aquantia.h"
> >  
> > @@ -92,10 +96,40 @@
> >  #define MDIO_C22EXT_STAT_SGMII_TX_RUNT_FRAMES		0xd31b
> >  
> >  /* Vendor specific 1, MDIO_MMD_VEND1 */
> > +#define VEND1_GLOBAL_SC				0x0
> > +#define VEND1_GLOBAL_SC_SOFT_RESET		BIT(15)
> > +#define VEND1_GLOBAL_SC_LOW_POWER		BIT(11)
> > +
> >  #define VEND1_GLOBAL_FW_ID			0x0020
> >  #define VEND1_GLOBAL_FW_ID_MAJOR		GENMASK(15, 8)
> >  #define VEND1_GLOBAL_FW_ID_MINOR		GENMASK(7, 0)
> >  
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1			0x0200
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_EXECUTE		BIT(15)
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE		BIT(14)
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET	BIT(12)
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_BUSY		BIT(8)
> > +
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE2			0x0201
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE3			0x0202
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR_MASK	GENMASK(15, 0)
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR(x)	FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR_MASK, (u16)((x) >> 16))
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE4			0x0203
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK	GENMASK(15, 2)
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(x)	FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK, (u16)(x))
> > +
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE5			0x0204
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA_MASK	GENMASK(15, 0)
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(x)	FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA_MASK, (u16)((x) >> 16))
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE6			0x0205
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA_MASK	GENMASK(15, 0)
> > +#define VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA(x)	FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA_MASK, (u16)(x))
> > +
> > +#define VEND1_GLOBAL_CONTROL2			0xc001
> > +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_RST	BIT(15)
> > +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD	BIT(6)
> > +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL	BIT(0)
> > +
> >  #define VEND1_GLOBAL_GEN_STAT2			0xc831
> >  #define VEND1_GLOBAL_GEN_STAT2_OP_IN_PROG	BIT(15)
> >  
> > @@ -152,6 +186,30 @@
> >  #define AQR107_OP_IN_PROG_SLEEP		1000
> >  #define AQR107_OP_IN_PROG_TIMEOUT	100000
> >  
> > +#define UP_RESET_SLEEP		100
> > +
> > +/* addresses of memory segments in the phy */
> > +#define DRAM_BASE_ADDR		0x3FFE0000
> > +#define IRAM_BASE_ADDR		0x40000000
> > +
> > +/* firmware image format constants */
> > +#define VERSION_STRING_SIZE		0x40
> > +#define VERSION_STRING_OFFSET		0x0200
> > +/* primary offset is written at an offset from the start of the fw blob */
> > +#define PRIMARY_OFFSET_OFFSET		0x8
> > +/* primary offset needs to be then added to a base offset */
> > +#define PRIMARY_OFFSET_SHIFT		12
> > +#define PRIMARY_OFFSET(x)		((x) << PRIMARY_OFFSET_SHIFT)
> > +#define HEADER_OFFSET			0x300
> > +
> > +struct aqr_fw_header {
> > +	u32 padding;
> > +	u8 iram_offset[3];
> > +	u8 iram_size[3];
> > +	u8 dram_offset[3];
> > +	u8 dram_size[3];
> > +} __packed;
> > +
> >  struct aqr107_hw_stat {
> >  	const char *name;
> >  	int reg;
> > @@ -677,6 +735,166 @@ static int aqr107_wait_processor_intensive_op(struct phy_device *phydev)
> >  	return 0;
> >  }
> >  
> > +/* load data into the phy's memory */
> > +static int aquantia_load_memory(struct phy_device *phydev, u32 addr,
> > +				const u8 *data, size_t len)
> > +{
> 
> > +	for (pos = 0; pos < len; pos += min(sizeof(u32), len - pos)) {
> > +		u32 word = 0;
> > +
> > +		memcpy(&word, data + pos, min(sizeof(u32), len - pos));
> 
> Rather than do a memcpy, use the get_unaligned_ macros. They might map
> to a memcpy(), but some architectures can do unaligned accesses
> without problems.
> 

I don't think this is doable for this loop, think we would end up in
some funny situation where for the last run we have to copy less than
u32. (get_unaligned would always take u32 of data and that would end up
reading more than requested) Am I wrong?

Aside from this, in the other part of the code I can use the macro and
skip having to convert them.

> > +static int aqr_fw_boot(struct phy_device *phydev, const u8 *data, size_t size)
> > +{
> > +	const struct aqr_fw_header *header;
> > +	u32 iram_offset = 0, iram_size = 0;
> > +	u32 dram_offset = 0, dram_size = 0;
> > +	char version[VERSION_STRING_SIZE];
> > +	u16 calculated_crc, read_crc;
> > +	u32 primary_offset = 0;
> > +	int ret;
> > +
> > +	/* extract saved CRC at the end of the fw */
> > +	memcpy(&read_crc, data + size - 2, sizeof(read_crc));
> 
> Say size == 1. You just had a buffer underrun.
> 
> > +	/* CRC is saved in big-endian as PHY is BE */
> > +	read_crc = be16_to_cpu(read_crc);
> > +	calculated_crc = crc_ccitt_false(0, data, size - 2);
> > +	if (read_crc != calculated_crc) {
> > +		phydev_err(phydev, "bad firmware CRC: file 0x%04x calculated 0x%04x\n",
> > +			   read_crc, calculated_crc);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Get the primary offset to extract DRAM and IRAM sections. */
> > +	memcpy(&primary_offset, data + PRIMARY_OFFSET_OFFSET, sizeof(u16));
> 
> What if PRIMARY_OFFSET_OFFSET + sizeof(u16) is greater than size? A
> buffer overrun.
> 
> Assume the firmware is evil and is trying to hack you. Always test
> everything.
> 
> I would suggest some helpers, something like
> 
> int aqr_fw_get_u16(const u8 *data, size_t size, size_t offset, u16 *value)
> 
> Check that offset + sizeof(u16) is within the firmware, and if not return -EINVAL.
> Otherwise set *value to the u16 from the firmware and return 0.
> 
> This is where Rust would be nice :-)
> 
> 	Andrew
> 
> ---
> pw-bot: cr

-- 
	Ansuel