Re: [SCSI] fix media change events for polled devices

Re: [SCSI] fix media change events for polled devices

[Jeff Garzik]

Linux Kernel Mailing List wrote:
>     [SCSI] fix media change events for polled devices
>     
>     Commit:
>       a341cd0f (SCSI: add asynchronous event notification API)
>     breaks:
>       285e9670 (sr,sd: send media state change modification events)
>     by introducing an event filter, which is removed here, to make
>     events, we are depending on, happen again.


By simply reading the code history, it is trivial to verify that this 
description is false:

Commit 285e9670 depends on a341cd0f, so by definition it is 285e9670 -- 
or rather the incomplete update of your original patch that resulted in 
285e9670 -- that is broken.

Since commit 285e9670 is broken, you fixed the wrong thing.

Furthermore, you broke a userspace interface that was introduced by 
a341cd0f, by removing the event filter controlled by userspace.

Did anyone bother to read any code at all?

When 285e9670 was updated for the scsi_evt_* interface, it should have 
initialized the supported_events mask.

And that is the fix -- initialize supported_events according to sr/sd's 
needs, and revert this change (4d1566ed2100d074ccc654e5cf2e44cdea3a01d0) 
as obviously broken.

	Jeff

Re: [SCSI] fix media change events for polled devices

[Linus Torvalds]

On Fri, 21 Mar 2008, Jeff Garzik wrote:
> 
> And that is the fix -- initialize supported_events according to sr/sd's needs,
> and revert this change (4d1566ed2100d074ccc654e5cf2e44cdea3a01d0) as obviously
> broken.

Patches, please? I can do the revert, but the patch to supported_events 
and the testing, please?

		Linus

Re: [SCSI] fix media change events for polled devices

[James Bottomley]

On Fri, 2008-03-21 at 10:31 -0700, Linus Torvalds wrote:
> 
> On Fri, 21 Mar 2008, Jeff Garzik wrote:
> > 
> > And that is the fix -- initialize supported_events according to sr/sd's needs,
> > and revert this change (4d1566ed2100d074ccc654e5cf2e44cdea3a01d0) as obviously
> > broken.
> 
> Patches, please? I can do the revert, but the patch to supported_events 
> and the testing, please?

I tested this and discussed it with Kristen before it went in.  The
current fix you sucked in with scsi-rc-fixes basically makes the whole
lot work for 2.6.25 (both AN media change events and traditional polled
ones).  The thing that doesn't work is the ability to turn off AN events
from user space, but that feature never worked anyway.

I promised Kay I'd work on a better solution for 2.6.26 which will
include separating up the input events into polled and AN for a CD but
emitting the same media change event to udev at the top (so you can rely
on either polled or AN for media change and select which or both if you
desire).

James

Re: [SCSI] fix media change events for polled devices

[Jeff Garzik]

James Bottomley wrote:
> On Fri, 2008-03-21 at 10:31 -0700, Linus Torvalds wrote:
>> On Fri, 21 Mar 2008, Jeff Garzik wrote:
>>> And that is the fix -- initialize supported_events according to sr/sd's needs,
>>> and revert this change (4d1566ed2100d074ccc654e5cf2e44cdea3a01d0) as obviously
>>> broken.
>> Patches, please? I can do the revert, but the patch to supported_events 
>> and the testing, please?
> 
> I tested this and discussed it with Kristen before it went in.  The
> current fix you sucked in with scsi-rc-fixes basically makes the whole
> lot work for 2.6.25 (both AN media change events and traditional polled
> ones).  The thing that doesn't work is the ability to turn off AN events
> from user space, but that feature never worked anyway.

Not true.  Change the perm with chmod...

But furthermore, change 4d1566ed renders the userspace interface 
/inconsistent/ even for the read-only case that we all acknowledge 
_does_ work.

IOW, after your "fix", the userspace interface gives an incorrect 
picture of what events are/are not masked.


If you don't have the time to complete work on

	commit 285e9670d91cdeb6b6693729950339cb45410fdc
	Author: Kay Sievers <kay.sievers@vrfy.org>
	Date:   Tue Aug 14 14:10:39 2007 +0200

	    [SCSI] sr,sd: send media state change modification events

then revert _that_.

Don't break my patch, just because yours (which came _after_ mine) 
doesn't work.

	Jeff

Re: [SCSI] fix media change events for polled devices

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Fri, 21 Mar 2008, Jeff Garzik wrote:
>> And that is the fix -- initialize supported_events according to sr/sd's needs,
>> and revert this change (4d1566ed2100d074ccc654e5cf2e44cdea3a01d0) as obviously
>> broken.
> 
> Patches, please? I can do the revert, but the patch to supported_events 
> and the testing, please?

James was the one who wrote the incomplete/broken changes,

	commit 285e9670d91cdeb6b6693729950339cb45410fdc
	Author: Kay Sievers <kay.sievers@vrfy.org>
	Date:   Tue Aug 14 14:10:39 2007 +0200

	    [SCSI] sr,sd: send media state change modification events
[...]
	[jejb: modified for new event API]

so I'm hoping he will fix it up.

I'll send a patch if he doesn't, though.

	Jeff

Re: [SCSI] fix media change events for polled devices

[Kay Sievers]

On Fri, 2008-03-21 at 13:12 -0400, Jeff Garzik wrote:
> Linux Kernel Mailing List wrote:
> >     [SCSI] fix media change events for polled devices
> >     
> >     Commit:
> >       a341cd0f (SCSI: add asynchronous event notification API)
> >     breaks:
> >       285e9670 (sr,sd: send media state change modification events)
> >     by introducing an event filter, which is removed here, to make
> >     events, we are depending on, happen again.
> 
> 
> By simply reading the code history, it is trivial to verify that this 
> description is false:
> 
> Commit 285e9670 depends on a341cd0f, so by definition it is 285e9670 -- 
> or rather the incomplete update of your original patch that resulted in 
> 285e9670 -- that is broken.

It worked fine with Kristen's patches, and that's where it is coming
from from.

> Since commit 285e9670 is broken, you fixed the wrong thing.
> 
> Furthermore, you broke a userspace interface that was introduced by 
> a341cd0f, by removing the event filter controlled by userspace.

You mean the read-only sysfs attribute? :)

> Did anyone bother to read any code at all?
> 
> When 285e9670 was updated for the scsi_evt_* interface, it should have 
> initialized the supported_events mask.
> 
> And that is the fix -- initialize supported_events according to sr/sd's 
> needs, and revert this change (4d1566ed2100d074ccc654e5cf2e44cdea3a01d0) 
> as obviously broken.

You mean adding a new flag? As every device will "support" these events.

Thanks,
Kay

Re: [SCSI] fix media change events for polled devices

[Jeff Garzik]

Kay Sievers wrote:
> On Fri, 2008-03-21 at 13:12 -0400, Jeff Garzik wrote:
>> Linux Kernel Mailing List wrote:
>>>     [SCSI] fix media change events for polled devices
>>>     
>>>     Commit:
>>>       a341cd0f (SCSI: add asynchronous event notification API)
>>>     breaks:
>>>       285e9670 (sr,sd: send media state change modification events)
>>>     by introducing an event filter, which is removed here, to make
>>>     events, we are depending on, happen again.
>>
>> By simply reading the code history, it is trivial to verify that this 
>> description is false:
>>
>> Commit 285e9670 depends on a341cd0f, so by definition it is 285e9670 -- 
>> or rather the incomplete update of your original patch that resulted in 
>> 285e9670 -- that is broken.
> 
> It worked fine with Kristen's patches, and that's where it is coming
> from from.

Neither her patches nor yours went upstream verbatim at version one. 
You need to look at what happens upstream, not what happened in your 
private testing six months ago.


> You mean the read-only sysfs attribute? :)

I mean the attribute with both 'show' (read) and 'store' (write) 
functions.  The perms do need to change, thanks for noticing that bug.


> 
>> Did anyone bother to read any code at all?
>>
>> When 285e9670 was updated for the scsi_evt_* interface, it should have 
>> initialized the supported_events mask.
>>
>> And that is the fix -- initialize supported_events according to sr/sd's 
>> needs, and revert this change (4d1566ed2100d074ccc654e5cf2e44cdea3a01d0) 
>> as obviously broken.
> 
> You mean adding a new flag? As every device will "support" these events.

Does every device attached to every controller wish to fire these events?

If so, then that wants new flag, yes.

	Jeff

Re: [SCSI] fix media change events for polled devices

[Kay Sievers]

On Fri, 2008-03-21 at 16:42 -0400, Jeff Garzik wrote:
> Kay Sievers wrote:
> > On Fri, 2008-03-21 at 13:12 -0400, Jeff Garzik wrote:
> >> Linux Kernel Mailing List wrote:


> >> When 285e9670 was updated for the scsi_evt_* interface, it should have 
> >> initialized the supported_events mask.
> >>
> >> And that is the fix -- initialize supported_events according to sr/sd's 
> >> needs, and revert this change (4d1566ed2100d074ccc654e5cf2e44cdea3a01d0) 
> >> as obviously broken.
> > 
> > You mean adding a new flag? As every device will "support" these events.
> 
> Does every device attached to every controller wish to fire these events?
> 
> If so, then that wants new flag, yes.

Yes, a new flag sounds good. Every device that detects a media change
should send such event, so userspace can possibly update it's
representation of the volume.
That way, userspace can switch to a single generic event, regardless if
the device is periodically polled, or supports AN.

Thanks,
Kay

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [SCSI] fix media change events for polled devices

[Kay Sievers]

On Fri, 2008-03-21 at 16:42 -0400, Jeff Garzik wrote:
> Kay Sievers wrote:
> > On Fri, 2008-03-21 at 13:12 -0400, Jeff Garzik wrote:
> >> Linux Kernel Mailing List wrote:


> >> When 285e9670 was updated for the scsi_evt_* interface, it should have 
> >> initialized the supported_events mask.
> >>
> >> And that is the fix -- initialize supported_events according to sr/sd's 
> >> needs, and revert this change (4d1566ed2100d074ccc654e5cf2e44cdea3a01d0) 
> >> as obviously broken.
> > 
> > You mean adding a new flag? As every device will "support" these events.
> 
> Does every device attached to every controller wish to fire these events?
> 
> If so, then that wants new flag, yes.

Yes, a new flag sounds good. Every device that detects a media change
should send such event, so userspace can possibly update it's
representation of the volume.
That way, userspace can switch to a single generic event, regardless if
the device is periodically polled, or supports AN.

Thanks,
Kay

Re: [SCSI] fix media change events for polled devices

[James Bottomley]

On Fri, 2008-03-21 at 16:42 -0400, Jeff Garzik wrote:
> Kay Sievers wrote:
> > On Fri, 2008-03-21 at 13:12 -0400, Jeff Garzik wrote:
> >> Linux Kernel Mailing List wrote:
> >>>     [SCSI] fix media change events for polled devices
> >>>     
> >>>     Commit:
> >>>       a341cd0f (SCSI: add asynchronous event notification API)
> >>>     breaks:
> >>>       285e9670 (sr,sd: send media state change modification events)
> >>>     by introducing an event filter, which is removed here, to make
> >>>     events, we are depending on, happen again.
> >>
> >> By simply reading the code history, it is trivial to verify that this 
> >> description is false:
> >>
> >> Commit 285e9670 depends on a341cd0f, so by definition it is 285e9670 -- 
> >> or rather the incomplete update of your original patch that resulted in 
> >> 285e9670 -- that is broken.
> > 
> > It worked fine with Kristen's patches, and that's where it is coming
> > from from.
> 
> Neither her patches nor yours went upstream verbatim at version one. 
> You need to look at what happens upstream, not what happened in your 
> private testing six months ago.
> 
> 
> > You mean the read-only sysfs attribute? :)
> 
> I mean the attribute with both 'show' (read) and 'store' (write) 
> functions.  The perms do need to change, thanks for noticing that bug.

That's not a bug.

For starters, we have transport classes that provide generic store
methods but can't pass the information on to drivers.  For these, we set
the attribute to read only even if there is a store method.  Even if
that weren't the case, how do you know which of UGO wants the write
setting?  Finally, if you look at the sysfs code, the place where mode
settings of the sysfs files are handled doesn't see the show/store
methods at all (these are handled at a different layering within the
generic driver model).

James

Re: [SCSI] fix media change events for polled devices

[Jeff Garzik]

James Bottomley wrote:
> On Fri, 2008-03-21 at 16:42 -0400, Jeff Garzik wrote:
>> Kay Sievers wrote:
>>> On Fri, 2008-03-21 at 13:12 -0400, Jeff Garzik wrote:
>>>> Linux Kernel Mailing List wrote:
>>>>>     [SCSI] fix media change events for polled devices
>>>>>     
>>>>>     Commit:
>>>>>       a341cd0f (SCSI: add asynchronous event notification API)
>>>>>     breaks:
>>>>>       285e9670 (sr,sd: send media state change modification events)
>>>>>     by introducing an event filter, which is removed here, to make
>>>>>     events, we are depending on, happen again.
>>>> By simply reading the code history, it is trivial to verify that this 
>>>> description is false:
>>>>
>>>> Commit 285e9670 depends on a341cd0f, so by definition it is 285e9670 -- 
>>>> or rather the incomplete update of your original patch that resulted in 
>>>> 285e9670 -- that is broken.
>>> It worked fine with Kristen's patches, and that's where it is coming
>>> from from.
>> Neither her patches nor yours went upstream verbatim at version one. 
>> You need to look at what happens upstream, not what happened in your 
>> private testing six months ago.
>>
>>
>>> You mean the read-only sysfs attribute? :)
>> I mean the attribute with both 'show' (read) and 'store' (write) 
>> functions.  The perms do need to change, thanks for noticing that bug.
> 
> That's not a bug.

Yes, it is.  That sysfs node was intended to be writable.


> For starters, we have transport classes that provide generic store
> methods but can't pass the information on to drivers.  For these, we set
> the attribute to read only even if there is a store method.  Even if
> that weren't the case, how do you know which of UGO wants the write
> setting?

You give it to root, and let userspace change it after that, like we 
always do.

	Jeff

Re: [SCSI] fix media change events for polled devices

[James Bottomley]

On Fri, 2008-03-21 at 13:12 -0400, Jeff Garzik wrote:
> Linux Kernel Mailing List wrote:
> >     [SCSI] fix media change events for polled devices
> >     
> >     Commit:
> >       a341cd0f (SCSI: add asynchronous event notification API)
> >     breaks:
> >       285e9670 (sr,sd: send media state change modification events)
> >     by introducing an event filter, which is removed here, to make
> >     events, we are depending on, happen again.
> 
> 
> By simply reading the code history, it is trivial to verify that this 
> description is false:
> 
> Commit 285e9670 depends on a341cd0f, so by definition it is 285e9670 -- 
> or rather the incomplete update of your original patch that resulted in 
> 285e9670 -- that is broken.
> 
> Since commit 285e9670 is broken, you fixed the wrong thing.
> 
> Furthermore, you broke a userspace interface that was introduced by 
> a341cd0f, by removing the event filter controlled by userspace.

> Did anyone bother to read any code at all?

Yes, I did.  The userspace interface has never worked as a writeable
attribute because it's read only (as Kay has pointed out several times).
Based on that, I just #if'd out the filter code to get polled media
events working again.

The filter can be put back with a better discriminator for 2.6.26

> When 285e9670 was updated for the scsi_evt_* interface, it should have 
> initialized the supported_events mask.

That would break HAL ... it's using the presence of the media_event flag
in the variable to indicate it doesn't need to poll.  We need two flags,
one for polling and one for AN.

> And that is the fix -- initialize supported_events according to sr/sd's 
> needs, and revert this change (4d1566ed2100d074ccc654e5cf2e44cdea3a01d0) 
> as obviously broken.

James

Re: [SCSI] fix media change events for polled devices

[Jeff Garzik]

James Bottomley wrote:
> On Fri, 2008-03-21 at 13:12 -0400, Jeff Garzik wrote:
>> Linux Kernel Mailing List wrote:
>>>     [SCSI] fix media change events for polled devices
>>>     
>>>     Commit:
>>>       a341cd0f (SCSI: add asynchronous event notification API)
>>>     breaks:
>>>       285e9670 (sr,sd: send media state change modification events)
>>>     by introducing an event filter, which is removed here, to make
>>>     events, we are depending on, happen again.
>>
>> By simply reading the code history, it is trivial to verify that this 
>> description is false:
>>
>> Commit 285e9670 depends on a341cd0f, so by definition it is 285e9670 -- 
>> or rather the incomplete update of your original patch that resulted in 
>> 285e9670 -- that is broken.
>>
>> Since commit 285e9670 is broken, you fixed the wrong thing.
>>
>> Furthermore, you broke a userspace interface that was introduced by 
>> a341cd0f, by removing the event filter controlled by userspace.
> 
>> Did anyone bother to read any code at all?
> 
> Yes, I did.  The userspace interface has never worked as a writeable
> attribute because it's read only (as Kay has pointed out several times).
> Based on that, I just #if'd out the filter code to get polled media
> events working again.

It has a show attribute and was obviously intended as a writable 
interface (and I should know, I wrote it).


> The filter can be put back with a better discriminator for 2.6.26
> 
>> When 285e9670 was updated for the scsi_evt_* interface, it should have 
>> initialized the supported_events mask.
> 
> That would break HAL ... it's using the presence of the media_event flag
> in the variable to indicate it doesn't need to poll.  We need two flags,
> one for polling and one for AN.

Oh for pete's sake.  How hard is it to create a second bit _and_ keep 
the existing AN featureset working?

_YOU_ are the one that broke this stuff in the first place, can you 
please take two seconds to fix it without breaking other stuff?

	Jeff

Re: [SCSI] fix media change events for polled devices

[James Bottomley]

On Fri, 2008-03-21 at 16:36 -0400, Jeff Garzik wrote:
> James Bottomley wrote:
> > On Fri, 2008-03-21 at 13:12 -0400, Jeff Garzik wrote:
> >> Linux Kernel Mailing List wrote:
> >>>     [SCSI] fix media change events for polled devices
> >>>     
> >>>     Commit:
> >>>       a341cd0f (SCSI: add asynchronous event notification API)
> >>>     breaks:
> >>>       285e9670 (sr,sd: send media state change modification events)
> >>>     by introducing an event filter, which is removed here, to make
> >>>     events, we are depending on, happen again.
> >>
> >> By simply reading the code history, it is trivial to verify that this 
> >> description is false:
> >>
> >> Commit 285e9670 depends on a341cd0f, so by definition it is 285e9670 -- 
> >> or rather the incomplete update of your original patch that resulted in 
> >> 285e9670 -- that is broken.
> >>
> >> Since commit 285e9670 is broken, you fixed the wrong thing.
> >>
> >> Furthermore, you broke a userspace interface that was introduced by 
> >> a341cd0f, by removing the event filter controlled by userspace.
> > 
> >> Did anyone bother to read any code at all?
> > 
> > Yes, I did.  The userspace interface has never worked as a writeable
> > attribute because it's read only (as Kay has pointed out several times).
> > Based on that, I just #if'd out the filter code to get polled media
> > events working again.
> 
> It has a show attribute and was obviously intended as a writable 
> interface (and I should know, I wrote it).

It's all the fault of those annoying computer systems that do what we
tell them rather than what we intend.  In this case sysfs won't allow
you to write a read only attribute even if it does have a store method.

> > The filter can be put back with a better discriminator for 2.6.26
> > 
> >> When 285e9670 was updated for the scsi_evt_* interface, it should have 
> >> initialized the supported_events mask.
> > 
> > That would break HAL ... it's using the presence of the media_event flag
> > in the variable to indicate it doesn't need to poll.  We need two flags,
> > one for polling and one for AN.
> 
> Oh for pete's sake.  How hard is it to create a second bit _and_ keep 
> the existing AN featureset working?

The existing feature set currently works, even with the event filter
removed, so it's the fastest and smallest bug fix to get polling to
work.  I'm just pointing out that your suggested fix wouldn't work ... I
think at -rc6 the more complex one involving two bits is really a
feature enhancement.

> _YOU_ are the one that broke this stuff in the first place, can you 
> please take two seconds to fix it without breaking other stuff?

It's fixed, and nothing else is additionally broken by the current fix.

James

Re: [SCSI] fix media change events for polled devices

[Jeff Garzik]

James Bottomley wrote:
> It's fixed, and nothing else is additionally broken by the current fix.

Other than the newly-inconsistent, exported-to-userspace interface, ITYM.

	Jeff

Re: [SCSI] fix media change events for polled devices

[James Bottomley]

On Fri, 2008-03-21 at 17:04 -0400, Jeff Garzik wrote:
> James Bottomley wrote:
> > It's fixed, and nothing else is additionally broken by the current fix.
> 
> Other than the newly-inconsistent, exported-to-userspace interface, ITYM.

How is it newly inconsistent?  It can still be used in the manner you
intended it, namely to tell if the CD supports AN or not.  When we add
the new bit, we'll have to add a new file for it anyway.

James

Re: [SCSI] fix media change events for polled devices

[Jeff Garzik]

James Bottomley wrote:
> On Fri, 2008-03-21 at 17:04 -0400, Jeff Garzik wrote:
>> James Bottomley wrote:
>>> It's fixed, and nothing else is additionally broken by the current fix.
>> Other than the newly-inconsistent, exported-to-userspace interface, ITYM.
> 
> How is it newly inconsistent?  It can still be used in the manner you
> intended it, namely to tell if the CD supports AN or not.  When we add
> the new bit, we'll have to add a new file for it anyway.

Previously, the events could be expected to be sent (or sent) in strict 
accordance with supported_events.

Now, you are essentially sending out-of-band events, because the 
receiver requested supported_events -- but is getting much more than that!

You broke a fundamental assumption of the interface -- that 
supported_events accurately and fully represented all events sent via 
the new API.

	Jeff

Re: [SCSI] fix media change events for polled devices

[James Bottomley]

On Fri, 2008-03-21 at 17:13 -0400, Jeff Garzik wrote:
> James Bottomley wrote:
> > On Fri, 2008-03-21 at 17:04 -0400, Jeff Garzik wrote:
> >> James Bottomley wrote:
> >>> It's fixed, and nothing else is additionally broken by the current fix.
> >> Other than the newly-inconsistent, exported-to-userspace interface, ITYM.
> > 
> > How is it newly inconsistent?  It can still be used in the manner you
> > intended it, namely to tell if the CD supports AN or not.  When we add
> > the new bit, we'll have to add a new file for it anyway.
> 
> Previously, the events could be expected to be sent (or sent) in strict 
> accordance with supported_events.

we don't publish supported_events; we publish a single file called
media_change.  Right at the moment its meaning is I have a 1 if AN is
supported and 0 if it's not.

Before the fix, its meaning was  1 if AN is supported and 0 if it's not.

The file name now looks wrong ... it should probably be an_media_change
or something if we have to introduce a new file called poll_media_chage
to distinguish them ... but nothing's changed about the way it works.

> Now, you are essentially sending out-of-band events, because the 
> receiver requested supported_events -- but is getting much more than that!
> 
> You broke a fundamental assumption of the interface -- that 
> supported_events accurately and fully represented all events sent via 
> the new API.

Anyway, realistically, since no CD or DVD on the market today seems to
support the AHCI AN method, this argument is really moot ... we just
need polling to work.

James

Re: [SCSI] fix media change events for polled devices

[Jeff Garzik]

James Bottomley wrote:
> Anyway, realistically, since no CD or DVD on the market today seems to
> support the AHCI AN method, this argument is really moot ...

No, it's not.  This information is published to userspace EVEN WHEN AN 
SUPPORT IS ABSENT.  As any sane interface would do -- report a zero value.

Thus, the interface is useful even in the absence of AN.

Anyway, to recap...

before your "fix":

	userspace interface always reflected list of events sent
	via my new API

after your "fix":

	events may or may not be reflected in userspace interface,
	who knows?

Is that distinction so difficult to see?

This interface, like it or not, is in 2.6.24, which means its published 
and "out there."

This is a clear regression from 2.6.24.

supported_events' value was accurate in 2.6.24.  Now it is not.

	Jeff

Re: [SCSI] fix media change events for polled devices

[James Bottomley]

On Fri, 2008-03-21 at 18:35 -0400, Jeff Garzik wrote:
> James Bottomley wrote:
> > Anyway, realistically, since no CD or DVD on the market today seems to
> > support the AHCI AN method, this argument is really moot ...
> 
> No, it's not.  This information is published to userspace EVEN WHEN AN 
> SUPPORT IS ABSENT.  As any sane interface would do -- report a zero value.

Right, it still does this.  Currently hal is taking 0 in the
media_change file to mean I don't support AN.

> Thus, the interface is useful even in the absence of AN.

Yes, that's why we're having the argument ... it's the use of the event
in the absence of AN that people care about.

> Anyway, to recap...
> 
> before your "fix":
> 
> 	userspace interface always reflected list of events sent
> 	via my new API

It didn't show a list to userspace ... it's a single file with a 0 or 1
value.  0 means doesn't support AN, 1 means does.

> after your "fix":
> 
> 	events may or may not be reflected in userspace interface,
> 	who knows?

It's still a single file with a 0 or 1 value.  0 means doesn't support
AN, 1 means does.

> Is that distinction so difficult to see?

Well, if they have the same userspace effect, and display the same
information to userspace, it's a bit hard to see how a user would
distinguish them, yes.

> This interface, like it or not, is in 2.6.24, which means its published 
> and "out there."
> 
> This is a clear regression from 2.6.24.
> 
> supported_events' value was accurate in 2.6.24.  Now it is not.

The current published API is the media_events file.  HAL is using that
to indicate support for AN.  This is why we can't simply change it to 1
wholesale because we'll confuse HAL (HAL still has to send polling
events if AN isn't supported).

So, the best fix for 2.6.25 at the current -rc6 is to keep the meaning
of the media_change file the same (0 for no AN, 1 for AN) and let HAL
take the polled events via udev, which basically means it's preserving
the behaviour and isn't a regression.

For 2.6.26 we can add a new media_events_polled (or some other name)
file, fix the sysfs ro attribute and make them true writeable filters so
some raving user can turn off polled events if they want and everyone
will be happy.

James

Re: [SCSI] fix media change events for polled devices

[Jeff Garzik]

James Bottomley wrote:
> The current published API is the media_events file.  HAL is using that
> to indicate support for AN.  This is why we can't simply change it to 1
> wholesale because we'll confuse HAL (HAL still has to send polling
> events if AN isn't supported).
> 
> So, the best fix for 2.6.25 at the current -rc6 is to keep the meaning
> of the media_change file the same (0 for no AN, 1 for AN) and let HAL
> take the polled events via udev, which basically means it's preserving
> the behaviour and isn't a regression.
> 
> For 2.6.26 we can add a new media_events_polled (or some other name)
> file, fix the sysfs ro attribute and make them true writeable filters so
> some raving user can turn off polled events if they want and everyone
> will be happy.


So version 3 of the interface will be the first stable and usable one... 
sigh :/

It's just disheartening that the userspace filtering stuff (including 
interface) was disabled rather than fixed, given that that change came 
first and arguably the follow-on change (285e9670) was an abuse of the 
API that was never corrected -- which seems to be tacitly acknowledged 
since everyone seems to agree more than one flag is needed.

	Jeff

Re: [SCSI] fix media change events for polled devices

[James Bottomley]

On Mon, 2008-03-24 at 23:09 -0400, Jeff Garzik wrote:
> James Bottomley wrote:
> > The current published API is the media_events file.  HAL is using that
> > to indicate support for AN.  This is why we can't simply change it to 1
> > wholesale because we'll confuse HAL (HAL still has to send polling
> > events if AN isn't supported).
> > 
> > So, the best fix for 2.6.25 at the current -rc6 is to keep the meaning
> > of the media_change file the same (0 for no AN, 1 for AN) and let HAL
> > take the polled events via udev, which basically means it's preserving
> > the behaviour and isn't a regression.
> > 
> > For 2.6.26 we can add a new media_events_polled (or some other name)
> > file, fix the sysfs ro attribute and make them true writeable filters so
> > some raving user can turn off polled events if they want and everyone
> > will be happy.
> 
> 
> So version 3 of the interface will be the first stable and usable one... 
> sigh :/

Well, it's open source ... this interface has had a better passage than
some of the block ones I could name.

> It's just disheartening that the userspace filtering stuff (including 
> interface) was disabled rather than fixed, given that that change came 
> first and arguably the follow-on change (285e9670) was an abuse of the 
> API that was never corrected -- which seems to be tacitly acknowledged 
> since everyone seems to agree more than one flag is needed.

I think the current hack is the simplest way to fix up 2.6.25, given the
lateness in the -rc cycle ... we can do a far better job in 2.6.26 and
still be backwards compatible with the 2.6.25 hack.  In fact, I
committed to Kay that we would ... although if you want to do it
instead, that would be easier for me.

Thanks,

James