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

[Dan Carpenter]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Dan Carpenter]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Pavel Machek]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[atull]

On Thu, 24 Sep 2015, Dan Carpenter wrote:

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

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.

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

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

[atull]

On Wed, 23 Sep 2015, Josh Cartwright wrote:

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

Good call.  Thanks for the clarification.  I'll have something more robust
in v12.

> [..]
> > > > +   dt_label = of_get_property(mgr->dev.of_node, "label", NULL);
> > > > +   if (dt_label)
> > > > +   ret = dev_set_name(>dev, "%s", dt_label);
> > > > +   else
> > > > +   ret = dev_set_name(>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.

OK yes, will use that in the next version.

Thanks for the review,
Alan

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

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

[Dan Carpenter]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Dan Carpenter]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[atull]

On Wed, 23 Sep 2015, Josh Cartwright wrote:

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

Good call.  Thanks for the clarification.  I'll have something more robust
in v12.

> [..]
> > > > +   dt_label = of_get_property(mgr->dev.of_node, "label", NULL);
> > > > +   if (dt_label)
> > > > +   ret = dev_set_name(>dev, "%s", dt_label);
> > > > +   else
> > > > +   ret = dev_set_name(>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.

OK yes, will use that in the next version.

Thanks for the review,
Alan

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

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

[Pavel Machek]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[atull]

On Thu, 24 Sep 2015, Dan Carpenter wrote:

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

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.

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

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

[Josh Cartwright]

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(>dev, "%s", dt_label);
> > > + else
> > > + ret = dev_set_name(>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


signature.asc
Description: PGP signature

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

[atull]

Hi Josh,

Thanks for the review.  This is all at the tail end of a long
(>2 years) discussion on this.  I hope that the way this has
shaped out still meets the needs of the people who have been
in this discussion the most and have had the strongest feelings
(due to being current users of FPGAs under Linux).

On Tue, 22 Sep 2015, Josh Cartwright wrote:

> > 
> > The following sysfs files are created:
> > * /sys/class/fpga_manager//name
> >   Name of low level driver.
> 
> Don't 'struct device's already have a name?  Why do you need another name
> attribute?

It's a nicety but not a necessity.  And it's not the only kernel framework
that has human readable names like that. Look at i2c for one example.
Nobody's complained about this one before but I guess it's not
absolutely needed.

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

Nope, not for debugging.

This feature was requested two years ago by folks who have userspace
bringing up a complicated system in FPGAs under Linux.

https://lkml.org/lkml/2013/9/18/490

There are likely to be interfaces that can be triggered by userspace. Please
look at my last patchset for one example: using device tree overlays.
That can be triggered from userspace.  You could have a layered system
where userspace loads one DT overlay (which triggers FPGA programming
and drivers probing), checks for success, then loads the next
DT overlay to program the next FPGA in the system.  In this case
the userspace interface is the configfs interface for DT overlays
so I would not be able to add FPGA manager status to that interface.
The only status I would have would be in the configfs, indicating
whether the overlay got applied successfully or not.

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

Exposing the low level driver's state is not exactly the intent here.
Above I commented further on the intent of exposing state.
I want to expose how far the FPGA manager core's state machine gets
in the process of programming.  So if we can't get the FPGA into
a state ready for receiving data, it will be "write init error", for
instance.  If we can't load the specific firmware file we wanted,
it's "firmware request error." 

> 
> > +
> > +static DEVICE_ATTR_RO(name);
> > +static DEVICE_ATTR_RO(state);
> > +
> > +static struct attribute *fpga_mgr_attrs[] = {
> > +   _attr_name.attr,
> > +   _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.
> > 

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

[atull]

On Wed, 23 Sep 2015, Dan Carpenter wrote:

> 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 = >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
> 
> 

That's correct, it's not dereferenced.

Is there some community agreement on whether we want to check a pointer
that has been passed for NULL or not?  This is C code after all.  Checking
a passed pointer for NULL is a very common reason to return -ENODEV.

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

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

[Dan Carpenter]

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 = >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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Pavel Machek]

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 = >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 = >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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Pavel Machek]

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 = >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 = >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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Dan Carpenter]

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 = >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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[atull]

Hi Josh,

Thanks for the review.  This is all at the tail end of a long
(>2 years) discussion on this.  I hope that the way this has
shaped out still meets the needs of the people who have been
in this discussion the most and have had the strongest feelings
(due to being current users of FPGAs under Linux).

On Tue, 22 Sep 2015, Josh Cartwright wrote:

> > 
> > The following sysfs files are created:
> > * /sys/class/fpga_manager//name
> >   Name of low level driver.
> 
> Don't 'struct device's already have a name?  Why do you need another name
> attribute?

It's a nicety but not a necessity.  And it's not the only kernel framework
that has human readable names like that. Look at i2c for one example.
Nobody's complained about this one before but I guess it's not
absolutely needed.

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

Nope, not for debugging.

This feature was requested two years ago by folks who have userspace
bringing up a complicated system in FPGAs under Linux.

https://lkml.org/lkml/2013/9/18/490

There are likely to be interfaces that can be triggered by userspace. Please
look at my last patchset for one example: using device tree overlays.
That can be triggered from userspace.  You could have a layered system
where userspace loads one DT overlay (which triggers FPGA programming
and drivers probing), checks for success, then loads the next
DT overlay to program the next FPGA in the system.  In this case
the userspace interface is the configfs interface for DT overlays
so I would not be able to add FPGA manager status to that interface.
The only status I would have would be in the configfs, indicating
whether the overlay got applied successfully or not.

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

Exposing the low level driver's state is not exactly the intent here.
Above I commented further on the intent of exposing state.
I want to expose how far the FPGA manager core's state machine gets
in the process of programming.  So if we can't get the FPGA into
a state ready for receiving data, it will be "write init error", for
instance.  If we can't load the specific firmware file we wanted,
it's "firmware request error." 

> 
> > +
> > +static DEVICE_ATTR_RO(name);
> > +static DEVICE_ATTR_RO(state);
> > +
> > +static struct attribute *fpga_mgr_attrs[] = {
> > +   _attr_name.attr,
> > +   _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.
> > 

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

[atull]

On Wed, 23 Sep 2015, Dan Carpenter wrote:

> 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 = >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
> 
> 

That's correct, it's not dereferenced.

Is there some community agreement on whether we want to check a pointer
that has been passed for NULL or not?  This is C code after all.  Checking
a passed pointer for NULL is a very common reason to return -ENODEV.

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

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

[Josh Cartwright]

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(>dev, "%s", dt_label);
> > > + else
> > > + ret = dev_set_name(>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


signature.asc
Description: PGP signature

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

[Josh Cartwright]

On Tue, Sep 22, 2015 at 10:21:10AM -0500, at...@opensource.altera.com wrote:
> From: Alan Tull 
> 
> 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//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//state
>   State of fpga manager
> 
> Signed-off-by: Alan Tull 
> Acked-by: Michal Simek 
[..]
> +++ 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 = >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 = >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(, 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[] = {
> + _attr_name.attr,
> + _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 

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

[Josh Cartwright]

On Tue, Sep 22, 2015 at 10:21:10AM -0500, at...@opensource.altera.com wrote:
> From: Alan Tull 
> 
> 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//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//state
>   State of fpga manager
> 
> Signed-off-by: Alan Tull 
> Acked-by: Michal Simek 
[..]
> +++ 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 = >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 = >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(, 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[] = {
> + _attr_name.attr,
> + _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