Date:   Wed, 18 Jul 2018 15:07:10 -0500
From:   Alan Tull <atull@...nel.org>
To:     Moritz Fischer <mdf@...nel.org>
Cc:     Alan Tull <atull@...nel.org>,
        Appana Durga Kedareswara rao <appana.durga.rao@...inx.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-fpga@...r.kernel.org
Subject: Re: [PATCH] fpga: mgr: rework fpga_mgr_get API for multiple managers

On Mon, Jul 9, 2018 at 4:39 PM, Alan Tull <atull@...nel.org> wrote:

On Mon, Jul 9, 2018 at 4:39 PM, Alan Tull <atull@...nel.org> wrote:

This patch is now outdated and would break the upstream.  I currently
doubt that this change is needed or would be helpful.  The discussion
on whether this patch is needed is on a separate thread:
https://lkml.org/lkml/2018/7/18/866

Alan

> Change fpga_mgr_get() function to take manager as the parameter
> instead of dev.  Caller probably has a pointer to manager already
> anyway, so remove code that searched for manager based on dev.  The
> rationale for this change is that cards that have more than one FPGA
> may have more than one manager.
>
> Add new __fpga_mgr_create() API which adds an 'owner' parameter.  This
> API will save owner in struct fpga_manager.
>
> Existing fpga_mgr_create() API becomes a macro that calls
> __fpga_mgr_create(), setting owner to THIS_MODULE of caller.
>
> fpga_mgr_get() will do try_module_get(mgr->owner).  For drivers that
> have one manager, this has the same effect as the code it replaces
> that did try_module_get(dev->parent->driver->owner) since the parent
> is the low level FPGA manager driver.
>
> Fixes: 9dce0287a60d ("fpga: add method to get fpga manager from device")
> Reported-by: Appana Durga Kedareswara rao <appana.durga.rao@...inx.com>
> Signed-off-by: Alan Tull <atull@...nel.org>
> ---
>  drivers/fpga/fpga-mgr.c       | 77 +++++++++++++++++++------------------------
>  include/linux/fpga/fpga-mgr.h | 15 +++++----
>  2 files changed, 43 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index c1564cf..8d0ac96 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -162,9 +162,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr,
>   *
>   * Step the low level fpga manager through the device-specific steps of getting
>   * an FPGA ready to be configured, writing the image to it, then doing whatever
> - * post-configuration steps necessary.  This code assumes the caller got the
> - * mgr pointer from of_fpga_mgr_get() or fpga_mgr_get() and checked that it is
> - * not an error code.
> + * post-configuration steps necessary.
>   *
>   * This is the preferred entry point for FPGA programming, it does not require
>   * any contiguous kernel memory.
> @@ -239,8 +237,7 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
>   *
>   * Step the low level fpga manager through the device-specific steps of getting
>   * an FPGA ready to be configured, writing the image to it, then doing whatever
> - * post-configuration steps necessary.  This code assumes the caller got the
> - * mgr pointer from of_fpga_mgr_get() and checked that it is not an error code.
> + * post-configuration steps necessary.
>   *
>   * Return: 0 on success, negative error code otherwise.
>   */
> @@ -310,9 +307,7 @@ static int fpga_mgr_buf_load(struct fpga_manager *mgr,
>   *
>   * Request an FPGA image using the firmware class, then write out to the FPGA.
>   * Update the state before each step to provide info on what step failed if
> - * there is a failure.  This code assumes the caller got the mgr pointer
> - * from of_fpga_mgr_get() or fpga_mgr_get() and checked that it is not an error
> - * code.
> + * there is a failure.
>   *
>   * Return: 0 on success, negative error code otherwise.
>   */
> @@ -347,8 +342,11 @@ static int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>   * @mgr:       fpga manager
>   * @info:      fpga image information.
>   *
> - * Load the FPGA from an image which is indicated in @info.  If successful, the
> - * FPGA ends up in operating mode.
> + * Load the FPGA from an image which is indicated in @info.  @mgr is a
> + * valid pointer to an FPGA manager whose refcount has been
> + * incremented by of_fpga_mgr_get() or fpga_mgr_get() and has been
> + * locked by fpga_mgr_lock().  If successful, the FPGA ends up in
> + * operating mode.
>   *
>   * Return: 0 on success, negative error code otherwise.
>   */
> @@ -416,41 +414,28 @@ static struct attribute *fpga_mgr_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(fpga_mgr);
>
> -static struct fpga_manager *__fpga_mgr_get(struct device *dev)
> +/* Assumes mgr->dev has refcount incremented already. */
> +static struct fpga_manager *__fpga_mgr_get(struct fpga_manager *mgr)
>  {
> -       struct fpga_manager *mgr;
> -
> -       mgr = to_fpga_manager(dev);
> -
> -       if (!try_module_get(dev->parent->driver->owner))
> -               goto err_dev;
> +       if (try_module_get(mgr->owner))
> +               return mgr;
>
> -       return mgr;
> +       put_device(&mgr->dev);
>
> -err_dev:
> -       put_device(dev);
>         return ERR_PTR(-ENODEV);
>  }
>
> -static int fpga_mgr_dev_match(struct device *dev, const void *data)
> -{
> -       return dev->parent == data;
> -}
> -
>  /**
> - * fpga_mgr_get - Given a device, get a reference to a fpga mgr.
> - * @dev:       parent device that fpga mgr was registered with
> + * fpga_mgr_get - Increment refcount for a fpga mgr.
> + * @mgr:       fpga manager
>   *
>   * Return: fpga manager struct or IS_ERR() condition containing error code.
>   */
> -struct fpga_manager *fpga_mgr_get(struct device *dev)
> +struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr)
>  {
> -       struct device *mgr_dev = class_find_device(fpga_mgr_class, NULL, dev,
> -                                                  fpga_mgr_dev_match);
> -       if (!mgr_dev)
> -               return ERR_PTR(-ENODEV);
> +       get_device(&mgr->dev);
>
> -       return __fpga_mgr_get(mgr_dev);
> +       return __fpga_mgr_get(mgr);
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_get);
>
> @@ -468,6 +453,7 @@ static int fpga_mgr_of_node_match(struct device *dev, const void *data)
>   */
>  struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
>  {
> +       struct fpga_manager *mgr;
>         struct device *dev;
>
>         dev = class_find_device(fpga_mgr_class, NULL, node,
> @@ -475,7 +461,9 @@ struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
>         if (!dev)
>                 return ERR_PTR(-ENODEV);
>
> -       return __fpga_mgr_get(dev);
> +       mgr = to_fpga_manager(dev);
> +
> +       return __fpga_mgr_get(mgr);
>  }
>  EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
>
> @@ -485,7 +473,7 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
>   */
>  void fpga_mgr_put(struct fpga_manager *mgr)
>  {
> -       module_put(mgr->dev.parent->driver->owner);
> +       module_put(mgr->owner);
>         put_device(&mgr->dev);
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_put);
> @@ -494,9 +482,8 @@ EXPORT_SYMBOL_GPL(fpga_mgr_put);
>   * fpga_mgr_lock - Lock FPGA manager for exclusive use
>   * @mgr:       fpga manager
>   *
> - * Given a pointer to FPGA Manager (from fpga_mgr_get() or
> - * of_fpga_mgr_put()) attempt to get the mutex. The user should call
> - * fpga_mgr_lock() and verify that it returns 0 before attempting to
> + * Given a pointer to FPGA Manager, attempt to get the mutex. The user should
> + * call fpga_mgr_lock() and verify that it returns 0 before attempting to
>   * program the FPGA.  Likewise, the user should call fpga_mgr_unlock
>   * when done programming the FPGA.
>   *
> @@ -524,17 +511,20 @@ void fpga_mgr_unlock(struct fpga_manager *mgr)
>  EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
>
>  /**
> - * fpga_mgr_create - create and initialize a FPGA manager struct
> + * __fpga_mgr_create - create and initialize a FPGA manager struct
>   * @dev:       fpga manager device from pdev
> + * @owner:     owner of manager, i.e. THIS_MODULE of manager driver
>   * @name:      fpga manager name
>   * @mops:      pointer to structure of fpga manager ops
>   * @priv:      fpga manager private data
>   *
>   * Return: pointer to struct fpga_manager or NULL
>   */
> -struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
> -                                    const struct fpga_manager_ops *mops,
> -                                    void *priv)
> +struct fpga_manager *__fpga_mgr_create(struct device *dev,
> +                                      struct module *owner,
> +                                      const char *name,
> +                                      const struct fpga_manager_ops *mops,
> +                                      void *priv)
>  {
>         struct fpga_manager *mgr;
>         int id, ret;
> @@ -563,6 +553,7 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>
>         mutex_init(&mgr->ref_mutex);
>
> +       mgr->owner = owner;
>         mgr->name = name;
>         mgr->mops = mops;
>         mgr->priv = priv;
> @@ -587,7 +578,7 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>
>         return NULL;
>  }
> -EXPORT_SYMBOL_GPL(fpga_mgr_create);
> +EXPORT_SYMBOL_GPL(__fpga_mgr_create);
>
>  /**
>   * fpga_mgr_free - deallocate a FPGA manager
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index eec7c24..f948202 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -127,6 +127,7 @@ struct fpga_manager_ops {
>  /**
>   * struct fpga_manager - fpga manager structure
>   * @name: name of low level fpga manager
> + * @owner: module that owns this manager
>   * @dev: fpga manager device
>   * @ref_mutex: only allows one reference to fpga manager
>   * @state: state of fpga manager
> @@ -135,6 +136,7 @@ struct fpga_manager_ops {
>   */
>  struct fpga_manager {
>         const char *name;
> +       struct module *owner;
>         struct device dev;
>         struct mutex ref_mutex;
>         enum fpga_mgr_states state;
> @@ -154,14 +156,15 @@ int fpga_mgr_lock(struct fpga_manager *mgr);
>  void fpga_mgr_unlock(struct fpga_manager *mgr);
>
>  struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
> -
> -struct fpga_manager *fpga_mgr_get(struct device *dev);
> -
> +struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>  void fpga_mgr_put(struct fpga_manager *mgr);
>
> -struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
> -                                    const struct fpga_manager_ops *mops,
> -                                    void *priv);
> +struct fpga_manager *__fpga_mgr_create(struct device *dev,
> +                                      struct module *owner, const char *name,
> +                                      const struct fpga_manager_ops *mops,
> +                                      void *priv);
> +#define fpga_mgr_create(dev, name, mops, priv) \
> +       __fpga_mgr_create((dev), THIS_MODULE, (name), (mops), (priv))
>  void fpga_mgr_free(struct fpga_manager *mgr);
>  int fpga_mgr_register(struct fpga_manager *mgr);
>  void fpga_mgr_unregister(struct fpga_manager *mgr);
> --
> 2.7.4
>

Powered by blists - more mailing lists