| Message ID | 20250213091418.2067626-8-tianx@yunsilicon.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net-next/yunsilicon: ADD Yunsilicon XSC Ethernet Driver | expand |
On Thu, Feb 13, 2025 at 05:14:19PM +0800, Xin Tian wrote: > Initialize eth auxiliary device when pci probing > > Co-developed-by: Honggang Wei <weihg@yunsilicon.com> > Signed-off-by: Honggang Wei <weihg@yunsilicon.com> > Co-developed-by: Lei Yan <jacky@yunsilicon.com> > Signed-off-by: Lei Yan <jacky@yunsilicon.com> > Signed-off-by: Xin Tian <tianx@yunsilicon.com> > --- > .../ethernet/yunsilicon/xsc/common/xsc_core.h | 12 ++ > .../net/ethernet/yunsilicon/xsc/pci/Makefile | 3 +- > .../net/ethernet/yunsilicon/xsc/pci/adev.c | 110 ++++++++++++++++++ > .../net/ethernet/yunsilicon/xsc/pci/adev.h | 14 +++ > .../net/ethernet/yunsilicon/xsc/pci/main.c | 10 ++ > 5 files changed, 148 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/ethernet/yunsilicon/xsc/pci/adev.c > create mode 100644 drivers/net/ethernet/yunsilicon/xsc/pci/adev.h <...> > diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/adev.c b/drivers/net/ethernet/yunsilicon/xsc/pci/adev.c > new file mode 100644 > index 000000000..1f8f27d72 > --- /dev/null > +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/adev.c > @@ -0,0 +1,110 @@ > +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB > +/* > + * Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd. > + * All rights reserved. > + */ > + > +#include <linux/auxiliary_bus.h> > +#include <linux/idr.h> > + > +#include "adev.h" > + > +static DEFINE_IDA(xsc_adev_ida); > + > +enum xsc_adev_idx { > + XSC_ADEV_IDX_ETH, > + XSC_ADEV_IDX_MAX > +}; > + > +static const char * const xsc_adev_name[] = { > + [XSC_ADEV_IDX_ETH] = XSC_ETH_ADEV_NAME, > +}; > + > +static void xsc_release_adev(struct device *dev) > +{ > + /* Doing nothing, but auxiliary bus requires a release function */ > +} It is unlikely to be true in driver lifetime model. At least you should free xsc_adev here. Thanks > + > +static int xsc_reg_adev(struct xsc_core_device *xdev, int idx) > +{ > + struct auxiliary_device *adev; > + struct xsc_adev *xsc_adev; > + int ret; > + > + xsc_adev = kzalloc(sizeof(*xsc_adev), GFP_KERNEL); > + if (!xsc_adev) > + return -ENOMEM; > + > + adev = &xsc_adev->adev; > + adev->name = xsc_adev_name[idx]; > + adev->id = xdev->adev_id; > + adev->dev.parent = &xdev->pdev->dev; > + adev->dev.release = xsc_release_adev; > + xsc_adev->xdev = xdev; > + > + ret = auxiliary_device_init(adev); > + if (ret) > + goto err_free_adev; > + > + ret = auxiliary_device_add(adev); > + if (ret) > + goto err_uninit_adev; > + > + xdev->xsc_adev_list[idx] = xsc_adev; > + > + return 0; > +err_uninit_adev: > + auxiliary_device_uninit(adev); > +err_free_adev: > + kfree(xsc_adev); > + > + return ret; > +}
On 2025/2/13 22:37, Leon Romanovsky wrote: > On Thu, Feb 13, 2025 at 05:14:19PM +0800, Xin Tian wrote: >> Initialize eth auxiliary device when pci probing >> >> Co-developed-by: Honggang Wei <weihg@yunsilicon.com> >> Signed-off-by: Honggang Wei <weihg@yunsilicon.com> >> Co-developed-by: Lei Yan <jacky@yunsilicon.com> >> Signed-off-by: Lei Yan <jacky@yunsilicon.com> >> Signed-off-by: Xin Tian <tianx@yunsilicon.com> >> --- >> .../ethernet/yunsilicon/xsc/common/xsc_core.h | 12 ++ >> .../net/ethernet/yunsilicon/xsc/pci/Makefile | 3 +- >> .../net/ethernet/yunsilicon/xsc/pci/adev.c | 110 ++++++++++++++++++ >> .../net/ethernet/yunsilicon/xsc/pci/adev.h | 14 +++ >> .../net/ethernet/yunsilicon/xsc/pci/main.c | 10 ++ >> 5 files changed, 148 insertions(+), 1 deletion(-) >> create mode 100644 drivers/net/ethernet/yunsilicon/xsc/pci/adev.c >> create mode 100644 drivers/net/ethernet/yunsilicon/xsc/pci/adev.h > <...> > >> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/adev.c b/drivers/net/ethernet/yunsilicon/xsc/pci/adev.c >> new file mode 100644 >> index 000000000..1f8f27d72 >> --- /dev/null >> +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/adev.c >> @@ -0,0 +1,110 @@ >> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB >> +/* >> + * Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd. >> + * All rights reserved. >> + */ >> + >> +#include <linux/auxiliary_bus.h> >> +#include <linux/idr.h> >> + >> +#include "adev.h" >> + >> +static DEFINE_IDA(xsc_adev_ida); >> + >> +enum xsc_adev_idx { >> + XSC_ADEV_IDX_ETH, >> + XSC_ADEV_IDX_MAX >> +}; >> + >> +static const char * const xsc_adev_name[] = { >> + [XSC_ADEV_IDX_ETH] = XSC_ETH_ADEV_NAME, >> +}; >> + >> +static void xsc_release_adev(struct device *dev) >> +{ >> + /* Doing nothing, but auxiliary bus requires a release function */ >> +} > It is unlikely to be true in driver lifetime model. At least you should > free xsc_adev here. > > Thanks Hi Leon, xsc_adev has already been freed after calling auxiliary_device_uninit. If I free it again in the release callback, it will cause a double free. >> + >> +static int xsc_reg_adev(struct xsc_core_device *xdev, int idx) >> +{ >> + struct auxiliary_device *adev; >> + struct xsc_adev *xsc_adev; >> + int ret; >> + >> + xsc_adev = kzalloc(sizeof(*xsc_adev), GFP_KERNEL); >> + if (!xsc_adev) >> + return -ENOMEM; >> + >> + adev = &xsc_adev->adev; >> + adev->name = xsc_adev_name[idx]; >> + adev->id = xdev->adev_id; >> + adev->dev.parent = &xdev->pdev->dev; >> + adev->dev.release = xsc_release_adev; >> + xsc_adev->xdev = xdev; >> + >> + ret = auxiliary_device_init(adev); >> + if (ret) >> + goto err_free_adev; >> + >> + ret = auxiliary_device_add(adev); >> + if (ret) >> + goto err_uninit_adev; >> + >> + xdev->xsc_adev_list[idx] = xsc_adev; >> + >> + return 0; >> +err_uninit_adev: >> + auxiliary_device_uninit(adev); >> +err_free_adev: >> + kfree(xsc_adev); >> + >> + return ret; >> +}
On Fri, Feb 14, 2025 at 11:14:45AM +0800, tianx wrote: > On 2025/2/13 22:37, Leon Romanovsky wrote: > > On Thu, Feb 13, 2025 at 05:14:19PM +0800, Xin Tian wrote: > >> Initialize eth auxiliary device when pci probing > >> > >> Co-developed-by: Honggang Wei <weihg@yunsilicon.com> > >> Signed-off-by: Honggang Wei <weihg@yunsilicon.com> > >> Co-developed-by: Lei Yan <jacky@yunsilicon.com> > >> Signed-off-by: Lei Yan <jacky@yunsilicon.com> > >> Signed-off-by: Xin Tian <tianx@yunsilicon.com> > >> --- > >> .../ethernet/yunsilicon/xsc/common/xsc_core.h | 12 ++ > >> .../net/ethernet/yunsilicon/xsc/pci/Makefile | 3 +- > >> .../net/ethernet/yunsilicon/xsc/pci/adev.c | 110 ++++++++++++++++++ > >> .../net/ethernet/yunsilicon/xsc/pci/adev.h | 14 +++ > >> .../net/ethernet/yunsilicon/xsc/pci/main.c | 10 ++ > >> 5 files changed, 148 insertions(+), 1 deletion(-) > >> create mode 100644 drivers/net/ethernet/yunsilicon/xsc/pci/adev.c > >> create mode 100644 drivers/net/ethernet/yunsilicon/xsc/pci/adev.h > > <...> <...> > >> + [XSC_ADEV_IDX_ETH] = XSC_ETH_ADEV_NAME, > >> +}; > >> + > >> +static void xsc_release_adev(struct device *dev) > >> +{ > >> + /* Doing nothing, but auxiliary bus requires a release function */ > >> +} > > It is unlikely to be true in driver lifetime model. At least you should > > free xsc_adev here. > > > > Thanks > > Hi Leon, xsc_adev has already been freed after calling > auxiliary_device_uninit. If I free it again in the release callback, it > will cause a double free. You should follow standard driver lifetime model. Your auxiliary_device_uninit() is wrong and shouldn't exist from the beginning. Thanks
On 2025/2/16 17:59, Leon Romanovsky wrote: > On Fri, Feb 14, 2025 at 11:14:45AM +0800, tianx wrote: >> On 2025/2/13 22:37, Leon Romanovsky wrote: >>> On Thu, Feb 13, 2025 at 05:14:19PM +0800, Xin Tian wrote: >>>> Initialize eth auxiliary device when pci probing >>>> >>>> Co-developed-by: Honggang Wei <weihg@yunsilicon.com> >>>> Signed-off-by: Honggang Wei <weihg@yunsilicon.com> >>>> Co-developed-by: Lei Yan <jacky@yunsilicon.com> >>>> Signed-off-by: Lei Yan <jacky@yunsilicon.com> >>>> Signed-off-by: Xin Tian <tianx@yunsilicon.com> >>>> --- >>>> .../ethernet/yunsilicon/xsc/common/xsc_core.h | 12 ++ >>>> .../net/ethernet/yunsilicon/xsc/pci/Makefile | 3 +- >>>> .../net/ethernet/yunsilicon/xsc/pci/adev.c | 110 ++++++++++++++++++ >>>> .../net/ethernet/yunsilicon/xsc/pci/adev.h | 14 +++ >>>> .../net/ethernet/yunsilicon/xsc/pci/main.c | 10 ++ >>>> 5 files changed, 148 insertions(+), 1 deletion(-) >>>> create mode 100644 drivers/net/ethernet/yunsilicon/xsc/pci/adev.c >>>> create mode 100644 drivers/net/ethernet/yunsilicon/xsc/pci/adev.h >>> <...> > <...> > >>>> + [XSC_ADEV_IDX_ETH] = XSC_ETH_ADEV_NAME, >>>> +}; >>>> + >>>> +static void xsc_release_adev(struct device *dev) >>>> +{ >>>> + /* Doing nothing, but auxiliary bus requires a release function */ >>>> +} >>> It is unlikely to be true in driver lifetime model. At least you should >>> free xsc_adev here. >>> >>> Thanks >> Hi Leon, xsc_adev has already been freed after calling >> auxiliary_device_uninit. If I free it again in the release callback, it >> will cause a double free. > You should follow standard driver lifetime model. Your > auxiliary_device_uninit() is wrong and shouldn't exist from the > beginning. > > Thanks OK, I'll change it. Thanks for reviewing.
diff --git a/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h b/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h index 738823474..c417a2867 100644 --- a/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h +++ b/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h @@ -7,6 +7,7 @@ #define __XSC_CORE_H #include <linux/pci.h> +#include <linux/auxiliary_bus.h> #include "common/xsc_cmdq.h" @@ -222,6 +223,15 @@ struct xsc_irq_info { char name[XSC_MAX_IRQ_NAME]; }; +// adev +#define XSC_PCI_DRV_NAME "xsc_pci" +#define XSC_ETH_ADEV_NAME "eth" + +struct xsc_adev { + struct auxiliary_device adev; + struct xsc_core_device *xdev; +}; + // hw struct xsc_reg_addr { u64 tx_db; @@ -368,6 +378,8 @@ enum xsc_interface_state { struct xsc_core_device { struct pci_dev *pdev; struct device *device; + int adev_id; + struct xsc_adev **xsc_adev_list; void *eth_priv; struct xsc_dev_resource *dev_res; int numa_node; diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/Makefile b/drivers/net/ethernet/yunsilicon/xsc/pci/Makefile index 3525d1c74..ad0ecc122 100644 --- a/drivers/net/ethernet/yunsilicon/xsc/pci/Makefile +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/Makefile @@ -6,4 +6,5 @@ ccflags-y += -I$(srctree)/drivers/net/ethernet/yunsilicon/xsc obj-$(CONFIG_YUNSILICON_XSC_PCI) += xsc_pci.o -xsc_pci-y := main.o cmdq.o hw.o qp.o cq.o alloc.o eq.o pci_irq.o +xsc_pci-y := main.o cmdq.o hw.o qp.o cq.o alloc.o eq.o pci_irq.o adev.o + diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/adev.c b/drivers/net/ethernet/yunsilicon/xsc/pci/adev.c new file mode 100644 index 000000000..1f8f27d72 --- /dev/null +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/adev.c @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB +/* + * Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd. + * All rights reserved. + */ + +#include <linux/auxiliary_bus.h> +#include <linux/idr.h> + +#include "adev.h" + +static DEFINE_IDA(xsc_adev_ida); + +enum xsc_adev_idx { + XSC_ADEV_IDX_ETH, + XSC_ADEV_IDX_MAX +}; + +static const char * const xsc_adev_name[] = { + [XSC_ADEV_IDX_ETH] = XSC_ETH_ADEV_NAME, +}; + +static void xsc_release_adev(struct device *dev) +{ + /* Doing nothing, but auxiliary bus requires a release function */ +} + +static int xsc_reg_adev(struct xsc_core_device *xdev, int idx) +{ + struct auxiliary_device *adev; + struct xsc_adev *xsc_adev; + int ret; + + xsc_adev = kzalloc(sizeof(*xsc_adev), GFP_KERNEL); + if (!xsc_adev) + return -ENOMEM; + + adev = &xsc_adev->adev; + adev->name = xsc_adev_name[idx]; + adev->id = xdev->adev_id; + adev->dev.parent = &xdev->pdev->dev; + adev->dev.release = xsc_release_adev; + xsc_adev->xdev = xdev; + + ret = auxiliary_device_init(adev); + if (ret) + goto err_free_adev; + + ret = auxiliary_device_add(adev); + if (ret) + goto err_uninit_adev; + + xdev->xsc_adev_list[idx] = xsc_adev; + + return 0; +err_uninit_adev: + auxiliary_device_uninit(adev); +err_free_adev: + kfree(xsc_adev); + + return ret; +} + +static void xsc_unreg_adev(struct xsc_core_device *xdev, int idx) +{ + struct xsc_adev *xsc_adev = xdev->xsc_adev_list[idx]; + struct auxiliary_device *adev = &xsc_adev->adev; + + auxiliary_device_delete(adev); + auxiliary_device_uninit(adev); + + kfree(xsc_adev); + xdev->xsc_adev_list[idx] = NULL; +} + +int xsc_adev_init(struct xsc_core_device *xdev) +{ + struct xsc_adev **xsc_adev_list; + int adev_id; + int ret; + + xsc_adev_list = kzalloc(sizeof(void *) * XSC_ADEV_IDX_MAX, GFP_KERNEL); + if (!xsc_adev_list) + return -ENOMEM; + xdev->xsc_adev_list = xsc_adev_list; + + adev_id = ida_alloc(&xsc_adev_ida, GFP_KERNEL); + if (adev_id < 0) + goto err_free_adev_list; + xdev->adev_id = adev_id; + + ret = xsc_reg_adev(xdev, XSC_ADEV_IDX_ETH); + if (ret) + goto err_dalloc_adev_id; + + return 0; +err_dalloc_adev_id: + ida_free(&xsc_adev_ida, xdev->adev_id); +err_free_adev_list: + kfree(xsc_adev_list); + + return ret; +} + +void xsc_adev_uninit(struct xsc_core_device *xdev) +{ + xsc_unreg_adev(xdev, XSC_ADEV_IDX_ETH); + ida_free(&xsc_adev_ida, xdev->adev_id); + kfree(xdev->xsc_adev_list); +} diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/adev.h b/drivers/net/ethernet/yunsilicon/xsc/pci/adev.h new file mode 100644 index 000000000..3de4dd26f --- /dev/null +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/adev.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd. + * All rights reserved. + */ + +#ifndef __ADEV_H +#define __ADEV_H + +#include "common/xsc_core.h" + +int xsc_adev_init(struct xsc_core_device *xdev); +void xsc_adev_uninit(struct xsc_core_device *xdev); + +#endif diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/main.c b/drivers/net/ethernet/yunsilicon/xsc/pci/main.c index 72eb2a37d..48c4a2a7f 100644 --- a/drivers/net/ethernet/yunsilicon/xsc/pci/main.c +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/main.c @@ -10,6 +10,7 @@ #include "cq.h" #include "eq.h" #include "pci_irq.h" +#include "adev.h" static const struct pci_device_id xsc_pci_id_table[] = { { PCI_DEVICE(XSC_PCI_VENDOR_ID, XSC_MC_PF_DEV_ID) }, @@ -260,10 +261,18 @@ static int xsc_load(struct xsc_core_device *xdev) goto err_hw_cleanup; } + err = xsc_adev_init(xdev); + if (err) { + pci_err(xdev->pdev, "xsc_adev_init failed %d\n", err); + goto err_irq_eq_destroy; + } + set_bit(XSC_INTERFACE_STATE_UP, &xdev->intf_state); mutex_unlock(&xdev->intf_state_mutex); return 0; +err_irq_eq_destroy: + xsc_irq_eq_destroy(xdev); err_hw_cleanup: xsc_hw_cleanup(xdev); out: @@ -273,6 +282,7 @@ static int xsc_load(struct xsc_core_device *xdev) static int xsc_unload(struct xsc_core_device *xdev) { + xsc_adev_uninit(xdev); mutex_lock(&xdev->intf_state_mutex); if (!test_bit(XSC_INTERFACE_STATE_UP, &xdev->intf_state)) { xsc_hw_cleanup(xdev);