* [PATCH 1/3] hpilo: staging for interrupt handling
2009-08-10 19:57 [PATCH 0/3] Enable poll handler in hpilo David Altobelli
@ 2009-08-10 19:58 ` David Altobelli
2009-08-10 20:00 ` [PATCH 2/3] hpilo: add interrupt handler David Altobelli
2009-08-10 20:00 ` [PATCH 3/3] hpilo: add poll f_op David Altobelli
2 siblings, 0 replies; 6+ messages in thread
From: David Altobelli @ 2009-08-10 19:58 UTC (permalink / raw)
To: David Altobelli; +Cc: linux-kernel, gregkh
Refactor some hpilo routines in order to allow for locking changes in
interrupt handling. Should not be functionally different.
Please CC me on any replies.
Signed-off-by: David Altobelli <david.altobelli@hp.com>
---
hpilo.c | 120 ++++++++++++++++++++++++++++++++++++++--------------------------
1 file changed, 73 insertions(+), 47 deletions(-)
--- linux-2.6.30.3/drivers/misc/hpilo.c.orig 2009-08-05 12:53:28.000000000 -0500
+++ linux-2.6.30.3/drivers/misc/hpilo.c 2009-08-05 12:54:13.000000000 -0500
@@ -151,6 +151,7 @@ static inline void doorbell_clr(struct c
{
iowrite8(2, ccb->ccb_u5.db_base);
}
+
static inline int ctrl_set(int l2sz, int idxmask, int desclim)
{
int active = 0, go = 1;
@@ -160,6 +161,7 @@ static inline int ctrl_set(int l2sz, int
active << CTRL_BITPOS_A |
go << CTRL_BITPOS_G;
}
+
static void ctrl_setup(struct ccb *ccb, int nr_desc, int l2desc_sz)
{
/* for simplicity, use the same parameters for send and recv ctrls */
@@ -192,13 +194,10 @@ static void fifo_setup(void *base_addr,
static void ilo_ccb_close(struct pci_dev *pdev, struct ccb_data *data)
{
- struct ccb *driver_ccb;
- struct ccb __iomem *device_ccb;
+ struct ccb *driver_ccb = &data->driver_ccb;
+ struct ccb __iomem *device_ccb = data->mapped_ccb;
int retries;
- driver_ccb = &data->driver_ccb;
- device_ccb = data->mapped_ccb;
-
/* complicated dance to tell the hw we are stopping */
doorbell_clr(driver_ccb);
iowrite32(ioread32(&device_ccb->send_ctrl) & ~(1 << CTRL_BITPOS_G),
@@ -225,26 +224,22 @@ static void ilo_ccb_close(struct pci_dev
pci_free_consistent(pdev, data->dma_size, data->dma_va, data->dma_pa);
}
-static int ilo_ccb_open(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
+static int ilo_ccb_setup(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
{
char *dma_va, *dma_pa;
- int pkt_id, pkt_sz, i, error;
struct ccb *driver_ccb, *ilo_ccb;
- struct pci_dev *pdev;
driver_ccb = &data->driver_ccb;
ilo_ccb = &data->ilo_ccb;
- pdev = hw->ilo_dev;
data->dma_size = 2 * fifo_sz(NR_QENTRY) +
2 * desc_mem_sz(NR_QENTRY) +
ILO_START_ALIGN + ILO_CACHE_SZ;
- error = -ENOMEM;
- data->dma_va = pci_alloc_consistent(pdev, data->dma_size,
+ data->dma_va = pci_alloc_consistent(hw->ilo_dev, data->dma_size,
&data->dma_pa);
if (!data->dma_va)
- goto out;
+ return -ENOMEM;
dma_va = (char *)data->dma_va;
dma_pa = (char *)data->dma_pa;
@@ -290,10 +285,18 @@ static int ilo_ccb_open(struct ilo_hwinf
driver_ccb->ccb_u5.db_base = hw->db_vaddr + (slot << L2_DB_SIZE);
ilo_ccb->ccb_u5.db_base = NULL; /* hw ccb's doorbell is not used */
+ return 0;
+}
+
+static void ilo_ccb_open(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
+{
+ int pkt_id, pkt_sz;
+ struct ccb *driver_ccb = &data->driver_ccb;
+
/* copy the ccb with physical addrs to device memory */
data->mapped_ccb = (struct ccb __iomem *)
(hw->ram_vaddr + (slot * ILOHW_CCB_SZ));
- memcpy_toio(data->mapped_ccb, ilo_ccb, sizeof(struct ccb));
+ memcpy_toio(data->mapped_ccb, &data->ilo_ccb, sizeof(struct ccb));
/* put packets on the send and receive queues */
pkt_sz = 0;
@@ -306,7 +309,14 @@ static int ilo_ccb_open(struct ilo_hwinf
for (pkt_id = 0; pkt_id < NR_QENTRY; pkt_id++)
ilo_pkt_enqueue(hw, driver_ccb, RECVQ, pkt_id, pkt_sz);
+ /* the ccb is ready to use */
doorbell_clr(driver_ccb);
+}
+
+static int ilo_ccb_verify(struct ilo_hwinfo *hw, struct ccb_data *data)
+{
+ int pkt_id, i;
+ struct ccb *driver_ccb = &data->driver_ccb;
/* make sure iLO is really handling requests */
for (i = MAX_WAIT; i > 0; i--) {
@@ -315,20 +325,14 @@ static int ilo_ccb_open(struct ilo_hwinf
udelay(WAIT_TIME);
}
- if (i) {
- ilo_pkt_enqueue(hw, driver_ccb, SENDQ, pkt_id, 0);
- doorbell_set(driver_ccb);
- } else {
- dev_err(&pdev->dev, "Open could not dequeue a packet\n");
- error = -EBUSY;
- goto free;
+ if (i == 0) {
+ dev_err(&hw->ilo_dev->dev, "Open could not dequeue a packet\n");
+ return -EBUSY;
}
+ ilo_pkt_enqueue(hw, driver_ccb, SENDQ, pkt_id, 0);
+ doorbell_set(driver_ccb);
return 0;
-free:
- ilo_ccb_close(pdev, data);
-out:
- return error;
}
static inline int is_channel_reset(struct ccb *ccb)
@@ -343,16 +347,31 @@ static inline void set_channel_reset(str
FIFOBARTOHANDLE(ccb->ccb_u1.send_fifobar)->reset = 1;
}
+static inline int get_device_outbound(struct ilo_hwinfo *hw)
+{
+ return ioread32(&hw->mmio_vaddr[DB_OUT]);
+}
+
+static inline int is_db_reset(int db_out)
+{
+ return db_out & (1 << DB_RESET);
+}
+
static inline int is_device_reset(struct ilo_hwinfo *hw)
{
/* check for global reset condition */
- return ioread32(&hw->mmio_vaddr[DB_OUT]) & (1 << DB_RESET);
+ return is_db_reset(get_device_outbound(hw));
+}
+
+static inline void clear_pending_db(struct ilo_hwinfo *hw, int clr)
+{
+ iowrite32(clr, &hw->mmio_vaddr[DB_OUT]);
}
static inline void clear_device(struct ilo_hwinfo *hw)
{
/* clear the device (reset bits, pending channel entries) */
- iowrite32(-1, &hw->mmio_vaddr[DB_OUT]);
+ clear_pending_db(hw, -1);
}
static void ilo_locked_reset(struct ilo_hwinfo *hw)
@@ -387,15 +406,11 @@ static ssize_t ilo_read(struct file *fp,
size_t len, loff_t *off)
{
int err, found, cnt, pkt_id, pkt_len;
- struct ccb_data *data;
- struct ccb *driver_ccb;
- struct ilo_hwinfo *hw;
+ struct ccb_data *data = fp->private_data;
+ struct ccb *driver_ccb = &data->driver_ccb;
+ struct ilo_hwinfo *hw = data->ilo_hw;
void *pkt;
- data = fp->private_data;
- driver_ccb = &data->driver_ccb;
- hw = data->ilo_hw;
-
if (is_device_reset(hw) || is_channel_reset(driver_ccb)) {
/*
* If the device has been reset, applications
@@ -442,15 +457,11 @@ static ssize_t ilo_write(struct file *fp
size_t len, loff_t *off)
{
int err, pkt_id, pkt_len;
- struct ccb_data *data;
- struct ccb *driver_ccb;
- struct ilo_hwinfo *hw;
+ struct ccb_data *data = fp->private_data;
+ struct ccb *driver_ccb = &data->driver_ccb;
+ struct ilo_hwinfo *hw = data->ilo_hw;
void *pkt;
- data = fp->private_data;
- driver_ccb = &data->driver_ccb;
- hw = data->ilo_hw;
-
if (is_device_reset(hw) || is_channel_reset(driver_ccb)) {
/*
* If the device has been reset, applications
@@ -532,14 +543,28 @@ static int ilo_open(struct inode *ip, st
/* each fd private_data holds sw/hw view of ccb */
if (hw->ccb_alloc[slot] == NULL) {
/* create a channel control block for this minor */
- error = ilo_ccb_open(hw, data, slot);
- if (!error) {
- hw->ccb_alloc[slot] = data;
- hw->ccb_alloc[slot]->ccb_cnt = 1;
- hw->ccb_alloc[slot]->ccb_excl = fp->f_flags & O_EXCL;
- hw->ccb_alloc[slot]->ilo_hw = hw;
- } else
+ error = ilo_ccb_setup(hw, data, slot);
+ if (error) {
+ kfree(data);
+ goto out;
+ }
+
+ /* write the ccb to hw */
+ ilo_ccb_open(hw, data, slot);
+
+ /* make sure the channel is functional */
+ error = ilo_ccb_verify(hw, data);
+ if (error) {
+ ilo_ccb_close(hw->ilo_dev, data);
kfree(data);
+ goto out;
+ }
+
+ data->ccb_cnt = 1;
+ data->ccb_excl = fp->f_flags & O_EXCL;
+ data->ilo_hw = hw;
+ hw->ccb_alloc[slot] = data;
+
} else {
kfree(data);
if (fp->f_flags & O_EXCL || hw->ccb_alloc[slot]->ccb_excl) {
@@ -554,6 +579,7 @@ static int ilo_open(struct inode *ip, st
error = 0;
}
}
+out:
spin_unlock(&hw->alloc_lock);
if (!error)
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 2/3] hpilo: add interrupt handler
2009-08-10 19:57 [PATCH 0/3] Enable poll handler in hpilo David Altobelli
2009-08-10 19:58 ` [PATCH 1/3] hpilo: staging for interrupt handling David Altobelli
@ 2009-08-10 20:00 ` David Altobelli
2009-08-18 21:30 ` Andrew Morton
2009-08-10 20:00 ` [PATCH 3/3] hpilo: add poll f_op David Altobelli
2 siblings, 1 reply; 6+ messages in thread
From: David Altobelli @ 2009-08-10 20:00 UTC (permalink / raw)
To: David Altobelli; +Cc: linux-kernel, gregkh
Add interrupt handler to hpilo. This is enablement for poll handler, and
it also simplifies the logic for handling an iLO reset, because now
only the interrupt handler needs to look for reset, the file system interfaces
only need to return failure when a reset has happened.
Please CC me on any replies.
Signed-off-by: David Altobelli <david.altobelli@hp.com>
---
hpilo.c | 138 +++++++++++++++++++++++++++++++++++++++++++---------------------
hpilo.h | 8 +++
2 files changed, 101 insertions(+), 45 deletions(-)
diff -uprN -X linux-2.6.30.3-orig/Documentation/dontdiff linux-2.6.30.3-orig/drivers/misc/hpilo.c linux-2.6.30.3/drivers/misc/hpilo.c
--- linux-2.6.30.3-orig/drivers/misc/hpilo.c 2009-08-07 09:26:45.000000000 -0500
+++ linux-2.6.30.3/drivers/misc/hpilo.c 2009-08-07 09:26:24.000000000 -0500
@@ -13,6 +13,7 @@
#include <linux/module.h>
#include <linux/fs.h>
#include <linux/pci.h>
+#include <linux/interrupt.h>
#include <linux/ioport.h>
#include <linux/device.h>
#include <linux/file.h>
@@ -21,6 +22,7 @@
#include <linux/delay.h>
#include <linux/uaccess.h>
#include <linux/io.h>
+#include <linux/wait.h>
#include "hpilo.h"
static struct class *ilo_class;
@@ -61,9 +63,10 @@ static inline int desc_mem_sz(int nr_ent
static int fifo_enqueue(struct ilo_hwinfo *hw, char *fifobar, int entry)
{
struct fifo *fifo_q = FIFOBARTOHANDLE(fifobar);
+ unsigned long flags;
int ret = 0;
- spin_lock(&hw->fifo_lock);
+ spin_lock_irqsave(&hw->fifo_lock, flags);
if (!(fifo_q->fifobar[(fifo_q->tail + 1) & fifo_q->imask]
& ENTRY_MASK_O)) {
fifo_q->fifobar[fifo_q->tail & fifo_q->imask] |=
@@ -71,7 +74,7 @@ static int fifo_enqueue(struct ilo_hwinf
fifo_q->tail += 1;
ret = 1;
}
- spin_unlock(&hw->fifo_lock);
+ spin_unlock_irqrestore(&hw->fifo_lock, flags);
return ret;
}
@@ -79,10 +82,11 @@ static int fifo_enqueue(struct ilo_hwinf
static int fifo_dequeue(struct ilo_hwinfo *hw, char *fifobar, int *entry)
{
struct fifo *fifo_q = FIFOBARTOHANDLE(fifobar);
+ unsigned long flags;
int ret = 0;
u64 c;
- spin_lock(&hw->fifo_lock);
+ spin_lock_irqsave(&hw->fifo_lock, flags);
c = fifo_q->fifobar[fifo_q->head & fifo_q->imask];
if (c & ENTRY_MASK_C) {
if (entry)
@@ -93,7 +97,7 @@ static int fifo_dequeue(struct ilo_hwinf
fifo_q->head += 1;
ret = 1;
}
- spin_unlock(&hw->fifo_lock);
+ spin_unlock_irqrestore(&hw->fifo_lock, flags);
return ret;
}
@@ -374,7 +378,18 @@ static inline void clear_device(struct i
clear_pending_db(hw, -1);
}
-static void ilo_locked_reset(struct ilo_hwinfo *hw)
+static inline void ilo_enable_interrupts(struct ilo_hwinfo *hw)
+{
+ iowrite8(ioread8(&hw->mmio_vaddr[DB_IRQ]) | 1, &hw->mmio_vaddr[DB_IRQ]);
+}
+
+static inline void ilo_disable_interrupts(struct ilo_hwinfo *hw)
+{
+ iowrite8(ioread8(&hw->mmio_vaddr[DB_IRQ]) & ~1,
+ &hw->mmio_vaddr[DB_IRQ]);
+}
+
+static void ilo_set_reset(struct ilo_hwinfo *hw)
{
int slot;
@@ -387,19 +402,6 @@ static void ilo_locked_reset(struct ilo_
continue;
set_channel_reset(&hw->ccb_alloc[slot]->driver_ccb);
}
-
- clear_device(hw);
-}
-
-static void ilo_reset(struct ilo_hwinfo *hw)
-{
- spin_lock(&hw->alloc_lock);
-
- /* reset might have been handled after lock was taken */
- if (is_device_reset(hw))
- ilo_locked_reset(hw);
-
- spin_unlock(&hw->alloc_lock);
}
static ssize_t ilo_read(struct file *fp, char __user *buf,
@@ -411,12 +413,11 @@ static ssize_t ilo_read(struct file *fp,
struct ilo_hwinfo *hw = data->ilo_hw;
void *pkt;
- if (is_device_reset(hw) || is_channel_reset(driver_ccb)) {
+ if (is_channel_reset(driver_ccb)) {
/*
* If the device has been reset, applications
* need to close and reopen all ccbs.
*/
- ilo_reset(hw);
return -ENODEV;
}
@@ -462,14 +463,8 @@ static ssize_t ilo_write(struct file *fp
struct ilo_hwinfo *hw = data->ilo_hw;
void *pkt;
- if (is_device_reset(hw) || is_channel_reset(driver_ccb)) {
- /*
- * If the device has been reset, applications
- * need to close and reopen all ccbs.
- */
- ilo_reset(hw);
+ if (is_channel_reset(driver_ccb))
return -ENODEV;
- }
/* get a packet to send the user command */
if (!ilo_pkt_dequeue(hw, driver_ccb, SENDQ, &pkt_id, &pkt_len, &pkt))
@@ -496,27 +491,28 @@ static int ilo_close(struct inode *ip, s
int slot;
struct ccb_data *data;
struct ilo_hwinfo *hw;
+ unsigned long flags;
slot = iminor(ip) % MAX_CCB;
hw = container_of(ip->i_cdev, struct ilo_hwinfo, cdev);
- spin_lock(&hw->alloc_lock);
-
- if (is_device_reset(hw))
- ilo_locked_reset(hw);
+ spin_lock(&hw->open_lock);
if (hw->ccb_alloc[slot]->ccb_cnt == 1) {
data = fp->private_data;
+ spin_lock_irqsave(&hw->alloc_lock, flags);
+ hw->ccb_alloc[slot] = NULL;
+ spin_unlock_irqrestore(&hw->alloc_lock, flags);
+
ilo_ccb_close(hw->ilo_dev, data);
kfree(data);
- hw->ccb_alloc[slot] = NULL;
} else
hw->ccb_alloc[slot]->ccb_cnt--;
- spin_unlock(&hw->alloc_lock);
+ spin_unlock(&hw->open_lock);
return 0;
}
@@ -526,6 +522,7 @@ static int ilo_open(struct inode *ip, st
int slot, error;
struct ccb_data *data;
struct ilo_hwinfo *hw;
+ unsigned long flags;
slot = iminor(ip) % MAX_CCB;
hw = container_of(ip->i_cdev, struct ilo_hwinfo, cdev);
@@ -535,10 +532,7 @@ static int ilo_open(struct inode *ip, st
if (!data)
return -ENOMEM;
- spin_lock(&hw->alloc_lock);
-
- if (is_device_reset(hw))
- ilo_locked_reset(hw);
+ spin_lock(&hw->open_lock);
/* each fd private_data holds sw/hw view of ccb */
if (hw->ccb_alloc[slot] == NULL) {
@@ -549,22 +543,31 @@ static int ilo_open(struct inode *ip, st
goto out;
}
+ data->ccb_cnt = 1;
+ data->ccb_excl = fp->f_flags & O_EXCL;
+ data->ilo_hw = hw;
+ init_waitqueue_head(&data->ccb_waitq);
+
/* write the ccb to hw */
+ spin_lock_irqsave(&hw->alloc_lock, flags);
ilo_ccb_open(hw, data, slot);
+ hw->ccb_alloc[slot] = data;
+ spin_unlock_irqrestore(&hw->alloc_lock, flags);
/* make sure the channel is functional */
error = ilo_ccb_verify(hw, data);
if (error) {
+
+ spin_lock_irqsave(&hw->alloc_lock, flags);
+ hw->ccb_alloc[slot] = NULL;
+ spin_unlock_irqrestore(&hw->alloc_lock, flags);
+
ilo_ccb_close(hw->ilo_dev, data);
+
kfree(data);
goto out;
}
- data->ccb_cnt = 1;
- data->ccb_excl = fp->f_flags & O_EXCL;
- data->ilo_hw = hw;
- hw->ccb_alloc[slot] = data;
-
} else {
kfree(data);
if (fp->f_flags & O_EXCL || hw->ccb_alloc[slot]->ccb_excl) {
@@ -580,7 +583,7 @@ static int ilo_open(struct inode *ip, st
}
}
out:
- spin_unlock(&hw->alloc_lock);
+ spin_unlock(&hw->open_lock);
if (!error)
fp->private_data = hw->ccb_alloc[slot];
@@ -596,6 +599,41 @@ static const struct file_operations ilo_
.release = ilo_close,
};
+static irqreturn_t ilo_isr(int irq, void *data)
+{
+ struct ilo_hwinfo *hw = data;
+ int pending, i;
+
+ spin_lock(&hw->alloc_lock);
+
+ /* check for ccbs which have data */
+ pending = get_device_outbound(hw);
+ if (!pending) {
+ spin_unlock(&hw->alloc_lock);
+ return IRQ_NONE;
+ }
+
+ if (is_db_reset(pending)) {
+ /* wake up all ccbs if the device was reset */
+ pending = -1;
+ ilo_set_reset(hw);
+ }
+
+ for (i = 0; i < MAX_CCB; i++) {
+ if (!hw->ccb_alloc[i])
+ continue;
+ if (pending & (1 << i))
+ wake_up_interruptible(&hw->ccb_alloc[i]->ccb_waitq);
+ }
+
+ /* clear the device of the channels that have been handled */
+ clear_pending_db(hw, pending);
+
+ spin_unlock(&hw->alloc_lock);
+
+ return IRQ_HANDLED;
+}
+
static void ilo_unmap_device(struct pci_dev *pdev, struct ilo_hwinfo *hw)
{
pci_iounmap(pdev, hw->db_vaddr);
@@ -649,6 +687,8 @@ static void ilo_remove(struct pci_dev *p
device_destroy(ilo_class, MKDEV(ilo_major, i));
cdev_del(&ilo_hw->cdev);
+ ilo_disable_interrupts(ilo_hw);
+ free_irq(pdev->irq, ilo_hw);
ilo_unmap_device(pdev, ilo_hw);
pci_release_regions(pdev);
pci_disable_device(pdev);
@@ -684,6 +724,7 @@ static int __devinit ilo_probe(struct pc
ilo_hw->ilo_dev = pdev;
spin_lock_init(&ilo_hw->alloc_lock);
spin_lock_init(&ilo_hw->fifo_lock);
+ spin_lock_init(&ilo_hw->open_lock);
error = pci_enable_device(pdev);
if (error)
@@ -702,13 +743,19 @@ static int __devinit ilo_probe(struct pc
pci_set_drvdata(pdev, ilo_hw);
clear_device(ilo_hw);
+ error = request_irq(pdev->irq, ilo_isr, IRQF_SHARED, "hpilo", ilo_hw);
+ if (error)
+ goto unmap;
+
+ ilo_enable_interrupts(ilo_hw);
+
cdev_init(&ilo_hw->cdev, &ilo_fops);
ilo_hw->cdev.owner = THIS_MODULE;
start = devnum * MAX_CCB;
error = cdev_add(&ilo_hw->cdev, MKDEV(ilo_major, start), MAX_CCB);
if (error) {
dev_err(&pdev->dev, "Could not add cdev\n");
- goto unmap;
+ goto remove_isr;
}
for (minor = 0 ; minor < MAX_CCB; minor++) {
@@ -721,6 +768,9 @@ static int __devinit ilo_probe(struct pc
}
return 0;
+remove_isr:
+ ilo_disable_interrupts(ilo_hw);
+ free_irq(pdev->irq, ilo_hw);
unmap:
ilo_unmap_device(pdev, ilo_hw);
free_regions:
diff -uprN -X linux-2.6.30.3-orig/Documentation/dontdiff linux-2.6.30.3-orig/drivers/misc/hpilo.h linux-2.6.30.3/drivers/misc/hpilo.h
--- linux-2.6.30.3-orig/drivers/misc/hpilo.h 2009-08-07 09:05:09.000000000 -0500
+++ linux-2.6.30.3/drivers/misc/hpilo.h 2009-07-28 16:27:46.000000000 -0500
@@ -46,11 +46,14 @@ struct ilo_hwinfo {
spinlock_t alloc_lock;
spinlock_t fifo_lock;
+ spinlock_t open_lock;
struct cdev cdev;
};
-/* offset from mmio_vaddr */
+/* offset from mmio_vaddr for enabling doorbell interrupts */
+#define DB_IRQ 0xB2
+/* offset from mmio_vaddr for outbound communications */
#define DB_OUT 0xD4
/* DB_OUT reset bit */
#define DB_RESET 26
@@ -131,6 +134,9 @@ struct ccb_data {
/* pointer to hardware device info */
struct ilo_hwinfo *ilo_hw;
+ /* queue for this ccb to wait for recv data */
+ wait_queue_head_t ccb_waitq;
+
/* usage count, to allow for shared ccb's */
int ccb_cnt;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/3] hpilo: add interrupt handler
2009-08-10 20:00 ` [PATCH 2/3] hpilo: add interrupt handler David Altobelli
@ 2009-08-18 21:30 ` Andrew Morton
2009-08-18 22:25 ` Altobelli, David
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2009-08-18 21:30 UTC (permalink / raw)
To: David Altobelli; +Cc: david.altobelli, linux-kernel, gregkh
On Mon, 10 Aug 2009 14:00:05 -0600
David Altobelli <david.altobelli@hp.com> wrote:
> Add interrupt handler to hpilo. This is enablement for poll handler, and
> it also simplifies the logic for handling an iLO reset, because now
> only the interrupt handler needs to look for reset, the file system interfaces
> only need to return failure when a reset has happened.
>
> Please CC me on any replies.
It would be very helpful if there were comments in hpilo.h which fully
describe the roles of each lock.
>
> ...
>
> @@ -496,27 +491,28 @@ static int ilo_close(struct inode *ip, s
> int slot;
> struct ccb_data *data;
> struct ilo_hwinfo *hw;
> + unsigned long flags;
>
> slot = iminor(ip) % MAX_CCB;
> hw = container_of(ip->i_cdev, struct ilo_hwinfo, cdev);
>
> - spin_lock(&hw->alloc_lock);
> -
> - if (is_device_reset(hw))
> - ilo_locked_reset(hw);
> + spin_lock(&hw->open_lock);
>
> if (hw->ccb_alloc[slot]->ccb_cnt == 1) {
>
> data = fp->private_data;
>
> + spin_lock_irqsave(&hw->alloc_lock, flags);
> + hw->ccb_alloc[slot] = NULL;
> + spin_unlock_irqrestore(&hw->alloc_lock, flags);
> +
> ilo_ccb_close(hw->ilo_dev, data);
>
> kfree(data);
> - hw->ccb_alloc[slot] = NULL;
> } else
> hw->ccb_alloc[slot]->ccb_cnt--;
>
> - spin_unlock(&hw->alloc_lock);
> + spin_unlock(&hw->open_lock);
>
> return 0;
> }
Here I'd have expected that alloc_lock would provide the protection for
->ccb_cnt. But the code doesn't do that - it instead appears to use
->open_lock.
The code doesn't tell us what open_lock's role is intended to be so
this reviewer can't really tell whether or not this was a mistake.
>
> ...
>
> @@ -549,22 +543,31 @@ static int ilo_open(struct inode *ip, st
> goto out;
> }
>
> + data->ccb_cnt = 1;
> + data->ccb_excl = fp->f_flags & O_EXCL;
> + data->ilo_hw = hw;
> + init_waitqueue_head(&data->ccb_waitq);
> +
> /* write the ccb to hw */
> + spin_lock_irqsave(&hw->alloc_lock, flags);
> ilo_ccb_open(hw, data, slot);
> + hw->ccb_alloc[slot] = data;
> + spin_unlock_irqrestore(&hw->alloc_lock, flags);
>
> /* make sure the channel is functional */
> error = ilo_ccb_verify(hw, data);
> if (error) {
> +
> + spin_lock_irqsave(&hw->alloc_lock, flags);
> + hw->ccb_alloc[slot] = NULL;
> + spin_unlock_irqrestore(&hw->alloc_lock, flags);
> +
> ilo_ccb_close(hw->ilo_dev, data);
> +
> kfree(data);
> goto out;
> }
>
> - data->ccb_cnt = 1;
> - data->ccb_excl = fp->f_flags & O_EXCL;
> - data->ilo_hw = hw;
> - hw->ccb_alloc[slot] = data;
> -
> } else {
> kfree(data);
> if (fp->f_flags & O_EXCL || hw->ccb_alloc[slot]->ccb_excl) {
> @@ -580,7 +583,7 @@ static int ilo_open(struct inode *ip, st
> }
> }
> out:
> - spin_unlock(&hw->alloc_lock);
> + spin_unlock(&hw->open_lock);
>
> if (!error)
> fp->private_data = hw->ccb_alloc[slot];
> @@ -596,6 +599,41 @@ static const struct file_operations ilo_
> .release = ilo_close,
> };
>From this function I infer that the designed lock nesting is
open_lock
->alloc_lock
->fifo_lock
yes? It would be useful to document that also.
Have these changes been runtime tested with lockdep enabled?
>
> ...
>
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH 2/3] hpilo: add interrupt handler
2009-08-18 21:30 ` Andrew Morton
@ 2009-08-18 22:25 ` Altobelli, David
0 siblings, 0 replies; 6+ messages in thread
From: Altobelli, David @ 2009-08-18 22:25 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel@vger.kernel.org, gregkh@suse.de
From: Andrew Morton
> It would be very helpful if there were comments in hpilo.h which fully
> describe the roles of each lock.
Okay, thanks. I'll put something together.
>>
>> ...
>>
>> @@ -496,27 +491,28 @@ static int ilo_close(struct inode *ip, s
>> int slot;
>> struct ccb_data *data;
>> struct ilo_hwinfo *hw;
>> + unsigned long flags;
>>
>> slot = iminor(ip) % MAX_CCB;
>> hw = container_of(ip->i_cdev, struct ilo_hwinfo, cdev);
>>
>> - spin_lock(&hw->alloc_lock);
>> -
>> - if (is_device_reset(hw))
>> - ilo_locked_reset(hw);
>> + spin_lock(&hw->open_lock);
>>
>> if (hw->ccb_alloc[slot]->ccb_cnt == 1) {
>>
>> data = fp->private_data;
>>
>> + spin_lock_irqsave(&hw->alloc_lock, flags);
>> + hw->ccb_alloc[slot] = NULL;
>> + spin_unlock_irqrestore(&hw->alloc_lock, flags);
>> +
>> ilo_ccb_close(hw->ilo_dev, data);
>>
>> kfree(data);
>> - hw->ccb_alloc[slot] = NULL;
>> } else
>> hw->ccb_alloc[slot]->ccb_cnt--;
>>
>> - spin_unlock(&hw->alloc_lock);
>> + spin_unlock(&hw->open_lock);
>>
>> return 0;
>> }
> Here I'd have expected that alloc_lock would provide the protection for
> ->ccb_cnt. But the code doesn't do that - it instead appears to use
> ->open_lock.
Yes. My intention is to use open_lock to serialize access by open and close,
and alloc_lock to serialize pointers being added to or deleted from hw->ccb_alloc[]
(list add/remove in ilo_open/close, and run list in ilo_isr).
I wanted a lock for open/close in order to keep the reference counts and
manage ccb allocation. I wanted to separate this from actually
adding/removing a list element, because the list was searched by the interrupt
handler, and I didn't want open/close to spend a long time with irqs disabled.
Perhaps my naming/logic needs improvement, I'm open to suggestions.
> The code doesn't tell us what open_lock's role is intended to be so
> this reviewer can't really tell whether or not this was a mistake.
> From this function I infer that the designed lock nesting is
>
> open_lock
> ->alloc_lock
> ->fifo_lock
>
> yes? It would be useful to document that also.
Yes, that is the design, although taking fifo_lock doesn't require
holding alloc_lock.
> Have these changes been runtime tested with lockdep enabled?
CONFIG_LOCKDEP=y (along with the list in SubmitChecklist #12).
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] hpilo: add poll f_op
2009-08-10 19:57 [PATCH 0/3] Enable poll handler in hpilo David Altobelli
2009-08-10 19:58 ` [PATCH 1/3] hpilo: staging for interrupt handling David Altobelli
2009-08-10 20:00 ` [PATCH 2/3] hpilo: add interrupt handler David Altobelli
@ 2009-08-10 20:00 ` David Altobelli
2 siblings, 0 replies; 6+ messages in thread
From: David Altobelli @ 2009-08-10 20:00 UTC (permalink / raw)
To: David Altobelli; +Cc: linux-kernel, gregkh
Add poll handler to hpilo, to allow applications a low overhead method
of waiting for data.
Please CC me on any replies.
Signed-off-by: David Altobelli <david.altobelli@hp.com>
---
hpilo.c | 42 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 41 insertions(+), 1 deletion(-)
--- linux-2.6.30.3/drivers/misc/hpilo.c.orig 2009-08-07 09:40:48.000000000 -0500
+++ linux-2.6.30.3/drivers/misc/hpilo.c 2009-08-07 09:41:10.000000000 -0500
@@ -23,6 +23,7 @@
#include <linux/uaccess.h>
#include <linux/io.h>
#include <linux/wait.h>
+#include <linux/poll.h>
#include "hpilo.h"
static struct class *ilo_class;
@@ -102,6 +103,22 @@ static int fifo_dequeue(struct ilo_hwinf
return ret;
}
+static int fifo_check_recv(struct ilo_hwinfo *hw, char *fifobar)
+{
+ struct fifo *fifo_q = FIFOBARTOHANDLE(fifobar);
+ unsigned long flags;
+ int ret = 0;
+ u64 c;
+
+ spin_lock_irqsave(&hw->fifo_lock, flags);
+ c = fifo_q->fifobar[fifo_q->head & fifo_q->imask];
+ if (c & ENTRY_MASK_C)
+ ret = 1;
+ spin_unlock_irqrestore(&hw->fifo_lock, flags);
+
+ return ret;
+}
+
static int ilo_pkt_enqueue(struct ilo_hwinfo *hw, struct ccb *ccb,
int dir, int id, int len)
{
@@ -146,6 +163,13 @@ static int ilo_pkt_dequeue(struct ilo_hw
return ret;
}
+static int ilo_pkt_recv(struct ilo_hwinfo *hw, struct ccb *ccb)
+{
+ char *fifobar = ccb->ccb_u3.recv_fifobar;
+
+ return fifo_check_recv(hw, fifobar);
+}
+
static inline void doorbell_set(struct ccb *ccb)
{
iowrite8(1, ccb->ccb_u5.db_base);
@@ -486,6 +510,21 @@ static ssize_t ilo_write(struct file *fp
return err ? -EFAULT : len;
}
+static unsigned int ilo_poll(struct file *fp, poll_table *wait)
+{
+ struct ccb_data *data = fp->private_data;
+ struct ccb *driver_ccb = &data->driver_ccb;
+
+ poll_wait(fp, &data->ccb_waitq, wait);
+
+ if (is_channel_reset(driver_ccb))
+ return POLLERR;
+ else if (ilo_pkt_recv(data->ilo_hw, driver_ccb))
+ return POLLIN | POLLRDNORM;
+
+ return 0;
+}
+
static int ilo_close(struct inode *ip, struct file *fp)
{
int slot;
@@ -595,6 +634,7 @@ static const struct file_operations ilo_
.owner = THIS_MODULE,
.read = ilo_read,
.write = ilo_write,
+ .poll = ilo_poll,
.open = ilo_open,
.release = ilo_close,
};
@@ -835,7 +875,7 @@ static void __exit ilo_exit(void)
class_destroy(ilo_class);
}
-MODULE_VERSION("1.1");
+MODULE_VERSION("1.2");
MODULE_ALIAS(ILO_NAME);
MODULE_DESCRIPTION(ILO_NAME);
MODULE_AUTHOR("David Altobelli <david.altobelli@hp.com>");
^ permalink raw reply [flat|nested] 6+ messages in thread