* Re: [PATCH] udev: Allow ALSA input jacks to be accessed by the current
From: David Henningsson @ 2011-06-17 6:17 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <4DFA0F40.7000204@canonical.com>
On 2011-06-16 16:48, Takashi Iwai wrote:
> At Thu, 16 Jun 2011 16:28:08 +0200,
> Kay Sievers wrote:
>>
>> On Thu, Jun 16, 2011 at 16:12, David Henningsson
>> <david.henningsson@canonical.com> wrote:
>>> One missing piece for userspace (PulseAudio etc) to actually be able to use
>>> the jack input devices that ALSA create, is that these devices are
>>> accessible by root only. This patch makes the input device nodes accessible
>>> by the same users that can access the sound card: the current logged in
>>> user, as well as users in the audio group.
>>>
>>> One thing I was thinking about, was that the udev-acl rule actually grants
>>> read-write access to the input device node, where probably only read access
>>> is needed. Is this dangerous?
>>
>> Takashi, wasn't there already something else to use from ALSA than the
>> artificial input devices?
>
> These input devices are just notification from the sound driver at
> jack plugging, so basically it's read-only indeed, and setting the
> file permission RO would make sense.
>
> I can't judge more since I haven't seen the patch.
It was posted to alsa-devel (for comments) two days ago, see:
http://mailman.alsa-project.org/pipermail/alsa-devel/2011-June/040916.html
and
http://mailman.alsa-project.org/pipermail/alsa-devel/2011-June/040917.html
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
^ permalink raw reply
* Re: [PATCH] udev: Allow ALSA input jacks to be accessed by the current user
From: Takashi Iwai @ 2011-06-17 6:20 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <4DFA0F40.7000204@canonical.com>
At Fri, 17 Jun 2011 08:17:07 +0200,
David Henningsson wrote:
>
> On 2011-06-16 16:48, Takashi Iwai wrote:
> > At Thu, 16 Jun 2011 16:28:08 +0200,
> > Kay Sievers wrote:
> >>
> >> On Thu, Jun 16, 2011 at 16:12, David Henningsson
> >> <david.henningsson@canonical.com> wrote:
> >>> One missing piece for userspace (PulseAudio etc) to actually be able to use
> >>> the jack input devices that ALSA create, is that these devices are
> >>> accessible by root only. This patch makes the input device nodes accessible
> >>> by the same users that can access the sound card: the current logged in
> >>> user, as well as users in the audio group.
> >>>
> >>> One thing I was thinking about, was that the udev-acl rule actually grants
> >>> read-write access to the input device node, where probably only read access
> >>> is needed. Is this dangerous?
> >>
> >> Takashi, wasn't there already something else to use from ALSA than the
> >> artificial input devices?
> >
> > These input devices are just notification from the sound driver at
> > jack plugging, so basically it's read-only indeed, and setting the
> > file permission RO would make sense.
> >
> > I can't judge more since I haven't seen the patch.
>
> It was posted to alsa-devel (for comments) two days ago, see:
>
> http://mailman.alsa-project.org/pipermail/alsa-devel/2011-June/040916.html
>
> and
>
> http://mailman.alsa-project.org/pipermail/alsa-devel/2011-June/040917.html
Thanks, I see it now.
Takashi
^ permalink raw reply
* Re: [PATCH] udev: Allow ALSA input jacks to be accessed by the current
From: David Henningsson @ 2011-06-17 11:27 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <4DFA0F40.7000204@canonical.com>
On 2011-06-16 17:02, Takashi Iwai wrote:
> At Thu, 16 Jun 2011 16:55:39 +0200,
> Kay Sievers wrote:
>>
>> On Thu, Jun 16, 2011 at 16:48, Takashi Iwai<tiwai@suse.de> wrote:
>>> At Thu, 16 Jun 2011 16:28:08 +0200,
>>> Kay Sievers wrote:
>>>>
>>>> On Thu, Jun 16, 2011 at 16:12, David Henningsson
>>>> <david.henningsson@canonical.com> wrote:
>>>>> One missing piece for userspace (PulseAudio etc) to actually be able to use
>>>>> the jack input devices that ALSA create, is that these devices are
>>>>> accessible by root only. This patch makes the input device nodes accessible
>>>>> by the same users that can access the sound card: the current logged in
>>>>> user, as well as users in the audio group.
>>>>>
>>>>> One thing I was thinking about, was that the udev-acl rule actually grants
>>>>> read-write access to the input device node, where probably only read access
>>>>> is needed. Is this dangerous?
>>>>
>>>> Takashi, wasn't there already something else to use from ALSA than the
>>>> artificial input devices?
>>>
>>> These input devices are just notification from the sound driver at
>>> jack plugging, so basically it's read-only indeed, and setting the
>>> file permission RO would make sense.
So the problem is that there is no way to accomplish that in
70-acl.rules right now. Would it be some kind of security flaw if we
allowed write access? What can you really do/damage by writing to these
things?
>> Na, I mean, I remember you talking about some other channel of
>> notifications about jack changes.
>
> Ah, yeah, it's still possible to implement via ALSA control API, too.
> Just add a new control element for jack notification, and the apps can
> listen to it via normal ALSA API like the mixer elements.
>
>> Didn't the input devices get replaced by something on the control
>> device? Or was that just a plan?
>
> I had some patches for it years ago, but never got merged.
> If apps prefer it, I can revisit it. Meanwhile, the jack-input stuff
> is already present, so it might not be worth for it.
As for which architecture works better, I'm not sure. Anyway, my point
is, as this is what ALSA currently uses, this is what we should support
all the way up the stack. If ALSA switches architecture, let's revisit
this patch at that point. (Btw, there are already an experimental set of
patches available for PulseAudio that polls for these input events.)
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
^ permalink raw reply
* Re: [PATCH] udev: Allow ALSA input jacks to be accessed by the
From: Lennart Poettering @ 2011-06-17 12:31 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <4DFA0F40.7000204@canonical.com>
On Fri, 17.06.11 13:27, David Henningsson (david.henningsson@canonical.com) wrote:
> On 2011-06-16 17:02, Takashi Iwai wrote:
> >At Thu, 16 Jun 2011 16:55:39 +0200,
> >Kay Sievers wrote:
> >>
> >>On Thu, Jun 16, 2011 at 16:48, Takashi Iwai<tiwai@suse.de> wrote:
> >>>At Thu, 16 Jun 2011 16:28:08 +0200,
> >>>Kay Sievers wrote:
> >>>>
> >>>>On Thu, Jun 16, 2011 at 16:12, David Henningsson
> >>>><david.henningsson@canonical.com> wrote:
> >>>>>One missing piece for userspace (PulseAudio etc) to actually be able to use
> >>>>>the jack input devices that ALSA create, is that these devices are
> >>>>>accessible by root only. This patch makes the input device nodes accessible
> >>>>>by the same users that can access the sound card: the current logged in
> >>>>>user, as well as users in the audio group.
> >>>>>
> >>>>>One thing I was thinking about, was that the udev-acl rule actually grants
> >>>>>read-write access to the input device node, where probably only read access
> >>>>>is needed. Is this dangerous?
> >>>>
> >>>>Takashi, wasn't there already something else to use from ALSA than the
> >>>>artificial input devices?
> >>>
> >>>These input devices are just notification from the sound driver at
> >>>jack plugging, so basically it's read-only indeed, and setting the
> >>>file permission RO would make sense.
>
> So the problem is that there is no way to accomplish that in
> 70-acl.rules right now. Would it be some kind of security flaw if we
> allowed write access? What can you really do/damage by writing to
> these things?
>
> >>Na, I mean, I remember you talking about some other channel of
> >>notifications about jack changes.
> >
> >Ah, yeah, it's still possible to implement via ALSA control API, too.
> >Just add a new control element for jack notification, and the apps can
> >listen to it via normal ALSA API like the mixer elements.
Takashi, what's the perspective on this? Your patches, would they
convert *all* existing drivers to this new interfaces, or just HDA?
If we add new infrastructure to udev (and PA), does it really make sense
to that with an interface that is suboptimal and is going to be replaced
very quickly anyway?
If you have the patches ready to convert all drivers which currently can
do jack sensing over to the new scheme with control elements, then we
should probably focus on that for the future, shouldn't we?
Lennart
--
Lennart Poettering - Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH] udev: Allow ALSA input jacks to be accessed by the current user
From: Takashi Iwai @ 2011-06-17 12:42 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <4DFA0F40.7000204@canonical.com>
At Fri, 17 Jun 2011 14:31:48 +0200,
Lennart Poettering wrote:
>
> On Fri, 17.06.11 13:27, David Henningsson (david.henningsson@canonical.com) wrote:
>
> > On 2011-06-16 17:02, Takashi Iwai wrote:
> > >At Thu, 16 Jun 2011 16:55:39 +0200,
> > >Kay Sievers wrote:
> > >>
> > >>On Thu, Jun 16, 2011 at 16:48, Takashi Iwai<tiwai@suse.de> wrote:
> > >>>At Thu, 16 Jun 2011 16:28:08 +0200,
> > >>>Kay Sievers wrote:
> > >>>>
> > >>>>On Thu, Jun 16, 2011 at 16:12, David Henningsson
> > >>>><david.henningsson@canonical.com> wrote:
> > >>>>>One missing piece for userspace (PulseAudio etc) to actually be able to use
> > >>>>>the jack input devices that ALSA create, is that these devices are
> > >>>>>accessible by root only. This patch makes the input device nodes accessible
> > >>>>>by the same users that can access the sound card: the current logged in
> > >>>>>user, as well as users in the audio group.
> > >>>>>
> > >>>>>One thing I was thinking about, was that the udev-acl rule actually grants
> > >>>>>read-write access to the input device node, where probably only read access
> > >>>>>is needed. Is this dangerous?
> > >>>>
> > >>>>Takashi, wasn't there already something else to use from ALSA than the
> > >>>>artificial input devices?
> > >>>
> > >>>These input devices are just notification from the sound driver at
> > >>>jack plugging, so basically it's read-only indeed, and setting the
> > >>>file permission RO would make sense.
> >
> > So the problem is that there is no way to accomplish that in
> > 70-acl.rules right now. Would it be some kind of security flaw if we
> > allowed write access? What can you really do/damage by writing to
> > these things?
> >
> > >>Na, I mean, I remember you talking about some other channel of
> > >>notifications about jack changes.
> > >
> > >Ah, yeah, it's still possible to implement via ALSA control API, too.
> > >Just add a new control element for jack notification, and the apps can
> > >listen to it via normal ALSA API like the mixer elements.
>
> Takashi, what's the perspective on this? Your patches, would they
> convert *all* existing drivers to this new interfaces, or just HDA?
So far, only HD-audio and ASoC drivers provide the jack-sensing
notification. My patch didn't change ASoC drivers, so it's only about
HD-audio.
> If we add new infrastructure to udev (and PA), does it really make sense
> to that with an interface that is suboptimal and is going to be replaced
> very quickly anyway?
We've had already this infrastructure for years, but mostly used by
ASoC, so basically independently from desktops, thus they had little
care about the file permissions.
> If you have the patches ready to convert all drivers which currently can
> do jack sensing over to the new scheme with control elements, then we
> should probably focus on that for the future, shouldn't we?
For PA work, I suppose so (although I won't object to someone working
on the implementation with the present input-jack layer :)
But you can see David's patch basically independent from PA.
thanks,
Takashi
^ permalink raw reply
* Re: [PATCH] udev: Allow ALSA input jacks to be accessed by the
From: Lennart Poettering @ 2011-06-17 12:45 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <4DFA0F40.7000204@canonical.com>
On Fri, 17.06.11 14:42, Takashi Iwai (tiwai@suse.de) wrote:
> > If you have the patches ready to convert all drivers which currently can
> > do jack sensing over to the new scheme with control elements, then we
> > should probably focus on that for the future, shouldn't we?
>
> For PA work, I suppose so (although I won't object to someone working
> on the implementation with the present input-jack layer :)
> But you can see David's patch basically independent from PA.
Well, but I'd guess that the access permissions only really matter for
PA, right? So if we say that proper jack sensing via control elements is
the way to go, then PA should adopt this new scheme and this new scheme
only, and not bother with the input device cruft, and the udev patch
should not be merged either as PA would be its only consumer...
Lennart
--
Lennart Poettering - Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH] udev: Allow ALSA input jacks to be accessed by the current user
From: Takashi Iwai @ 2011-06-17 12:51 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <4DFA0F40.7000204@canonical.com>
At Fri, 17 Jun 2011 14:45:52 +0200,
Lennart Poettering wrote:
>
> On Fri, 17.06.11 14:42, Takashi Iwai (tiwai@suse.de) wrote:
>
> > > If you have the patches ready to convert all drivers which currently can
> > > do jack sensing over to the new scheme with control elements, then we
> > > should probably focus on that for the future, shouldn't we?
> >
> > For PA work, I suppose so (although I won't object to someone working
> > on the implementation with the present input-jack layer :)
> > But you can see David's patch basically independent from PA.
>
> Well, but I'd guess that the access permissions only really matter for
> PA, right?
Not really. For example, I was asked about the input-jack stuff by
a couple of vendors for their own products, apparently running without
PA.
> So if we say that proper jack sensing via control elements is
> the way to go, then PA should adopt this new scheme and this new scheme
> only, and not bother with the input device cruft, and the udev patch
> should not be merged either as PA would be its only consumer...
I would be against the patch if the change were too intrusive,
but this change itself looks small enough to me.
The question whether PA should use it or not is a different thing.
thanks,
Takashi
^ permalink raw reply
* Re: [PATCH] udev: Allow ALSA input jacks to be accessed by the
From: Lennart Poettering @ 2011-06-17 13:00 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <4DFA0F40.7000204@canonical.com>
On Fri, 17.06.11 14:51, Takashi Iwai (tiwai@suse.de) wrote:
>
> At Fri, 17 Jun 2011 14:45:52 +0200,
> Lennart Poettering wrote:
> >
> > On Fri, 17.06.11 14:42, Takashi Iwai (tiwai@suse.de) wrote:
> >
> > > > If you have the patches ready to convert all drivers which currently can
> > > > do jack sensing over to the new scheme with control elements, then we
> > > > should probably focus on that for the future, shouldn't we?
> > >
> > > For PA work, I suppose so (although I won't object to someone working
> > > on the implementation with the present input-jack layer :)
> > > But you can see David's patch basically independent from PA.
> >
> > Well, but I'd guess that the access permissions only really matter for
> > PA, right?
>
> Not really. For example, I was asked about the input-jack stuff by
> a couple of vendors for their own products, apparently running without
> PA.
Well, but the point I am trying to make: the embedded vendors are not
interested in the automatic udev ACL management for the jack sensing
input devices, right?
That means the udev patch is really and only relevant for usage in PA on
desktops.
> > So if we say that proper jack sensing via control elements is
> > the way to go, then PA should adopt this new scheme and this new scheme
> > only, and not bother with the input device cruft, and the udev patch
> > should not be merged either as PA would be its only consumer...
>
> I would be against the patch if the change were too intrusive,
> but this change itself looks small enough to me.
> The question whether PA should use it or not is a different thing.
As I understand Kay he's quite keen on keeping the number of default
udev rules small, especially with stuff that people might then start to
rely on.
Lennart
--
Lennart Poettering - Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH] udev: Allow ALSA input jacks to be accessed by the
From: Kay Sievers @ 2011-06-17 13:05 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <4DFA0F40.7000204@canonical.com>
On Fri, Jun 17, 2011 at 14:51, Takashi Iwai <tiwai@suse.de> wrote:
> At Fri, 17 Jun 2011 14:45:52 +0200,
> Lennart Poettering wrote:
>>
>> On Fri, 17.06.11 14:42, Takashi Iwai (tiwai@suse.de) wrote:
>>
>> > > If you have the patches ready to convert all drivers which currently can
>> > > do jack sensing over to the new scheme with control elements, then we
>> > > should probably focus on that for the future, shouldn't we?
>> >
>> > For PA work, I suppose so (although I won't object to someone working
>> > on the implementation with the present input-jack layer :)
>> > But you can see David's patch basically independent from PA.
>>
>> Well, but I'd guess that the access permissions only really matter for
>> PA, right?
>
> Not really. Â For example, I was asked about the input-jack stuff by
> a couple of vendors for their own products, apparently running without
> PA.
I can't imagine they asked for it because they wanted input devces. I
guess they just asked for jack-events, didn't they?
>> So if we say that proper jack sensing via control elements is
>> the way to go, then PA should adopt this new scheme and this new scheme
>> only, and not bother with the input device cruft, and the udev patch
>> should not be merged either as PA would be its only consumer...
>
> I would be against the patch if the change were too intrusive,
> but this change itself looks small enough to me.
> The question whether PA should use it or not is a different thing.
The thing is that currently PA doesn't use input jack devices, and
honestly I think all the input device misuse must stop. From my point
of view jack senses belong entirely to the alsa device domain and not
to unrelated fake input devices.
So if you can get the control events working in a reasonable time
frame, I would really prefer to add support for that to PA, and
nothing is needed on udev's side.
Kay
^ permalink raw reply
* Re: [PATCH] udev: Allow ALSA input jacks to be accessed by the current user
From: Takashi Iwai @ 2011-06-17 13:10 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <4DFA0F40.7000204@canonical.com>
At Fri, 17 Jun 2011 15:00:58 +0200,
Lennart Poettering wrote:
>
> On Fri, 17.06.11 14:51, Takashi Iwai (tiwai@suse.de) wrote:
>
> >
> > At Fri, 17 Jun 2011 14:45:52 +0200,
> > Lennart Poettering wrote:
> > >
> > > On Fri, 17.06.11 14:42, Takashi Iwai (tiwai@suse.de) wrote:
> > >
> > > > > If you have the patches ready to convert all drivers which currently can
> > > > > do jack sensing over to the new scheme with control elements, then we
> > > > > should probably focus on that for the future, shouldn't we?
> > > >
> > > > For PA work, I suppose so (although I won't object to someone working
> > > > on the implementation with the present input-jack layer :)
> > > > But you can see David's patch basically independent from PA.
> > >
> > > Well, but I'd guess that the access permissions only really matter for
> > > PA, right?
> >
> > Not really. For example, I was asked about the input-jack stuff by
> > a couple of vendors for their own products, apparently running without
> > PA.
>
> Well, but the point I am trying to make: the embedded vendors are not
> interested in the automatic udev ACL management for the jack sensing
> input devices, right?
Ah, no, what I was asked weren't about ASoC but with HD-audio.
They were (so-called) multimedia-center devices, so small-factor PCs
indeed. I don't know whether udev is used, but I won't be surprised
if they do.
Takashi
^ permalink raw reply
* Re: [PATCH] udev: Allow ALSA input jacks to be accessed by the current user
From: Takashi Iwai @ 2011-06-17 13:15 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <4DFA0F40.7000204@canonical.com>
At Fri, 17 Jun 2011 15:05:14 +0200,
Kay Sievers wrote:
>
> On Fri, Jun 17, 2011 at 14:51, Takashi Iwai <tiwai@suse.de> wrote:
> > At Fri, 17 Jun 2011 14:45:52 +0200,
> > Lennart Poettering wrote:
> >>
> >> On Fri, 17.06.11 14:42, Takashi Iwai (tiwai@suse.de) wrote:
> >>
> >> > > If you have the patches ready to convert all drivers which currently can
> >> > > do jack sensing over to the new scheme with control elements, then we
> >> > > should probably focus on that for the future, shouldn't we?
> >> >
> >> > For PA work, I suppose so (although I won't object to someone working
> >> > on the implementation with the present input-jack layer :)
> >> > But you can see David's patch basically independent from PA.
> >>
> >> Well, but I'd guess that the access permissions only really matter for
> >> PA, right?
> >
> > Not really. Â For example, I was asked about the input-jack stuff by
> > a couple of vendors for their own products, apparently running without
> > PA.
>
> I can't imagine they asked for it because they wanted input devces. I
> guess they just asked for jack-events, didn't they?
Right. And the only handy and existing solutions were to use these
input devices.
> >> So if we say that proper jack sensing via control elements is
> >> the way to go, then PA should adopt this new scheme and this new scheme
> >> only, and not bother with the input device cruft, and the udev patch
> >> should not be merged either as PA would be its only consumer...
> >
> > I would be against the patch if the change were too intrusive,
> > but this change itself looks small enough to me.
> > The question whether PA should use it or not is a different thing.
>
> The thing is that currently PA doesn't use input jack devices, and
> honestly I think all the input device misuse must stop. From my point
> of view jack senses belong entirely to the alsa device domain and not
> to unrelated fake input devices.
>
> So if you can get the control events working in a reasonable time
> frame, I would really prefer to add support for that to PA, and
> nothing is needed on udev's side.
OK, it's your call.
thanks,
Takashi
^ permalink raw reply
* Re: [PATCH] udev: Allow ALSA input jacks to be accessed by the
From: Kay Sievers @ 2011-06-17 13:25 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <4DFA0F40.7000204@canonical.com>
On Fri, Jun 17, 2011 at 15:15, Takashi Iwai <tiwai@suse.de> wrote:
> At Fri, 17 Jun 2011 15:05:14 +0200, Kay Sievers wrote:
>> So if you can get the control events working in a reasonable time
>> frame, I would really prefer to add support for that to PA, and
>> nothing is needed on udev's side.
>
> OK, it's your call.
Oh, I think it's actually yours. :)
If you can provide us with the (I think much nicer) ALSA control
events, I would wait for that.
But if you tell us that we can expect that anytime soon, we will
probably need to merge the PA and udev bits (which I think is
sub-optimal).
Thanks,
Kay
^ permalink raw reply
* >=udev-168 race condition
From: Fabio Erculiani @ 2011-06-18 17:12 UTC (permalink / raw)
To: linux-hotplug
82063a88d1fbcceade7e0475ebef2fc69fa5fa87 is the first bad commit
commit 82063a88d1fbcceade7e0475ebef2fc69fa5fa87
Author: Kay Sievers <kay.sievers@vrfy.org>
Date: Mon Apr 18 02:14:24 2011 +0200
udevd: ppoll() -> epoll + signalfd
:100644 100644 c09613313155991315c83aa4db823257c263dd84
80d50860106aa7db13e7e9d1eb156c28938adac7 M TODO
:040000 040000 c92d63f81c3745ca0524c39eecfdcbc5f88a1816
c841597a4a3ca4dc6cdf30eaa86fff7e638de978 M udev
This commit is causing udevadm settle to get stuck while booting off a
Sabayon LiveCD with both 2.6.38/.39 (does it matter?).
The race doesn't happen after install, nor when I enable udev.log-priority=8.
It happens (roughly) with a probability of 0.95.
The error is:
udevadm settle - timeout of 60 seconds reached, the event queue contains:
/sys/devices/virtual/tty/ptys2 (2298)
/sys/devices/virtual/tty/ptyua (2338)
git bisect doesn't lie ;-), of course.
--
Fabio Erculiani
^ permalink raw reply
* Re: udev queue error
From: Kay Sievers @ 2011-06-18 21:41 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <BANLkTim9kJEroUeqpqNoE-tCRvxa5=nghw@mail.gmail.com>
On Wed, 2011-06-15 at 11:40 +0200, Tejun Heo wrote:
> Hello,
>
> On Wed, Jun 15, 2011 at 11:34:23AM +0200, Kay Sievers wrote:
> > Any idea why with the first check GET_EVENT seems, but TUR seems to
> > indicate no change?
>
> That's probably because probing logic already consumed UA. Reset
> during probe also causes UA so detection logic consumes them
> afterwards. I don't think driver can tell apart different UAs at that
> point. If the device determines to report media change after reset
> via GET_EVENT, you get a mismatch. So, it's more or less expected.
Here is a new patch. Would be great, if one of you guys can check if it
fixes the loop we are currently seeing.
Thanks,
Kay
From: Kay Sievers <kay.sievers@vrfy.org>
Subject: sr: check_events() ignore GET_EVENT when TUR says otherwise
Some (broken) devices return GET_EVENT at every open() which
lets udev run into a loop when the device is accessed fron an
event handler.
When GET_EVENT and TUR disagree for a few times in a row,
we ignore the GET_EVENT events, and trust only the TUR status.
This is the log of a USB stick with a (broken) fake CDROM drive:
scsi 5:0:0:0: Direct-Access SanDisk U3 Cruzer Micro 8.02 PQ: 0 ANSI: 0 CCS
sd 5:0:0:0: Attached scsi generic sg3 type 0
scsi 5:0:0:1: CD-ROM SanDisk U3 Cruzer Micro 8.02 PQ: 0 ANSI: 0
sd 5:0:0:0: [sdb] Attached SCSI removable disk
sr2: scsi3-mmc drive: 48x/48x tray
sr 5:0:0:1: Attached scsi CD-ROM sr2
sr 5:0:0:1: Attached scsi generic sg4 type 5
sr2: GET_EVENT and TUR disagree continuously, suppress GET_EVENT events
sd 5:0:0:0: [sdb] 31777279 512-byte logical blocks: (16.2 GB/15.1 GiB)
sd 5:0:0:0: [sdb] No Caching mode page present
sd 5:0:0:0: [sdb] Assuming drive cache: write through
sd 5:0:0:0: [sdb] No Caching mode page present
sd 5:0:0:0: [sdb] Assuming drive cache: write through
sdb: sdb1
Reported-By: Markus Rathgeb maggu2810@googlemail.com
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
---
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 4778e27..1b42dbd 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -229,6 +229,15 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
goto skip_tur;
+ /*
+ * Earlier GET_EVENT_STATUS_NOTIFICATION and TUR did not agree
+ * for a couple of times in a row. We rely on TUR only for this
+ * likely broken device, to prevent generating incorrect media
+ * changed events for every open().
+ */
+ if (cd->ignore_get_event)
+ events &= ~DISK_EVENT_MEDIA_CHANGE;
+
/* let's see whether the media is there with TUR */
last_present = cd->media_present;
ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
@@ -241,8 +250,19 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
cd->media_present = scsi_status_is_good(ret) ||
(scsi_sense_valid(&sshdr) && sshdr.asc != 0x3a);
- if (last_present != cd->media_present)
+ if (last_present != cd->media_present) {
events |= DISK_EVENT_MEDIA_CHANGE;
+ } else if (events & DISK_EVENT_MEDIA_CHANGE) {
+ if (cd->tur_mismatch > 8) {
+ printk("%s: GET_EVENT and TUR disagree continuously, "
+ "suppress GET_EVENT events\n", cd->cdi.name);
+ cd->ignore_get_event = true;
+ } else {
+ cd->tur_mismatch++;
+ }
+ } else if (!cd->ignore_get_event && cd->tur_mismatch > 0) {
+ cd->tur_mismatch = 0;
+ }
skip_tur:
if (cd->device->changed) {
events |= DISK_EVENT_MEDIA_CHANGE;
diff --git a/drivers/scsi/sr.h b/drivers/scsi/sr.h
index e036f1d..94a3215 100644
--- a/drivers/scsi/sr.h
+++ b/drivers/scsi/sr.h
@@ -41,6 +41,8 @@ typedef struct scsi_cd {
unsigned readcd_known:1; /* drive supports READ_CD (0xbe) */
unsigned readcd_cdda:1; /* reading audio data using READ_CD */
unsigned media_present:1; /* media is present */
+ unsigned ignore_get_event:1; /* get_event is unreliable, use TUR */
+ int tur_mismatch; /* nr of get_event TUR mismatches */
struct cdrom_device_info cdi;
/* We hold gendisk and scsi_device references on probe and use
* the refs on this kref to decide when to release them */
^ permalink raw reply related
* pciehp udev remove
From: Carl Karsten @ 2011-06-21 1:20 UTC (permalink / raw)
To: linux-hotplug
Not sure where the right place to post this is. no response on pci
list, so trying here. if this isn't good, suggestion on where next?
guessing udev or 1394.
Laptop: Hewlett-Packard F.0C HP EliteBook 8530w (the one where pciehp
works, no need for acpiphp)
when I remove a firewire card from the EC slot, syslog shows:
[22225.626360] pciehp 0000:00:1c.2:pcie04: Card not present on Slot(2)
[22227.630061] firewire_ohci: failed to write phy reg
[22237.690127] firewire_ohci 0000:04:00.0: PCI INT A disabled
[22237.690137] firewire_ohci: Removed fw-ohci device.
udev events fire at 22227 and 22237. I am wondering why nothing fires
at the first one.
Given the last event takes 12 seconds, and I am not sure what happens
if I insert a card during that time, I want to hear beeps when I
should not insert a card, so I would like a udev rule with
RUN="/usr/bin/screen -S pci_beep -d -m /bin/bash -c 'for x in
{1..12}; do /usr/bin/sox -n -d synth .05 sin 700 gain -16; /bin/sleep
.9; done'"
I would do it on the firewire remove event, but that also fires when
firewire devices are removed (like camera) and I am not smart enough
to keep track of when I should ignore the warning beeps.
--
Carl K
--
Carl K
^ permalink raw reply
* high CPU load when plugging in a flash drive with "fake" integrated
From: Dmitry Savvateev @ 2011-06-27 15:55 UTC (permalink / raw)
To: linux-hotplug
Hello,
I wonder if anyone familiar with udev can help in resolving this
issue: https://bugs.launchpad.net/bugs/780266
Your help would be greatly appreciated.
Thanks in advance
Dmitry Savvateev
^ permalink raw reply
* Re: high CPU load when plugging in a flash drive with "fake"
From: Kay Sievers @ 2011-06-27 16:09 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <BANLkTikEzWXo=893j0niBeNTO7H_A08PAw@mail.gmail.com>
On Mon, Jun 27, 2011 at 17:55, Dmitry Savvateev <dsavvateev@mail.ru> wrote:
> I wonder if anyone familiar with udev can help in resolving this
> issue: https://bugs.launchpad.net/bugs/780266
>
> Your help would be greatly appreciated.
It's a broken device, that tells with every check that the media has
changed. This patch needs to be tested:
http://marc.info/?l=linux-hotplug&m\x130843331916959&w=2
Kay
^ permalink raw reply
* Re: udev queue error
From: Tejun Heo @ 2011-06-28 13:45 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <BANLkTim9kJEroUeqpqNoE-tCRvxa5=nghw@mail.gmail.com>
Hello, Kay.
I've played with the patch and the usb stick and it seems to be
generally working. I've made some changes tho.
* The TUR and GET_EVENT results accumulated between clearing
check_events() should be compared, not the ones from single run.
* TUR reporting media change while GET_EVENT doesn't is okay.
* If ignore_get_event is set, kernel media polling can continue
operating using TUR so that userland doesn't have to worry about
working around.
* Flags moved to a separate bitfield var for proper exclusion.
How does this one look?
Thanks.
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 4778e27..ffb173e 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -221,14 +221,33 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
return 0;
events = sr_get_events(cd->device);
+ cd->get_event_changed |= events & DISK_EVENT_MEDIA_CHANGE;
+
+ /*
+ * If earlier GET_EVENT_STATUS_NOTIFICATION and TUR did not agree
+ * for a couple of times in a row. We rely on TUR only for this
+ * likely broken device, to prevent generating incorrect media
+ * changed events for every open().
+ */
+ if (cd->ignore_get_event) {
+ events &= ~DISK_EVENT_MEDIA_CHANGE;
+ goto do_tur;
+ }
+
/*
* GET_EVENT_STATUS_NOTIFICATION is enough unless MEDIA_CHANGE
* is being cleared. Note that there are devices which hang
* if asked to execute TUR repeatedly.
*/
- if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
- goto skip_tur;
+ if (cd->device->changed) {
+ events |= DISK_EVENT_MEDIA_CHANGE;
+ cd->device->changed = 0;
+ cd->tur_changed = true;
+ }
+ if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
+ return events;
+do_tur:
/* let's see whether the media is there with TUR */
last_present = cd->media_present;
ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
@@ -242,11 +261,27 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
(scsi_sense_valid(&sshdr) && sshdr.asc != 0x3a);
if (last_present != cd->media_present)
- events |= DISK_EVENT_MEDIA_CHANGE;
-skip_tur:
+ cd->device->changed = 1;
+
if (cd->device->changed) {
events |= DISK_EVENT_MEDIA_CHANGE;
cd->device->changed = 0;
+ cd->tur_changed = true;
+ }
+
+ if (!cd->ignore_get_event) {
+ /* check for spurious changed events from GET_EVENT */
+ if (cd->get_event_changed && !cd->tur_changed) {
+ if (cd->tur_mismatch++ > 8) {
+ sdev_printk(KERN_WARNING, cd->device,
+ "GET_EVENT and TUR disagree continuously, suppress GET_EVENT events\n");
+ cd->ignore_get_event = true;
+ }
+ } else {
+ cd->tur_mismatch = 0;
+ }
+ cd->tur_changed = true;
+ cd->get_event_changed = true;
}
return events;
diff --git a/drivers/scsi/sr.h b/drivers/scsi/sr.h
index e036f1d..f8ea01e 100644
--- a/drivers/scsi/sr.h
+++ b/drivers/scsi/sr.h
@@ -41,6 +41,13 @@ typedef struct scsi_cd {
unsigned readcd_known:1; /* drive supports READ_CD (0xbe) */
unsigned readcd_cdda:1; /* reading audio data using READ_CD */
unsigned media_present:1; /* media is present */
+
+ /* GET_EVENT spurious event handling, block guarantees exclusion */
+ int tur_mismatch; /* nr of get_event TUR mismatches */
+ bool tur_changed:1; /* changed according to TUR */
+ bool get_event_changed:1; /* changed according to GET_EVENT */
+ bool ignore_get_event:1; /* get_event is unreliable, use TUR */
+
struct cdrom_device_info cdi;
/* We hold gendisk and scsi_device references on probe and use
* the refs on this kref to decide when to release them */
^ permalink raw reply related
* Re: udev queue error
From: Tejun Heo @ 2011-06-28 15:53 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <BANLkTim9kJEroUeqpqNoE-tCRvxa5=nghw@mail.gmail.com>
Hello, again.
Please test this one. The previous one was broken and I also updated
it such that tur_mismatch is cleared iff !tur_changed &&
!get_event_changed.
Thanks.
---
drivers/scsi/sr.c | 46 ++++++++++++++++++++++++++++++++++++++++++----
drivers/scsi/sr.h | 7 +++++++
2 files changed, 49 insertions(+), 4 deletions(-)
Index: work/drivers/scsi/sr.c
=================================--- work.orig/drivers/scsi/sr.c
+++ work/drivers/scsi/sr.c
@@ -221,14 +221,33 @@ static unsigned int sr_check_events(stru
return 0;
events = sr_get_events(cd->device);
+ cd->get_event_changed |= events & DISK_EVENT_MEDIA_CHANGE;
+
+ /*
+ * If earlier GET_EVENT_STATUS_NOTIFICATION and TUR did not agree
+ * for a couple of times in a row. We rely on TUR only for this
+ * likely broken device, to prevent generating incorrect media
+ * changed events for every open().
+ */
+ if (cd->ignore_get_event) {
+ events &= ~DISK_EVENT_MEDIA_CHANGE;
+ goto do_tur;
+ }
+
/*
* GET_EVENT_STATUS_NOTIFICATION is enough unless MEDIA_CHANGE
* is being cleared. Note that there are devices which hang
* if asked to execute TUR repeatedly.
*/
- if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
- goto skip_tur;
+ if (cd->device->changed) {
+ events |= DISK_EVENT_MEDIA_CHANGE;
+ cd->device->changed = 0;
+ cd->tur_changed = true;
+ }
+ if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
+ return events;
+do_tur:
/* let's see whether the media is there with TUR */
last_present = cd->media_present;
ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
@@ -242,12 +261,31 @@ static unsigned int sr_check_events(stru
(scsi_sense_valid(&sshdr) && sshdr.asc != 0x3a);
if (last_present != cd->media_present)
- events |= DISK_EVENT_MEDIA_CHANGE;
-skip_tur:
+ cd->device->changed = 1;
+
if (cd->device->changed) {
events |= DISK_EVENT_MEDIA_CHANGE;
cd->device->changed = 0;
+ cd->tur_changed = true;
+ }
+
+ if (cd->ignore_get_event)
+ return events;
+
+ /* check for spurious changed events from GET_EVENT */
+ if (!cd->tur_changed) {
+ if (cd->get_event_changed) {
+ if (cd->tur_mismatch++ > 8) {
+ sdev_printk(KERN_WARNING, cd->device,
+ "GET_EVENT and TUR disagree continuously, suppress GET_EVENT events\n");
+ cd->ignore_get_event = true;
+ }
+ } else {
+ cd->tur_mismatch = 0;
+ }
}
+ cd->tur_changed = false;
+ cd->get_event_changed = false;
return events;
}
Index: work/drivers/scsi/sr.h
=================================--- work.orig/drivers/scsi/sr.h
+++ work/drivers/scsi/sr.h
@@ -41,6 +41,13 @@ typedef struct scsi_cd {
unsigned readcd_known:1; /* drive supports READ_CD (0xbe) */
unsigned readcd_cdda:1; /* reading audio data using READ_CD */
unsigned media_present:1; /* media is present */
+
+ /* GET_EVENT spurious event handling, block guarantees exclusion */
+ int tur_mismatch; /* nr of get_event TUR mismatches */
+ bool tur_changed:1; /* changed according to TUR */
+ bool get_event_changed:1; /* changed according to GET_EVENT */
+ bool ignore_get_event:1; /* get_event is unreliable, use TUR */
+
struct cdrom_device_info cdi;
/* We hold gendisk and scsi_device references on probe and use
* the refs on this kref to decide when to release them */
^ permalink raw reply
* Re: udev queue error
From: Tejun Heo @ 2011-06-28 15:57 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <BANLkTim9kJEroUeqpqNoE-tCRvxa5=nghw@mail.gmail.com>
And here's a patch which triggers clearing check on close. It's on
top of the following branch + the previous patch.
git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.git for-linus
With the patch applied, eject triggers MEDIA_CHANGE if the drive had a
media in it.
Thanks.
---
block/genhd.c | 22 ++++++++++++----------
fs/block_dev.c | 17 ++++++++---------
include/linux/genhd.h | 2 +-
3 files changed, 21 insertions(+), 20 deletions(-)
Index: work/block/genhd.c
=================================--- work.orig/block/genhd.c
+++ work/block/genhd.c
@@ -1500,30 +1500,32 @@ void disk_unblock_events(struct gendisk
}
/**
- * disk_check_events - schedule immediate event checking
- * @disk: disk to check events for
+ * disk_flush_events - schedule immediate event checking and flushing
+ * @disk: disk to check and flush events for
+ * @mask: events to flush
*
- * Schedule immediate event checking on @disk if not blocked.
+ * Schedule immediate event checking on @disk if not blocked. Events in
+ * @mask are scheduled to be cleared from the driver. Note that this
+ * doesn't clear the events from @disk->ev.
*
* CONTEXT:
- * Don't care. Safe to call from irq context.
+ * If @mask is non-zero must be called with bdev->bd_mutex held.
*/
-void disk_check_events(struct gendisk *disk)
+void disk_flush_events(struct gendisk *disk, unsigned int mask)
{
struct disk_events *ev = disk->ev;
- unsigned long flags;
if (!ev)
return;
- spin_lock_irqsave(&ev->lock, flags);
+ spin_lock_irq(&ev->lock);
+ ev->clearing |= mask;
if (!ev->block) {
cancel_delayed_work(&ev->dwork);
queue_delayed_work(system_nrt_wq, &ev->dwork, 0);
}
- spin_unlock_irqrestore(&ev->lock, flags);
+ spin_unlock_irq(&ev->lock);
}
-EXPORT_SYMBOL_GPL(disk_check_events);
/**
* disk_clear_events - synchronously check, clear and return pending events
@@ -1713,7 +1715,7 @@ static int disk_events_set_dfl_poll_msec
mutex_lock(&disk_events_mutex);
list_for_each_entry(ev, &disk_events, node)
- disk_check_events(ev->disk);
+ disk_flush_events(ev->disk, 0);
mutex_unlock(&disk_events_mutex);
Index: work/fs/block_dev.c
=================================--- work.orig/fs/block_dev.c
+++ work/fs/block_dev.c
@@ -1447,6 +1447,8 @@ static int __blkdev_put(struct block_dev
int blkdev_put(struct block_device *bdev, fmode_t mode)
{
+ mutex_lock(&bdev->bd_mutex);
+
if (mode & FMODE_EXCL) {
bool bdev_free;
@@ -1455,7 +1457,6 @@ int blkdev_put(struct block_device *bdev
* are protected with bdev_lock. bd_mutex is to
* synchronize disk_holder unlinking.
*/
- mutex_lock(&bdev->bd_mutex);
spin_lock(&bdev_lock);
WARN_ON_ONCE(--bdev->bd_holders < 0);
@@ -1473,17 +1474,15 @@ int blkdev_put(struct block_device *bdev
* If this was the last claim, remove holder link and
* unblock evpoll if it was a write holder.
*/
- if (bdev_free) {
- if (bdev->bd_write_holder) {
- disk_unblock_events(bdev->bd_disk);
- disk_check_events(bdev->bd_disk);
- bdev->bd_write_holder = false;
- }
+ if (bdev_free && bdev->bd_write_holder) {
+ disk_unblock_events(bdev->bd_disk);
+ bdev->bd_write_holder = false;
}
-
- mutex_unlock(&bdev->bd_mutex);
}
+ disk_flush_events(bdev->bd_disk, DISK_EVENT_MEDIA_CHANGE);
+ mutex_unlock(&bdev->bd_mutex);
+
return __blkdev_put(bdev, mode, 0);
}
EXPORT_SYMBOL(blkdev_put);
Index: work/include/linux/genhd.h
=================================--- work.orig/include/linux/genhd.h
+++ work/include/linux/genhd.h
@@ -420,7 +420,7 @@ static inline int get_disk_ro(struct gen
extern void disk_block_events(struct gendisk *disk);
extern void disk_unblock_events(struct gendisk *disk);
-extern void disk_check_events(struct gendisk *disk);
+extern void disk_flush_events(struct gendisk *disk, unsigned int mask);
extern unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask);
/* drivers/char/random.c */
^ permalink raw reply
* Re: udev queue error
From: Kay Sievers @ 2011-06-28 22:20 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <BANLkTim9kJEroUeqpqNoE-tCRvxa5=nghw@mail.gmail.com>
On Tue, Jun 28, 2011 at 17:53, Tejun Heo <htejun@gmail.com> wrote:
> Please test this one. Â The previous one was broken and I also updated
> it such that tur_mismatch is cleared iff !tur_changed &&
> !get_event_changed.
Looks good. Works fine for me.
The firmware in this SANDISK device is so completely broken, that it's
not funny.
Thanks,
Kay
^ permalink raw reply
* Re: udev queue error
From: Kay Sievers @ 2011-06-28 22:21 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <BANLkTim9kJEroUeqpqNoE-tCRvxa5=nghw@mail.gmail.com>
On Tue, Jun 28, 2011 at 17:57, Tejun Heo <htejun@gmail.com> wrote:
> And here's a patch which triggers clearing check on close. It's on
> top of the following branch + the previous patch.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.git for-linus
>
> With the patch applied, eject triggers MEDIA_CHANGE if the drive had a
> media in it.
Seems to works fine for me.
I think that was the last missing piece for the userspace switch-over.
Thanks,
Kay
^ permalink raw reply
* add support for archos 43
From: Kamus @ 2011-06-29 16:22 UTC (permalink / raw)
To: linux-hotplug
I got an archos 43 (with android 'froyo') and I have been able to
temporarily fix this by adding '.isaudio_player'. I obtained this
information from another andoird device; the google nexsus. Based on
what I read on that bug I was supposed to fill out a new bug for this
particular hardware.
libmtp 1.0.6-2 (Ubuntu Natty Narwhal)
lsusb -v output:
Bus 002 Device 003: ID 0e79:1417 Archos, Inc.
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 0 (Defined at Interface level)
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize0 64
idVendor 0x0e79 Archos, Inc.
idProduct 0x1417
bcdDevice 2.16
iManufacturer 1
iProduct 2
iSerial 3
bNumConfigurations 1
Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 32
bNumInterfaces 1
bConfigurationValue 1
iConfiguration 0
bmAttributes 0xa0
(Bus Powered)
Remote Wakeup
MaxPower 500mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 2
bInterfaceClass 8 Mass Storage
bInterfaceSubClass 6 SCSI
bInterfaceProtocol 80 Bulk (Zip)
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x01 EP 1 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 1
can't get device qualifier: Operation not permitted
can't get debug descriptor: Operation not permitted
cannot read device status, Operation not permitted (1)
--
Victor Vargas B.
Latitud: -33.439177,-70.625267
Santiago, Chile.
^ permalink raw reply
* Re: udev queue error
From: Markus Rathgeb @ 2011-06-29 20:54 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <BANLkTim9kJEroUeqpqNoE-tCRvxa5=nghw@mail.gmail.com>
Thanks a lot!
I have to do a deeper look to udev (userspace and kernelspace) to
understand what is going on and what your patch change.
Good work...
2011/6/29 Kay Sievers <kay.sievers@vrfy.org>:
> On Tue, Jun 28, 2011 at 17:57, Tejun Heo <htejun@gmail.com> wrote:
>> And here's a patch which triggers clearing check on close. It's on
>> top of the following branch + the previous patch.
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.git for-linus
>>
>> With the patch applied, eject triggers MEDIA_CHANGE if the drive had a
>> media in it.
>
> Seems to works fine for me.
>
> I think that was the last missing piece for the userspace switch-over.
>
> Thanks,
> Kay
>
^ permalink raw reply
* [PATCH SCSI] sr: check_events() ignore GET_EVENT when TUR says
From: Tejun Heo @ 2011-06-30 13:03 UTC (permalink / raw)
To: James E.J. Bottomley; +Cc: linux-scsi, kay.sievers, maggu2810, linux-hotplug
From: Kay Sievers <kay.sievers@vrfy.org>
Some broken devices indicates that media has changed on every
GET_EVENT_STATUS_NOTIFICATION. This translates into MEDIA_CHANGE
uevent on every open() which lets udev run into a loop.
Verify GET_EVENT result against TUR and if it generates spurious
events for several times in a row, ignore the GET_EVENT events, and
trust only the TUR status.
This is the log of a USB stick with a (broken) fake CDROM drive:
scsi 5:0:0:0: Direct-Access SanDisk U3 Cruzer Micro 8.02 PQ: 0 ANSI: 0 CCS
sd 5:0:0:0: Attached scsi generic sg3 type 0
scsi 5:0:0:1: CD-ROM SanDisk U3 Cruzer Micro 8.02 PQ: 0 ANSI: 0
sd 5:0:0:0: [sdb] Attached SCSI removable disk
sr2: scsi3-mmc drive: 48x/48x tray
sr 5:0:0:1: Attached scsi CD-ROM sr2
sr 5:0:0:1: Attached scsi generic sg4 type 5
sr2: GET_EVENT and TUR disagree continuously, suppress GET_EVENT events
sd 5:0:0:0: [sdb] 31777279 512-byte logical blocks: (16.2 GB/15.1 GiB)
sd 5:0:0:0: [sdb] No Caching mode page present
sd 5:0:0:0: [sdb] Assuming drive cache: write through
sd 5:0:0:0: [sdb] No Caching mode page present
sd 5:0:0:0: [sdb] Assuming drive cache: write through
sdb: sdb1
-tj: Updated to consider only spurious GET_EVENT events among
different types of disagreement and allow using TUR for kernel
event polling after GET_EVENT is ignored.
Reported-By: Markus Rathgeb maggu2810@googlemail.com
Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: stable@kernel.org # >= v2.6.38, fixes udev busy looping w/ certain devices
---
drivers/scsi/sr.c | 46 ++++++++++++++++++++++++++++++++++++++++++----
drivers/scsi/sr.h | 7 +++++++
2 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 4778e27..5fc97d2 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -221,14 +221,33 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
return 0;
events = sr_get_events(cd->device);
+ cd->get_event_changed |= events & DISK_EVENT_MEDIA_CHANGE;
+
+ /*
+ * If earlier GET_EVENT_STATUS_NOTIFICATION and TUR did not agree
+ * for several times in a row. We rely on TUR only for this likely
+ * broken device, to prevent generating incorrect media changed
+ * events for every open().
+ */
+ if (cd->ignore_get_event) {
+ events &= ~DISK_EVENT_MEDIA_CHANGE;
+ goto do_tur;
+ }
+
/*
* GET_EVENT_STATUS_NOTIFICATION is enough unless MEDIA_CHANGE
* is being cleared. Note that there are devices which hang
* if asked to execute TUR repeatedly.
*/
- if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
- goto skip_tur;
+ if (cd->device->changed) {
+ events |= DISK_EVENT_MEDIA_CHANGE;
+ cd->device->changed = 0;
+ cd->tur_changed = true;
+ }
+ if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
+ return events;
+do_tur:
/* let's see whether the media is there with TUR */
last_present = cd->media_present;
ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
@@ -242,12 +261,31 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
(scsi_sense_valid(&sshdr) && sshdr.asc != 0x3a);
if (last_present != cd->media_present)
- events |= DISK_EVENT_MEDIA_CHANGE;
-skip_tur:
+ cd->device->changed = 1;
+
if (cd->device->changed) {
events |= DISK_EVENT_MEDIA_CHANGE;
cd->device->changed = 0;
+ cd->tur_changed = true;
+ }
+
+ if (cd->ignore_get_event)
+ return events;
+
+ /* check whether GET_EVENT is reporting spurious MEDIA_CHANGE */
+ if (!cd->tur_changed) {
+ if (cd->get_event_changed) {
+ if (cd->tur_mismatch++ > 8) {
+ sdev_printk(KERN_WARNING, cd->device,
+ "GET_EVENT and TUR disagree continuously, suppress GET_EVENT events\n");
+ cd->ignore_get_event = true;
+ }
+ } else {
+ cd->tur_mismatch = 0;
+ }
}
+ cd->tur_changed = false;
+ cd->get_event_changed = false;
return events;
}
diff --git a/drivers/scsi/sr.h b/drivers/scsi/sr.h
index e036f1d..37c8f6b 100644
--- a/drivers/scsi/sr.h
+++ b/drivers/scsi/sr.h
@@ -41,6 +41,13 @@ typedef struct scsi_cd {
unsigned readcd_known:1; /* drive supports READ_CD (0xbe) */
unsigned readcd_cdda:1; /* reading audio data using READ_CD */
unsigned media_present:1; /* media is present */
+
+ /* GET_EVENT spurious event handling, blk layer guarantees exclusion */
+ int tur_mismatch; /* nr of get_event TUR mismatches */
+ bool tur_changed:1; /* changed according to TUR */
+ bool get_event_changed:1; /* changed according to GET_EVENT */
+ bool ignore_get_event:1; /* GET_EVENT is unreliable, use TUR */
+
struct cdrom_device_info cdi;
/* We hold gendisk and scsi_device references on probe and use
* the refs on this kref to decide when to release them */
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox