fix media change events for polled devices
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 8 lines · Linux Kernel Mailing List
[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
Quoted 4 lines · Jeff Garzik
And that is the fix -- initialize
supported_eventsaccording 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
James Bottomley
maintainerQuoted 9 lines · Linus Torvalds
Quoted 4 lines · Jeff Garzik
And that is the fix -- initialize
supported_eventsaccording 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_eventsand 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
Quoted 5 lines · James Bottomley
Quoted 4 lines · Linus Torvalds
Quoted 3 lines · Jeff Garzik
And that is the fix -- initialize
supported_eventsaccording to sr/sd's needs, and revert this change (4d1566ed2100d074ccc654e5cf2e44cdea3a01d0) as obviously broken.
Quoted 7 lines
Quoted 1 line
Patches, please? I can do the revert, but the patch to
supported_eventsand 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 <[email protected]> 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
Quoted 7 lines · Linus Torvalds
Quoted 3 lines · Jeff Garzik
And that is the fix -- initialize
supported_eventsaccording 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_eventsand the testing, please?
James was the one who wrote the incomplete/broken changes,
commit 285e9670d91cdeb6b6693729950339cb45410fdc Author: Kay Sievers <[email protected]> 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
Quoted 11 lines · Jeff Garzik
Quoted 8 lines · Linux Kernel Mailing List
[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.
Quoted 6 lines
By simply reading the code history, it is trivial to verify that this description is false:
Commit
285e9670depends ona341cd0f, so by definition it is285e9670-- or rather the incomplete update of your original patch that resulted in285e9670-- that is broken.
It worked fine with Kristen's patches, and that's where it is coming from from.
Quoted 4 lines
Since commit
285e9670is 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? :)
Quoted 8 lines
Did anyone bother to read any code at all?
When
285e9670was updated for the scsi_evt_* interface, it should have initialized thesupported_eventsmask.And that is the fix -- initialize
supported_eventsaccording 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
Quoted 11 lines · Kay Sievers
Quoted 10 lines · Jeff Garzik
Quoted 8 lines · Linux Kernel Mailing List
[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.
Quoted 6 lines
Quoted 3 lines
By simply reading the code history, it is trivial to verify that this description is false:
Commit
285e9670depends ona341cd0f, so by definition it is285e9670-- or rather the incomplete update of your original patch that resulted in285e9670-- 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.
Quoted 1 line
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.
Quoted 8 lines
Quoted 6 lines
Did anyone bother to read any code at all?
When
285e9670was updated for the scsi_evt_* interface, it should have initialized thesupported_eventsmask.And that is the fix -- initialize
supported_eventsaccording 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
Quoted 12 lines · Jeff Garzik
Quoted 11 lines · Kay Sievers
Quoted 9 lines · Jeff Garzik
Linux Kernel Mailing List wrote:
When
285e9670was updated for the scsi_evt_* interface, it should have initialized thesupported_eventsmask.And that is the fix -- initialize
supported_eventsaccording to sr/sd's needs, and revert this change (4d1566ed2100d074ccc654e5cf2e44cdea3a01d0) as obviously broken.
Quoted 5 lines
Quoted 1 line
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
James Bottomley
maintainerQuoted 27 lines · Jeff Garzik
Quoted 14 lines · Kay Sievers
Quoted 13 lines · Jeff Garzik
Quoted 8 lines · Linux Kernel Mailing List
[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:
Quoted 6 lines
Quoted 3 lines
Commit
285e9670depends ona341cd0f, so by definition it is285e9670-- or rather the incomplete update of your original patch that resulted in285e9670-- 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.
Quoted 4 lines
Quoted 1 line
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
Quoted 17 lines · James Bottomley
Quoted 16 lines · Jeff Garzik
Quoted 15 lines · Kay Sievers
Quoted 9 lines · Jeff Garzik
Quoted 8 lines · Linux Kernel Mailing List
[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.Quoted 3 lines
By simply reading the code history, it is trivial to verify that this description is false:
Commit
285e9670depends ona341cd0f, so by definition it is285e9670-- or rather the incomplete update of your original patch that resulted in285e9670-- that is broken.It worked fine with Kristen's patches, and that's where it is coming from from.
Quoted 7 lines
Quoted 5 lines
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.
Quoted 1 line
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.
Quoted 5 lines
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
James Bottomley
maintainerQuoted 18 lines · Jeff Garzik
Quoted 8 lines · Linux Kernel Mailing List
[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
285e9670depends ona341cd0f, so by definition it is285e9670-- or rather the incomplete update of your original patch that resulted in285e9670-- that is broken.
Quoted 6 lines
Since commit
285e9670is 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
Quoted 2 lines
When
285e9670was updated for the scsi_evt_* interface, it should have initialized thesupported_eventsmask.
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.
Quoted 3 lines
And that is the fix -- initialize
supported_eventsaccording to sr/sd's needs, and revert this change (4d1566ed2100d074ccc654e5cf2e44cdea3a01d0) as obviously broken.
James
Quoted 17 lines · James Bottomley
Quoted 16 lines · Jeff Garzik
Quoted 8 lines · Linux Kernel Mailing List
[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
285e9670depends ona341cd0f, so by definition it is285e9670-- or rather the incomplete update of your original patch that resulted in285e9670-- that is broken.Since commit
285e9670is broken, you fixed the wrong thing.
Quoted 8 lines
Quoted 3 lines
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).
Quoted 7 lines
The filter can be put back with a better discriminator for 2.6.26
Quoted 1 line
When
285e9670was updated for the scsi_evt_* interface, it should have initialized thesupported_eventsmask.That would break HAL ... it's using the presence of the
media_eventflag 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
James Bottomley
maintainerQuoted 26 lines · Jeff Garzik
Quoted 25 lines · James Bottomley
Quoted 23 lines · Jeff Garzik
Quoted 8 lines · Linux Kernel Mailing List
[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
285e9670depends ona341cd0f, so by definition it is285e9670-- or rather the incomplete update of your original patch that resulted in285e9670-- that is broken.Since commit
285e9670is 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?
Quoted 7 lines
Quoted 4 lines
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.
Quoted 11 lines
Quoted 8 lines
The filter can be put back with a better discriminator for 2.6.26
Quoted 2 lines
When
285e9670was updated for the scsi_evt_* interface, it should have initialized thesupported_eventsmask.That would break HAL ... it's using the presence of the
media_eventflag 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.
Quoted 2 lines
_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
Quoted 1 line · James Bottomley
It's fixed, and nothing else is additionally broken by the current fix.
Other than the newly-inconsistent, exported-to-userspace interface, ITYM.
Jeff
James Bottomley
maintainerQuoted 4 lines · Jeff Garzik
Quoted 1 line · James Bottomley
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
Quoted 8 lines · James Bottomley
Quoted 3 lines · Jeff Garzik
Quoted 1 line · James Bottomley
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
James Bottomley
maintainerQuoted 4 lines · Jeff Garzik
Quoted 3 lines · James Bottomley
Quoted 2 lines · Jeff Garzik
Quoted 1 line · James Bottomley
It's fixed, and nothing else is additionally broken by the current fix.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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.
Quoted 6 lines
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_eventsaccurately 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
Quoted 2 lines · James Bottomley
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
James Bottomley
maintainerQuoted 6 lines · Jeff Garzik
Quoted 2 lines · James Bottomley
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.
Quoted 1 line
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.
Quoted 6 lines
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.
Quoted 4 lines
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.
Quoted 1 line
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.
Quoted 6 lines
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
Quoted 10 lines · James Bottomley
The current published API is the
media_eventsfile. 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_changefile 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.
Quoted 4 lines
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
James Bottomley
maintainerQuoted 11 lines · Jeff Garzik
Quoted 10 lines · James Bottomley
The current published API is the
media_eventsfile. 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_changefile 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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 5 lines
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