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 

[PATCH v11 3/4] add FPGA manager core

[atull]

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.

* /sys/class/fpga_manager//state
  State of fpga manager

Signed-off-by: Alan Tull 
Acked-by: Michal Simek 
---
v2: s/mangager/manager/
check for invalid request_nr
add fpga reset interface
rework the state code
remove FPGA_MGR_FAIL flag
add _ERR states to fpga manager states enum
low level state op now returns a state enum value
initialize framework state from driver state op
remove unused fpga read stuff
merge sysfs.c into fpga-mgr.c
move suspend/resume from bus.c to fpga-mgr.c

v3: Add struct device to fpga_manager (not as a pointer)
Add to_fpga_manager
Don't get irq in fpga-mgr.c (let low level driver do it)
remove from struct fpga_manager: nr, np, parent
get rid of fpga_mgr_get_new_minor()
simplify fpga_manager_register:
  reorder parameters
  use dev instead of pdev
get rid of code that used to make more sense when this
  was a char driver, don't alloc_chrdev_region
use a mutex instead of flags

v4: Move to drivers/staging

v5: Make some things be static
Kconfig: add 'if FPGA'
Makefile: s/fpga-mgr-core.o/fpga-mgr.o/
clean up includes
use enum fpga_mgr_states instead of int
static const char *state_str
use DEVICE_ATTR_RO/RW/WO and ATTRIBUTE_GROUPS
return -EINVAL instead of BUG_ON
move ida_simple_get after kzalloc
clean up fpga_mgr_remove
fpga-mgr.h: remove '#if IS_ENABLED(CONFIG_FPGA)'
add kernel-doc documentation
Move header to new include/linux/fpga folder
static const struct fpga_mgr_ops
dev_info msg simplified

v6: no statically allocated string for image_name
kernel doc fixes
changes regarding enabling SYSFS for fpga mgr
Makefile cleanup

v7: no change in this patch for v7 of the patchset

v8: no change in this patch for v8 of the patchset

v9: remove writable attributes 'firmware' and 'reset'
remove suspend/resume support for now
remove list of fpga managers; use class device iterators instead
simplify locking by giving out only one ref exclusively
add device tree support
add flags
par down API into fewer functions
update copyright year
rename some functions for clarity
clean up unnecessary #includes
add some error messages
write_init may need to look at buffer header, so add to params

v10: Make this a tristate in Kconfig
 pass flags parameter to write_complete
 use BIT(0) in macro
 move to drivers/fpga
 fpga_manager_get/put call module_try_get/module_put

v11: Set state to 'operating' after successfully programming the FPGA
---
 drivers/Kconfig   |2 +
 drivers/Makefile  |1 +
 drivers/fpga/Kconfig  |   14 ++
 drivers/fpga/Makefile |8 +
 drivers/fpga/fpga-mgr.c   |  382 +
 include/linux/fpga/fpga-mgr.h |  127 ++
 6 files changed, 534 insertions(+)
 create mode 100644 drivers/fpga/Kconfig
 create mode 100644 drivers/fpga/Makefile
 create mode 100644 drivers/fpga/fpga-mgr.c
 create mode 100644 include/linux/fpga/fpga-mgr.h

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 6e973b8..2683346 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -184,4 +184,6 @@ source "drivers/android/Kconfig"
 
 source "drivers/nvdimm/Kconfig"
 
+source "drivers/fpga/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index b64b49f..832a6e0 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -165,3 +165,4 @@ obj-$(CONFIG_RAS)   += ras/
 obj-$(CONFIG_THUNDERBOLT)  += thunderbolt/
 obj-$(CONFIG_CORESIGHT)+= hwtracing/coresight/
 obj-$(CONFIG_ANDROID)  += android/
+obj-$(CONFIG_FPGA) += fpga/
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
new file mode 100644
index 000..f1f1f6d
--- /dev/null
+++ b/drivers/fpga/Kconfig
@@ -0,0 +1,14 @@
+#
+# FPGA framework configuration
+#
+
+menu "FPGA Configuration Support"
+
+config FPGA
+   tristate "FPGA Configuration Framework"
+   help
+ Say Y here if you want support for configuring FPGAs from the
+ kernel.  The FPGA framework adds a FPGA manager class and FPGA
+ manager drivers.
+
+endmenu
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
new file mode 100644
index 000..3313c52
--- /dev/null
+++ b/drivers/fpga/Makefile
@@ -0,0 +1,8 @@
+#
+# Makefile for the fpga 

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 

[PATCH v11 3/4] add FPGA manager core

[atull]

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.

* /sys/class/fpga_manager//state
  State of fpga manager

Signed-off-by: Alan Tull 
Acked-by: Michal Simek 
---
v2: s/mangager/manager/
check for invalid request_nr
add fpga reset interface
rework the state code
remove FPGA_MGR_FAIL flag
add _ERR states to fpga manager states enum
low level state op now returns a state enum value
initialize framework state from driver state op
remove unused fpga read stuff
merge sysfs.c into fpga-mgr.c
move suspend/resume from bus.c to fpga-mgr.c

v3: Add struct device to fpga_manager (not as a pointer)
Add to_fpga_manager
Don't get irq in fpga-mgr.c (let low level driver do it)
remove from struct fpga_manager: nr, np, parent
get rid of fpga_mgr_get_new_minor()
simplify fpga_manager_register:
  reorder parameters
  use dev instead of pdev
get rid of code that used to make more sense when this
  was a char driver, don't alloc_chrdev_region
use a mutex instead of flags

v4: Move to drivers/staging

v5: Make some things be static
Kconfig: add 'if FPGA'
Makefile: s/fpga-mgr-core.o/fpga-mgr.o/
clean up includes
use enum fpga_mgr_states instead of int
static const char *state_str
use DEVICE_ATTR_RO/RW/WO and ATTRIBUTE_GROUPS
return -EINVAL instead of BUG_ON
move ida_simple_get after kzalloc
clean up fpga_mgr_remove
fpga-mgr.h: remove '#if IS_ENABLED(CONFIG_FPGA)'
add kernel-doc documentation
Move header to new include/linux/fpga folder
static const struct fpga_mgr_ops
dev_info msg simplified

v6: no statically allocated string for image_name
kernel doc fixes
changes regarding enabling SYSFS for fpga mgr
Makefile cleanup

v7: no change in this patch for v7 of the patchset

v8: no change in this patch for v8 of the patchset

v9: remove writable attributes 'firmware' and 'reset'
remove suspend/resume support for now
remove list of fpga managers; use class device iterators instead
simplify locking by giving out only one ref exclusively
add device tree support
add flags
par down API into fewer functions
update copyright year
rename some functions for clarity
clean up unnecessary #includes
add some error messages
write_init may need to look at buffer header, so add to params

v10: Make this a tristate in Kconfig
 pass flags parameter to write_complete
 use BIT(0) in macro
 move to drivers/fpga
 fpga_manager_get/put call module_try_get/module_put

v11: Set state to 'operating' after successfully programming the FPGA
---
 drivers/Kconfig   |2 +
 drivers/Makefile  |1 +
 drivers/fpga/Kconfig  |   14 ++
 drivers/fpga/Makefile |8 +
 drivers/fpga/fpga-mgr.c   |  382 +
 include/linux/fpga/fpga-mgr.h |  127 ++
 6 files changed, 534 insertions(+)
 create mode 100644 drivers/fpga/Kconfig
 create mode 100644 drivers/fpga/Makefile
 create mode 100644 drivers/fpga/fpga-mgr.c
 create mode 100644 include/linux/fpga/fpga-mgr.h

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 6e973b8..2683346 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -184,4 +184,6 @@ source "drivers/android/Kconfig"
 
 source "drivers/nvdimm/Kconfig"
 
+source "drivers/fpga/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index b64b49f..832a6e0 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -165,3 +165,4 @@ obj-$(CONFIG_RAS)   += ras/
 obj-$(CONFIG_THUNDERBOLT)  += thunderbolt/
 obj-$(CONFIG_CORESIGHT)+= hwtracing/coresight/
 obj-$(CONFIG_ANDROID)  += android/
+obj-$(CONFIG_FPGA) += fpga/
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
new file mode 100644
index 000..f1f1f6d
--- /dev/null
+++ b/drivers/fpga/Kconfig
@@ -0,0 +1,14 @@
+#
+# FPGA framework configuration
+#
+
+menu "FPGA Configuration Support"
+
+config FPGA
+   tristate "FPGA Configuration Framework"
+   help
+ Say Y here if you want support for configuring FPGAs from the
+ kernel.  The FPGA framework adds a FPGA manager class and FPGA
+ manager drivers.
+
+endmenu
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
new file mode 100644
index 000..3313c52
---