netdev - Re: [PATCH 05/15] net: rnpgbe: Add download firmware for n210 chip

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <37ede55f-613b-481f-a8d9-43ee1414849a@lunn.ch>
Date: Fri, 4 Jul 2025 20:33:14 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Dong Yibo <dong100@...se.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, horms@...nel.org, corbet@....net,
	andrew+netdev@...n.ch, gur.stavi@...wei.com, maddy@...ux.ibm.com,
	mpe@...erman.id.au, danishanwar@...com, lee@...ger.us,
	gongfan1@...wei.com, lorenzo@...nel.org, geert+renesas@...der.be,
	Parthiban.Veerasooran@...rochip.com, lukas.bulwahn@...hat.com,
	alexanderduyck@...com, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 05/15] net: rnpgbe: Add download firmware for n210 chip

>  static int init_firmware_for_n210(struct mucse_hw *hw)
>  {
> -	return 0;
> +	char *filename = "n210_driver_update.bin";
> +	const struct firmware *fw;
> +	struct pci_dev *pdev = hw->pdev;
> +	int rc = 0;
> +	int err = 0;
> +	struct mucse *mucse = (struct mucse *)hw->back;
> +
> +	rc = request_firmware(&fw, filename, &pdev->dev);
> +
> +	if (rc != 0) {
> +		dev_err(&pdev->dev, "requesting firmware file failed\n");
> +		return rc;
> +	}
> +
> +	if (rnpgbe_check_fw_from_flash(hw, fw->data)) {
> +		dev_info(&pdev->dev, "firmware type error\n");

Why dev_info()? If this is an error then you should use dev_err().

> +	dev_info(&pdev->dev, "init firmware successfully.");
> +	dev_info(&pdev->dev, "Please reboot.");

Don't spam the lock with status messages.

Reboot? Humm, maybe this should be devlink flash command.

request_firmware() is normally used for download into SRAM which is
then used immediately. If you need to reboot the machine, devlink is
more appropriate.

> +static inline void mucse_sfc_command(u8 __iomem *hw_addr, u32 cmd)
> +{
> +	iowrite32(cmd, (hw_addr + 0x8));
> +	iowrite32(1, (hw_addr + 0x0));
> +	while (ioread32(hw_addr) != 0)
> +		;


Never do endless loops waiting for hardware. It might never give what
you want, and there is no escape.

> +static int32_t mucse_sfc_flash_wait_idle(u8 __iomem *hw_addr)
> +{
> +	int time = 0;
> +	int ret = HAL_OK;
> +
> +	iowrite32(CMD_CYCLE(8), (hw_addr + 0x10));
> +	iowrite32(RD_DATA_CYCLE(8), (hw_addr + 0x14));
> +
> +	while (1) {
> +		mucse_sfc_command(hw_addr, CMD_READ_STATUS);
> +		if ((ioread32(hw_addr + 0x4) & 0x1) == 0)
> +			break;
> +		time++;
> +		if (time > 1000)
> +			ret = HAL_FAIL;
> +	}

iopoll.h 

> +static int mucse_sfc_flash_erase_sector(u8 __iomem *hw_addr,
> +					u32 address)
> +{
> +	int ret = HAL_OK;
> +
> +	if (address >= RSP_FLASH_HIGH_16M_OFFSET)
> +		return HAL_EINVAL;

Use linux error codes, EINVAL.

> +
> +	if (address % 4096)
> +		return HAL_EINVAL;

EINVAL

> +
> +	mucse_sfc_flash_write_enable(hw_addr);
> +
> +	iowrite32((CMD_CYCLE(8) | ADDR_CYCLE(24)), (hw_addr + 0x10));
> +	iowrite32((RD_DATA_CYCLE(0) | WR_DATA_CYCLE(0)), (hw_addr + 0x14));
> +	iowrite32(SFCADDR(address), (hw_addr + 0xc));
> +	mucse_sfc_command(hw_addr, CMD_SECTOR_ERASE);
> +	if (mucse_sfc_flash_wait_idle(hw_addr)) {
> +		ret = HAL_FAIL;
> +		goto failed;

mucse_sfc_flash_wait_idle() should return -ETIMEDOUT, so return that.

	Andrew