| Message ID | 20230322091958.13103-7-vburru@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | octeon_ep: deferred probe and mailbox | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 18 this patch: 18 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 18 this patch: 18 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 18 this patch: 18 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 41 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Wed, Mar 22, 2023 at 02:19:55AM -0700, Veerasenareddy Burru wrote: > Add asynchronous notification support to the control mailbox. > > Signed-off-by: Veerasenareddy Burru <vburru@marvell.com> > Signed-off-by: Abhijit Ayarekar <aayarekar@marvell.com> > --- > v3 -> v4: > * 0005-xxx.patch in v3 is 0006-xxx.patch in v4. > * addressed review comments > https://lore.kernel.org/all/Y+0J94sowllCe5Gs@boxer/ > - fixed rct violation. > - process_mbox_notify() now returns void. > > v2 -> v3: > * no change > > v1 -> v2: > * no change > > .../marvell/octeon_ep/octep_ctrl_net.c | 29 +++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c > index cef4bc3b1ec0..465eef2824e3 100644 > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c > @@ -271,6 +271,33 @@ static void process_mbox_resp(struct octep_device *oct, > } > } > > +static int process_mbox_notify(struct octep_device *oct, > + struct octep_ctrl_mbox_msg *msg) > +{ > + struct net_device *netdev = oct->netdev; > + struct octep_ctrl_net_f2h_req *req; > + > + req = (struct octep_ctrl_net_f2h_req *)msg->sg_list[0].msg; > + switch (req->hdr.s.cmd) { > + case OCTEP_CTRL_NET_F2H_CMD_LINK_STATUS: > + if (netif_running(netdev)) { > + if (req->link.state) { > + dev_info(&oct->pdev->dev, "netif_carrier_on\n"); > + netif_carrier_on(netdev); > + } else { > + dev_info(&oct->pdev->dev, "netif_carrier_off\n"); > + netif_carrier_off(netdev); > + } Shouldn't netdev changes be protected by some lock? Is is safe to get event from FW and process it as is? Thanks
> -----Original Message----- > From: Leon Romanovsky <leon@kernel.org> > Sent: Thursday, March 23, 2023 3:39 AM > To: Veerasenareddy Burru <vburru@marvell.com> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Abhijit Ayarekar > <aayarekar@marvell.com>; Sathesh B Edara <sedara@marvell.com>; > Satananda Burla <sburla@marvell.com>; linux-doc@vger.kernel.org; David S. > Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; > Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com> > Subject: [EXT] Re: [PATCH net-next v4 6/8] octeon_ep: support > asynchronous notifications > > External Email > > ---------------------------------------------------------------------- > On Wed, Mar 22, 2023 at 02:19:55AM -0700, Veerasenareddy Burru wrote: > > Add asynchronous notification support to the control mailbox. > > > > Signed-off-by: Veerasenareddy Burru <vburru@marvell.com> > > Signed-off-by: Abhijit Ayarekar <aayarekar@marvell.com> > > --- > > v3 -> v4: > > * 0005-xxx.patch in v3 is 0006-xxx.patch in v4. > > * addressed review comments > > https://urldefense.proofpoint.com/v2/url?u=https- > 3A__lore.kernel.org_all_Y-2B0J94sowllCe5Gs- > 40boxer_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=XkP_75lnbPIeeucsP > X36ZgjiMqEKttwZfwNyWMCLjT0&m=5CnsD- > SX6ZoW98szwM0k4IXgNC3wY0EwCQHxDKGyNIRUJxdaNe3zorLcOhc9iU6d&s > =k73McQSsjbjj87VbCCB8EFFtGWtksMIGhn15RK12XF8&e= > > - fixed rct violation. > > - process_mbox_notify() now returns void. > > > > v2 -> v3: > > * no change > > > > v1 -> v2: > > * no change > > > > .../marvell/octeon_ep/octep_ctrl_net.c | 29 +++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c > > b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c > > index cef4bc3b1ec0..465eef2824e3 100644 > > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c > > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c > > @@ -271,6 +271,33 @@ static void process_mbox_resp(struct > octep_device *oct, > > } > > } > > > > +static int process_mbox_notify(struct octep_device *oct, > > + struct octep_ctrl_mbox_msg *msg) { > > + struct net_device *netdev = oct->netdev; > > + struct octep_ctrl_net_f2h_req *req; > > + > > + req = (struct octep_ctrl_net_f2h_req *)msg->sg_list[0].msg; > > + switch (req->hdr.s.cmd) { > > + case OCTEP_CTRL_NET_F2H_CMD_LINK_STATUS: > > + if (netif_running(netdev)) { > > + if (req->link.state) { > > + dev_info(&oct->pdev->dev, > "netif_carrier_on\n"); > > + netif_carrier_on(netdev); > > + } else { > > + dev_info(&oct->pdev->dev, > "netif_carrier_off\n"); > > + netif_carrier_off(netdev); > > + } > > Shouldn't netdev changes be protected by some lock? > Is is safe to get event from FW and process it as is? > > Thanks Thanks for the kind feedback. I do not see netif_carrier_on/off require any protection. I referred few other drivers and do not see such protection used for carrier on/off. Please suggest if I am missing something here.
On Thu, Mar 23, 2023 at 05:24:55PM +0000, Veerasenareddy Burru wrote: > > > > -----Original Message----- > > From: Leon Romanovsky <leon@kernel.org> > > Sent: Thursday, March 23, 2023 3:39 AM > > To: Veerasenareddy Burru <vburru@marvell.com> > > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Abhijit Ayarekar > > <aayarekar@marvell.com>; Sathesh B Edara <sedara@marvell.com>; > > Satananda Burla <sburla@marvell.com>; linux-doc@vger.kernel.org; David S. > > Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; > > Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com> > > Subject: [EXT] Re: [PATCH net-next v4 6/8] octeon_ep: support > > asynchronous notifications > > > > External Email > > > > ---------------------------------------------------------------------- > > On Wed, Mar 22, 2023 at 02:19:55AM -0700, Veerasenareddy Burru wrote: > > > Add asynchronous notification support to the control mailbox. > > > > > > Signed-off-by: Veerasenareddy Burru <vburru@marvell.com> > > > Signed-off-by: Abhijit Ayarekar <aayarekar@marvell.com> > > > --- > > > v3 -> v4: > > > * 0005-xxx.patch in v3 is 0006-xxx.patch in v4. > > > * addressed review comments > > > https://urldefense.proofpoint.com/v2/url?u=https- > > 3A__lore.kernel.org_all_Y-2B0J94sowllCe5Gs- > > 40boxer_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=XkP_75lnbPIeeucsP > > X36ZgjiMqEKttwZfwNyWMCLjT0&m=5CnsD- > > SX6ZoW98szwM0k4IXgNC3wY0EwCQHxDKGyNIRUJxdaNe3zorLcOhc9iU6d&s > > =k73McQSsjbjj87VbCCB8EFFtGWtksMIGhn15RK12XF8&e= > > > - fixed rct violation. > > > - process_mbox_notify() now returns void. > > > > > > v2 -> v3: > > > * no change > > > > > > v1 -> v2: > > > * no change > > > > > > .../marvell/octeon_ep/octep_ctrl_net.c | 29 +++++++++++++++++++ > > > 1 file changed, 29 insertions(+) > > > > > > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c > > > b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c > > > index cef4bc3b1ec0..465eef2824e3 100644 > > > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c > > > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c > > > @@ -271,6 +271,33 @@ static void process_mbox_resp(struct > > octep_device *oct, > > > } > > > } > > > > > > +static int process_mbox_notify(struct octep_device *oct, > > > + struct octep_ctrl_mbox_msg *msg) { > > > + struct net_device *netdev = oct->netdev; > > > + struct octep_ctrl_net_f2h_req *req; > > > + > > > + req = (struct octep_ctrl_net_f2h_req *)msg->sg_list[0].msg; > > > + switch (req->hdr.s.cmd) { > > > + case OCTEP_CTRL_NET_F2H_CMD_LINK_STATUS: > > > + if (netif_running(netdev)) { > > > + if (req->link.state) { > > > + dev_info(&oct->pdev->dev, > > "netif_carrier_on\n"); > > > + netif_carrier_on(netdev); > > > + } else { > > > + dev_info(&oct->pdev->dev, > > "netif_carrier_off\n"); > > > + netif_carrier_off(netdev); > > > + } > > > > Shouldn't netdev changes be protected by some lock? > > Is is safe to get event from FW and process it as is? > > > > Thanks > > Thanks for the kind feedback. > I do not see netif_carrier_on/off require any protection. I referred few other drivers and do not see such protection used for carrier on/off. > Please suggest if I am missing something here. I see that Dave already applied your v5. I think that you are missing context in which you are running FW commands. They run independently from netdev and makes netif_running() check to be racy. Thanks >
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c index cef4bc3b1ec0..465eef2824e3 100644 --- a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c @@ -271,6 +271,33 @@ static void process_mbox_resp(struct octep_device *oct, } } +static int process_mbox_notify(struct octep_device *oct, + struct octep_ctrl_mbox_msg *msg) +{ + struct net_device *netdev = oct->netdev; + struct octep_ctrl_net_f2h_req *req; + + req = (struct octep_ctrl_net_f2h_req *)msg->sg_list[0].msg; + switch (req->hdr.s.cmd) { + case OCTEP_CTRL_NET_F2H_CMD_LINK_STATUS: + if (netif_running(netdev)) { + if (req->link.state) { + dev_info(&oct->pdev->dev, "netif_carrier_on\n"); + netif_carrier_on(netdev); + } else { + dev_info(&oct->pdev->dev, "netif_carrier_off\n"); + netif_carrier_off(netdev); + } + } + break; + default: + pr_info("Unknown mbox req : %u\n", req->hdr.s.cmd); + break; + } + + return 0; +} + void octep_ctrl_net_recv_fw_messages(struct octep_device *oct) { static u16 msg_sz = sizeof(union octep_ctrl_net_max_data); @@ -291,6 +318,8 @@ void octep_ctrl_net_recv_fw_messages(struct octep_device *oct) if (msg.hdr.s.flags & OCTEP_CTRL_MBOX_MSG_HDR_FLAG_RESP) process_mbox_resp(oct, &msg); + else if (msg.hdr.s.flags & OCTEP_CTRL_MBOX_MSG_HDR_FLAG_NOTIFY) + process_mbox_notify(oct, &msg); } }