Xilinx: hwicap driver comments

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

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.

On Thu, 07 Feb 2008 21:08:50 +0100
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.

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.

- io resources are at least ulong

Quoted 6 lines · Andrew Morton

On Thu, 07 Feb 2008 21:08:50 +0100
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.

Quoted 2 lines

Perhaps Paul has just done a stealth merge, but the patch to which you refer doesn't have his signoff. Very confused.

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?

Quoted 6 lines · Andrew Morton

On Thu, 07 Feb 2008 21:08:50 +0100
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.

Quoted 2 lines

Perhaps Paul has just done a stealth merge, but the patch to which you refer doesn't have his signoff. Very confused.

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?

Quoted 1 line · Jiri Slaby

- io resources are at least ulong

They should actually be resource_size_t :-)

Quoted 5 lines · On Thu, 7 Feb 2008 13:54:06 -0700 "Grant Likely" <[email protected]>…

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.

Quoted 2 lines

My screw up, sorry I broke the rules. What is the best way to resolve this?

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

Quoted 4 lines · Benjamin Herrenschmidt

Quoted 1 line · Jiri Slaby

- io resources are at least ulong

They should actually be resource_size_t :-)

Yup, should... [I don't know how ofter people have 64bit resources on 32 bit this becomes a problem.]

Quoted 2 lines · Jiri Slaby

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.

Quoted 2 lines

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

Quoted 1 line

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

Quoted 1 line

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

Quoted 1 line

understand it).

Quoted 3 lines · On Thu, 07 Feb 2008 21:08:50 +0100 Jiri Slaby <[email protected]> wrote:

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.

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?

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.

Quoted 2 lines

Quoted 2 lines

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

Quoted 1 line

Quoted 1 line

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

transfers which

Quoted 1 line

Quoted 1 line

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

dimly

Quoted 1 line

Quoted 1 line

understand it).

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?

Quoted 2 lines

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

Quoted 1 line

out what is expected.

We're all still trying to figure it out, really. We've gone and made

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

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

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

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

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

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

coverage, but won't do much to improve review. And it won't do

improve our post-merge bugfixing.

Quoted 1 line

Maybe what is needed is git pull --ack?

I'm also hoping/expecting/planning that if someone tries to merge a

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

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

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

Quoted 1 line

Guys, can we all play too?

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.

-----Original Message-----
From: Jiri Slaby [mailto:[email protected]]
Sent: Thursday, February 07, 2008 12:09 PM
To: Stephen Neuendorffer; Grant Likely
Cc: Linux Kernel Mailing List; Andrew Morton
Subject: Xilinx: hwicap driver comments

Hi,

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

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

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

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

- semaphores are deprecated
- class_device_create is deprecated

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

it's subset)
- this piece:

         drvdata = kmalloc(sizeof(struct hwicap_drvdata), GFP_KERNEL);
         if (!drvdata) {
                 dev_err(dev, "Couldn't allocate device private

                 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 -

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

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

Quoted 2 lines

Few comments I have:
- release f_op retval is silently ignored, I guess you will get your

device into

Quoted 1 line

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

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

Quoted 2 lines

- semaphores are deprecated
- class_device_create is deprecated

What is preferred?

Quoted 4 lines

- don't understand this:

                 memcpy(kbuf, drvdata->read_buffer, bytes_remaining);
                 drvdata->read_buffer_in_use = bytes_remaining;
                 free_page((unsigned long)kbuf);

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 partial words, I suppose

Quoted 5 lines · On Thu, 7 Feb 2008 15:35:45 -0600 Josh Boyer <[email protected]> wrote:

Quoted 1 line

Guys, can we all play too?

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.

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.

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 <[email protected]>

Fixed to add mutexes, and a few style issues.

Acked-by Grant Likely <[email protected]>

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 <[email protected]>

---

drivers/char/xilinx_hwicap/xilinx_hwicap.c new file +0 -0

new file mode 100644
index 0000000..2caac31
@@ -0,0 +1,923 @@

 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
 create mode 100644 drivers/char/xilinx_hwicap/buffer_icap.h
 create mode 100644 drivers/char/xilinx_hwicap/fifo_icap.c
 create mode 100644 drivers/char/xilinx_hwicap/fifo_icap.h
 create mode 100644 drivers/char/xilinx_hwicap/xilinx_hwicap.c
 create mode 100644 drivers/char/xilinx_hwicap/xilinx_hwicap.h

drivers/char/xilinx_hwicap/buffer_icap.c new file +0 -0

new file mode 100644
index 0000000..dfea2bd

+ * The status register contains the ICAP status and the done bit.
+ *
+ * D8 - cfgerr
+ * D7 - dalign
+ * D6 - rip
+ * D5 - in_abort_l
+ * D4 - Always 1
+ * D3 - Always 1
+ * D2 - Always 1
+ * D1 - Always 1
+ * D0 - Done bit
+ **/

drivers/char/xilinx_hwicap/buffer_icap.c new file +4 -0

+static inline u32 buffer_icap_get_status(void __iomem *base_address)
+{
+	return in_be32(base_address + XHI_STATUS_REG_OFFSET);
+}

drivers/char/xilinx_hwicap/xilinx_hwicap.h new file +1 -0

new file mode 100644
index 0000000..5718679
@@ -0,0 +1,193 @@
 
+#ifndef XILINX_HWICAP_H_	/* prevent circular inclusions */

drivers/char/xilinx_hwicap/xilinx_hwicap.h new file +20 -0

+	u32 write_buffer_in_use;  /* Always in [0,3] */
+	u8 write_buffer[4];
+	u32 read_buffer_in_use;	  /* Always in [0,3] */
+	u8 read_buffer[4];
+	resource_size_t mem_start;/* phys. address of the control registers */
+	resource_size_t mem_end;  /* phys. address of the control registers */
+	resource_size_t mem_size;
+	void __iomem *base_address;/* virt. address of the control registers */
+
+	struct device *dev;
+	struct cdev cdev;	/* Char device structure */
+	dev_t devt;
+
+	const struct hwicap_driver_config *config;
+	const struct config_registers *config_regs;
+	void *private_data;
+	bool is_open;
+	struct mutex sem;
+};
+

-----Original Message-----
From: Jiri Slaby [mailto:[email protected]]
Sent: Thursday, February 07, 2008 12:09 PM
To: Stephen Neuendorffer; Grant Likely
Cc: Linux Kernel Mailing List; Andrew Morton
Subject: Xilinx: hwicap driver comments

Hi,

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

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

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

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

- semaphores are deprecated

- class_device_create is deprecated

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

it's subset)

- this piece:

         drvdata = kmalloc(sizeof(struct hwicap_drvdata), GFP_KERNEL);
         if (!drvdata) {
                 dev_err(dev, "Couldn't allocate device private

                 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 -

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

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