linux-kernel - Re: [PATCH v2 04/15] net: rnpgbe: Add get_capability mbx

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <91FE839B62404862+20250723031536.GC169181@nic-Precision-5820-Tower>
Date: Wed, 23 Jul 2025 11:15:36 +0800
From: Yibo Dong <dong100@...se.com>
To: Simon Horman <horms@...nel.org>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, corbet@....net,
	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, richardcochran@...il.com,
	netdev@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 04/15] net: rnpgbe: Add get_capability mbx_fw ops
 support

On Tue, Jul 22, 2025 at 02:19:11PM +0100, Simon Horman wrote:
> On Mon, Jul 21, 2025 at 07:32:27PM +0800, Dong Yibo wrote:
> > Initialize get hw capability from mbx_fw ops.
> > 
> > Signed-off-by: Dong Yibo <dong100@...se.com>
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
> 
> ...
> 
> > +struct hw_abilities {
> > +	u8 link_stat;
> > +	u8 lane_mask;
> > +	__le32 speed;
> > +	__le16 phy_type;
> > +	__le16 nic_mode;
> > +	__le16 pfnum;
> > +	__le32 fw_version;
> > +	__le32 axi_mhz;
> > +	union {
> > +		u8 port_id[4];
> > +		__le32 port_ids;
> > +	};
> > +	__le32 bd_uid;
> > +	__le32 phy_id;
> > +	__le32 wol_status;
> > +	union {
> > +		__le32 ext_ability;
> > +		struct {
> > +			__le32 valid : 1; /* 0 */
> > +			__le32 wol_en : 1; /* 1 */
> > +			__le32 pci_preset_runtime_en : 1; /* 2 */
> > +			__le32 smbus_en : 1; /* 3 */
> > +			__le32 ncsi_en : 1; /* 4 */
> > +			__le32 rpu_en : 1; /* 5 */
> > +			__le32 v2 : 1; /* 6 */
> > +			__le32 pxe_en : 1; /* 7 */
> > +			__le32 mctp_en : 1; /* 8 */
> > +			__le32 yt8614 : 1; /* 9 */
> > +			__le32 pci_ext_reset : 1; /* 10 */
> > +			__le32 rpu_availble : 1; /* 11 */
> > +			__le32 fw_lldp_ability : 1; /* 12 */
> > +			__le32 lldp_enabled : 1; /* 13 */
> > +			__le32 only_1g : 1; /* 14 */
> > +			__le32 force_down_en: 1; /* 15 */
> > +		} e;
> 
> I am not sure how __le32 bitfields work on big endian hosts. Do they?
> 
> I would suggest using some combination of BIT/GENMASK,
> FIELD_PREP/FIELT_GET, and le32_from_cpu/cpu_from_le32 instead.
> 
> Flagged by Sparse.
> 

You are right, '__le32 bitfields' is error. I will fix it. 

> > +		struct {
> > +			u32 valid : 1; /* 0 */
> > +			u32 wol_en : 1; /* 1 */
> > +			u32 pci_preset_runtime_en : 1; /* 2 */
> > +			u32 smbus_en : 1; /* 3 */
> > +			u32 ncsi_en : 1; /* 4 */
> > +			u32 rpu_en : 1; /* 5 */
> > +			u32 v2 : 1; /* 6 */
> > +			u32 pxe_en : 1; /* 7 */
> > +			u32 mctp_en : 1; /* 8 */
> > +			u32 yt8614 : 1; /* 9 */
> > +			u32 pci_ext_reset : 1; /* 10 */
> > +			u32 rpu_availble : 1; /* 11 */
> > +			u32 fw_lldp_ability : 1; /* 12 */
> > +			u32 lldp_enabled : 1; /* 13 */
> > +			u32 only_1g : 1; /* 14 */
> > +			u32 force_down_en: 1; /* 15 */
> > +		} e_host;
> > +	};
> > +} __packed;
> 
> ...
> 
> > +/* req is little endian. bigendian should be conserened */
> > +struct mbx_fw_cmd_req {
> 
> ...
> 
> > +		struct {
> > +			__le32 lane;
> > +			__le32 op;
> > +			__le32 enable;
> > +			__le32 inteval;
> 
> interval
> 
> Flagged by checkpatch.pl --codespell
> 
> ...
> 

Got it, I will also use checkpatch.pl to check other patches.

> > +/* firmware -> driver */
> > +struct mbx_fw_cmd_reply {
> > +	/* fw must set: DD, CMP, Error(if error), copy value */
> > +	__le16 flags;
> > +	/* from command: LB,RD,VFC,BUF,SI,EI,FE */
> > +	__le16 opcode; /* 2-3: copy from req */
> > +	__le16 error_code; /* 4-5: 0 if no error */
> > +	__le16 datalen; /* 6-7: */
> > +	union {
> > +		struct {
> > +			__le32 cookie_lo; /* 8-11: */
> > +			__le32 cookie_hi; /* 12-15: */
> > +		};
> > +		void *cookie;
> > +	};
> > +	/* ===== data ==== [16-64] */
> > +	union {
> > +		u8 data[40];
> > +
> > +		struct version {
> > +			__le32 major;
> > +			__le32 sub;
> > +			__le32 modify;
> > +		} version;
> > +
> > +		struct {
> > +			__le32 value[4];
> > +		} r_reg;
> > +
> > +		struct {
> > +			__le32 new_value;
> > +		} modify_reg;
> > +
> > +		struct get_temp {
> > +			__le32 temp;
> > +			__le32 volatage;
> 
> voltage
> 

Got it, I will fix this.

> > +		} get_temp;
> > +
> > +		struct {
> > +#define MBX_SFP_READ_MAX_CNT 32
> > +			u8 value[MBX_SFP_READ_MAX_CNT];
> > +		} sfp_read;
> > +
> > +		struct mac_addr {
> > +			__le32 lanes;
> > +			struct _addr {
> > +				/*
> > +				 * for macaddr:01:02:03:04:05:06
> > +				 * mac-hi=0x01020304 mac-lo=0x05060000
> > +				 */
> > +				u8 mac[8];
> > +			} addrs[4];
> > +		} mac_addr;
> > +
> > +		struct get_dump_reply {
> > +			__le32 flags;
> > +			__le32 version;
> > +			__le32 bytes;
> > +			__le32 data[4];
> > +		} get_dump;
> > +
> > +		struct get_lldp_reply {
> > +			__le32 value;
> > +			__le32 inteval;
> 
> interval
> 

Got it, I will fix this.

> > +		} get_lldp;
> > +
> > +		struct rnpgbe_eee_cap phy_eee_abilities;
> > +		struct lane_stat_data lanestat;
> > +		struct hw_abilities hw_abilities;
> > +		struct phy_statistics phy_statistics;
> > +	};
> > +} __packed;
> 
> ...
> 

Thanks for your feedback.