On Mon, 2002-10-14 at 03:06, Corey Minyard wrote:
> Yet one more update, mostly fixes for stylistic things that Randy Dunlap
> pointed out, and a bug fix that lets the KCS state machine get out of
> the "hosed" state on the next message (buggy hardware can sometimes be
> useful :-). As usual, on my home page at http://home.attbi.com/~minyard.
>
> -Corey
+static int ipmi_open(struct inode *inode, struct file *file)
..
+ priv = kmalloc(sizeof(*priv), GFP_KERNEL);
..
+ MOD_INC_USE_COUNT;
hmm. Ok so you either need the MOD_INC_USE_COUNT or you don't, but if
you do it should go before the sleeping kmalloc ;)
+static int ipmi_ioctl(struct inode *inode,
...
+ if (copy_from_user(&req, (void *) data, sizeof(req))) {
+ rv = -EFAULT;
+ break;
+ }
+
+ if (req.addr_len > sizeof(struct ipmi_addr))
+ {
+ rv = -EINVAL;
+ break;
+ }
+
+ if (copy_from_user(&addr, req.addr, req.addr_len)) {
since addr_len is a signed value, a user that passes -1 will get a
LOOOONG copy_from_user scribbling all over kernel memory...same for
data_len a few lines onwards
+static void sender(void *send_info,
..
+ if (kcs_info->run_to_completion) {
+ /* If we are running to completion, then throw it in
+ the list and run transactions until everything is
+ clear. Priority doesn't matter here. */
+ list_add_tail(&(msg->link), &(kcs_info->xmit_msgs));
+ result = kcs_event_handler(kcs_info, 0);
+ while (result != KCS_SM_IDLE) {
+ udelay(500);
+ result = kcs_event_handler(kcs_info, 500);
+ }
is that unconditional udelay with interrupts off really needed?
+/* The locking for these for a write lock is done by two locks, first
+ the "outside" lock then the normal lock. This way, the interfaces
+ lock can be converted to a read lock without allowing a new write
+ locker to come in. Note that at interrupt level, this can only be
+ claimed read, so there is no reason for read lock to save
+ interrupts. Write locks must still save interrupts because they
+ can block an interrupt. */
I get the feeling this locking is fishy. A double r/w lock smells bad.
really. Either you always take both the same way (eg both for read or
both for write) and then the inner lock could be a normal spinlock; or
you will deadlock in new and surprising ways....
+ spin_lock_irqsave(&interfaces_outside_lock, flags);
+ write_lock(&interfaces_lock);
+ for (i=0; i<MAX_IPMI_INTERFACES; i++) {
+ if (ipmi_interfaces[i] == NULL) {
+ new_intf = kmalloc(sizeof(*new_intf), GFP_KERNEL);
ehm ehm didn't just take TWO spinlocks 3 lines above this sleeping
function ?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Tue Oct 15 2002 - 22:00:49 EST