Hi,
first of all, I think that the driver should go through lkml before upstream

merge or at least be in -mm for a while (I think this used to be a rule some

time ago), correct me if I'm wrong, but none of it happened.
Few comments I have:

- release f_op retval is silently ignored, I guess you will get your device into

undefined state when the first function fails (esp. when you interrupt the sem)

- semaphores are deprecated

- class_device_create is deprecated

- module_init/exit functions should be __init, not __devinit/exit (not a bug,

it's subset)

- this piece:

         drvdata = kmalloc(sizeof(struct hwicap_drvdata), GFP_KERNEL);

         if (!drvdata) {

                 dev_err(dev, "Couldn't allocate device private record\n");

                 return -ENOMEM;

         }

  memset((void *)drvdata, 0, sizeof(struct hwicap_drvdata));
  kmalloc + memset = kzalloc

  null probed_devices[id] on that fail path and on failed1 label
- from/to (void *) casts are useless

- io resources are at least ulong

- don't understand this:

                 memcpy(kbuf, drvdata->read_buffer, bytes_remaining);

                 drvdata->read_buffer_in_use = bytes_remaining;

                 free_page((unsigned long)kbuf);

- can this overlap (=>memmove)?

                 memcpy(drvdata->read_buffer + bytes_to_read,

                                 drvdata->read_buffer, 4 - bytes_to_read);

- is platform probing function race-proof (like pci)?

- run sparse on it, you mix __user with non-__user at least

--

the sem)
bytes_to_read);

Yes, it can!  fixed.
Given the length of time that these errors have been there, I'm really

Fixed.  There is one warning (related to a function that is not called),

but I'd like to leave this in, as it should get bound to an ioctl, most

likely.
Thanks alot for the comments!
Steve
--

This includes code for new fifo-based xps_hwicap in addition to the

older opb_hwicap, which has a significantly different interface.  The

common code between the two drivers is largely shared.
Significant differences exists between this driver and what is

supported in the EDK drivers.  In particular, most of the

architecture-specific code for reconfiguring individual FPGA resources

has been removed.  This functionality is likely better provided in a

user-space support library.  In addition, read and write access is

supported.  In addition, although the xps_hwicap cores support

interrupt-driver mode, this driver only supports polled operation, in

order to make the code simpler, and since the interrupt processing

overhead is likely to slow down the throughput under Linux.
Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
Fixed to add mutexes, and a few style issues.
Acked-by: Grant Likely <grant.likely@secretlab.ca>
The final update to xilinx_hwicap.h was missing.
fix some missing __user tags and incorrect section tags.

convert semaphores to mutexes.

make probed_devices re-entrancy and error condition safe.

fix some backwards memcpys.

some other minor cleanups.
Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>

---

 drivers/char/Kconfig                       |    7 +

 drivers/char/Makefile                      |    1 +

 drivers/char/xilinx_hwicap/Makefile        |    7 +

 drivers/char/xilinx_hwicap/buffer_icap.c   |  380 ++++++++++++

 drivers/char/xilinx_hwicap/buffer_icap.h   |   57 ++

 drivers/char/xilinx_hwicap/fifo_icap.c     |  381 ++++++++++++

 drivers/char/xilinx_hwicap/fifo_icap.h     |   62 ++

 drivers/char/xilinx_hwicap/xilinx_hwicap.c |  923 ++++++++++++++++++++++++++++

 drivers/char/xilinx_hwicap/xilinx_hwicap.h |  193 ++++++

 9 files changed, 2011 insertions(+), 0 deletions(-)

 create mode 100644 drivers/char/xilinx_hwicap/Makefile

 create mode 100644 drivers/char/xilinx_hwicap/buffer_icap.c

 creat...
[ message continues ]

Hi,

For this function and many others in these source files:

Please see Documentation/kernel-doc-nano-HOWTO.txt for the correct

kernel-doc notation format.
If you have questions or need help, please ask.
Hints:

a. function name & short description: separate name & description with '-'
---

~Randy

--

Just a sideway note, static DEFINE_MUTEX(icap_sem); and you don't need to

runtime init it then.
--

the sem)
Hmm..  hadn't realized that.  I'm open to suggestions on how to do this

better.  The real reason why the synchronization is there is to make

sure that only one client is using the device at a time, using the
The physical device only generates/accepts complete words, the intention

is to account for the possibility that a read does not read complete

words, and to fulfill the read whenever possible.

It's arguable that read() and write() should not accept or return
no idea.  even if it is, it's unlikely that the of_driver probing is
I thought I had been.... hmm... looks like a few other things to fix

there, too.
Steve
--

device_create(); and pass the struct device as a parent when you are at it. 
OK, but why are you copying anything to buffer which you free 2 lines below?

--

They should actually be resource_size_t :-)
Ben.
--

Yup, should... [I don't know how ofter people have 64bit resources on 32 bit

this becomes a problem.]

--

For MMIO it's becoming more and more common actually in the embedded

space. Xilinx uses 405 cores which I think still only have a 32 bits

physical bus though, but if they ever extend it or switch to 440 they'll

have a 36 bits bus.
Ben.
--

The Virtex5 has a 440.  It's going to be released "real soon now"
g.
--

Grant Likely, B.Sc., P.Eng.

Secret Lab Technologies Ltd.

--

On Thu, 07 Feb 2008 21:08:50 +0100
Never seen it before in my life.  Can't find any references to it in any of

the mailing lists.  I ended up googling the changelog text for a whopping

three hits and it appears that this change went into the powerpc patch

system (http://patchwork.ozlabs.org/linuxppc/patch?id=16712)
How it got from there into Linux is also a mystery.  I see a batch of

powerpc updates just went into mainline but I don't know whose tree was

pulled - I wasn't copied on any pull request and I can't find one on the

kernel mailing list.
Perhaps Paul has just done a stealth merge, but the patch to which you

refer doesn't have his signoff.  Very confused.
Guys, can we all play too?

--

On Thu, 7 Feb 2008 12:42:03 -0800
Gah, no.  Don't blame Paul.  It wasn't actually stealth either as it

got posted a few times to the powerpc list.
You can blame me and/or Grant if you'd like.  It's a Virtex specific

driver and I didn't see any problem with it going in through the
Sure.  You could add my tree to -mm if you want.  Though I'd really

rather if we all sync up with Paul sooner so you don't have to track 5

or 6 "powerpc" trees.
josh

--

On Thu, 7 Feb 2008 15:35:45 -0600
Yes, I was trying to pull Kumar's tree for a while.  I still am doing so,

but it's presently 6.5MB of conflicts, so I'll give up on that ;)
In theory, your 2.6.x+1 material shold be ready to go well in advance of

the 2.6.x release day (hah), so it'd be good if it could be in Paul's tree

during 2.6.x's late -rc's.

--

On Thu, 7 Feb 2008 14:11:43 -0800
Kumar's tree is speshul ;).  I'm entirely too nice and my tree tends to
Actually, 90% of it was.  The recent stuff from my tree was all bug

fixes and/or patches that were waiting on some other subsystem

maintainer to get merged (the ehci usb bits, etc).  I think I'm still

waiting on Jeff for a netdev patch even.  Anyway, the exception to that

was the Virtex stuff so my bad there.
So I really am trying to play nice and mostly have stuff in by the

-rc's already.  I'll take heed to your warning though and watch the

late commits from now on.
josh

--

It went through my tree.  Paul pulls from Josh Boyer's tree for

powerpc-4xx patches, and Josh pulls from mine for xilinx virtex

powerpc 405 patches.
My screw up, sorry I broke the rules.  What is the best way to resolve this?
g.
--

Grant Likely, B.Sc., P.Eng.

Secret Lab Technologies Ltd.

--

It's unclear to me whether you did anything wrong here.
This driver is totally platform specific (it's not like it was a

wireless driver or something like that) and has been reviewed on the

platform mailing list (even if, apprently, not enough).
If those problems haven't been spotted, then too bad, and thanks Jiri

for picking them up later on, that's much welcome, but I don't think we

should start having all of the platform bits go through lkml, it

wouldn't be practical at all.
As far as -mm is concerned, that depends how long this has been in

paulus tree before the merge window I suppose. I suspect that while

paulus for-2.6.25 has been around getting ready for merge for some time

now (we've been good citizen in that regard, getting everything together

way before the actual merge window), we haven't necessarily yet totally

sorted out the timing with out own sub-maintainer and sub-sub-maintainer

trees (powerpc is a complicated architecture !).
So while I would have expected this to show up at least for a little

while in -mm via paulus for-2.6.25, it's possible that it didn't happen,

or that paulus didn't include for-2.6.25 in powerpc.git or something

like that....
So we can try to improve in this area, but I don't think Grant himself

did anything wrong including that driver in his tree and having paul

merge it, except maybe for a bit more in depth review (but we all do

miss things sometimes).
Cheers,

Ben.
--

On Thu, 7 Feb 2008 13:54:06 -0700
Well I think we'd like to see the patches appear in Paul's tree well before

the merge window if poss - that way they'll get a little bit of tyre-kicking

and perhaps review via -mm.  Kamalesh and I (at least) do perform build-

and runtime testing of powerpc.
I would request that Paul copy myself and the main mailing list on pull

requests.
It seems wrong that the signoff trail for that patch didn't actually

reflect reality - it should have had both Josh's and Paul's signoffs.  That

would require that the changelog be altered during git->git transfers which

I expect is just incompatible with the way git works (as far as I dimly

understand it).
It never hurts to send a patch to lkml even if you believe it isn't of

general interest.  People will pass an idle eye across it, and things might

get picked up, yielding improvements.  Plus more people know about its

existence.  Probably this is more the case for something which resides

under drivers/char/ than with something which resides in arch/powerpc/...

--

No, it's correct.
If you look deeper, you'll see (in gitk, or using "--pretty=fuller") that

it has:
	Author:     Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>

	Commit:     Grant Likely <grant.likely@secretlab.ca>
which means that the sign-off chain is complete and matches:
    Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>

    Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
ie it was written by Stephen, and then committed by Grant, and that's the

whole sequence.
Then, of course, that commit was merged into another tree entirely (by

Josh) in 256ae6a720618cbbfacc5e62ea1fe7c129d1b644, and by Paul in

5ab3e84f66321579ca36b63a13bf78decba65121 and then finally by me in

37969581301e50872a1ae84dc73962b5f7ee6b76, but those merge commits only

show up because there was other development too (ie those things would not

have showed up at all if the merges had been just fast-forwards).
So in general, the rule is that the sign-off chain should take you from

the author to the committer.
You can't really go any further: we could have some "forced sign-off on

merge between trees", but that would not just be inconvenient, it is

fundamentally questionable in a git environment: who is "upstream" is

really just a matter of opinion (ie I may be "obviously upstream", but if

I have merge problems I'll actually ask the downstream person to do the

merge, because he's the one who has the knowledge about that particular

merge!)
		Linus

--

ok
Cheers,

g.
--

Grant Likely, B.Sc., P.Eng.

Secret Lab Technologies Ltd.

--

I'm rather new to all this and still (to some extent) trying to figure

out what is expected.
Maybe what is needed is git pull --ack?
Steve
--

On Thu, 7 Feb 2008 13:35:09 -0800
We're all still trying to figure it out, really.  We've gone and made it

awfully easy to get code into the kernel nowadays.  Perhaps too easy.  I'm

presently having a little campaign of watching what's going on a bit more

closely, and ecouraging people to make it easier for others to see what's

going on, should they choose to do so.
I'm hoping that by the time we're developing 2.6.26 there will be a unified

git tree (aka linux-next) (basically all the git and quilt trees from -mm)

for people to look at and play with.  That'll improve our pre-merge test

coverage, but won't do much to improve review.  And it won't do anything to
I'm also hoping/expecting/planning that if someone tries to merge a patch

into mainline, and that patch hasn't been in linux-next for "long enough",

flags will wave and lights will flash and we can get to take a closer look

at why it was so late and whether it's OK.

--

Hmm...  what if git told you what time the original git commit happened?

That can't be that hard to add (if it's not already there!)
Steve
--

Hmmm, if that's the rule then I apologize.  I had thought that arch

specific drivers were okay (assuming not part of a common subsystem

like USB, Eth, etc).  The driver went through a number of review

cycles on the linuxppc mailing list and the comments had settled down.

 I didn't see any problem in merging it as it is a platform specific

driver only used by the Xilinx Virtex chips.  It is only used by

arch/powerpc and only when CONFIG_XILINX_VIRTEX is selected.
Linus has already pulled Paul's tree so the patch is already merged

upstream.  Does it need to come back out?
Stephen, can you please repost you patch to the LKML?  Even if the

driver is allowed to stay in for the time being you can at least get

feedback on what needs to be changed.
Cheers,
--

Grant Likely, B.Sc., P.Eng.

Secret Lab Technologies Ltd.

--