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?
> > +
> > + 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.
> > + 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?
> > + 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;
>
