On Tue, Aug 26, 2025 at 11:14:19AM +0100, Vadim Fedorenko wrote:
> 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
>
Ok, I will remove it in this series and add later.
> >
> > > > +
> > > > + 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?
>
memory is allocated by dma_alloc_coherent, and get physical address.
Then fw use it.
> >
> > > > + 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.
>
Yes. It is not safe, so I 'must wait_event_timeout before free cookie'....
But is there a safe way to do it?
Maybe:
->allocate cookie
-> map it to an unique id
->set the id to req->cookie
->receive response and check id valid? Then access cookie?
Please give me some advice...
> >
> > > > + 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;
> > >
>
>
