In 3.10-rc2 I see this happening..
[ 72.318133] microcode: CPU1 sig=0x306c3, pf=0x2, revision=0x6
[ 132.446449] microcode: CPU2 sig=0x306c3, pf=0x2, revision=0x6
[ 192.573101] microcode: CPU3 sig=0x306c3, pf=0x2, revision=0x6
[ 252.702055] microcode: Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
For some reason the events for udev seem to be getting delayed 60s for each core.
Dave
On 05/21/2013 04:03 PM, Dave Jones wrote:
> In 3.10-rc2 I see this happening..
>
> [ 72.318133] microcode: CPU1 sig=0x306c3, pf=0x2, revision=0x6
> [ 132.446449] microcode: CPU2 sig=0x306c3, pf=0x2, revision=0x6
> [ 192.573101] microcode: CPU3 sig=0x306c3, pf=0x2, revision=0x6
> [ 252.702055] microcode: Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
>
> For some reason the events for udev seem to be getting delayed 60s for each core.
>
> Dave
>
Is this a regression from -rc1?
-hpa
On Wed, May 22, 2013 at 12:03:04PM -0700, H. Peter Anvin wrote:
> On 05/21/2013 04:03 PM, Dave Jones wrote:
> > In 3.10-rc2 I see this happening..
> >
> > [ 72.318133] microcode: CPU1 sig=0x306c3, pf=0x2, revision=0x6
> > [ 132.446449] microcode: CPU2 sig=0x306c3, pf=0x2, revision=0x6
> > [ 192.573101] microcode: CPU3 sig=0x306c3, pf=0x2, revision=0x6
> > [ 252.702055] microcode: Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
> >
> > For some reason the events for udev seem to be getting delayed 60s for each core.
> >
> > Dave
> >
>
> Is this a regression from -rc1?
No. Looks like it was introduced between 3.9 and 3.10-rc1
I see bisecting in my future.
Dave
A bisect would be appreciated. Cc Fenghua.
Dave Jones <[email protected]> wrote:
>On Wed, May 22, 2013 at 12:03:04PM -0700, H. Peter Anvin wrote:
> > On 05/21/2013 04:03 PM, Dave Jones wrote:
> > > In 3.10-rc2 I see this happening..
> > >
> > > [ 72.318133] microcode: CPU1 sig=0x306c3, pf=0x2, revision=0x6
> > > [ 132.446449] microcode: CPU2 sig=0x306c3, pf=0x2, revision=0x6
> > > [ 192.573101] microcode: CPU3 sig=0x306c3, pf=0x2, revision=0x6
>> > [ 252.702055] microcode: Microcode Update Driver: v2.00
><[email protected]>, Peter Oruba
> > >
>> > For some reason the events for udev seem to be getting delayed 60s
>for each core.
> > >
> > > Dave
> > >
> >
> > Is this a regression from -rc1?
>
>No. Looks like it was introduced between 3.9 and 3.10-rc1
>
>I see bisecting in my future.
>
> Dave
--
Sent from my mobile phone. Please excuse brevity and lack of formatting.
On Wed, May 22, 2013 at 01:09:52PM -0700, H. Peter Anvin wrote:
> Dave Jones <[email protected]> wrote:
>
> >On Wed, May 22, 2013 at 12:03:04PM -0700, H. Peter Anvin wrote:
> > > On 05/21/2013 04:03 PM, Dave Jones wrote:
> > > > In 3.10-rc2 I see this happening..
> > > >
> > > > [ 72.318133] microcode: CPU1 sig=0x306c3, pf=0x2, revision=0x6
> > > > [ 132.446449] microcode: CPU2 sig=0x306c3, pf=0x2, revision=0x6
> > > > [ 192.573101] microcode: CPU3 sig=0x306c3, pf=0x2, revision=0x6
> >> > [ 252.702055] microcode: Microcode Update Driver: v2.00
> ><[email protected]>, Peter Oruba
> > > >
> >> > For some reason the events for udev seem to be getting delayed 60s
> >for each core.
> > > >
> > > > Dave
> > > >
> > >
> > > Is this a regression from -rc1?
> >
> >No. Looks like it was introduced between 3.9 and 3.10-rc1
> >
> >I see bisecting in my future.
> >
> A bisect would be appreciated. Cc Fenghua.
Turns out that I can reproduce this on 3.9 too (and likely earlier) with the right
config file. I'm not sure what option causes the delay yet. Isolating differences
between configs now.
Dave
> On 05/21/2013 04:03 PM, Dave Jones wrote:
>
> [ 72.318133] microcode: CPU1 sig=0x306c3, pf=0x2, revision=0x6
> [ 132.446449] microcode: CPU2 sig=0x306c3, pf=0x2, revision=0x6
> [ 192.573101] microcode: CPU3 sig=0x306c3, pf=0x2, revision=0x6
> [ 252.702055] microcode: Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
>
> For some reason the events for udev seem to be getting delayed 60s
> for each core.
Screwed up my .config, and had CONFIG_FW_LOADER_USER_HELPER inadvertantly set
Odd though that it causes that 60 second delay, given that it's supposedly a
'fallback' when the direct loading fails.
It seems I don't actually need to set that option, so I'm not bothered
if there's an actual bug here or not, but the behaviour seems odd.
Dave
At Wed, 22 May 2013 23:39:11 -0400,
Dave Jones wrote:
>
> > On 05/21/2013 04:03 PM, Dave Jones wrote:
> >
> > [ 72.318133] microcode: CPU1 sig=0x306c3, pf=0x2, revision=0x6
> > [ 132.446449] microcode: CPU2 sig=0x306c3, pf=0x2, revision=0x6
> > [ 192.573101] microcode: CPU3 sig=0x306c3, pf=0x2, revision=0x6
> > [ 252.702055] microcode: Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
> >
> > For some reason the events for udev seem to be getting delayed 60s
> > for each core.
>
> Screwed up my .config, and had CONFIG_FW_LOADER_USER_HELPER inadvertantly set
> Odd though that it causes that 60 second delay, given that it's supposedly a
> 'fallback' when the direct loading fails.
The 60 seconds delay is just because it falls back to usermode
helper, ended in the default timeout. By some reason, either udev
didn't return properly or the firmware loader didn't handle the
response properly.
Might be a regression in the firmware loader part, or might be
user-space side. I'll take a look at this later.
thanks,
Takashi
> It seems I don't actually need to set that option, so I'm not bothered
> if there's an actual bug here or not, but the behaviour seems odd.
>
> Dave
>
On Thu, May 23, 2013 at 11:39 AM, Dave Jones <[email protected]> wrote:
> > On 05/21/2013 04:03 PM, Dave Jones wrote:
> >
> > [ 72.318133] microcode: CPU1 sig=0x306c3, pf=0x2, revision=0x6
> > [ 132.446449] microcode: CPU2 sig=0x306c3, pf=0x2, revision=0x6
> > [ 192.573101] microcode: CPU3 sig=0x306c3, pf=0x2, revision=0x6
> > [ 252.702055] microcode: Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
> >
> > For some reason the events for udev seem to be getting delayed 60s
> > for each core.
>
> Screwed up my .config, and had CONFIG_FW_LOADER_USER_HELPER inadvertantly set
> Odd though that it causes that 60 second delay, given that it's supposedly a
> 'fallback' when the direct loading fails.
udevd has the ugly problem previously at some situations(for example,
request_firmware called in probe(), and that is why direct loading is
introduced),
but not sure why the direct loading is failed first.
Could you enable 'CONFIG_DEBUG_DRIVER' and post 'dmesg' out?
And better to check where the affected firmware(microcode) is in your
distribution.
>
> It seems I don't actually need to set that option, so I'm not bothered
> if there's an actual bug here or not, but the behaviour seems odd.
If the option isn't set, the firmware will be lost for the requested CPU, :-)
Thanks,
--
Ming Lei
At Thu, 23 May 2013 15:45:32 +0800,
Ming Lei wrote:
>
> On Thu, May 23, 2013 at 11:39 AM, Dave Jones <[email protected]> wrote:
> > > On 05/21/2013 04:03 PM, Dave Jones wrote:
> > >
> > > [ 72.318133] microcode: CPU1 sig=0x306c3, pf=0x2, revision=0x6
> > > [ 132.446449] microcode: CPU2 sig=0x306c3, pf=0x2, revision=0x6
> > > [ 192.573101] microcode: CPU3 sig=0x306c3, pf=0x2, revision=0x6
> > > [ 252.702055] microcode: Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
> > >
> > > For some reason the events for udev seem to be getting delayed 60s
> > > for each core.
> >
> > Screwed up my .config, and had CONFIG_FW_LOADER_USER_HELPER inadvertantly set
> > Odd though that it causes that 60 second delay, given that it's supposedly a
> > 'fallback' when the direct loading fails.
>
> udevd has the ugly problem previously at some situations(for example,
> request_firmware called in probe(), and that is why direct loading is
> introduced),
> but not sure why the direct loading is failed first.
The microcode update is optional, so it's no error even if the
microcode firmwares are not found.
But yes, this seems happening during the module probing. The lines
"microcde: CPU..." show before "microcode: Microcode Update
Driver...", which means the f/w loading has been done before finishing
the module load.
I thought (or hoped) this mess (60s stalls) was fixed in the recent
udev, but apparently not...?
> Could you enable 'CONFIG_DEBUG_DRIVER' and post 'dmesg' out?
> And better to check where the affected firmware(microcode) is in your
> distribution.
>
> >
> > It seems I don't actually need to set that option, so I'm not bothered
> > if there's an actual bug here or not, but the behaviour seems odd.
>
> If the option isn't set, the firmware will be lost for the requested CPU, :-)
I don't think it's relevant. If firmware files exist, the microcode
update should have been successful even without usermode helper
kconfig. If not exist, no update -- so should it be.
thanks,
Takashi
At Thu, 23 May 2013 10:06:56 +0200,
Takashi Iwai wrote:
>
> At Thu, 23 May 2013 15:45:32 +0800,
> Ming Lei wrote:
> >
> > On Thu, May 23, 2013 at 11:39 AM, Dave Jones <[email protected]> wrote:
> > > > On 05/21/2013 04:03 PM, Dave Jones wrote:
> > > >
> > > > [ 72.318133] microcode: CPU1 sig=0x306c3, pf=0x2, revision=0x6
> > > > [ 132.446449] microcode: CPU2 sig=0x306c3, pf=0x2, revision=0x6
> > > > [ 192.573101] microcode: CPU3 sig=0x306c3, pf=0x2, revision=0x6
> > > > [ 252.702055] microcode: Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
> > > >
> > > > For some reason the events for udev seem to be getting delayed 60s
> > > > for each core.
> > >
> > > Screwed up my .config, and had CONFIG_FW_LOADER_USER_HELPER inadvertantly set
> > > Odd though that it causes that 60 second delay, given that it's supposedly a
> > > 'fallback' when the direct loading fails.
> >
> > udevd has the ugly problem previously at some situations(for example,
> > request_firmware called in probe(), and that is why direct loading is
> > introduced),
> > but not sure why the direct loading is failed first.
>
> The microcode update is optional, so it's no error even if the
> microcode firmwares are not found.
>
> But yes, this seems happening during the module probing. The lines
> "microcde: CPU..." show before "microcode: Microcode Update
> Driver...", which means the f/w loading has been done before finishing
> the module load.
>
> I thought (or hoped) this mess (60s stalls) was fixed in the recent
> udev, but apparently not...?
Thinking on this again, if the user-space continues to be broken in
that point, we should provide request_firmware() variant without udev,
e.g. request_firmware_direct(), and use it in known places like this?
Takashi
On Thu, May 23, 2013 at 3:46 PM, Takashi Iwai <[email protected]> wrote:
> At Thu, 23 May 2013 10:06:56 +0200,
> Takashi Iwai wrote:
>>
>> At Thu, 23 May 2013 15:45:32 +0800,
>> Ming Lei wrote:
>> >
>> > On Thu, May 23, 2013 at 11:39 AM, Dave Jones <[email protected]> wrote:
>> > > > On 05/21/2013 04:03 PM, Dave Jones wrote:
>> > > >
>> > > > [ 72.318133] microcode: CPU1 sig=0x306c3, pf=0x2, revision=0x6
>> > > > [ 132.446449] microcode: CPU2 sig=0x306c3, pf=0x2, revision=0x6
>> > > > [ 192.573101] microcode: CPU3 sig=0x306c3, pf=0x2, revision=0x6
>> > > > [ 252.702055] microcode: Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
>> > > >
>> > > > For some reason the events for udev seem to be getting delayed 60s
>> > > > for each core.
>> > >
>> > > Screwed up my .config, and had CONFIG_FW_LOADER_USER_HELPER inadvertantly set
>> > > Odd though that it causes that 60 second delay, given that it's supposedly a
>> > > 'fallback' when the direct loading fails.
>> >
>> > udevd has the ugly problem previously at some situations(for example,
>> > request_firmware called in probe(), and that is why direct loading is
>> > introduced),
>> > but not sure why the direct loading is failed first.
>>
>> The microcode update is optional, so it's no error even if the
>> microcode firmwares are not found.
>>
>> But yes, this seems happening during the module probing. The lines
>> "microcde: CPU..." show before "microcode: Microcode Update
>> Driver...", which means the f/w loading has been done before finishing
>> the module load.
>>
>> I thought (or hoped) this mess (60s stalls) was fixed in the recent
>> udev, but apparently not...?
>
> Thinking on this again, if the user-space continues to be broken in
> that point, we should provide request_firmware() variant without udev,
> e.g. request_firmware_direct(), and use it in known places like this?
Is it not already there?I was thinking that some time back this support
was added.
>
>
> Takashi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Thu, May 23, 2013 at 6:16 PM, Takashi Iwai <[email protected]> wrote:
>
> Thinking on this again, if the user-space continues to be broken in
> that point, we should provide request_firmware() variant without udev,
> e.g. request_firmware_direct(), and use it in known places like this?
As anish said, it has been already there, wrt. this problem, I think we
need to know why the direct loading is failed.
Thanks,
--
Ming Lei
At Thu, 23 May 2013 15:48:52 +0530,
anish singh wrote:
>
> On Thu, May 23, 2013 at 3:46 PM, Takashi Iwai <[email protected]> wrote:
> > At Thu, 23 May 2013 10:06:56 +0200,
> > Takashi Iwai wrote:
> >>
> >> At Thu, 23 May 2013 15:45:32 +0800,
> >> Ming Lei wrote:
> >> >
> >> > On Thu, May 23, 2013 at 11:39 AM, Dave Jones <[email protected]> wrote:
> >> > > > On 05/21/2013 04:03 PM, Dave Jones wrote:
> >> > > >
> >> > > > [ 72.318133] microcode: CPU1 sig=0x306c3, pf=0x2, revision=0x6
> >> > > > [ 132.446449] microcode: CPU2 sig=0x306c3, pf=0x2, revision=0x6
> >> > > > [ 192.573101] microcode: CPU3 sig=0x306c3, pf=0x2, revision=0x6
> >> > > > [ 252.702055] microcode: Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
> >> > > >
> >> > > > For some reason the events for udev seem to be getting delayed 60s
> >> > > > for each core.
> >> > >
> >> > > Screwed up my .config, and had CONFIG_FW_LOADER_USER_HELPER inadvertantly set
> >> > > Odd though that it causes that 60 second delay, given that it's supposedly a
> >> > > 'fallback' when the direct loading fails.
> >> >
> >> > udevd has the ugly problem previously at some situations(for example,
> >> > request_firmware called in probe(), and that is why direct loading is
> >> > introduced),
> >> > but not sure why the direct loading is failed first.
> >>
> >> The microcode update is optional, so it's no error even if the
> >> microcode firmwares are not found.
> >>
> >> But yes, this seems happening during the module probing. The lines
> >> "microcde: CPU..." show before "microcode: Microcode Update
> >> Driver...", which means the f/w loading has been done before finishing
> >> the module load.
> >>
> >> I thought (or hoped) this mess (60s stalls) was fixed in the recent
> >> udev, but apparently not...?
> >
> > Thinking on this again, if the user-space continues to be broken in
> > that point, we should provide request_firmware() variant without udev,
> > e.g. request_firmware_direct(), and use it in known places like this?
> Is it not already there?I was thinking that some time back this support
> was added.
My point is the function that *only* uses the direct loading without
fallback to user mode helper.
Takashi
At Thu, 23 May 2013 18:27:57 +0800,
Ming Lei wrote:
>
> On Thu, May 23, 2013 at 6:16 PM, Takashi Iwai <[email protected]> wrote:
> >
> > Thinking on this again, if the user-space continues to be broken in
> > that point, we should provide request_firmware() variant without udev,
> > e.g. request_firmware_direct(), and use it in known places like this?
>
> As anish said, it has been already there,
No, f/w loader always fall back to user mode helper, as long as its
support is built in. And doing that for microcode driver in that code
path isn't only superfluous but also broken due to request_firmware
call in module init.
> wrt. this problem, I think we
> need to know why the direct loading is failed.
The reason is obvious: the requested f/w file doesn't exist.
And it's fine, because the microcode update is an optional operation.
If no f/w file is found, it's not handled as an error. It just means
that no need to update, continuing to work.
Takashi
On Thu, May 23, 2013 at 6:36 PM, Takashi Iwai <[email protected]> wrote:
>
> No, f/w loader always fall back to user mode helper, as long as its
> support is built in. And doing that for microcode driver in that code
> path isn't only superfluous but also broken due to request_firmware
> call in module init.
Firstly, it is not good to do this since some distributions doesn't support
direct loading and doesn't have udevd(such as, android).
Secondly, returning failure from request_firmware_direct() doesn't mean
the firmware doesn't exist since distribution may put the firmware other where.
Anyway, this example is very specific(no firmware can be accepted), and
request_firmware_nowait() should be OK for the situation.
>
>> wrt. this problem, I think we
>> need to know why the direct loading is failed.
>
> The reason is obvious: the requested f/w file doesn't exist.
> And it's fine, because the microcode update is an optional operation.
> If no f/w file is found, it's not handled as an error. It just means
> that no need to update, continuing to work.
OK, as said above, the example is very specific, and might be
workarounded by request_firmware_nowait().
Thanks,
--
Ming Lei
At Thu, 23 May 2013 18:45:29 +0800,
Ming Lei wrote:
>
> On Thu, May 23, 2013 at 6:36 PM, Takashi Iwai <[email protected]> wrote:
> >
> > No, f/w loader always fall back to user mode helper, as long as its
> > support is built in. And doing that for microcode driver in that code
> > path isn't only superfluous but also broken due to request_firmware
> > call in module init.
>
> Firstly, it is not good to do this since some distributions doesn't support
> direct loading and doesn't have udevd(such as, android).
>
> Secondly, returning failure from request_firmware_direct() doesn't mean
> the firmware doesn't exist since distribution may put the firmware other where.
Right, the non-standard path is the problem, and basically the only
problem. The distribution that doesn't support the direct loading
means nothing but that.
> Anyway, this example is very specific(no firmware can be accepted), and
> request_firmware_nowait() should be OK for the situation.
Oh no, rewriting with request_firmware_nowait() should be really the
last choice. It would change the code flow awfully bad in most
cases.
The new kernel driver has a better firmware mechanism. If it's only
the question of paths, we should move on toward that direction and
drop the too complex old way. I'd vote for a warning shown when a
firmware file is loaded via user mode helper (except for explicit
cases like FW_ACTION_NOHOTPLUG), for example.
> >> wrt. this problem, I think we
> >> need to know why the direct loading is failed.
> >
> > The reason is obvious: the requested f/w file doesn't exist.
> > And it's fine, because the microcode update is an optional operation.
> > If no f/w file is found, it's not handled as an error. It just means
> > that no need to update, continuing to work.
>
> OK, as said above, the example is very specific, and might be
> workarounded by request_firmware_nowait().
It's not that easy in this case. The microcode loader driver core
module doesn't invoke request_firmware() directly but it's via cpu
driver. And the same callback is called in different code paths, not
only at init but also via sysfs write. Thus the request_firmware()
call must be synchronous there.
Takashi
On Thu, May 23, 2013 at 8:05 PM, Takashi Iwai <[email protected]> wrote:
> At Thu, 23 May 2013 18:45:29 +0800,
> Ming Lei wrote:
>>
>> On Thu, May 23, 2013 at 6:36 PM, Takashi Iwai <[email protected]> wrote:
>> >
>> > No, f/w loader always fall back to user mode helper, as long as its
>> > support is built in. And doing that for microcode driver in that code
>> > path isn't only superfluous but also broken due to request_firmware
>> > call in module init.
>>
>> Firstly, it is not good to do this since some distributions doesn't support
>> direct loading and doesn't have udevd(such as, android).
>>
>> Secondly, returning failure from request_firmware_direct() doesn't mean
>> the firmware doesn't exist since distribution may put the firmware other where.
>
> Right, the non-standard path is the problem, and basically the only
> problem. The distribution that doesn't support the direct loading
> means nothing but that.
Suppose it is, it is the fact, and it isn't OK to break this distribution.
Also udev supports user-defined rules to load firmware, which
means some drivers may not put their firmware in the default
path of distribution's firmware.
>
>> Anyway, this example is very specific(no firmware can be accepted), and
>> request_firmware_nowait() should be OK for the situation.
>
> Oh no, rewriting with request_firmware_nowait() should be really the
> last choice. It would change the code flow awfully bad in most
> cases.
>
> The new kernel driver has a better firmware mechanism. If it's only
> the question of paths, we should move on toward that direction and
> drop the too complex old way. I'd vote for a warning shown when a
Simply dropping the old way may cause user space regression.
> firmware file is loaded via user mode helper (except for explicit
> cases like FW_ACTION_NOHOTPLUG), for example.
As it is a very driver specific problem, it is better to solve it inside driver.
>
>
>> >> wrt. this problem, I think we
>> >> need to know why the direct loading is failed.
>> >
>> > The reason is obvious: the requested f/w file doesn't exist.
>> > And it's fine, because the microcode update is an optional operation.
>> > If no f/w file is found, it's not handled as an error. It just means
>> > that no need to update, continuing to work.
>>
>> OK, as said above, the example is very specific, and might be
>> workarounded by request_firmware_nowait().
>
> It's not that easy in this case. The microcode loader driver core
> module doesn't invoke request_firmware() directly but it's via cpu
> driver. And the same callback is called in different code paths, not
> only at init but also via sysfs write. Thus the request_firmware()
> call must be synchronous there.
I don't think the way is too difficult to implement. In the path which
requires synchronization, it can be waited on one completion after
calling request_firmware_nowait().
Thanks,
--
Ming Lei
At Thu, 23 May 2013 21:04:53 +0800,
Ming Lei wrote:
>
> On Thu, May 23, 2013 at 8:05 PM, Takashi Iwai <[email protected]> wrote:
> > At Thu, 23 May 2013 18:45:29 +0800,
> > Ming Lei wrote:
> >>
> >> On Thu, May 23, 2013 at 6:36 PM, Takashi Iwai <[email protected]> wrote:
> >> >
> >> > No, f/w loader always fall back to user mode helper, as long as its
> >> > support is built in. And doing that for microcode driver in that code
> >> > path isn't only superfluous but also broken due to request_firmware
> >> > call in module init.
> >>
> >> Firstly, it is not good to do this since some distributions doesn't support
> >> direct loading and doesn't have udevd(such as, android).
> >>
> >> Secondly, returning failure from request_firmware_direct() doesn't mean
> >> the firmware doesn't exist since distribution may put the firmware other where.
> >
> > Right, the non-standard path is the problem, and basically the only
> > problem. The distribution that doesn't support the direct loading
> > means nothing but that.
>
> Suppose it is, it is the fact, and it isn't OK to break this distribution.
>
> Also udev supports user-defined rules to load firmware, which
> means some drivers may not put their firmware in the default
> path of distribution's firmware.
It's why I suggested to put a warning in that path as the first step.
So we can see whether there is any actual user.
> >> Anyway, this example is very specific(no firmware can be accepted), and
> >> request_firmware_nowait() should be OK for the situation.
> >
> > Oh no, rewriting with request_firmware_nowait() should be really the
> > last choice. It would change the code flow awfully bad in most
> > cases.
> >
> > The new kernel driver has a better firmware mechanism. If it's only
> > the question of paths, we should move on toward that direction and
> > drop the too complex old way. I'd vote for a warning shown when a
>
> Simply dropping the old way may cause user space regression.
It's already broken :)
> > firmware file is loaded via user mode helper (except for explicit
> > cases like FW_ACTION_NOHOTPLUG), for example.
>
> As it is a very driver specific problem, it is better to solve it inside driver.
Yes, this proposal is basically not meant as a fix for this particular
issue but rather for future movement in general.
> >> >> wrt. this problem, I think we
> >> >> need to know why the direct loading is failed.
> >> >
> >> > The reason is obvious: the requested f/w file doesn't exist.
> >> > And it's fine, because the microcode update is an optional operation.
> >> > If no f/w file is found, it's not handled as an error. It just means
> >> > that no need to update, continuing to work.
> >>
> >> OK, as said above, the example is very specific, and might be
> >> workarounded by request_firmware_nowait().
> >
> > It's not that easy in this case. The microcode loader driver core
> > module doesn't invoke request_firmware() directly but it's via cpu
> > driver. And the same callback is called in different code paths, not
> > only at init but also via sysfs write. Thus the request_firmware()
> > call must be synchronous there.
>
> I don't think the way is too difficult to implement. In the path which
> requires synchronization, it can be waited on one completion after
> calling request_firmware_nowait().
This sounds already like unnecessary complexity. Also, what if
concurrent accesses?
Also, I wonder why the kernel needs to be "fixed" for this, if the
problem is really the stuck in udev. In this regard, we didn't change
anything from the beginning. There was an implicit "wish", that the
f/w loading shouldn't be done in the module init, but this has been
never treated as a golden rule.
Takashi
On Thu, May 23, 2013 at 9:21 PM, Takashi Iwai <[email protected]> wrote:
> At Thu, 23 May 2013 21:04:53 +0800,
> Ming Lei wrote:
>>
>> On Thu, May 23, 2013 at 8:05 PM, Takashi Iwai <[email protected]> wrote:
>> > At Thu, 23 May 2013 18:45:29 +0800,
>> > Ming Lei wrote:
>> >>
>> >> On Thu, May 23, 2013 at 6:36 PM, Takashi Iwai <[email protected]> wrote:
>> >> >
>> >> > No, f/w loader always fall back to user mode helper, as long as its
>> >> > support is built in. And doing that for microcode driver in that code
>> >> > path isn't only superfluous but also broken due to request_firmware
>> >> > call in module init.
>> >>
>> >> Firstly, it is not good to do this since some distributions doesn't support
>> >> direct loading and doesn't have udevd(such as, android).
>> >>
>> >> Secondly, returning failure from request_firmware_direct() doesn't mean
>> >> the firmware doesn't exist since distribution may put the firmware other where.
>> >
>> > Right, the non-standard path is the problem, and basically the only
>> > problem. The distribution that doesn't support the direct loading
>> > means nothing but that.
>>
>> Suppose it is, it is the fact, and it isn't OK to break this distribution.
>>
>> Also udev supports user-defined rules to load firmware, which
>> means some drivers may not put their firmware in the default
>> path of distribution's firmware.
>
> It's why I suggested to put a warning in that path as the first step.
> So we can see whether there is any actual user.
If you plan to do it, it'd better to add default firmware path of some
distributions into firmware_class.c first, otherwise it may cause
unnecessary noise for this distribution.
But if more default search paths are added, it might cause mistaken
firmwares found under incorrect path, for example, android's
default path is "/etc/firmware" and "/vendor/firmware"(maybe different
for different versions).
Also, putting default search paths into kernel isn't good, which was
introduced unwillingly for well-known reason.
>
>> >> Anyway, this example is very specific(no firmware can be accepted), and
>> >> request_firmware_nowait() should be OK for the situation.
>> >
>> > Oh no, rewriting with request_firmware_nowait() should be really the
>> > last choice. It would change the code flow awfully bad in most
>> > cases.
>> >
>> > The new kernel driver has a better firmware mechanism. If it's only
>> > the question of paths, we should move on toward that direction and
>> > drop the too complex old way. I'd vote for a warning shown when a
>>
>> Simply dropping the old way may cause user space regression.
>
> It's already broken :)
It is different, the current issue is caused by udev, not by kernel, :-)
>
>> > firmware file is loaded via user mode helper (except for explicit
>> > cases like FW_ACTION_NOHOTPLUG), for example.
>>
>> As it is a very driver specific problem, it is better to solve it inside driver.
>
> Yes, this proposal is basically not meant as a fix for this particular
> issue but rather for future movement in general.
>
>> >> >> wrt. this problem, I think we
>> >> >> need to know why the direct loading is failed.
>> >> >
>> >> > The reason is obvious: the requested f/w file doesn't exist.
>> >> > And it's fine, because the microcode update is an optional operation.
>> >> > If no f/w file is found, it's not handled as an error. It just means
>> >> > that no need to update, continuing to work.
>> >>
>> >> OK, as said above, the example is very specific, and might be
>> >> workarounded by request_firmware_nowait().
>> >
>> > It's not that easy in this case. The microcode loader driver core
>> > module doesn't invoke request_firmware() directly but it's via cpu
>> > driver. And the same callback is called in different code paths, not
>> > only at init but also via sysfs write. Thus the request_firmware()
>> > call must be synchronous there.
>>
>> I don't think the way is too difficult to implement. In the path which
>> requires synchronization, it can be waited on one completion after
>> calling request_firmware_nowait().
>
> This sounds already like unnecessary complexity. Also, what if
> concurrent accesses?
The request_firmware_no_wait() supports concurrent accesses on
either same firmware or not.
>
> Also, I wonder why the kernel needs to be "fixed" for this, if the
> problem is really the stuck in udev. In this regard, we didn't change
> anything from the beginning. There was an implicit "wish", that the
> f/w loading shouldn't be done in the module init, but this has been
> never treated as a golden rule.
No, there isn't the golden rule, and it is reasonable or inevitable
sometimes to load firmware in module init, for example, I remember some
wireless dongles in which people can't read its mac address without
downloading firmware, that means some devices may not be initialized
successfully without firmware.
Thanks,
--
Ming Lei
At Thu, 23 May 2013 22:28:51 +0800,
Ming Lei wrote:
>
> On Thu, May 23, 2013 at 9:21 PM, Takashi Iwai <[email protected]> wrote:
> > At Thu, 23 May 2013 21:04:53 +0800,
> > Ming Lei wrote:
> >>
> >> On Thu, May 23, 2013 at 8:05 PM, Takashi Iwai <[email protected]> wrote:
> >> > At Thu, 23 May 2013 18:45:29 +0800,
> >> > Ming Lei wrote:
> >> >>
> >> >> On Thu, May 23, 2013 at 6:36 PM, Takashi Iwai <[email protected]> wrote:
> >> >> >
> >> >> > No, f/w loader always fall back to user mode helper, as long as its
> >> >> > support is built in. And doing that for microcode driver in that code
> >> >> > path isn't only superfluous but also broken due to request_firmware
> >> >> > call in module init.
> >> >>
> >> >> Firstly, it is not good to do this since some distributions doesn't support
> >> >> direct loading and doesn't have udevd(such as, android).
> >> >>
> >> >> Secondly, returning failure from request_firmware_direct() doesn't mean
> >> >> the firmware doesn't exist since distribution may put the firmware other where.
> >> >
> >> > Right, the non-standard path is the problem, and basically the only
> >> > problem. The distribution that doesn't support the direct loading
> >> > means nothing but that.
> >>
> >> Suppose it is, it is the fact, and it isn't OK to break this distribution.
> >>
> >> Also udev supports user-defined rules to load firmware, which
> >> means some drivers may not put their firmware in the default
> >> path of distribution's firmware.
> >
> > It's why I suggested to put a warning in that path as the first step.
> > So we can see whether there is any actual user.
>
> If you plan to do it, it'd better to add default firmware path of some
> distributions into firmware_class.c first, otherwise it may cause
> unnecessary noise for this distribution.
>
> But if more default search paths are added, it might cause mistaken
> firmwares found under incorrect path, for example, android's
> default path is "/etc/firmware" and "/vendor/firmware"(maybe different
> for different versions).
>
> Also, putting default search paths into kernel isn't good, which was
> introduced unwillingly for well-known reason.
Maybe we can create a new Kconfig to specify non-standard firmware
path?
> >> >> Anyway, this example is very specific(no firmware can be accepted), and
> >> >> request_firmware_nowait() should be OK for the situation.
> >> >
> >> > Oh no, rewriting with request_firmware_nowait() should be really the
> >> > last choice. It would change the code flow awfully bad in most
> >> > cases.
> >> >
> >> > The new kernel driver has a better firmware mechanism. If it's only
> >> > the question of paths, we should move on toward that direction and
> >> > drop the too complex old way. I'd vote for a warning shown when a
> >>
> >> Simply dropping the old way may cause user space regression.
> >
> > It's already broken :)
>
> It is different, the current issue is caused by udev, not by kernel, :-)
Yeah :)
> >> > firmware file is loaded via user mode helper (except for explicit
> >> > cases like FW_ACTION_NOHOTPLUG), for example.
> >>
> >> As it is a very driver specific problem, it is better to solve it inside driver.
> >
> > Yes, this proposal is basically not meant as a fix for this particular
> > issue but rather for future movement in general.
> >
> >> >> >> wrt. this problem, I think we
> >> >> >> need to know why the direct loading is failed.
> >> >> >
> >> >> > The reason is obvious: the requested f/w file doesn't exist.
> >> >> > And it's fine, because the microcode update is an optional operation.
> >> >> > If no f/w file is found, it's not handled as an error. It just means
> >> >> > that no need to update, continuing to work.
> >> >>
> >> >> OK, as said above, the example is very specific, and might be
> >> >> workarounded by request_firmware_nowait().
> >> >
> >> > It's not that easy in this case. The microcode loader driver core
> >> > module doesn't invoke request_firmware() directly but it's via cpu
> >> > driver. And the same callback is called in different code paths, not
> >> > only at init but also via sysfs write. Thus the request_firmware()
> >> > call must be synchronous there.
> >>
> >> I don't think the way is too difficult to implement. In the path which
> >> requires synchronization, it can be waited on one completion after
> >> calling request_firmware_nowait().
> >
> > This sounds already like unnecessary complexity. Also, what if
> > concurrent accesses?
>
> The request_firmware_no_wait() supports concurrent accesses on
> either same firmware or not.
Yes, but I meant about the synchronization part. Then you'll need
multiple waiters.
> > Also, I wonder why the kernel needs to be "fixed" for this, if the
> > problem is really the stuck in udev. In this regard, we didn't change
> > anything from the beginning. There was an implicit "wish", that the
> > f/w loading shouldn't be done in the module init, but this has been
> > never treated as a golden rule.
>
> No, there isn't the golden rule, and it is reasonable or inevitable
> sometimes to load firmware in module init, for example, I remember some
> wireless dongles in which people can't read its mac address without
> downloading firmware, that means some devices may not be initialized
> successfully without firmware.
Right.
Takashi
On Thu, May 23, 2013 at 04:36:20PM +0200, Takashi Iwai wrote:
> > >> Also udev supports user-defined rules to load firmware, which
> > >> means some drivers may not put their firmware in the default
> > >> path of distribution's firmware.
> > >
> > > It's why I suggested to put a warning in that path as the first step.
> > > So we can see whether there is any actual user.
> >
> > If you plan to do it, it'd better to add default firmware path of some
> > distributions into firmware_class.c first, otherwise it may cause
> > unnecessary noise for this distribution.
> >
> > But if more default search paths are added, it might cause mistaken
> > firmwares found under incorrect path, for example, android's
> > default path is "/etc/firmware" and "/vendor/firmware"(maybe different
> > for different versions).
> >
> > Also, putting default search paths into kernel isn't good, which was
> > introduced unwillingly for well-known reason.
>
> Maybe we can create a new Kconfig to specify non-standard firmware
> path?
You keep mentioning non-standard paths for firmware, but in this case,
I don't think Fedora is doing anything unusual. We have microcode firmware
in /lib/firmware/intel-ucode/ just like (afaik) everyone else.
What *is* happening, I think is that the CPU is new enough that there's no
newer firmware file available for it.
thoughts?
Dave
At Thu, 23 May 2013 10:48:00 -0400,
Dave Jones wrote:
>
> On Thu, May 23, 2013 at 04:36:20PM +0200, Takashi Iwai wrote:
>
> > > >> Also udev supports user-defined rules to load firmware, which
> > > >> means some drivers may not put their firmware in the default
> > > >> path of distribution's firmware.
> > > >
> > > > It's why I suggested to put a warning in that path as the first step.
> > > > So we can see whether there is any actual user.
> > >
> > > If you plan to do it, it'd better to add default firmware path of some
> > > distributions into firmware_class.c first, otherwise it may cause
> > > unnecessary noise for this distribution.
> > >
> > > But if more default search paths are added, it might cause mistaken
> > > firmwares found under incorrect path, for example, android's
> > > default path is "/etc/firmware" and "/vendor/firmware"(maybe different
> > > for different versions).
> > >
> > > Also, putting default search paths into kernel isn't good, which was
> > > introduced unwillingly for well-known reason.
> >
> > Maybe we can create a new Kconfig to specify non-standard firmware
> > path?
>
> You keep mentioning non-standard paths for firmware, but in this case,
> I don't think Fedora is doing anything unusual. We have microcode firmware
> in /lib/firmware/intel-ucode/ just like (afaik) everyone else.
>
> What *is* happening, I think is that the CPU is new enough that there's no
> newer firmware file available for it.
>
> thoughts?
Yes, in your case, everything is fine in the kernel itself. And no
microcode update is needed for new CPU, thus no firmware.
The problem is that the f/w loader tries to call udev and udev gets
stuck when invoked from module init. This doesn't hit most drivers
because usually the firmware is mandatory and it must exist. Thus the
direct f/w loading always works for them. If it hits, it's only the
error case.
But, for the microcode loader, it's normal that the firmware doesn't
exist, like your case. Unfortunately, this falls back to user helper
mode, and now you're seeing the problem.
So, the option would be to fix udev, let it behaving like before.
The second option would be to change ("fix") the kernel behavior, but
the question is which way.
Takashi
> Dave
>
On Thu, May 23, 2013 at 10:36 PM, Takashi Iwai <[email protected]> wrote:
> At Thu, 23 May 2013 22:28:51 +0800,
> Ming Lei wrote:
>>
>> On Thu, May 23, 2013 at 9:21 PM, Takashi Iwai <[email protected]> wrote:
>> > At Thu, 23 May 2013 21:04:53 +0800,
>> > Ming Lei wrote:
>> >>
>> >> On Thu, May 23, 2013 at 8:05 PM, Takashi Iwai <[email protected]> wrote:
>> >> > At Thu, 23 May 2013 18:45:29 +0800,
>> >> > Ming Lei wrote:
>> >> >>
>> >> >> On Thu, May 23, 2013 at 6:36 PM, Takashi Iwai <[email protected]> wrote:
>> >> >> >
>> >> >> > No, f/w loader always fall back to user mode helper, as long as its
>> >> >> > support is built in. And doing that for microcode driver in that code
>> >> >> > path isn't only superfluous but also broken due to request_firmware
>> >> >> > call in module init.
>> >> >>
>> >> >> Firstly, it is not good to do this since some distributions doesn't support
>> >> >> direct loading and doesn't have udevd(such as, android).
>> >> >>
>> >> >> Secondly, returning failure from request_firmware_direct() doesn't mean
>> >> >> the firmware doesn't exist since distribution may put the firmware other where.
>> >> >
>> >> > Right, the non-standard path is the problem, and basically the only
>> >> > problem. The distribution that doesn't support the direct loading
>> >> > means nothing but that.
>> >>
>> >> Suppose it is, it is the fact, and it isn't OK to break this distribution.
>> >>
>> >> Also udev supports user-defined rules to load firmware, which
>> >> means some drivers may not put their firmware in the default
>> >> path of distribution's firmware.
>> >
>> > It's why I suggested to put a warning in that path as the first step.
>> > So we can see whether there is any actual user.
>>
>> If you plan to do it, it'd better to add default firmware path of some
>> distributions into firmware_class.c first, otherwise it may cause
>> unnecessary noise for this distribution.
>>
>> But if more default search paths are added, it might cause mistaken
>> firmwares found under incorrect path, for example, android's
>> default path is "/etc/firmware" and "/vendor/firmware"(maybe different
>> for different versions).
>>
>> Also, putting default search paths into kernel isn't good, which was
>> introduced unwillingly for well-known reason.
>
> Maybe we can create a new Kconfig to specify non-standard firmware
> path?
Yes.
>
>
>> >> >> Anyway, this example is very specific(no firmware can be accepted), and
>> >> >> request_firmware_nowait() should be OK for the situation.
>> >> >
>> >> > Oh no, rewriting with request_firmware_nowait() should be really the
>> >> > last choice. It would change the code flow awfully bad in most
>> >> > cases.
>> >> >
>> >> > The new kernel driver has a better firmware mechanism. If it's only
>> >> > the question of paths, we should move on toward that direction and
>> >> > drop the too complex old way. I'd vote for a warning shown when a
>> >>
>> >> Simply dropping the old way may cause user space regression.
>> >
>> > It's already broken :)
>>
>> It is different, the current issue is caused by udev, not by kernel, :-)
>
> Yeah :)
>
>
>> >> > firmware file is loaded via user mode helper (except for explicit
>> >> > cases like FW_ACTION_NOHOTPLUG), for example.
>> >>
>> >> As it is a very driver specific problem, it is better to solve it inside driver.
>> >
>> > Yes, this proposal is basically not meant as a fix for this particular
>> > issue but rather for future movement in general.
>> >
>> >> >> >> wrt. this problem, I think we
>> >> >> >> need to know why the direct loading is failed.
>> >> >> >
>> >> >> > The reason is obvious: the requested f/w file doesn't exist.
>> >> >> > And it's fine, because the microcode update is an optional operation.
>> >> >> > If no f/w file is found, it's not handled as an error. It just means
>> >> >> > that no need to update, continuing to work.
>> >> >>
>> >> >> OK, as said above, the example is very specific, and might be
>> >> >> workarounded by request_firmware_nowait().
>> >> >
>> >> > It's not that easy in this case. The microcode loader driver core
>> >> > module doesn't invoke request_firmware() directly but it's via cpu
>> >> > driver. And the same callback is called in different code paths, not
>> >> > only at init but also via sysfs write. Thus the request_firmware()
>> >> > call must be synchronous there.
>> >>
>> >> I don't think the way is too difficult to implement. In the path which
>> >> requires synchronization, it can be waited on one completion after
>> >> calling request_firmware_nowait().
>> >
>> > This sounds already like unnecessary complexity. Also, what if
>> > concurrent accesses?
>>
>> The request_firmware_no_wait() supports concurrent accesses on
>> either same firmware or not.
>
> Yes, but I meant about the synchronization part. Then you'll need
> multiple waiters.
If request_firmware_no_wait() is called on one same firmware image,
one waiter should be OK since the returned data is same for all requests.
>
>> > Also, I wonder why the kernel needs to be "fixed" for this, if the
>> > problem is really the stuck in udev. In this regard, we didn't change
>> > anything from the beginning. There was an implicit "wish", that the
>> > f/w loading shouldn't be done in the module init, but this has been
>> > never treated as a golden rule.
>>
>> No, there isn't the golden rule, and it is reasonable or inevitable
>> sometimes to load firmware in module init, for example, I remember some
>> wireless dongles in which people can't read its mac address without
>> downloading firmware, that means some devices may not be initialized
>> successfully without firmware.
>
> Right.
>
>
> Takashi
Thanks,
--
Ming Lei
On Thu, May 23, 2013 at 11:06 PM, Takashi Iwai <[email protected]> wrote:
> At Thu, 23 May 2013 10:48:00 -0400,
> Dave Jones wrote:
>>
>> On Thu, May 23, 2013 at 04:36:20PM +0200, Takashi Iwai wrote:
>>
>> > > >> Also udev supports user-defined rules to load firmware, which
>> > > >> means some drivers may not put their firmware in the default
>> > > >> path of distribution's firmware.
>> > > >
>> > > > It's why I suggested to put a warning in that path as the first step.
>> > > > So we can see whether there is any actual user.
>> > >
>> > > If you plan to do it, it'd better to add default firmware path of some
>> > > distributions into firmware_class.c first, otherwise it may cause
>> > > unnecessary noise for this distribution.
>> > >
>> > > But if more default search paths are added, it might cause mistaken
>> > > firmwares found under incorrect path, for example, android's
>> > > default path is "/etc/firmware" and "/vendor/firmware"(maybe different
>> > > for different versions).
>> > >
>> > > Also, putting default search paths into kernel isn't good, which was
>> > > introduced unwillingly for well-known reason.
>> >
>> > Maybe we can create a new Kconfig to specify non-standard firmware
>> > path?
>>
>> You keep mentioning non-standard paths for firmware, but in this case,
>> I don't think Fedora is doing anything unusual. We have microcode firmware
>> in /lib/firmware/intel-ucode/ just like (afaik) everyone else.
>>
>> What *is* happening, I think is that the CPU is new enough that there's no
>> newer firmware file available for it.
>>
>> thoughts?
>
> Yes, in your case, everything is fine in the kernel itself. And no
> microcode update is needed for new CPU, thus no firmware.
Can the driver decide if the CPU need microcode? Or there will
be the microcode for the CPU in future?
>
> The problem is that the f/w loader tries to call udev and udev gets
> stuck when invoked from module init. This doesn't hit most drivers
> because usually the firmware is mandatory and it must exist. Thus the
> direct f/w loading always works for them. If it hits, it's only the
> error case.
>
> But, for the microcode loader, it's normal that the firmware doesn't
> exist, like your case. Unfortunately, this falls back to user helper
> mode, and now you're seeing the problem.
The problem is that if driver call request_firmware(), it means it need
the firmware and it know there should be one. So maybe the driver
shouldn't call request_firmware() if it can figure out that case.
> So, the option would be to fix udev, let it behaving like before.
> The second option would be to change ("fix") the kernel behavior, but
> the question is which way.
Sounds like the microcode driver depends on userspace or filesystem
to decide if there is one microcode available for me, is the way correct?
Thanks,
--
Ming Lei
On Thu, May 23, 2013 at 11:27:08PM +0800, Ming Lei wrote:
> > Yes, in your case, everything is fine in the kernel itself. And no
> > microcode update is needed for new CPU, thus no firmware.
>
> Can the driver decide if the CPU need microcode? Or there will
> be the microcode for the CPU in future?
The kernel can't know. The microcode updates are asynchonously released
from the kernel. I might be running a months old kernel, and get a new
microcode tomorrow. The kernel handles this by looking for a file
with a name matching the cpu family/model/stepping. (See how they are
formed in /lib/firmware/intel-ucode/*)
DAve