@@ -1,11 +1,81 @@// SPDX-License-Identifier: GPL-2.0/* Copyright(c) 2020 - 2025 Mucse Corporation. */+#include<linux/pci.h>#include<linux/string.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-Getpermanentmac+*@hw:hwinformationstructure+*@mac_addr:pointertostoremac+*+*rnpgbe_get_permanent_mactriestogetmacfromhw.+*Ituseeth_random_addriffailed.+*+*@return:0or-EINVAL+**/+staticintrnpgbe_get_permanent_mac(structmucse_hw*hw,+u8*mac_addr)+{+structdevice*dev=&hw->pdev->dev;++if(mucse_fw_get_macaddr(hw,hw->pfvfnum,mac_addr,hw->port)||+!is_valid_ether_addr(mac_addr)){+dev_err(dev,"Failed to get valid MAC from FW\n");+return-EINVAL;+}++return0;+}++/**+*rnpgbe_reset_hw_ops-Doahardwarereset+*@hw:hwinformationstructure+*+*rnpgbe_reset_hw_opscallsfwtodoahardware+*reset,andcleanssomeregstodefault.+*+*@return:0onsuccess,negativeonfailure+**/+staticintrnpgbe_reset_hw_ops(structmucse_hw*hw)+{+structmucse_dma_info*dma=&hw->dma;+interr;++rnpgbe_dma_wr32(dma,RNPGBE_DMA_AXI_EN,0);+err=mucse_mbx_fw_reset_phy(hw);+if(err)+returnerr;+returnrnpgbe_get_permanent_mac(hw,hw->perm_addr);+}++/**+*rnpgbe_driver_status_hw_ops-Echodriverstatustohw+*@hw:hwinformationstructure+*@enable:trueorfalsestatus+*@mode:statusmode+**/+staticvoidrnpgbe_driver_status_hw_ops(structmucse_hw*hw,+boolenable,+intmode)+{+switch(mode){+casemucse_driver_insmod:+mucse_mbx_ifinsmod(hw,enable);+break;+}+}++staticconststructmucse_hw_operationsrnpgbe_hw_ops={+.reset_hw=&rnpgbe_reset_hw_ops,+.driver_status=&rnpgbe_driver_status_hw_ops,+};/***rnpgbe_init_common-Setupcommonattribute
@@ -37,7 +38,24 @@ struct mucse_mac_info {void__iomem*mac_addr;};+structmucse_mbx_stats{+u32msgs_tx;+u32msgs_rx;+u32acks;+u32reqs;+u32rsts;+};+structmucse_mbx_info{+structmucse_mbx_statsstats;+u32timeout;+u32usec_delay;+u16size;+u16fw_req;+u16fw_ack;+/* lock for only one use mbx */+structmutexlock;+boolirq_enabled;/* fw <--> pf mbx */u32fw_pf_shm_base;u32pf2fw_mbox_ctrl;
@@ -0,0 +1,481 @@+// SPDX-License-Identifier: GPL-2.0+/* Copyright(c) 2022 - 2025 Mucse Corporation. */++#include<linux/pci.h>+#include<linux/errno.h>+#include<linux/delay.h>+#include<linux/iopoll.h>++#include"rnpgbe.h"+#include"rnpgbe_mbx.h"+#include"rnpgbe_hw.h"++/**+*mbx_data_rd32-Readsregwithbasembx->fw_pf_shm_base+*@mbx:pointertotheMBXstructure+*@reg:registeroffset+*+*@return:registervalue+**/+staticu32mbx_data_rd32(structmucse_mbx_info*mbx,u32reg)+{+structmucse_hw*hw=container_of(mbx,structmucse_hw,mbx);++returnreadl(hw->hw_addr+mbx->fw_pf_shm_base+reg);+}++/**+*mbx_data_wr32-Writesvaluetoregwithbasembx->fw_pf_shm_base+*@mbx:pointertotheMBXstructure+*@reg:registeroffset+*@value:valuetobewrite+*+**/+staticvoidmbx_data_wr32(structmucse_mbx_info*mbx,u32reg,u32value)+{+structmucse_hw*hw=container_of(mbx,structmucse_hw,mbx);++writel(value,hw->hw_addr+mbx->fw_pf_shm_base+reg);+}++/**+*mbx_ctrl_rd32-Readsregwithbasembx->fw2pf_mbox_vec+*@mbx:pointertotheMBXstructure+*@reg:registeroffset+*+*@return:registervalue+**/+staticu32mbx_ctrl_rd32(structmucse_mbx_info*mbx,u32reg)+{+structmucse_hw*hw=container_of(mbx,structmucse_hw,mbx);++returnreadl(hw->hw_addr+mbx->fw2pf_mbox_vec+reg);+}++/**+*mbx_ctrl_wr32-Writesvaluetoregwithbasembx->fw2pf_mbox_vec+*@mbx:pointertotheMBXstructure+*@reg:registeroffset+*@value:valuetobewrite+*+**/+staticvoidmbx_ctrl_wr32(structmucse_mbx_info*mbx,u32reg,u32value)+{+structmucse_hw*hw=container_of(mbx,structmucse_hw,mbx);++writel(value,hw->hw_addr+mbx->fw2pf_mbox_vec+reg);+}++/**+*mucse_mbx_get_req-Readreqfromreg+*@mbx:pointertothembxstructure+*@reg:registertoread+*+*@return:thereqvalue+**/+staticu16mucse_mbx_get_req(structmucse_mbx_info*mbx,intreg)+{+returnmbx_data_rd32(mbx,reg)&GENMASK(15,0);+}++/**+*mucse_mbx_get_ack-Readackfromreg+*@mbx:pointertotheMBXstructure+*@reg:registertoread+*+*@return:theackvalue+**/+staticu16mucse_mbx_get_ack(structmucse_mbx_info*mbx,intreg)+{+return(mbx_data_rd32(mbx,reg)>>16);+}++/**+*mucse_mbx_inc_pf_req-Increasereq+*@hw:pointertotheHWstructure+*+*mucse_mbx_inc_pf_reqreadpf_reqfromhw,thenwrite+*newvaluebackafterincrease+**/+staticvoidmucse_mbx_inc_pf_req(structmucse_hw*hw)+{+structmucse_mbx_info*mbx=&hw->mbx;+u16req;+u32v;++v=mbx_data_rd32(mbx,MBX_PF2FW_COUNTER);+req=(v&GENMASK(15,0));+req++;+v&=GENMASK(31,16);+v|=req;+mbx_data_wr32(mbx,MBX_PF2FW_COUNTER,v);+hw->mbx.stats.msgs_tx++;+}++/**+*mucse_mbx_inc_pf_ack-Increaseack+*@hw:pointertotheHWstructure+*+*mucse_mbx_inc_pf_ackreadpf_ackfromhw,thenwrite+*newvaluebackafterincrease+**/+staticvoidmucse_mbx_inc_pf_ack(structmucse_hw*hw)+{+structmucse_mbx_info*mbx=&hw->mbx;+u16ack;+u32v;++v=mbx_data_rd32(mbx,MBX_PF2FW_COUNTER);+ack=(v>>16)&GENMASK(15,0);+ack++;+v&=GENMASK(15,0);+v|=(ack<<16);+mbx_data_wr32(mbx,MBX_PF2FW_COUNTER,v);+hw->mbx.stats.msgs_rx++;+}++/**+*mucse_check_for_msg_pf-Checktoseeifthefwhassentmail+*@hw:pointertotheHWstructure+*+*@return:0ifthefwhassettheStatusbitorelse+*-EIO+**/+staticintmucse_check_for_msg_pf(structmucse_hw*hw)+{+structmucse_mbx_info*mbx=&hw->mbx;+u16hw_req_count=0;++hw_req_count=mucse_mbx_get_req(mbx,MBX_FW2PF_COUNTER);+/* chip's register is reset to 0 when rc send reset+*mbxcommand.Thiscauses'hw_req_count!=hw->mbx.fw_req'+*beTRUEbeforefwreallyreply.Drivermustwaitfwreset+*donereplybeforeusingchip,wemustcheckno-zero.+**/+if(hw_req_count!=0&&hw_req_count!=hw->mbx.fw_req){+hw->mbx.stats.reqs++;+return0;+}++return-EIO;+}++/**+*mucse_poll_for_msg-Waitformessagenotification+*@hw:pointertotheHWstructure+*+*@return:0onsuccess,negativeonfailure+**/+staticintmucse_poll_for_msg(structmucse_hw*hw)+{+structmucse_mbx_info*mbx=&hw->mbx;+intcountdown=mbx->timeout;+intval;++returnread_poll_timeout(mucse_check_for_msg_pf,+val,val==0,mbx->usec_delay,+countdown*mbx->usec_delay,+false,hw);+}++/**+*mucse_check_for_ack_pf-ChecktoseeiftheVFhasACKed+*@hw:pointertotheHWstructure+*+*@return:0ifthefwhassettheStatusbitorelse+*-EIO+**/+staticintmucse_check_for_ack_pf(structmucse_hw*hw)+{+structmucse_mbx_info*mbx=&hw->mbx;+u16hw_fw_ack;++hw_fw_ack=mucse_mbx_get_ack(mbx,MBX_FW2PF_COUNTER);+/* chip's register is reset to 0 when rc send reset+*mbxcommand.Thiscauses'hw_fw_ack!=hw->mbx.fw_ack'+*beTRUEbeforefwreallyreply.Drivermustwaitfwreset+*donereplybeforeusingchip,wemustcheckno-zero.+**/+if(hw_fw_ack!=0&&hw_fw_ack!=hw->mbx.fw_ack){+hw->mbx.stats.acks++;+return0;+}++return-EIO;+}++/**+*mucse_poll_for_ack-Waitformessageacknowledgment+*@hw:pointertotheHWstructure+*+*@return:0ifitsuccessfullyreceivedamessageacknowledgment+**/+staticintmucse_poll_for_ack(structmucse_hw*hw)+{+structmucse_mbx_info*mbx=&hw->mbx;+intcountdown=mbx->timeout;+intval;++returnread_poll_timeout(mucse_check_for_ack_pf,+val,val==0,mbx->usec_delay,+countdown*mbx->usec_delay,+false,hw);+}++/**+*mucse_obtain_mbx_lock_pf-Obtainmailboxlock+*@hw:pointertotheHWstructure+*+*Thisfunctionmaybeusedinanirqhandler.+*+*@return:0ifweobtainedthemailboxlock+**/+staticintmucse_obtain_mbx_lock_pf(structmucse_hw*hw)+{+structmucse_mbx_info*mbx=&hw->mbx;+inttry_cnt=5000;+u32reg;++reg=PF2FW_MBOX_CTRL(mbx);+while(try_cnt-->0){+mbx_ctrl_wr32(mbx,reg,MBOX_PF_HOLD);+/* force write back before check */+wmb();+if(mbx_ctrl_rd32(mbx,reg)&MBOX_PF_HOLD)+return0;+udelay(100);+}+return-EIO;+}++/**+*mucse_read_mbx_pf-Readamessagefromthemailbox+*@hw:pointertotheHWstructure+*@msg:themessagebuffer+*@size:lengthofbuffer+*+*Thisfunctioncopiesamessagefromthemailboxbuffertothecaller's+*memorybuffer.Thepresumptionisthatthecallerknowsthattherewas+*amessageduetoafwrequestsonopollingformessageisneeded.+*+*@return:0onsuccess,negativeonfailure+**/+staticintmucse_read_mbx_pf(structmucse_hw*hw,u32*msg,u16size)+{+structmucse_mbx_info*mbx=&hw->mbx;+intsize_inwords=size/4;+u32ctrl_reg;+intret;+inti;++ctrl_reg=PF2FW_MBOX_CTRL(mbx);++ret=mucse_obtain_mbx_lock_pf(hw);+if(ret)+returnret;+for(i=0;i<size_inwords;i++)+msg[i]=mbx_data_rd32(mbx,MBX_FW_PF_SHM_DATA+4*i);+/* Hw need write data_reg at last */+mbx_data_wr32(mbx,MBX_FW_PF_SHM_DATA,0);+hw->mbx.fw_req=mucse_mbx_get_req(mbx,MBX_FW2PF_COUNTER);+mucse_mbx_inc_pf_ack(hw);+mbx_ctrl_wr32(mbx,ctrl_reg,0);++return0;+}++/**+*mucse_read_posted_mbx-Waitformessagenotificationandreceivemessage+*@hw:pointertotheHWstructure+*@msg:themessagebuffer+*@size:lengthofbuffer+*+*@return:0ifitsuccessfullyreceivedamessagenotificationand+*copieditintothereceivebuffer.+**/+intmucse_read_posted_mbx(structmucse_hw*hw,u32*msg,u16size)+{+intret;++ret=mucse_poll_for_msg(hw);+if(ret)+returnret;++returnmucse_read_mbx_pf(hw,msg,size);+}++/**+*mucse_mbx_reset-Resetmbxinfo,syncinfofromregs+*@hw:pointertotheHWstructure+*+*Thisfunctionresetallmbxvariablestodefault.+**/+staticvoidmucse_mbx_reset(structmucse_hw*hw)+{+structmucse_mbx_info*mbx=&hw->mbx;+u32v;++v=mbx_data_rd32(mbx,MBX_FW2PF_COUNTER);+hw->mbx.fw_req=v&GENMASK(15,0);+hw->mbx.fw_ack=(v>>16)&GENMASK(15,0);+mbx_ctrl_wr32(mbx,PF2FW_MBOX_CTRL(mbx),0);+mbx_ctrl_wr32(mbx,FW_PF_MBOX_MASK(mbx),GENMASK(31,16));+}++/**+*mucse_mbx_configure_pf-Configurembxtousenr_vecinterrupt+*@hw:pointertotheHWstructure+*@nr_vec:vectornumberformbx+*@enable:TRUEforenable,FALSEfordisable+*+*Thisfunctionconfigurembxtouseinterruptnr_vec.+**/+voidmucse_mbx_configure_pf(structmucse_hw*hw,intnr_vec,+boolenable)+{+structmucse_mbx_info*mbx=&hw->mbx;+u32v;++if(enable){+v=mbx_data_rd32(mbx,MBX_FW2PF_COUNTER);+hw->mbx.fw_req=v&GENMASK(15,0);+hw->mbx.fw_ack=(v>>16)&GENMASK(15,0);+mbx_ctrl_wr32(mbx,PF2FW_MBOX_CTRL(mbx),0);+mbx_ctrl_wr32(mbx,FW2PF_MBOX_VEC,nr_vec);+mbx_ctrl_wr32(mbx,FW_PF_MBOX_MASK(mbx),GENMASK(31,16));+}else{+mbx_ctrl_wr32(mbx,FW_PF_MBOX_MASK(mbx),0xfffffffe);+mbx_ctrl_wr32(mbx,PF2FW_MBOX_CTRL(mbx),0);+mbx_ctrl_wr32(mbx,RNPGBE_DMA_DUMY,0);+}+}++/**+*mucse_init_mbx_params_pf-Setinitialvaluesforpfmailbox+*@hw:pointertotheHWstructure+*+*Initializesthehw->mbxstructtocorrectvaluesforpfmailbox+*/+voidmucse_init_mbx_params_pf(structmucse_hw*hw)+{+structmucse_mbx_info*mbx=&hw->mbx;++mbx->usec_delay=100;+mbx->timeout=(4*USEC_PER_SEC)/mbx->usec_delay;+mbx->stats.msgs_tx=0;+mbx->stats.msgs_rx=0;+mbx->stats.reqs=0;+mbx->stats.acks=0;+mbx->stats.rsts=0;+mbx->size=MUCSE_MAILBOX_BYTES;+mutex_init(&mbx->lock);+mucse_mbx_reset(hw);+}++/**+*mucse_read_mbx-Readsamessagefromthemailbox+*@hw:pointertotheHWstructure+*@msg:themessagebuffer+*@size:lengthofbuffer+*+*@return:0onsuccess,negativeonfailure+**/+intmucse_read_mbx(structmucse_hw*hw,u32*msg,u16size)+{+structmucse_mbx_info*mbx=&hw->mbx;+structdevice*dev=&hw->pdev->dev;++if(size>mbx->size){+dev_err(dev,"mbx read size too large\n");+return-EINVAL;+}+returnmucse_read_mbx_pf(hw,msg,size);+}++/**+*mucse_write_mbx_pf-Placeamessageinthemailbox+*@hw:pointertotheHWstructure+*@msg:themessagebuffer+*@size:lengthofbuffer+*+*Thisfunctionmaybeusedinanirqhandler.+*+*@return:0ifitsuccessfullycopiedmessageintothebuffer+**/+intmucse_write_mbx_pf(structmucse_hw*hw,u32*msg,u16size)+{+structmucse_mbx_info*mbx=&hw->mbx;+intsize_inwords=size/4;+u32ctrl_reg;+intret;+inti;++ctrl_reg=PF2FW_MBOX_CTRL(mbx);+ret=mucse_obtain_mbx_lock_pf(hw);+if(ret)+returnret;++for(i=0;i<size_inwords;i++)+mbx_data_wr32(mbx,MBX_FW_PF_SHM_DATA+i*4,msg[i]);++/* flush msg and acks as we are overwriting the message buffer */+hw->mbx.fw_ack=mucse_mbx_get_ack(mbx,MBX_FW2PF_COUNTER);+mucse_mbx_inc_pf_req(hw);+mbx_ctrl_wr32(mbx,ctrl_reg,MBOX_CTRL_REQ);++return0;+}++/**+*mucse_write_mbx-Writeamessagetothemailbox+*@hw:pointertotheHWstructure+*@msg:themessagebuffer+*@size:lengthofbuffer+*+*@return:0onsuccess,negativeonfailure+**/+intmucse_write_mbx(structmucse_hw*hw,u32*msg,u16size)+{+returnmucse_write_mbx_pf(hw,msg,size);+}++/**+*mucse_write_posted_mbx-Writeamessagetothemailbox,waitforack+*@hw:pointertotheHWstructure+*@msg:themessagebuffer+*@size:lengthofbuffer+*+*@return:0ifitsuccessfullycopiedmessageintothebufferand+*receivedanacktothatmessagewithindelay*timeoutperiod+**/+intmucse_write_posted_mbx(structmucse_hw*hw,u32*msg,u16size)+{+intret;++ret=mucse_write_mbx_pf(hw,msg,size);+if(ret)+returnret;+returnmucse_poll_for_ack(hw);+}++/**+*mucse_check_for_msg-Checktoseeiffwsentusmail+*@hw:pointertotheHWstructure+*+*@return:0onsuccess,negativeonfailure+**/+intmucse_check_for_msg(structmucse_hw*hw)+{+returnmucse_check_for_msg_pf(hw);+}++/**+*mucse_check_for_ack-ChecktoseeiffwsentusACK+*@hw:pointertotheHWstructure+*+*@return:0onsuccess,negativeonfailure+**/+intmucse_check_for_ack(structmucse_hw*hw)+{+returnmucse_check_for_ack_pf(hw);+}
@@ -0,0 +1,21 @@+.. SPDX-License-Identifier: GPL-2.0++===========================================================+Linux Base Driver for MUCSE(R) Gigabit PCI Express Adapters+===========================================================++MUCSE Gigabit Linux driver.+Copyright (c) 2020 - 2025 MUCSE Co.,Ltd.++Identifying Your Adapter+========================+The driver is compatible with devices based on the following:++* MUCSE(R) Ethernet Controller N500 series+* MUCSE(R) Ethernet Controller N210 series++Support+=======+ If you have problems with the software or hardware, please contact our+ customer support team via email at techsupport@mucse.com or check our+ website at https://www.mucse.com/en/
What is this field for? You don't use it anywhere in the patchset apart
from initialization. Maybe it's better to introduce it once it's used?
Together with the defines of values for this field...
What is this field for? You don't use it anywhere in the patchset apart
from initialization. Maybe it's better to introduce it once it's used?
Together with the defines of values for this field...
It is used to store chip frequency which is used to calculate values
related to 'delay register' in the future. I will improve this.
Thanks for your feedback.
What is this field for? You don't use it anywhere in the patchset apart
from initialization. Maybe it's better to introduce it once it's used?
Together with the defines of values for this field...
It is used to store chip frequency which is used to calculate values
related to 'delay register' in the future. I will improve this.
Maybe also see if you can find a better name. count is rather
vague. Count of what?
Andrew
total_queue_pair_cnts is the same for all three. So it probably does
not need to be in the structure. You can just use RNPGBE_MAX_QUEUES.
Andrew
---
pw-bot: cr
+int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int size_inwords = size / 4;
+ u32 ctrl_reg;
+ int ret;
+ int i;
+
+ ctrl_reg = PF2FW_MBOX_CTRL(mbx);
+ ret = mucse_obtain_mbx_lock_pf(hw);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < size_inwords; i++)
+ mbx_data_wr32(mbx, MBX_FW_PF_SHM_DATA + i * 4, msg[i]);
+
+ /* flush msg and acks as we are overwriting the message buffer */
+ hw->mbx.fw_ack = mucse_mbx_get_ack(mbx, MBX_FW2PF_COUNTER);
It seems like the ACK is always at MBX_FW2PF_COUNTER. So why pass it
to mucse_mbx_get_ack()? Please look at your other getters and setters.
+/**
+ * mucse_write_mbx - Write a message to the mailbox
+ * @hw: pointer to the HW structure
+ * @msg: the message buffer
+ * @size: length of buffer
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_write_mbx(struct mucse_hw *hw, u32 *msg, u16 size)
+{
+ return mucse_write_mbx_pf(hw, msg, size);
+}
This function does not do anything useful. Why not call
mucse_write_mbx_pf() directly?
+/**
+ * mucse_check_for_msg - Check to see if fw sent us mail
+ * @hw: pointer to the HW structure
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_check_for_msg(struct mucse_hw *hw)
+{
+ return mucse_check_for_msg_pf(hw);
+}
+
+/**
+ * mucse_check_for_ack - Check to see if fw sent us ACK
+ * @hw: pointer to the HW structure
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_check_for_ack(struct mucse_hw *hw)
+{
+ return mucse_check_for_ack_pf(hw);
+}
If you can get it from the hardware, why do you need to initialise it
in the earlier patch?
I guess you have a bootstrap problem, you need it to get it. But
cannot you just initialise it to a single pessimistic value which will
work well enough for all hardware variants until you can actually ask
the hardware?
Andrew
What is your design with respect to mutex_lock_interruptible() and
then calling wait_event_timeout() which will ignore signals?
Is your intention that you can always ^C the driver, and it will clean
up whatever it was doing and return -EINTR? Such unwinding can be
tricky and needs careful review. Before i do that, i just want to make
sure this is your intention, and you yourself have carefully reviewed
the code.
Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: 2025-08-20 20:43:17
+static int rnpgbe_get_permanent_mac(struct mucse_hw *hw,
+ u8 *mac_addr)
+{
+ struct device *dev = &hw->pdev->dev;
+
+ if (mucse_fw_get_macaddr(hw, hw->pfvfnum, mac_addr, hw->port) ||
+ !is_valid_ether_addr(mac_addr)) {
+ dev_err(dev, "Failed to get valid MAC from FW\n");
+ return -EINVAL;
I _think_ mucse_fw_get_macaddr() can return -EINTR, because deep down,
it has a call to mutex_lock_interruptible(). If that happens, you
should not return iEINVAL, it is not an invalid value, its just an
interrupted system call.
This is what i'm talking about needing careful review...
Andrew
---
pw-bot: cr
On Wed, Aug 20, 2025 at 10:06:00PM +0200, Andrew Lunn wrote:
quoted
+/**
+ * rnpgbe_init_module - Driver init routine
+ *
+ * rnpgbe_init_module is called when driver insmod
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int __init rnpgbe_init_module(void)
+{
+ return pci_register_driver(&rnpgbe_driver);
+}
+
+module_init(rnpgbe_init_module);
+
+/**
+ * rnpgbe_exit_module - Driver remove routine
+ *
+ * rnpgbe_exit_module is called when driver is removed
+ **/
+static void __exit rnpgbe_exit_module(void)
+{
+ pci_unregister_driver(&rnpgbe_driver);
+}
+
+module_exit(rnpgbe_exit_module);
This can be replaced by module_pci_driver()
Ok, I will improve it.
By the way, if I want to add some functions(maybe workqueue to handle
link status, or debugfs for the driver) in the future. The function is
only one for driver, not each for every pci device, should I turn back to
'module_exit' and 'module_init'?
Maybe workqueue can use 'system_power_efficient_wq' just like libwx
does?
+int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int size_inwords = size / 4;
+ u32 ctrl_reg;
+ int ret;
+ int i;
+
+ ctrl_reg = PF2FW_MBOX_CTRL(mbx);
+ ret = mucse_obtain_mbx_lock_pf(hw);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < size_inwords; i++)
+ mbx_data_wr32(mbx, MBX_FW_PF_SHM_DATA + i * 4, msg[i]);
+
+ /* flush msg and acks as we are overwriting the message buffer */
+ hw->mbx.fw_ack = mucse_mbx_get_ack(mbx, MBX_FW2PF_COUNTER);
It seems like the ACK is always at MBX_FW2PF_COUNTER. So why pass it
to mucse_mbx_get_ack()? Please look at your other getters and setters.
'mucse_mbx_get_ack' is always at MBX_FW2PF_COUNTER now, just for pf-fw mbx.
But, in the future, there will be pf-vf mbx with different input.
Should I move 'MBX_FW2PF_COUNTER' to function 'mucse_mbx_get_ack', and
update the function when I add vf relative code in the future?
quoted
+/**
+ * mucse_write_mbx - Write a message to the mailbox
+ * @hw: pointer to the HW structure
+ * @msg: the message buffer
+ * @size: length of buffer
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_write_mbx(struct mucse_hw *hw, u32 *msg, u16 size)
+{
+ return mucse_write_mbx_pf(hw, msg, size);
+}
This function does not do anything useful. Why not call
mucse_write_mbx_pf() directly?
Yes, I should call it directly.
quoted
+/**
+ * mucse_check_for_msg - Check to see if fw sent us mail
+ * @hw: pointer to the HW structure
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_check_for_msg(struct mucse_hw *hw)
+{
+ return mucse_check_for_msg_pf(hw);
+}
+
+/**
+ * mucse_check_for_ack - Check to see if fw sent us ACK
+ * @hw: pointer to the HW structure
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_check_for_ack(struct mucse_hw *hw)
+{
+ return mucse_check_for_ack_pf(hw);
+}
What is this field for? You don't use it anywhere in the patchset apart
from initialization. Maybe it's better to introduce it once it's used?
Together with the defines of values for this field...
It is used to store chip frequency which is used to calculate values
related to 'delay register' in the future. I will improve this.
Maybe also see if you can find a better name. count is rather
vague. Count of what?
Andrew
Chip use clock cycle not us(microsecond) to achieve timing. If chip is 125MHz,
driver setups a register to 125 to get 1us timing(125 * 10^6 * 10^(-6)).
Maybe usecstocycle better?
Thanks for your feedback.
If you can get it from the hardware, why do you need to initialise it
in the earlier patch?
I guess you have a bootstrap problem, you need it to get it. But
cannot you just initialise it to a single pessimistic value which will
work well enough for all hardware variants until you can actually ask
the hardware?
Andrew
It is a problem related with fw version. Older fw may return with axi_mhz
0, So I init a no-zero default value first. Also, I missed to check the axi_mhz
here. The 'usecstocount' is removed in v6, I will update here like this in
the patch which truely use 'usecstocount':
if (le32_to_cpu(ability.axi_mhz))
hw->usecstocount = le32_to_cpu(ability.axi_mhz);
/* else keep use the default value */
Thanks for your feedback.
What is your design with respect to mutex_lock_interruptible() and
then calling wait_event_timeout() which will ignore signals?
Is your intention that you can always ^C the driver, and it will clean
up whatever it was doing and return -EINTR? Such unwinding can be
tricky and needs careful review. Before i do that, i just want to make
sure this is your intention, and you yourself have carefully reviewed
the code.
Andrew
'mucse_mbx_fw_post_req' is designed can be called by 'cat /sys/xxx', So I used
xx_interruptible() before.
The design sequence is:
write_mbx with cookie ------> fw ----> dirver_irq_handler(call wake_up)
| |
V V
wait_event_xxxx -------------------> free(cookie)
But if ^C just after 'wait_event_interruptible_timeout', cookie will
be free before fw really response, a crash will happen.
cookie pointer is in mbx.req, and fw response with no change.
write_mbx with cookie ------> fw ---------> dirver_irq_handler(call wake_up)
| |
V V
wait_event_xxxx ---> free(cookie) crash with freed cookie
|
v
^C
So I use goto retry if -ERESTARTSYS with wait_event_interruptible_timeout.
And it is the same with wait_event_timeout.
If ^C in mutex_lock_interruptible, it is safe return since no write to
fw and no response from fw.
From: Andrew Lunn <andrew@lunn.ch> Date: 2025-08-21 03:01:29
Ok, I will improve it.
By the way, if I want to add some functions(maybe workqueue to handle
link status, or debugfs for the driver) in the future. The function is
only one for driver, not each for every pci device, should I turn back to
'module_exit' and 'module_init'?
Maybe workqueue can use 'system_power_efficient_wq' just like libwx
does?
Generally, you don't have per driver data, only per device data. It
makes the locking simpler, and handling bind/unbind etc.
Andrew
+int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int size_inwords = size / 4;
+ u32 ctrl_reg;
+ int ret;
+ int i;
+
+ ctrl_reg = PF2FW_MBOX_CTRL(mbx);
+ ret = mucse_obtain_mbx_lock_pf(hw);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < size_inwords; i++)
+ mbx_data_wr32(mbx, MBX_FW_PF_SHM_DATA + i * 4, msg[i]);
+
+ /* flush msg and acks as we are overwriting the message buffer */
+ hw->mbx.fw_ack = mucse_mbx_get_ack(mbx, MBX_FW2PF_COUNTER);
It seems like the ACK is always at MBX_FW2PF_COUNTER. So why pass it
to mucse_mbx_get_ack()? Please look at your other getters and setters.
'mucse_mbx_get_ack' is always at MBX_FW2PF_COUNTER now, just for pf-fw mbx.
But, in the future, there will be pf-vf mbx with different input.
Should I move 'MBX_FW2PF_COUNTER' to function 'mucse_mbx_get_ack', and
update the function when I add vf relative code in the future?
Maybe add mucse_mbx_get_pf_ack() so you can later add
mucse_mbx_get_vf_ack()?
The problem is, our crystal ball about what will come next is not very
good. So we review the code we see now, and make comments about it
now. You can add comments explaining why something is the way it is
because in the future it needs to be more generic to handle additional
use cases, etc. Or explain in the commit message.
Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: 2025-08-21 03:14:01
'mucse_mbx_fw_post_req' is designed can be called by 'cat /sys/xxx',
It is pretty unusual for ethernet drivers to export data in /sys,
except via standard APIs, like statistics, carrier, address, opstate
etc. I don't know how well the core will handle EINTR. It is not
something most drivers do. -ETIMEDOUT is more likely when the firmware
has crashed and does not respond in time.
Do you have any operations which take a long time when things are
working correctly?
Andrew
On Thu, Aug 21, 2025 at 05:13:27AM +0200, Andrew Lunn wrote:
quoted
'mucse_mbx_fw_post_req' is designed can be called by 'cat /sys/xxx',
It is pretty unusual for ethernet drivers to export data in /sys,
except via standard APIs, like statistics, carrier, address, opstate
etc. I don't know how well the core will handle EINTR. It is not
something most drivers do. -ETIMEDOUT is more likely when the firmware
has crashed and does not respond in time.
Do you have any operations which take a long time when things are
working correctly?
Andrew
'Update firmware operation' will take long time, maybe more than
10s. If user use 'ethtool -f' to update firmware, and ^C before done?
If ^C before mucse_write_mbx, return as soon as possible. If after mucse_write_mbx,
wait until fw true response.
On Wed, Aug 20, 2025 at 10:42:47PM +0200, Andrew Lunn wrote:
quoted
+static int rnpgbe_get_permanent_mac(struct mucse_hw *hw,
+ u8 *mac_addr)
+{
+ struct device *dev = &hw->pdev->dev;
+
+ if (mucse_fw_get_macaddr(hw, hw->pfvfnum, mac_addr, hw->port) ||
+ !is_valid_ether_addr(mac_addr)) {
+ dev_err(dev, "Failed to get valid MAC from FW\n");
+ return -EINVAL;
I _think_ mucse_fw_get_macaddr() can return -EINTR, because deep down,
it has a call to mutex_lock_interruptible(). If that happens, you
should not return iEINVAL, it is not an invalid value, its just an
interrupted system call.
This is what i'm talking about needing careful review...
Andrew
---
pw-bot: cr
Ok, Maybe like This?
Just return function return if mucse_fw_get_macaddr failed, and return
-EINVAL if not a valid mac.
static int rnpgbe_get_permanent_mac(struct mucse_hw *hw,
u8 *mac_addr)
{
struct device *dev = &hw->pdev->dev;
int ret;
ret = mucse_fw_get_macaddr(hw, hw->pfvfnum, mac_addr, hw->port);
if (ret) {
dev_err(dev, "Failed to get MAC from FW\n");
return ret;
}
if (!is_valid_ether_addr(mac_addr)) {
dev_err(dev, "MAC from FW is not valid\n");
return -EINVAL;
}
return 0;
}
Thanks for your feedback.
+int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int size_inwords = size / 4;
+ u32 ctrl_reg;
+ int ret;
+ int i;
+
+ ctrl_reg = PF2FW_MBOX_CTRL(mbx);
+ ret = mucse_obtain_mbx_lock_pf(hw);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < size_inwords; i++)
+ mbx_data_wr32(mbx, MBX_FW_PF_SHM_DATA + i * 4, msg[i]);
+
+ /* flush msg and acks as we are overwriting the message buffer */
+ hw->mbx.fw_ack = mucse_mbx_get_ack(mbx, MBX_FW2PF_COUNTER);
It seems like the ACK is always at MBX_FW2PF_COUNTER. So why pass it
to mucse_mbx_get_ack()? Please look at your other getters and setters.
'mucse_mbx_get_ack' is always at MBX_FW2PF_COUNTER now, just for pf-fw mbx.
But, in the future, there will be pf-vf mbx with different input.
Should I move 'MBX_FW2PF_COUNTER' to function 'mucse_mbx_get_ack', and
update the function when I add vf relative code in the future?
Maybe add mucse_mbx_get_pf_ack() so you can later add
mucse_mbx_get_vf_ack()?
The problem is, our crystal ball about what will come next is not very
good. So we review the code we see now, and make comments about it
now. You can add comments explaining why something is the way it is
because in the future it needs to be more generic to handle additional
use cases, etc. Or explain in the commit message.
Andrew
Got it, I will move MBX_FW2PF_COUNTER to mucse_mbx_get_ack and rename
the fucntion. And add new function later.
Thanks for your feedback.
Thanks for your feedback.
From: Andrew Lunn <andrew@lunn.ch> Date: 2025-08-22 19:53:02
'Update firmware operation' will take long time, maybe more than
10s. If user use 'ethtool -f' to update firmware, and ^C before done?
If ^C before mucse_write_mbx, return as soon as possible. If after mucse_write_mbx,
wait until fw true response.
And what happens if the firmware writing is interrupted? Could you end
up with a brick? This is actually one of the operations i would not
expect to be able to ^C.
You might also want consider devlink flash.
https://www.kernel.org/doc/html/latest/networking/devlink/devlink-flash.html
It replaces the older ethtool-flash mechanism, and doesn’t require
taking any networking locks in the kernel to perform the flash
update.
I assume this is meaning ethtool take RTNL, and while that is held, no
other network configuration can be performed on any interface. devlink
has its own lock so avoids this.
Andrew
On Fri, Aug 22, 2025 at 09:52:25PM +0200, Andrew Lunn wrote:
quoted
'Update firmware operation' will take long time, maybe more than
10s. If user use 'ethtool -f' to update firmware, and ^C before done?
If ^C before mucse_write_mbx, return as soon as possible. If after mucse_write_mbx,
wait until fw true response.
And what happens if the firmware writing is interrupted? Could you end
up with a brick? This is actually one of the operations i would not
expect to be able to ^C.
You might also want consider devlink flash.
https://www.kernel.org/doc/html/latest/networking/devlink/devlink-flash.html
It replaces the older ethtool-flash mechanism, and doesn’t require
taking any networking locks in the kernel to perform the flash
update.
I assume this is meaning ethtool take RTNL, and while that is held, no
other network configuration can be performed on any interface. devlink
has its own lock so avoids this.
Andrew
ethtool or devlink both call mbx(mucse_mbx_fw_post_req)
to do the true update to firmware. FW not end up with a brick, it has
fault tolerance itself.
But that's not the point. The original question is
about 'wait_event_timeout', I add some comment link this in v6:
Wait fw response without interruptible.
static int mucse_mbx_fw_post_req(struct mucse_hw *hw,
struct mbx_fw_cmd_req *req,
struct mbx_req_cookie *cookie)
{
int len = le16_to_cpu(req->datalen);
int err;
cookie->errcode = 0;
cookie->done = 0;
init_waitqueue_head(&cookie->wait);
err = mutex_lock_interruptible(&hw->mbx.lock);
if (err)
return err;
err = mucse_write_mbx_pf(hw, (u32 *)req, len);
if (err)
goto out;
/* if write succeeds, we must wait for firmware response or
* timeout to avoid using the already freed cookie->wait
*/
err = wait_event_timeout(cookie->wait,
cookie->done == 1,
cookie->timeout_jiffies);
if (!err)
err = -ETIMEDOUT;
else
err = 0;
if (!err && cookie->errcode)
err = cookie->errcode;
out:
mutex_unlock(&hw->mbx.lock);
return err;
}