[net-next,v8,1/8] bnxt_en: Add auxiliary driver support
Commit Message
Add auxiliary driver support.
An auxiliary device will be created if the hardware indicates
support for RDMA.
The bnxt_ulp_probe() function has been removed and a new
bnxt_rdma_aux_device_add() function has been added.
The bnxt_free_msix_vecs() and bnxt_req_msix_vecs() will now hold
the RTNL lock when they call the bnxt_close_nic()and bnxt_open_nic()
since the device close and open need to be protected under RTNL lock.
The operations between the bnxt_en and bnxt_re will be protected
using the en_ops_lock.
This will be used by the bnxt_re driver in a follow-on patch
to create ROCE interfaces.
Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Reviewed-by: Selvin Xavier <selvin.xavier@broadcom.com>
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
---
drivers/net/ethernet/broadcom/Kconfig | 1 +
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 +-
drivers/net/ethernet/broadcom/bnxt/bnxt.h | 8 +
drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 161 +++++++++++++++---
drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h | 4 +-
5 files changed, 155 insertions(+), 27 deletions(-)
Comments
On Thu, 19 Jan 2023 22:05:28 -0800 Ajit Khaparde wrote:
> @@ -13212,6 +13214,7 @@ static void bnxt_remove_one(struct pci_dev *pdev)
> kfree(bp->rss_indir_tbl);
> bp->rss_indir_tbl = NULL;
> bnxt_free_port_stats(bp);
> + bnxt_aux_priv_free(bp);
> free_netdev(dev);
You're still freeing the memory in which struct device sits regardless
of its reference count.
Greg, is it legal to call:
auxiliary_device_delete(adev); // AKA device_del(&auxdev->dev);
auxiliary_device_uninit(adev); // AKA put_device(&auxdev->dev);
free(adev); // frees struct device
? I tried to explain this three times, maybe there's some wait during
device_del() I'm not seeing which makes this safe :S
On Mon, Jan 23, 2023 at 10:33:05PM -0800, Jakub Kicinski wrote:
> On Thu, 19 Jan 2023 22:05:28 -0800 Ajit Khaparde wrote:
> > @@ -13212,6 +13214,7 @@ static void bnxt_remove_one(struct pci_dev *pdev)
> > kfree(bp->rss_indir_tbl);
> > bp->rss_indir_tbl = NULL;
> > bnxt_free_port_stats(bp);
> > + bnxt_aux_priv_free(bp);
> > free_netdev(dev);
>
> You're still freeing the memory in which struct device sits regardless
> of its reference count.
>
> Greg, is it legal to call:
>
> auxiliary_device_delete(adev); // AKA device_del(&auxdev->dev);
> auxiliary_device_uninit(adev); // AKA put_device(&auxdev->dev);
> free(adev); // frees struct device
Ick, the aux device release callback should be doing the freeing of the
memory, you shouldn't ever have to free it "manually" like this. To do
so would be a problem (i.e. the release callback would then free it
again, right?)
> ? I tried to explain this three times, maybe there's some wait during
> device_del() I'm not seeing which makes this safe :S
Nope, no intentional wait normally. You can add one by enabling a
debugging option to find all of the places where people are doing bad
things like this by delaying the freeing of the device memory until a
few seconds later, but that's generally not something you should run in
a production kernel as it finds all sorts of nasty bugs...
thanks,
greg k-h
On Mon, Jan 23, 2023 at 10:33:05PM -0800, Jakub Kicinski wrote:
> On Thu, 19 Jan 2023 22:05:28 -0800 Ajit Khaparde wrote:
> > @@ -13212,6 +13214,7 @@ static void bnxt_remove_one(struct pci_dev *pdev)
> > kfree(bp->rss_indir_tbl);
> > bp->rss_indir_tbl = NULL;
> > bnxt_free_port_stats(bp);
> > + bnxt_aux_priv_free(bp);
> > free_netdev(dev);
>
> You're still freeing the memory in which struct device sits regardless
> of its reference count.
BTW, Ajit does the same wrong kfree in bnxt_rdma_aux_device_add() too.
+ ret = auxiliary_device_add(aux_dev);
+ if (ret)
+ goto aux_dev_uninit;
+
...
+aux_dev_uninit:
+ auxiliary_device_uninit(aux_dev);
+free_edev:
+ kfree(edev); <----- wrong
+ bp->edev = NULL;
Thanks
On Mon, Jan 23, 2023 at 10:42 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jan 23, 2023 at 10:33:05PM -0800, Jakub Kicinski wrote:
> > On Thu, 19 Jan 2023 22:05:28 -0800 Ajit Khaparde wrote:
> > > @@ -13212,6 +13214,7 @@ static void bnxt_remove_one(struct pci_dev *pdev)
> > > kfree(bp->rss_indir_tbl);
> > > bp->rss_indir_tbl = NULL;
> > > bnxt_free_port_stats(bp);
> > > + bnxt_aux_priv_free(bp);
> > > free_netdev(dev);
> >
> > You're still freeing the memory in which struct device sits regardless
> > of its reference count.
> >
> > Greg, is it legal to call:
> >
> > auxiliary_device_delete(adev); // AKA device_del(&auxdev->dev);
> > auxiliary_device_uninit(adev); // AKA put_device(&auxdev->dev);
> > free(adev); // frees struct device
>
> Ick, the aux device release callback should be doing the freeing of the
> memory, you shouldn't ever have to free it "manually" like this. To do
> so would be a problem (i.e. the release callback would then free it
> again, right?)
Yikes!
Thanks for the refresher.
>
> > ? I tried to explain this three times, maybe there's some wait during
> > device_del() I'm not seeing which makes this safe :S
Apologies.
I thought since the driver was allocating the memory, it had to free it.
I stand corrected.
Thanks
Ajit
>
> Nope, no intentional wait normally. You can add one by enabling a
> debugging option to find all of the places where people are doing bad
> things like this by delaying the freeing of the device memory until a
> few seconds later, but that's generally not something you should run in
> a production kernel as it finds all sorts of nasty bugs...
>
> thanks,
>
> greg k-h
On Mon, Jan 23, 2023 at 10:53 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Mon, Jan 23, 2023 at 10:33:05PM -0800, Jakub Kicinski wrote:
> > On Thu, 19 Jan 2023 22:05:28 -0800 Ajit Khaparde wrote:
> > > @@ -13212,6 +13214,7 @@ static void bnxt_remove_one(struct pci_dev *pdev)
> > > kfree(bp->rss_indir_tbl);
> > > bp->rss_indir_tbl = NULL;
> > > bnxt_free_port_stats(bp);
> > > + bnxt_aux_priv_free(bp);
> > > free_netdev(dev);
> >
> > You're still freeing the memory in which struct device sits regardless
> > of its reference count.
>
> BTW, Ajit does the same wrong kfree in bnxt_rdma_aux_device_add() too.
I will address this in the next version. Thanks again.
Thanks
Ajit
> + ret = auxiliary_device_add(aux_dev);
> + if (ret)
> + goto aux_dev_uninit;
> +
> ...
> +aux_dev_uninit:
> + auxiliary_device_uninit(aux_dev);
> +free_edev:
> + kfree(edev); <----- wrong
> + bp->edev = NULL;
>
> Thanks
On Mon, Jan 23, 2023 at 11:07:07PM -0800, Ajit Khaparde wrote:
> On Mon, Jan 23, 2023 at 10:42 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jan 23, 2023 at 10:33:05PM -0800, Jakub Kicinski wrote:
> > > On Thu, 19 Jan 2023 22:05:28 -0800 Ajit Khaparde wrote:
> > > > @@ -13212,6 +13214,7 @@ static void bnxt_remove_one(struct pci_dev *pdev)
> > > > kfree(bp->rss_indir_tbl);
> > > > bp->rss_indir_tbl = NULL;
> > > > bnxt_free_port_stats(bp);
> > > > + bnxt_aux_priv_free(bp);
> > > > free_netdev(dev);
> > >
> > > You're still freeing the memory in which struct device sits regardless
> > > of its reference count.
> > >
> > > Greg, is it legal to call:
> > >
> > > auxiliary_device_delete(adev); // AKA device_del(&auxdev->dev);
> > > auxiliary_device_uninit(adev); // AKA put_device(&auxdev->dev);
> > > free(adev); // frees struct device
> >
> > Ick, the aux device release callback should be doing the freeing of the
> > memory, you shouldn't ever have to free it "manually" like this. To do
> > so would be a problem (i.e. the release callback would then free it
> > again, right?)
> Yikes!
> Thanks for the refresher.
>
> >
> > > ? I tried to explain this three times, maybe there's some wait during
> > > device_del() I'm not seeing which makes this safe :S
> Apologies.
> I thought since the driver was allocating the memory, it had to free it.
Yes, you do, in the release function that will be called when the device
reference count is dropped to 0.
thanks,
greg k-h
@@ -213,6 +213,7 @@ config BNXT
select NET_DEVLINK
select PAGE_POOL
select DIMLIB
+ select AUXILIARY_BUS
help
This driver supports Broadcom NetXtreme-C/E 10/25/40/50 gigabit
Ethernet cards. To compile this driver as a module, choose M here:
@@ -13178,6 +13178,8 @@ static void bnxt_remove_one(struct pci_dev *pdev)
struct net_device *dev = pci_get_drvdata(pdev);
struct bnxt *bp = netdev_priv(dev);
+ bnxt_rdma_aux_device_uninit(bp);
+
if (BNXT_PF(bp))
bnxt_sriov_disable(bp);
@@ -13212,6 +13214,7 @@ static void bnxt_remove_one(struct pci_dev *pdev)
kfree(bp->rss_indir_tbl);
bp->rss_indir_tbl = NULL;
bnxt_free_port_stats(bp);
+ bnxt_aux_priv_free(bp);
free_netdev(dev);
}
@@ -13776,11 +13779,13 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
bnxt_dl_fw_reporters_create(bp);
+ bnxt_rdma_aux_device_init(bp);
+
bnxt_print_device_info(bp);
pci_save_state(pdev);
- return 0;
+ return 0;
init_err_cleanup:
bnxt_dl_unregister(bp);
init_err_dl:
@@ -13824,7 +13829,6 @@ static void bnxt_shutdown(struct pci_dev *pdev)
if (netif_running(dev))
dev_close(dev);
- bnxt_ulp_shutdown(bp);
bnxt_clear_int_mode(bp);
pci_disable_device(pdev);
@@ -24,6 +24,7 @@
#include <linux/interrupt.h>
#include <linux/rhashtable.h>
#include <linux/crash_dump.h>
+#include <linux/auxiliary_bus.h>
#include <net/devlink.h>
#include <net/dst_metadata.h>
#include <net/xdp.h>
@@ -1631,6 +1632,12 @@ struct bnxt_fw_health {
#define BNXT_FW_IF_RETRY 10
#define BNXT_FW_SLOT_RESET_RETRY 4
+struct bnxt_aux_priv {
+ struct auxiliary_device aux_dev;
+ struct bnxt_en_dev *edev;
+ int id;
+};
+
enum board_idx {
BCM57301,
BCM57302,
@@ -1852,6 +1859,7 @@ struct bnxt {
#define BNXT_CHIP_P4_PLUS(bp) \
(BNXT_CHIP_P4(bp) || BNXT_CHIP_P5(bp))
+ struct bnxt_aux_priv *aux_priv;
struct bnxt_en_dev *edev;
struct bnxt_napi **bnapi;
@@ -25,6 +25,8 @@
#include "bnxt_hwrm.h"
#include "bnxt_ulp.h"
+static DEFINE_IDA(bnxt_aux_dev_ids);
+
static int bnxt_register_dev(struct bnxt_en_dev *edev, unsigned int ulp_id,
struct bnxt_ulp_ops *ulp_ops, void *handle)
{
@@ -32,7 +34,6 @@ static int bnxt_register_dev(struct bnxt_en_dev *edev, unsigned int ulp_id,
struct bnxt *bp = netdev_priv(dev);
struct bnxt_ulp *ulp;
- ASSERT_RTNL();
if (ulp_id >= BNXT_MAX_ULP)
return -EINVAL;
@@ -50,7 +51,7 @@ static int bnxt_register_dev(struct bnxt_en_dev *edev, unsigned int ulp_id,
return -ENOMEM;
}
- atomic_set(&ulp->ref_count, 0);
+ atomic_set(&ulp->ref_count, 1);
ulp->handle = handle;
rcu_assign_pointer(ulp->ulp_ops, ulp_ops);
@@ -69,10 +70,11 @@ static int bnxt_unregister_dev(struct bnxt_en_dev *edev, unsigned int ulp_id)
struct bnxt_ulp *ulp;
int i = 0;
- ASSERT_RTNL();
if (ulp_id >= BNXT_MAX_ULP)
return -EINVAL;
+ edev->flags |= BNXT_EN_FLAG_ULP_STOPPED;
+
ulp = &edev->ulp_tbl[ulp_id];
if (!rcu_access_pointer(ulp->ulp_ops)) {
netdev_err(bp->dev, "ulp id %d not registered\n", ulp_id);
@@ -126,7 +128,6 @@ static int bnxt_req_msix_vecs(struct bnxt_en_dev *edev, unsigned int ulp_id,
int total_vecs;
int rc = 0;
- ASSERT_RTNL();
if (ulp_id != BNXT_ROCE_ULP)
return -EINVAL;
@@ -149,10 +150,12 @@ static int bnxt_req_msix_vecs(struct bnxt_en_dev *edev, unsigned int ulp_id,
max_idx = min_t(int, bp->total_irqs, max_cp_rings);
idx = max_idx - avail_msix;
}
+
edev->ulp_tbl[ulp_id].msix_base = idx;
edev->ulp_tbl[ulp_id].msix_requested = avail_msix;
hw_resc = &bp->hw_resc;
total_vecs = idx + avail_msix;
+ rtnl_lock();
if (bp->total_irqs < total_vecs ||
(BNXT_NEW_RM(bp) && hw_resc->resv_irqs < total_vecs)) {
if (netif_running(dev)) {
@@ -162,6 +165,7 @@ static int bnxt_req_msix_vecs(struct bnxt_en_dev *edev, unsigned int ulp_id,
rc = bnxt_reserve_rings(bp, true);
}
}
+ rtnl_unlock();
if (rc) {
edev->ulp_tbl[ulp_id].msix_requested = 0;
return -EAGAIN;
@@ -184,7 +188,6 @@ static int bnxt_free_msix_vecs(struct bnxt_en_dev *edev, unsigned int ulp_id)
struct net_device *dev = edev->net;
struct bnxt *bp = netdev_priv(dev);
- ASSERT_RTNL();
if (ulp_id != BNXT_ROCE_ULP)
return -EINVAL;
@@ -193,10 +196,13 @@ static int bnxt_free_msix_vecs(struct bnxt_en_dev *edev, unsigned int ulp_id)
edev->ulp_tbl[ulp_id].msix_requested = 0;
edev->flags &= ~BNXT_EN_FLAG_MSIX_REQUESTED;
+ rtnl_lock();
if (netif_running(dev) && !(edev->flags & BNXT_EN_FLAG_ULP_STOPPED)) {
bnxt_close_nic(bp, true, false);
bnxt_open_nic(bp, true, false);
}
+ rtnl_unlock();
+
return 0;
}
@@ -347,25 +353,6 @@ void bnxt_ulp_sriov_cfg(struct bnxt *bp, int num_vfs)
}
}
-void bnxt_ulp_shutdown(struct bnxt *bp)
-{
- struct bnxt_en_dev *edev = bp->edev;
- struct bnxt_ulp_ops *ops;
- int i;
-
- if (!edev)
- return;
-
- for (i = 0; i < BNXT_MAX_ULP; i++) {
- struct bnxt_ulp *ulp = &edev->ulp_tbl[i];
-
- ops = rtnl_dereference(ulp->ulp_ops);
- if (!ops || !ops->ulp_shutdown)
- continue;
- ops->ulp_shutdown(ulp->handle);
- }
-}
-
void bnxt_ulp_irq_stop(struct bnxt *bp)
{
struct bnxt_en_dev *edev = bp->edev;
@@ -475,6 +462,132 @@ static const struct bnxt_en_ops bnxt_en_ops_tbl = {
.bnxt_register_fw_async_events = bnxt_register_async_events,
};
+void bnxt_aux_priv_free(struct bnxt *bp)
+{
+ kfree(bp->aux_priv);
+}
+
+static struct bnxt_aux_priv *bnxt_aux_priv_alloc(struct bnxt *bp)
+{
+ return kzalloc(sizeof(struct bnxt_aux_priv), GFP_KERNEL);
+}
+
+void bnxt_rdma_aux_device_uninit(struct bnxt *bp)
+{
+ struct bnxt_aux_priv *aux_priv;
+ struct auxiliary_device *adev;
+
+ /* Skip if no auxiliary device init was done. */
+ if (!(bp->flags & BNXT_FLAG_ROCE_CAP))
+ return;
+
+ aux_priv = bp->aux_priv;
+ adev = &aux_priv->aux_dev;
+ auxiliary_device_delete(adev);
+ auxiliary_device_uninit(adev);
+ ida_free(&bnxt_aux_dev_ids, aux_priv->id);
+}
+
+static void bnxt_aux_dev_release(struct device *dev)
+{
+ struct bnxt_aux_priv *aux_priv =
+ container_of(dev, struct bnxt_aux_priv, aux_dev.dev);
+ struct bnxt *bp = netdev_priv(aux_priv->edev->net);
+
+ aux_priv->edev->en_ops = NULL;
+ kfree(aux_priv->edev);
+ aux_priv->edev = NULL;
+ bp->edev = NULL;
+}
+
+static void bnxt_set_edev_info(struct bnxt_en_dev *edev, struct bnxt *bp)
+{
+ edev->en_ops = &bnxt_en_ops_tbl;
+ edev->net = bp->dev;
+ edev->pdev = bp->pdev;
+ edev->l2_db_size = bp->db_size;
+ edev->l2_db_size_nc = bp->db_size;
+
+ if (bp->flags & BNXT_FLAG_ROCEV1_CAP)
+ edev->flags |= BNXT_EN_FLAG_ROCEV1_CAP;
+ if (bp->flags & BNXT_FLAG_ROCEV2_CAP)
+ edev->flags |= BNXT_EN_FLAG_ROCEV2_CAP;
+}
+
+static int bnxt_rdma_aux_device_add(struct bnxt *bp)
+{
+ struct bnxt_aux_priv *aux_priv = bp->aux_priv;
+ struct bnxt_en_dev *edev = aux_priv->edev;
+ struct auxiliary_device *aux_dev;
+ int ret;
+
+ edev = kzalloc(sizeof(*edev), GFP_KERNEL);
+ if (!edev)
+ return -ENOMEM;
+
+ aux_dev = &aux_priv->aux_dev;
+ aux_dev->id = aux_priv->id;
+ aux_dev->name = "rdma";
+ aux_dev->dev.parent = &bp->pdev->dev;
+ aux_dev->dev.release = bnxt_aux_dev_release;
+
+ aux_priv->edev = edev;
+ bp->edev = edev;
+ bnxt_set_edev_info(edev, bp);
+
+ ret = auxiliary_device_init(aux_dev);
+ if (ret)
+ goto free_edev;
+
+ ret = auxiliary_device_add(aux_dev);
+ if (ret)
+ goto aux_dev_uninit;
+
+ return 0;
+aux_dev_uninit:
+ auxiliary_device_uninit(aux_dev);
+free_edev:
+ kfree(edev);
+ bp->edev = NULL;
+
+ return ret;
+}
+
+void bnxt_rdma_aux_device_init(struct bnxt *bp)
+{
+ int rc;
+
+ if (!(bp->flags & BNXT_FLAG_ROCE_CAP))
+ return;
+
+ bp->aux_priv = bnxt_aux_priv_alloc(bp);
+ if (!bp->aux_priv)
+ goto skip_ida_init;
+
+ bp->aux_priv->id = ida_alloc(&bnxt_aux_dev_ids, GFP_KERNEL);
+ if (bp->aux_priv->id < 0) {
+ netdev_warn(bp->dev,
+ "ida alloc failed for ROCE auxiliary device\n");
+ goto skip_aux_init;
+ }
+
+ /* If aux bus init fails, continue with netdev init. */
+ rc = bnxt_rdma_aux_device_add(bp);
+ if (rc) {
+ netdev_warn(bp->dev,
+ "Failed to add auxiliary device for ROCE\n");
+ goto aux_add_failed;
+ }
+ return;
+
+aux_add_failed:
+ ida_free(&bnxt_aux_dev_ids, bp->aux_priv->id);
+skip_aux_init:
+ bnxt_aux_priv_free(bp);
+skip_ida_init:
+ bp->flags &= ~BNXT_FLAG_ROCE_CAP;
+}
+
struct bnxt_en_dev *bnxt_ulp_probe(struct net_device *dev)
{
struct bnxt *bp = netdev_priv(dev);
@@ -102,10 +102,12 @@ int bnxt_get_ulp_stat_ctxs(struct bnxt *bp);
void bnxt_ulp_stop(struct bnxt *bp);
void bnxt_ulp_start(struct bnxt *bp, int err);
void bnxt_ulp_sriov_cfg(struct bnxt *bp, int num_vfs);
-void bnxt_ulp_shutdown(struct bnxt *bp);
void bnxt_ulp_irq_stop(struct bnxt *bp);
void bnxt_ulp_irq_restart(struct bnxt *bp, int err);
void bnxt_ulp_async_events(struct bnxt *bp, struct hwrm_async_event_cmpl *cmpl);
+void bnxt_rdma_aux_device_uninit(struct bnxt *bp);
+void bnxt_rdma_aux_device_init(struct bnxt *bp);
+void bnxt_aux_priv_free(struct bnxt *bp);
struct bnxt_en_dev *bnxt_ulp_probe(struct net_device *dev);
#endif