lkml.org 
[lkml]   [2007]   [Oct]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 0/2] [SCSI] Asynchronous event notification infrastructure
James Bottomley wrote:
> On Mon, 2007-10-29 at 12:24 -0400, Jeff Garzik wrote:
>> James Bottomley wrote:
>>> Ah, OK; I haven't communicated what we need very clearly. We need a way
>>> to see if the event is supported by the device, as well as a way to turn
>>> it off. For some of the events (possibly not the SATA AN one, since I
>>> know all SATA devices will be well behaved) there's going to be a need
>>> to deal with berserk or broken devices that become trigger happy, so
>>> turning off the event will be a useful (and possibly essential) way of
>>> coping.
>>
>> That's possible with the presented interface[1]:
>>
>> # see if event is supported
>> cat $path/evt_media_change
>>
>> # turn off event to deal with broken/beserk devices
>> echo 0 > $path/evt_media_change
>>
>> Some sillyhead can always do
>>
>> echo 1 > $path/evt_some_event_my_device_does_not_support
>>
>> but that will be obviously be a no-op because their device simply will
>> not send such events.
>>
>> Granted ls(1) is no longer a method for viewing supported-at-boot-time
>> list of events -- ls(1) in the presented interface lists what events the
>> _kernel_ supports, and cat(1) is used to discover which events are
>> actually enabled.
>>
>> I think that is the only difference between our two positions: [if I
>> understand you correctly] you want ls(1) to be able to list the device's
>> supported events. However, I feel that is inconsistent: for your
>> proposal, userspace must perform two checks in order to determine a
>> feature's availability: 1) does the file exist? 2) is the file context
>> non-zero?
>
> Yes, I agree ... however, open file is one op for the user -ENXIO means
> device doesn't support the event; value indicates whether the event is
> currently triggering.
>
> I just would rather we use the file exists if device supports event,
> because it's consistent with all the rest of our SCSI interfaces.

Two problems with what you just described:

1) "value indicates current event state" is a new concept in this thread
(maybe you were thinking this all along, but I didn't get that from your
writing).

Watching the sysfs node for event activity is definitely outside the
scope of this work, and IMO not very useful. The time from when LLDD
calls sdev_evt_notify() until uevent completion is very short, so the
time window for actually receiving a useful value in your scenario is
also short.

My patch presented the attributes purely as control nodes, only affected
sdev->supported_events and nothing more. You seem to be suggesting
exporting the true-for-only-a-few-milliseconds activity state, rather
then enable/disable state.


2) Event support itself is dynamic, which causes me to revisit the
"complexity" argument. In libata, for example, we only note that the
media-change event is supported after some time passes -- not in the
initial slave_config. Or error handler may disable it at runtime
because that event is problematic.

As such, that implies that the LLDD (with help from scsi_lib) is
dynamically adding and removing these attributes at runtime -- a lot
more complexity than is really needed AFAICS.

It is easy and straightforward for the driver to set a bit.

We cannot assume the state of event support bits are constant from
modprobe/slave_config time.

Jeff


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

\
 
 \ /
  Last update: 2007-10-29 17:51    [from the cache]
©2003-2011 Jasper Spaans