RE: [PATCH][v5] HP iLO driver

[Altobelli, David]

Andrew Morton wrote:
> On Wed, 2 Jul 2008 09:38:53 -0600 David Altobelli
> <david.altobelli@xxxxxx> wrote:
>
>>
>> +static ssize_t ilo_write(struct file *fp, const char __user *buf,
>> +                      size_t len, loff_t *off)
>> +{
>> +     int err, pkt_id, pkt_len;
>> +     struct ccb_data *data;
>> +     struct ccb *driver_ccb;
>> +     struct ilo_hwinfo *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
>> +              * need to close and reopen all ccbs. +              */
>> +             ilo_reset(hw);
>> +             return -ENODEV;
>> +     }
>
> Can multiple processes open and write to this "device"?  If so, the
> above looks awfully racy.  Perhaps vfs-level i_mutex coverage will
> save us on the write-versus-write side, but not on write-versus-read.

Yes, the channels can be shared.

I'm probably missing the race you are seeing, but is_device_reset()
checks a hardware bit, and this hw bit is only cleared after the flag
checked in is_channel_reset() is set.  Multiple processes could get into
ilo_reset(), but that function rechecks the hardware bit after taking a
lock (and I don't believe it could happen, but there is no real downside
to multiple setters of the reset flag).  Also, this bit of code is only
there to tell the applications that they need to close and reopen channels,
by returning -ENODEV.  Reading/writing from a reset channel will not cause
problems, the read or write will simply not succeed.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/