| Message ID | 20230406234143.11318-13-shannon.nelson@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | pds_core driver | expand |
On Thu, Apr 06, 2023 at 04:41:41PM -0700, Shannon Nelson wrote: > Add the client API operations for running adminq commands. > The core registers the client with the FW, then the client > has a context for requesting adminq services. We expect > to add additional operations for other clients, including > requesting additional private adminqs and IRQs, but don't have > the need yet. > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > --- > drivers/net/ethernet/amd/pds_core/auxbus.c | 135 ++++++++++++++++++++- > include/linux/pds/pds_auxbus.h | 28 +++++ > 2 files changed, 160 insertions(+), 3 deletions(-) <...> > +static struct pds_core_ops pds_core_ops = { > + .adminq_cmd = pds_client_adminq_cmd, > +}; > + <...> > diff --git a/include/linux/pds/pds_auxbus.h b/include/linux/pds/pds_auxbus.h > index aa0192af4a29..f98efd578e1c 100644 > --- a/include/linux/pds/pds_auxbus.h > +++ b/include/linux/pds/pds_auxbus.h > @@ -10,7 +10,35 @@ struct pds_auxiliary_dev { > struct auxiliary_device aux_dev; > struct pci_dev *vf_pdev; > struct pci_dev *pf_pdev; > + struct pds_core_ops *ops; I honestly don't understand why pds_core functionality is espoused through ops callbacks on auxdevice. IMHO, they shouldn't be callbacks and that functionality shouldn't operate on auxdevice. Thanks > u16 client_id; > void *priv; > }; > + > +/* > + * ptrs to functions to be used by the client for core services > + */ > +struct pds_core_ops { > + /* .adminq_cmd() - process an adminq request for the client > + * padev: ptr to the client device > + * req: ptr to buffer with request > + * req_len: length of actual struct used for request > + * resp: ptr to buffer where answer is to be copied > + * flags: optional flags defined by enum pds_core_adminq_flags > + * and used for more flexible adminq behvior > + * > + * returns 0 on success, or > + * negative for error > + * Client sends pointers to request and response buffers > + * Core copies request data into pds_core_client_request_cmd > + * Core sets other fields as needed > + * Core posts to AdminQ > + * Core copies completion data into response buffer > + */ > + int (*adminq_cmd)(struct pds_auxiliary_dev *padev, > + union pds_core_adminq_cmd *req, > + size_t req_len, > + union pds_core_adminq_comp *resp, > + u64 flags); > +}; > #endif /* _PDSC_AUXBUS_H_ */ > -- > 2.17.1 >
On 4/9/23 10:07 AM, Leon Romanovsky wrote: > > On Thu, Apr 06, 2023 at 04:41:41PM -0700, Shannon Nelson wrote: >> Add the client API operations for running adminq commands. >> The core registers the client with the FW, then the client >> has a context for requesting adminq services. We expect >> to add additional operations for other clients, including >> requesting additional private adminqs and IRQs, but don't have >> the need yet. >> >> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >> --- >> drivers/net/ethernet/amd/pds_core/auxbus.c | 135 ++++++++++++++++++++- >> include/linux/pds/pds_auxbus.h | 28 +++++ >> 2 files changed, 160 insertions(+), 3 deletions(-) > > <...> > >> +static struct pds_core_ops pds_core_ops = { >> + .adminq_cmd = pds_client_adminq_cmd, >> +}; >> + > > <...> > >> diff --git a/include/linux/pds/pds_auxbus.h b/include/linux/pds/pds_auxbus.h >> index aa0192af4a29..f98efd578e1c 100644 >> --- a/include/linux/pds/pds_auxbus.h >> +++ b/include/linux/pds/pds_auxbus.h >> @@ -10,7 +10,35 @@ struct pds_auxiliary_dev { >> struct auxiliary_device aux_dev; >> struct pci_dev *vf_pdev; >> struct pci_dev *pf_pdev; >> + struct pds_core_ops *ops; > > I honestly don't understand why pds_core functionality is espoused > through ops callbacks on auxdevice. IMHO, they shouldn't be callbacks > and that functionality shouldn't operate on auxdevice. The original design had several more operations and wrapped all the interaction into a single defined interface rather that polluting the kernel with additional direct EXPORTed functions from the PF. Since much has disappeared as we simplified the interface and don't yet have use for some of them, this ops struct with its single entry is the last vestige of that idea. Perhaps it is time to put it away and add one more EXPORTed function. We can revisit this idea if/when the interface grows again. > > Thanks > >> u16 client_id; >> void *priv; >> }; >> + >> +/* >> + * ptrs to functions to be used by the client for core services >> + */ >> +struct pds_core_ops { >> + /* .adminq_cmd() - process an adminq request for the client >> + * padev: ptr to the client device >> + * req: ptr to buffer with request >> + * req_len: length of actual struct used for request >> + * resp: ptr to buffer where answer is to be copied >> + * flags: optional flags defined by enum pds_core_adminq_flags >> + * and used for more flexible adminq behvior >> + * >> + * returns 0 on success, or >> + * negative for error >> + * Client sends pointers to request and response buffers >> + * Core copies request data into pds_core_client_request_cmd >> + * Core sets other fields as needed >> + * Core posts to AdminQ >> + * Core copies completion data into response buffer >> + */ >> + int (*adminq_cmd)(struct pds_auxiliary_dev *padev, >> + union pds_core_adminq_cmd *req, >> + size_t req_len, >> + union pds_core_adminq_comp *resp, >> + u64 flags); >> +}; >> #endif /* _PDSC_AUXBUS_H_ */ >> -- >> 2.17.1 >>
On Mon, Apr 10, 2023 at 01:50:52PM -0700, Shannon Nelson wrote: > On 4/9/23 10:07 AM, Leon Romanovsky wrote: > > > > On Thu, Apr 06, 2023 at 04:41:41PM -0700, Shannon Nelson wrote: > > > Add the client API operations for running adminq commands. > > > The core registers the client with the FW, then the client > > > has a context for requesting adminq services. We expect > > > to add additional operations for other clients, including > > > requesting additional private adminqs and IRQs, but don't have > > > the need yet. > > > > > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > > > --- > > > drivers/net/ethernet/amd/pds_core/auxbus.c | 135 ++++++++++++++++++++- > > > include/linux/pds/pds_auxbus.h | 28 +++++ > > > 2 files changed, 160 insertions(+), 3 deletions(-) <...> > > > > > diff --git a/include/linux/pds/pds_auxbus.h b/include/linux/pds/pds_auxbus.h > > > index aa0192af4a29..f98efd578e1c 100644 > > > --- a/include/linux/pds/pds_auxbus.h > > > +++ b/include/linux/pds/pds_auxbus.h > > > @@ -10,7 +10,35 @@ struct pds_auxiliary_dev { > > > struct auxiliary_device aux_dev; > > > struct pci_dev *vf_pdev; > > > struct pci_dev *pf_pdev; > > > + struct pds_core_ops *ops; > > > > I honestly don't understand why pds_core functionality is espoused > > through ops callbacks on auxdevice. IMHO, they shouldn't be callbacks > > and that functionality shouldn't operate on auxdevice. > > The original design had several more operations and wrapped all the > interaction into a single defined interface rather that polluting the kernel > with additional direct EXPORTed functions from the PF. Since much has > disappeared as we simplified the interface and don't yet have use for some > of them, this ops struct with its single entry is the last vestige of that > idea. > > Perhaps it is time to put it away and add one more EXPORTed function. We can > revisit this idea if/when the interface grows again. Thanks
diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c index 6757a5174eb7..c8c5cc5c9ca6 100644 --- a/drivers/net/ethernet/amd/pds_core/auxbus.c +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c @@ -6,6 +6,115 @@ #include "core.h" #include <linux/pds/pds_auxbus.h> +static int pds_client_register(struct pdsc *vf, struct pdsc *pf, char *name) +{ + union pds_core_adminq_comp comp = { 0 }; + union pds_core_adminq_cmd cmd = { 0 }; + int err; + u16 ci; + + if (pf->state) + return -ENXIO; + + cmd.client_reg.opcode = PDS_AQ_CMD_CLIENT_REG; + snprintf(cmd.client_reg.devname, sizeof(cmd.client_reg.devname), + "%s.%s.%d", PDS_CORE_DRV_NAME, name, vf->uid); + + err = pdsc_adminq_post(pf, &cmd, &comp, false); + if (err) { + dev_info(pf->dev, "register dev_name %s with DSC failed, status %d: %pe\n", + name, comp.status, ERR_PTR(err)); + return err; + } + + ci = le16_to_cpu(comp.client_reg.client_id); + if (!ci) { + dev_err(pf->dev, "%s: device returned null client_id\n", + __func__); + return -EIO; + } + + dev_dbg(pf->dev, "%s: device returned client_id %d for %s\n", + __func__, ci, cmd.client_reg.devname); + + return ci; +} + +static int pds_client_unregister(struct pdsc *pf, u16 client_id) +{ + union pds_core_adminq_comp comp = { 0 }; + union pds_core_adminq_cmd cmd = { 0 }; + int err; + + if (pf->state) + return -ENXIO; + + cmd.client_unreg.opcode = PDS_AQ_CMD_CLIENT_UNREG; + cmd.client_unreg.client_id = cpu_to_le16(client_id); + + err = pdsc_adminq_post(pf, &cmd, &comp, false); + if (err) + dev_info(pf->dev, "unregister client_id %d failed, status %d: %pe\n", + client_id, comp.status, ERR_PTR(err)); + + return err; +} + +/** + * pds_client_adminq_cmd - Process an adminq request for the client + * @padev: ptr to the client device + * @req: ptr to buffer with request + * @req_len: length of actual struct used for request + * @resp: ptr to buffer where answer is to be copied + * @flags: optional flags from pds_core_adminq_flags + * + * Return: 0 on success, or + * negative for error + * + * Client sends pointers to request and response buffers + * Core copies request data into pds_core_client_request_cmd + * Core sets other fields as needed + * Core posts to AdminQ + * Core copies completion data into response buffer + */ +static int pds_client_adminq_cmd(struct pds_auxiliary_dev *padev, + union pds_core_adminq_cmd *req, + size_t req_len, + union pds_core_adminq_comp *resp, + u64 flags) +{ + union pds_core_adminq_cmd cmd = { 0 }; + struct pdsc *pf; + size_t cp_len; + int err; + + pf = pci_get_drvdata(padev->pf_pdev); + + dev_dbg(pf->dev, "%s: %s opcode %d\n", + __func__, dev_name(&padev->aux_dev.dev), req->opcode); + + if (pf->state) + return -ENXIO; + + /* Wrap the client's request */ + cmd.client_request.opcode = PDS_AQ_CMD_CLIENT_CMD; + cmd.client_request.client_id = cpu_to_le16(padev->client_id); + cp_len = min_t(size_t, req_len, sizeof(cmd.client_request.client_cmd)); + memcpy(cmd.client_request.client_cmd, req, cp_len); + + err = pdsc_adminq_post(pf, &cmd, resp, + !!(flags & PDS_AQ_FLAG_FASTPOLL)); + if (err && err != -EAGAIN) + dev_info(pf->dev, "client admin cmd failed: %pe\n", + ERR_PTR(err)); + + return err; +} + +static struct pds_core_ops pds_core_ops = { + .adminq_cmd = pds_client_adminq_cmd, +}; + static void pdsc_auxbus_dev_release(struct device *dev) { struct pds_auxiliary_dev *padev = @@ -16,7 +125,9 @@ static void pdsc_auxbus_dev_release(struct device *dev) static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *vf, struct pdsc *pf, - char *name) + u16 client_id, + char *name, + struct pds_core_ops *ops) { struct auxiliary_device *aux_dev; struct pds_auxiliary_dev *padev; @@ -28,6 +139,8 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *vf, padev->vf_pdev = vf->pdev; padev->pf_pdev = pf->pdev; + padev->ops = ops; + padev->client_id = client_id; aux_dev = &padev->aux_dev; aux_dev->name = name; @@ -67,8 +180,10 @@ int pdsc_auxbus_dev_del_vf(struct pdsc *vf, struct pdsc *pf) padev = pf->vfs[vf->vf_id].padev; if (padev) { + pds_client_unregister(pf, padev->client_id); auxiliary_device_delete(&padev->aux_dev); auxiliary_device_uninit(&padev->aux_dev); + padev->client_id = 0; } pf->vfs[vf->vf_id].padev = NULL; @@ -80,6 +195,7 @@ int pdsc_auxbus_dev_add_vf(struct pdsc *vf, struct pdsc *pf) { struct pds_auxiliary_dev *padev; enum pds_core_vif_types vt; + int client_id; int err = 0; mutex_lock(&pf->config_lock); @@ -94,9 +210,22 @@ int pdsc_auxbus_dev_add_vf(struct pdsc *vf, struct pdsc *pf) pf->viftype_status[vt].enabled)) continue; - padev = pdsc_auxbus_dev_register(vf, pf, - pf->viftype_status[vt].name); + /* need to register with FW and get the client_id before + * creating the aux device so that the aux client can run + * adminq commands as part its probe + */ + client_id = pds_client_register(vf, pf, + pf->viftype_status[vt].name); + if (client_id < 0) { + err = client_id; + goto out_unlock; + } + + padev = pdsc_auxbus_dev_register(vf, pf, client_id, + pf->viftype_status[vt].name, + &pds_core_ops); if (IS_ERR(padev)) { + pds_client_unregister(pf, client_id); err = PTR_ERR(padev); goto out_unlock; } diff --git a/include/linux/pds/pds_auxbus.h b/include/linux/pds/pds_auxbus.h index aa0192af4a29..f98efd578e1c 100644 --- a/include/linux/pds/pds_auxbus.h +++ b/include/linux/pds/pds_auxbus.h @@ -10,7 +10,35 @@ struct pds_auxiliary_dev { struct auxiliary_device aux_dev; struct pci_dev *vf_pdev; struct pci_dev *pf_pdev; + struct pds_core_ops *ops; u16 client_id; void *priv; }; + +/* + * ptrs to functions to be used by the client for core services + */ +struct pds_core_ops { + /* .adminq_cmd() - process an adminq request for the client + * padev: ptr to the client device + * req: ptr to buffer with request + * req_len: length of actual struct used for request + * resp: ptr to buffer where answer is to be copied + * flags: optional flags defined by enum pds_core_adminq_flags + * and used for more flexible adminq behvior + * + * returns 0 on success, or + * negative for error + * Client sends pointers to request and response buffers + * Core copies request data into pds_core_client_request_cmd + * Core sets other fields as needed + * Core posts to AdminQ + * Core copies completion data into response buffer + */ + int (*adminq_cmd)(struct pds_auxiliary_dev *padev, + union pds_core_adminq_cmd *req, + size_t req_len, + union pds_core_adminq_comp *resp, + u64 flags); +}; #endif /* _PDSC_AUXBUS_H_ */
Add the client API operations for running adminq commands. The core registers the client with the FW, then the client has a context for requesting adminq services. We expect to add additional operations for other clients, including requesting additional private adminqs and IRQs, but don't have the need yet. Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> --- drivers/net/ethernet/amd/pds_core/auxbus.c | 135 ++++++++++++++++++++- include/linux/pds/pds_auxbus.h | 28 +++++ 2 files changed, 160 insertions(+), 3 deletions(-)