Message-ID: <20150922222929.GA26335@jcartwri.amer.corp.natinst.com>
Date:	Tue, 22 Sep 2015 17:29:29 -0500
From:	Josh Cartwright <joshc@...com>
To:	atull@...nsource.altera.com
Cc:	gregkh@...uxfoundation.org, jgunthorpe@...idianresearch.com,
	hpa@...or.com, monstr@...str.eu, michal.simek@...inx.com,
	rdunlap@...radead.org, Moritz Fischer <moritz.fischer@...us.com>,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	pantelis.antoniou@...sulko.com, robh+dt@...nel.org,
	grant.likely@...aro.org, iws@...o.caltech.edu,
	linux-doc@...r.kernel.org, pavel@...x.de, broonie@...nel.org,
	philip@...ister.org, rubini@...dd.com, s.trumtrar@...gutronix.de,
	jason@...edaemon.net, kyle.teske@...com, nico@...aro.org,
	balbi@...com, m.chehab@...sung.com, davidb@...eaurora.org,
	rob@...dley.net, davem@...emloft.net, cesarb@...arb.net,
	sameo@...ux.intel.com, akpm@...ux-foundation.org,
	linus.walleij@...aro.org, pawel.moll@....com, mark.rutland@....com,
	ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
	devel@...verdev.osuosl.org, Petr Cvek <petr.cvek@....cz>,
	delicious.quinoa@...il.com, dinguyen@...nsource.altera.com
Subject: Re: [PATCH v11 3/4] add FPGA manager core

On Tue, Sep 22, 2015 at 10:21:10AM -0500, atull@...nsource.altera.com wrote:
> From: Alan Tull <atull@...nsource.altera.com>
> 
> API to support programming FPGA's.
> 
> The following functions are exported as GPL:
> * fpga_mgr_buf_load
>    Load fpga from image in buffer
> 
> * fpga_mgr_firmware_load
>    Request firmware and load it to the FPGA.
> 
> * fpga_mgr_register
> * fpga_mgr_unregister
>    FPGA device drivers can be added by calling
>    fpga_mgr_register() to register a set of
>    fpga_manager_ops to do device specific stuff.
> 
> * of_fpga_mgr_get
> * fpga_mgr_put
>    Get/put a reference to a fpga manager.
> 
> The following sysfs files are created:
> * /sys/class/fpga_manager/<fpga>/name
>   Name of low level driver.

Don't 'struct device's already have a name?  Why do you need another name
attribute?

> 
> * /sys/class/fpga_manager/<fpga>/state
>   State of fpga manager
> 
> Signed-off-by: Alan Tull <atull@...nsource.altera.com>
> Acked-by: Michal Simek <michal.simek@...inx.com>
[..]
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -0,0 +1,382 @@
[..]
> +int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
> +		      size_t count)
> +{
> +	struct device *dev = &mgr->dev;
> +	int ret;
> +
> +	if (!mgr)
> +		return -ENODEV;

This seems overly defensive.

[..]
> +int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
> +			   const char *image_name)
> +{
> +	struct device *dev = &mgr->dev;
> +	const struct firmware *fw;
> +	int ret;
> +
> +	if (!mgr)
> +		return -ENODEV;

Again; I'm of the opinion this is needlessly defensive.

> +
> +	dev_info(dev, "writing %s to %s\n", image_name, mgr->name);
> +
> +	mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
> +
> +	ret = request_firmware(&fw, image_name, dev);
> +	if (ret) {
> +		mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
> +		dev_err(dev, "Error requesting firmware %s\n", image_name);
> +		return ret;
> +	}
> +
> +	ret = fpga_mgr_buf_load(mgr, flags, fw->data, fw->size);
> +	if (ret)
> +		return ret;
> +
> +	release_firmware(fw);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
> +
> +static const char * const state_str[] = {
> +	[FPGA_MGR_STATE_UNKNOWN] =		"unknown",
> +	[FPGA_MGR_STATE_POWER_OFF] =		"power off",
> +	[FPGA_MGR_STATE_POWER_UP] =		"power up",
> +	[FPGA_MGR_STATE_RESET] =		"reset",
> +
> +	/* requesting FPGA image from firmware */
> +	[FPGA_MGR_STATE_FIRMWARE_REQ] =		"firmware request",
> +	[FPGA_MGR_STATE_FIRMWARE_REQ_ERR] =	"firmware request error",
> +
> +	/* Preparing FPGA to receive image */
> +	[FPGA_MGR_STATE_WRITE_INIT] =		"write init",
> +	[FPGA_MGR_STATE_WRITE_INIT_ERR] =	"write init error",
> +
> +	/* Writing image to FPGA */
> +	[FPGA_MGR_STATE_WRITE] =		"write",
> +	[FPGA_MGR_STATE_WRITE_ERR] =		"write error",
> +
> +	/* Finishing configuration after image has been written */
> +	[FPGA_MGR_STATE_WRITE_COMPLETE] =	"write complete",
> +	[FPGA_MGR_STATE_WRITE_COMPLETE_ERR] =	"write complete error",
> +
> +	/* FPGA reports to be in normal operating mode */
> +	[FPGA_MGR_STATE_OPERATING] =		"operating",
> +};

Is it really necessary to expose the whole FPGA manager state machine to
userspace?  Is the only justification "for debugging"?

If so, that seems pretty short-sighted, as it then makes the state
machine part of the stable usermode API.  It even makes less sense given
this patchsets current state: configuration of the FPGA is not something
that userspace is directly triggering.

> +
> +static ssize_t name_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> +	return sprintf(buf, "%s\n", mgr->name);
> +}
> +
> +static ssize_t state_show(struct device *dev,
> +			  struct device_attribute *attr, char *buf)
> +{
> +	struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> +	return sprintf(buf, "%s\n", state_str[mgr->state]);
> +}

Is it possible that the state of the FPGA has changed since the last
time we've done an update?  Should the lower-level drivers' state()
callback be invoked here?

> +
> +static DEVICE_ATTR_RO(name);
> +static DEVICE_ATTR_RO(state);
> +
> +static struct attribute *fpga_mgr_attrs[] = {
> +	&dev_attr_name.attr,
> +	&dev_attr_state.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(fpga_mgr);
> +
> +static int fpga_mgr_of_node_match(struct device *dev, const void *data)
> +{
> +	return dev->of_node == data;
> +}
> +
> +/**
> + * of_fpga_mgr_get - get an exclusive reference to a fpga mgr
> + * @node:	device node
> + *
> + * Given a device node, get an exclusive reference to a fpga mgr.
> + *
> + * Return: fpga manager struct or IS_ERR() condition containing error code.
> + */
> +struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
> +{
> +	struct fpga_manager *mgr;
> +	struct device *dev;
> +
> +	if (!node)
> +		return ERR_PTR(-EINVAL);
> +
> +	dev = class_find_device(fpga_mgr_class, NULL, node,
> +				fpga_mgr_of_node_match);
> +	if (!dev)
> +		return ERR_PTR(-ENODEV);
> +
> +	mgr = to_fpga_manager(dev);
> +	put_device(dev);

Who's ensuring the FPGA manager device's lifetime between _get and _put?

> +	if (!mgr)
> +		return ERR_PTR(-ENODEV);
> +
> +	/* Get exclusive use of fpga manager */
> +	if (!mutex_trylock(&mgr->ref_mutex))
> +		return ERR_PTR(-EBUSY);
> +
> +	if (!try_module_get(THIS_MODULE)) {
> +		mutex_unlock(&mgr->ref_mutex);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	return mgr;
> +}
[..]
> +int fpga_mgr_register(struct device *dev, const char *name,
> +		      const struct fpga_manager_ops *mops,
> +		      void *priv)
> +{
> +	struct fpga_manager *mgr;
> +	const char *dt_label;
> +	int id, ret;
> +
> +	if (!mops || !mops->write_init || !mops->write ||
> +	    !mops->write_complete || !mops->state) {
> +		dev_err(dev, "Attempt to register without fpga_manager_ops\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!name || !strlen(name)) {
> +		dev_err(dev, "Attempt to register with no name!\n");
> +		return -EINVAL;
> +	}
> +
> +	mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
> +	if (!mgr)
> +		return -ENOMEM;
> +
> +	id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
> +	if (id < 0) {
> +		ret = id;
> +		goto error_kfree;
> +	}
> +
> +	mutex_init(&mgr->ref_mutex);
> +
> +	mgr->name = name;
> +	mgr->mops = mops;
> +	mgr->priv = priv;
> +
> +	/*
> +	 * Initialize framework state by requesting low level driver read state
> +	 * from device.  FPGA may be in reset mode or may have been programmed
> +	 * by bootloader or EEPROM.
> +	 */
> +	mgr->state = mgr->mops->state(mgr);
> +
> +	device_initialize(&mgr->dev);
> +	mgr->dev.class = fpga_mgr_class;
> +	mgr->dev.parent = dev;
> +	mgr->dev.of_node = dev->of_node;
> +	mgr->dev.id = id;
> +	dev_set_drvdata(dev, mgr);
> +
> +	dt_label = of_get_property(mgr->dev.of_node, "label", NULL);
> +	if (dt_label)
> +		ret = dev_set_name(&mgr->dev, "%s", dt_label);
> +	else
> +		ret = dev_set_name(&mgr->dev, "fpga%d", id);

I'm wondering if an alias {} node is better for this.

[..]
> +++ b/include/linux/fpga/fpga-mgr.h
[..]
> +/*
> + * FPGA Manager flags
> + * FPGA_MGR_PARTIAL_RECONFIG: do partial reconfiguration if supported
> + */
> +#define FPGA_MGR_PARTIAL_RECONFIG	BIT(0)
> +
> +/**
> + * struct fpga_manager_ops - ops for low level fpga manager drivers
> + * @state: returns an enum value of the FPGA's state
> + * @write_init: prepare the FPGA to receive confuration data

                                               ^configuration

> + * @write: write count bytes of configuration data to the FPGA
> + * @write_complete: set FPGA to operating state after writing is done
> + * @fpga_remove: optional: Set FPGA into a specific state during driver remove

Any specific state?  State in the FPGA-manager-state-machine case?  Or a
basic 'reset' state?

  Josh

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)

Powered by blists - more mailing lists