On 26/08/2025 02:31, Yibo Dong wrote:
On Mon, Aug 25, 2025 at 05:37:27PM +0100, Vadim Fedorenko wrote:
On 22/08/2025 03:34, Dong Yibo wrote:
[...]
+/**
+ * mucse_mbx_fw_post_req - Posts a mbx req to firmware and wait reply
+ * @hw: pointer to the HW structure
+ * @req: pointer to the cmd req structure
+ * @cookie: pointer to the req cookie
+ *
+ * mucse_mbx_fw_post_req posts a mbx req to firmware and wait for the
+ * reply. cookie->wait will be set in irq handler.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+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);
it's unclear to me, what part of the code is managing values of cookie
structure? I didn't get the reason why are you putting the address of
cookie structure into request which is then directly passed to the FW.
Is the FW supposed to change values in cookie?
cookie will be used in an irq-handler. like this:
static int rnpgbe_mbx_fw_reply_handler(struct mucse *mucse,
struct mbx_fw_cmd_reply *reply)
{
struct mbx_req_cookie *cookie;
cookie = reply->cookie;
if (cookie->priv_len > 0)
memcpy(cookie->priv, reply->data, cookie->priv_len);
cookie->done = 1;
if (le16_to_cpu(reply->flags) & FLAGS_ERR)
cookie->errcode = -EIO;
else
cookie->errcode = 0;
wake_up(&cookie->wait);
return 0;
}
That is why we must wait for firmware response.
But irq is not added in this patch series. Maybe I should move all
cookie relative codes to the patch will add irq?
well, yes, in general it's better to introduce the code as a solid
solution. this way it's much easier to review
+
+ if (!err)
+ err = -ETIMEDOUT;
+ else
+ err = 0;
+ if (!err && cookie->errcode)
+ err = cookie->errcode;
+out:
+ mutex_unlock(&hw->mbx.lock);
+ return err;
+}
[...]
+struct mbx_fw_cmd_req {
+ __le16 flags;
+ __le16 opcode;
+ __le16 datalen;
+ __le16 ret_value;
+ union {
+ struct {
+ __le32 cookie_lo;
+ __le32 cookie_hi;
+ };
+
+ void *cookie;
+ };
+ __le32 reply_lo;
+ __le32 reply_hi;
what do these 2 fields mean? are you going to provide reply's buffer
address directly to FW?
No, this is defined by fw. Some fw can access physical address.
But I don't use it in this driver.
FW can access physical address without previously configuring IOMMU?
How can that be?
+ union {
+ u8 data[32];
+ struct {
+ __le32 version;
+ __le32 status;
+ } ifinsmod;
+ struct {
+ __le32 port_mask;
+ __le32 pfvf_num;
+ } get_mac_addr;
+ };
+} __packed;
+
+struct mbx_fw_cmd_reply {
+ __le16 flags;
+ __le16 opcode;
+ __le16 error_code;
+ __le16 datalen;
+ union {
+ struct {
+ __le32 cookie_lo;
+ __le32 cookie_hi;
+ };
+ void *cookie;
+ };
This part looks like the request, apart from datalen and error_code are
swapped in the header. And it actually means that the FW will put back
the address of provided cookie into reply, right? If yes, then it
doesn't look correct at all...
It is yes. cookie is used in irq handler as show above.
Sorry, I didn't understand 'the not correct' point?
The example above showed that the irq handler uses some value received
from the device as a pointer to kernel memory. That's not safe, you
cannot be sure that provided value is valid pointer, and that it points
to previously allocated cookie structure. It is a clear way to corrupt
memory.
+ union {
+ u8 data[40];
+ struct mac_addr {
+ __le32 ports;
+ struct _addr {
+ /* for macaddr:01:02:03:04:05:06
+ * mac-hi=0x01020304 mac-lo=0x05060000
+ */
+ u8 mac[8];
+ } addrs[4];
+ } mac_addr;
+ struct hw_abilities hw_abilities;
+ };
+} __packed;
