| 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
| ||
|
Message-ID: <Yw6mFbl8abA1lgma@lunn.ch>
Date: Wed, 31 Aug 2022 02:06:45 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Jiawen Wu <jiawenwu@...stnetic.com>
Cc: netdev@...r.kernel.org, mengyuanlou@...-swift.com
Subject: Re: [PATCH net-next v2 01/16] net: txgbe: Store PCI info
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_dummy.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2015 - 2022 Beijing WangXun Technology Co., Ltd. */
> +
> +#ifndef _TXGBE_DUMMY_H_
> +#define _TXGBE_DUMMY_H_
> +
> +#ifdef TUP
> +#elif defined(__GNUC__)
> + #define TUP(x) x##_unused __always_unused
> +#elif defined(__LCLINT__)
> + #define TUP(x) x /*@...sed@*/
> +#else
> + #define TUP(x) x
> +#endif /*TUP*/
> +#define TUP0 TUP(p0)
> +#define TUP1 TUP(p1)
> +#define TUP2 TUP(p2)
> +#define TUP3 TUP(p3)
> +#define TUP4 TUP(p4)
Please avoid macro magic, unless really needed.
> +
> +/* struct txgbe_mac_operations */
> +static void txgbe_bus_set_lan_id_dummy(struct txgbe_hw *TUP0)
> +{
> +}
Seems pointless, so just remove this function, and then you don't need
the macro magic.
> +
> +static void txgbe_init_ops_dummy(struct txgbe_hw *hw)
> +{
> + struct txgbe_mac_info *mac = &hw->mac;
> +
> + /* MAC */
> + mac->ops.set_lan_id = txgbe_bus_set_lan_id_dummy;
Just set it to NULL. A user can first check if its NULL, and not call
it, or return -EOPNOTSUPP etc.
> +/* cmd_addr is used for some special command:
> + * 1. to be sector address, when implemented erase sector command
> + * 2. to be flash address when implemented read, write flash address
> + */
> +int txgbe_fmgr_cmd_op(struct txgbe_hw *hw, u32 cmd, u32 cmd_addr)
> +{
> + u32 cmd_val = 0, val = 0;
> +
> + cmd_val = (cmd << SPI_CLK_CMD_OFFSET) |
> + (SPI_CLK_DIV << SPI_CLK_DIV_OFFSET) | cmd_addr;
> + wr32(hw, SPI_H_CMD_REG_ADDR, cmd_val);
> +
> + return read_poll_timeout(rd32, val, (val & 0x1), 10, SPI_TIME_OUT_VALUE,
> + false, hw, SPI_H_STA_REG_ADDR);
> +}
> +
> +int txgbe_flash_read_dword(struct txgbe_hw *hw, u32 addr, u32 *data)
> +{
> + int ret = 0;
> +
> + ret = txgbe_fmgr_cmd_op(hw, SPI_CMD_READ_DWORD, addr);
> + if (ret == -ETIMEDOUT)
> + return ret;
Are you absolutely sure it will never return any other error code?
The pattern in the kernel is
> + if (ret)
> + return ret;
or
> + if (ret < 0)
> + return ret;
> +int txgbe_check_flash_load(struct txgbe_hw *hw, u32 check_bit)
> +{
> + u32 i = 0, reg = 0;
> + int err = 0;
> +
> + /* if there's flash existing */
> + if (!(rd32(hw, TXGBE_SPI_STATUS) &
> + TXGBE_SPI_STATUS_FLASH_BYPASS)) {
> + /* wait hw load flash done */
> + for (i = 0; i < TXGBE_MAX_FLASH_LOAD_POLL_TIME; i++) {
> + reg = rd32(hw, TXGBE_SPI_ILDR_STATUS);
> + if (!(reg & check_bit)) {
> + /* done */
> + break;
> + }
> + msleep(200);
> + }
This is what iopoll.h is for. Any sort of loop waiting for something
to happen should use one of the helpers in there.
> + if (i == TXGBE_MAX_FLASH_LOAD_POLL_TIME) {
> + err = -ETIMEDOUT;
> + txgbe_info(hw, "Check flash load timeout.\n");
Either dev_err() if this is fatal, or dev_dbg() if it is
not. dev_info() is not used very often, maybe just in your probe
function when you have found your hardware.
> +static int txgbe_sw_init(struct txgbe_adapter *adapter)
> +{
> + struct pci_dev *pdev = adapter->pdev;
> + struct txgbe_hw *hw = &adapter->hw;
> + u32 ssid = 0;
> + int err = 0;
> +
> + /* PCI config space info */
> + hw->vendor_id = pdev->vendor;
> + hw->device_id = pdev->device;
> + hw->revision_id = pdev->revision;
> + hw->oem_svid = pdev->subsystem_vendor;
> + hw->oem_ssid = pdev->subsystem_device;
> +
> + if (hw->oem_svid == PCI_VENDOR_ID_WANGXUN) {
> + hw->subsystem_vendor_id = pdev->subsystem_vendor;
> + hw->subsystem_device_id = pdev->subsystem_device;
> + } else {
> + err = txgbe_flash_read_dword(hw, 0xfffdc, &ssid);
> + if (err == -ETIMEDOUT) {
> + netif_err(adapter, probe, adapter->netdev,
> + "read of internal subsystem device id failed\n");
> + return -ENODEV;
Don't turn an -ETIMEDOUT into an -ENODEV. In general, never convert
from one error code to another, unless you have a really good reason,
and if you do, comment why. -ETIMEDOUT is useful information, it helps
narrow down the problem.
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> index b2e329f50bae..3b1dd104373b 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> @@ -4,7 +4,9 @@
> #ifndef _TXGBE_TYPE_H_
> #define _TXGBE_TYPE_H_
>
> +#include <linux/pci.h>
> #include <linux/types.h>
> +#include <linux/iopoll.h>
> #include <linux/netdevice.h>
These should be in the .c file. Avoid them in header files, since you
will include more headers than actually needed, slowing down the
build. A lot of time is spent in the build processing useless
headers. Please don't make it worse.
>
> /************ txgbe_register.h ************/
> @@ -54,4 +56,153 @@
> /* Revision ID */
> #define TXGBE_SP_MPW 1
>
> +/**************** Global Registers ****************************/
> +/* chip control Registers */
> +#define TXGBE_MIS_RST 0x1000C
> +#define TXGBE_MIS_PWR 0x10000
> +#define TXGBE_MIS_CTL 0x10004
> +#define TXGBE_MIS_PF_SM 0x10008
> +#define TXGBE_MIS_PRB_CTL 0x10010
> +#define TXGBE_MIS_ST 0x10028
> +#define TXGBE_MIS_SWSM 0x1002C
> +#define TXGBE_MIS_RST_ST 0x10030
> +
> +#define TXGBE_MIS_RST_SW_RST 0x00000001U
> +#define TXGBE_MIS_RST_LAN0_RST 0x00000002U
> +#define TXGBE_MIS_RST_LAN1_RST 0x00000004U
> +#define TXGBE_MIS_RST_LAN0_CHG_ETH_MODE 0x20000000U
> +#define TXGBE_MIS_RST_LAN1_CHG_ETH_MODE 0x40000000U
> +#define TXGBE_MIS_RST_GLOBAL_RST 0x80000000U
Generally that would be BIT(0), BIT(1), BIT(2), BIT(29), BIT(30),
BIT(31). I find this easier to read, especially when it is bit(21),
somewhere in the middle.
Please consider changing all the #defines that are for bits.
Andrew
Powered by blists - more mailing lists