Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1231013 > unrolled thread

Re: [PATCH v11 3/4] add FPGA manager core

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v11 3/4] add FPGA manager core Josh Cartwright <joshc@ni.com> - 2015-09-23 00:40 +0200
    Re: [PATCH v11 3/4] add FPGA manager core Pavel Machek <pavel@denx.de> - 2015-09-23 15:30 +0200
      Re: [PATCH v11 3/4] add FPGA manager core Dan Carpenter <dan.carpenter@oracle.com> - 2015-09-23 16:20 +0200
        Re: [PATCH v11 3/4] add FPGA manager core Dan Carpenter <dan.carpenter@oracle.com> - 2015-09-24 10:00 +0200
          Re: [PATCH v11 3/4] add FPGA manager core Pavel Machek <pavel@denx.de> - 2015-09-24 23:20 +0200
          Re: [PATCH v11 3/4] add FPGA manager core Dan Carpenter <dan.carpenter@oracle.com> - 2015-09-25 12:10 +0200
    Re: [PATCH v11 3/4] add FPGA manager core Josh Cartwright <joshc@ni.com> - 2015-09-24 01:10 +0200

#1231013 — Re: [PATCH v11 3/4] add FPGA manager core

[Multipart message — attachments visible in raw view] — view raw

On Tue, Sep 22, 2015 at 10:21:10AM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.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@opensource.altera.com>
> Acked-by: Michal Simek <michal.simek@xilinx.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

[toc] | [next] | [standalone]

#1231434

Hi!

> > +++ 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.

Not only that, it can never happen. mgr is already dereferenced above.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231467

On Wed, Sep 23, 2015 at 03:23:54PM +0200, Pavel Machek wrote:
> > > +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.
> 
> Not only that, it can never happen. mgr is already dereferenced above.
> 

It's not dereferenced.  We're taking the address of mgr->dev but we
don't dereference mgr.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231928

Of course, the maintainer gets the last word regardless of what anyone
else thinks.

Generally, minimal code is better.  Trying to future proof code is a
waste of time because you can't predict what will happen in the future.
It's way more likely that some pointer you never expected to be NULL
will be NULL instead of the few checked at the beginning of a function.
Adding useless code uses RAM and makes the function slower.  It's a bit
confusing for users as well because they will wonder when the NULL check
is used.  A lot of times this sort of error handling is a bit fake and
what I mean is that it looks correct but the system will just crash in a
later function.

Also especially with a simple NULL dereferences like this theoretical
one, it's better to just get the oops.  It kills the module but you get
a good message in the log and it's normally straight forward to debug.

We spent a surprising amount of time discussing useless code.  I made
someone redo a patch yesterday because they had incomplete error
handling for a situation which could never happen.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232436

Hi!

> > Of course, the maintainer gets the last word regardless of what anyone
> > else thinks.
> > 
> > Generally, minimal code is better.  Trying to future proof code is a
> > waste of time because you can't predict what will happen in the future.
> > It's way more likely that some pointer you never expected to be NULL
> > will be NULL instead of the few checked at the beginning of a function.
> > Adding useless code uses RAM and makes the function slower.  It's a bit
> > confusing for users as well because they will wonder when the NULL check
> > is used.  A lot of times this sort of error handling is a bit fake and
> > what I mean is that it looks correct but the system will just crash in a
> > later function.
> > 
> > Also especially with a simple NULL dereferences like this theoretical
> > one, it's better to just get the oops.  It kills the module but you get
> > a good message in the log and it's normally straight forward to debug.
> > 
> > We spent a surprising amount of time discussing useless code.  I made
> > someone redo a patch yesterday because they had incomplete error
> > handling for a situation which could never happen.
> 
> Thanks for the discussion.
> 
> Interesting.  The amount of code bloat here compiles down to about two
> machine instructions (in two places).  Actually a little more since I should
> be using IS_ERR_OR_NULL.  But the main question is whether I should do
> it at all.
> 
> The behaviour I should drive here is that the user will do their own error
> checking.  After they get a pointer to a FPGA manager using
> of_fpga_mgr_get(), they should check it and not assume that
> fpga_mgr_firmware_load() will do it for them, i.e.
> 
> 	mgr = of_fpga_mgr_get(mgr_node);
> 	if (IS_ERR(mgr))
> 		return PTR_ERR(mgr);
> 	fpga_mgr_firmware_load(mgr, flags, path);
> 
> I could take out these NULL pointer checks and it won't hurt anything unless
> someone is just using the functions badly, in which case: kablooey.

2 instructions is not that bad, do whatever is easier for you. These
patches received enough bikeshed painting.

Best regards,
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232703

On Thu, Sep 24, 2015 at 03:47:26PM -0500, atull wrote:
> Interesting.  The amount of code bloat here compiles down to about two
> machine instructions (in two places).  Actually a little more since I should
> be using IS_ERR_OR_NULL.  But the main question is whether I should do
> it at all.
> 

They kernel already has too many bogus checks for IS_ERR().  It's a very
common bug to check for IS_ERR() when you should be checking for NULL.

-	foo = some_allocator();
+	foo = kmalloc();
	if (IS_ERR(foo))
		goto fail;

I have a static checker for "warn: 'foo' isn't an ERR_PTR" but I haven't
published it because too much code has impossible checks.

> The behaviour I should drive here is that the user will do their own error
> checking.  After they get a pointer to a FPGA manager using
> of_fpga_mgr_get(), they should check it and not assume that
> fpga_mgr_firmware_load() will do it for them, i.e.
> 
> 	mgr = of_fpga_mgr_get(mgr_node);
> 	if (IS_ERR(mgr))
> 		return PTR_ERR(mgr);
> 	fpga_mgr_firmware_load(mgr, flags, path);
> 

I don't understand completely how of_fpga_mgr_get() ever returns NULL.
A lot of the of_ functions return ERR_PTRs if OF_ is compiled in but
they return NULL if it's not.  I think this is so people can build with
COMPILE_TEST so we get more coverage with static analysis?

> I could take out these NULL pointer checks and it won't hurt anything unless
> someone is just using the functions badly, in which case: kablooey.

Linux devs are very good about doing error checking.  An early kablooey
is what we want for people who don't.

Also if you provide a sanity check then Markus Elfring will remove all
the error checking in the callers.  Don't do it.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231777

[Multipart message — attachments visible in raw view] — view raw

On Wed, Sep 23, 2015 at 12:10:13PM -0500, atull wrote:
> On Tue, 22 Sep 2015, Josh Cartwright wrote:
[..]
> > > +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?
> 
> Well... get_device and put_device are not sufficient?

Sorry if I was too opaque.

You've just put_device()'d the only reference to the device you had
here, so nothing is preventing from the device from being ripped away
while it's being used.

You should remove the put_device() call here, and add a corresponding
put_device() in fpga_mgr_put().

The module refcounting is also a bit strange, as you are
acquiring/releasing a reference to THIS_MODULE, but you also should care
about the lower-level driver's module refcount.

[..]
> > > +	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.
> 
> I could look into that.  Is there an example of that you particularly
> like for this?

Look at i2c's usage of the of_alias_*() functions.

  Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web