[net-next,v1,1/2] octeon_ep: implement device unload control net API
Commit Message
Device unload control net function should inform firmware
of driver unload to let it take necessary actions to cleanup.
Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
.../ethernet/marvell/octeon_ep/octep_ctrl_net.c | 16 +++++++++++++++-
.../ethernet/marvell/octeon_ep/octep_ctrl_net.h | 11 +++++++++++
2 files changed, 26 insertions(+), 1 deletion(-)
Comments
On Mon, 27 Nov 2023 08:21:34 -0800 Shinas Rasheed wrote:
> Device unload control net function should inform firmware
What is "control net" again?
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, November 28, 2023 8:13 AM
> To: Shinas Rasheed <srasheed@marvell.com>
>
> On Mon, 27 Nov 2023 08:21:34 -0800 Shinas Rasheed wrote:
> > Device unload control net function should inform firmware
>
> What is "control net" again?
Control net is just a software layer which is used by the host driver as well as the firmware to communicate with each other, given in the source file octep_ctrl_net.c and the corresponding octep_ctrl_net.h interface, which is already part of upstreamed driver.
On Tue, 28 Nov 2023 04:22:11 +0000 Shinas Rasheed wrote:
> > On Mon, 27 Nov 2023 08:21:34 -0800 Shinas Rasheed wrote:
> > > Device unload control net function should inform firmware
> >
> > What is "control net" again?
>
> Control net is just a software layer which is used by the host driver
> as well as the firmware to communicate with each other, given in the
> source file octep_ctrl_net.c and the corresponding octep_ctrl_net.h
> interface, which is already part of upstreamed driver.
Yes, I think it went in before I had time to nack it.
I'm strongly against using the IP stack to talk to FW,
if you read the ML you would know it.
No new patches to octep_ctrl_net will be accepted.
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, November 28, 2023 9:57 PM
> To: Shinas Rasheed <srasheed@marvell.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Haseeb Gani
> On Tue, 28 Nov 2023 04:22:11 +0000 Shinas Rasheed wrote:
> > > On Mon, 27 Nov 2023 08:21:34 -0800 Shinas Rasheed wrote:
> > > > Device unload control net function should inform firmware
> > >
> > > What is "control net" again?
> >
> > Control net is just a software layer which is used by the host driver
> > as well as the firmware to communicate with each other, given in the
> > source file octep_ctrl_net.c and the corresponding octep_ctrl_net.h
> > interface, which is already part of upstreamed driver.
>
> Yes, I think it went in before I had time to nack it.
> I'm strongly against using the IP stack to talk to FW,
> if you read the ML you would know it.
>
> No new patches to octep_ctrl_net will be accepted.
Control net doesn't use the IP stack at all. It's a simple producer-consumer based ring mechanism using PCIe BAR4 memory. Sorry maybe the nomenclature suggests something of that nature.
On Tue, 28 Nov 2023 19:08:26 +0000 Shinas Rasheed wrote:
> > Yes, I think it went in before I had time to nack it.
> > I'm strongly against using the IP stack to talk to FW,
> > if you read the ML you would know it.
> >
> > No new patches to octep_ctrl_net will be accepted.
>
> Control net doesn't use the IP stack at all. It's a simple
> producer-consumer based ring mechanism using PCIe BAR4 memory.
> Sorry maybe the nomenclature suggests something of that nature.
Ah, got it. I read that as "separate netdev for control", my bad.
Just one nit then:
>+ dev_info(&oct->pdev->dev, "Sending dev_unload msg to fw\n");
Is it really necessary to print this at info level for each remove?
Remove is a normal part of operation and we shouldn't spam logs
unless we have a good reason..
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, November 29, 2023 3:25 AM
> To: Shinas Rasheed <srasheed@marvell.com>
> >+ dev_info(&oct->pdev->dev, "Sending dev_unload msg to fw\n");
>
> Is it really necessary to print this at info level for each remove?
> Remove is a normal part of operation and we shouldn't spam logs
> unless we have a good reason..
I suppose it's not a need other than maybe for debug purposes. I'll use dev_dbg and submit a second version
@@ -26,7 +26,7 @@ static atomic_t ctrl_net_msg_id;
/* Control plane version in which OCTEP_CTRL_NET_H2F_CMD was added */
static const u32 octep_ctrl_net_h2f_cmd_versions[OCTEP_CTRL_NET_H2F_CMD_MAX] = {
- [OCTEP_CTRL_NET_H2F_CMD_INVALID ... OCTEP_CTRL_NET_H2F_CMD_GET_INFO] =
+ [OCTEP_CTRL_NET_H2F_CMD_INVALID ... OCTEP_CTRL_NET_H2F_CMD_DEV_REMOVE] =
OCTEP_CP_VERSION(1, 0, 0)
};
@@ -393,10 +393,24 @@ int octep_ctrl_net_get_info(struct octep_device *oct, int vfid,
return 0;
}
+int octep_ctrl_net_dev_remove(struct octep_device *oct, int vfid)
+{
+ struct octep_ctrl_net_wait_data d = {};
+ struct octep_ctrl_net_h2f_req *req;
+
+ req = &d.data.req;
+ dev_info(&oct->pdev->dev, "Sending dev_unload msg to fw\n");
+ init_send_req(&d.msg, req, sizeof(int), vfid);
+ req->hdr.s.cmd = OCTEP_CTRL_NET_H2F_CMD_DEV_REMOVE;
+
+ return octep_send_mbox_req(oct, &d, false);
+}
int octep_ctrl_net_uninit(struct octep_device *oct)
{
struct octep_ctrl_net_wait_data *pos, *n;
+ octep_ctrl_net_dev_remove(oct, OCTEP_CTRL_NET_INVALID_VFID);
+
list_for_each_entry_safe(pos, n, &oct->ctrl_req_wait_list, list)
pos->done = 1;
@@ -42,6 +42,7 @@ enum octep_ctrl_net_h2f_cmd {
OCTEP_CTRL_NET_H2F_CMD_RX_STATE,
OCTEP_CTRL_NET_H2F_CMD_LINK_INFO,
OCTEP_CTRL_NET_H2F_CMD_GET_INFO,
+ OCTEP_CTRL_NET_H2F_CMD_DEV_REMOVE,
OCTEP_CTRL_NET_H2F_CMD_MAX
};
@@ -370,6 +371,16 @@ void octep_ctrl_net_recv_fw_messages(struct octep_device *oct);
int octep_ctrl_net_get_info(struct octep_device *oct, int vfid,
struct octep_fw_info *info);
+/**
+ * octep_ctrl_net_dev_remove() - Indicate to firmware that a device unload has happened.
+ *
+ * @oct: non-null pointer to struct octep_device.
+ * @vfid: Index of virtual function.
+ *
+ * return value: 0 on success, -errno on failure.
+ */
+int octep_ctrl_net_dev_remove(struct octep_device *oct, int vfid);
+
/**
* octep_ctrl_net_uninit() - Uninitialize data for ctrl net.
*