Message-ID: <CACGkMEuytKwDp3GLcaQmU1CtWSmb2RZRaGdgFyXoCqveruJBpA@mail.gmail.com>
Date: Mon, 15 May 2023 13:02:03 +0800
From: Jason Wang <jasowang@...hat.com>
To: Shannon Nelson <shannon.nelson@....com>
Cc: mst@...hat.com, virtualization@...ts.linux-foundation.org, 
	brett.creeley@....com, netdev@...r.kernel.org, simon.horman@...igine.com, 
	drivers@...sando.io
Subject: Re: [PATCH v5 virtio 10/11] pds_vdpa: subscribe to the pds_core events

On Thu, May 4, 2023 at 2:13 AM Shannon Nelson <shannon.nelson@....com> wrote:
>
> Register for the pds_core's notification events, primarily to
> find out when the FW has been reset so we can pass this on
> back up the chain.
>
> Signed-off-by: Shannon Nelson <shannon.nelson@....com>
> ---
>  drivers/vdpa/pds/vdpa_dev.c | 68 ++++++++++++++++++++++++++++++++++++-
>  drivers/vdpa/pds/vdpa_dev.h |  1 +
>  2 files changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/pds/vdpa_dev.c b/drivers/vdpa/pds/vdpa_dev.c
> index 9970657cdb3d..377eefc2fa1e 100644
> --- a/drivers/vdpa/pds/vdpa_dev.c
> +++ b/drivers/vdpa/pds/vdpa_dev.c
> @@ -21,6 +21,61 @@ static struct pds_vdpa_device *vdpa_to_pdsv(struct vdpa_device *vdpa_dev)
>         return container_of(vdpa_dev, struct pds_vdpa_device, vdpa_dev);
>  }
>
> +static int pds_vdpa_notify_handler(struct notifier_block *nb,
> +                                  unsigned long ecode,
> +                                  void *data)
> +{
> +       struct pds_vdpa_device *pdsv = container_of(nb, struct pds_vdpa_device, nb);
> +       struct device *dev = &pdsv->vdpa_aux->padev->aux_dev.dev;
> +
> +       dev_dbg(dev, "%s: event code %lu\n", __func__, ecode);
> +
> +       /* Give the upper layers a hint that something interesting
> +        * may have happened.  It seems that the only thing this
> +        * triggers in the virtio-net drivers above us is a check
> +        * of link status.
> +        *
> +        * We don't set the NEEDS_RESET flag for EVENT_RESET
> +        * because we're likely going through a recovery or
> +        * fw_update and will be back up and running soon.
> +        */
> +       if (ecode == PDS_EVENT_RESET || ecode == PDS_EVENT_LINK_CHANGE) {

The code here seems to conflict with the comment above. If we don't
set NEEDS_RESET, there's no need for the config callback?

Thanks

> +               if (pdsv->config_cb.callback)
> +                       pdsv->config_cb.callback(pdsv->config_cb.private);
> +       }
> +
> +       return 0;
> +}
> +
> +static int pds_vdpa_register_event_handler(struct pds_vdpa_device *pdsv)
> +{
> +       struct device *dev = &pdsv->vdpa_aux->padev->aux_dev.dev;
> +       struct notifier_block *nb = &pdsv->nb;
> +       int err;
> +
> +       if (!nb->notifier_call) {
> +               nb->notifier_call = pds_vdpa_notify_handler;
> +               err = pdsc_register_notify(nb);
> +               if (err) {
> +                       nb->notifier_call = NULL;
> +                       dev_err(dev, "failed to register pds event handler: %ps\n",
> +                               ERR_PTR(err));
> +                       return -EINVAL;
> +               }
> +               dev_dbg(dev, "pds event handler registered\n");
> +       }
> +
> +       return 0;
> +}
> +
> +static void pds_vdpa_unregister_event_handler(struct pds_vdpa_device *pdsv)
> +{
> +       if (pdsv->nb.notifier_call) {
> +               pdsc_unregister_notify(&pdsv->nb);
> +               pdsv->nb.notifier_call = NULL;
> +       }
> +}
> +
>  static int pds_vdpa_set_vq_address(struct vdpa_device *vdpa_dev, u16 qid,
>                                    u64 desc_addr, u64 driver_addr, u64 device_addr)
>  {
> @@ -522,6 +577,12 @@ static int pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>
>         pdsv->vdpa_dev.mdev = &vdpa_aux->vdpa_mdev;
>
> +       err = pds_vdpa_register_event_handler(pdsv);
> +       if (err) {
> +               dev_err(dev, "Failed to register for PDS events: %pe\n", ERR_PTR(err));
> +               goto err_unmap;
> +       }
> +
>         /* We use the _vdpa_register_device() call rather than the
>          * vdpa_register_device() to avoid a deadlock because our
>          * dev_add() is called with the vdpa_dev_lock already set
> @@ -530,13 +591,15 @@ static int pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>         err = _vdpa_register_device(&pdsv->vdpa_dev, pdsv->num_vqs);
>         if (err) {
>                 dev_err(dev, "Failed to register to vDPA bus: %pe\n", ERR_PTR(err));
> -               goto err_unmap;
> +               goto err_unevent;
>         }
>
>         pds_vdpa_debugfs_add_vdpadev(vdpa_aux);
>
>         return 0;
>
> +err_unevent:
> +       pds_vdpa_unregister_event_handler(pdsv);
>  err_unmap:
>         put_device(&pdsv->vdpa_dev.dev);
>         vdpa_aux->pdsv = NULL;
> @@ -546,8 +609,11 @@ static int pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>  static void pds_vdpa_dev_del(struct vdpa_mgmt_dev *mdev,
>                              struct vdpa_device *vdpa_dev)
>  {
> +       struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
>         struct pds_vdpa_aux *vdpa_aux;
>
> +       pds_vdpa_unregister_event_handler(pdsv);
> +
>         vdpa_aux = container_of(mdev, struct pds_vdpa_aux, vdpa_mdev);
>         _vdpa_unregister_device(vdpa_dev);
>
> diff --git a/drivers/vdpa/pds/vdpa_dev.h b/drivers/vdpa/pds/vdpa_dev.h
> index a21596f438c1..1650a2b08845 100644
> --- a/drivers/vdpa/pds/vdpa_dev.h
> +++ b/drivers/vdpa/pds/vdpa_dev.h
> @@ -40,6 +40,7 @@ struct pds_vdpa_device {
>         u8 vdpa_index;                  /* rsvd for future subdevice use */
>         u8 num_vqs;                     /* num vqs in use */
>         struct vdpa_callback config_cb;
> +       struct notifier_block nb;
>  };
>
>  int pds_vdpa_get_mgmt_info(struct pds_vdpa_aux *vdpa_aux);
> --
> 2.17.1
>

Powered by blists - more mailing lists