| 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: <7ABD3AEE5360FED8+20250917083307.GA55664@nic-Precision-5820-Tower>
Date: Wed, 17 Sep 2025 16:33:07 +0800
From: Yibo Dong <dong100@...se.com>
To: MD Danish Anwar <danishanwar@...com>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
corbet@....net, gur.stavi@...wei.com, maddy@...ux.ibm.com,
mpe@...erman.id.au, lee@...ger.us, gongfan1@...wei.com,
lorenzo@...nel.org, geert+renesas@...der.be,
Parthiban.Veerasooran@...rochip.com, lukas.bulwahn@...hat.com,
alexanderduyck@...com, richardcochran@...il.com, kees@...nel.org,
gustavoars@...nel.org, rdunlap@...radead.org,
vadim.fedorenko@...ux.dev, joerg@...so.de, netdev@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH net-next v12 5/5] net: rnpgbe: Add register_netdev
On Wed, Sep 17, 2025 at 12:39:59PM +0530, MD Danish Anwar wrote:
> On 16/09/25 4:59 pm, Dong Yibo wrote:
> > Complete the network device (netdev) registration flow for Mucse Gbe
> > Ethernet chips, including:
> > 1. Hardware state initialization:
> > - Send powerup notification to firmware (via echo_fw_status)
> > - Sync with firmware
> > - Reset hardware
> > 2. MAC address handling:
> > - Retrieve permanent MAC from firmware (via mucse_mbx_get_macaddr)
> > - Fallback to random valid MAC (eth_random_addr) if not valid mac
> > from Fw
> >
> > Signed-off-by: Dong Yibo <dong100@...se.com>
> > ---
> > drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 18 +++
> > .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c | 80 ++++++++++++++
> > drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h | 2 +
> > .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c | 103 ++++++++++++++++++
> > 4 files changed, 203 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> > index 41b580f2168f..4c4b2f13cb4a 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> > @@ -6,6 +6,7 @@
> >
> > #include <linux/types.h>
> > #include <linux/mutex.h>
> > +#include <linux/netdevice.h>
> >
> > enum rnpgbe_boards {
> > board_n500,
> > @@ -34,12 +35,26 @@ struct mucse_mbx_info {
> > u32 fwpf_ctrl_base;
> > };
> >
> > +enum {
> > + mucse_fw_powerup,
> > +};
> > +
>
> This enum has only one value. You should either use a #define or add
> more values to justify having an enum.
>
Yes, it has only one value now, but more values will be added in the
future.
If I add new value here, but this patch not use it, it is
confict with 'add define along with truely use'.
If I use a #define here, when I add new values in the future, I should
redefine it as enum.
So can I keep enum here with a commit like this ?
/* Enum for firmware notification modes,
more modes (e.g., portup, link_report) will be added in future */
+enum {
+ mucse_fw_powerup,
+};
+
> > struct mucse_hw {
> > void __iomem *hw_addr;
> > + struct pci_dev *pdev;
> > + const struct mucse_hw_operations *ops;
> > struct mucse_mbx_info mbx;
> > + int port;
> > + u8 perm_addr[ETH_ALEN];
> > u8 pfvfnum;
> > };
> >
> > +struct mucse_hw_operations {
> > + int (*reset_hw)(struct mucse_hw *hw);
> > + int (*get_perm_mac)(struct mucse_hw *hw);
> > + int (*mbx_send_notify)(struct mucse_hw *hw, bool enable, int mode);
> > +};
> > +
> > struct mucse {
> > struct net_device *netdev;
> > struct pci_dev *pdev;
> > @@ -54,4 +69,7 @@ int rnpgbe_init_hw(struct mucse_hw *hw, int board_type);
> > #define PCI_DEVICE_ID_N500_DUAL_PORT 0x8318
> > #define PCI_DEVICE_ID_N210 0x8208
> > #define PCI_DEVICE_ID_N210L 0x820a
> > +
> > +#define mucse_hw_wr32(hw, reg, val) \
> > + writel((val), (hw)->hw_addr + (reg))
> > #endif /* _RNPGBE_H */
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> > index 86f1c75796b0..667e372387a2 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> > @@ -1,11 +1,88 @@
> > // SPDX-License-Identifier: GPL-2.0
> > /* Copyright(c) 2020 - 2025 Mucse Corporation. */
> >
> > +#include <linux/pci.h>
> > #include <linux/errno.h>
> > +#include <linux/etherdevice.h>
> >
> > #include "rnpgbe.h"
> > #include "rnpgbe_hw.h"
> > #include "rnpgbe_mbx.h"
> > +#include "rnpgbe_mbx_fw.h"
> > +
> > +/**
> > + * rnpgbe_get_permanent_mac - Get permanent mac
> > + * @hw: hw information structure
> > + *
> > + * rnpgbe_get_permanent_mac tries to get mac from hw
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + **/
> > +static int rnpgbe_get_permanent_mac(struct mucse_hw *hw)
> > +{
> > + struct device *dev = &hw->pdev->dev;
> > + u8 *mac_addr = hw->perm_addr;
> > + int err;
> > +
> > + err = mucse_mbx_get_macaddr(hw, hw->pfvfnum, mac_addr, hw->port);
> > + if (err) {
> > + dev_err(dev, "Failed to get MAC from FW %d\n", err);
> > + return err;
> > + }
> > +
> > + if (!is_valid_ether_addr(mac_addr)) {
> > + dev_err(dev, "Failed to get valid MAC from FW\n");
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * rnpgbe_reset - Do a hardware reset
> > + * @hw: hw information structure
> > + *
> > + * rnpgbe_reset calls fw to do a hardware
> > + * reset, and cleans some regs to default.
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + **/
> > +static int rnpgbe_reset(struct mucse_hw *hw)
> > +{
> > + mucse_hw_wr32(hw, RNPGBE_DMA_AXI_EN, 0);
> > + return mucse_mbx_reset_hw(hw);
> > +}
> > +
> > +/**
> > + * rnpgbe_mbx_send_notify - Echo fw status
> > + * @hw: hw information structure
> > + * @enable: true or false status
> > + * @mode: status mode
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + **/
> > +static int rnpgbe_mbx_send_notify(struct mucse_hw *hw,
> > + bool enable,
> > + int mode)
> > +{
> > + int err;
> > +
> > + switch (mode) {
> > + case mucse_fw_powerup:
> > + err = mucse_mbx_powerup(hw, enable);
> > + break;
> > + default:
> > + err = -EINVAL;
> > + }
> > +
>
> Since you only have one mode currently, this switch statement seems
> unnecessary.
>
The same reason with enum....
Can I keep switch here with a commit?
/* Keep switch structure to support more modes in the future */
switch (mode) {
case mucse_fw_powerup:
err = mucse_mbx_powerup(hw, enable);
break;
default:
err = -EINVAL;
}
> > + return err;
> > +}
>
>
> --
> Thanks and Regards,
> Danish
>
>
Thanks for feedback.
Powered by blists - more mailing lists